Summary: | Annotate WTF assertion methods to prevent false-positives from clang static analyzer | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Component: | Web Template Framework | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Status: | RESOLVED DUPLICATE | ||||||
Severity: | Normal | CC: | darin, harrison, mrowe, sam | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Mac | ||||||
OS: | OS X 10.5 | ||||||
URL: | http://clang-analyzer.llvm.org/annotations.html#custom_assertions | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 42796 | ||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2009-06-22 16:15:32 PDT
Would switching to abort() rather than dereferencing a bad address also have the same effect as adding annotations to our own code? Created attachment 31691 [details]
Patch v1
Proposed fix.
(In reply to comment #1) > Would switching to abort() rather than dereferencing a bad address also have > the same effect as adding annotations to our own code? Yes, it has the same effect. Comment on attachment 31691 [details]
Patch v1
Let's look in to a solution using abort() rather than some whacky attributes instead.
(In reply to comment #4) > (From update of attachment 31691 [details] [review]) > Let's look in to a solution using abort() rather than some whacky attributes > instead. This will essentially require disabling -Wmissing-noreturn in WebCore as there are about 71 files that don't compile when we switch CRASH() to use abort() from stdlib.h. Otherwise every function that uses ASSERT_NOT_REACHED, like this one in JSPropertyNameIterator.cpp,: JSValue JSPropertyNameIterator::toPrimitive(ExecState*, PreferredPrimitiveType) const { ASSERT_NOT_REACHED(); return JSValue(); } must be modified thusly (and don't forget the UNUSED_PARAM() macros that must be added if the argument names are present): ASSERT_NO_RETURN JSValue JSPropertyNameIterator::toPrimitive(ExecState*, PreferredPrimitiveType) const { #ifndef NDEBUG ASSERT_NOT_REACHED(); #else return JSValue(); #endif } where "ASSERT_NO_RETURN" is defined in wtf/AlwaysInline.h thusly: #if defined(NDEBUG) #define ASSERT_NO_RETURN #else #define ASSERT_NO_RETURN NO_RETURN #endif I think it's easier to stop using -Wmissing-noreturn (e.g., switch to using -Wno-missing-noreturn) than to add the above markup to every method that uses ASSERT_NOT_REACHED(). (In reply to comment #5) > I think it's easier to stop using -Wmissing-noreturn (e.g., switch to using > -Wno-missing-noreturn) than to add the above markup to every method that uses > ASSERT_NOT_REACHED(). Agreed. (In reply to comment #4) > (From update of attachment 31691 [details]) > Let's look in to a solution using abort() rather than some whacky attributes > instead. Some concern was expressed by David Harrison and Darin Adler about using abort() instead of the current method of dereferencing 0xbbadbeef in that (1) the crash would not occur in the same frame as the ASSERT() or CRASH() macro was declared and that (2) gcc may not preserve stack variables at the actual point of the crash when the frame was changed back to the ASSERT() or CRASH() macro in gdb. Switching to abort() thus requires additional testing on various combinations of operating systems and architectures to make sure that stack variables are preserved when the crash occurs and the frame is moved to where the crash happens (in addition to the changes in Comment #5). Another possibility is to combine the current approach with an abort function call after the code designed to cause a crash. (In reply to comment #9) > I wind up fixing this with bug 66152. > > *** This bug has been marked as a duplicate of bug 66152 *** *wound* |