[analyzer] Add werror flag for analyzer warnings

[loladiro]

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.

[Szelethus]

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]

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.

[xazax.hun]

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]

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.

In D62885#1530573, @xazax.hun wrote:

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.

[Szelethus]

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 :)

cfe/trunk/include/clang/Driver/CC1Options.td

HelpText<"Display the list of checker and package options meant for "
            "development purposes only">;
 
 def analyzer_config_compatibility_mode : Separate<["-"], "analyzer-config-compatibility-mode">,
   HelpText<"Don't emit errors on invalid analyzer-config inputs">;
 
 def analyzer_config_compatibility_mode_EQ : Joined<["-"], "analyzer-config-compatibility-mode=">,
   Alias<analyzer_config_compatibility_mode>;
 
+def analyzer_werror : Flag<["-"], "analyzer-werror">,
+  HelpText<"Emit analyzer results as errors rather than warnings">;
+
 //===----------------------------------------------------------------------===//
 // Migrator Options
 //===----------------------------------------------------------------------===//
 def migrator_no_nsalloc_error : Flag<["-"], "no-ns-alloc-error">,
   HelpText<"Do not error on use of NSAllocateCollectable/NSReallocateCollectable">;
 
 def migrator_no_finalize_removal : Flag<["-"], "no-finalize-removal">,
   HelpText<"Do not remove finalize method in gc mode">;

cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

public:
   unsigned visualizeExplodedGraphWithGraphViz : 1;
   unsigned UnoptimizedCFG : 1;
   unsigned PrintStats : 1;
 
   /// Do not re-analyze paths leading to exhausted nodes with a different
   /// strategy. We get better code coverage when retry is enabled.
   unsigned NoRetryExhausted : 1;
 
+  /// Emit analyzer warnings as errors.
+  unsigned AnalyzerWerror : 1;
+
   /// The inlining stack depth limit.
   // Cap the stack depth at 4 calls (5 stack frames, base + 4 calls).
   unsigned InlineMaxStackDepth = 5;
 
   /// The mode of function selection used during inlining.
   AnalysisInliningMode InliningMode = NoRedundancy;
 
   // Create a field for each -analyzer-config option.
AnalyzerOptions()
       : DisableAllChecks(false), ShowCheckerHelp(false),
         ShowCheckerHelpAlpha(false), ShowCheckerHelpDeveloper(false),
         ShowCheckerOptionList(false), ShowCheckerOptionAlphaList(false),
         ShowCheckerOptionDeveloperList(false), ShowEnabledCheckerList(false),
         ShowConfigOptionsList(false), AnalyzeAll(false),
         AnalyzerDisplayProgress(false), AnalyzeNestedBlocks(false),
         eagerlyAssumeBinOpBifurcation(false), TrimGraph(false),
         visualizeExplodedGraphWithGraphViz(false), UnoptimizedCFG(false),
-        PrintStats(false), NoRetryExhausted(false) {
+        PrintStats(false), NoRetryExhausted(false), AnalyzerWerror(false) {
     llvm::sort(AnalyzerConfigCmdFlags);
   }
 
   /// Interprets an option's string value as a boolean. The "true" string is
   /// interpreted as true and the "false" string is interpreted as false.
   ///
   /// If an option value is not provided, returns the given \p DefaultVal.
   /// @param [in] CheckerName The *full name* of the checker. One may retrieve

cfe/trunk/lib/Frontend/CompilerInvocation.cpp

Opts.ShouldEmitErrorsOnInvalidConfigValue =
         .Case("false", false)
         .Default(false);
   Opts.DisableAllChecks = Args.hasArg(OPT_analyzer_disable_all_checks);
 
   Opts.visualizeExplodedGraphWithGraphViz =
     Args.hasArg(OPT_analyzer_viz_egraph_graphviz);
   Opts.DumpExplodedGraphTo = Args.getLastArgValue(OPT_analyzer_dump_egraph);
   Opts.NoRetryExhausted = Args.hasArg(OPT_analyzer_disable_retry_exhausted);
+  Opts.AnalyzerWerror = Args.hasArg(OPT_analyzer_werror);
   Opts.AnalyzeAll = Args.hasArg(OPT_analyzer_opt_analyze_headers);
   Opts.AnalyzerDisplayProgress = Args.hasArg(OPT_analyzer_display_progress);
   Opts.AnalyzeNestedBlocks =
     Args.hasArg(OPT_analyzer_opt_analyze_nested_blocks);
   Opts.AnalyzeSpecificFunction = Args.getLastArgValue(OPT_analyze_function);
   Opts.UnoptimizedCFG = Args.hasArg(OPT_analysis_UnoptimizedCFG);
   Opts.TrimGraph = Args.hasArg(OPT_trim_egraph);
   Opts.maxBlockVisitOnPath =

cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

void ento::createTextPathDiagnosticConsumer(AnalyzerOptions &AnalyzerOpts,
                                             const std::string &Prefix,
                                             const clang::Preprocessor &PP) {
   llvm_unreachable("'text' consumer should be enabled on ClangDiags");
 }
 
 namespace {
 class ClangDiagPathDiagConsumer : public PathDiagnosticConsumer {
   DiagnosticsEngine &Diag;
-  bool IncludePath;
+  bool IncludePath, ShouldEmitAsError;
+
 public:
   ClangDiagPathDiagConsumer(DiagnosticsEngine &Diag)
-    : Diag(Diag), IncludePath(false) {}
+      : Diag(Diag), IncludePath(false), ShouldEmitAsError(false) {}
   ~ClangDiagPathDiagConsumer() override {}
   StringRef getName() const override { return "ClangDiags"; }
 
   bool supportsLogicalOpControlFlow() const override { return true; }
   bool supportsCrossFileDiagnostics() const override { return true; }
 
   PathGenerationScheme getGenerationScheme() const override {
     return IncludePath ? Minimal : None;
   }
 
   void enablePaths() {
     IncludePath = true;
   }
 
+  void enableWerror() { ShouldEmitAsError = true; }
+
   void FlushDiagnosticsImpl(std::vector<const PathDiagnostic *> &Diags,
                             FilesMade *filesMade) override {
-    unsigned WarnID = Diag.getCustomDiagID(DiagnosticsEngine::Warning, "%0");
+    unsigned WarnID =
+        ShouldEmitAsError
+            ? Diag.getCustomDiagID(DiagnosticsEngine::Error, "%0")
+            : Diag.getCustomDiagID(DiagnosticsEngine::Warning, "%0");
     unsigned NoteID = Diag.getCustomDiagID(DiagnosticsEngine::Note, "%0");
 
     for (std::vector<const PathDiagnostic*>::iterator I = Diags.begin(),
          E = Diags.end(); I != E; ++I) {
       const PathDiagnostic *PD = *I;
       SourceLocation WarnLoc = PD->getLocation().asLocation();
       Diag.Report(WarnLoc, WarnID) << PD->getShortDescription()
                                    << PD->path.back()->getRanges();
public:
 
   void DigestAnalyzerOptions() {
     if (Opts->AnalysisDiagOpt != PD_NONE) {
       // Create the PathDiagnosticConsumer.
       ClangDiagPathDiagConsumer *clangDiags =
           new ClangDiagPathDiagConsumer(PP.getDiagnostics());
       PathConsumers.push_back(clangDiags);
 
+      if (Opts->AnalyzerWerror)
+        clangDiags->enableWerror();
+
       if (Opts->AnalysisDiagOpt == PD_TEXT) {
         clangDiags->enablePaths();
 
       } else if (!OutDir.empty()) {
         switch (Opts->AnalysisDiagOpt) {
         default:
 #define ANALYSIS_DIAGNOSTICS(NAME, CMDFLAG, DESC, CREATEFN)                    \
   case PD_##NAME:                                                              \
if (VD->hasExternalStorage() || VD->isStaticDataMember()) {
       // Cannot be initialized in another TU.
       return true;
     }
 
     if (VD->getAnyInitializer())
       return true;
 
     llvm::Expected<const VarDecl *> CTUDeclOrError =
-      CTU.getCrossTUDefinition(VD, Opts->CTUDir, Opts->CTUIndexName, 
+      CTU.getCrossTUDefinition(VD, Opts->CTUDir, Opts->CTUIndexName,
                                Opts->DisplayCTUProgress);
 
     if (!CTUDeclOrError) {
       handleAllErrors(CTUDeclOrError.takeError(),
                       [&](const cross_tu::IndexError &IE) {
                         CTU.emitCrossTUDiagnostics(IE);
                       });
     }

cfe/trunk/test/Analysis/override-werror.c

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core -Werror %s -analyzer-store=region -verify
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core -Werror %s -analyzer-store=region -analyzer-werror -verify=werror
 
 // This test case illustrates that using '-analyze' overrides the effect of
 // -Werror.  This allows basic warnings not to interfere with producing
 // analyzer results.
 
-char* f(int *p) { 
-  return p; // expected-warning{{incompatible pointer types}}
+char* f(int *p) {
+  return p; // expected-warning{{incompatible pointer types}} \
+               werror-warning{{incompatible pointer types}}
 }
 
 void g(int *p) {
-  if (!p) *p = 0; // expected-warning{{null}}  
+  if (!p) *p = 0; // expected-warning{{null}} \
+                     werror-error{{null}}
 }