[PATCH] D25326: [StaticAnalyser] Don't merge different returns in ExplodedGraph

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

In https://reviews.llvm.org/D25326#564082, @NoQ wrote:

> Funny facts about the Environment:
>
> (a) Environment allows taking SVals of ReturnStmt, which is not an expression, by transparently converting it into its sub-expression. In fact, it only stores expressions.
>
> Having just noticed (a), i also understand that (a) is not of direct importance to the question, however:
>
> (b)  With a similar trick, Environment allows taking SVal of literal expressions, but never stores literal expressions. Instead, it reconstructs the constant value by looking at the literal and returns the freshly constructed value when asked.
>
> This leads to "return 0;" and "return 1;" branches having the same program state. The difference should have been within Environment (return values are different), however return values are literals, and they aren't stored in the Environment, and hence the Environment is equal in both states. If only the function returned, say, 0 and i, the problem wouldn't have been there.


I did not know "a" and "b", thanks for info.

> This leaves us with two options:
> 
> 1. Tell the Environment to store everything. This would make things heavy. However, i do not completely understand the consequences of this bug at the moment - perhaps there would be more problems due to this state merging unless we start storing everything.
> 2. Rely on the ProgramPoint to remember where we are. After all, why do we merge when program points should clearly be different? The program point that fails us is CallExitBegin - we could construct it with ReturnStmt and its block count to discriminate between different returns. It should fix this issue, and is similar to the approach taken in this patch (just puts things to their own place), however, as i mentioned, there might be more problems with misbehavior (b) of the Environment, need to think.



1. yes sounds heavy.
2. I assume you are right.. however as I understand it the ProgramPoint when CallExitBegin is called is the same (in the exit block). do you suggest that we take the ProgramPoint for the exit block's predecessor?

> Finally, i'm not quite sure why CallExitBegin is at all necessary. I wonder if we could just remove it and jump straight to Bind Return Value, also need to think.

me too. however there is much I wonder about when it comes to ExplodedGraph. :-)


Repository:
  rL LLVM

https://reviews.llvm.org/D25326