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 chris.plummer at oracle.com
Sat Dec 7 05:28:42 UTC 2019 

On 12/6/19 3:26 PM, serguei.spitsyn at oracle.com wrote:
> On 12/6/19 13:52, Chris Plummer wrote:
>> 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.
>
> I think, I understood your point correctly.
> Your point is that the code that can be eliminated (e.g. local++) is 
> not that meaningless as it seems to be.
> My point is that there are still other more reliable ways to trigger 
> the agent.
> So that relying on something that can be eliminated by JIT compilers 
> is not important to support.
>
Yes, I wasn't trying to imply that it is important. However, the spec 
should be clear about it.

Chris
> Thanks,
> Serguei
>
>> 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
>>>>>>>
>>>>>>
>>>>
>>>>
>>>
>>
>>
>

More information about the serviceability-dev mailing list