https://github.com/alexander-shaposhnikov created https://github.com/llvm/llvm-project/pull/215980
Address the FIXME in computeIncludeCleanerFindings: locations inside the preamble patch are mapped back into the main file via their presumed locations. This fixes hang in include-cleaner when mapping refs through a stale preamble. Test plan: ninja check-all >From e811727a91c0668c673b0fec255db220b7653a7b Mon Sep 17 00:00:00 2001 From: Alexander Shaposhnikov <[email protected]> Date: Thu, 13 Aug 2026 00:54:44 -0700 Subject: [PATCH] [clangd] Fix hang in include-cleaner when mapping refs through a stale preamble --- clang-tools-extra/clangd/IncludeCleaner.cpp | 27 ++++++-- .../clangd/unittests/IncludeCleanerTests.cpp | 61 +++++++++++++++++++ 2 files changed, 84 insertions(+), 4 deletions(-) diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp index 382ea3ffe342b..c9b8f8cd2db00 100644 --- a/clang-tools-extra/clangd/IncludeCleaner.cpp +++ b/clang-tools-extra/clangd/IncludeCleaner.cpp @@ -445,16 +445,35 @@ computeIncludeCleanerFindings(ParsedAST &AST, bool AnalyzeAngledIncludes) { // offsets could lead into crashes in presence of stale preambles. Hence // we use "getFileLoc" instead to make sure it always points into main // file. - // FIXME: Use presumed locations to map such usages back to patched - // locations safely. auto Loc = SM.getFileLoc(Ref.RefLocation); // File locations can be outside of the main file if macro is expanded // through an #include. - while (SM.getFileID(Loc) != SM.getMainFileID()) + while (Loc.isValid() && SM.getFileID(Loc) != SM.getMainFileID()) { + // Use presumed locations to map locations from the preamble section + // and the preamble patch (which is not included from the main file) + // back into the main file. + PresumedLoc Presumed = SM.getPresumedLoc(Loc); + if (Presumed.isValid() && Presumed.getLine() != 0 && + Presumed.getColumn() != 0 && + Presumed.getFilename() == + SM.getFileEntryRefForID(SM.getMainFileID())->getName()) { + Loc = SM.translateLineCol(SM.getMainFileID(), Presumed.getLine(), + Presumed.getColumn()); + break; + } Loc = SM.getIncludeLoc(SM.getFileID(Loc)); + } + // Bail out if the chain didn't reach the main file, e.g. a file + // entered from the command line is rooted at the predefines buffer. + if (Loc.isInvalid()) + return; auto TouchingTokens = syntax::spelledTokensTouching(Loc, AST.getTokens()); - assert(!TouchingTokens.empty()); + // Locations translated through a stale preamble refer to the baseline + // contents and are not guaranteed to point at a token in the current + // contents. + if (TouchingTokens.empty()) + return; // Loc points to the start offset of the ref token, here we use the last // element of the TouchingTokens, e.g. avoid getting the "::" for // "ns::^abc". diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp index ec733cbe9c42d..4e398f7f3a386 100644 --- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp +++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "Annotations.h" +#include "Compiler.h" #include "Diagnostics.h" #include "IncludeCleaner.h" #include "ParsedAST.h" @@ -525,6 +526,66 @@ TEST(IncludeCleaner, MissingIncludesAreUnique) { EXPECT_EQ(halfOpenToRange(SM, RefRange.toCharRange(SM)), MainFile.range()); } +TEST(IncludeCleaner, NoHangOnRefExpandedInsidePreamblePatchInclude) { + // Reproduces a hang in computeIncludeCleanerFindings, which maps a symbol + // reference back to the main file by walking up include locations: + // + // while (SM.getFileID(Loc) != SM.getMainFileID()) + // Loc = SM.getIncludeLoc(SM.getFileID(Loc)); + // + // With a stale preamble, an include that is new in the modified contents is + // re-injected through the preamble patch, so its include location is the + // patch file, which in turn is entered from the predefines buffer. That + // chain never reaches the main file: + // + // rettype.inc -> __preamble_patch__.h -> <built-in> -> invalid -> invalid + // + // getIncludeLoc() of an invalid FileID is invalid again, so the loop spins + // forever. The patch carries #line directives mapping its contents back to + // the main file, which is how the reference should be (and now is) mapped. + // + // Here the baseline preamble defines RET, and the modified contents also + // include "rettype.inc". The type token `Foo` is spelled in the baseline + // preamble (which is what makes include-cleaner report the reference at + // all) but is *expanded* inside rettype.inc, while the name of the + // declaration it belongs to is written in the main file, which is what puts + // the declaration into the list of local top-level decls. + llvm::StringLiteral Baseline = R"cpp(// comment +#include "all.h" +#define RET Foo +)cpp"; + Annotations Modified(R"cpp(// comment +#include "all.h" +#define RET Foo +#include [["rettype.inc"]] +plugin_callback(); +)cpp"); + + TestTU TU; + TU.AdditionalFiles["foo.h"] = guard("struct Foo {};"); + TU.AdditionalFiles["all.h"] = guard("#include \"foo.h\""); + TU.AdditionalFiles["rettype.inc"] = "RET\n"; + + TU.Code = Baseline.str(); + auto BaselinePreamble = TU.preamble(); + ASSERT_TRUE(BaselinePreamble); + + IgnoreDiagnostics Diags; + MockFS FS; + TU.Code = Modified.code().str(); + auto CI = buildCompilerInvocation(TU.inputs(FS), Diags); + ASSERT_TRUE(CI); + auto AST = ParsedAST::build(testPath(TU.Filename), TU.inputs(FS), + std::move(CI), {}, std::move(BaselinePreamble)); + ASSERT_TRUE(AST); + auto Findings = computeIncludeCleanerFindings(*AST).MissingIncludes; + ASSERT_THAT(Findings, testing::SizeIs(1)); + auto RefRange = Findings.front().SymRefRange; + const auto &SM = AST->getSourceManager(); + EXPECT_EQ(RefRange.file(), SM.getMainFileID()); + EXPECT_EQ(halfOpenToRange(SM, RefRange.toCharRange(SM)), Modified.range()); +} + TEST(IncludeCleaner, NoCrash) { TestTU TU; Annotations MainCode(R"cpp( _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
