JVMTI retransformation and addition of private methods

David Holmes david.holmes at oracle.com
Thu Feb 22 01:10:47 UTC 2018 

Correction ...

On 22/02/2018 10:59 AM, David Holmes wrote:
> On 22/02/2018 7:44 AM, serguei.spitsyn at oracle.com wrote:
>> Hi Karen,
>>
>> Thank you for sorting this out!
>>
>>
>> On 2/21/18 09:55, Karen Kinnear wrote:
>>> Dan,
>>>
>>> Thank you for all the background digging. This is really helpful.
>>>
>>> Serguei - do you know what tests exist for this behavior?
>>
>> Dan already replied (thanks, Dan!)
>> There are two tests in the open/test/jdk/java/lang/instrument:
>> RedefineMethodAddInvoke*
>> RedefineMethodDelInvoke*
>>
>>
>>> The way I read the source code - we currently allow ADD and DELETE for
>>> PRIVATE OR STATIC OR FINAL methods. Did I read that correctly?
>> The above does not look correct to me.
>> We have the same check for both ADD and DELETE method:
>>        if ((old_flags & JVM_ACC_PRIVATE) == 0
>>             // hack: private should be treated as final, but alas
>>            || (old_flags & (JVM_ACC_FINAL|JVM_ACC_STATIC)) == 0
>>           ) {
>>          // deleted methods must be private
>>          return JVMTI_ERROR_UNSUPPORTED_REDEFINITION_METHOD_DELETED;
>>        }
>>
>> I read it as we allow ADD and DELETE for PRIVATE && (STATIC || FINAL) 
>> methods.
>> (Rephrase: We allow PRIVATE FINAL or PRIVATE STATIC methods.)
>> As private should always be treated final then we can simplify the 
>> above to:
>>    We allow and and delete PRIVATE INSTANCE or PRIVATE STATIC methods
>>    which is equal to just "PRIVATE methods".
> 
> I read it as "PRIVATE" or "FINAL STATIC".

Nope I read it wrong - Serguei is right.

Which is somewhat strange as the underlying problem being addressed 
seemed to require retransformation of Object.wait and Thread.sleep - the 
former is a public final instance method; the latter a public static 
method. Neither are private ... ???

David

> David
> -----
> 
>>> With the current implementation, I am not sure if deletion works for 
>>> private methods - do we
>>> have a test for that? Or could you add one as part of this exercise?
>>
>> Yes, we have one j.l.instrument test: RedefineMethodDelInvoke.sh.
>>
>>
>>> Today we create a vtable entry for private methods (my 
>>> misunderstanding ~ 2006ish). After discussions
>>> with David I no longer believe we need those.
>>> Today, klassVtable::adjust_method_entries has an assertion
>>>   assert(!old_method->is_deleted(), “vtable methods may not be deleted”)
>>>
>>> I may have read the code incorrectly - but I would expect to hit this 
>>> assertion if you had a private
>>> method you were deleting that was not final and not static.
>>>
>>> option 1) we could explicitly tighten the restrictions to match what 
>>> we have implemented
>>> option 2) we could make this work by changing 
>>> klassVtable.cpp::update_inherited_vtable
>>>   handling of private fields to be done the way it handles final fields.
>>> option 3) I read the code incorrectly?
>>
>> If we create a vtable entry for private methods then we should hit the 
>> assert above.
>> If we no longer need this then we have no problem.
>>
>> Thanks,
>> Serguei
>>
>>> thanks,
>>> Karen
>>>
>>>> On Feb 21, 2018, at 10:40 AM, Daniel D. Daugherty 
>>>> <daniel.daugherty at oracle.com <mailto:daniel.daugherty at oracle.com>> 
>>>> wrote:
>>>>
>>>> On 2/21/18 2:45 AM, serguei.spitsyn at oracle.com wrote:
>>>>> On 2/20/18 23:01, David Holmes wrote:
>>>>>> On 21/02/2018 4:50 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>> Hi Karen and David,
>>>>>>>
>>>>>>>
>>>>>>> On 2/20/18 19:52, David Holmes wrote:
>>>>>>>> Hi Karen,
>>>>>>>>
>>>>>>>> On 21/02/2018 1:54 AM, Karen Kinnear wrote:
>>>>>>>>> Folks,
>>>>>>>>>
>>>>>>>>> As part of the Valhalla EG discussions for JVMTI changes for 
>>>>>>>>> nestmates (thank you Serguei and David),
>>>>>>>>> IBM brought up a request that we update the JVMTI documentation 
>>>>>>>>> to reflect that we allow addition
>>>>>>>>> of private methods.
>>>>>>>>>
>>>>>>>>> Is there a reason we do not document this? I’m inviting those 
>>>>>>>>> who were involved at the time - please include
>>>>>>>>> others if needed.
>>>>>>>
>>>>>>> I support documenting this in the JVMTI spec and had a plan to 
>>>>>>> fix it in 11.
>>>>>>> However, it is not clear to me yet if we have a consensus on it.
>>>>>>
>>>>>> I would like to see a detailed analysis of the implications of 
>>>>>> allowing this. I _think_ it is safe but ...
>>>>>
>>>>> Valid concern.
>>>>> Also, I'd love to collect more details on the initial motivation to 
>>>>> relax the JVMTI spec.
>>>>> Most likely we had no CCC/CSR filed on this change.
>>>>>
>>>>>
>>>>>>>> This issue is tracked by:
>>>>>>>>
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8192936
>>>>>>>>
>>>>>>>> "RI does not follow the JVMTI RedefineClasses spec that is too 
>>>>>>>> strict in the definition"
>>>>>>>
>>>>>>> Yes, this is the one.
>>>>>>> Thank you, David, for posting the link.
>>>>>>>
>>>>>>>
>>>>>>>> As I wrote there ... It is not at all clear how JDK-6404550 
>>>>>>>> morphed into "Permit the adding or deleting of private 
>>>>>>>> final/static methods with redefine" - nor why those changes 
>>>>>>>> failed to make any change to the spec itself. It is also unclear 
>>>>>>>> whether the add/delete is restricted to final/static methods or 
>>>>>>>> any private method? I can see that the intent was to only allow 
>>>>>>>> something that would not perturb the vtable for existing instances.
>>>>>>>
>>>>>>> I agree, there is a confusion somewhere.
>>>>>>> Is it possible, the JDK-6404550 in JIRA is a different bug than 
>>>>>>> the one in the Bugtraq system?
>>>>>>>
>>>>>>> The JDK-6404550 in JIRA has a different synopsis:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-6404550
>>>>>>>       Cannot implement late attach in NetBeans Profiler due to 
>>>>>>> missing functionality in JVMTI
>>>>>>
>>>>>> Digging deeper ... to fix the problem described in that bug they 
>>>>>> augmented JVM TI to allow private method redefinition as an 
>>>>>> alternate to the "native rebinding" technique that had been used 
>>>>>> previously. See the final comment in:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-6341303
>>>>>>
>>>>>> "JVMTI Spec: Need a way how to rebind Object.wait and Thread.sleep 
>>>>>> with late attach"
>>>>>>
>>>>>> which was closed as a duplicate.
>>>>>
>>>>> Thank you for the point.
>>>>> This explains it.
>>>>> It seems, the bug synopsis was changed at some moment.
>>>>
>>>> The synopsis for 6404550 has never changed. Here's the subject line 
>>>> when
>>>> it was created on 2006.03.27:
>>>>
>>>> > CR 6404550 *HOT* Created P1 hotspot/jvmti Cannot implement late 
>>>> attach in NetBeans Profiler due to missing functionality in JVMTI
>>>>
>>>> I think the confusion arises over comments like this in 6341303:
>>>>
>>>>> rfield Robert Field 
>>>>> <https://bugs.openjdk.java.net/secure/ViewProfile.jspa?name=rfield> 
>>>>> added a comment - 2006-05-04 11:54
>>>>> BT2:EVALUATION
>>>>>
>>>>> This can now be accomplished with Java programming language 
>>>>> instrumentation, via:
>>>>>
>>>>>      6404550: missing late attach (JVM TI redefine) functionality
>>>>>            Permit the adding or deleting of private final/static 
>>>>> methods with redefine
>>>>>
>>>>> Closing this bug as a duplicate.
>>>>
>>>> That's just Robert's style for an sccs delta comment:
>>>>
>>>> D 1.65.2.3 06/04/25 23:36:35 rfield 140 139 00023/00013/03263
>>>> MRs:
>>>> COMMENTS:
>>>> 6404550: missing late attach (JVM TI redefine) functionality
>>>>             Add/delete private methods, continued: changes per review
>>>>
>>>> Back in the ancient past we tried to include some brief
>>>> info about the change in the delta comment. This was one of many
>>>> deltas associated with 6404550.
>>>>
>>>> Please see the attached email that I sent on 2012.12.17 about the
>>>> history behind this issue... (sent to Karen, Mikael V, and Serguei)
>>>>
>>>> It seems I forwarded that same email to Coleen, Markus G and Serguei
>>>> back on 2014.05.20. Since Markus is on that thread, it must have had
>>>> something to do with research about JFR...
>>>>
>>>> I need to do a detailed read thru my e-mail archive for 6404550 to
>>>> see if I can spot some clues about why we didn't do a spec update.
>>>>
>>>> Dan
>>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>>
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>>>
>>>>>>>
>>>>>>>> -- 
>>>>>>>> David
>>>>>>>>
>>>>>>>>
>>>>>>>>> thanks,
>>>>>>>>> Karen
>>>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>>
>>>> <Attached Message.eml>
>>>
>>

More information about the serviceability-dev mailing list