https://github.com/steakhal updated https://github.com/llvm/llvm-project/pull/214462
From d54a0a531dc08d1f19e94b909278280ea35ee5de Mon Sep 17 00:00:00 2001 From: Balazs Benics <[email protected]> Date: Thu, 6 Aug 2026 11:04:59 +0100 Subject: [PATCH] [analyzer] Fix -analyzer-output=sarif crash on macro-expanded ranges A path piece whose range ends inside a macro expansion aborted the whole document: https://godbolt.org/z/61vWYcsWj Cannot create a physicalLocation from invalid SourceRange! convertTokenRangeToCharRange() built the end with Lexer::getLocForEndOfToken(), which returns an invalid location for a macro ID that is not at the end of its expansion, and used it unchecked. The analyzer's own test corpus hits this in nine files; text and plist output were unaffected because both already map such ranges to the expansion. - Use getExpansionRangeInFile(), so the region covers the macro use like the other two outputs. - Fall back to a caret when the range is unusable. A thread flow needs a location per piece, so dropping one would truncate the reported path. This also stops reversed ranges producing regions with endColumn < startColumn. Single-token ranges keep their zero-width regions; widening them would churn every expected-sarif file, so that is left for a separate change. Assisted-By: claude --- .../StaticAnalyzer/Core/SarifDiagnostics.cpp | 45 ++++++++++--------- .../diagnostics/sarif-macro-expansion.c | 35 +++++++++++++++ 2 files changed, 58 insertions(+), 22 deletions(-) create mode 100644 clang/test/Analysis/diagnostics/sarif-macro-expansion.c diff --git a/clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp b/clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp index c9f5774c7db7c..1d5139c067c7b 100644 --- a/clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp +++ b/clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp @@ -18,6 +18,8 @@ #include "clang/Basic/Sarif.h" #include "clang/Basic/SourceManager.h" #include "clang/Basic/Version.h" +#include "clang/Frontend/DiagnosticRenderer.h" +#include "clang/Lex/Lexer.h" #include "clang/Lex/Preprocessor.h" #include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h" #include "llvm/ADT/StringMap.h" @@ -124,23 +126,25 @@ calculateImportance(const PathDiagnosticPiece &Piece) { return ThreadFlowImportance::Unimportant; } -/// Accepts a SourceRange corresponding to a pair of the first and last tokens -/// and converts to a Character granular CharSourceRange. -static CharSourceRange convertTokenRangeToCharRange(const SourceRange &R, - const SourceManager &SM, - const LangOptions &LO) { - // Caret diagnostics have the first and last locations pointed at the same - // location, return these as-is. - if (R.getBegin() == R.getEnd()) - return CharSourceRange::getCharRange(R); - - SourceLocation BeginCharLoc = R.getBegin(); - // For token ranges, the raw end SLoc points at the first character of the - // last token in the range. This must be moved to one past the end of the - // last character using the lexer. - SourceLocation EndCharLoc = - Lexer::getLocForEndOfToken(R.getEnd(), /* Offset = */ 0, SM, LO); - return CharSourceRange::getCharRange(BeginCharLoc, EndCharLoc); +/// Returns the character range to report for \p Loc. +/// +/// A thread flow needs a location for every piece, so an unusable range falls +/// back to a caret rather than being dropped, which would truncate the path. +static CharSourceRange getDisplayCharRange(const PathDiagnosticLocation &Loc, + const LangOptions &LO) { + const SourceManager &SM = Loc.getManager(); + FullSourceLoc Caret = Loc.asLocation().getExpansionLoc(); + SourceRange Range = Loc.asRange(); + + // FIXME: A single-token range is reported as a zero-width region. Widening it + // would churn every expected-sarif file, so it is left alone for now. + if (Range.getBegin() != Range.getEnd()) { + if (std::optional<CharSourceRange> FileRange = getExpansionRangeInFile( + CharSourceRange::getTokenRange(Range), Caret.getFileID(), SM)) + return Lexer::getAsCharRange(*FileRange, SM, LO); + } + + return CharSourceRange::getCharRange(Caret, Caret); } static SmallVector<ThreadFlow, 8> createThreadFlows(const PathDiagnostic *Diag, @@ -148,11 +152,9 @@ static SmallVector<ThreadFlow, 8> createThreadFlows(const PathDiagnostic *Diag, SmallVector<ThreadFlow, 8> Flows; const PathPieces &Pieces = Diag->path.flatten(false); for (const auto &Piece : Pieces) { - auto Range = convertTokenRangeToCharRange( - Piece->getLocation().asRange(), Piece->getLocation().getManager(), LO); auto Flow = ThreadFlow::create() .setImportance(calculateImportance(*Piece)) - .setRange(Range) + .setRange(getDisplayCharRange(Piece->getLocation(), LO)) .setMessage(Piece->getString()); Flows.push_back(Flow); } @@ -190,8 +192,7 @@ SarifDiagnostics::createResult(const PathDiagnostic *Diag, StringRef CheckName = Diag->getCheckerName(); uint32_t RuleIdx = RuleMapping.lookup(CheckName); - auto Range = convertTokenRangeToCharRange( - Diag->getLocation().asRange(), Diag->getLocation().getManager(), LO); + CharSourceRange Range = getDisplayCharRange(Diag->getLocation(), LO); SmallVector<ThreadFlow, 8> Flows = createThreadFlows(Diag, LO); diff --git a/clang/test/Analysis/diagnostics/sarif-macro-expansion.c b/clang/test/Analysis/diagnostics/sarif-macro-expansion.c new file mode 100644 index 0000000000000..3f8ee27b03a00 --- /dev/null +++ b/clang/test/Analysis/diagnostics/sarif-macro-expansion.c @@ -0,0 +1,35 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc \ +// RUN: -analyzer-output=sarif -verify %s -o - | FileCheck %s + +typedef __typeof(sizeof(int)) size_t; +void *malloc(size_t); + +#define ALLOC int *x = (int *)malloc(12); + +void ends_inside_expansion(void) { + ALLOC // no-crash +} // expected-warning {{Potential leak of memory pointed to by 'x'}} + +// The note covers the 'ALLOC' use rather than reaching into the macro body. +// CHECK: "text": "Memory is allocated" +// CHECK: "region": { +// CHECK-NEXT: "endColumn": 8, +// CHECK-NEXT: "endLine": [[#ALLOC_LINE:]], +// CHECK-NEXT: "startColumn": 3, +// CHECK-NEXT: "startLine": [[#ALLOC_LINE]] + +#define IS_NULL !p + +void ends_at_end_of_expansion(int *p) { + if (IS_NULL) + *p = 1; // expected-warning {{Dereference of null pointer}} +} + +// This range already ended at the end of the expansion, so it worked before. +// CHECK: "text": "Assuming 'p' is null" +// CHECK: "region": { +// CHECK-NEXT: "endColumn": 14, +// CHECK-NEXT: "endLine": [[#IS_NULL_LINE:]], +// CHECK-NEXT: "startColumn": 7, +// CHECK-NEXT: "startLine": [[#IS_NULL_LINE]] + _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
