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"

Fri Dec 6 21:52:32 UTC 2019

On 12/6/19 1:18 PM, serguei.spitsyn at oracle.com wrote:
> On 12/6/19 11:07, Chris Plummer wrote:
>> 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?
>
> My interpretation is that the current JVM TI PopFrame behavior does 
> not break this model.
> The spec says: "any changes to the arguments, which occurred in the 
> called method, remain;"
> As the code was eliminated by the compiler then no changes to this 
> argument occurred.
> So, the PopFrame behavior follows the spec. So, I think, the option #2 
> is not right. But it depends on our basic philosophy.
> If the developer wants to control the agent then the program has to be 
> designed to do something meaningful that is not going to be optimized 
> out by the JIT compiler.
You misunderstood my point. What I'm saying is that someone might do 
something like assign to a local with the specific intent of having that 
trigger a jmvti event, with the specific intent of having the agent 
perform some expected action as a result. Think of it as being a trigger 
for the agent, not as the agent monitoring the app. For example, you 
could right a program + agent, and setting a specific local in the 
program triggers the agent to turn on a light, and setting some other 
local turns it off. Absurd, but possible, and maybe there are less 
absurd applications.

Chris
>
>>>
>>>> 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.
>
> Agreed. The term "restoring" is not accurate here.
>
>> 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.
>
> Right.
>
>>>
>>> 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.
>
> Even with this model it is possible and better to do something 
> meaningful to control the agent.
> This model is very rare use case.
> It is hard to justify a 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".
>
> If the method is not actually being called then missing breakpoints 
> there gives a clue what is going on.
> Otherwise, it will cause cause some serious head scratching for a 
> debugger user.
> In general, my preference would be to debug actual behavior.
> It is not good we have no support breakpoints in compiled methods.
>
>
>>>
>>> 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?
>
> I thought, it is a little bit early to file a bug for it.
> Also, probably, it can be an umbrella enhancement or task.
>
> Thanks,
> Serguei
>
>>
>> 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
>>>>>
>>>>
>>
>>
>