Author: Donát Nagy Date: 2026-10-26T11:52:56+01:00 New Revision: 6c11ea9aed9c99f52a0146954998b40ab097f68d
URL: https://github.com/llvm/llvm-project/commit/6c11ea9aed9c99f52a0146954998b40ab097f68d DIFF: https://github.com/llvm/llvm-project/commit/6c11ea9aed9c99f52a0146954998b40ab097f68d.diff LOG: [analyzer] Remove questionable assertions in tryRearrange (#234624) The `tryRearrange` logic in `SimpleSValBuilder.cpp` has asserted that if a `SymIntExpr` reaches this point (as an operand of the expression that is being rearranged), then its RHS (a concrete `APSInt`) has the same type as the symbolic LHS. This commit replaces these assertions with a defensive early return. I did not observe any failure of these assertions; 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, this is not a general invariant of all `SymIntExpr` instances (there are 50+ LIT tests where one argument of `evalBinOpNN` is a `SymIntExpr` where the types of the LHS and RHS are different) and there isn't any concrete reason that would filter out mixed-type `SymIntExpr`s before the `tryRearrange` logic, so the assertion isn't justified. Even if this was an invariant of `SymIntExpr`, the right place for these assertions would be the constructor of `SymIntExpr` and not the middle of a random algorithm that only sees a small fraction of `SymIntExpr`s. Note that the ~5 mixed-type `SymIntExpr` instances that I inspected were all originating from `evalBinOpNN` calls performed by checkers like ArrayBound or CString – it seems that if a `SymIntExpr`s originates from calculations that are spelled out in the source, then the RHS and LHS usually (perhaps even always?) has the same type. This may be caused by the implicit casts that establish a common type before the evaluation of the BinaryOperator – which is missing in the `evalBinOpNN` calls of the checkers. Added: Modified: clang/include/clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp Removed: ################################################################################ diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h index f1c50e721937b..290b123597198 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h @@ -92,6 +92,8 @@ 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 f0c7a91f6ea0f..c89271af525b8 100644 --- a/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp +++ b/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp @@ -336,18 +336,16 @@ 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)); }; 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)), _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
