diff --git a/clang-tools-extra/clangd/Headers.cpp b/clang-tools-extra/clangd/Headers.cpp --- a/clang-tools-extra/clangd/Headers.cpp +++ b/clang-tools-extra/clangd/Headers.cpp @@ -23,11 +23,6 @@ namespace clangd { namespace { -bool isMainFile(llvm::StringRef FileName, const SourceManager &SM) { - auto FE = SM.getFileManager().getFile(FileName); - return FE && *FE == SM.getFileEntryForID(SM.getMainFileID()); -} - class RecordHeaders : public PPCallbacks { public: RecordHeaders(const SourceManager &SM, IncludeStructure *Out) diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp --- a/clang-tools-extra/clangd/Hover.cpp +++ b/clang-tools-extra/clangd/Hover.cpp @@ -555,7 +555,14 @@ // Try to get the full definition, not just the name SourceLocation StartLoc = Macro.DefRange.getBegin(); SourceLocation EndLoc = Macro.DefRange.getEnd(); - if (EndLoc.isValid()) { + // Ensure that EndLoc is a valid offset. For example it might come from + // preamble, and source file might've changed, in such a scenario EndLoc still + // stays valid, but getLocForEndOfToken will fail as it is no longer a valid + // offset. + // Note that this check is just to ensure there's text data inside the range. + // It will still succeed even when the data inside the range is irrelevant to + // macro definition. + if (SM.getPresumedLoc(EndLoc, /*UseLineDirectives=*/false).isValid()) { EndLoc = Lexer::getLocForEndOfToken(EndLoc, 0, SM, AST.getLangOpts()); bool Invalid; StringRef Buffer = SM.getBufferData(SM.getFileID(StartLoc), &Invalid); @@ -706,7 +713,7 @@ if (Tok.kind() == tok::identifier) { // Prefer the identifier token as a fallback highlighting range. HighlightRange = Tok.range(SM).toCharRange(SM); - if (auto M = locateMacroAt(Tok, AST.getPreprocessor())) { + if (auto M = locateMacroAt(Tok, AST.getPreprocessor(), AST.getTokens())) { HI = getHoverContents(*M, AST); break; } diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/clang-tools-extra/clangd/Preamble.cpp @@ -423,8 +423,10 @@ // Note that we deliberately ignore conditional directives and undefs to // reduce complexity. The former might cause problems because scanning is // imprecise and might pick directives from disabled regions. - for (const auto &TD : ModifiedScan->TextualDirectives) + for (const auto &TD : ModifiedScan->TextualDirectives) { + Patch << "#line " << TD.DirectiveLine << '\n'; Patch << TD.Text << '\n'; + } } dlog("Created preamble patch: {0}", Patch.str()); Patch.flush(); diff --git a/clang-tools-extra/clangd/SourceCode.h b/clang-tools-extra/clangd/SourceCode.h --- a/clang-tools-extra/clangd/SourceCode.h +++ b/clang-tools-extra/clangd/SourceCode.h @@ -291,12 +291,18 @@ struct DefinedMacro { llvm::StringRef Name; + /// Subject to #line directive substitution. E.g. a macro defined in a + /// built-in file might have an identifier location inside the main file. + SourceLocation IdentLoc; + /// Full range of defintion, including the macro identifier. Isn't subject to + /// #line directive substitutions. CharSourceRange DefRange; }; /// Gets the macro referenced by \p SpelledTok. It must be a spelled token /// aligned to the beginning of an identifier. llvm::Optional locateMacroAt(const syntax::Token &SpelledTok, - Preprocessor &PP); + Preprocessor &PP, + const syntax::TokenBuffer &TB); /// Infers whether this is a header from the FileName and LangOpts (if /// presents). @@ -306,6 +312,9 @@ /// Returns true if the given location is in a generated protobuf file. bool isProtoFile(SourceLocation Loc, const SourceManager &SourceMgr); +/// Checks whether \p FileName is a valid spelling of main file. +bool isMainFile(llvm::StringRef FileName, const SourceManager &SM); + } // namespace clangd } // namespace clang #endif diff --git a/clang-tools-extra/clangd/SourceCode.cpp b/clang-tools-extra/clangd/SourceCode.cpp --- a/clang-tools-extra/clangd/SourceCode.cpp +++ b/clang-tools-extra/clangd/SourceCode.cpp @@ -945,8 +945,14 @@ return Result; } +bool isMainFile(llvm::StringRef FileName, const SourceManager &SM) { + auto FE = SM.getFileManager().getFile(FileName); + return FE && *FE == SM.getFileEntryForID(SM.getMainFileID()); +} + llvm::Optional locateMacroAt(const syntax::Token &SpelledTok, - Preprocessor &PP) { + Preprocessor &PP, + const syntax::TokenBuffer &TB) { SourceLocation Loc = SpelledTok.location(); assert(Loc.isFileID()); const auto &SM = PP.getSourceManager(); @@ -960,13 +966,47 @@ if (SM.getLocForStartOfFile(SM.getFileID(Loc)) != Loc) Loc = Loc.getLocWithOffset(-1); MacroDefinition MacroDef = PP.getMacroDefinitionAtLoc(IdentifierInfo, Loc); - if (auto *MI = MacroDef.getMacroInfo()) - return DefinedMacro{ - IdentifierInfo->getName(), - // MacroInfo::getDefinitionEndLoc returns the location for last token. - CharSourceRange::getTokenRange(MI->getDefinitionLoc(), - MI->getDefinitionEndLoc())}; - return None; + auto *MI = MacroDef.getMacroInfo(); + if (!MI) + return None; + + SourceLocation IdentLoc = MI->getDefinitionLoc(); + + // Macro definitions could be injected through preamble patch. These contain + // line directives to hint their original location in main file. + auto DefFile = SM.getFileID(IdentLoc); + auto IncludeLoc = SM.getIncludeLoc(DefFile); + // Preamble patch is included inside the builtin file. + if (IncludeLoc.isValid() && SM.isWrittenInBuiltinFile(IncludeLoc)) { + auto Presumed = SM.getPresumedLoc(IdentLoc); + // Check that line directive is pointing at main file. + if (Presumed.isValid() && Presumed.getFileID().isInvalid() && + isMainFile(Presumed.getFilename(), SM)) { + auto Presumed = SM.getPresumedLoc(IdentLoc); + // Now find the spelled token for macro identifier. The line looks like: + // # /*comment*/ define /*comment*/ MACRO_IDENT.... + auto LocForStartOfLine = + SM.translateLineCol(SM.getMainFileID(), Presumed.getLine(), 1); + auto SpelledToks = TB.spelledTokens(SM.getMainFileID()); + auto *MacroIdent = llvm::partition_point( + SpelledToks, [&LocForStartOfLine](const syntax::Token &Tok) { + return Tok.location() < LocForStartOfLine; + }); + assert(MacroIdent); + // This loop should terminate in a few iterations, unless something went + // really wrong. + while (MacroIdent < SpelledToks.end() && + MacroIdent->text(SM) != SpelledTok.text(SM)) + ++MacroIdent; + + if (MacroIdent < SpelledToks.end()) + IdentLoc = MacroIdent->location(); + } + } + + return DefinedMacro{IdentifierInfo->getName(), IdentLoc, + CharSourceRange::getTokenRange( + MI->getDefinitionLoc(), MI->getDefinitionEndLoc())}; } llvm::Expected Edit::apply() const { diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp --- a/clang-tools-extra/clangd/XRefs.cpp +++ b/clang-tools-extra/clangd/XRefs.cpp @@ -205,9 +205,10 @@ llvm::Optional locateMacroReferent(const syntax::Token &TouchedIdentifier, ParsedAST &AST, llvm::StringRef MainFilePath) { - if (auto M = locateMacroAt(TouchedIdentifier, AST.getPreprocessor())) { - if (auto Loc = makeLocation(AST.getASTContext(), M->DefRange.getBegin(), - MainFilePath)) { + if (auto M = locateMacroAt(TouchedIdentifier, AST.getPreprocessor(), + AST.getTokens())) { + if (auto Loc = + makeLocation(AST.getASTContext(), M->IdentLoc, MainFilePath)) { LocatedSymbol Macro; Macro.Name = std::string(M->Name); Macro.PreferredDeclaration = *Loc; @@ -749,14 +750,14 @@ llvm::Optional Macro; if (const auto *IdentifierAtCursor = syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens())) { - Macro = locateMacroAt(*IdentifierAtCursor, AST.getPreprocessor()); + Macro = locateMacroAt(*IdentifierAtCursor, AST.getPreprocessor(), + AST.getTokens()); } RefsRequest Req; if (Macro) { // Handle references to macro. - if (auto MacroSID = - getSymbolID(Macro->Name, Macro->DefRange.getBegin(), SM)) { + if (auto MacroSID = getSymbolID(Macro->Name, Macro->IdentLoc, SM)) { // Collect macro references from main file. const auto &IDToRefs = AST.getMacros().MacroRefs; auto Refs = IDToRefs.find(*MacroSID); @@ -871,12 +872,12 @@ if (!IdentifierAtCursor) return Results; - if (auto M = locateMacroAt(*IdentifierAtCursor, AST.getPreprocessor())) { + if (auto M = locateMacroAt(*IdentifierAtCursor, AST.getPreprocessor(), + AST.getTokens())) { SymbolDetails NewMacro; NewMacro.name = std::string(M->Name); llvm::SmallString<32> USR; - if (!index::generateUSRForMacro(NewMacro.name, M->DefRange.getBegin(), SM, - USR)) { + if (!index::generateUSRForMacro(NewMacro.name, M->IdentLoc, SM, USR)) { NewMacro.USR = std::string(USR.str()); NewMacro.ID = SymbolID(NewMacro.USR); } diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp --- a/clang-tools-extra/clangd/refactor/Rename.cpp +++ b/clang-tools-extra/clangd/refactor/Rename.cpp @@ -477,7 +477,7 @@ return makeError(ReasonToReject::NoSymbolFound); // FIXME: Renaming macros is not supported yet, the macro-handling code should // be moved to rename tooling library. - if (locateMacroAt(*IdentifierToken, AST.getPreprocessor())) + if (locateMacroAt(*IdentifierToken, AST.getPreprocessor(), AST.getTokens())) return makeError(ReasonToReject::UnsupportedSymbol); auto DeclsUnderCursor = locateDeclAt(AST, IdentifierToken->location()); diff --git a/clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp b/clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp --- a/clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp +++ b/clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp @@ -91,9 +91,9 @@ ASSERT_TRUE(bool(Loc)); const auto *Id = syntax::spelledIdentifierTouching(*Loc, AST.getTokens()); ASSERT_TRUE(Id); - auto Macro = locateMacroAt(*Id, PP); + auto Macro = locateMacroAt(*Id, PP, AST.getTokens()); assert(Macro); - auto SID = getSymbolID(Macro->Name, Macro->DefRange.getBegin(), SM); + auto SID = getSymbolID(Macro->Name, Macro->IdentLoc, SM); assert(SID); EXPECT_THAT(ExpectedRefs, diff --git a/clang-tools-extra/clangd/unittests/PreambleTests.cpp b/clang-tools-extra/clangd/unittests/PreambleTests.cpp --- a/clang-tools-extra/clangd/unittests/PreambleTests.cpp +++ b/clang-tools-extra/clangd/unittests/PreambleTests.cpp @@ -9,9 +9,12 @@ #include "Annotations.h" #include "Compiler.h" #include "Headers.h" +#include "Hover.h" #include "Preamble.h" +#include "SourceCode.h" #include "TestFS.h" #include "TestTU.h" +#include "clang/Format/Format.h" #include "clang/Frontend/PrecompiledPreamble.h" #include "clang/Lex/PreprocessorOptions.h" #include "llvm/ADT/STLExtras.h" @@ -198,7 +201,7 @@ ADD_FAILURE() << "Failed to build compiler invocation"; return llvm::None; } - return ParsedAST::build(testPath("main.cpp"), TU.inputs(), std::move(CI), {}, + return ParsedAST::build(testPath(TU.Filename), TU.inputs(), std::move(CI), {}, BaselinePreamble); } @@ -243,6 +246,122 @@ EXPECT_THAT(AST->getDiagnostics(), Not(Contains(Field(&Diag::Range, Modified.range())))); } + +TEST(PreamblePatchTest, LocateMacroAtWorks) { + struct { + llvm::StringLiteral Baseline; + llvm::StringLiteral Modified; + } Cases[] = { + // Addition of new directive + { + "", + R"cpp( + #define $def^FOO + $use^FOO)cpp", + }, + // Available inside preamble section + { + "", + R"cpp( + #define $def^FOO + #undef $use^FOO)cpp", + }, + // Available after undef, as we don't patch those + { + "", + R"cpp( + #define $def^FOO + #undef FOO + $use^FOO)cpp", + }, + // Identifier on a different line + { + "", + R"cpp( + #define \ + $def^FOO + $use^FOO)cpp", + }, + // In presence of comment tokens + { + "", + R"cpp( + #\ + define /* FOO */\ + /* FOO */ $def^FOO + $use^FOO)cpp", + }, + // Moved around + { + "#define FOO", + R"cpp( + #define BAR + #define $def^FOO + $use^FOO)cpp", + }, + }; + for (const auto &Case : Cases) { + SCOPED_TRACE(Case.Modified); + llvm::Annotations Modified(Case.Modified); + auto AST = createPatchedAST(Case.Baseline, Modified.code()); + ASSERT_TRUE(AST); + + const auto &SM = AST->getSourceManager(); + auto *MacroTok = AST->getTokens().spelledTokenAt( + SM.getComposedLoc(SM.getMainFileID(), Modified.point("use"))); + ASSERT_TRUE(MacroTok); + + auto FoundMacro = + locateMacroAt(*MacroTok, AST->getPreprocessor(), AST->getTokens()); + ASSERT_TRUE(FoundMacro); + EXPECT_THAT(FoundMacro->Name, "FOO"); + + auto MacroLoc = FoundMacro->IdentLoc; + EXPECT_EQ(SM.getFileID(MacroLoc), SM.getMainFileID()); + EXPECT_EQ(SM.getFileOffset(MacroLoc), Modified.point("def")); + } +} + +TEST(PreamblePatchTest, LocateMacroAtDeletion) { + { + // We don't patch deleted define directives, make sure we don't crash. + llvm::StringLiteral Baseline = "#define FOO"; + llvm::Annotations Modified("^FOO"); + + auto AST = createPatchedAST(Baseline, Modified.code()); + ASSERT_TRUE(AST); + + const auto &SM = AST->getSourceManager(); + auto *MacroTok = AST->getTokens().spelledTokenAt( + SM.getComposedLoc(SM.getMainFileID(), Modified.point())); + ASSERT_TRUE(MacroTok); + + auto FoundMacro = + locateMacroAt(*MacroTok, AST->getPreprocessor(), AST->getTokens()); + ASSERT_TRUE(FoundMacro); + EXPECT_THAT(FoundMacro->Name, "FOO"); + auto HI = + getHover(*AST, offsetToPosition(Modified.code(), Modified.point()), + format::getLLVMStyle(), nullptr); + ASSERT_TRUE(HI); + EXPECT_THAT(HI->Definition, testing::IsEmpty()); + } + + { + // Offset is valid, but underlying text is different. + llvm::StringLiteral Baseline = "#define FOO"; + Annotations Modified(R"cpp(#define BAR + ^FOO")cpp"); + + auto AST = createPatchedAST(Baseline, Modified.code()); + ASSERT_TRUE(AST); + + auto HI = getHover(*AST, Modified.point(), format::getLLVMStyle(), nullptr); + ASSERT_TRUE(HI); + EXPECT_THAT(HI->Definition, "#define BAR"); + } +} + } // namespace } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp b/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp --- a/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp +++ b/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp @@ -514,7 +514,7 @@ ASSERT_TRUE(bool(CurLoc)); const auto *Id = syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens()); ASSERT_TRUE(Id); - auto Result = locateMacroAt(*Id, AST.getPreprocessor()); + auto Result = locateMacroAt(*Id, AST.getPreprocessor(), AST.getTokens()); ASSERT_TRUE(Result); EXPECT_THAT(*Result, MacroName("MACRO")); } @@ -528,7 +528,7 @@ ASSERT_TRUE(bool(CurLoc)); const auto *Id = syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens()); ASSERT_TRUE(Id); - auto Result = locateMacroAt(*Id, AST.getPreprocessor()); + auto Result = locateMacroAt(*Id, AST.getPreprocessor(), AST.getTokens()); ASSERT_TRUE(Result); EXPECT_THAT(*Result, MacroName("MACRO")); }