On Jul 2, 2011, at 2:23 AM, Jasper van de Gronde wrote:

On 2011-07-02 10:05, Campbell Barton wrote:

...

Hi, just ran scan build with inkscape c/c++ source, using LLVM/CLang from latest SVN

Heres the results, some false positives but looks like some real bugs too.

http://clang.blenderheads.org/inkscape/2011-07-02/

run from r10398

VERY cool! Having looked at some of the reports I see that there might indeed be some actual bugs, but also quite a few possible false positives. Is there any way to communicate certain "preconditions" to the static analyzer? For example, in filters/componenttransfer-funcnode.cpp it reports a dereference of a null pointer, but probably only because it cannot be sure that the pointer is not null, however, as a human I would say that this pointer should not be null, unless there is a bug elsewhere. So would it be possible to give a kind of precondition for this function? So that it doesn't complain inside the function, but rather whenever this function might be called with null pointer?

I think in general it's better to code defensively. As you mention, things could be null if "there is a bug elsewhere". I know that Coverity is very good with this type of reporting, and I've even seen it properly ignore parameter declarations on functions being called when it turned out that null was not a safe value after all.

Now, especially since this is the compiler itself doing checks, I would wager that it is the human reading that is in error. (If not, then it is a bug in their checker, and should be fixed).

I went in to look at the few specific cases here, and yes, as a human one might see the variable get assigned from "SP_FEFUNCNODE()" and say "Aha, I recognize that pattern. That is a GTK+ style type check and conversion macro that will either ensure the GObject pointer is of the correct type or it will emit a warning and do an early return".

However, knowing that humans are fallible (in addition to software) I did a quick check. SP_FEFUNCNODE() is defined in filters/componenttransfer-funcnode.h

Looking into its actual declaration, there is essentially a big if/else chain of type checks (using the '?...:...' conditional operator). And finally I see that the last item in the list is indeed a 'NULL'. So the macro being invoked *can* return a null, and by GTK+ convention being violated we humans have been tricked into allowing dereferencing of a null pointer to creep in.

The proper remediation in this case would most likely be to fix that macro to ensure it results in a return instead of giving NULL, so that it conforms with GKT+ practice. Of course, all usage of that macro should also be checked to ensure that this change will not itself introduce bugs in areas where the returning of NULL was desired.

Oh, and the point of this whole story (aside from getting our bug fixed) is to say that we *really* do not want to suppress warnings such as these. Instead we need to figure them out and get them fixed. And if it turns out to be the tool in error, we can also get that fixed since it is an open source compiler.