aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
LGTM aside from testing request. ================ Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:313 + AST_POLYMORPHIC_SUPPORTED_TYPES(Decl, Stmt, TypeLoc), + std::string, MacroName) { // Verifies that the statement' beginning and ending are both expanded from ---------------- njames93 wrote: > aaron.ballman wrote: > > 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. > 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`. > ```lang=c++ > 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 > ```lang=c++ > void addVarFromMacro(ASTMatchFinder* Finder, std::string MacroName) { > Finder->addMatcher(varDecl(isExpandedFromMacro(MacroName)), this); > } > ``` Oh, how interesting! I can see now where the lifetime issues come in to play, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90303/new/ https://reviews.llvm.org/D90303 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits