[ASTMatchers] Output currently processing match and nodes on crash

[njames93]

Summary

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.

[njames93]

I've reused the test cases from the old clang-tidy implementation of this, however this should eventually be moved into ASTMatchers tests.

[njames93]

Moved TraceReporter into the MatchASTVisitor class
Added some missing new lines
Fixed patch not being based off a commit in trunk

clang/lib/ASTMatchers/ASTMatchFinder.cpp

1541

Is there a precedent to instrument this function too. AFAIK this is only used when running 1 matcher on a predetermined node so doesn't have the same issues when debugging as below.

[njames93]

Moved tests into ASTMatchersInternal, makes sense in here and removes the dirty clang-tidy plugin hack.

[njames93]

Update test case

[aaron.ballman]

clang/lib/ASTMatchers/ASTMatchFinder.cpp

789

Should this be "<anonymous> : " instead?

796

Do we also need a match for TypeLoc matchers, or does the else cover that sufficiently well?

(Actually, should we handle all of the various matchers at: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/ASTMatchers/ASTMatchers.h#L141 rather than leaving it to an else? Then the else can become an unreachable so we know to update this interface?)

[njames93]

clang/lib/ASTMatchers/ASTMatchFinder.cpp

789

Good catch

796

The else should be sufficient for most general cases, the only reason some are special cased is to improve the output, but I don't want there to be a burden to update this interface if new nodes are added.

[aaron.ballman]

clang/lib/ASTMatchers/ASTMatchFinder.cpp

796

I should verify: does this map hold arbitrary AST nodes, or does the map only hold the top-level classes in the AST matching hierarchy?

If it's arbitrary AST nodes, then yeah, I definitely agree the else here is fine. If it's top-level classes instead, we don't add those all that often and so it doesn't seem like a major burden to keep those in sync.

[njames93]

clang/lib/ASTMatchers/ASTMatchFinder.cpp

796

It can hold any kind of node that can be stored in a DynTypedNode, so essentially it's any arbitrary AST node.

[aaron.ballman]

Essentially looks good to me now, thanks!

clang/lib/ASTMatchers/ASTMatchFinder.cpp

789

It'd probably be good to add some test coverage for these weird edge cases (unnamed structs, constructors or other special member functions without a name).

796

Thanks for clarifying that; the else look *beautiful* to me! :-D

[njames93]

Rework testing infrastructure.
Add more test cases.

[njames93]

Remove <anonymous> from Decls that aren't NamedDecls

[aaron.ballman]

LGTM!

[thakis]

Looks like this breaks tests on Windows: http://45.33.8.238/win/54785/step_7.txt

Please take a look, and revert for now if takes a while to fix.

[njames93]

In D120185#3397508, @thakis wrote:

Looks like this breaks tests on Windows: http://45.33.8.238/win/54785/step_7.txt

Please take a look, and revert for now if takes a while to fix.

Seems to be that on windows we aren't getting the crash dump. I'll revert and investigate

[njames93]

In D120185#3397508, @thakis wrote:

Looks like this breaks tests on Windows: http://45.33.8.238/win/54785/step_7.txt

Please take a look, and revert for now if takes a while to fix.

I can't seem to reproduce the test failure on windows, are you opposed to relanding this with the test disabled on windows?

[aaron.ballman]

In D120185#3403230, @njames93 wrote:

In D120185#3397508, @thakis wrote:

Looks like this breaks tests on Windows: http://45.33.8.238/win/54785/step_7.txt

Please take a look, and revert for now if takes a while to fix.

I can't seem to reproduce the test failure on windows, are you opposed to relanding this with the test disabled on windows?

I'd like to understand what's happening better rather than land with a disabled test. Not getting the crash dump in some circumstances could either be specific to the machine configuration (which would be pretty tricky for us to solve in a unit test) or it could be something functionally wrong with the changes (though I would be surprised, tbh).

[njames93]

In D120185#3403270, @aaron.ballman wrote:

I'd like to understand what's happening better rather than land with a disabled test. Not getting the crash dump in some circumstances could either be specific to the machine configuration (which would be pretty tricky for us to solve in a unit test) or it could be something functionally wrong with the changes (though I would be surprised, tbh).

From what it looks like on the buildbot, the testing infrastructure wasn't recieving any output from stderr.

[aaron.ballman]

In D120185#3403676, @njames93 wrote:

In D120185#3403270, @aaron.ballman wrote:

I'd like to understand what's happening better rather than land with a disabled test. Not getting the crash dump in some circumstances could either be specific to the machine configuration (which would be pretty tricky for us to solve in a unit test) or it could be something functionally wrong with the changes (though I would be surprised, tbh).

From what it looks like on the buildbot, the testing infrastructure wasn't recieving any output from stderr.

@thakis -- is it possible your build bot is configured to disable generation of crash dumps?

[njames93]

In D120185#3405091, @aaron.ballman wrote:

@thakis -- is it possible your build bot is configured to disable generation of crash dumps?

So with a lot of trial and error, it seems that on certain windows configurations, crash dumps just aren't being emitted.
Best I can see so far is using clang to build the tests, results in ENABLE_BACKTRACES being enabled, but no crash dumps being emitted, causing the test failure.
I did change the test to throw an assert instead of a TRAP instruction, and the assert message was captured but no crash dump was reported, so the test infrastructure has no issue there.
I feel like a stop gap may be to disable the test when clang is the host compiler and windows is the platform.

[aaron.ballman]

In D120185#3406765, @njames93 wrote:

In D120185#3405091, @aaron.ballman wrote:

@thakis -- is it possible your build bot is configured to disable generation of crash dumps?

So with a lot of trial and error, it seems that on certain windows configurations, crash dumps just aren't being emitted.
Best I can see so far is using clang to build the tests, results in ENABLE_BACKTRACES being enabled, but no crash dumps being emitted, causing the test failure.

From looking at CMake, I don't see anything that actually tests if backtraces are enabled by the OS; we just trust the user told us something valid and set ENABLE_BACKTRACES accordingly. So I'd still like to hear from @thakis about his bot configuration.

I did change the test to throw an assert instead of a TRAP instruction, and the assert message was captured but no crash dump was reported, so the test infrastructure has no issue there.

That's good at least.

I feel like a stop gap may be to disable the test when clang is the host compiler and windows is the platform.

I wonder if "clang is the host compiler" is just happenstance though and this will cause us to pass @thakis' bots but not other testing situations (other downstreams, for example). So this may work as a stopgap, but if we can get to the bottom of the issue, I think it would be valuable. e.g., perhaps the better solution is for cmake to run a configure test to see if crash dumps are actually generated (or enabled, if there's a WMI query or something we can use), and if not, warn the user and don't define ENABLE_BACKTRACES in that case.

[thakis]

That bot sets ENABLE_BACKTRACES=1.

It seems to work for clang-cl (foo.cc contains #pragma clang __debug parser_crash):

>out\gn\bin\clang-cl /c foo.cc
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: out\\gn\\bin\\clang-cl /c foo.cc
1.      foo.cc:1:23: at annotation token
 #0 0x00007ff6653b7395 (C:\src\llvm-project\out\gn\bin\clang-cl.exe+0x3997395)

I tried setting up a local cmake build, but cmake complains that it can't find py3 even though I have it installed. I also have today off, so I don't want to spend a lot of time debugging it.

I'd suggest you land this with the test disabled on Win, and I'll debug it on Windows next week.

[aaron.ballman]

In D120185#3408258, @thakis wrote:

That bot sets ENABLE_BACKTRACES=1.

It seems to work for clang-cl (foo.cc contains #pragma clang __debug parser_crash):

>out\gn\bin\clang-cl /c foo.cc
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: out\\gn\\bin\\clang-cl /c foo.cc
1.      foo.cc:1:23: at annotation token
 #0 0x00007ff6653b7395 (C:\src\llvm-project\out\gn\bin\clang-cl.exe+0x3997395)

I tried setting up a local cmake build, but cmake complains that it can't find py3 even though I have it installed. I also have today off, so I don't want to spend a lot of time debugging it.

I'd suggest you land this with the test disabled on Win, and I'll debug it on Windows next week.

Okay, that works for me, this definitely sounds more involved than "bot configured wrong" but doesn't sound like "functionality is incorrect with the patch". Thanks for digging into this next week @thakis (and enjoy your day off!), and thanks for the discussion @njames93 while we figured out what to do next.

[njames93]

I've landed it and your build bot is happy, there was another failing buildbot so I'll keep an eye on it.

[mgorny]

This change broke standalone build of clang. Please fix.

clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp

15

This is private header of LLVM and you can't use it in clang.

[njames93]

In D120185#3668029, @mgorny wrote:

This change broke standalone build of clang. Please fix.

I've pushed rG251b5b864183e868ffc86522e320f91ab3c5a771 which should fix the build, but need to look into a way to re-enable the tests.

[mgorny]

Thanks. The usual way of doing is to move the necessary bits to llvm-config.h.

clang-tools-extra/docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 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
 ^^^^^^^^^^
 
 - New :doc:`bugprone-shared-ptr-array-mismatch <clang-tidy/checks/bugprone-shared-ptr-array-mismatch>` check.
 
   Finds initializations of C++ shared pointers to non-array type that are initialized with an array.
 
 New check aliases

clang/lib/ASTMatchers/ASTMatchFinder.cpp

 //===----------------------------------------------------------------------===//
 
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #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>
 #include <set>
 
 namespace clang {
 namespace ast_matchers {
 namespace internal {
public:
   }
 
   bool TraverseTemplateInstantiations(FunctionTemplateDecl *D) {
     ASTNodeNotSpelledInSourceScope RAII(this, true);
     return RecursiveASTVisitor<MatchASTVisitor>::TraverseTemplateInstantiations(
         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) {
       V->TraversingASTNodeNotSpelledInSource = B;
     }
     ~ASTNodeNotSpelledInSourceScope() {
       MV->TraversingASTNodeNotSpelledInSource = MB;
     }
private:
   void matchWithoutFilter(const T &Node, const MC &Matchers) {
     const bool EnableCheckProfiling = Options.CheckProfiling.hasValue();
     TimeBucketRegion Timer;
     for (const auto &MP : Matchers) {
       if (EnableCheckProfiling)
         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);
       }
     }
   }
 
   void matchWithFilter(const DynTypedNode &DynNode) {
     auto Kind = DynNode.getNodeKind();
     auto it = MatcherFiltersMap.find(Kind);
for (unsigned short I : Filter) {
       {
         TraversalKindScope RAII(getASTContext(), MP.first.getTraversalKind());
         if (getASTContext().getParentMapContext().traverseIgnored(DynNode) !=
             DynNode)
           continue;
       }
 
       if (MP.first.matches(DynNode, this, &Builder)) {
-        MatchVisitor Visitor(ActiveASTContext, MP.second);
+        MatchVisitor Visitor(*this, ActiveASTContext, MP.second);
         Builder.visitMatches(&Visitor);
       }
     }
   }
 
   const std::vector<unsigned short> &getFilterForKind(ASTNodeKind Kind) {
     auto &Filter = MatcherFiltersMap[Kind];
     auto &Matchers = this->Matchers->DeclOrStmt;
#endif
       }
     }
     return Finish(false);
   }
 
   // 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;
   };
 
   // Returns true if 'TypeNode' has an alias that matches the given matcher.
   bool typeHasMatchingAlias(const Type *TypeNode,
                             const Matcher<NamedDecl> &Matcher,
                             BoundNodesTreeBuilder *Builder) {
bool MatchFinder::addDynamicMatcher(const internal::DynTypedMatcher &NodeMatch,
   return false;
 }
 
 std::unique_ptr<ASTConsumer> MatchFinder::newASTConsumer() {
   return std::make_unique<internal::MatchASTConsumer>(this, ParsingDone);
 }
 
 void MatchFinder::match(const clang::DynTypedNode &Node, ASTContext &Context) {
   internal::MatchASTVisitor Visitor(&Matchers, Options);
   Visitor.set_active_ast_context(&Context);
   Visitor.match(Node);
 }
 
 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);
   Visitor.onEndOfTranslationUnit();
 }
 
 void MatchFinder::registerTestCallbackAfterParsing(
     MatchFinder::ParsingDoneTestCallback *NewParsingDone) {

clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp

 // unittests/ASTMatchers/ASTMatchersInternalTest.cpp - AST matcher unit tests //
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "ASTMatchersTest.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #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 {
 namespace ast_matchers {
 using internal::DynTypedMatcher;
 
 #if GTEST_HAS_DEATH_TEST
 TEST(HasNameDeathTest, DiesOnEmptyName) {
   ASSERT_DEBUG_DEATH({
     DeclarationMatcher HasEmptyName = recordDecl(hasName(""));
     EXPECT_TRUE(notMatches("class X {};", HasEmptyName));
   }, "");
 }
 
 TEST(HasNameDeathTest, DiesOnEmptyPattern) {
   ASSERT_DEBUG_DEATH({
       DeclarationMatcher HasEmptyName = recordDecl(matchesName(""));
       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) {
   EXPECT_TRUE(
       matches("const int a = 0;", internal::DynTypedMatcher::constructVariadic(
                                       internal::DynTypedMatcher::VO_AnyOf,
                                       ASTNodeKind::getFromNodeKind<QualType>(),
                                       {isConstQualified(), arrayType()})