https://github.com/steakhal updated https://github.com/llvm/llvm-project/pull/219136
From bab37be515b0ec8d7cd3416965d890fab99c8f9f Mon Sep 17 00:00:00 2001 From: Balazs Benics <[email protected]> Date: Wed, 26 Aug 2026 15:39:38 +0100 Subject: [PATCH 1/2] [analyzer] Print comparison literals with the right signedness `ConditionBRVisitor::patternMatch()` streamed the literal operand of a comparison through `APInt`'s stream operator, which hardcodes `print(OS, /*isSigned=*/true)`. Any unsigned literal with its top bit set therefore rendered as a negative number, so a path note could claim an unsigned variable equals a value it cannot hold: note: Assuming 'x' is equal to -1 // x is 'unsigned' note: Assuming 't' is >= -446744073709551616 // t is 'unsigned long long' The note describes the *other* operand of the comparison, so print the literal the way that operand would hold it: take the signedness from that operand rather than from the literal. The caller already has both operands, so it just passes the opposite one in. Note that keying off the literal's own type does not work. In `macros.cpp` an `int` is compared against `UINT32_MAX`, and there `-1` is the correct rendering -- that variable genuinely holds `-1`, and `4294967295` would be the misleading spelling. The literal is an identical `unsigned int 4294967295` in both that case and the ones above, so the two are indistinguishable from the literal alone. Both directions are now covered by tests. Display-only: this string is built for diagnostic text and never feeds the analysis. `patternMatch()`'s return value is used solely to decide operand ordering. This is a follow-up for #218940 Assisted-By: claude --- .../Core/BugReporter/BugReporterVisitors.h | 8 +++--- .../Core/BugReporterVisitors.cpp | 25 ++++++++++++++----- clang/test/Analysis/bitwise-shift-common.c | 5 +--- clang/test/Analysis/diagnostics/macros.cpp | 12 +++++++++ 4 files changed, 36 insertions(+), 14 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h index 495e74e47e51d..18d299f8bf7e2 100644 --- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h @@ -513,10 +513,10 @@ class ConditionBRVisitor final : public BugReporterVisitor { bool printValue(const Expr *CondVarExpr, raw_ostream &Out, const ExplodedNode *N, bool TookTrue, bool IsAssuming); - bool patternMatch(const Expr *Ex, const Expr *ParentEx, raw_ostream &Out, - BugReporterContext &BRC, PathSensitiveBugReport &R, - const ExplodedNode *N, std::optional<bool> &prunable, - bool IsSameFieldName); + bool patternMatch(const Expr *Ex, const Expr *ParentEx, const Expr *OtherEx, + raw_ostream &Out, BugReporterContext &BRC, + PathSensitiveBugReport &R, const ExplodedNode *N, + std::optional<bool> &prunable, bool IsSameFieldName); static bool isPieceMessageGeneric(const PathDiagnosticPiece *Piece); }; diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index 170d96699890b..704fcccbea1e7 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -2899,7 +2899,8 @@ ConditionBRVisitor::VisitTrueTest(const Expr *Cond, BugReporterContext &BRC, } bool ConditionBRVisitor::patternMatch(const Expr *Ex, const Expr *ParentEx, - raw_ostream &Out, BugReporterContext &BRC, + const Expr *OtherEx, raw_ostream &Out, + BugReporterContext &BRC, PathSensitiveBugReport &report, const ExplodedNode *N, std::optional<bool> &prunable, @@ -2964,7 +2965,17 @@ bool ConditionBRVisitor::patternMatch(const Expr *Ex, const Expr *ParentEx, } } - Out << IL->getValue(); + // This literal is compared against OtherEx, and the note describes that + // operand ("Assuming 'x' is equal to ..."), so print the literal the way + // that operand would hold it. Otherwise an unsigned 4294967295 would + // render as -1. + bool IsSigned = IL->getType()->isSignedIntegerOrEnumerationType(); + if (OtherEx) { + QualType OtherTy = OtherEx->IgnoreParenCasts()->getType(); + if (OtherTy->isIntegralOrEnumerationType()) + IsSigned = OtherTy->isSignedIntegerOrEnumerationType(); + } + IL->getValue().print(Out, IsSigned); return false; } @@ -3003,10 +3014,12 @@ PathDiagnosticPieceRef ConditionBRVisitor::VisitTrueTest( SmallString<128> LhsString, RhsString; { llvm::raw_svector_ostream OutLHS(LhsString), OutRHS(RhsString); - const bool isVarLHS = patternMatch(BExpr->getLHS(), BExpr, OutLHS, BRC, R, - N, shouldPrune, IsSameFieldName); - const bool isVarRHS = patternMatch(BExpr->getRHS(), BExpr, OutRHS, BRC, R, - N, shouldPrune, IsSameFieldName); + const bool isVarLHS = patternMatch(BExpr->getLHS(), BExpr, BExpr->getRHS(), + OutLHS, BRC, R, N, shouldPrune, + IsSameFieldName); + const bool isVarRHS = patternMatch(BExpr->getRHS(), BExpr, BExpr->getLHS(), + OutRHS, BRC, R, N, shouldPrune, + IsSameFieldName); shouldInvert = !isVarLHS && isVarRHS; } diff --git a/clang/test/Analysis/bitwise-shift-common.c b/clang/test/Analysis/bitwise-shift-common.c index de83f71ec068b..9ae72daa3ee44 100644 --- a/clang/test/Analysis/bitwise-shift-common.c +++ b/clang/test/Analysis/bitwise-shift-common.c @@ -91,10 +91,7 @@ int too_large_right_operand_symbolic(int left, int right) { } unsigned huge_right_operand_symbolic(unsigned x, unsigned long long t) { - // FIXME: the 'Assuming' note below prints the literal via APInt's stream - // operator, which always formats as signed, so 18000000000000000000 comes - // out as a bogus negative. That is a bug in ConditionBRVisitor. - // expected-note@+2 {{Assuming 't' is >= -446744073709551616}} + // expected-note@+2 {{Assuming 't' is >= 18000000000000000000}} // expected-note@+1 {{Taking false branch}} if (t < 18000000000000000000ULL) return 0; diff --git a/clang/test/Analysis/diagnostics/macros.cpp b/clang/test/Analysis/diagnostics/macros.cpp index b3887b39a446f..978a6cd1037c9 100644 --- a/clang/test/Analysis/diagnostics/macros.cpp +++ b/clang/test/Analysis/diagnostics/macros.cpp @@ -72,3 +72,15 @@ void testNestedNullSplitMacro(int i, int *p) { *p = 1; // expected-warning {{Dereference of null pointer (loaded from variable 'p')}} // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}} } + +// Same comparison as above, but against an 'unsigned' variable: here the +// literal must be spelled unsigned. Printing it as signed (as the raw APInt +// stream operator does) would claim an unsigned value is equal to -1. +void testNestedNullSplitMacroUnsigned(unsigned i, int *p) { + nested_null_split(i); // expected-note {{Assuming 'i' is equal to 4294967295}} + // expected-note@-1 {{Taking false branch}} + if (!p) // expected-note {{Assuming 'p' is null}} + // expected-note@-1 {{Taking true branch}} + *p = 1; // expected-warning {{Dereference of null pointer (loaded from variable 'p')}} + // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}} +} From cf041d6d1ab562a024cff25a910c4decbd39df70 Mon Sep 17 00:00:00 2001 From: Balazs Benics <[email protected]> Date: Thu, 27 Aug 2026 09:09:59 +0100 Subject: [PATCH 2/2] Handtuned refinements --- .../StaticAnalyzer/Core/BugReporterVisitors.cpp | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index 704fcccbea1e7..3734afddd9a2b 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -2907,6 +2907,7 @@ bool ConditionBRVisitor::patternMatch(const Expr *Ex, const Expr *ParentEx, bool IsSameFieldName) { const Expr *OriginalExpr = Ex; Ex = Ex->IgnoreParenCasts(); + OtherEx = OtherEx->IgnoreParenCasts(); if (isa<GNUNullExpr, ObjCBoolLiteralExpr, CXXBoolLiteralExpr, IntegerLiteral, FloatingLiteral>(Ex)) { @@ -2965,17 +2966,9 @@ bool ConditionBRVisitor::patternMatch(const Expr *Ex, const Expr *ParentEx, } } - // This literal is compared against OtherEx, and the note describes that - // operand ("Assuming 'x' is equal to ..."), so print the literal the way - // that operand would hold it. Otherwise an unsigned 4294967295 would - // render as -1. - bool IsSigned = IL->getType()->isSignedIntegerOrEnumerationType(); - if (OtherEx) { - QualType OtherTy = OtherEx->IgnoreParenCasts()->getType(); - if (OtherTy->isIntegralOrEnumerationType()) - IsSigned = OtherTy->isSignedIntegerOrEnumerationType(); - } - IL->getValue().print(Out, IsSigned); + bool IsAnySigned = Ex->getType()->isSignedIntegerOrEnumerationType() || + OtherEx->getType()->isSignedIntegerOrEnumerationType(); + IL->getValue().print(Out, /*isSigned=*/IsAnySigned); return false; } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
