Bug 26630 – Annotate WTF assertion methods to prevent false-positives from clang static analyzer

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Web Template Framework

[David Kilzer (:ddkilzer)]

* SUMMARY
When using the clang static analyzer on C and Objective-C source code that uses ASSERT() macros, the following false-positive issue is reported:

source.m:7:5: warning: Called function pointer is a null or undefined pointer value
     ASSERT(ptr);
     ^
Assertions.h:165:9: note: instantiated from:
         CRASH(); \
         ^
Assertions.h:134:5: note: instantiated from:
     ((void(*)())0)(); /* More reliable, but doesn't say BBADBEEF */ \
     ^
1 diagnostic generated.

* RESOLUTION
The fix is to annotate the WTFReport*() methods in Assertions.h with an attribute that tells the clang static analyzer that this method effectively never returns.  That, in turn, prevents the false positive issue in the CRASH() macro from being emitted.  This has the desired effect of squelching the false positive.

* NOTES
See the URL for more details about this custom annotation and how it's used by the clang static analyzer.

[Mark Rowe (bdash)]

Would switching to abort() rather than dereferencing a bad address also have the same effect as adding annotations to our own code?

[David Kilzer (:ddkilzer)]

Created attachment 31691 [details]
Patch v1

Proposed fix.

[David Kilzer (:ddkilzer)]

(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.

[Mark Rowe (bdash)]

Comment on attachment 31691 [details]
Patch v1

Let's look in to a solution using abort() rather than some whacky attributes instead.

[David Kilzer (:ddkilzer)]

(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().

[Mark Rowe (bdash)]

(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.

[David Kilzer (:ddkilzer)]

(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).

[Darin Adler]

Another possibility is to combine the current approach with an abort function call after the code designed to cause a crash.

[Sam Weinig]

I wind up fixing this with bug 66152.

*** This bug has been marked as a duplicate of bug 66152 ***

[Sam Weinig]

(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*