Page MenuHomePhabricator

[analyzer] Add werror flag for analyzer warnings
ClosedPublic

Authored by loladiro on Jun 4 2019, 4:51 PM.

Details

Summary

We're using the clang static analyzer together with a number of
custom analyses in our CI system to ensure that certain invariants
are statiesfied for by the code every commit. Unfortunately, there
currently doesn't seem to be a good way to determine whether any
analyzer warnings were emitted, other than parsing clang's output
(or using scan-build, which then in turn parses clang's output).
As a simpler mechanism, simply add a -analyzer-werror flag to CC1
that causes the analyzer to emit its warnings as errors instead.
I briefly tried to have this be Werror=analyzer and make it go
through that machinery instead, but that seemed more trouble than
it was worth in terms of conflicting with options to the actual build
and special cases that would be required to circumvent the analyzers
usual attempts to quiet non-analyzer warnings. This is simple and it
works well.

Diff Detail

Repository
rL LLVM

Event Timeline

loladiro created this revision.Jun 4 2019, 4:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2019, 4:51 PM
Szelethus added a comment.EditedJun 4 2019, 5:38 PM

Interesting. Let's also have a link to your cfe-dev letter: http://lists.llvm.org/pipermail/cfe-dev/2019-June/062495.html

So, you'd like to make this a frontend flag in order not to expose it to "regular" end users? Or was it because, well, every other flag we have is a frontend flag?

We've recently had a lot of discussion about what features should the user access -- now, we currently discourage anyone from using the analyzer from the command line (as seen by the fact that every configuration flag we have only exposed through the frontend), which at the same time forces anyone who'd like to custom-configure it to be exposed to every frontend flag we have.

This is, in my opinion, somewhat counter intuitive: we use the frontend to hide developer-only flags, yet the expectation that nobody will visit them is unreasonable if we don't provide a subset of end-user facing flags through the driver. This has resulted (or so I've been told) in numerous bugreports from users using experimental features that were known to be not stable.

For a project whose entire foundation is based on an estimation (I mean to refer to false positives), I fear that this flag would be impractical for most users. I certainly wouldn't like to receive an error on a false positive, but I can see that for your specific use, while only specific checkers are enabled, this would be fine. But again, do we really do this flag justice by "simply" making it a frontend flag?

At the end of the day, the idea sounds great, and I'm not opposed to landing this before we resolve this paradox.

test/Analysis/override-werror.c
10 ↗(On Diff #203047)

Why isn't this werror-error?

loladiro marked an inline comment as done.Jun 4 2019, 6:13 PM

So, you'd like to make this a frontend flag in order not to expose it to "regular" end users? Or was it because, well, every other flag we have is a frontend flag?

Little bit of both? We already need to pass a bunch of Xanalyzer and Xclang features in order to set things up correctly and it seemed like a more prominent flag would cause more debate and delay in getting this in ;).

For a project whose entire foundation is based on an estimation (I mean to refer to false positives), I fear that this flag would be impractical for most users. I certainly wouldn't like to receive an error on a false positive, but I can see that for your specific use, while on specific checkers are enabled, this would be fine. But again, do we really do this flag justice by "simply" making it a frontend flag?

Yeah, we're a bit of a special case here in that we consider false positives for our analysis a bug (in the analysis), so Werror makes more sense for us (since the analysis is also in the repo being CI'd).

At the end of the day, the idea sounds great, and I'm not opposed to landing this before resolve this paradox.

test/Analysis/override-werror.c
10 ↗(On Diff #203047)

Unlike the other error (which is generated by the analyzer), this is a semantic error (i.e. would get emitted even in the absence of the analyzer). The -analyzer-werror flag only touches those warnings generated by the analyzer, so this is the desired behavior.

The only problem I see with this approach is that it is an all or nothing thing at the moment. Most of the checks in CSA can have false positives and people usually do not want to fail a build due to a false positive finding. This would force the users to do two separate passes with the static analyzer, one with the checks as errors enabled and one with the rest of the checks. The former run should not include any path sensitive checks as they are never free of false positives (due to the infeasible path problem).

NoQ added a comment.Jun 6 2019, 9:08 PM

Looks great! Feel free to add a Driver flag as well (i.e., --analyzer-werror or something like that) so that not to type -Xclang every time.

The only problem I see with this approach is that it is an all or nothing thing at the moment. Most of the checks in CSA can have false positives and people usually do not want to fail a build due to a false positive finding. This would force the users to do two separate passes with the static analyzer, one with the checks as errors enabled and one with the rest of the checks. The former run should not include any path sensitive checks as they are never free of false positives (due to the infeasible path problem).

It is a well-respected workflow to keep the codebase "analyzer-clean" (completely free of static analyzer warnings) at all costs. If you find true positives important enough, you'll be willing to suppress false positives as long as it allows you to address true positives as soon as they're introduced. It is also much cheaper to suppress false positives as you trigger them than to occasionally dig and triage. And it's a straightforward way to never look at the same false positive twice.

NoQ accepted this revision.Jun 6 2019, 9:08 PM
This revision is now accepted and ready to land.Jun 6 2019, 9:08 PM
Szelethus accepted this revision.Jun 7 2019, 1:35 PM
include/clang/Driver/CC1Options.td
170 ↗(On Diff #203047)

how about "rather than warnings"?

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
86 ↗(On Diff #203047)

I personally dislike boolean variable names that do not start with "is", "has", "should", "was" etc.

test/Analysis/override-werror.c
10 ↗(On Diff #203047)

Ah okay :)

Closed by commit rL362855: [analyzer] Add werror flag for analyzer warnings (authored by kfischer, committed by ). · Explain WhyJun 7 2019, 4:33 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2019, 4:33 PM