[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: Review Request (M) 7187554: JSR 292: JVMTI PopFrame needs to handle appendix arguments
From:       "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date:       2013-07-30 17:00:46
Message-ID: 51F7F13E.8040104 () oracle ! com
[Download RAW message or body]

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@oracle.com wrote:
>> On 7/29/13 8:22 PM, David Holmes wrote:
>>> On 30/07/2013 10:34 AM, serguei.spitsyn@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@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
>>>>
>>

[prev in list] [next in list] [prev in thread] [next in thread]