Closed
Bug 605349
Opened 14 years ago
Closed 9 years ago
Use MOZ_WARN_UNUSED_RESULT on SpiderMonkey functions that return bool/JSBool
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
DUPLICATE
of bug 1267551
People
(Reporter: Waldo, Unassigned)
Details
Attachments
(1 file)
(deleted),
patch
|
Details | Diff | Splinter Review |
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.)
![]() |
||
Comment 1•14 years ago
|
||
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.
Reporter | ||
Comment 2•14 years ago
|
||
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
Comment 3•14 years ago
|
||
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
Reporter | ||
Comment 4•14 years ago
|
||
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.
Comment 5•14 years ago
|
||
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.
![]() |
||
Comment 6•14 years ago
|
||
Sounds like the consensus is to use warn_unused_result on the functions that already return a bool.
![]() |
||
Comment 7•14 years ago
|
||
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.
![]() |
||
Comment 8•14 years ago
|
||
(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!
![]() |
||
Comment 10•12 years ago
|
||
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.
![]() |
||
Updated•12 years ago
|
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
![]() |
||
Comment 11•12 years ago
|
||
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.
Comment 12•12 years ago
|
||
(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 | ||
Updated•10 years ago
|
Assignee: general → nobody
Comment 13•9 years ago
|
||
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
Comment 14•9 years ago
|
||
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.
Reporter | ||
Comment 15•9 years ago
|
||
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. :-\
![]() |
||
Comment 16•9 years ago
|
||
The action is now happening in bug 1267551.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•