663442 - Consider redesigning static analysis in the build system

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Joshua Cranmer [:jcranmer]]

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]

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]]

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]]

(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)]

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]]

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.

Comment 6

[Jonathan Watt [:jwatt]]

Jan, I know you're doing some static analysis stuff with the build system. Maybe you or someone else on this bug can comment about whether this bug should stay open.

Comment 7

[Andi [:andi]]

This is a side note, but what I think we should do is update the clang version that's used to do static-analysis. Currently tooltool uses 3.9 builds with some extra patches applied. This limits is the possibility of adopting more modern ways when doing matching in the ast.

Comment 8

[Andi [:andi]]

Note 2: now static-analysis can also be performed via Mach, that uses a special clang-tidy binary built against our own in-tree checkers, so we don't actually build the tree.
A list of the clang-tidy checkers that is enabled is found here: https://dxr.mozilla.org/mozilla-central/source/tools/clang-tidy/config.yaml

Comment 9

[Jan Keromnes [:janx]]

Closing this 7 year-old bug as it doesn't seem relevant anymore. Please re-open it if you feel otherwise.