NoQ added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/CheckerManager.cpp:682
anyEvaluated = true;
+ Dst.clear();
Dst.insert(checkDst);
----------------
vrnithinkumar wrote:
> > runCheckersForEvalCall() already has its own builder, you don't need
> > another.
>
> In that case is it okay to clear the `ExplodedNodeSet DstEvaluated` passed
> as `Dst` which contains parent node [[
> https://github.com/llvm/llvm-project/blob/master/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp#L604
> | contains parent node ]] ?
>
> When the any of the evaluated checker is evaluated the node successfully we
> can clear the `Dst` which is part of the `StmtNodeBuilder Bldr(DstPreCall,
> DstEvaluated, *currBldrCtx);` before inserting new nodes.
Hmm actually your code is now incorrect because `runCheckersForEvalCall()` is
in fact invoked multiple times in a loop (if there were state splits in
PreStmt/PreCall), therefore it's possible that `Dst` does in fact already
contain nodes.
That also means that i can't put in the assertions that i thought of; the
destination set is indeed potentially non-empty in many cases.
Anyway, here's what i meant:
```lang=diff
ExplodedNodeSet DstPreCall;
getCheckerManager().runCheckersForPreCall(DstPreCall, PreInitialized,
*Call, *this);
ExplodedNodeSet DstEvaluated;
- StmtNodeBuilder Bldr(DstPreCall, DstEvaluated, *currBldrCtx);
if (CE && CE->getConstructor()->isTrivial() &&
CE->getConstructor()->isCopyOrMoveConstructor() &&
!CallOpts.IsArrayCtorOrDtor) {
+ StmtNodeBuilder Bldr(DstPreCall, DstEvaluated, *currBldrCtx);
// FIXME: Handle other kinds of trivial constructors as well.
for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E =
DstPreCall.end();
I != E; ++I)
performTrivialCopy(Bldr, *I, *Call);
} else {
for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E =
DstPreCall.end();
I != E; ++I)
getCheckerManager().runCheckersForEvalCall(DstEvaluated, *I, *Call,
*this,
CallOpts);
}
// If the CFG was constructed without elements for temporary destructors
// and the just-called constructor created a temporary object then
// stop exploration if the temporary object has a noreturn constructor.
// This can lose coverage because the destructor, if it were present
// in the CFG, would be called at the end of the full expression or
// later (for life-time extended temporaries) -- but avoids infeasible
// paths when no-return temporary destructors are used for assertions.
+ ExplodedNodeSet DstEvaluatedPostProcessed;
+ StmtNodeBuilder Bldr(DstEvaluated, DstEvaluatedPostProcessed, *currBldrCtx);
const AnalysisDeclContext *ADC = LCtx->getAnalysisDeclContext();
if (!ADC->getCFGBuildOptions().AddTemporaryDtors) {
if (llvm::isa_and_nonnull<CXXTempObjectRegion>(TargetRegion) &&
cast<CXXConstructorDecl>(Call->getDecl())
->getParent()
->isAnyDestructorNoReturn()) {
// If we've inlined the constructor, then DstEvaluated would be empty.
// In this case we still want a sink, which could be implemented
// in processCallExit. But we don't have that implemented at the moment,
// so if you hit this assertion, see if you can avoid inlining
// the respective constructor when analyzer-config cfg-temporary-dtors
// is set to false.
// Otherwise there's nothing wrong with inlining such constructor.
assert(!DstEvaluated.empty() &&
"We should not have inlined this constructor!");
for (ExplodedNode *N : DstEvaluated) {
Bldr.generateSink(E, N, N->getState());
}
// There is no need to run the PostCall and PostStmt checker
// callbacks because we just generated sinks on all nodes in th
// frontier.
return;
}
}
ExplodedNodeSet DstPostArgumentCleanup;
- for (ExplodedNode *I : DstEvaluated)
+ for (ExplodedNode *I : DstEvaluatedPostProcessed)
finishArgumentConstruction(DstPostArgumentCleanup, I, *Call);
```
This way exactly one builder exists at any given moment of time and exactly one
builder operates on each pair of source-destination sets.
Also this entire `AddTemporaryDtors` option could probably be banned already
which would make things a whole lot easier; i'll look into that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85796/new/
https://reviews.llvm.org/D85796
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
