[analyzer] print() JSONify: Checker messages implementation

[Charusso]

Summary

-

[Charusso]

• Improved the JSON format.
• Now the message is a multi-line string properly JSON-spaced.

[NoQ]

Let's add a real test here as well. This code is complicated enough.

[Charusso]

• Added a test.

[Charusso]

Not sure what causes the nondeterministic CHECKs, but sometimes it writes out
"a->b: moved", "a: moved" instead of the logical order: "a: moved", a->b: moved",
so that it could match the wrong line.

I do not know why we prints out two states at one point in one File-check run.
It looks like that:

"program_state": {
  "store": [
    { "cluster": "a", "items": [
      { "kind": "Default", "offset": 128, "value": "0 S8b" },
      { "kind": "Direct", "offset": 0, "value": "42 S32b" },
      { "kind": "Direct", "offset": 64, "value": "Unknown" },
    ]},
  ],
  "environment": [
    { "location_context": "#0 Call", "calling": "simpleMoveCtorTest", "call_line": null, "items": [
      { "lctx_id": 1008, "stmt_id": 37353, "pretty": "clang_analyzer_printState", "value": "&code{clang_analyzer_printState}" },
    ]},
  ],
  "constraints": null,
  "dynamic_types": null,
  "constructing_objects": null,
  "checker_messages": [
    { "checker": "cplusplus.Move", "messages": [
      "Moved-from objects :", 
      "a: moved", 
      "a->b: moved", 
      "",
    ]},
  ],
}
"program_state": {
  "store": [
    { "cluster": "a", "items": [
      { "kind": "Default", "offset": 128, "value": "0 S8b" },
      { "kind": "Direct", "offset": 0, "value": "42 S32b" },
      { "kind": "Direct", "offset": 64, "value": "Unknown" },
    ]},
  ],
  "environment": [
    { "location_context": "#0 Call", "calling": "simpleMoveCtorTest", "call_line": null, "items": [
      { "lctx_id": 621, "stmt_id": 37353, "pretty": "clang_analyzer_printState", "value": "&code{clang_analyzer_printState}" },
    ]},
  ],
  "constraints": null,
  "dynamic_types": null,
  "constructing_objects": null,
  "checker_messages": [
    { "checker": "cplusplus.Move", "messages": [
      "Moved-from objects :", 
      "a->b: moved", 
      "a: moved", 
      "",
    ]},
  ],
}

[NoQ]

These prints are different across runs, but same across different states on the same run, right? That's kinda expected because the map is ordered by pointer values internally, which may change across runs. This is not that bad of a non-determinism, i don't think it's worth fixing. Let's test a single line to make sure the test definitely doesn't test the order (i.e., leave only a->b or only a) and then commit!

[Charusso]

In D62086#1517701, @NoQ wrote:

These prints are different across runs, but same across different states on the same run, right? That's kinda expected because the map is ordered by pointer values internally, which may change across runs. This is not that bad of a non-determinism, i don't think it's worth fixing. Let's test a single line to make sure the test definitely doesn't test the order (i.e., leave only a->b or only a) and then commit!

So that two printState() dumps are only one test-RUN, which is the new one. Because there are two entries of printState() it is 50% to pass the test as it sometimes catches the other printState() and that is why a->b: moved cannot be set to the first, nor a: moved. It would be cool to allow only one printState() somehow.

[NoQ]

I mean, let's remove one of the lines from the test so that it became deterministic and commit and then polish later.

[Charusso]

• 100% pure JSON.
• Removed one CHECK.

[Charusso]

It still looks like broken, but if you like that, I like that too.

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

//===----------------------------------------------------------------------===//
   void runCheckersOnEndOfTranslationUnit(const TranslationUnitDecl *TU,
                                          AnalysisManager &mgr,
                                          BugReporter &BR);
 
   /// Run checkers for debug-printing a ProgramState.
   ///
   /// Unlike most other callbacks, any checker can simply implement the virtual
   /// method CheckerBase::printState if it has custom data to print.
-  /// \param Out The output stream
+  ///
+  /// \param Out   The output stream
   /// \param State The state being printed
-  /// \param NL The preferred representation of a newline.
-  /// \param Sep The preferred separator between different kinds of data.
-  void runCheckersForPrintState(raw_ostream &Out, ProgramStateRef State,
-                                const char *NL, const char *Sep);
+  /// \param NL    The preferred representation of a newline.
+  /// \param Sep   The preferred separator between different messages.
+  /// \param Space The preferred space between the left side and the message.
+  /// \param IsDot Whether the message will be printed in 'dot' format.
+  void runCheckersForPrintStateJson(raw_ostream &Out, ProgramStateRef State,
+                                    const char *NL = "\n",
+                                    unsigned int Space = 0,
+                                    bool IsDot = false) const;
 
   //===----------------------------------------------------------------------===//
   // Internal registration functions for AST traversing.
   //===----------------------------------------------------------------------===//
 
   // Functions used by the registration mechanism, checkers should not touch
   // these directly.
 
   using CheckDeclFunc =
       CheckerFn<void (const Decl *, AnalysisManager&, BugReporter &)>;
 
   using HandlesDeclFunc = bool (*)(const Decl *D);

cfe/trunk/lib/StaticAnalyzer/Core/CheckerManager.cpp

 //===- CheckerManager.cpp - Static Analyzer Checker Manager ---------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // Defines the Static Analyzer Checker Manager.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/Stmt.h"
 #include "clang/Analysis/ProgramPoint.h"
+#include "clang/Basic/JsonSupport.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"

 void CheckerManager::runCheckersOnEndOfTranslationUnit(
                                                   const TranslationUnitDecl *TU,
                                                   AnalysisManager &mgr,
                                                   BugReporter &BR) {
   for (const auto EndOfTranslationUnitChecker : EndOfTranslationUnitCheckers)
     EndOfTranslationUnitChecker(TU, mgr, BR);
 }
 
-void CheckerManager::runCheckersForPrintState(raw_ostream &Out,
+void CheckerManager::runCheckersForPrintStateJson(raw_ostream &Out,
                                                   ProgramStateRef State,
-                                              const char *NL, const char *Sep) {
-  for (const auto &CheckerTag : CheckerTags)
-    CheckerTag.second->printState(Out, State, NL, Sep);
+                                                  const char *NL,
+                                                  unsigned int Space,
+                                                  bool IsDot) const {
+  Indent(Out, Space, IsDot) << "\"checker_messages\": ";
+
+  // Create a temporary stream to see whether we have any message.
+  SmallString<1024> TempBuf;
+  llvm::raw_svector_ostream TempOut(TempBuf);
+  unsigned int InnerSpace = Space + 2;
+
+  // Create the new-line in JSON with enough space.
+  SmallString<128> NewLine;
+  llvm::raw_svector_ostream NLOut(NewLine);
+  NLOut << "\", " << NL;                     // Inject the ending and a new line
+  Indent(NLOut, InnerSpace, IsDot) << "\"";  // then begin the next message.
+
+  ++Space;
+  bool HasMessage = false;
+
+  // Store the last CheckerTag.
+  const void *LastCT = nullptr;
+  for (const auto &CT : CheckerTags) {
+    // See whether the current checker has a message.
+    CT.second->printState(TempOut, State, /*NL=*/NewLine.c_str(), /*Sep=*/"");
+
+    if (TempBuf.empty())
+      continue;
+
+    if (!HasMessage) {
+      Out << '[' << NL;
+      HasMessage = true;
+    }
+
+    LastCT = &CT;
+    TempBuf.clear();
+  }
+
+  for (const auto &CT : CheckerTags) {
+    // See whether the current checker has a message.
+    CT.second->printState(TempOut, State, /*NL=*/NewLine.c_str(), /*Sep=*/"");
+
+    if (TempBuf.empty())
+      continue;
+
+    Indent(Out, Space, IsDot)
+        << "{ \"checker\": \"" << CT.second->getCheckName().getName()
+        << "\", \"messages\": [" << NL;
+    Indent(Out, InnerSpace, IsDot)
+        << '\"' << TempBuf.str().trim() << '\"' << NL;
+    Indent(Out, Space, IsDot) << "]}";
+
+    if (&CT != LastCT)
+      Out << ',';
+    Out << NL;
+
+    TempBuf.clear();
+  }
+
+  // It is the last element of the 'program_state' so do not add a comma.
+  if (HasMessage)
+    Indent(Out, --Space, IsDot) << "]";
+  else
+    Out << "null";
+
+  Out << NL;
 }
 
 //===----------------------------------------------------------------------===//
 // Internal registration functions for AST traversing.
 //===----------------------------------------------------------------------===//
 
 void CheckerManager::_registerForDecl(CheckDeclFunc checkfn,
                                       HandlesDeclFunc isForDeclFn) {

cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp

if (LCtx && !State->get<ObjectsUnderConstruction>().isEmpty()) {
     });
 
     --Space;
     Indent(Out, Space, IsDot) << "]," << NL; // End of "constructing_objects".
   } else {
     Out << "null," << NL;
   }
 
-  getCheckerManager().runCheckersForPrintState(Out, State, NL, "");
+  getCheckerManager().runCheckersForPrintStateJson(Out, State, NL, Space,
+                                                   IsDot);
 }
 
 void ExprEngine::processEndWorklist() {
   getCheckerManager().runCheckersForEndAnalysis(G, BR, *this);
 }
 
 void ExprEngine::processCFGElement(const CFGElement E, ExplodedNode *Pred,
                                    unsigned StmtIdx, NodeBuilderContext *Ctx) {

cfe/trunk/test/Analysis/expr-inspection.c

 // CHECK-NEXT:     { "lctx_id": 1, "stmt_id": 847, "pretty": "clang_analyzer_printState", "value": "&code{clang_analyzer_printState}" }
 // CHECK-NEXT:   ]}
 // CHECK-NEXT: ],
 // CHECK-NEXT: "constraints": [
 // CHECK-NEXT:   { "symbol": "reg_$0<int x>", "range": "{ [-2147483648, 13] }" }
 // CHECK-NEXT: ],
 // CHECK-NEXT: "dynamic_types": null,
 // CHECK-NEXT: "constructing_objects": null,
+// CHECK-NEXT: "checker_messages": null
+

cfe/trunk/test/Analysis/use-after-move.cpp

 // RUN:   -analyzer-config cplusplus.Move:WarnOn="a bunch of things" \
 // RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-MOVE-INVALID-VALUE
 
 // CHECK-MOVE-INVALID-VALUE: (frontend): invalid input for checker option
 // CHECK-MOVE-INVALID-VALUE-SAME: 'cplusplus.Move:WarnOn', that expects either
 // CHECK-MOVE-INVALID-VALUE-SAME: "KnownsOnly", "KnownsAndLocals" or "All"
 // CHECK-MOVE-INVALID-VALUE-SAME: string value
 
+// Tests checker-messages printing.
+// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.Move %s\
+// RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
+// RUN:  -analyzer-config exploration_strategy=dfs -DDFS\
+// RUN:  -analyzer-config cplusplus.Move:WarnOn=All -DAGGRESSIVE_DFS\
+// RUN:  -analyzer-checker core,cplusplus.SmartPtr,debug.ExprInspection\
+// RUN:  -verify=expected,peaceful,aggressive %s 2>&1 | FileCheck %s
+
 #include "Inputs/system-header-simulator-cxx.h"
 
 void clang_analyzer_warnIfReached();
+void clang_analyzer_printState();
 
 class B {
 public:
   B() = default;
   B(const B &) = default;
   B(B &&) = default;
   B& operator=(const B &q) = default;
   void operator=(B &&b) {

 void copyOrMoveCall(A a) {
   a.foo();
 }
 
 void simpleMoveCtorTest() {
   {
     A a;
     A b = std::move(a); // peaceful-note {{Object 'a' is moved}}
+
+#ifdef AGGRESSIVE_DFS
+    clang_analyzer_printState();
+
+// CHECK:      "checker_messages": [
+// CHECK-NEXT:   { "checker": "cplusplus.Move", "messages": [
+// CHECK-NEXT:     "Moved-from objects :",
+// CHECK:          "a: moved",
+// CHECK:          ""
+// CHECK-NEXT:   ]}
+// CHECK-NEXT: ]
+#endif
+
     a.foo(); // peaceful-warning {{Method called on moved-from object 'a'}}
              // peaceful-note@-1 {{Method called on moved-from object 'a'}}
   }
   {
     A a;
     A b = std::move(a); // peaceful-note {{Object 'a' is moved}}
     b = a; // peaceful-warning {{Moved-from object 'a' is copied}}
            // peaceful-note@-1 {{Moved-from object 'a' is copied}}