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

Thank you for writing this down.
Totally agree with you here.


On 12/5/19 6:45 PM, David Holmes wrote:
> 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.

I've mentioned the general discussion you started about JIT compiler
optimizations in one of my previous replies to this review threads.
Sorry, I was busy with other things and was not able to participate in 
it properly.
But I'm looking forward to continue this when there is a chance.

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

It is painful that we have not established it yet.

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

Right.

Thanks,
Serguei

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