llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Nathan Ridge (HighCommander4) <details> <summary>Changes</summary> Fixes https://github.com/llvm/llvm-project/issues/99617 --- Full diff: https://github.com/llvm/llvm-project/pull/106329.diff 4 Files Affected: - (modified) clang-tools-extra/clangd/CollectMacros.cpp (+1) - (modified) clang-tools-extra/clangd/CollectMacros.h (+8) - (modified) clang-tools-extra/clangd/ParsedAST.cpp (+7-1) - (modified) clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp (+17) ``````````diff diff --git a/clang-tools-extra/clangd/CollectMacros.cpp b/clang-tools-extra/clangd/CollectMacros.cpp index c5ba8d903ba482..96298ee3ea50ae 100644 --- a/clang-tools-extra/clangd/CollectMacros.cpp +++ b/clang-tools-extra/clangd/CollectMacros.cpp @@ -32,6 +32,7 @@ void CollectMainFileMacros::add(const Token &MacroNameTok, const MacroInfo *MI, if (Loc.isInvalid() || Loc.isMacroID()) return; + assert(isInsideMainFile(Loc, SM)); auto Name = MacroNameTok.getIdentifierInfo()->getName(); Out.Names.insert(Name); size_t Start = SM.getFileOffset(Loc); diff --git a/clang-tools-extra/clangd/CollectMacros.h b/clang-tools-extra/clangd/CollectMacros.h index e3900c08e5df7b..e7198641d8d53c 100644 --- a/clang-tools-extra/clangd/CollectMacros.h +++ b/clang-tools-extra/clangd/CollectMacros.h @@ -82,6 +82,14 @@ class CollectMainFileMacros : public PPCallbacks { void SourceRangeSkipped(SourceRange R, SourceLocation EndifLoc) override; + // Called when the AST build is done to disable further recording + // of macros by this class. This is needed because some clang-tidy + // checks can trigger PP callbacks by calling directly into the + // preprocessor. Such calls are not interleaved with FileChanged() + // in the expected way, leading this class to erroneously process + // macros that are not in the main file. + void doneParse() { InMainFile = false; } + private: void add(const Token &MacroNameTok, const MacroInfo *MI, bool IsDefinition = false, bool InConditionalDirective = false); diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index 2bd1fbcad2ada0..ee012846da9f5e 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -681,7 +681,9 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, Marks = Patch->marks(); } auto &PP = Clang->getPreprocessor(); - PP.addPPCallbacks(std::make_unique<CollectMainFileMacros>(PP, Macros)); + auto MacroCollector = std::make_unique<CollectMainFileMacros>(PP, Macros); + auto *MacroCollectorPtr = MacroCollector.get(); // so we can call doneParse() + PP.addPPCallbacks(std::move(MacroCollector)); PP.addPPCallbacks( collectPragmaMarksCallback(Clang->getSourceManager(), Marks)); @@ -702,6 +704,10 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, log("Execute() failed when building AST for {0}: {1}", MainInput.getFile(), toString(std::move(Err))); + // Disable the macro collector for the remainder of this function, e.g. + // clang-tidy checkers. + MacroCollectorPtr->doneParse(); + // We have to consume the tokens before running clang-tidy to avoid collecting // tokens from running the preprocessor inside the checks (only // modernize-use-trailing-return-type does that today). diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp index 25d2f03e0b366b..096f77e414f5a5 100644 --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -940,6 +940,23 @@ TEST(DiagnosticTest, ClangTidySelfContainedDiagsFormatting) { withFix(equalToFix(ExpectedFix2)))))); } +TEST(DiagnosticsTest, ClangTidyCallingIntoPreprocessor) { + std::string Main = R"cpp( + extern "C" { + #include "b.h" + } + )cpp"; + std::string Header = R"cpp( + #define EXTERN extern + EXTERN int waldo(); + )cpp"; + auto TU = TestTU::withCode(Main); + TU.AdditionalFiles["b.h"] = Header; + TU.ClangTidyProvider = addTidyChecks("modernize-use-trailing-return-type"); + // Check that no assertion failures occur during the build + TU.build(); +} + TEST(DiagnosticsTest, Preprocessor) { // This looks like a preamble, but there's an #else in the middle! // Check that: `````````` </details> https://github.com/llvm/llvm-project/pull/106329 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
