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]

On 11/11/19 03:06, serguei.spitsyn at oracle.com wrote:
> 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;"

Forgot to list one more option which is:
   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.


Thanks,
Serguei

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