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

[serguei.spitsyn at oracle.com]

Coleen,

Thank you a lot for reviewing!

At least, one mlvm test is reliably passing with this fix:
   vm/mlvm/indy/func/jvmti/stepBreakPopReturn

It is possible that the fix positively impacts more tests but the 
picture is not
clear yet as it is masked by a couple of more jsr-292 related jvmti and 
test bugs.

Thanks,
Serguei

On 8/6/13 2:52 PM, Coleen Phillimore wrote:
>
> 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
>>>>>>
>>>>
>>
>