[ASTMatchers] Output currently matching node on crash

[njames93]

Summary

Extend D120185 to also log the node being matched on in case of a crash.
This can help if a matcher is causing a crash or there are not enough interesting nodes bound.

[njames93]

Update tests.

[aaron.ballman]

LGTM!

[njames93]

Split PointerUnion to work on 32bit platforms

[njames93]

Use macro to reduce duplication

[aaron.ballman]

This looks.... good?.... to me. :-)

Despite this complicating things by a fair amount, I don't have a better suggestion to offer. LGTM

clang/lib/ASTMatchers/ASTMatchFinder.cpp

771

[njames93]

In D122529#3422158, @aaron.ballman wrote:

This looks.... good?.... to me. :-)

Despite this complicating things by a fair amount, I don't have a better suggestion to offer. LGTM

This was the "nicest" cheapest way I could think to implement this, It's also why I thought it best to resubmit this for review.
Annoyingly this is now suboptimal for 64bit code, but I'm not sure it's worth writing 2 implementations for 64 and 32 bit.

[aaron.ballman]

In D122529#3422688, @njames93 wrote:

In D122529#3422158, @aaron.ballman wrote:

This looks.... good?.... to me. :-)

Despite this complicating things by a fair amount, I don't have a better suggestion to offer. LGTM

This was the "nicest" cheapest way I could think to implement this, It's also why I thought it best to resubmit this for review.
Annoyingly this is now suboptimal for 64bit code, but I'm not sure it's worth writing 2 implementations for 64 and 32 bit.

Given that it has to do with crash analysis (which had better not be performance sensitive!!), I think I agree it's not worth writing two implementations for this.

[sammccall]

In D122529#3422158, @aaron.ballman wrote:

This looks.... good?.... to me. :-)

Despite this complicating things by a fair amount, I don't have a better suggestion to offer. LGTM

(sorry to turn up a year later, just want to see if I'm missing something)

AFAICT there's only ever one CurMatchData object.
In which case squeezing the union discriminator into the low bits saves... 8 bytes of stack over the whole AST traversal.

I ran into this when I tried to add a ninth pointer type. We could use a second bit of the callback, but seems like we can just store the discriminator in an int instead?

clang/lib/ASTMatchers/ASTMatchFinder.cpp

public:
   }
 
   bool TraverseTemplateInstantiations(FunctionTemplateDecl *D) {
     ASTNodeNotSpelledInSourceScope RAII(this, true);
     return RecursiveASTVisitor<MatchASTVisitor>::TraverseTemplateInstantiations(
         D);
   }
 
-  class TraceReporter : llvm::PrettyStackTraceEntry {
+private:
+  bool TraversingASTNodeNotSpelledInSource = false;
+  bool TraversingASTNodeNotAsIs = false;
+  bool TraversingASTChildrenNotSpelledInSource = false;
+
+  class CurMatchData {
   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");
+    CurMatchData() = default;
+
+    template <typename NodeType>
+    void SetCallbackAndRawNode(const MatchCallback *CB, const NodeType &N) {
+      assertEmpty();
+      Callback = CB;
+      MatchingNode = &N;
 
-      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());
+    const MatchCallback *getCallback() const { return Callback; }
+
+    void SetBoundNodes(const BoundNodes &BN) {
+      assertHoldsState();
+      BNodes = &BN;
+
+    void clearBoundNodes() {
+      assertHoldsState();
+      BNodes = nullptr;
+    }
+
+    template <typename T> const T *getNode() const {
+      assertHoldsState();
+      return MatchingNode.dyn_cast<const T *>();
+    }
+
+    const BoundNodes *getBoundNodes() const {
+      assertHoldsState();
+      return BNodes;
+    }
+
+    void reset() {
+      assertHoldsState();
+      Callback = nullptr;
+      MatchingNode = nullptr;
+    }
+
+  private:
+    void assertHoldsState() const {
+      assert(Callback != nullptr && !MatchingNode.isNull());
+    }
+
+    void assertEmpty() const {
+      assert(Callback == nullptr && MatchingNode.isNull() && BNodes == nullptr);
+    }
+
+    const MatchCallback *Callback = nullptr;
+    const BoundNodes *BNodes = nullptr;
+
+    llvm::PointerUnion<
+        const QualType *, const TypeLoc *, const NestedNameSpecifier *,
+        const NestedNameSpecifierLoc *, const CXXCtorInitializer *,
+        const TemplateArgumentLoc *, const Attr *, const DynTypedNode *>
+        MatchingNode;
+  } CurMatchState;
+
+  struct CurMatchRAII {
+    template <typename NodeType>
+    CurMatchRAII(MatchASTVisitor &MV, const MatchCallback *CB,
+                 const NodeType &NT)
+        : MV(MV) {
+      MV.CurMatchState.SetCallbackAndRawNode(CB, NT);
+    }
+
+    ~CurMatchRAII() { MV.CurMatchState.reset(); }
+
+  private:
+    MatchASTVisitor &MV;
+  };
+
+public:
+  class TraceReporter : llvm::PrettyStackTraceEntry {
+    static void dumpNode(const ASTContext &Ctx, const DynTypedNode &Node,
+                         raw_ostream &OS) {
+      if (const auto *D = Node.get<Decl>()) {
+        D->getSourceRange().print(OS, Ctx.getSourceManager());
+      } else if (const auto *S = Node.get<Stmt>()) {
+        S->getSourceRange().print(OS, Ctx.getSourceManager());
+      } else if (const auto *T = Node.get<Type>()) {
+        QualType(T, 0).print(OS, Ctx.getPrintingPolicy());
+      } else if (const auto *QT = Node.get<QualType>()) {
+        QT->print(OS, Ctx.getPrintingPolicy());
+        OS << Node.getNodeKind().asStringRef() << " : ";
+        Node.getSourceRange().print(OS, Ctx.getSourceManager());
+      }
+    }
+
+    static void dumpNodeFromState(const ASTContext &Ctx,
+                                  const CurMatchData &State, raw_ostream &OS) {
+      if (const DynTypedNode *MatchNode = State.getNode<DynTypedNode>()) {
+        dumpNode(Ctx, *MatchNode, OS);
+      } else if (const auto *QT = State.getNode<QualType>()) {
+        dumpNode(Ctx, DynTypedNode::create(*QT), OS);
+      } else if (const auto *TL = State.getNode<TypeLoc>()) {
+        dumpNode(Ctx, DynTypedNode::create(*TL), OS);
+      } else if (const auto *NNS = State.getNode<NestedNameSpecifier>()) {
+        dumpNode(Ctx, DynTypedNode::create(*NNS), OS);
+      } else if (const auto *NNSL = State.getNode<NestedNameSpecifierLoc>()) {
+        dumpNode(Ctx, DynTypedNode::create(*NNSL), OS);
+      } else if (const auto *CtorInit = State.getNode<CXXCtorInitializer>()) {
+        dumpNode(Ctx, DynTypedNode::create(*CtorInit), OS);
+      } else if (const auto *TAL = State.getNode<TemplateArgumentLoc>()) {
+        dumpNode(Ctx, DynTypedNode::create(*TAL), OS);
+      } else if (const auto *At = State.getNode<Attr>()) {
+        dumpNode(Ctx, DynTypedNode::create(*At), OS);
+      }
+    }
+
+  public:
+    TraceReporter(const MatchASTVisitor &MV) : MV(MV) {}
+    void print(raw_ostream &OS) const override {
+      const CurMatchData &State = MV.CurMatchState;
+      const MatchCallback *CB = State.getCallback();
+      if (!CB) {
+        OS << "ASTMatcher: Not currently matching\n";
+        return;
+      }
+
+      assert(MV.ActiveASTContext &&
+             "ActiveASTContext should be set if there is a matched callback");
+
+      ASTContext &Ctx = MV.getASTContext();
+
+      if (const BoundNodes *Nodes = State.getBoundNodes()) {
+        OS << "ASTMatcher: Processing '" << CB->getID() << "' against:\n\t";
+        dumpNodeFromState(Ctx, State, OS);
+        const BoundNodes::IDToNodeMap &Map = Nodes->getMap();
+        if (Map.empty()) {
+          OS << "\nNo bound nodes\n";
+          return;
         }
+        OS << "\n--- Bound Nodes Begin ---\n";
+        for (const auto &Item : Map) {
+          OS << "    " << Item.first << " - { ";
+          dumpNode(Ctx, Item.second, OS);
           OS << " }\n";
         }
         OS << "--- Bound Nodes End ---\n";
+      } else {
+        OS << "ASTMatcher: Matching '" << CB->getID() << "' against:\n\t";
+        dumpNodeFromState(Ctx, State, OS);
+        OS << '\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:
   template <typename T, typename MC>
   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;
+      CurMatchRAII RAII(*this, MP.second, Node);
       if (MP.first.matches(Node, this, &Builder)) {
         MatchVisitor Visitor(*this, ActiveASTContext, MP.second);
         Builder.visitMatches(&Visitor);
       }
     }
   }
 
   void matchWithFilter(const DynTypedNode &DynNode) {
for (unsigned short I : Filter) {
 
       {
         TraversalKindScope RAII(getASTContext(), MP.first.getTraversalKind());
         if (getASTContext().getParentMapContext().traverseIgnored(DynNode) !=
             DynNode)
           continue;
       }
 
+      CurMatchRAII RAII(*this, MP.second, DynNode);
       if (MP.first.matches(DynNode, this, &Builder)) {
         MatchVisitor Visitor(*this, ActiveASTContext, MP.second);
         Builder.visitMatches(&Visitor);
       }
     }
   }
 
   const std::vector<unsigned short> &getFilterForKind(ASTNodeKind Kind) {
#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(MatchASTVisitor::CurMatchData &State, const BoundNodes &BN)
+          : State(State) {
+        State.SetBoundNodes(BN);
       }
 
-      ~CurBoundScope() { MV.CurBoundNodes = nullptr; }
+      ~CurBoundScope() { State.clearBoundNodes(); }
 
     private:
-      MatchASTVisitor &MV;
+      MatchASTVisitor::CurMatchData &State;
     };
 
   public:
     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; }
+        : State(MV.CurMatchState), Context(Context), Callback(Callback) {}
 
     void visitMatch(const BoundNodes& BoundNodesView) override {
       TraversalKindScope RAII(*Context, Callback->getCheckTraversalKind());
-      CurBoundScope RAII2(MV, BoundNodesView);
+      CurBoundScope RAII2(State, BoundNodesView);
       Callback->run(MatchFinder::MatchResult(BoundNodesView, Context));
     }
 
   private:
-    MatchASTVisitor &MV;
+    MatchASTVisitor::CurMatchData &State;
     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) {

clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp

ASSERT_DEBUG_DEATH({
       DeclarationMatcher HasEmptyName = recordDecl(matchesName(""));
       EXPECT_TRUE(notMatches("class X {};", HasEmptyName));
     }, "");
 }
 
 // FIXME: Figure out why back traces aren't being generated on clang builds on
 // windows.
 #if ENABLE_BACKTRACES && (!defined(_MSC_VER) || !defined(__clang__))
+
+AST_MATCHER(Decl, causeCrash) {
+  abort();
+  return true;
+}
+
+TEST(MatcherCrashDeathTest, CrashOnMatcherDump) {
+  llvm::EnablePrettyStackTrace();
+  auto Matcher = testing::HasSubstr(
+      "ASTMatcher: Matching '<unknown>' against:\n\tFunctionDecl foo : "
+      "<input.cc:1:1, col:10>");
+  ASSERT_DEATH(matches("void foo();", functionDecl(causeCrash())), Matcher);
+}
+
 template <typename MatcherT>
 static void crashTestNodeDump(MatcherT Matcher,
                               ArrayRef<StringRef> MatchedNodes,
-                              StringRef Code) {
+                              StringRef Against, StringRef Code) {
   llvm::EnablePrettyStackTrace();
   MatchFinder Finder;
 
   struct CrashCallback : public MatchFinder::MatchCallback {
     void run(const MatchFinder::MatchResult &Result) override { abort(); }
     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"));
+                     ("ASTMatcher: Processing 'CrashTester' against:\n\t" +
+                      Against + "\nNo bound nodes")
+                         .str()));
   } 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 ---"),
+            ("ASTMatcher: Processing 'CrashTester' against:\n\t" + Against +
+             "\n--- Bound Nodes Begin ---")
+                .str()),
         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(), {}, "ForStmt : <input.cc:1:14, col:21>",
+                    "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> }"},
+      "ForStmt : <input.cc:3:5, line:4:5>",
       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> }"},
+      "CXXRecordDecl (anonymous) : <input.cc:1:1, col:36>",
       "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> }"},
+      "CXXRecordDecl Foo : <input.cc:1:1, col:49>",
       "struct Foo { Foo() = default; ~Foo() = default; };");
 }
 #endif // ENABLE_BACKTRACES
 #endif
 
 TEST(ConstructVariadic, MismatchedTypes_Regression) {
   EXPECT_TRUE(
       matches("const int a = 0;", internal::DynTypedMatcher::constructVariadic(