[analyzer] Expose return statement from CallExit program point

[george.karpenkov]

Summary

If the return statement is stored, we might as well allow querying against it.
Also fix the bug where the return statement is not stored if there is no return value.
And expose the return statement through getStatement helper function.

[george.karpenkov]

@NoQ I think this should be OK to commit?

[NoQ]

lib/StaticAnalyzer/Core/CoreEngine.cpp

317

All right, so this is not entirely NFC; it un-merges two ExplodedNodes during call exit when the state is otherwise identical - the CallExitBegin node itself and the "Bind Return Value"-tagged node.

For example:

 1	int coin();
 2
 3	void foo() {
 4	  int x = coin();
 5	  if (x > 0)
 6	    return;
 7	  else
 8	    return;
 9	}
10
11	void bar() {
12	  foo();
13	}

After binding the return value, removeDead is called to remove dead bindings for the entire callee context. It would therefore be called once before the patch and twice after the patch (but yielding just one node in both cases because the resulting node is identical).

Here's a CallbackOrder test that shows that checkLiveSymbols is called twice:

test.patch2 KBDownload

Note that checkDeadSymbols is not called at all because there are no dead symbols here to take care of.

Even though dead symbols is the slowest thing ever in the analyzer, i believe that the overhead here of collecting dead symbols twice would be minimal because the situation when two returns have exactly identical states is relatively rare. If it isn't, then suddenly it might make sense to create a global (path-independent) cache for results of collecting dead symbols over a program state (sounds fun).

[george.karpenkov]

@NoQ I would say the patch still makes sense. We would still get the same behavior with "return 0 / return 0" in two different locations.
IMO two states for two different locations should not be merged, also it indeed seems the situation is rare enough to seriously affect the performance.

[NoQ]

Yep. Could you still add the test (ideally with some explanation of what it tests because i forgot to add it^^)?

[george.karpenkov]

Yep. Could you still add the test

Sure, what would be the testable observable behavior?

[NoQ]

This test ensures that check::LiveSymbols is called as many times on the path through the second "return" as it is through the first "return" (three), and therefore the two paths were not merged prematurely before the respective return statement is evaluated. The paths would still be merged later, so we'd have only one post-call for foo(), but it is incorrect to merge them in the middle of evaluating two different statements.

[george.karpenkov]

Adding the test by Artem.

[NoQ]

Pls squeeze my previous comment into the test?

include/clang/Analysis/ProgramPoint.h

 /// - Run Remove dead bindings (to clean up the dead symbols from the callee).
 /// - CallExitEnd
 class CallExitBegin : public ProgramPoint {
 public:
   // CallExitBegin uses the callee's location context.
   CallExitBegin(const StackFrameContext *L, const ReturnStmt *RS)
     : ProgramPoint(RS, CallExitBeginKind, L, nullptr) { }
 
+  const ReturnStmt *getReturnStmt() const {
+    return static_cast<const ReturnStmt *>(getData1());
+  }
+
 private:
   friend class ProgramPoint;
   CallExitBegin() {}
   static bool isKind(const ProgramPoint &Location) {
     return Location.getKind() == CallExitBeginKind;
   }
 };
 

lib/StaticAnalyzer/Core/CoreEngine.cpp

if (Blk == &(L.getLocationContext()->getCFG()->getExit())) {
 
     assert (L.getLocationContext()->getCFG()->getExit().size() == 0
             && "EXIT block cannot contain Stmts.");
 
     // Get return statement..
     const ReturnStmt *RS = nullptr;
     if (!L.getSrc()->empty()) {
       if (Optional<CFGStmt> LastStmt = L.getSrc()->back().getAs<CFGStmt>()) {
-        if ((RS = dyn_cast<ReturnStmt>(LastStmt->getStmt()))) {
-          if (!RS->getRetValue())
-            RS = nullptr;
-        }
+        RS = dyn_cast<ReturnStmt>(LastStmt->getStmt());
       }
     }
 
     // Process the final state transition.
     SubEng.processEndOfFunction(BuilderCtx, Pred, RS);
 
     // This path is done. Don't enqueue any more nodes.
     return;