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:31:57 UTC 2019 

On 12/6/19 6:12 PM, serguei.spitsyn at oracle.com wrote:
> On 12/6/19 17:24, Daniel D. Daugherty wrote:
>> On 12/6/19 6: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.
>>
>> You are making the assumption that the agent author understands what
>> Java code/variables *might* be eliminated by the JIT compiler. I don't
>> think that's a good assumption. I might have code that does a really
>> complicated thing in a local variable that is only useful to the
>> agent itself. The JIT will see that the local variable cannot escape
>> the function and is not used outside the function (as far as it can
>> see) so it will elide the local variable and the code that was used
>> to generated the local result in the variable.
>>
>> If that local result happens to be some computation that the agent
>> needed to see to do its next operation...
>
> Thank you for sharing your point.
> I'm not insisting on my assumptions here, just not sure this is more 
> important than allowing optimizations.
> Do you actually think this use case needs to be supported?
>
> In general, to identify our philosophy about interaction between JIT 
> compiler
> code elimination and JVM TI we need to make some assumptions.
>
>
> Let's temporarily put JVM TI out of scope.
> Are there any assumptions when JIT compilers eliminate some code?
> Is it based on some vision what code is observable?
> If it was decided some code can be eliminated then is it JVM TI only 
> that breaks such assumptions about observability?
>
> If so, then such optimizations can be disabled at some level.
> Then we end up debugging/profiling/monitoring, and finally, observing 
> a slightly different application.
> Are we Okay with this? Do we need any compromises here?
> Maybe we need more flags to control the JIT compiler behavior.
>
Dan is restating the point I was making, but I also agree that unless 
someone can show us a useful application of that kind of use of jvmti 
events, I don't think we need to support it. We do need to clarify it in 
the spec however.

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