https://github.com/steakhal updated https://github.com/llvm/llvm-project/pull/214461
From a9207acd87fea7a5933312f33ac6a9e821200c58 Mon Sep 17 00:00:00 2001 From: Balazs Benics <[email protected]> Date: Thu, 6 Aug 2026 10:55:52 +0100 Subject: [PATCH 1/5] [clang][NFC] Extract getExpansionRangeInFile out of the diagnostic renderers Prep for the following commits, which fix crashes in the analyzer's SARIF and HTML output on ranges that end inside a macro expansion. Fixing them means mapping such a range into the reported file - the normalization the frontend text and SARIF renderers already do, and that the two analyzer consumers each do differently and incorrectly. Hoist that logic into getExpansionRangeInFile, beside the DiagnosticRenderer base both frontend renderers derive from, so the fixes reuse one implementation instead of adding two more copies. TextDiagnostic and SARIFDiagnostic move onto it here with no behavior change; the analyzer consumers follow in later commits. getFileID() replaces SARIFDiagnostic's getDecomposedLoc(...).first - equivalent here, and what TextDiagnostic has used since c113cbb51005. Assisted-By: claude --- .../clang/Frontend/DiagnosticRenderer.h | 7 ++++++ clang/lib/Frontend/DiagnosticRenderer.cpp | 16 +++++++++++++ clang/lib/Frontend/SARIFDiagnostic.cpp | 24 ++++++------------- clang/lib/Frontend/TextDiagnostic.cpp | 17 +++++-------- 4 files changed, 36 insertions(+), 28 deletions(-) diff --git a/clang/include/clang/Frontend/DiagnosticRenderer.h b/clang/include/clang/Frontend/DiagnosticRenderer.h index 3f03a6e02da4b..06aa8aa35ebca 100644 --- a/clang/include/clang/Frontend/DiagnosticRenderer.h +++ b/clang/include/clang/Frontend/DiagnosticRenderer.h @@ -23,6 +23,7 @@ #include "llvm/ADT/IntrusiveRefCntPtr.h" #include "llvm/ADT/PointerUnion.h" #include "llvm/ADT/StringRef.h" +#include <optional> namespace clang { @@ -32,6 +33,12 @@ class SourceManager; using DiagOrStoredDiag = llvm::PointerUnion<const Diagnostic *, const StoredDiagnostic *>; +/// Maps both endpoints of \p Range to their macro expansion, so that the range +/// can be shown to a user. +std::optional<CharSourceRange> getExpansionRangeInFile(CharSourceRange Range, + FileID FID, + const SourceManager &SM); + /// Class to encapsulate the logic for formatting a diagnostic message. /// /// Actual "printing" logic is implemented by subclasses. diff --git a/clang/lib/Frontend/DiagnosticRenderer.cpp b/clang/lib/Frontend/DiagnosticRenderer.cpp index 2b6fbc274e587..c0c6e15ec0378 100644 --- a/clang/lib/Frontend/DiagnosticRenderer.cpp +++ b/clang/lib/Frontend/DiagnosticRenderer.cpp @@ -34,6 +34,22 @@ DiagnosticRenderer::DiagnosticRenderer(const LangOptions &LangOpts, DiagnosticRenderer::~DiagnosticRenderer() = default; +std::optional<CharSourceRange> +clang::getExpansionRangeInFile(CharSourceRange Range, FileID FID, + const SourceManager &SM) { + if (Range.isInvalid()) + return std::nullopt; + + SourceLocation Begin = SM.getExpansionLoc(Range.getBegin()); + CharSourceRange EndRange = SM.getExpansionRange(Range.getEnd()); + SourceLocation End = EndRange.getEnd(); + + if (SM.getFileID(Begin) != FID || SM.getFileID(End) != FID) + return std::nullopt; + + return CharSourceRange(SourceRange(Begin, End), EndRange.isTokenRange()); +} + namespace { class FixitReceiver : public edit::EditsReceiver { diff --git a/clang/lib/Frontend/SARIFDiagnostic.cpp b/clang/lib/Frontend/SARIFDiagnostic.cpp index 04ee14edfe74c..5183462b76f41 100644 --- a/clang/lib/Frontend/SARIFDiagnostic.cpp +++ b/clang/lib/Frontend/SARIFDiagnostic.cpp @@ -120,29 +120,20 @@ SARIFDiagnostic::getSarifLocation(FullSourceLoc Loc, PresumedLoc PLoc, FileID CaretFileID = Loc.getExpansionLoc().getFileID(); + auto &SM = Loc.getManager(); for (const CharSourceRange Range : Ranges) { - // Ignore invalid ranges. - if (Range.isInvalid()) + std::optional<CharSourceRange> FileRange = + getExpansionRangeInFile(Range, CaretFileID, SM); + if (!FileRange) continue; - auto &SM = Loc.getManager(); - SourceLocation B = SM.getExpansionLoc(Range.getBegin()); - CharSourceRange ERange = SM.getExpansionRange(Range.getEnd()); - SourceLocation E = ERange.getEnd(); - bool IsTokenRange = ERange.isTokenRange(); - - FileIDAndOffset BInfo = SM.getDecomposedLoc(B); - FileIDAndOffset EInfo = SM.getDecomposedLoc(E); - - // If the start or end of the range is in another file, just discard - // it. - if (BInfo.first != CaretFileID || EInfo.first != CaretFileID) - continue; + SourceLocation B = FileRange->getBegin(); + SourceLocation E = FileRange->getEnd(); // Add in the length of the token, so that we cover multi-char // tokens. unsigned TokSize = 0; - if (IsTokenRange) + if (FileRange->isTokenRange()) TokSize = Lexer::MeasureTokenLength(E, SM, LangOpts); FullSourceLoc BF(B, SM), EF(E, SM); @@ -157,7 +148,6 @@ SARIFDiagnostic::getSarifLocation(FullSourceLoc Loc, PresumedLoc PLoc, // Text and SARIF diagnostics. } - auto &SM = Loc.getManager(); auto FID = PLoc.getFileID(); // Visual Studio 2010 or earlier expects column number to be off by one. unsigned int ColNo = (LangOpts.MSCompatibilityVersion && diff --git a/clang/lib/Frontend/TextDiagnostic.cpp b/clang/lib/Frontend/TextDiagnostic.cpp index 01a4d2f6392d3..5221ed4de02ff 100644 --- a/clang/lib/Frontend/TextDiagnostic.cpp +++ b/clang/lib/Frontend/TextDiagnostic.cpp @@ -921,23 +921,18 @@ void TextDiagnostic::emitDiagnosticLoc(FullSourceLoc Loc, PresumedLoc PLoc, const SourceManager &SM = Loc.getManager(); for (const auto &R : Ranges) { - // Ignore invalid ranges. - if (!R.isValid()) + std::optional<CharSourceRange> FileRange = + getExpansionRangeInFile(R, CaretFileID, SM); + if (!FileRange) continue; - SourceLocation B = SM.getExpansionLoc(R.getBegin()); - CharSourceRange ERange = SM.getExpansionRange(R.getEnd()); - SourceLocation E = ERange.getEnd(); - - // If the start or end of the range is in another file, just - // discard it. - if (SM.getFileID(B) != CaretFileID || SM.getFileID(E) != CaretFileID) - continue; + SourceLocation B = FileRange->getBegin(); + SourceLocation E = FileRange->getEnd(); // Add in the length of the token, so that we cover multi-char // tokens. unsigned TokSize = 0; - if (ERange.isTokenRange()) + if (FileRange->isTokenRange()) TokSize = Lexer::MeasureTokenLength(E, SM, LangOpts); FullSourceLoc BF(B, SM), EF(E, SM); From a117608cd1487401064079bd097622b22942e0d4 Mon Sep 17 00:00:00 2001 From: Balazs Benics <[email protected]> Date: Mon, 10 Aug 2026 10:56:22 +0100 Subject: [PATCH 2/5] [clang][NFC] Reuse SourceManager::getExpansionRange in getExpansionRangeInFile --- clang/lib/Frontend/DiagnosticRenderer.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/clang/lib/Frontend/DiagnosticRenderer.cpp b/clang/lib/Frontend/DiagnosticRenderer.cpp index c0c6e15ec0378..f7c4114cb5aa9 100644 --- a/clang/lib/Frontend/DiagnosticRenderer.cpp +++ b/clang/lib/Frontend/DiagnosticRenderer.cpp @@ -40,14 +40,12 @@ clang::getExpansionRangeInFile(CharSourceRange Range, FileID FID, if (Range.isInvalid()) return std::nullopt; - SourceLocation Begin = SM.getExpansionLoc(Range.getBegin()); - CharSourceRange EndRange = SM.getExpansionRange(Range.getEnd()); - SourceLocation End = EndRange.getEnd(); - - if (SM.getFileID(Begin) != FID || SM.getFileID(End) != FID) + CharSourceRange Expansion = SM.getExpansionRange(Range); + if (SM.getFileID(Expansion.getBegin()) != FID || + SM.getFileID(Expansion.getEnd()) != FID) return std::nullopt; - return CharSourceRange(SourceRange(Begin, End), EndRange.isTokenRange()); + return Expansion; } namespace { From f9e6d5ff00622b1970345028795920a2f71520d5 Mon Sep 17 00:00:00 2001 From: Balazs Benics <[email protected]> Date: Mon, 10 Aug 2026 10:56:45 +0100 Subject: [PATCH 3/5] [clang][NFC] Document getExpansionRangeInFile's nullopt cases --- clang/include/clang/Frontend/DiagnosticRenderer.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/clang/include/clang/Frontend/DiagnosticRenderer.h b/clang/include/clang/Frontend/DiagnosticRenderer.h index 06aa8aa35ebca..0b123a87131ff 100644 --- a/clang/include/clang/Frontend/DiagnosticRenderer.h +++ b/clang/include/clang/Frontend/DiagnosticRenderer.h @@ -35,6 +35,8 @@ using DiagOrStoredDiag = /// Maps both endpoints of \p Range to their macro expansion, so that the range /// can be shown to a user. +/// +/// \returns nullopt if \p Range is invalid, if an endpoint lies outside \p FID. std::optional<CharSourceRange> getExpansionRangeInFile(CharSourceRange Range, FileID FID, const SourceManager &SM); From 87b26f05a23c91875d01d411ab95c6214b3754e8 Mon Sep 17 00:00:00 2001 From: Balazs Benics <[email protected]> Date: Mon, 10 Aug 2026 12:15:23 +0100 Subject: [PATCH 4/5] NFC Add braces --- clang/lib/Frontend/DiagnosticRenderer.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/clang/lib/Frontend/DiagnosticRenderer.cpp b/clang/lib/Frontend/DiagnosticRenderer.cpp index f7c4114cb5aa9..910aa1054b602 100644 --- a/clang/lib/Frontend/DiagnosticRenderer.cpp +++ b/clang/lib/Frontend/DiagnosticRenderer.cpp @@ -42,8 +42,9 @@ clang::getExpansionRangeInFile(CharSourceRange Range, FileID FID, CharSourceRange Expansion = SM.getExpansionRange(Range); if (SM.getFileID(Expansion.getBegin()) != FID || - SM.getFileID(Expansion.getEnd()) != FID) + SM.getFileID(Expansion.getEnd()) != FID) { return std::nullopt; + } return Expansion; } From 8d425c10f02a6f0cc52d90cd1d9046d635193fc4 Mon Sep 17 00:00:00 2001 From: Balazs Benics <[email protected]> Date: Thu, 6 Aug 2026 11:02:08 +0100 Subject: [PATCH 5/5] [clang] Reject ranges getExpansionRangeInFile cannot represent getExpansionRangeInFile passed reversed ranges through, inherited from the loop it was extracted from. The analyzer's SARIF and HTML consumers, added on top, walk begin->end, so reject such ranges in the shared helper rather than in each output. The mapped endpoints are compared as-is, which is stricter than Lexer::makeFileCharRange(): that extends a token range to the end of its last token before comparing offsets. The new postcondition, plus the invalid- and cross-file-range guards, are covered by a GetExpansionRangeInFile unit test in clang/unittests/Frontend/TextDiagnosticTest.cpp. Assisted-By: claude --- .../clang/Frontend/DiagnosticRenderer.h | 5 +- clang/lib/Frontend/DiagnosticRenderer.cpp | 6 ++ .../unittests/Frontend/TextDiagnosticTest.cpp | 94 +++++++++++++++++++ 3 files changed, 104 insertions(+), 1 deletion(-) diff --git a/clang/include/clang/Frontend/DiagnosticRenderer.h b/clang/include/clang/Frontend/DiagnosticRenderer.h index 0b123a87131ff..7fa280a6ff3df 100644 --- a/clang/include/clang/Frontend/DiagnosticRenderer.h +++ b/clang/include/clang/Frontend/DiagnosticRenderer.h @@ -36,7 +36,10 @@ using DiagOrStoredDiag = /// Maps both endpoints of \p Range to their macro expansion, so that the range /// can be shown to a user. /// -/// \returns nullopt if \p Range is invalid, if an endpoint lies outside \p FID. +/// The result never spans backwards: its begin does not lie after its end. +/// +/// \returns nullopt if \p Range is invalid, if an endpoint lies outside \p FID, +/// or if the mapped endpoints are reversed. std::optional<CharSourceRange> getExpansionRangeInFile(CharSourceRange Range, FileID FID, const SourceManager &SM); diff --git a/clang/lib/Frontend/DiagnosticRenderer.cpp b/clang/lib/Frontend/DiagnosticRenderer.cpp index 910aa1054b602..67760e2b7c4dd 100644 --- a/clang/lib/Frontend/DiagnosticRenderer.cpp +++ b/clang/lib/Frontend/DiagnosticRenderer.cpp @@ -46,6 +46,12 @@ clang::getExpansionRangeInFile(CharSourceRange Range, FileID FID, return std::nullopt; } + // Both endpoints are in FID, so comparing their offsets is meaningful. + if (SM.getFileOffset(Expansion.getBegin()) > + SM.getFileOffset(Expansion.getEnd())) { + return std::nullopt; + } + return Expansion; } diff --git a/clang/unittests/Frontend/TextDiagnosticTest.cpp b/clang/unittests/Frontend/TextDiagnosticTest.cpp index 4c4decc4a6857..382b2f068a089 100644 --- a/clang/unittests/Frontend/TextDiagnosticTest.cpp +++ b/clang/unittests/Frontend/TextDiagnosticTest.cpp @@ -9,10 +9,13 @@ #include "clang/Frontend/TextDiagnostic.h" #include "clang/Basic/FileManager.h" #include "clang/Basic/LangOptions.h" +#include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" +#include "clang/Frontend/DiagnosticRenderer.h" #include "clang/Frontend/TextDiagnosticPrinter.h" #include "llvm/Support/SmallVectorMemoryBuffer.h" #include "gtest/gtest.h" +#include <optional> using namespace llvm; using namespace clang; @@ -121,4 +124,95 @@ TEST_P(ShowLevelNoLocationTest, LevelPrefixRespected) { INSTANTIATE_TEST_SUITE_P(ShowLevelNoLocation, ShowLevelNoLocationTest, ::testing::Bool()); +// Creates a virtual file with the given contents and returns its FileID. +static FileID makeFile(FileManager &FileMgr, SourceManager &SrcMgr, + StringRef Path, StringRef Contents) { + FileEntryRef FE = FileMgr.getVirtualFileRef( + Path, /*Size=*/static_cast<off_t>(Contents.size()), + /*ModificationTime=*/0); + SmallVector<char, 64> Buffer(Contents.begin(), Contents.end()); + SrcMgr.overrideFileContents(FE, std::make_unique<SmallVectorMemoryBuffer>( + std::move(Buffer), Path, + /*RequiresNullTerminator=*/false)); + return SrcMgr.createFileID(FE, SourceLocation(), SrcMgr::C_User); +} + +TEST(DiagnosticRenderer, GetExpansionRangeInFileTest) { + FileSystemOptions FSOpts; + FileManager FileMgr(FSOpts); + DiagnosticOptions DiagEngineOpts; + DiagnosticsEngine DiagEngine(DiagnosticIDs::create(), DiagEngineOpts, + new IgnoringDiagConsumer()); + SourceManager SM(DiagEngine, FileMgr); + + FileID FID = makeFile(FileMgr, SM, "main.cpp", "some\nsource\ncode\n"); + FileID OtherFID = makeFile(FileMgr, SM, "other.cpp", "other\n"); + SM.setMainFileID(FID); + + auto Loc = [&](unsigned Line, unsigned Col) { + return SM.translateLineCol(FID, Line, Col); + }; + + const SourceLocation L1C1 = Loc(/*Line=*/1, /*Col=*/1); + const SourceLocation L1C3 = Loc(/*Line=*/1, /*Col=*/3); + + // An invalid range is rejected. + EXPECT_FALSE(getExpansionRangeInFile(CharSourceRange(), FID, SM)); + + // A char range stays a char range. + std::optional<CharSourceRange> CharR = getExpansionRangeInFile( + CharSourceRange::getCharRange(L1C1, L1C3), FID, SM); + ASSERT_TRUE(CharR); + EXPECT_TRUE(CharR->isCharRange()); + + // A token range stays a token range. + std::optional<CharSourceRange> TokR = getExpansionRangeInFile( + CharSourceRange::getTokenRange(L1C1, L1C3), FID, SM); + ASSERT_TRUE(TokR); + EXPECT_TRUE(TokR->isTokenRange()); + + // A reversed range (begin lies after end) is rejected. + EXPECT_FALSE(getExpansionRangeInFile( + CharSourceRange::getCharRange(L1C3, L1C1), FID, SM)); + + // The endpoints are compared as-is, so a reversed token range is rejected + // too, even though extending its end token would order the offsets. + EXPECT_FALSE(getExpansionRangeInFile( + CharSourceRange::getTokenRange(L1C3, L1C1), FID, SM)); + + // A range with an endpoint in another file is rejected. + SourceLocation OtherLoc = SM.getLocForStartOfFile(OtherFID); + EXPECT_FALSE(getExpansionRangeInFile( + CharSourceRange::getTokenRange(L1C1, OtherLoc), FID, SM)); + + { + const SourceLocation L2C1 = Loc(/*Line=*/2, /*Col=*/1); + const SourceLocation L2C6 = Loc(/*Line=*/2, /*Col=*/6); + + // Pretend that "source" expands "some". + SourceLocation MacroLoc = SM.createExpansionLoc( + /*SpellingLoc=*/L1C1, /*ExpansionLocStart=*/L2C1, + /*ExpansionLocEnd=*/L2C6, /*Length=*/4); + ASSERT_TRUE(MacroLoc.isMacroID()); + ASSERT_EQ(SM.getSpellingLoc(MacroLoc), L1C1); + ASSERT_EQ(SM.getExpansionLoc(MacroLoc), L2C1); + + // A macro-expanded range is remapped to its expansion in the file. + // A location inside the macro maps back to that file range. + auto MacroToken = CharSourceRange::getTokenRange(MacroLoc, MacroLoc); + auto MacroR = getExpansionRangeInFile(MacroToken, FID, SM); + ASSERT_TRUE(MacroR); + EXPECT_EQ(SM.getFileID(MacroR->getBegin()), FID); + EXPECT_EQ(SM.getFileID(MacroR->getEnd()), FID); + + // The range is a file range. + EXPECT_TRUE(MacroR->getBegin().isFileID()); + EXPECT_TRUE(MacroR->getEnd().isFileID()); + + // The range is the expansion range. + EXPECT_EQ(MacroR->getBegin(), L2C1); + EXPECT_EQ(MacroR->getEnd(), L2C6); + } +} + } // anonymous namespace _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
