[clang] 84e496d - [NFC][analyzer] Improve code quality in VisitBinaryOperator (#205151)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Jun 30 07:35:50 PDT 2026
Author: Donát Nagy
Date: 2026-06-30T16:35:44+02:00
New Revision: 84e496da5f0e3e5dde1e405bf6a011347ad4c460
URL: https://github.com/llvm/llvm-project/commit/84e496da5f0e3e5dde1e405bf6a011347ad4c460
DIFF: https://github.com/llvm/llvm-project/commit/84e496da5f0e3e5dde1e405bf6a011347ad4c460.diff
LOG: [NFC][analyzer] Improve code quality in VisitBinaryOperator (#205151)
I started out to remove the `NodeBuilder` in
`ExprEngine::VisitBinaryOperator` (as a part of my commit series that
removes `NodeBuilder`s), but I noticed that this method is full of
technical debt, so I decided to do a through clean-up (which includes
the removal of the trivial `NodeBuilder`).
Added:
Modified:
clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
index f2682b25630e1..33c31fe10782a 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -20,27 +20,10 @@ using namespace clang;
using namespace ento;
using llvm::APSInt;
-/// Optionally conjure and return a symbol for offset when processing
-/// \p Elem.
-/// If \p Other is a location, conjure a symbol for \p Symbol
-/// (offset) if it is unknown so that memory arithmetic always
-/// results in an ElementRegion.
-/// \p Count The number of times the current basic block was visited.
-static SVal conjureOffsetSymbolOnLocation(SVal Symbol, SVal Other,
- ConstCFGElementRef Elem, QualType Ty,
- SValBuilder &svalBuilder,
- unsigned Count,
- const StackFrame *SF) {
- if (isa<Loc>(Other) && Ty->isIntegralOrEnumerationType() &&
- Symbol.isUnknown()) {
- return svalBuilder.conjureSymbolVal(Elem, SF, Ty, Count);
- }
- return Symbol;
-}
-
void ExprEngine::VisitBinaryOperator(const BinaryOperator* B,
ExplodedNode *Pred,
ExplodedNodeSet &Dst) {
+ const StackFrame *SF = Pred->getStackFrame();
Expr *LHS = B->getLHS()->IgnoreParens();
Expr *RHS = B->getRHS()->IgnoreParens();
@@ -51,19 +34,14 @@ void ExprEngine::VisitBinaryOperator(const BinaryOperator* B,
getCheckerManager().runCheckersForPreStmt(CheckedSet, Pred, B, *this);
// With both the LHS and RHS evaluated, process the operation itself.
- for (ExplodedNodeSet::iterator it=CheckedSet.begin(), ei=CheckedSet.end();
- it != ei; ++it) {
-
- ProgramStateRef state = (*it)->getState();
- const StackFrame *SF = (*it)->getStackFrame();
- SVal LeftV = state->getSVal(LHS, SF);
- SVal RightV = state->getSVal(RHS, SF);
+ for (ExplodedNode *N : CheckedSet) {
+ ProgramStateRef State = N->getState();
+ SVal LeftV = State->getSVal(LHS, SF);
+ SVal RightV = State->getSVal(RHS, SF);
BinaryOperator::Opcode Op = B->getOpcode();
if (Op == BO_Assign) {
- // EXPERIMENTAL: "Conjured" symbols.
- // FIXME: Handle structs.
if (RightV.isUnknown()) {
unsigned Count = getNumVisitedCurrent();
RightV = svalBuilder.conjureSymbolVal(nullptr, getCFGElementRef(), SF,
@@ -72,44 +50,45 @@ void ExprEngine::VisitBinaryOperator(const BinaryOperator* B,
// Simulate the effects of a "store": bind the value of the RHS
// to the L-Value represented by the LHS.
SVal ExprVal = B->isGLValue() ? LeftV : RightV;
- evalStore(Tmp2, B, LHS, *it, state->BindExpr(B, SF, ExprVal), LeftV,
+ evalStore(Tmp2, B, LHS, N, State->BindExpr(B, SF, ExprVal), LeftV,
RightV);
continue;
}
if (!B->isAssignmentOp()) {
- NodeBuilder Bldr(*it, Tmp2, *currBldrCtx);
-
if (B->isAdditiveOp()) {
- // TODO: This can be removed after we enable history tracking with
- // SymSymExpr.
- unsigned Count = getNumVisitedCurrent();
- RightV = conjureOffsetSymbolOnLocation(
- RightV, LeftV, getCFGElementRef(), RHS->getType(), svalBuilder,
- Count, SF);
- LeftV = conjureOffsetSymbolOnLocation(LeftV, RightV, getCFGElementRef(),
- LHS->getType(), svalBuilder,
- Count, SF);
+ // Ensure that if `p` is a pointer and `i` is an integer with Unknown
+ // value, then `p+i`, `i+p` and `p-i` are evaluated to element regions
+ // (with a symbolic offset) instead of Unknown.
+ auto ConjureIfNeeded = [this, SF](SVal &V, SVal Other, QualType VTy) {
+ if (isa<Loc>(Other) && VTy->isIntegralOrEnumerationType() &&
+ V.isUnknown()) {
+ V = svalBuilder.conjureSymbolVal(getCFGElementRef(), SF, VTy,
+ getNumVisitedCurrent());
+ }
+ };
+ ConjureIfNeeded(RightV, LeftV, RHS->getType());
+ ConjureIfNeeded(LeftV, RightV, LHS->getType());
}
// Although we don't yet model pointers-to-members, we do need to make
// sure that the members of temporaries have a valid 'this' pointer for
// other checks.
if (B->getOpcode() == BO_PtrMemD)
- state = createTemporaryRegionIfNeeded(state, SF, LHS);
+ State = createTemporaryRegionIfNeeded(State, SF, LHS);
// Process non-assignments except commas or short-circuited
// logical expressions (LAnd and LOr).
- SVal Result = evalBinOp(state, Op, LeftV, RightV, B->getType());
+ SVal Result = evalBinOp(State, Op, LeftV, RightV, B->getType());
if (!Result.isUnknown()) {
- state = state->BindExpr(B, SF, Result);
+ State = State->BindExpr(B, SF, Result);
} else {
// If we cannot evaluate the operation escape the operands.
- state = escapeValues(state, LeftV, PSK_EscapeOther);
- state = escapeValues(state, RightV, PSK_EscapeOther);
+ State = escapeValues(State, LeftV, PSK_EscapeOther);
+ State = escapeValues(State, RightV, PSK_EscapeOther);
}
- Bldr.generateNode(B, *it, state);
+ Tmp2.insert(Engine.makePostStmtNode(B, State, N));
continue;
}
@@ -133,60 +112,51 @@ void ExprEngine::VisitBinaryOperator(const BinaryOperator* B,
// Perform a load (the LHS). This performs the checks for
// null dereferences, and so on.
ExplodedNodeSet Tmp;
- SVal location = LeftV;
- evalLoad(Tmp, B, LHS, *it, state, location);
+ evalLoad(Tmp, B, LHS, N, State, LeftV);
for (ExplodedNode *N : Tmp) {
- state = N->getState();
- const StackFrame *SF = N->getStackFrame();
- SVal V = state->getSVal(LHS, SF);
-
- // Get the computation type.
- QualType CTy =
- cast<CompoundAssignOperator>(B)->getComputationResultType();
- CTy = getContext().getCanonicalType(CTy);
+ State = N->getState();
+ SVal V = State->getSVal(LHS, SF);
- QualType CLHSTy =
- cast<CompoundAssignOperator>(B)->getComputationLHSType();
- CLHSTy = getContext().getCanonicalType(CLHSTy);
-
- QualType LTy = getContext().getCanonicalType(LHS->getType());
+ // Determine the relevant types.
+ const ASTContext &ACtx = getContext();
+ const auto *CAOpB = cast<CompoundAssignOperator>(B);
+ QualType CTy = ACtx.getCanonicalType(CAOpB->getComputationResultType());
+ QualType CLHSTy = ACtx.getCanonicalType(CAOpB->getComputationLHSType());
+ QualType LTy = ACtx.getCanonicalType(LHS->getType());
// Promote LHS.
V = svalBuilder.evalCast(V, CLHSTy, LTy);
// Compute the result of the operation.
- SVal Result = svalBuilder.evalCast(evalBinOp(state, Op, V, RightV, CTy),
+ SVal Result = svalBuilder.evalCast(evalBinOp(State, Op, V, RightV, CTy),
B->getType(), CTy);
- // EXPERIMENTAL: "Conjured" symbols.
- // FIXME: Handle structs.
-
- SVal LHSVal;
+ SVal StoredInLeftV;
if (Result.isUnknown()) {
// The symbolic value is actually for the type of the left-hand side
// expression, not the computation type, as this is the value the
// LValue on the LHS will bind to.
- LHSVal = svalBuilder.conjureSymbolVal(/*symbolTag=*/nullptr,
- getCFGElementRef(), SF, LTy,
- getNumVisitedCurrent());
+ StoredInLeftV = svalBuilder.conjureSymbolVal(
+ /*symbolTag=*/nullptr, getCFGElementRef(), SF, LTy,
+ getNumVisitedCurrent());
// However, we need to convert the symbol to the computation type.
- Result = svalBuilder.evalCast(LHSVal, CTy, LTy);
+ Result = svalBuilder.evalCast(StoredInLeftV, CTy, LTy);
} else {
// The left-hand side may bind to a
diff erent value then the
// computation type.
- LHSVal = svalBuilder.evalCast(Result, LTy, CTy);
+ StoredInLeftV = svalBuilder.evalCast(Result, LTy, CTy);
}
// In C++, assignment and compound assignment operators return an
// lvalue.
if (B->isGLValue())
- state = state->BindExpr(B, SF, location);
+ State = State->BindExpr(B, SF, LeftV);
else
- state = state->BindExpr(B, SF, Result);
+ State = State->BindExpr(B, SF, Result);
- evalStore(Tmp2, B, LHS, N, state, location, LHSVal);
+ evalStore(Tmp2, B, LHS, N, State, LeftV, StoredInLeftV);
}
}
More information about the cfe-commits
mailing list