[analyzer] Do not analyze bison-generated files

[george.karpenkov]

Summary

Bison/YACC generated files result in a very large number of (presumably) false positives from the analyzer.
These false positives are "true" in a sense of the information analyzer sees: assuming that the lexer can return any token at any point a number of uninitialized reads does occur.
(naturally, the analyzer can not capture a complex invariant that certain tokens can only occur under certain conditions).

Current fix simply stops analysis on those files.
I have examined a very large number of such auto-generated files, and they do all start with such a comment.
Conversely, user code is very unlikely to contain such a comment.

rdar://33608161

[a.sidorin]

Looks good, just a nit inline.

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

522

Could we use Buffer->getBuffer().startswith(...) instead?

[george.karpenkov]

Actually, after posting this I've realized that this behavior is somewhat inconsistent, and we should still produce an empty plist when requested to: just as we do for files with no generated reports.

[george.karpenkov]

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

522

we could if there was such a method? getBuffer() returns a MutableArrayRef

[george.karpenkov]

Actually, after posting this I've realized that this behavior is somewhat inconsistent, and we should still produce an empty plist when requested to: just as we do for files with no generated reports.

Actually, after thinking about this some more, the answer is those plists should not be produced, as they are not produced in similar cases (disableAllChecks is set, etc).

[NoQ]

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

522

Hmm okay. So is the buffer actually null-terminated?

[dcoughlin]

In D43421#1018270, @george.karpenkov wrote:

Actually, after posting this I've realized that this behavior is somewhat inconsistent, and we should still produce an empty plist when requested to: just as we do for files with no generated reports.

Actually, after thinking about this some more, the answer is those plists should not be produced, as they are not produced in similar cases (disableAllChecks is set, etc).

This seems to be about the contract that the analyzer promises to tools that ingest the plists. Will those tools be OK with missing plists? What do they expect?

[a.sidorin]

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

522

Looks like this method of the base class is overridden with different return result. However, we are still allowed to use parent class methods:

llvm::WritableMemoryBuffer *WBuf;
llvm::MemoryBuffer *MBuf;
StringRef Str = MBuf->MemoryBuffer::getBuffer();
Str = WBuf->MemoryBuffer::getBuffer();

[george.karpenkov]

Always produces plists now, less brittle string check.

[NoQ]

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

585

It seems that previously it didn't do the timer stop thing and whatever is below when analysis is skipped, now it does. Is this an intended or an accidental change?

[george.karpenkov]

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

585

Intended, as "whatever is below" is the code producing the plist.

[NoQ]

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

585

I mean, is it intended even in the Opts->DisableAllChecks mode?

585

Emm, wait, there's another return above. So that's dead code.

[george.karpenkov]

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

585

I mean, is it intended even in the Opts->DisableAllChecks mode?

Yes, to me it seems logical to have less code paths and to always perform same wrapping, lest the clients expect that.

[NoQ]

Yeah, looks correct. And they'd have to deal with some of these empty plists anyway now.

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

#include "clang/StaticAnalyzer/Core/Analyses.def"
   }
 
 private:
   void storeTopLevelDecls(DeclGroupRef DG);
   std::string getFunctionName(const Decl *D);
 
   /// \brief Check if we should skip (not analyze) the given function.
   AnalysisMode getModeForDecl(Decl *D, AnalysisMode Mode);
+  void runAnalysisOnTranslationUnit(ASTContext &C);
 
+  /// Print \p S to stderr if \c Opts->AnalyzerDisplayProgress is set.
+  void reportAnalyzerProgress(StringRef S);
 };
 } // end anonymous namespace
 
 
 //===----------------------------------------------------------------------===//
 // AnalysisConsumer implementation.
 //===----------------------------------------------------------------------===//
 bool AnalysisConsumer::HandleTopLevelDecl(DeclGroupRef DG) {
for (const Decl *Callee : VisitedCallees)
       // Decls from CallGraph are already canonical. But Decls coming from
       // CallExprs may be not. We should canonicalize them manually.
       Visited.insert(isa<ObjCMethodDecl>(Callee) ? Callee
                                                  : Callee->getCanonicalDecl());
     VisitedAsTopLevel.insert(D);
   }
 }
 
-void AnalysisConsumer::HandleTranslationUnit(ASTContext &C) {
-  // Don't run the actions if an error has occurred with parsing the file.
-  DiagnosticsEngine &Diags = PP.getDiagnostics();
-  if (Diags.hasErrorOccurred() || Diags.hasFatalErrorOccurred())
-    return;
-
-  // Don't analyze if the user explicitly asked for no checks to be performed
-  // on this file.
-  if (Opts->DisableAllChecks)
-    return;
-
-  {
-    if (TUTotalTimer) TUTotalTimer->startTimer();
+static bool isBisonFile(ASTContext &C) {
+  const SourceManager &SM = C.getSourceManager();
+  FileID FID = SM.getMainFileID();
+  StringRef Buffer = SM.getBuffer(FID)->getBuffer();
+  if (Buffer.startswith("/* A Bison parser, made by"))
+    return true;
+  return false;
+}
 
-    // Introduce a scope to destroy BR before Mgr.
-    BugReporter BR(*Mgr);
-    TranslationUnitDecl *TU = C.getTranslationUnitDecl();
-    checkerMgr->runCheckersOnASTDecl(TU, *Mgr, BR);
+void AnalysisConsumer::runAnalysisOnTranslationUnit(ASTContext &C) {
 
-    // Run the AST-only checks using the order in which functions are defined.
-    // If inlining is not turned on, use the simplest function order for path
-    // sensitive analyzes as well.
-    RecVisitorMode = AM_Syntax;
-    if (!Mgr->shouldInlineCall())
-      RecVisitorMode |= AM_Path;
-    RecVisitorBR = &BR;
 
-    // Process all the top level declarations.
-    //
-    // Note: TraverseDecl may modify LocalTUDecls, but only by appending more
-    // entries.  Thus we don't use an iterator, but rely on LocalTUDecls
-    // random access.  By doing so, we automatically compensate for iterators
-    // possibly being invalidated, although this is a bit slower.
-    const unsigned LocalTUDeclsSize = LocalTUDecls.size();
-    for (unsigned i = 0 ; i < LocalTUDeclsSize ; ++i) {
-      TraverseDecl(LocalTUDecls[i]);
-    }
 
-    if (Mgr->shouldInlineCall())
-      HandleDeclsCallGraph(LocalTUDeclsSize);
 
-    // After all decls handled, run checkers on the entire TranslationUnit.
-    checkerMgr->runCheckersOnEndOfTranslationUnit(TU, *Mgr, BR);
 
-    RecVisitorBR = nullptr;
-  }
 
+void AnalysisConsumer::reportAnalyzerProgress(StringRef S) {
+  if (Opts->AnalyzerDisplayProgress)
+    llvm::errs() << S;
+}
+
+void AnalysisConsumer::HandleTranslationUnit(ASTContext &C) {
+
+  // Don't run the actions if an error has occurred with parsing the file.
+  DiagnosticsEngine &Diags = PP.getDiagnostics();
+  if (Diags.hasErrorOccurred() || Diags.hasFatalErrorOccurred())
+    return;
+
+  if (TUTotalTimer) TUTotalTimer->startTimer();
+
+  if (isBisonFile(C)) {
+    reportAnalyzerProgress("Skipping bison-generated file\n");
+  } else if (Opts->DisableAllChecks) {
+
+    // Don't analyze if the user explicitly asked for no checks to be performed
+    // on this file.
+    reportAnalyzerProgress("All checks are disabled using a supplied option\n");
+  } else {
+    // Otherwise, just run the analysis.
+    runAnalysisOnTranslationUnit(C);
+  }
+
   if (TUTotalTimer) TUTotalTimer->stopTimer();
 
   // Count how many basic blocks we have not covered.
   NumBlocksInAnalyzedFunctions = FunctionSummaries.getTotalNumBasicBlocks();
   NumVisitedBlocksInAnalyzedFunctions =
       FunctionSummaries.getTotalNumVisitedBasicBlocks();
   if (NumBlocksInAnalyzedFunctions > 0)
     PercentReachableBlocks =
       (FunctionSummaries.getTotalNumVisitedBasicBlocks() * 100) /

test/Analysis/yaccignore.c

+/* A Bison parser, made by GNU Bison 1.875.  */
+
+// RUN: rm -rf %t.plist
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-output=plist -o %t.plist -verify %s
+// RUN: FileCheck --input-file=%t.plist %s
+
+// expected-no-diagnostics
+int foo() {
+  int *x = 0;
+  return *x; // no-warning
+}
+
+// CHECK:   <key>diagnostics</key>