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:[email protected]]
<<inline: image002.gif>>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
