[analyzer] Avoid out-of-order node traversal on void return (#117863)

The motivating example: https://compiler-explorer.com/z/WjsxYfs43
```C++
#include <stdlib.h>
void inf_loop_break_callee() {
  void* data = malloc(10);
  while (1) {
    (void)data; // line 3
    break; // -> execution continues on line 3 ?!!
  }
}
```

To correct the flow steps in this example (see the fixed version in the
added test case) I changed two things in the engine:
- Make `processCallExit` create a new StmtPoint only for return
  statements. If the last non-jump statement is not a return statement,
  e.g. `(void)data;`, it is no longer inserted in the exploded graph after
  the function exit.
- Skip the purge program points. In the example above, purge
  points are still inserted after the `break;` executes. Now, when the bug
  reporter is looking for the next statement executed after the function
  execution is finished, it will ignore the purge program points, so it
  won't confusingly pick the `(void)data;` statement.

CPP-5778

Author: necto

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

@@ -2416,6 +2416,28 @@ PathSensitiveBugReport::getRanges() const {
   return Ranges;
 }
 
+static bool exitingDestructor(const ExplodedNode *N) {
+  // Need to loop here, as some times the Error node is already outside of the
+  // destructor context, and the previous node is an edge that is also outside.
+  while (N && !N->getLocation().getAs<StmtPoint>()) {
+    N = N->getFirstPred();
+  }
+  return N && isa<CXXDestructorDecl>(N->getLocationContext()->getDecl());
+}
+
+static const Stmt *
+findReasonableStmtCloseToFunctionExit(const ExplodedNode *N) {
+  if (exitingDestructor(N)) {
+    // If we are exiting a destructor call, it is more useful to point to
+    // the next stmt which is usually the temporary declaration.
+    if (const Stmt *S = N->getNextStmtForDiagnostics())
+      return S;
+    // If next stmt is not found, it is likely the end of a top-level
+    // function analysis. find the last execution statement then.
+  }
+  return N->getPreviousStmtForDiagnostics();
+}
+
 PathDiagnosticLocation
 PathSensitiveBugReport::getLocation() const {
   assert(ErrorNode && "Cannot create a location with a null node.");
@@ -2433,15 +2455,7 @@ PathSensitiveBugReport::getLocation() const {
       if (const ReturnStmt *RS = FE->getStmt())
         return PathDiagnosticLocation::createBegin(RS, SM, LC);
 
-      // If we are exiting a destructor call, it is more useful to point to the
-      // next stmt which is usually the temporary declaration.
-      // For non-destructor and non-top-level calls, the next stmt will still
-      // refer to the last executed stmt of the body.
-      S = ErrorNode->getNextStmtForDiagnostics();
-      // If next stmt is not found, it is likely the end of a top-level function
-      // analysis. find the last execution statement then.
-      if (!S)
-        S = ErrorNode->getPreviousStmtForDiagnostics();
+      S = findReasonableStmtCloseToFunctionExit(ErrorNode);
     }
     if (!S)
       S = ErrorNode->getNextStmtForDiagnostics();

clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp

@@ -349,6 +349,8 @@ const Stmt *ExplodedNode::getStmtForDiagnostics() const {
 
 const Stmt *ExplodedNode::getNextStmtForDiagnostics() const {
   for (const ExplodedNode *N = getFirstSucc(); N; N = N->getFirstSucc()) {
+    if (N->getLocation().isPurgeKind())
+      continue;
     if (const Stmt *S = N->getStmtForDiagnostics()) {
       // Check if the statement is '?' or '&&'/'||'.  These are "merges",
       // not actual statement points.

clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

@@ -353,14 +353,19 @@ void ExprEngine::processCallExit(ExplodedNode *CEBNode) {
   ExplodedNodeSet CleanedNodes;
   if (LastSt && Blk && AMgr.options.AnalysisPurgeOpt != PurgeNone) {
     static SimpleProgramPointTag retValBind("ExprEngine", "Bind Return Value");
-    PostStmt Loc(LastSt, calleeCtx, &retValBind);
+    auto Loc = isa<ReturnStmt>(LastSt)
+                   ? ProgramPoint{PostStmt(LastSt, calleeCtx, &retValBind)}
+                   : ProgramPoint{EpsilonPoint(calleeCtx, /*Data1=*/nullptr,
+                                               /*Data2=*/nullptr, &retValBind)};
+    const CFGBlock *PrePurgeBlock =
+        isa<ReturnStmt>(LastSt) ? Blk : &CEBNode->getCFG().getExit();
     bool isNew;
     ExplodedNode *BindedRetNode = G.getNode(Loc, state, false, &isNew);
     BindedRetNode->addPredecessor(CEBNode, G);
     if (!isNew)
       return;
 
-    NodeBuilderContext Ctx(getCoreEngine(), Blk, BindedRetNode);
+    NodeBuilderContext Ctx(getCoreEngine(), PrePurgeBlock, BindedRetNode);
     currBldrCtx = &Ctx;
     // Here, we call the Symbol Reaper with 0 statement and callee location
     // context, telling it to clean up everything in the callee's context

clang/test/Analysis/copy-elision.cpp

@@ -263,17 +263,17 @@ void testVariable() {
 
 struct TestCtorInitializer {
   ClassWithDestructor c;
-  TestCtorInitializer(AddressVector<ClassWithDestructor> &v)
-    : c(ClassWithDestructor(v)) {}
-  // no-elide-warning@-1 {{Address of stack memory associated with temporary \
-object of type 'ClassWithDestructor' is still referred \
-to by the caller variable 'v' upon returning to the caller}}
+  TestCtorInitializer(AddressVector<ClassWithDestructor> &refParam)
+    : c(ClassWithDestructor(refParam)) {}
 };
 
 void testCtorInitializer() {
   AddressVector<ClassWithDestructor> v;
   {
     TestCtorInitializer t(v);
+    // no-elide-warning@-1 {{Address of stack memory associated with temporary \
+object of type 'ClassWithDestructor' is still referred \
+to by the caller variable 'v' upon returning to the caller}}
     // Check if the last destructor is an automatic destructor.
     // A temporary destructor would have fired by now.
 #if ELIDE

clang/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp

@@ -163,8 +163,8 @@ class UnguardedFieldThroughMethodTest {
       Volume = 0;
       break;
     case A:
-      Area = 0; // expected-warning {{1 uninitialized field}}
-      break;
+      Area = 0;
+      break; // expected-warning {{1 uninitialized field}}
     }
   }
 
@@ -201,8 +201,8 @@ class UnguardedPublicFieldsTest {
       Volume = 0;
       break;
     case A:
-      Area = 0; // expected-warning {{1 uninitialized field}}
-      break;
+      Area = 0;
+      break; // expected-warning {{1 uninitialized field}}
     }
   }
 

clang/test/Analysis/void-call-exit-modelling.c

@@ -0,0 +1,26 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-output text -verify %s
+
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t size);
+
+void inf_loop_break_callee() {
+  void* data = malloc(10); // expected-note{{Memory is allocated}}
+  while (1) { // expected-note{{Loop condition is true}}
+    (void)data;
+    break; // No note that we jump to the line above from this break
+  } // expected-note@-1{{Execution jumps to the end of the function}}
+} // expected-warning{{Potential leak of memory pointed to by 'data'}}
+// expected-note@-1  {{Potential leak of memory pointed to by 'data'}}
+
+void inf_loop_break_caller() {
+  inf_loop_break_callee(); // expected-note{{Calling 'inf_loop_break_callee'}}
+}
+
+void inf_loop_break_top() {
+  void* data = malloc(10); // expected-note{{Memory is allocated}}
+  while (1) { // expected-note{{Loop condition is true}}
+    (void)data;
+    break; // No note that we jump to the line above from this break
+  } // expected-note@-1{{Execution jumps to the end of the function}}
+} // expected-warning{{Potential leak of memory pointed to by 'data'}}
+// expected-note@-1  {{Potential leak of memory pointed to by 'data'}}