vrnithinkumar marked an inline comment as done.
vrnithinkumar added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/CheckerManager.cpp:682
anyEvaluated = true;
+ Dst.clear();
Dst.insert(checkDst);
----------------
NoQ wrote:
> 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.
> 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.
Oh, I missed that point.
Made the above suggested changes to make sure exactly one `NodeBuilder` exists
at any given time.
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
