Skip to content

[analyzer] Avoid use of CallEvents with obsolete state #160707

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+46 −34
Open

[analyzer] Avoid use of CallEvents with obsolete state #160707

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0685f09
[analyzer] Avoid use of `CallEvent`s with obsolete state
NagyDonat Sep 25, 2025
bb42fbe
Merge branch 'main' into evalCall-cleanup-obsolete-Call
NagyDonat Sep 30, 2025
1649033
Fix doubled word typos
NagyDonat Oct 7, 2025
5b8416f
Revert some non-functional changes to reduce diff size
NagyDonat Oct 7, 2025
abccb02
Fix a typo in an old comment (unrelated to the commit)
NagyDonat Oct 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,33 +731,36 @@ void CheckerManager::runCheckersForEvalCall(ExplodedNodeSet &Dst,
ExplodedNodeSet checkDst;
NodeBuilder B(Pred, checkDst, Eng.getBuilderContext());

ProgramStateRef State = Pred->getState();
CallEventRef<> UpdatedCall = Call.cloneWithState(State);

// Check if any of the EvalCall callbacks can evaluate the call.
for (const auto &EvalCallChecker : EvalCallCheckers) {
// TODO: Support the situation when the call doesn't correspond
// to any Expr.
ProgramPoint L = ProgramPoint::getProgramPoint(
Call.getOriginExpr(), ProgramPoint::PostStmtKind,
UpdatedCall->getOriginExpr(), ProgramPoint::PostStmtKind,
Pred->getLocationContext(), EvalCallChecker.Checker);
bool evaluated = false;
{ // CheckerContext generates transitions(populates checkDest) on
// destruction, so introduce the scope to make sure it gets properly
// populated.
CheckerContext C(B, Eng, Pred, L);
evaluated = EvalCallChecker(Call, C);
evaluated = EvalCallChecker(*UpdatedCall, C);
}
#ifndef NDEBUG
if (evaluated && evaluatorChecker) {
const auto toString = [](const CallEvent &Call) -> std::string {
const auto toString = [](CallEventRef<> Call) -> std::string {
std::string Buf;
llvm::raw_string_ostream OS(Buf);
Call.dump(OS);
Call->dump(OS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured this could works. Why did you change this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dump probably doesn't change after the clone.

But generally, in the checker loop, the updated call event indicates that the call was evaluated by multiple checkers. Which should be impossible in practice but it looks like we've decided to act gracefully when assertions are turned off.

That said, we aren't doing a great job at that, given that we're using llvm_unreachable() that translates to pure undefined behavior (aka __builtin_unreachable()) when assertions are turned off. (See also LLVM_UNREACHABLE_OPTIMIZE. These UBs are sometimes incredibly fun to debug.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change this?

It is true that this dump (which IIUC just prints the function name) doesn't rely on the State within the CallEvent object, so it would work with the non-updated instance -- but as we needed to create the UpdatedCall (because we need to pass that to the checkers), I thought that it would be better to consistently use it.

I don't exactly recall why I changed the type of this lambda, I think the compiler complained about my first attempt. I can try to avoid these changes (CallEventRef<> is a somewhat esoteric smart pointer type, but it should be possible to get a const CallEvent & from it).

<offtopic>
By the way I feel an urge to get rid of this lambda, because it is much more verbose than e.g.

std::string FunctionName;
Call.dump(llvm:raw_string_ostream(FunctionName));

(I don't see a reason why we would need to declare a variable for the stream instead of just using a temporary object -- but I'd test this before applying it.) However, I acknowledge that this is subjective bikeshedding, and I won't remove the lambda if you prefer to keep it.
</offtopic>

But generally, in the checker loop, the updated call event indicates that the call was evaluated by multiple checkers. Which should be impossible in practice but it looks like we've decided to act gracefully when assertions are turned off.

What do you mean by this? My understanding of this situation is that:

  • When assertions are turned off (#ifdef NDEBUG block a few lines below the displayed area) we break and leave the loop eagerly when one checker evaluated the call.
  • When assertions are turned on, we always iterate over all the checker callback, and do this formatted error printout if a second checker callback evaluated the call.

That said, we aren't doing a great job at that, given that we're using llvm_unreachable() that translates to pure undefined behavior (aka __builtin_unreachable()) when assertions are turned off.

Good to know in general, but this particular llvm_unreachable() call is within an #ifndef NDEBUG block so I would guess that it is completely skipped when assertions are turned off. (Is this correct? What is the relationship between assertions and NDEBUG?)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, it's the other way round. We're doing a great job and everything's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit 5b8416f I'm reverting the NFC tweaks that affect this part of the code.

return Buf;
};
std::string AssertionMessage = llvm::formatv(
"The '{0}' call has been already evaluated by the {1} checker, "
"while the {2} checker also tried to evaluate the same call. At "
"most one checker supposed to evaluate a call.",
toString(Call), evaluatorChecker,
toString(UpdatedCall), evaluatorChecker,
EvalCallChecker.Checker->getDebugTag());
llvm_unreachable(AssertionMessage.c_str());
}
Expand All @@ -774,7 +777,7 @@ void CheckerManager::runCheckersForEvalCall(ExplodedNodeSet &Dst,
// If none of the checkers evaluated the call, ask ExprEngine to handle it.
if (!evaluatorChecker) {
NodeBuilder B(Pred, Dst, Eng.getBuilderContext());
Eng.defaultEvalCall(B, Pred, Call, CallOpts);
Eng.defaultEvalCall(B, Pred, *UpdatedCall, CallOpts);
}
}
}
Expand Down
69 changes: 39 additions & 30 deletions clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,8 @@ void ExprEngine::VisitCallExpr(const CallExpr *CE, ExplodedNode *Pred,

ProgramStateRef ExprEngine::finishArgumentConstruction(ProgramStateRef State,
const CallEvent &Call) {
// WARNING: The state attached to 'Call' may be obsolete, do not call any
// methods that rely on it!
const Expr *E = Call.getOriginExpr();
// FIXME: Constructors to placement arguments of operator new
// are not supported yet.
Expand All @@ -653,6 +655,8 @@ ProgramStateRef ExprEngine::finishArgumentConstruction(ProgramStateRef State,
void ExprEngine::finishArgumentConstruction(ExplodedNodeSet &Dst,
ExplodedNode *Pred,
const CallEvent &Call) {
// WARNING: The state attached to 'Call' may be obsolete, do not call any
// methods that rely on it!
ProgramStateRef State = Pred->getState();
ProgramStateRef CleanedState = finishArgumentConstruction(State, Call);
if (CleanedState == State) {
Expand All @@ -670,35 +674,40 @@ void ExprEngine::finishArgumentConstruction(ExplodedNodeSet &Dst,
}

void ExprEngine::evalCall(ExplodedNodeSet &Dst, ExplodedNode *Pred,
const CallEvent &Call) {
// WARNING: At this time, the state attached to 'Call' may be older than the
// state in 'Pred'. This is a minor optimization since CheckerManager will
// use an updated CallEvent instance when calling checkers, but if 'Call' is
// ever used directly in this function all callers should be updated to pass
// the most recent state. (It is probably not worth doing the work here since
// for some callers this will not be necessary.)
const CallEvent &CallTemplate) {
// WARNING: As this function performs transitions between several different
// states (perhaps in a branching structure) we must be careful to avoid
// referencing obsolete or irrelevant states. In particular, 'CallEvent'
// instances have an attached state (because this is is convenient within the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is is

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 1649033

// checker callbacks) and it is our responsibility to keep these up-to-date.
// In fact, the parameter 'CallTemplate' is a "template" because its attached
// state may be older than the state of 'Pred' (which will be further
// transformed by the transitions within this method).
// (Note that 'runCheckersFor*Call' and 'finishArgumentConstruction' are
// prepared to take this template and and attach the proper state before
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and and

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 1649033

// forwarding it to the checkers.)

// Run any pre-call checks using the generic call interface.
ExplodedNodeSet dstPreVisit;
getCheckerManager().runCheckersForPreCall(dstPreVisit, Pred,
Call, *this);
getCheckerManager().runCheckersForPreCall(dstPreVisit, Pred, CallTemplate,
*this);

// Actually evaluate the function call. We try each of the checkers
// to see if the can evaluate the function call, and get a callback at
// defaultEvalCall if all of them fail.
ExplodedNodeSet dstCallEvaluated;
getCheckerManager().runCheckersForEvalCall(dstCallEvaluated, dstPreVisit,
Call, *this, EvalCallOptions());
getCheckerManager().runCheckersForEvalCall(
dstCallEvaluated, dstPreVisit, CallTemplate, *this, EvalCallOptions());

// If there were other constructors called for object-type arguments
// of this call, clean them up.
ExplodedNodeSet dstArgumentCleanup;
for (ExplodedNode *I : dstCallEvaluated)
finishArgumentConstruction(dstArgumentCleanup, I, Call);
finishArgumentConstruction(dstArgumentCleanup, I, CallTemplate);

ExplodedNodeSet dstPostCall;
getCheckerManager().runCheckersForPostCall(dstPostCall, dstArgumentCleanup,
Call, *this);
CallTemplate, *this);

// Escaping symbols conjured during invalidating the regions above.
// Note that, for inlined calls the nodes were put back into the worklist,
Expand All @@ -708,12 +717,13 @@ void ExprEngine::evalCall(ExplodedNodeSet &Dst, ExplodedNode *Pred,
// Run pointerEscape callback with the newly conjured symbols.
SmallVector<std::pair<SVal, SVal>, 8> Escaped;
for (ExplodedNode *I : dstPostCall) {
NodeBuilder B(I, Dst, *currBldrCtx);
ProgramStateRef State = I->getState();
CallEventRef<> Call = CallTemplate.cloneWithState(State);
NodeBuilder B(I, Dst, *currBldrCtx);
Escaped.clear();
{
unsigned Arg = -1;
for (const ParmVarDecl *PVD : Call.parameters()) {
for (const ParmVarDecl *PVD : Call->parameters()) {
++Arg;
QualType ParamTy = PVD->getType();
if (ParamTy.isNull() ||
Expand All @@ -722,13 +732,13 @@ void ExprEngine::evalCall(ExplodedNodeSet &Dst, ExplodedNode *Pred,
QualType Pointee = ParamTy->getPointeeType();
if (Pointee.isConstQualified() || Pointee->isVoidType())
continue;
if (const MemRegion *MR = Call.getArgSVal(Arg).getAsRegion())
if (const MemRegion *MR = Call->getArgSVal(Arg).getAsRegion())
Escaped.emplace_back(loc::MemRegionVal(MR), State->getSVal(MR, Pointee));
}
}

State = processPointerEscapedOnBind(State, Escaped, I->getLocationContext(),
PSK_EscapeOutParameters, &Call);
PSK_EscapeOutParameters, &*Call);

if (State == I->getState())
Dst.insert(I);
Expand Down Expand Up @@ -1212,59 +1222,58 @@ static bool isTrivialObjectAssignment(const CallEvent &Call) {
}

void ExprEngine::defaultEvalCall(NodeBuilder &Bldr, ExplodedNode *Pred,
const CallEvent &CallTemplate,
const CallEvent &Call,
const EvalCallOptions &CallOpts) {
// Make sure we have the most recent state attached to the call.
ProgramStateRef State = Pred->getState();
CallEventRef<> Call = CallTemplate.cloneWithState(State);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm removing this cloneWithState call because this method is only called by CheckerManager::runCheckersForEvalCall and I had to add a cloneWithState call within that method.


// Special-case trivial assignment operators.
if (isTrivialObjectAssignment(*Call)) {
performTrivialCopy(Bldr, Pred, *Call);
if (isTrivialObjectAssignment(Call)) {
performTrivialCopy(Bldr, Pred, Call);
return;
}

// Try to inline the call.
// The origin expression here is just used as a kind of checksum;
// this should still be safe even for CallEvents that don't come from exprs.
const Expr *E = Call->getOriginExpr();
const Expr *E = Call.getOriginExpr();

ProgramStateRef InlinedFailedState = getInlineFailedState(State, E);
if (InlinedFailedState) {
// If we already tried once and failed, make sure we don't retry later.
State = InlinedFailedState;
} else {
RuntimeDefinition RD = Call->getRuntimeDefinition();
Call->setForeign(RD.isForeign());
RuntimeDefinition RD = Call.getRuntimeDefinition();
Call.setForeign(RD.isForeign());
const Decl *D = RD.getDecl();
if (shouldInlineCall(*Call, D, Pred, CallOpts)) {
if (shouldInlineCall(Call, D, Pred, CallOpts)) {
if (RD.mayHaveOtherDefinitions()) {
AnalyzerOptions &Options = getAnalysisManager().options;

// Explore with and without inlining the call.
if (Options.getIPAMode() == IPAK_DynamicDispatchBifurcate) {
BifurcateCall(RD.getDispatchRegion(), *Call, D, Bldr, Pred);
BifurcateCall(RD.getDispatchRegion(), Call, D, Bldr, Pred);
return;
}

// Don't inline if we're not in any dynamic dispatch mode.
if (Options.getIPAMode() != IPAK_DynamicDispatch) {
conservativeEvalCall(*Call, Bldr, Pred, State);
conservativeEvalCall(Call, Bldr, Pred, State);
return;
}
}
ctuBifurcate(*Call, D, Bldr, Pred, State);
ctuBifurcate(Call, D, Bldr, Pred, State);
return;
}
}

// If we can't inline it, clean up the state traits used only if the function
// is inlined.
State = removeStateTraitsUsedForArrayEvaluation(
State, dyn_cast_or_null<CXXConstructExpr>(E), Call->getLocationContext());
State, dyn_cast_or_null<CXXConstructExpr>(E), Call.getLocationContext());

// Also handle the return value and invalidate the regions.
conservativeEvalCall(*Call, Bldr, Pred, State);
conservativeEvalCall(Call, Bldr, Pred, State);
}

void ExprEngine::BifurcateCall(const MemRegion *BifurReg,
Expand Down