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"

[Chris Plummer]

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.
Hmm. I have the emails that precede yours above, but not that one. Not 
sure how what happened. Just read through it and it did give me one 
thought. Consider a model where the program is designed drive behavior 
of the agent, triggering the agent to do certain things by having the 
program do certain things. Normally an agent monitors the application, 
but in this case the application is purposefully controlling actions 
performed by the agent. If code is elided from the program, then the 
agent no longer performs as expected. It's a kind of backwards jvmti 
programming model, and you may ask why would anyone do this. I'm not 
sure if there's a good reason for it, but should it be expected to work 
given how the spec is written?
>
>> 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.
Is "restoring" the proper term here? I thought they were just left on 
the stack and reused on the subsequent invoke. In fact I figured the 
reason for the language in the spec in the first place is to alleviate 
JVMTI from having to restore them to their original values, which is 
probably not even possible.
>
> 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.
So, this goes back to my example above where the program is trying to 
elicit behavior from the agent. It's not meaningless in that case, but 
that doesn't mean I think we need to support 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?
Breakpoints force interpreted mode for the method, although I suppose 
that's a hotspot implementation detail and not something a VM would be 
required to do. A VM that allows breakpoints in compiled methods has the 
potential to miss the breakpoint if code is elided.

Also, what if you put a breakpoint in a method, the call to it is 
elided. You would never hit the breakpoint. That could cause some 
serious head scratching for a debugger user if they know the code doing 
the method call is "executed".

>
> 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.
Agreed. I think it's ok to work around the test issue as long as we keep 
this overall issue on the radar. Do we have a bug field for that?

thanks,

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