nsresult analysis

After I wrote prcheck, I was surprised by the errors it found. I expected to find lots of cases of prbool variables having integers assigned into them. Indeed there were some of those, but the most frequent offenders were things like

NS_ENSURE_SUCCESS(rv,rv);

in methods with a PRBool return value. In this case (and many similarĀ  return values within macros) the function will likely do the opposite of what was intended if there is an error condition. Here is a less hypothetical example in bugzilla.

So I’m thinking that instead of porting the prbool analysis to Treehydra (such that it’d based on a less buggy backend and can be integrated into the build) it might be more interesting to ensure that nsresults do not mix with other integer types. That would catch all of the worst prbool offenders and possibly other nsresult misfortunes.

Has anyone run into bugs like this that do not involve prbools?

I suppose a general solution would be to define a lattice of typedefs with rules specifying which typedefs can be assigned to each other. This would make GCC distinguish certain typedefs as discrete and incompatible types. Thoughts?

4 comments

  1. I know I’ve caught myself using NS_ENSURE_SUCCESS, and returning a PRBool for an nsresult before. Wasn’t caught in review either, but I caught it before checkin.

  2. One of the crufty old files I’ve had to deal with loves to mix PRInt32 and nsresult together.

    Another thing that would be cool–if difficult–would be to have C++ analysis that could warn if you passing something bad to a pseudo-enum for IDL code, like:
    mailnews’s nsMsgSearchCore values.

  3. Layout hackers might note the common-typing of so-called appunits and CSS pixels, which are currently the same integer type and which at times have been used without conversions one way or the other. Note also that these units may eventually be changed to floats (to get auto-saturating behavior when over- and underflows occur), so you’d have to be prepared for that possibility.

  4. One way to build the lattice would be to initially define all typedefs as being incompatible, and then slowly adding permitted rules until a decent ruleset has been built.
    i.e. “not allowed unless permitted”.

    That would also make catching new problems at lot easier, because they’d immediately show up when new typedefs were added.

    But as Joshua noted, this would need an awful lot of cleanup work to be really effective.
    But that cleanup work would probably be worthwhile in it’s own right, even if rather tedious.