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

[Artem Dergachev via Phabricator via cfe-commits]

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