[ASTMatchers] Output currently processing match and nodes on crash

njames93 · njames93 · commit d89f9e963e49 · 2022-03-21T19:13:36.000Z
Create a PrettyStackTraceEvent that will dump the current `MatchCallback` id as well as the `BoundNodes` if the 'run' method of a `MatchCallback` results in a crash. The purpose of this is sometimes clang-tidy checks can crash in the `check` method. And in a large codebase with alot of checks enabled and in a release build, it can be near impossible to figure out which check as well as the source code that caused the crash. Without that information a reproducer is very hard to create. This is a more generalised version of D118520 which has a nicer integration and should be useful to clients other than clang-tidy. Reviewed By: aaron.ballman Differential Revision: https://reviews.llvm.org/D120185
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -96,6 +96,9 @@ The improvements are...
 Improvements to clang-tidy
 --------------------------
 
+- Added trace code to help narrow down any checks and the relevant source code
+  that result in crashes.
+
 New checks
 ^^^^^^^^^^
 
diff --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Timer.h"
 #include <deque>
 #include <memory>
@@ -760,11 +761,67 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>,
         D);
   }
 
+  class TraceReporter : llvm::PrettyStackTraceEntry {
+  public:
+    TraceReporter(const MatchASTVisitor &MV) : MV(MV) {}
+    void print(raw_ostream &OS) const override {
+      if (!MV.CurMatched) {
+        OS << "ASTMatcher: Not currently matching\n";
+        return;
+      }
+      assert(MV.ActiveASTContext &&
+             "ActiveASTContext should be set if there is a matched callback");
+
+      OS << "ASTMatcher: Processing '" << MV.CurMatched->getID() << "'\n";
+      const BoundNodes::IDToNodeMap &Map = MV.CurBoundNodes->getMap();
+      if (Map.empty()) {
+        OS << "No bound nodes\n";
+        return;
+      }
+      OS << "--- Bound Nodes Begin ---\n";
+      for (const auto &Item : Map) {
+        OS << "    " << Item.first << " - { ";
+        if (const auto *D = Item.second.get<Decl>()) {
+          OS << D->getDeclKindName() << "Decl ";
+          if (const auto *ND = dyn_cast<NamedDecl>(D)) {
+            ND->printQualifiedName(OS);
+            OS << " : ";
+          } else
+            OS << ": ";
+          D->getSourceRange().print(OS,
+                                    MV.ActiveASTContext->getSourceManager());
+        } else if (const auto *S = Item.second.get<Stmt>()) {
+          OS << S->getStmtClassName() << " : ";
+          S->getSourceRange().print(OS,
+                                    MV.ActiveASTContext->getSourceManager());
+        } else if (const auto *T = Item.second.get<Type>()) {
+          OS << T->getTypeClassName() << "Type : ";
+          QualType(T, 0).print(OS, MV.ActiveASTContext->getPrintingPolicy());
+        } else if (const auto *QT = Item.second.get<QualType>()) {
+          OS << "QualType : ";
+          QT->print(OS, MV.ActiveASTContext->getPrintingPolicy());
+        } else {
+          OS << Item.second.getNodeKind().asStringRef() << " : ";
+          Item.second.getSourceRange().print(
+              OS, MV.ActiveASTContext->getSourceManager());
+        }
+        OS << " }\n";
+      }
+      OS << "--- Bound Nodes End ---\n";
+    }
+
+  private:
+    const MatchASTVisitor &MV;
+  };
+
 private:
   bool TraversingASTNodeNotSpelledInSource = false;
   bool TraversingASTNodeNotAsIs = false;
   bool TraversingASTChildrenNotSpelledInSource = false;
 
+  const MatchCallback *CurMatched = nullptr;
+  const BoundNodes *CurBoundNodes = nullptr;
+
   struct ASTNodeNotSpelledInSourceScope {
     ASTNodeNotSpelledInSourceScope(MatchASTVisitor *V, bool B)
         : MV(V), MB(V->TraversingASTNodeNotSpelledInSource) {
@@ -831,7 +888,7 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>,
         Timer.setBucket(&TimeByBucket[MP.second->getID()]);
       BoundNodesTreeBuilder Builder;
       if (MP.first.matches(Node, this, &Builder)) {
-        MatchVisitor Visitor(ActiveASTContext, MP.second);
+        MatchVisitor Visitor(*this, ActiveASTContext, MP.second);
         Builder.visitMatches(&Visitor);
       }
     }
@@ -863,7 +920,7 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>,
       }
 
       if (MP.first.matches(DynNode, this, &Builder)) {
-        MatchVisitor Visitor(ActiveASTContext, MP.second);
+        MatchVisitor Visitor(*this, ActiveASTContext, MP.second);
         Builder.visitMatches(&Visitor);
       }
     }
@@ -1049,18 +1106,36 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>,
   // Implements a BoundNodesTree::Visitor that calls a MatchCallback with
   // the aggregated bound nodes for each match.
   class MatchVisitor : public BoundNodesTreeBuilder::Visitor {
+    struct CurBoundScope {
+      CurBoundScope(MatchASTVisitor &MV, const BoundNodes &BN) : MV(MV) {
+        assert(MV.CurMatched && !MV.CurBoundNodes);
+        MV.CurBoundNodes = &BN;
+      }
+
+      ~CurBoundScope() { MV.CurBoundNodes = nullptr; }
+
+    private:
+      MatchASTVisitor &MV;
+    };
+
   public:
-    MatchVisitor(ASTContext* Context,
-                 MatchFinder::MatchCallback* Callback)
-      : Context(Context),
-        Callback(Callback) {}
+    MatchVisitor(MatchASTVisitor &MV, ASTContext *Context,
+                 MatchFinder::MatchCallback *Callback)
+        : MV(MV), Context(Context), Callback(Callback) {
+      assert(!MV.CurMatched && !MV.CurBoundNodes);
+      MV.CurMatched = Callback;
+    }
+
+    ~MatchVisitor() { MV.CurMatched = nullptr; }
 
     void visitMatch(const BoundNodes& BoundNodesView) override {
       TraversalKindScope RAII(*Context, Callback->getCheckTraversalKind());
+      CurBoundScope RAII2(MV, BoundNodesView);
       Callback->run(MatchFinder::MatchResult(BoundNodesView, Context));
     }
 
   private:
+    MatchASTVisitor &MV;
     ASTContext* Context;
     MatchFinder::MatchCallback* Callback;
   };
@@ -1470,6 +1545,7 @@ void MatchFinder::match(const clang::DynTypedNode &Node, ASTContext &Context) {
 
 void MatchFinder::matchAST(ASTContext &Context) {
   internal::MatchASTVisitor Visitor(&Matchers, Options);
+  internal::MatchASTVisitor::TraceReporter StackTrace(Visitor);
   Visitor.set_active_ast_context(&Context);
   Visitor.onStartOfTranslationUnit();
   Visitor.TraverseAST(Context);
diff --git a/clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
@@ -12,8 +12,10 @@
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/Triple.h"
+#include "llvm/Config/config.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Testing/Support/SupportHelpers.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
@@ -34,6 +36,87 @@ TEST(HasNameDeathTest, DiesOnEmptyPattern) {
       EXPECT_TRUE(notMatches("class X {};", HasEmptyName));
     }, "");
 }
+
+#if ENABLE_BACKTRACES
+
+template <typename MatcherT>
+static void crashTestNodeDump(MatcherT Matcher,
+                              ArrayRef<StringRef> MatchedNodes,
+                              StringRef Code) {
+  llvm::EnablePrettyStackTrace();
+  MatchFinder Finder;
+
+  struct CrashCallback : public MatchFinder::MatchCallback {
+    void run(const MatchFinder::MatchResult &Result) override {
+      LLVM_BUILTIN_TRAP;
+    }
+    llvm::Optional<TraversalKind> getCheckTraversalKind() const override {
+      return TK_IgnoreUnlessSpelledInSource;
+    }
+    StringRef getID() const override { return "CrashTester"; }
+  } Callback;
+  Finder.addMatcher(std::move(Matcher), &Callback);
+  if (MatchedNodes.empty()) {
+    ASSERT_DEATH(tooling::runToolOnCode(
+                     newFrontendActionFactory(&Finder)->create(), Code),
+                 testing::HasSubstr(
+                     "ASTMatcher: Processing 'CrashTester'\nNo bound nodes"));
+  } else {
+    std::vector<testing::PolymorphicMatcher<
+        testing::internal::HasSubstrMatcher<std::string>>>
+        Matchers;
+    Matchers.reserve(MatchedNodes.size());
+    for (auto Node : MatchedNodes) {
+      Matchers.push_back(testing::HasSubstr(Node.str()));
+    }
+    auto CrashMatcher = testing::AllOf(
+        testing::HasSubstr(
+            "ASTMatcher: Processing 'CrashTester'\n--- Bound Nodes Begin ---"),
+        testing::HasSubstr("--- Bound Nodes End ---"),
+        testing::AllOfArray(Matchers));
+
+    ASSERT_DEATH(tooling::runToolOnCode(
+                     newFrontendActionFactory(&Finder)->create(), Code),
+                 CrashMatcher);
+  }
+}
+TEST(MatcherCrashDeathTest, CrashOnCallbackDump) {
+  crashTestNodeDump(forStmt(), {}, "void foo() { for(;;); }");
+  crashTestNodeDump(
+      forStmt(hasLoopInit(declStmt(hasSingleDecl(
+                                       varDecl(hasType(qualType().bind("QT")),
+                                               hasType(type().bind("T")),
+                                               hasInitializer(
+                                                   integerLiteral().bind("IL")))
+                                           .bind("VD")))
+                              .bind("DS")))
+          .bind("FS"),
+      {"FS - { ForStmt : <input.cc:3:5, line:4:5> }",
+       "DS - { DeclStmt : <input.cc:3:10, col:19> }",
+       "IL - { IntegerLiteral : <input.cc:3:18> }", "QT - { QualType : int }",
+       "T - { BuiltinType : int }",
+       "VD - { VarDecl I : <input.cc:3:10, col:18> }"},
+      R"cpp(
+  void foo() {
+    for (int I = 0; I < 5; ++I) {
+    }
+  }
+  )cpp");
+  crashTestNodeDump(
+      cxxRecordDecl(hasMethod(cxxMethodDecl(hasName("operator+")).bind("Op+")))
+          .bind("Unnamed"),
+      {"Unnamed - { CXXRecordDecl (anonymous) : <input.cc:1:1, col:36> }",
+       "Op+ - { CXXMethodDecl (anonymous struct)::operator+ : <input.cc:1:10, "
+       "col:29> }"},
+      "struct { int operator+(int) const; } Unnamed;");
+  crashTestNodeDump(
+      cxxRecordDecl(hasMethod(cxxConstructorDecl(isDefaulted()).bind("Ctor")),
+                    hasMethod(cxxDestructorDecl(isDefaulted()).bind("Dtor"))),
+      {"Ctor - { CXXConstructorDecl Foo::Foo : <input.cc:1:14, col:28> }",
+       "Dtor - { CXXDestructorDecl Foo::~Foo : <input.cc:1:31, col:46> }"},
+      "struct Foo { Foo() = default; ~Foo() = default; };");
+}
+#endif // ENABLE_BACKTRACES
 #endif
 
 TEST(ConstructVariadic, MismatchedTypes_Regression) {
