https://github.com/NagyDonat updated https://github.com/llvm/llvm-project/pull/234624
From e28cfcca233e6db9ac363f599848f2ca38fad56b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 22 Oct 2026 11:16:45 +0200 Subject: [PATCH 1/3] [analyzer] Remove questionable assertions in tryRearrange The `tryRearrange` logic in `SimpleSValBuilder.cpp` has asserted that in the `SymIntExpr`s (that reach this point as operands of the expression that is being rearranged) the concrete (`APSInt`) operand has the same type as the symbolic operand. This commit replaces these assertions with more defensive logic that _converts_ the concrete operand to the type of the symbolic one. I did not observe any failure of these assertions and tests in `svalbuilder-rearrange-comparisons.cpp` validate that they are not triggered by some natural candidates, e.g. handling `x - 1U` where `x` is a symbolic integer. However, when I added analogous assertions to a different place (the arguments of `evalBinOpNN`), those were triggerd in 50+ LIT test files, which suggests that this is not an established invariant of `SymIntExpr` and we should not rely on it with assertions. The new logic may be inaccurate in corner cases, but it should be a reasonable fallback, which is (obviously) better than triggering an assertion failure. Instead of directly replacing the assertions, I placed the conversion logic _after_ the "symbolic parts cancel out" branch to let that branch use the `APSInt`s with their original type. --- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp b/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp index f0c7a91f6ea0f..795671cfa9fed 100644 --- a/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp +++ b/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp @@ -342,18 +342,16 @@ static std::optional<NonLoc> tryRearrange(ProgramStateRef State, if (BinaryOperator::isComparisonOp(Op) && !InBounds(LInt, RInt, LSym, RSym)) return std::nullopt; - // FIXME: Why can we assert these? - assert(APSIntType(LInt) == BV.getAPSIntType(CommonTy) && - "Integers are not of the same type as symbols!"); - assert(APSIntType(RInt) == BV.getAPSIntType(CommonTy) && - "Integers are not of the same type as symbols!"); - if (LSym == RSym) return SVB .evalBinOpNN(State, Op, nonloc::ConcreteInt(BV.getValue(LInt)), nonloc::ConcreteInt(BV.getValue(RInt)), ResultTy) .getAs<NonLoc>(); + APSIntType CommonAPSIntTy = BV.getAPSIntType(CommonTy); + CommonAPSIntTy.apply(LInt); + CommonAPSIntTy.apply(RInt); + auto MakeSymExprVal = [&SVB, CommonTy](SymbolRef Lhs, BinaryOperatorKind Op, SymbolRef Rhs) { return SVB.makeSymExprValNN(Op, nonloc::SymbolVal(Lhs), From 0c5c2ec547b327e438aaf8afb962c7f6c3d3e38f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 22 Oct 2026 19:06:41 +0200 Subject: [PATCH 2/3] Return early instead of converting the APSInts --- .../clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h | 4 ++++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp | 8 ++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h index f1c50e721937b..a175847d28fd7 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h @@ -92,6 +92,10 @@ class APSIntType { return BitWidth == Other.BitWidth && IsUnsigned == Other.IsUnsigned; } + bool operator!=(const APSIntType &Other) const { + return !(*this == Other); + } + /// Provide an ordering for finding a common conversion type. /// /// Unsigned integers are considered to be better conversion types than diff --git a/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp b/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp index 795671cfa9fed..c89271af525b8 100644 --- a/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp +++ b/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp @@ -336,6 +336,10 @@ static std::optional<NonLoc> tryRearrange(ProgramStateRef State, if (LSym->getType() != CommonTy || RSym->getType() != CommonTy) return std::nullopt; + APSIntType CommonAPSIntTy = BV.getAPSIntType(CommonTy); + if (APSIntType(LInt) != CommonAPSIntTy || APSIntType(RInt) != CommonAPSIntTy) + return std::nullopt; + auto InBounds = [State](auto... X) { return (... && isWithinConstantOverflowBounds(X, State)); }; @@ -348,10 +352,6 @@ static std::optional<NonLoc> tryRearrange(ProgramStateRef State, nonloc::ConcreteInt(BV.getValue(RInt)), ResultTy) .getAs<NonLoc>(); - APSIntType CommonAPSIntTy = BV.getAPSIntType(CommonTy); - CommonAPSIntTy.apply(LInt); - CommonAPSIntTy.apply(RInt); - auto MakeSymExprVal = [&SVB, CommonTy](SymbolRef Lhs, BinaryOperatorKind Op, SymbolRef Rhs) { return SVB.makeSymExprValNN(Op, nonloc::SymbolVal(Lhs), From 14561893f7dd16fe70998dc48ffddd6ffcfa54d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 22 Oct 2026 19:45:37 +0200 Subject: [PATCH 3/3] Put trivial method on single line --- .../clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h index a175847d28fd7..290b123597198 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h @@ -92,9 +92,7 @@ class APSIntType { return BitWidth == Other.BitWidth && IsUnsigned == Other.IsUnsigned; } - bool operator!=(const APSIntType &Other) const { - return !(*this == Other); - } + bool operator!=(const APSIntType &Other) const { return !(*this == Other); } /// Provide an ordering for finding a common conversion type. /// _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
