Make use of static-analysis annotations to guide static analyzer

[Quuxplusone]

Bugzilla Link	PR7492
Status	NEW
Importance	P enhancement
Reported by	Peter Gutmann (pgut001@cs.auckland.ac.nz)
Reported on	2010-06-25 06:43:51 -0700
Last modified on	2013-02-15 11:00:40 -0800
Version	unspecified
Hardware	All All
CC	jrose@belkadan.com, llvm-bugs@lists.llvm.org, peter@stairways.com.au, pgut001@cs.auckland.ac.nz, trivial@zoho.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

There are a number of bugs and feature requests that I think could be solved by
a single enhancement involving recognising static-analysis annotations.  For
example bug #2650 "Need a way to mark code to hush-up Static Analyzer for
specific instances" could be resolved through the SAL annotation
__analysis_assume():

__analysis_assume( foo != NULL );

which would guide the analyser by indicating that the pointer at this location
isn't NULL (or, in general, any expression you want).  I'm using SAL in this
case because that's by far the best-developed of the analyser annotation
mechanisms.  To make this portable you'd use:

#ifdef __clang_analyzer__
  #define ANALYSIS_ASSUME( x ) __analysis_assume( x )
#else
  #define ANALYSIS_ASSUME( x )
#endif

Bug 7926 "Disable specific warnings during static analysis" is another bug
that'd be resolved by this.  And 2900.  And 4832.  And possibly 2769.  And I'm
sure there are others, I've stopped looking :-).  Some useful annotations would
be:

__analysis_assume( expr ) - See above.
__nonnull - This function arg should never be called with a NULL pointer (gcc
implements this, but gets it wrong).
__checkreturn - This function return value must always be used (gcc also gets
this one wrong).
__dead( variable ) - This variable is now dead (e.g. after a dealloc function
is called on it), any use of it beyond this point is an error. This is
desperately needed to catch dangling refs, double frees, etc.
__success( value ) - Indicates what the function returns on success.  My
biggest source of false positives with the clang analyzer has been code like:

  status = foo( &val );
  if status == error
    exit;
  do_thing( val );

because the analyzer doesn't know that do_thing() will only get called if
status != error, and so doesn't need to warn about val not being initialised.
So the annotation is:

__success( status >= 0 ) int foo( ... );

and clang would then know that the 'status == error' check would only be passed
if the __success criterion was met, and that val would be initialised at that
point.

I've been working with static code analysis tools for some time, I have a fair
bit of experience with what's useful, and also a long wishlist... I'd really
kill for a __dead() ability for example, no existing analyser that I know of
(Coverity, Fortify, PREfast, Klocwork) has this.

[Quuxplusone]

Following up to my earlier comments, I would really, really kill for the ability to say something like 'success( status >= 0 ) int foo( ... );' as per my earlier post. In terms of false positives I'm now getting more of these than every other clang warning combined (in practice this means that if you clean up all the other warnings, what's left are all these false positives, but still... ). Even just an 'analysis_assume()' would help here if '__success()' is too much work.

[Quuxplusone]

Here is another case where the static analyer doesn't notice the values in an
assert (which should function similarly to Peter's suggested __analysis_assume).

typedef const void* DictionaryKey;

inline Boolean
CFDictionaryGet( CFDictionaryRef ref, DictionaryKey inKey, CFStringRef& result,
CFStringRef def )
{
    result = (CFStringRef) ::CFDictionaryGetValue( ref, inKey );
    if ( !result ) {
        result = def;
    }
    assert( (result != NULL) || (def == NULL) ); // Clearly indicates that result
cannot be NULL if def is not NULL
    return result != NULL;
}

CFStringRef mVariable;

void Test( CFDictionaryRef dictionary )
{
    CFStringRef value;
    CFDictionaryGet( dictionary, CFSTR("Variable"), value, CFSTR("Variable") );
    //  assert( value );
    mVariable = (CFStringRef)CFRetain( value ); // Erroneously detects as value
might be NULL
}

The assert clearly tells the compiler that if def is not null, then result
cannot be null either, but the analyzer persists in reporting a potential use
of NULL in the CFRetain.  Adding the assert( value ) inline in Test resolves
the problem, but it's a shame that the analyser, which traces through the
sequence fails to note the assert.

Adding to Peter's suggestion, it would obviously be beneficial to be able to
specify the post conditions for function.

[Quuxplusone]

The static analyzer /does/ look at assertions, if you are analyzing a debug build. Peter L, in your case the problem is that the analyzer doesn't understand that CFSTR is never null. Please file a separate bug for that.

Having a general annotation system is definitely something we're interested in, but it's a big project and likely won't be shipped in the near future.

[Quuxplusone]

Added bug 15270 http://llvm.org/bugs/show_bug.cgi?id=15270 for CFSTR not being known to be non NULL. Thanks.

[Quuxplusone]

>Having a general annotation system is definitely something we're
>interested in, but it's a big project and likely won't be shipped
>in the near future.

Oh, what a pity, I'd really kill for at least a __nonnull, __checkreturn (both
of which gcc already do, but really badly/incorrectly), and __dead().  Still,
thanks for the all the good work you're doing with clang, I'm not just
complaining :-).

[Quuxplusone]

FWIW, we do recognize GCC's attribute((nonnull)) and attribute((noreturn)), though there are a couple of places where they fall through the cracks. __checkreturn is a good one, though.