[PATCH] D85796: [Analysis] Bug fix for exploded graph branching in evalCall for constructor

[Nithin VR via Phabricator via cfe-commits]

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