Review Request (M) 7187554: JSR 292: JVMTI PopFrame needs to handle appendix arguments

[Coleen Phillimore]

Serguei,

This looks good.  It's a lot cleaner than I thought it'd be.  Do more of 
these nsk.mlvm tests pass with this change?

Thanks!
Coleen

On 07/30/2013 01:00 PM, serguei.spitsyn at oracle.com wrote:
> The updated webrev:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2013/hotspot/7187554-JVMTI-JSR292.3/ 
>
>
> Thanks,
> Serguei
>
> On 7/29/13 10:11 PM, David Holmes wrote:
>> Hi Serguei,
>>
>> I'm fine with restoring to what was in the first webrev.
>>
>> Further trimming of the JVMTI code is something the embedded folk 
>> could look at. It may not be worthwhile.
>>
>> Thanks,
>> David
>>
>> On 30/07/2013 3:05 PM, serguei.spitsyn at oracle.com wrote:
>>> On 7/29/13 8:22 PM, David Holmes wrote:
>>>> On 30/07/2013 10:34 AM, serguei.spitsyn at oracle.com wrote:
>>>>> Christian and David,
>>>>>
>>>>> Thank you for the quick review and comments!
>>>>>
>>>>> This is a new version of webrev:
>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2013/hotspot/7187554-JVMTI-JSR292.2 
>>>>>
>>>>>
>>>>
>>>> Sorry. You need that guard after all - at least you do if you continue
>>>> to use it in interpreterRuntime - otherwise member_name_arg_or_null
>>>> will not exist:
>>>>
>>>>   __ call_VM(rax, CAST_FROM_FN_PTR(address,
>>>> InterpreterRuntime::member_name_arg_or_null), rax, rdx, rsi);
>>>>
>>> You are right, nice catch again.
>>> Probably, it was the reason, I did not remove the #if/#endif in the
>>> first place. :)
>>>
>>>> I'm a little surprised that the assembly code does not have that whole
>>>> section guarded with INCLUDE_JVMTI - perhaps it should?
>>> It should.
>>> But it can be non-trivial because of other dependencies like the above.
>>> To make it right, both _remove_activation_preserving_args_entry and
>>> generate_earlyret_entry_for
>>> must be isolated with #if INCLUDE_JVMTI.
>>> Then more files have to be involved in this chain of changes:
>>>    interpreter/templateInterpreter.hpp
>>>    interpreter/templateInterpreter.hpp
>>>    memory/universe.hpp
>>>    memory/universe.cpp
>>>    code/codeCache.hpp
>>>    code/codeCache.cpp
>>>    . . . etc ..
>>>
>>> Please, note, that the HOTSWAP macro is used in many places as well.
>>> I'm not sure we still need it, so that another decision is needed 
>>> for it.
>>>
>>> So, it seems that this kind of clean up is going far beyond my fix.
>>> My suggestion is to restore the "#if INCLUDE_JVMTI" in 3
>>> platform-dependent files as it was in the webrev v1.
>>> I'm a little bit reluctant to open another clean-up bug for this issue
>>> but maybe it is needed.
>>> Please, let me know if you are comfortable with this solution.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>>
>>>> Run this through a JPRT test build for productEmb to check that the
>>>> minimal VM builds ok.
>>>>
>>>> David
>>>> -----
>>>>
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 7/28/13 9:11 PM, David Holmes wrote:
>>>>>> Hi Serguei,
>>>>>>
>>>>>> On 26/07/2013 10:14 AM, serguei.spitsyn at oracle.com wrote:
>>>>>>>
>>>>>>> Please, review the fix for:
>>>>>>>    bug: http://bugs.sun.com/view_bug.do?bug_id=7187554
>>>>>>>    jbs: https://jbs.oracle.com/bugs/browse/JDK-7187554
>>>>>>>
>>>>>>> Open webrev:
>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2013/hotspot/7187554-JVMTI-JSR292.1 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> In the templateInterpreter code why did you put this guard on 
>>>>>> your new
>>>>>> code (from x86_32 version):
>>>>>>
>>>>>> 1923 #if INCLUDE_JVMTI
>>>>>>
>>>>>> when the whole chunk of code this is situated in is specifically for
>>>>>> JVMTI support
>>>>>>
>>>>>> 1824   //
>>>>>> 1825   // JVMTI PopFrame support
>>>>>> 1826   //
>>>>>>
>>>>>> ???
>>>>>>
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>>
>>>>>>> Summary:
>>>>>>>    Restore the appendix argument of a polymorphic intrinsic call
>>>>>>>    needed for a invokestatic re-execution after JVMTI PopFrame().
>>>>>>>
>>>>>>> Description
>>>>>>>    When JVMTI's PopFrame removes a frame that was called via a call
>>>>>>> site
>>>>>>> that
>>>>>>>    takes an appendix and that call site is reexecuted the 
>>>>>>> appendix is
>>>>>>> not on
>>>>>>>    the stack anymore because it got removed by the adapter.
>>>>>>>    This fix is to detect such a case and push the appendix on the
>>>>>>> stack
>>>>>>> again before reexecution.
>>>>>>>
>>>>>>>
>>>>>>> Testing:
>>>>>>>    UTE tests - in progress: vm.mlvm.testlist, nsk.jvmti.testlist,
>>>>>>> nsk.jdi.testlist
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>
>>>
>