[analyzer] Serialize statistics to plist when serialize-stats=true is set

[george.karpenkov]

Summary

The change enables tracking statistics across revisions.

[george.karpenkov]

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

191

Raw pointers are annoying, and it made no sense to me that the timer is statistic, yet is always initialized in constructor.

207

Having a group gives nicer statistics output, which explicitly states that this is an analyzer timer (as opposed to timer.misc.time)

216

I hate doing stuff in destructors, so I would much rather set PrintOnExit back to true, and remove the destructor altogether, but I was worried of making this CL too large.

576

Moving reset down is required to make sure the timer and extra statistics are available. Again, I would have liked to explicitly call flush or similar instead of relying on destructor, but it seemed out of place in this CL.

[george.karpenkov]

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

493

I'm split between just adding it in an extra "statistics" field and having it as a comment. I would prefer the former, but I was afraid some random parser somewhere might choke.

[xazax.hun]

Why not turn this on by default when we turn on reporting statistics?
Having a test for the plist output would be nice.

[NoQ]

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

493

I suspect that any "normal" parser (libplist of some sort) would accept both and any "dumb" parser (that parses the plist line-by-line with regexps and expect exact these lines) would choke on any of those with roughly the same probability. So i'm in favor of adding a field - at least those are being added more often, and dumb parser maintainers might be used to it by now.

[NoQ]

So are these present only on translation units that actually caused bug reports to appear?

Having a test for the plist output would be nice.

I suspect such test would be super fragile. Every time we change any modeling, many of the stats may change.

[xazax.hun]

In D43131#1003607, @NoQ wrote:

So are these present only on translation units that actually caused bug reports to appear?

Having a test for the plist output would be nice.

I suspect such test would be super fragile. Every time we change any modeling, many of the stats may change.

Using regex we could omit the actual numbers from the output checking.

[NoQ]

In D43131#1003615, @xazax.hun wrote:

In D43131#1003607, @NoQ wrote:

So are these present only on translation units that actually caused bug reports to appear?

Having a test for the plist output would be nice.

I suspect such test would be super fragile. Every time we change any modeling, many of the stats may change.

Using regex we could omit the actual numbers from the output checking.

Hmm, yay, great point! Then yeah, i'm all for tests =)

[george.karpenkov]

Why not turn this on by default when we turn on reporting statistics?

One issue is that LLVM timers can only be printed once (yeah, I know...) and serializing those numbers would "eat" the output, and then timers would not be displayed with -analyzer-stats.
But I guess I might as well fix the underlying problem.

[george.karpenkov]

Having a test for the plist output would be nice.

The problem is tests can be run on release configuration, which can not generate statistics at all. How could we check for that in a sensible way?

[NoQ]

In D43131#1003640, @george.karpenkov wrote:

Having a test for the plist output would be nice.

The problem is tests can be run on release configuration, which can not generate statistics at all. How could we check for that in a sensible way?

Would // REQUIRES: asserts be relevant? I don't think there's any sort of // REQUIRES: debug, what exactly is needed here?
(https://llvm.org/docs/TestingGuide.html#constraining-test-execution)

[george.karpenkov]

Would // REQUIRES: asserts be relevant?

Ah right, that should work.

[george.karpenkov]

So if https://reviews.llvm.org/D43136 goes through, we could change this to print on -analyzer-stats or even always dumping statistics when an assertion-enabled build of clang is used.

[NoQ]

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

494–497

Would this actually be a valid plist file? Maybe we should surround statistics with <string>...</string>?

[george.karpenkov]

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

494–497

@NoQ EmitString inserts those

[NoQ]

Yep, so i guess printing statistics as plist key-value pairs would be tricky. LGTM!~

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

494–497

Oh, yay, nice. It seems that i didn't even look at the test.

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

private:
   Optional<bool> SuppressFromCXXStandardLibrary;
 
   /// \sa reportIssuesInMainSourceFile
   Optional<bool> ReportIssuesInMainSourceFile;
 
   /// \sa StableReportFilename
   Optional<bool> StableReportFilename;
 
+  Optional<bool> SerializeStats;
+
   /// \sa getGraphTrimInterval
   Optional<unsigned> GraphTrimInterval;
 
   /// \sa getMaxTimesInlineLarge
   Optional<unsigned> MaxTimesInlineLarge;
 
   /// \sa getMinCFGSizeTreatFunctionsAsLarge
   Optional<unsigned> MinCFGSizeTreatFunctionsAsLarge;
public:
   bool shouldReportIssuesInMainSourceFile();
 
   /// Returns whether or not the report filename should be random or not.
   ///
   /// This is controlled by the 'stable-report-filename' config option,
   /// which accepts the values "true" and "false". Default = false
   bool shouldWriteStableReportFilename();
 
+  /// \return Whether the analyzer should
+  /// serialize statistics to plist output.
+  /// Statistics would be serialized in JSON format inside the main dictionary
+  /// under the \c statistics key.
+  /// Available only if compiled in assert mode or with LLVM statistics
+  /// explicitly enabled.
+  bool shouldSerializeStats();
+
   /// Returns whether irrelevant parts of a bug report path should be pruned
   /// out of the final output.
   ///
   /// This is controlled by the 'prune-paths' config option, which accepts the
   /// values "true" and "false".
   bool shouldPrunePaths();
 
   /// Returns true if 'static' initializers should be in conditional logic

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

 
 
 bool AnalyzerOptions::shouldWriteStableReportFilename() {
   return getBooleanOption(StableReportFilename,
                           "stable-report-filename",
                           /* Default = */ false);
 }
 
+bool AnalyzerOptions::shouldSerializeStats() {
+  return getBooleanOption(SerializeStats,
+                          "serialize-stats",
+                          /* Default = */ false);
+}
+
 int AnalyzerOptions::getOptionAsInteger(StringRef Name, int DefaultVal,
                                         const CheckerBase *C,
                                         bool SearchInParents) {
   SmallString<10> StrBuf;
   llvm::raw_svector_ostream OS(StrBuf);
   OS << DefaultVal;
 
   StringRef V = C ? getCheckerOption(C->getTagDescription(), Name, OS.str(),

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/PlistSupport.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Version.h"
 #include "clang/Lex/Preprocessor.h"
+#include "clang/Rewrite/Core/HTMLRewrite.h"
+#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/IssueHash.h"
 #include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
+#include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Casting.h"
 using namespace clang;
 using namespace ento;
 using namespace markup;
 
 namespace {
   class PlistDiagnostics : public PathDiagnosticConsumer {
     const std::string OutputFile;
     const LangOptions &LangOpts;
     const bool SupportsCrossFileDiagnostics;
+    const bool SerializeStatistics;
   public:
     PlistDiagnostics(AnalyzerOptions &AnalyzerOpts,
                      const std::string& prefix,
                      const LangOptions &LangOpts,
                      bool supportsMultipleFiles);
 
     ~PlistDiagnostics() override {}
 

 } // end anonymous namespace
 
 PlistDiagnostics::PlistDiagnostics(AnalyzerOptions &AnalyzerOpts,
                                    const std::string& output,
                                    const LangOptions &LO,
                                    bool supportsMultipleFiles)
   : OutputFile(output),
     LangOpts(LO),
-    SupportsCrossFileDiagnostics(supportsMultipleFiles) {}
+    SupportsCrossFileDiagnostics(supportsMultipleFiles),
+    SerializeStatistics(AnalyzerOpts.shouldSerializeStats()) {}
 
 void ento::createPlistDiagnosticConsumer(AnalyzerOptions &AnalyzerOpts,
                                          PathDiagnosticConsumers &C,
                                          const std::string& s,
                                          const Preprocessor &PP) {
   C.push_back(new PlistDiagnostics(AnalyzerOpts, s,
                                    PP.getLangOpts(), false));
 }
for (std::vector<const PathDiagnostic*>::iterator DI=Diags.begin(),
     }
 
     // Close up the entry.
     o << "  </dict>\n";
   }
 
   o << " </array>\n";
 
+  if (llvm::AreStatisticsEnabled() && SerializeStatistics) {
+    o << " <key>statistics</key>\n";
+    std::string stats;
+    llvm::raw_string_ostream os(stats);
+    llvm::PrintStatisticsJSON(os);
+    os.flush();
+    EmitString(o, html::EscapeText(stats)) << '\n';
+  }
+
   // Finish.
   o << "</dict>\n</plist>";
 }

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

public:
 
   StoreManagerCreator CreateStoreMgr;
   ConstraintManagerCreator CreateConstraintMgr;
 
   std::unique_ptr<CheckerManager> checkerMgr;
   std::unique_ptr<AnalysisManager> Mgr;
 
   /// Time the analyzes time of each translation unit.
-  static llvm::Timer* TUTotalTimer;
+  std::unique_ptr<llvm::TimerGroup> AnalyzerTimers;
+  std::unique_ptr<llvm::Timer> TUTotalTimer;
 
   /// The information about analyzed functions shared throughout the
   /// translation unit.
   FunctionSummariesTy FunctionSummaries;
 
   AnalysisConsumer(const Preprocessor &pp, const std::string &outdir,
                    AnalyzerOptionsRef opts, ArrayRef<std::string> plugins,
                    CodeInjector *injector)
       : RecVisitorMode(0), RecVisitorBR(nullptr), Ctx(nullptr), PP(pp),
         OutDir(outdir), Opts(std::move(opts)), Plugins(plugins),
         Injector(injector) {
     DigestAnalyzerOptions();
-    if (Opts->PrintStats) {
-      llvm::EnableStatistics(false);
-      TUTotalTimer = new llvm::Timer("time", "Analyzer Total Time");
+    if (Opts->PrintStats || Opts->shouldSerializeStats()) {
+      AnalyzerTimers = llvm::make_unique<llvm::TimerGroup>(
+          "analyzer", "Analyzer timers");
+      TUTotalTimer = llvm::make_unique<llvm::Timer>(
+          "time", "Analyzer total time", *AnalyzerTimers);
+      llvm::EnableStatistics(/* PrintOnExit= */ false);
     }
   }
 
   ~AnalysisConsumer() override {
     if (Opts->PrintStats) {
-      delete TUTotalTimer;
       llvm::PrintStatistics();
     }
   }
 
   void DigestAnalyzerOptions() {
     if (Opts->AnalysisDiagOpt != PD_NONE) {
       // Create the PathDiagnosticConsumer.
       ClangDiagPathDiagConsumer *clangDiags =
           new ClangDiagPathDiagConsumer(PP.getDiagnostics());

 
 };
 } // end anonymous namespace
 
 
 //===----------------------------------------------------------------------===//
 // AnalysisConsumer implementation.
 //===----------------------------------------------------------------------===//
-llvm::Timer* AnalysisConsumer::TUTotalTimer = nullptr;
-
 bool AnalysisConsumer::HandleTopLevelDecl(DeclGroupRef DG) {
   storeTopLevelDecls(DG);
   return true;
 }
 
 void AnalysisConsumer::HandleTopLevelDeclInObjCContainer(DeclGroupRef DG) {
   storeTopLevelDecls(DG);
 }
if (Mgr->shouldInlineCall())
       HandleDeclsCallGraph(LocalTUDeclsSize);
 
     // After all decls handled, run checkers on the entire TranslationUnit.
     checkerMgr->runCheckersOnEndOfTranslationUnit(TU, *Mgr, BR);
 
     RecVisitorBR = nullptr;
   }
 
-  // Explicitly destroy the PathDiagnosticConsumer.  This will flush its output.
-  // FIXME: This should be replaced with something that doesn't rely on
-  // side-effects in PathDiagnosticConsumer's destructor. This is required when
-  // used with option -disable-free.
-  Mgr.reset();
-
   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) /
         NumBlocksInAnalyzedFunctions;
 
+  // Explicitly destroy the PathDiagnosticConsumer.  This will flush its output.
+  // FIXME: This should be replaced with something that doesn't rely on
+  // side-effects in PathDiagnosticConsumer's destructor. This is required when
+  // used with option -disable-free.
+  Mgr.reset();
 }
 
 std::string AnalysisConsumer::getFunctionName(const Decl *D) {
   std::string Str;
   llvm::raw_string_ostream OS(Str);
 
   if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
     OS << FD->getQualifiedNameAsString();

test/Analysis/analyzer-config.c

 // CHECK-NEXT: ipa-always-inline-size = 3
 // CHECK-NEXT: leak-diagnostics-reference-allocation = false
 // CHECK-NEXT: max-inlinable-size = 100
 // CHECK-NEXT: max-nodes = 225000
 // CHECK-NEXT: max-times-inline-large = 32
 // CHECK-NEXT: min-cfg-size-treat-functions-as-large = 14
 // CHECK-NEXT: mode = deep
 // CHECK-NEXT: region-store-small-struct-limit = 2
+// CHECK-NEXT: serialize-stats = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
 // CHECK-NEXT: num-entries = 21

test/Analysis/analyzer-config.cpp

 // CHECK-NEXT: ipa-always-inline-size = 3
 // CHECK-NEXT: leak-diagnostics-reference-allocation = false
 // CHECK-NEXT: max-inlinable-size = 100
 // CHECK-NEXT: max-nodes = 225000
 // CHECK-NEXT: max-times-inline-large = 32
 // CHECK-NEXT: min-cfg-size-treat-functions-as-large = 14
 // CHECK-NEXT: mode = deep
 // CHECK-NEXT: region-store-small-struct-limit = 2
+// CHECK-NEXT: serialize-stats = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
 // CHECK-NEXT: num-entries = 26

test/Analysis/plist-stats-output.c

+// RUN: %clang_analyze_cc1 %s -analyzer-checker=core -analyzer-output=plist -analyzer-config serialize-stats=true -o %t.plist
+// REQUIRES: asserts
+// RUN: FileCheck --input-file=%t.plist %s
+
+int foo() {}
+
+
+// CHECK:  <key>diagnostics</key>
+// CHECK-NEXT:  <array>
+// CHECK-NEXT:  </array>
+// CHECK-NEXT: <key>statistics</key>
+// CHECK-NEXT: <string>{
+// CHECK: }
+// CHECK-NEXT: </string>