From cff34ccb605aa78030cd51cfe44362ed1c1fb80b Mon Sep 17 00:00:00 2001 From: Nathan James Date: Mon, 21 Mar 2022 22:29:22 +0000 Subject: [PATCH] Revert "[ASTMatchers] Output currently processing match and nodes on crash" This reverts commit d89f9e963e4979466193dc6a15fe091bf7ca5c47. --- clang-tools-extra/docs/ReleaseNotes.rst | 3 - clang/lib/ASTMatchers/ASTMatchFinder.cpp | 88 ++----------------- .../ASTMatchers/ASTMatchersInternalTest.cpp | 83 ----------------- 3 files changed, 6 insertions(+), 168 deletions(-) diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index ca4aea676eda3..551e36dea0937 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -96,9 +96,6 @@ 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 index 70598460151ae..b19a7fe3be04c 100644 --- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp +++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp @@ -21,7 +21,6 @@ #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 #include @@ -761,67 +760,11 @@ class MatchASTVisitor : public RecursiveASTVisitor, 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()) { - OS << D->getDeclKindName() << "Decl "; - if (const auto *ND = dyn_cast(D)) { - ND->printQualifiedName(OS); - OS << " : "; - } else - OS << ": "; - D->getSourceRange().print(OS, - MV.ActiveASTContext->getSourceManager()); - } else if (const auto *S = Item.second.get()) { - OS << S->getStmtClassName() << " : "; - S->getSourceRange().print(OS, - MV.ActiveASTContext->getSourceManager()); - } else if (const auto *T = Item.second.get()) { - OS << T->getTypeClassName() << "Type : "; - QualType(T, 0).print(OS, MV.ActiveASTContext->getPrintingPolicy()); - } else if (const auto *QT = Item.second.get()) { - 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) { @@ -888,7 +831,7 @@ class MatchASTVisitor : public RecursiveASTVisitor, Timer.setBucket(&TimeByBucket[MP.second->getID()]); BoundNodesTreeBuilder Builder; if (MP.first.matches(Node, this, &Builder)) { - MatchVisitor Visitor(*this, ActiveASTContext, MP.second); + MatchVisitor Visitor(ActiveASTContext, MP.second); Builder.visitMatches(&Visitor); } } @@ -920,7 +863,7 @@ class MatchASTVisitor : public RecursiveASTVisitor, } if (MP.first.matches(DynNode, this, &Builder)) { - MatchVisitor Visitor(*this, ActiveASTContext, MP.second); + MatchVisitor Visitor(ActiveASTContext, MP.second); Builder.visitMatches(&Visitor); } } @@ -1106,36 +1049,18 @@ class MatchASTVisitor : public RecursiveASTVisitor, // 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(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; } + MatchVisitor(ASTContext* Context, + MatchFinder::MatchCallback* Callback) + : Context(Context), + Callback(Callback) {} 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; }; @@ -1545,7 +1470,6 @@ 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 index 728ba77077f8a..2766065f9e5d1 100644 --- a/clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp +++ b/clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp @@ -12,10 +12,8 @@ #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 { @@ -36,87 +34,6 @@ TEST(HasNameDeathTest, DiesOnEmptyPattern) { EXPECT_TRUE(notMatches("class X {};", HasEmptyName)); }, ""); } - -#if ENABLE_BACKTRACES - -template -static void crashTestNodeDump(MatcherT Matcher, - ArrayRef MatchedNodes, - StringRef Code) { - llvm::EnablePrettyStackTrace(); - MatchFinder Finder; - - struct CrashCallback : public MatchFinder::MatchCallback { - void run(const MatchFinder::MatchResult &Result) override { - LLVM_BUILTIN_TRAP; - } - llvm::Optional 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>> - 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 : }", - "DS - { DeclStmt : }", - "IL - { IntegerLiteral : }", "QT - { QualType : int }", - "T - { BuiltinType : int }", - "VD - { VarDecl I : }"}, - 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) : }", - "Op+ - { CXXMethodDecl (anonymous struct)::operator+ : }"}, - "struct { int operator+(int) const; } Unnamed;"); - crashTestNodeDump( - cxxRecordDecl(hasMethod(cxxConstructorDecl(isDefaulted()).bind("Ctor")), - hasMethod(cxxDestructorDecl(isDefaulted()).bind("Dtor"))), - {"Ctor - { CXXConstructorDecl Foo::Foo : }", - "Dtor - { CXXDestructorDecl Foo::~Foo : }"}, - "struct Foo { Foo() = default; ~Foo() = default; };"); -} -#endif // ENABLE_BACKTRACES #endif TEST(ConstructVariadic, MismatchedTypes_Regression) {