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"

[serguei.spitsyn at oracle.com]

Hi Chris,


On 11/8/19 16:55, Chris Plummer wrote:
> Hi Alex,
>
> Comments below:
>
> On 11/8/19 4:39 PM, Alex Menkov wrote:
>>
>>
>> On 11/08/2019 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/
> I don't really see a resolution in the JDK-8215196 comments as to what 
> is actually broken. Are we sure we want to fix this in the test, and 
> not require different behavior by the compiler (and also clarify the 
> spec)?

This is one more case to the topic "Should optimizations be observable 
for JVMTI agents?"
which was recently discussed on the serviceability-dev mailing list.
The question is about the following statement of the JVMTI PopFrame spec:
  "Note however, that any changes to the arguments, which occurred in 
the called method, remain;
   when execution continues, the first instruction to execute will be 
the invoke."

One point is we could consider this statement as a possible side affect 
which has to be preserved by the JIT compiler.
So, the optimization to delete such a code which "looks useless" has to 
be disabled.

Another point is that it is hard to understand why such a side effect 
can be really useful.
Maybe it was specified like this just because it does not make sense to 
preserve original argument values at the call side.
We can 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;"

Let's wait for other opinions.

Thanks,
Serguei

>>>
>>> Currently PopFrame is disabled with JVMCI by [1], so for testing I 
>>> reverted [1] changes.
>>
>> Just to be clear - I temporary reverted [1] for test runs.
>>
> The description for JDK-8218025 says that the intention is to only 
> disable these capabilities for JDK12. Is there a CR to re-enabled them?
>
> thanks,
>
> Chris
>> --alex
>>
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8218025
>>>
>>> --alex
>