[ASTMatchers] Made isExpandedFromMacro Polymorphic

[njames93]

Summary

Made the isExpandedFromMacro matcher work on Stmt's, TypeLocs and Decls in line with the other macro expansion matchers.
Also tweaked it to take a std::string instead of a StringRef.
This prevents potential use-after-free bugs if the matcher is created with a string thats destroyed before the matcher finishes matching.

[aaron.ballman]

clang/include/clang/ASTMatchers/ASTMatchers.h

313

You mentioned that the change from StringRef to std::string was to avoid lifetime issues while matching, but I'm wondering if you can expound on that situation a bit more. I would have assumed that any memoization that involves StringRef would be responsible for the lifetime issues rather than the matchers themselves, but maybe I'm thinking about a different way you can hit lifetime issues than you are.

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

150

Can you also add a test for type-based matching?

[njames93]

clang/include/clang/ASTMatchers/ASTMatchers.h

313

Take this as contrived, however using ASAN reports a heap-use-after-free for this code when isExpandedFromMacro takes a StringRef but works fine when using std::string.

TEST(IsExpandedFromMacro, IsExpandedFromMacro_MatchesDecls) {
  auto Matcher = isExpandedFromMacro(std::string("MY_MACRO_OVERFLOW_SMALL_STRING_SIZE"));// <-Temporary string created and destroyed here.
  StringRef input = R"cc(
#define MY_MACRO_OVERFLOW_SMALL_STRING_SIZE(a) int i = a;
    void Test() { MY_MACRO_OVERFLOW_SMALL_STRING_SIZE(4); }
  )cc";
  EXPECT_TRUE(matches(input, varDecl(Matcher))); // <- heap-use-after-free detected down the callstack from here.
}

Obviously this is very contrived to ensure asan detects the use-after-free and to ensure the temporary string is destroyed before the matcher. For these tests it doesn't look like a problem, but in other instances these matchers will outlive a string being passed to them

void addVarFromMacro(ASTMatchFinder* Finder, std::string MacroName) {
  Finder->addMatcher(varDecl(isExpandedFromMacro(MacroName)), this);
}

[aaron.ballman]

LGTM aside from testing request.

clang/include/clang/ASTMatchers/ASTMatchers.h

313

Oh, how interesting! I can see now where the lifetime issues come in to play, thanks!

clang/docs/LibASTMatchersReference.html

 Given
   __attribute__((device)) void f() { ... }
 decl(hasAttr(clang::attr::CUDADevice)) matches the function declaration of
 f. If the matcher is used from clang-query, attr::Kind parameter should be
 passed as a quoted string. e.g., hasAttr("attr::CUDADevice").
 </pre></td></tr>
 
 
+<tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1Decl.html">Decl</a>&gt;</td><td class="name" onclick="toggle('isExpandedFromMacro0')"><a name="isExpandedFromMacro0Anchor">isExpandedFromMacro</a></td><td>std::string MacroName</td></tr>
+<tr><td colspan="4" class="doc" id="isExpandedFromMacro0"><pre>Matches statements that are (transitively) expanded from the named macro.
+Does not match if only part of the statement is expanded from that macro or
+if different parts of the the statement are expanded from different
+appearances of the macro.
+</pre></td></tr>
+
+
 <tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1Decl.html">Decl</a>&gt;</td><td class="name" onclick="toggle('isExpansionInFileMatching0')"><a name="isExpansionInFileMatching0Anchor">isExpansionInFileMatching</a></td><td>StringRef RegExp, Regex::RegexFlags Flags = NoFlags</td></tr>
 <tr><td colspan="4" class="doc" id="isExpansionInFileMatching0"><pre>Matches AST nodes that were expanded within files whose name is
 partially matching a given regex.
 
 Example matches Y but not X
     (matcher = cxxRecordDecl(isExpansionInFileMatching("AST.*"))
   #include "ASTMatcher.h"
   class X {};

 
 <tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1Stmt.html">Stmt</a>&gt;</td><td class="name" onclick="toggle('equalsNode1')"><a name="equalsNode1Anchor">equalsNode</a></td><td>const Stmt* Other</td></tr>
 <tr><td colspan="4" class="doc" id="equalsNode1"><pre>Matches if a node equals another node.
 
 Stmt has pointer identity in the AST.
 </pre></td></tr>
 
 
-<tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1Stmt.html">Stmt</a>&gt;</td><td class="name" onclick="toggle('isExpandedFromMacro0')"><a name="isExpandedFromMacro0Anchor">isExpandedFromMacro</a></td><td>llvm::StringRef MacroName</td></tr>
-<tr><td colspan="4" class="doc" id="isExpandedFromMacro0"><pre>Matches statements that are (transitively) expanded from the named macro.
+<tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1Stmt.html">Stmt</a>&gt;</td><td class="name" onclick="toggle('isExpandedFromMacro1')"><a name="isExpandedFromMacro1Anchor">isExpandedFromMacro</a></td><td>std::string MacroName</td></tr>
+<tr><td colspan="4" class="doc" id="isExpandedFromMacro1"><pre>Matches statements that are (transitively) expanded from the named macro.
 Does not match if only part of the statement is expanded from that macro or
 if different parts of the the statement are expanded from different
 appearances of the macro.
-
-FIXME: Change to be a polymorphic matcher that works on any syntactic
-node. There's nothing `Stmt`-specific about it.
 </pre></td></tr>
 
 
 <tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1Stmt.html">Stmt</a>&gt;</td><td class="name" onclick="toggle('isExpansionInFileMatching1')"><a name="isExpansionInFileMatching1Anchor">isExpansionInFileMatching</a></td><td>StringRef RegExp, Regex::RegexFlags Flags = NoFlags</td></tr>
 <tr><td colspan="4" class="doc" id="isExpansionInFileMatching1"><pre>Matches AST nodes that were expanded within files whose name is
 partially matching a given regex.
 
 Example matches Y but not X

 Given
   template&lt;typename T&gt; struct C {};
   C&lt;int&gt; c;
 classTemplateSpecializationDecl(templateArgumentCountIs(1))
   matches C&lt;int&gt;.
 </pre></td></tr>
 
 
+<tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1TypeLoc.html">TypeLoc</a>&gt;</td><td class="name" onclick="toggle('isExpandedFromMacro2')"><a name="isExpandedFromMacro2Anchor">isExpandedFromMacro</a></td><td>std::string MacroName</td></tr>
+<tr><td colspan="4" class="doc" id="isExpandedFromMacro2"><pre>Matches statements that are (transitively) expanded from the named macro.
+Does not match if only part of the statement is expanded from that macro or
+if different parts of the the statement are expanded from different
+appearances of the macro.
+</pre></td></tr>
+
+
 <tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1TypeLoc.html">TypeLoc</a>&gt;</td><td class="name" onclick="toggle('isExpansionInFileMatching2')"><a name="isExpansionInFileMatching2Anchor">isExpansionInFileMatching</a></td><td>StringRef RegExp, Regex::RegexFlags Flags = NoFlags</td></tr>
 <tr><td colspan="4" class="doc" id="isExpansionInFileMatching2"><pre>Matches AST nodes that were expanded within files whose name is
 partially matching a given regex.
 
 Example matches Y but not X
     (matcher = cxxRecordDecl(isExpansionInFileMatching("AST.*"))
   #include "ASTMatcher.h"
   class X {};

clang/include/clang/ASTMatchers/ASTMatchers.h

AST_POLYMORPHIC_MATCHER_REGEX(isExpansionInFileMatching,
   auto Filename = FileEntry->getName();
   return RegExp->match(Filename);
 }
 
 /// Matches statements that are (transitively) expanded from the named macro.
 /// Does not match if only part of the statement is expanded from that macro or
 /// if different parts of the the statement are expanded from different
 /// appearances of the macro.
-///
-/// FIXME: Change to be a polymorphic matcher that works on any syntactic
-/// node. There's nothing `Stmt`-specific about it.
-AST_MATCHER_P(Stmt, isExpandedFromMacro, llvm::StringRef, MacroName) {
+AST_POLYMORPHIC_MATCHER_P(isExpandedFromMacro,
+                          AST_POLYMORPHIC_SUPPORTED_TYPES(Decl, Stmt, TypeLoc),
+                          std::string, MacroName) {
   // Verifies that the statement' beginning and ending are both expanded from
   // the same instance of the given macro.
   auto& Context = Finder->getASTContext();
   llvm::Optional<SourceLocation> B =
       internal::getExpansionLocOfMacro(MacroName, Node.getBeginLoc(), Context);
   if (!B) return false;
   llvm::Optional<SourceLocation> E =
       internal::getExpansionLocOfMacro(MacroName, Node.getEndLoc(), Context);

clang/lib/ASTMatchers/GtestMatchers.cpp

 // macro. The more uncommon the identified elements, the more efficient this
 // process will be.
 //
 // We use this approach to implement the derived matchers gtestAssert and
 // gtestExpect.
 internal::BindableMatcher<Stmt> gtestAssert(GtestCmp Cmp, StatementMatcher Left,
                                             StatementMatcher Right) {
   return callExpr(callee(getComparisonDecl(Cmp)),
-                  isExpandedFromMacro(getAssertMacro(Cmp)),
+                  isExpandedFromMacro(getAssertMacro(Cmp).str()),
                   hasArgument(2, Left), hasArgument(3, Right));
 }
 
 internal::BindableMatcher<Stmt> gtestExpect(GtestCmp Cmp, StatementMatcher Left,
                                             StatementMatcher Right) {
   return callExpr(callee(getComparisonDecl(Cmp)),
-                  isExpandedFromMacro(getExpectMacro(Cmp)),
+                  isExpandedFromMacro(getExpectMacro(Cmp).str()),
                   hasArgument(2, Left), hasArgument(3, Right));
 }
 
 } // end namespace ast_matchers
 } // end namespace clang

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

 TEST_P(ASTMatchersTest, IsExpandedFromMacro_NotMatchesDifferentInstances) {
   StringRef input = R"cc(
 #define FOUR 4
     void Test() { FOUR + FOUR; }
   )cc";
   EXPECT_TRUE(notMatches(input, binaryOperator(isExpandedFromMacro("FOUR"))));
 }
 
+TEST(IsExpandedFromMacro, IsExpandedFromMacro_MatchesDecls) {
+  StringRef input = R"cc(
+#define MY_MACRO(a) int i = a;
+    void Test() { MY_MACRO(4); }
+  )cc";
+  EXPECT_TRUE(matches(input, varDecl(isExpandedFromMacro("MY_MACRO"))));
+}
+
 TEST_P(ASTMatchersTest, AllOf) {
   const char Program[] = "struct T { };"
                          "int f(int, struct T*, int, int);"
                          "void g(int x) { struct T t; f(x, &t, 3, 4); }";
   EXPECT_TRUE(matches(
       Program, callExpr(allOf(callee(functionDecl(hasName("f"))),
                               hasArgument(0, declRefExpr(to(varDecl())))))));
   EXPECT_TRUE(matches(