Closed Bug 605349 Opened 14 years ago Closed 8 years ago

Use MOZ_WARN_UNUSED_RESULT on SpiderMonkey functions that return bool/JSBool

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1267551

People

(Reporter: Waldo, Unassigned)

Details

Attachments

(1 file)

gcc supports an attribute that will emit a warning if the result of a function call is never used.  I don't remember seeing it anywhere in SpiderMonkey; we should add it where it makes sense, most often for methods that might OOM.  (I originally suggested doing this in bug 602935 comment 8.)

(It's too bad gcc doesn't have something like warn_non_nullchecked_result, which would address many cases the existing attribute won't.  I seem to recall seeing or hearing at some point that WebKit was converting their instances of such into return-bool-and-outparam to permit use of this sort of warning, but it's possible my memory is inaccurate.)
If we don't convert to return-bool-and-outparam won't this attribute be pretty much useless?  It'll detect if we allocate dead space, which seems highly unlikely, but it won't detect a missing NULL check.
Sorry, my wording was imprecise.  If you look at the patch(es) in that bug, you'll see that not all of the bugs fixed there were pointer-returning methods -- things like js::TypedArrayTemplate::copyFrom would be good candidates for using this attribute.  Cutting out the maybe-null-pointer-returning methods reduces the set of possible candidates a lot, but it doesn't empty it.

http://hg.mozilla.org/tracemonkey/annotate/81d0ca612cc8/js/src/jstypedarray.cpp#l1131
Suggest we consider sixgill, which can see a lot farther than gcc.

Not gonna waste cycles and memory bandwidth on one-bit return values and gratuitous out params.

/be
Presumably we'd use inline definitions so as not to waste cycles or memory bandwidth, although I wouldn't be at all surprised if such "waste" were not actually observable in practice.
Sixgill would be good for picking up missing NULL checks, it's already doing this for all the NS_ASSERTION(foo != NULL).  The main thing needed would be to filter reports based on functions we're concerned about (so as to not pick up maybe-NULL reports everywhere in the code base).

The GCC warn_unused_result attribute is best for functions which already return a bool (and maybe some out parameters), which there are plenty of in JS.  Sixgill would not handle this (it checks assertions, not liveness properties), but this is a simple property and GCC's checking should be fine.
Sounds like the consensus is to use warn_unused_result on the functions that already return a bool.
FWIW, my initial patch for bug 618572 had to be backed out due to test failures, caused by my failing to check a bool return value of a function in the scanner.  One of these attributes would have fixed it.

Also, I was just playing with warn_unused_result.  Assigning the result to a variable is enough to silence a warning, even if that variable is never used.  So it's not a superbly strong check, but it's one that is dead easy to add to lots of functions.  SpiderMonkey is littered with functions that return a bool which indicates success/failure, so this definitely seems worthwhile.
(In reply to comment #7)
>
> Assigning the result to a
> variable is enough to silence a warning, even if that variable is never used. 
> So it's not a superbly strong check

Oh, it's better than I thought, because if the variable you assign to is itself not used, GCC will warn about that.  So to circumvent this warning you need to do something like this:

  bool dummy = f();
  (void)dummy;

which is hard to do unintentionally!
mfbt has MOZ_WARN_UNUSED_RESULT for this now, fwiw.
This patch adds MOZ_WARN_UNUSED_RESULT to all the methods in Vector.h that
return a bool.  It quickly becomes evident that our checking of Vector
methods for possible OOMs is lax, *especially* within JaegerMonkey.  The
patch fixes a few of them, but there are close to 100 warnings left, enough
that I'm not sure how to proceed.
Summary: Abstract __attribute__((warn_unused_result)) and use it on methods in SpiderMonkey → Use MOZ_WARN_UNUSED_RESULT on SpiderMonkey functions that return bool/JSBool
BTW, http://sourcefrog.net/weblog/software/aesthetics/interface-levels.html is relevant here -- adding MOZ_WARN_UNUSED_RESULT to a possibly-OOMing function moves it from #6  ("Follow common convention and you'll get it right") to #2 ("Compiler will warn if you get it wrong") on the interface spectrum.
(In reply to Nicholas Nethercote [:njn] from comment #10)
> Created attachment 650795 [details] [diff] [review]
> Use MOZ_WARN_UNUSED_RESULT in Vector.h and fix some callers.
> 
> This patch adds MOZ_WARN_UNUSED_RESULT to all the methods in Vector.h that
> return a bool.  It quickly becomes evident that our checking of Vector
> methods for possible OOMs is lax, *especially* within JaegerMonkey.  The
> patch fixes a few of them, but there are close to 100 warnings left, enough
> that I'm not sure how to proceed.

JM uses CompilerAllocPolicy in most of its vectors, where an allocation failure will set a bit in the compiler (oomInVector) and cause it to fail later.
Assignee: general → nobody
Side note to revive the topic, MSVC seems to have annotations such as _Check_return_ [1], which checks that the return value is used in a conditional, and also _Ret_maybenull_ [2].

Using both should ensure that we check pointers for null checks, at least on Windows builds.

[1] https://msdn.microsoft.com/en-us/library/jj159529.aspx
[2] https://msdn.microsoft.com/en-us/library/hh916382.aspx
I looked into that a little, but it seems like you need to also add an additional compiler flag ("-analyze") so that MSVC actually checks those annotations.
I have a vague recollection of bc doing builds with -analyze, but I could be misremembering.  Still, the payoff seems probably low for widespread use/macroization.  :-\
The action is now happening in bug 1267551.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: