Re: _DIAGASSERT(), noreturn attribute and static analysis

[Nhat Minh Le <nhat.minh.le%huoc.org@localhost>]

At Fri, 14 May 2010 00:32:47 +0000 (UTC),
Valeriy E. Ushakov wrote:
> 
> Nhat Minh Le <nhat.minh.le%huoc.org@localhost> wrote:
> 
> > I've been trying out the clang static analyzer, recently, and as it
> > is, NetBSD assert.h definitions don't play well with static
> > analysis. Basically, the analyzer always predicts the opposite of the
> > assertion predicate whenever it comes across an assert() or
> > _DIAGASSERT() call that is actually compiled (with NDEBUG undefined or
> > with _DIAGNOSTIC defined, respectively), which is pretty bad.
> 
> Do you mean that for assert(foo != NULL); it predicts that
> foo == NULL?  I'm not sure I understand the problem. 

Sort of. Actually, from the outside (I haven't looked at the code),
I'm guessing it goes something like this:

1. The analyzer comes across an assert(foo != NULL) call, which gets
   expanded to something like:

        ((foo != NULL) ? (void)0 : __assert13(...))

2. Since the programmer has written a test for it, the analyzer
   assumes the condition *can* occur.

3. The analyzer sees that this conditional tests whether foo is NULL,
   and since it doesn't know that __assert13() doesn't return, it
   deduces that the branch might be taken and will fallthrough onto
   the rest of the code.

4. Then the analyzer tries out every possible branching scenario up to
   some depth and sees that if the __assert13() route is taken, then
   foo == NULL and the next dereference is a bug.

Now, if we mark the function as dead, then the analyzer will stop
there and that's it, no bug.

And the problem doesn't show either if we don't compile in assertions,
because the analyzer is probably trying to be somewhat lax, so when it
can't prove that foo == NULL, it doesn't say a thing. Some analyzers
go the other way round and will warn you if it can't prove that foo !=
NULL when you're dereferencing it, but that's not the case with the
clang static analyzer, obviously.

> > As far as NetBSD is concerned, it all boils down to the __assert(),
> > __assert13(), __diagassert() and __diagassert13() routines not being
> > declared __dead in assert.h. However, __diagassert() and
> > __diagassert13() are not dead, actually; but they ought to be
> > considered dead as far as analysis is concerned. LLVM has a special
> > attribute for that, it's called analyzer_noreturn.
> >
> > The point of my post is: I think we should be nice to LLVM. :) (And
> > besides, having the static analyzer not spout loads of false positives
> > on NetBSD code without having to use an alternate system header would
> > be nice too!)
> > 
> > My suggestion is to either support analyzer_noreturn through
> > a #define, say __terminal in constrast to __dead, in sys/cdefs.h, the
> 
> __terminal is a very vague and so a very bad name.  I can't think of
> something good, though __undead comes to mind :)

He, __undead is nice; had not really given it any thought, actually,
__terminal just happened to be what I used in my assert.h hack. No
particular reason.

> > same way we support other GCC-specific attributes, or conditionally
> > define __diagassert13() as __dead when, say, __lint__ is defined, and
> > have LLVM -D__lint__ when it does its analysis (which is actually not
> > quite straightforward, with their scripts as they are...).
> 
> It's better to not to drag __lint__ into this, as we do quite a bit of
> other unholy cpp dances for __lint__.

I see... the point was just that AFAIK, this little reasoning should
apply to about all static analyzers out there. Some may handle
assert() specially, but since _DIAGASSERT() is pretty much
NetBSD-specific, there's no chance they'll consider that. So it'd be
nice to be able to turn it on/off at will, somehow.

--
Nhat Minh
minh%NetBSD.org@localhost