[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-07-28 Thread Michał Górny via Phabricator via cfe-commits 
mgorny added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-07-22 Thread Nathan James via Phabricator via cfe-commits 
njames93 closed this revision.
njames93 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-07-21 Thread Michał Górny via Phabricator via cfe-commits 
mgorny reopened this revision.
mgorny added a comment.
This revision is now accepted and ready to land.

This change broke standalone build of clang. Please fix.




Comment at: clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp:15
 #include "llvm/ADT/Triple.h"
+#include "llvm/Config/config.h"
 #include "llvm/Support/Host.h"

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-25 Thread Nathan James via Phabricator via cfe-commits 
njames93 added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-25 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

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 0x7ff6653b7395 
> (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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-25 Thread Nico Weber via Phabricator via cfe-commits 
thakis added a comment.

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 0x7ff6653b7395 (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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-25 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-24 Thread Nathan James via Phabricator via cfe-commits 
njames93 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-24 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-23 Thread Nathan James via Phabricator via cfe-commits 
njames93 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-23 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-23 Thread Nathan James via Phabricator via cfe-commits 
njames93 added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-21 Thread Nathan James via Phabricator via cfe-commits 
njames93 added a comment.

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, not sure if its 
gn related or just windows in general. I'll revert and investigate


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-21 Thread Nico Weber via Phabricator via cfe-commits 
thakis added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-21 Thread Nathan James via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd89f9e963e49: [ASTMatchers] Output currently processing 
match and nodes on crash (authored by njames93).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

Files:
  clang-tools-extra/docs/ReleaseNotes.rst
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
+++ 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 @@
   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 ) override {
+  LLVM_BUILTIN_TRAP;
+}
+llvm::Optional getCheckTraversalKind() const override {
+  return TK_IgnoreUnlessSpelledInSource;
+}
+StringRef getID() const override { return "CrashTester"; }
+  } Callback;
+  Finder.addMatcher(std::move(Matcher), );
+  if (MatchedNodes.empty()) {
+ASSERT_DEATH(tooling::runToolOnCode(
+ newFrontendActionFactory()->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()->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) {
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ 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 
 #include 
@@ -760,11 +761,67 @@
 D);
   }
 
+  class TraceReporter : llvm::PrettyStackTraceEntry {
+  public:
+TraceReporter(const MatchASTVisitor ) : MV(MV) {}
+void print(raw_ostream )

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-21 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-21 Thread Nathan James via Phabricator via cfe-commits 
njames93 updated this revision to Diff 417041.
njames93 added a comment.

Remove  from Decls that aren't NamedDecls


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

Files:
  clang-tools-extra/docs/ReleaseNotes.rst
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
+++ 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 @@
   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 ) override {
+  LLVM_BUILTIN_TRAP;
+}
+llvm::Optional getCheckTraversalKind() const override {
+  return TK_IgnoreUnlessSpelledInSource;
+}
+StringRef getID() const override { return "CrashTester"; }
+  } Callback;
+  Finder.addMatcher(std::move(Matcher), );
+  if (MatchedNodes.empty()) {
+ASSERT_DEATH(tooling::runToolOnCode(
+ newFrontendActionFactory()->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()->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) {
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ 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 
 #include 
@@ -760,11 +761,67 @@
 D);
   }
 
+  class TraceReporter : llvm::PrettyStackTraceEntry {
+  public:
+TraceReporter(const MatchASTVisitor ) : MV(MV) {}
+void print(raw_ostream ) const override {
+  if (!MV.CurMatched) {
+OS << "ASTMatcher: Not currently matching\n";
+return;
+  }
+

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-21 Thread Nathan James via Phabricator via cfe-commits 
njames93 updated this revision to Diff 416994.
njames93 marked 5 inline comments as done.
njames93 added a comment.

Rework testing infrastructure.
Add more test cases.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

Files:
  clang-tools-extra/docs/ReleaseNotes.rst
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
+++ 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 @@
   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 ) override {
+  LLVM_BUILTIN_TRAP;
+}
+llvm::Optional getCheckTraversalKind() const override {
+  return TK_IgnoreUnlessSpelledInSource;
+}
+StringRef getID() const override { return "CrashTester"; }
+  } Callback;
+  Finder.addMatcher(std::move(Matcher), );
+  if (MatchedNodes.empty()) {
+ASSERT_DEATH(tooling::runToolOnCode(
+ newFrontendActionFactory()->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()->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) {
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ 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 
 #include 
@@ -760,11 +761,67 @@
 D);
   }
 
+  class TraceReporter : llvm::PrettyStackTraceEntry {
+  public:
+TraceReporter(const MatchASTVisitor ) : MV(MV) {}
+void print(raw_ostream ) const override {
+  if (!MV.CurMatched) {
+OS << "ASTMatcher: Not currently

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-03 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

Essentially looks good to me now, thanks!




Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:789
+  else
+OS << " ";
+  D->getSourceRange().print(OS,

njames93 wrote:
> aaron.ballman wrote:
> > Should this be `" : "` instead?
> Good catch
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).



Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:796
+MV.ActiveASTContext->getSourceManager());
+} else if (const auto *T = Item.second.get()) {
+  OS << T->getTypeClassName() << "Type : ";

njames93 wrote:
> aaron.ballman wrote:
> > njames93 wrote:
> > > aaron.ballman wrote:
> > > > 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?)
> > > 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.
> > 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.
> It can hold any kind of node that can be stored in a DynTypedNode, so 
> essentially it's any arbitrary AST node.
Thanks for clarifying that; the `else` look *beautiful* to me! :-D


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-03 Thread Nathan James via Phabricator via cfe-commits 
njames93 added inline comments.



Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:796
+MV.ActiveASTContext->getSourceManager());
+} else if (const auto *T = Item.second.get()) {
+  OS << T->getTypeClassName() << "Type : ";

aaron.ballman wrote:
> njames93 wrote:
> > aaron.ballman wrote:
> > > 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?)
> > 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.
> 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.
It can hold any kind of node that can be stored in a DynTypedNode, so 
essentially it's any arbitrary AST node.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-03 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added inline comments.



Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:796
+MV.ActiveASTContext->getSourceManager());
+} else if (const auto *T = Item.second.get()) {
+  OS << T->getTypeClassName() << "Type : ";

njames93 wrote:
> aaron.ballman wrote:
> > 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?)
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-03 Thread Nathan James via Phabricator via cfe-commits 
njames93 added inline comments.



Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:789
+  else
+OS << " ";
+  D->getSourceRange().print(OS,

aaron.ballman wrote:
> Should this be `" : "` instead?
Good catch



Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:796
+MV.ActiveASTContext->getSourceManager());
+} else if (const auto *T = Item.second.get()) {
+  OS << T->getTypeClassName() << "Type : ";

aaron.ballman wrote:
> 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?)
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-03 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added inline comments.



Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:789
+  else
+OS << " ";
+  D->getSourceRange().print(OS,

Should this be `" : "` instead?



Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:796
+MV.ActiveASTContext->getSourceManager());
+} else if (const auto *T = Item.second.get()) {
+  OS << T->getTypeClassName() << "Type : ";

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?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-03 Thread Nathan James via Phabricator via cfe-commits 
njames93 updated this revision to Diff 412793.
njames93 added a comment.

Update test case


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

Files:
  clang-tools-extra/docs/ReleaseNotes.rst
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
@@ -12,6 +12,7 @@
 #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 "gtest/gtest.h"
@@ -34,6 +35,55 @@
   EXPECT_TRUE(notMatches("class X {};", HasEmptyName));
 }, "");
 }
+
+#if ENABLE_BACKTRACES
+TEST(MatcherCrashDeathTest, CrashOnCallbackDump) {
+  llvm::EnablePrettyStackTrace();
+  MatchFinder Finder;
+
+  struct CrashCallback : public MatchFinder::MatchCallback {
+void run(const MatchFinder::MatchResult ) override {
+  LLVM_BUILTIN_TRAP;
+}
+llvm::Optional getCheckTraversalKind() const override {
+  return TK_IgnoreUnlessSpelledInSource;
+}
+StringRef getID() const override { return "CrashTester"; }
+  } Callback;
+
+  Finder.addMatcher(
+  forStmt(hasLoopInit(declStmt(hasSingleDecl(
+   varDecl(hasType(qualType().bind("QT")),
+   hasType(type().bind("T")),
+   hasInitializer(
+   integerLiteral().bind("IL")))
+   .bind("VD")))
+  .bind("DS")))
+  .bind("FS"),
+  );
+
+  auto Matcher = testing::AllOf(
+  testing::ContainsRegex(
+  "ASTMatcher: Processing 'CrashTester'\n--- Bound Nodes Begin ---"),
+  testing::ContainsRegex("FS - \\{ ForStmt :  \\}"),
+  testing::ContainsRegex("DS - \\{ DeclStmt :  \\}"),
+  testing::ContainsRegex("IL - \\{ IntegerLiteral :  \\}"),
+  testing::ContainsRegex("QT - \\{ QualType : int \\}"),
+  testing::ContainsRegex("T - \\{ BuiltinType : int \\}"),
+  testing::ContainsRegex(
+  "VD - \\{ VarDecl I :  \\}"),
+  testing::ContainsRegex("--- Bound Nodes End ---"));
+
+  ASSERT_DEATH(
+  tooling::runToolOnCode(newFrontendActionFactory()->create(), R"cpp(
+  void foo() {
+for (int I = 0; I < 5; ++I) {
+}
+  }
+  )cpp"),
+  Matcher);
+}
+#endif // ENABLE_BACKTRACES
 #endif
 
 TEST(ConstructVariadic, MismatchedTypes_Regression) {
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ 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 
 #include 
@@ -760,11 +761,66 @@
 D);
   }
 
+  class TraceReporter : llvm::PrettyStackTraceEntry {
+  public:
+TraceReporter(const MatchASTVisitor ) : MV(MV) {}
+void print(raw_ostream ) 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  = MV.CurBoundNodes->getMap();
+  if (Map.empty()) {
+OS << "No bound nodes\n";
+return;
+  }
+  OS << "--- Bound Nodes Begin ---\n";
+  for (const auto  : Map) {
+OS << "" << Item.first << " - { ";
+if (const auto *D = Item.second.get()) {
+  OS << D->getDeclKindName() << "Decl ";
+  if (const auto *ND = dyn_cast(D))
+OS << ND->getDeclName() << " : ";
+  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,

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-03-03 Thread Nathan James via Phabricator via cfe-commits 
njames93 updated this revision to Diff 412772.
njames93 added a comment.
Herald added a project: All.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

Files:
  clang-tools-extra/docs/ReleaseNotes.rst
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
@@ -12,6 +12,7 @@
 #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 "gtest/gtest.h"
@@ -34,6 +35,55 @@
   EXPECT_TRUE(notMatches("class X {};", HasEmptyName));
 }, "");
 }
+
+#if ENABLE_BACKTRACES
+TEST(MatcherCrashDeathTest, CrashOnCallbackDump) {
+  llvm::EnablePrettyStackTrace();
+  MatchFinder Finder;
+
+  struct CrashCallback : public MatchFinder::MatchCallback {
+void run(const MatchFinder::MatchResult ) override {
+  LLVM_BUILTIN_TRAP;
+}
+llvm::Optional getCheckTraversalKind() const override {
+  return TK_IgnoreUnlessSpelledInSource;
+}
+StringRef getID() const override { return "CrashTester"; }
+  } Callback;
+
+  Finder.addMatcher(
+  forStmt(hasLoopInit(declStmt(hasSingleDecl(
+   varDecl(hasType(qualType().bind("QT")),
+   hasType(type().bind("T")),
+   hasInitializer(
+   integerLiteral().bind("IL")))
+   .bind("VD")))
+  .bind("DS")))
+  .bind("FS"),
+  );
+
+  auto Matcher = testing::AllOf(
+  testing::ContainsRegex("ASTMatcher: Processing 'CrashTester'"),
+  testing::ContainsRegex("--- Bound Nodes Begin ---"),
+  testing::ContainsRegex("FS - \\{ ForStmt :  \\}"),
+  testing::ContainsRegex("DS - \\{ DeclStmt :  \\}"),
+  testing::ContainsRegex("IL - \\{ IntegerLiteral :  \\}"),
+  testing::ContainsRegex("QT - \\{ QualType : int \\}"),
+  testing::ContainsRegex("T - \\{ BuiltinType : int \\}"),
+  testing::ContainsRegex(
+  "VD - \\{ VarDecl I :  \\}"),
+  testing::ContainsRegex("--- Bound Nodes End ---"));
+
+  ASSERT_DEATH(
+  tooling::runToolOnCode(newFrontendActionFactory()->create(), R"cpp(
+  void foo() {
+for (int I = 0; I < 5; ++I) {
+}
+  }
+  )cpp"),
+  Matcher);
+}
+#endif // ENABLE_BACKTRACES
 #endif
 
 TEST(ConstructVariadic, MismatchedTypes_Regression) {
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ 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 
 #include 
@@ -760,11 +761,66 @@
 D);
   }
 
+  class TraceReporter : llvm::PrettyStackTraceEntry {
+  public:
+TraceReporter(const MatchASTVisitor ) : MV(MV) {}
+void print(raw_ostream ) 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  = MV.CurBoundNodes->getMap();
+  if (Map.empty()) {
+OS << "No bound nodes\n";
+return;
+  }
+  OS << "--- Bound Nodes Begin ---\n";
+  for (const auto  : Map) {
+OS << "" << Item.first << " - { ";
+if (const auto *D = Item.second.get()) {
+  OS << D->getDeclKindName() << "Decl ";
+  if (const auto *ND = dyn_cast(D))
+OS << ND->getDeclName() << " : ";
+  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());
+

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-02-20 Thread Nathan James via Phabricator via cfe-commits 
njames93 added inline comments.



Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:1536
 void MatchFinder::match(const clang::DynTypedNode , ASTContext ) {
   internal::MatchASTVisitor Visitor(, Options);
   Visitor.set_active_ast_context();

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-02-20 Thread Nathan James via Phabricator via cfe-commits 
njames93 updated this revision to Diff 410139.
njames93 added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

Files:
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/CMakeLists.txt
  clang-tools-extra/test/clang-tidy/CTCrashTestTrace.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/crash-trace.c
  clang-tools-extra/test/lit.cfg.py
  clang/lib/ASTMatchers/ASTMatchFinder.cpp

Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ 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 
 #include 
@@ -760,11 +761,64 @@
 D);
   }
 
+  class TraceReporter : llvm::PrettyStackTraceEntry {
+  public:
+TraceReporter(const MatchASTVisitor ) : MV(MV) {}
+void print(raw_ostream ) const override {
+  if (!MV.CurMatched)
+return;
+  assert(MV.ActiveASTContext &&
+ "ActiveASTContext should be set if there is a matched callback");
+
+  OS << "Processing '" << MV.CurMatched->getID() << "'\n";
+  const BoundNodes::IDToNodeMap  = MV.CurBoundNodes->getMap();
+  if (Map.empty()) {
+OS << "No bound nodes\n";
+return;
+  }
+  OS << "--- Bound Nodes Begin ---\n";
+  for (const auto  : Map) {
+OS << "" << Item.first << " - { ";
+if (const auto *D = Item.second.get()) {
+  OS << D->getDeclKindName() << "Decl ";
+  if (const auto *ND = dyn_cast(D))
+OS << ND->getDeclName() << " : ";
+  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 
+  };
+
 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 +885,7 @@
 Timer.setBucket([MP.second->getID()]);
   BoundNodesTreeBuilder Builder;
   if (MP.first.matches(Node, this, )) {
-MatchVisitor Visitor(ActiveASTContext, MP.second);
+MatchVisitor Visitor(*this, ActiveASTContext, MP.second);
 Builder.visitMatches();
   }
 }
@@ -863,7 +917,7 @@
   }
 
   if (MP.first.matches(DynNode, this, )) {
-MatchVisitor Visitor(ActiveASTContext, MP.second);
+MatchVisitor Visitor(*this, ActiveASTContext, MP.second);
 Builder.visitMatches();
   }
 }
@@ -1049,18 +1103,36 @@
   // 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 , const BoundNodes ) : MV(MV) {
+assert(MV.CurMatched && !MV.CurBoundNodes);
+MV.CurBoundNodes = 
+  }
+
+  ~CurBoundScope() { MV.CurBoundNodes = nullptr; }
+
+private:
+  MatchASTVisitor 
+};
+
   public:
-MatchVisitor(ASTContext* Context,
- MatchFinder::MatchCallback* Callback)
-  : Context(Context),
-Callback(Callback) {}
+MatchVisitor(MatchASTVisitor , ASTContext *Context,
+ MatchFinder::MatchCallback *Callback)
+: MV(MV), Context(Context), Callback(Callback) {
+  assert(!MV.CurMatched && !MV.CurBoundNodes);
+  MV.CurMatched = Callback;
+}
+
+

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-02-19 Thread Nathan James via Phabricator via cfe-commits 
njames93 added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-02-19 Thread Nathan James via Phabricator via cfe-commits 
njames93 created this revision.
njames93 added reviewers: klimek, aaron.ballman, LegalizeAdulthood.
Herald added a subscriber: mgorny.
njames93 requested review of this revision.
Herald added projects: clang, clang-tools-extra.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120185

Files:
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/CMakeLists.txt
  clang-tools-extra/test/clang-tidy/CTCrashTestTrace.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/crash-trace.c
  clang-tools-extra/test/lit.cfg.py
  clang/lib/ASTMatchers/ASTMatchFinder.cpp

Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ 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 
 #include 
@@ -416,6 +417,8 @@
 // matchers.
 class MatchASTVisitor : public RecursiveASTVisitor,
 public ASTMatchFinder {
+  friend class BoundNodeRunStackTrace;
+
 public:
   MatchASTVisitor(const MatchFinder::MatchersByType *Matchers,
   const MatchFinder::MatchFinderOptions )
@@ -765,6 +768,9 @@
   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 +837,7 @@
 Timer.setBucket([MP.second->getID()]);
   BoundNodesTreeBuilder Builder;
   if (MP.first.matches(Node, this, )) {
-MatchVisitor Visitor(ActiveASTContext, MP.second);
+MatchVisitor Visitor(*this, ActiveASTContext, MP.second);
 Builder.visitMatches();
   }
 }
@@ -863,7 +869,7 @@
   }
 
   if (MP.first.matches(DynNode, this, )) {
-MatchVisitor Visitor(ActiveASTContext, MP.second);
+MatchVisitor Visitor(*this, ActiveASTContext, MP.second);
 Builder.visitMatches();
   }
 }
@@ -1049,18 +1055,34 @@
   // Implements a BoundNodesTree::Visitor that calls a MatchCallback with
   // the aggregated bound nodes for each match.
   class MatchVisitor : public BoundNodesTreeBuilder::Visitor {
-  public:
-MatchVisitor(ASTContext* Context,
- MatchFinder::MatchCallback* Callback)
-  : Context(Context),
-Callback(Callback) {}
+struct CurBoundScope {
+  CurBoundScope(MatchASTVisitor , const BoundNodes ) : MV(MV) {
+assert(MV.CurMatched && !MV.CurBoundNodes);
+MV.CurBoundNodes = 
+  }
+
+  ~CurBoundScope() { MV.CurBoundNodes = nullptr; }
 
+private:
+  MatchASTVisitor 
+};
+
+  public:
+MatchVisitor(MatchASTVisitor , 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 
 ASTContext* Context;
 MatchFinder::MatchCallback* Callback;
   };
@@ -1132,6 +1154,54 @@
   MemoizationMap ResultCache;
 };
 
+class BoundNodeRunStackTrace : llvm::PrettyStackTraceEntry {
+public:
+  BoundNodeRunStackTrace(const MatchASTVisitor ) : MV(MV) {}
+  void print(raw_ostream ) const override {
+if (!MV.CurMatched)
+  return;
+assert(MV.ActiveASTContext &&
+   "ActiveASTContext should be set if there is a matched callback");
+
+OS << "Processing '" << MV.CurMatched->getID() << "'\n";
+const BoundNodes::IDToNodeMap  = MV.CurBoundNodes->getMap();
+if (Map.empty()) {
+  OS << "No bound nodes\n";
+  return;
+}
+OS <<