RFR: JDK-8215196: [Graal] vmTestbase/nsk/jvmti/PopFrame/popframe003/TestDescription.java fails with "changes for the arguments of the popped frame's method, did not remain current argument values"

[David Holmes]

Hi Serguei,

On 6/12/2019 11:31 am, serguei.spitsyn at oracle.com wrote:
> Hi Chris and Alex,
> 
> (I've also included Dan, David and Dean to the mailing list)
> 
> We have to reach a consensus about this.

This is just part of a much broader issue with JVM TI that I tried to 
have a discussion started based on Richard Reingruber's proposals around 
Escape Analysis:

http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-September/029285.html

Unfortunately that discussion did not get much traction.

> We have 3 options:
> 
> Option #1:
>    The JIT optimization to delete a code which "looks useless"
>    has to be disabled if can_pop_frame capability is enabled.
>    Than this problem becomes a JIT compiler bug.
> 
> Option #2:
>    Consider to relax the JVMTI PopFrame spec by changing it to something 
> like:
>    "Note however, that the original argument values are not
>     preserved and can be changed by the called method;"
>    Than this problem becomes a JVM TI spec bug.
> 
> Option #3:
>    Consider it is Okay for compiler to eliminate useless code,
>    so the argument values can be reinitialized by the PopFrame.
>    Than this problem becomes just a test bug.
> 
> 
> My preference is option #3.
> The point is that if the arguments are not really used in
> a method then restoring them to any values is a no-op.
> It is really meaningless use case, so why should we care about it.

Thanks for setting that out clearly.

I'd like to agree this is particular case is a test bug. If we have a 
method:

int incr(int val) {
   val++;
   popFrameHere();
   return val;
}

then the change to the argument is necessary and must be preserved. In 
contrast:

void incr(int val) {
   val++;
   popFrameHere();
}

the change to the argument is meaningless and I would hope any decent 
JIT would simply elide it.

But we must have a consistent approach to such things. What would happen 
if a breakpoint were to be placed on the instruction that uselessly 
modified the argument - would we still see the modification or would it 
be elided?

And how do C1 and C2 avoid this issue? Do they simply not optimise away 
the useless assignment? Or do they actively disable that optimization in 
this context?

We need, IMO, to establish the basic philosophy of how to manage JVM TI 
/ JIT interactions, so we know what things must remain visible and which 
can be optimised away.

That said, changing the test allows us to defer having to reach that 
consensus.

David
-----

> Thanks,
> Serguei
> 
> 
> On 11/11/19 3:17 AM, serguei.spitsyn at oracle.com wrote:
>> Hi Alex,
>>
>> The fix itself looks Okay.
>> Minor: replace in the comment: "compiler don't drop" => "compiler 
>> doesn't drop".
>>
>> However, we still have to reach a consensus on how we treat this issue 
>> (as Chris already commented).
>>
>> Thanks,
>> Serguei
>>
>>
>> On 11/8/19 15:22, Alex Menkov wrote:
>>> Hi all,
>>>
>>> Please review the fix for
>>> https://bugs.openjdk.java.net/browse/JDK-8215196
>>> webrev:
>>> http://cr.openjdk.java.net/~amenkov/jdk14/popframe_args/webrev/
>>>
>>> Currently PopFrame is disabled with JVMCI by [1], so for testing I 
>>> reverted [1] changes.
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8218025
>>>
>>> --alex
>>
>