[9] RFR(S): 8006960: hotspot, "impossible" assertion failure

[Vladimir Kozlov]

Nicely done, Tobias!

Good analysis. Fix looks correct and thank you for the test.

 > One problem I see with the current implementation is that 'BCEscapeAnalyzer::is_return_allocated' returns false 
whenever an allocated object escapes. Even if the escaped object is not the one that is returned. Or am I missing something?

Correct and it is fine. The analysis is very conservative because it would require to track a lot more information to be 
more precise and it will be sloooow. It covers simple cases and it is enough for now. Yes, it could be improved but 
instead we have to try to implement EA based inlining which will inline such case and do normal analysis on C2 graph.

Thanks,
Vladimir

On 2/26/15 7:59 AM, Tobias Hartmann wrote:
> Hi,
>
> please review the following patch.
>
> https://bugs.openjdk.java.net/browse/JDK-8006960
> http://cr.openjdk.java.net/~thartmann/8006960/webrev.00/
>
> Problem:
> A javac test fails with an assert that checks for object equality and should never fail. I was able to pin the problem down to C2 EA that uses the BCEscapeAnalyzer to get the escape state of a newly allocated object returned by a non-inlined method [1]. EA wrongly assumes that the returned object does not escape. As a result, the 'OptimizePtrCompare' optimization removes the pointer comparison and replaces it by "not equal". [2]
> I was finally able to reproduce the problem with a simple regression test (see 'TestEscapeThroughInvoke.java'). While compiling the 'run' method, C2 computes the escape state of the object returned by 'create'. EA correctly determines that the object escapes while being passed to 'A::saveInto' as argument 0 (see [3]). However, EA marks 'create' as 'return allocated value' because only the escape states of caller arguments are adjusted after a call (see 'BCEscapeAnalyzer::invoke').
>
> Solution:
> Always adjust escape state of allocated objects after an invoke, even if they are not arguments of the caller method. As a result, 'create' is marked as 'return non-local value' [4].
>
> One problem I see with the current implementation is that 'BCEscapeAnalyzer::is_return_allocated' returns false whenever an allocated object escapes. Even if the escaped object is not the one that is returned. Or am I missing something?
>
> Testing:
> - 1k runs of failing test
> - JPRT with regression test
>
> Thanks,
> Tobias
>
>
> [1] See 'ConnectionGraph::add_call_node' (line 851 of escape.cpp).
>
> [2] See 'ConnectionGraph::optimize_ptr_compare' (line 1888 of escape.cpp).
>
> [3] Output of '-XX:+PrintEscapeAnalysis -XX:BCEATraceLevel=3':
>    [EA] estimated escape information for A::saveInto
>         non-escaping args:      {1-2}
>         stack-allocatable args: {1-2}
>         return non-local value
>         modified args:     0    0    0
>         flags:
>    [EA] estimated escape information for EscapeTest::create (not stored)
>         non-escaping args:      {}
>         stack-allocatable args: {0-1}
>         return allocated value
>         modified args:     0    0
>         flags:  return_allocated
>
> [3] Output of fixed version
>    [EA] estimated escape information for A::saveInto
>         non-escaping args:      {1-2}
>         stack-allocatable args: {1-2}
>         return non-local value
>         modified args:     0    0    0
>         flags:
>    [EA] estimated escape information for EscapeTest::create (not stored)
>         non-escaping args:      {}
>         stack-allocatable args: {0-1}
>         return non-local value
>         modified args:     0    0
>         flags:  return_allocated allocated_escapes
>