Last Comment Bug 663442 - Consider redesigning static analysis in the build system
: Consider redesigning static analysis in the build system
Status: NEW
:
Product: Core
Classification: Components
Component: Rewriting and Analysis (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
: Michael Layzell [:mystor] [:mrl]
Mentors:
Depends on:
Blocks: 711241
  Show dependency treegraph
 
Reported: 2011-06-10 10:31 PDT by Joshua Cranmer [:jcranmer]
Modified: 2015-06-03 09:50 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Joshua Cranmer [:jcranmer] 2011-06-10 10:31:05 PDT
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.
Comment 1 Benjamin Smedberg [:bsmedberg] 2011-06-10 10:54:02 PDT
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.
Comment 2 Gregory Szorc [:gps] 2011-12-15 12:47:00 PST
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);
 foo->bar
 ^
 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.

  #ifdef SIMPLE_ASSERTS
  #define PR_ASSERT(expr) assert(expr)
  #elif DEBUG
  ...

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.
Comment 3 Nathan Froyd [:froydnj] 2011-12-15 13:25:54 PST
(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);
>  foo->bar
>  ^
>  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.
Comment 4 (dormant account) 2011-12-15 13:45:29 PST
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
Comment 5 Daniel Veditz [:dveditz] 2011-12-18 11:32:24 PST
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.

Note You need to log in before you can comment on or make changes to this bug.