[cfe-commits] [Patch] Diagnostic group for the static analyzer : -Werror=static-analyzer

Benoit Belley Benoit.Belley at autodesk.com
Tue Aug 31 05:58:22 PDT 2010


Hi Ted,

Thanks for the feedback. I really appreciate it.

Unfortunately, I am currently busy working on another patch. It will take me a few days to come back to this issue.

Benoit

Le 2010-08-30 à 13:05, Ted Kremenek a écrit :


On Aug 26, 2010, at 9:56 AM, Benoit Belley wrote:

Hi Everyone,

The following patch adds a diagnostic group for the static analyzer, namely -Werror=static-analyzer. The effect of the compiler switch is to cause the static analyzer (i.e. the class BugReporter) to report issues as errors instead of warnings. This can be useful when integrating the static analyzer in a build system since the compiler exit status will be 1 if the compiler report an error.

Hi Benoit,

First, sorry for missing this patch.  If I had seen it sooner I would have gotten back to you earlier.

I think this patch provides a nice clean-up by using a regular diagnostic instead of a custom diagnostic (thus avoiding the escaping of '%' characters), but I don't think it really achieves your intended goal of having the compiler report an error for analyzer diagnostics.  There are a few reasons:

(1) The 'clang' driver operates in either analyzer or compiler mode, but not both at the same time.  Thus analyzer issues can't manifest during regular compilation.

(2) The AnalysisConsumer implicitly turns off the global "-Werror" flag so that emitting a single analyzer diagnostic does not prevent an entire file from being analyzed (the analyzer makes the conservative assumption to only analyze files with no compilation errors).  This has two implications.  First, the overall amalgamation of compiler warnings/errors and analyzer issues won't be as you likely intended (e.g., -Werror will not treat compiler warnings as errors when running in analyzer mode).  Second, if your test case included a second null pointer dereference bug, it wouldn't get emitted since the analyzer would stop analyzing your code after it detected that the frontend issued an error.  You can test this by adding the following code snippet to your test case:

void g2(int *q) {
 if (!q) *q = 0; // expected-error{{null}}
}

If you analyze the test file, all you will see is:

rning-as-errors.c:9:10: warning: incompatible pointer types returning 'int *' from a function with result type 'char *'
 return p; // expected-warning{{incompatible pointer types}}
        ^
warning-as-errors.c:13:11: error: Dereference of null pointer (loaded from variable 'p')
 if (!p) *p = 0; // expected-error{{null}}
         ^~
1 warning and 1 error generated.


Thus only the first analyzer issue gets reported.

(3) Analyzer reports are meant to be separate from compiler diagnostics, because the broader usage model of the analyzer is significantly different than the compiler's.


I'll elaborate on (3), since it's probably the most non-obvious.  Right now we emit analyzer diagnostics to the command line (by getting a custom diagnostic) largely as a tool for testing and validation.  That mode, however, is not how users are intended to consume bug reports from the analyzer.

This patch appears to be the right approach because of how the analyzer is currently run.  Right now the analyzer operates on a single translation unit at a time, which means that it can basically get invoked just like the regular compiler.  However, this is really a consequence of where we are in the analyzer's implementation.  The goal is for the analyzer to support cross-file, inter-procedural, whole program analysis, and thus report bugs that span multiple translation units.  Work is underway on supporting this.  Once this is in place, it will fundamentally change when analyzer diagnostics get emitted.  Instead of analyzer diagnostics being emitted as soon as the analyzer processes a file, they will get delayed until the point where the analyzer has analyzed the whole project in one lump.  At that point, a -Werror diagnostic, which has a standard interpretation as a compiler option to GCC and Clang, wouldn't make much sense.  Instead, at the low level, the analyzer will get invoked in an entirely different way.

This patch also makes sense if the user is directly invoking the clang command to run the analyzer, but that is also not the intended usage model.  Instead, user's should be using scan-build, which is intended to be the entry point for analyzing an entire codebase, i.e.:

$ scan-build <build command>

By default, scan-build's exit code is the same as <build command>, but one can toggle it to report '1' if any bugs were reported:

--status-bugs  - By default, the exit status of scan-build is the same as the
                 executed build command.  Specifying this option causes the
                 exit status of scan-build to be 1 if it found potential bugs
                 and 0 otherwise.

We could add other another variety where scan-build exits with 1 if it found bugs, and return the build command's status otherwise.  This would likely achieve what you were going for.

The reason why scan-build (or its eventual replacement) is intended to be the entry point for analyzing code is because it allows us to change the analysis model at any time.  Users only need to invoke scan-build to "analyze my project" without worrying about the details of how individual files are processed (or when they are processed).  If you look at commercial static analysis tools, e.g., Coverity Prevent, they take a similar tactic because it supports a more general checking model.

That said, I do think that most of your patch adds general goodness.  It provides a nice cleanup (no more escaping of the diagnostic string) and allows our DiagnosticClients to do something a little more intelligent with analyzer issues since they will be bundled under a specific diagnostic.  The only piece I don't think should be added is:

+def StaticAnalyzer : DiagGroup<"static-analyzer">;

Being able to control analyzer diagnostics using a -Werror flag doesn't accomplish your intended goal, and really would just be confusing to users.



Benoit Belley
Sr Principal Developer
M&E-Product Development Group

Autodesk Canada Inc.
10 Rue Duke
Montreal, Quebec  H3C 2L7
Canada

Direct 514 954-7154



[cid:image002.gif at 01CAF376.C0A99470]


-------------- next part --------------
A non-text attachment was scrubbed...
Name: image002.gif
Type: image/gif
Size: 651 bytes
Desc: image002.gif
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100831/7b250ad8/attachment.gif>


More information about the cfe-commits mailing list