This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Expose return statement from CallExit program point
ClosedPublic

Authored by george.karpenkov on Jan 16 2018, 1:50 PM.

Details

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.

Diff Detail

Event Timeline

george.karpenkov edited the summary of this revision. (Show Details)

@NoQ I think this should be OK to commit?

NoQ added inline comments.Jan 19 2018, 2:44 PM
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:


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).

@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 added a comment.Jan 19 2018, 3:04 PM

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

Yep. Could you still add the test

Sure, what would be the testable observable behavior?

NoQ added a comment.Jan 19 2018, 3:35 PM

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.

Adding the test by Artem.

NoQ accepted this revision.Jan 31 2018, 6:57 PM

Pls squeeze my previous comment into the test?

This revision is now accepted and ready to land.Jan 31 2018, 6:57 PM
This revision was automatically updated to reflect the committed changes.