[analyzer] NFC: Make sure we don't ever inline the constructor for which the temporary destructor is noreturn and missing.

[NoQ]

Summary

Because we're planning to add more cases when temporary constructors will be inlined, it is important to document the fact that the current heuristic of terminating the analysis upon encountering constructors that correspond to noreturn temporary destructors does not work in this case.

For now these constructors are not inlined for the sole reason that their respective destructors are non-trivial and missing from the CFG. We will hopefully be able to enable temporary destructors in the CFG by default, so it won't be an issue. However, the intent is to maintain the mode without temporary destructors usable in short-term, and this assertion will force us to put the checks for presence of the destructor in the CFG wherever we enable inlining temporary constructors.

In other words, if any new code violates this assertion, it should be fixed by un-inlining this constructor when -analyzer-config cfg-temporary-dtors is set to false.

[NoQ]

Add a comment explaining that it's not really bad to inline the constructor, but we simply have the sink not implemented in this case.

[dcoughlin]

This looks good to me, but I think you should include your explanatory comments in the commit message to the comment itself to help future violators of the assertion out!

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

335 ↗

(On Diff #132316)

Can you add to the comment the instructions from the commit message on what to do if the assertion fires? I..e, "If any new code violates this assertion, it should be fixed by not inlining this constructor when -analyzer-config cfg-temporary-dtors is set to false."

This will give someone who violates the assertion concrete steps to take.

[NoQ]

Add the comment.

cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
   // 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.
   const AnalysisDeclContext *ADC = LCtx->getAnalysisDeclContext();
   if (!ADC->getCFGBuildOptions().AddTemporaryDtors) {
-      const MemRegion *Target = Call->getCXXThisVal().getAsRegion();
-      if (Target && isa<CXXTempObjectRegion>(Target) &&
-          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(CE, N, N->getState());
       }
 
-      // There is no need to run the PostCall and PostStmtchecker
+      // 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 DstPostCall;
   getCheckerManager().runCheckersForPostCall(DstPostCall, DstEvaluated,
                                              *Call, *this);
   getCheckerManager().runCheckersForPostStmt(destNodes, DstPostCall, CE, *this);
 }
 
 void ExprEngine::VisitCXXDestructor(QualType ObjectType,