12:44 <@taras> oh sure
12:44 <@taras> whatever
12:44 <@taras> i was thinking of just making .ii files
12:44 <@taras> stick around
12:44 <@taras> and then running various analyzers on them in parallel
12:44 <@taras> skipping the build system inefficiency
I was thinking of finishing porting prcheck to clang, and then checking this into the codebase as well. If we run things on .ii files after the fact, we probably don't have to worry about compiler incongruencies, so it might be actually possible to run both gcc and clang analyses in the same build.
build system inefficiency? Note that you'd end up doing a lot of the compiling work twice (once for the actual compile, and once for the analysis). And it already can be done in parallel.
But yeah, if we're going to try magic with both GCC and clang, then it might make sense. We need to keep the ability for analyses to turn into compile errors, though.
I wrote up instructions for performing the Clang static analysis at https://developer.mozilla.org/en/Clang_Static_Analysis. You can see sample output at http://people.mozilla.org/~gszorc/clang/.
Unfortunately, as currently written, it hooks itself into the compile phase, so we have that inefficiency. But, before we even worry about efficiencies of running static analysis, we have to address the problem of false positives.
Currently, Clang doesn't grok many of our assert macros, like PR_ASSERT(). You'll see static analysis reports like:
PR_ASSERT(foo != NULL);
Possible NULL deference
If you run a static analysis against our current code base, you get over 2500 reports, over a thousand of these appear to be false-positives due to misunderstood macros.
I think the path of least resistance is to create versions of the macros that are simpler and can be understood by the Clang static analyzer. The simple changes at https://github.com/indygreg/mozilla-central/commit/e627c4cc457576c7872a7713e41aba91dfd3c4a6 seem to clear up many of these false positives.
I'm proposing adding more preprocessor magic to foster static analysis. e.g.
#define PR_ASSERT(expr) assert(expr)
Then, we can hook up configure glue: --enable-static-analysis
Finally, we add build system targets to do the magic (without compilation, if possible).
If nobody objects, I'd like to use this bug as a tracker. I'll need to open separate bugs against NSPR, JS, and the core to get the simpler ASSERT macros defined.
(In reply to Gregory Szorc [:gps] from comment #2)
> Currently, Clang doesn't grok many of our assert macros, like PR_ASSERT().
> You'll see static analysis reports like:
> PR_ASSERT(foo != NULL);
> Possible NULL deference
Could this possibly be because the underlying function guts aren't marked __attribute__((noreturn)? Currently, the static analyzer probably sees two arms of an if producing values; if it knew one arm could never return, then it can infer that to get to foo->bar, foo must not have been NULL.
Is this because clang assumes asserts to be fatal? ie what call NS_ABORT_IF_FALSE? Ie our existing asserts are useless for static analysis because they can be broken without any ill-effects
In a non-debug build our asserts aren't even there; the assert in the example above merely indicates the programmer's belief or desire that foo won't be null. If it asserted in common cases we'd probably notice and fix it, but I certainly don't want an assert to be the only thing standing between us and an edge-case security vulnerability. Not that a null-check assert is interesting in that regard, but some of our other assertions are more meaningful.