llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-static-analyzer-1 Author: Balázs Benics (steakhal) <details> <summary>Changes</summary> `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 --- Full diff: https://github.com/llvm/llvm-project/pull/219136.diff 4 Files Affected: - (modified) clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h (+4-4) - (modified) clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (+19-6) - (modified) clang/test/Analysis/bitwise-shift-common.c (+1-4) - (modified) clang/test/Analysis/diagnostics/macros.cpp (+12) ``````````diff 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')}} +} `````````` </details> https://github.com/llvm/llvm-project/pull/219136 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
