JVMTI retransformation and addition of private methods

David Holmes david.holmes at oracle.com
Thu Feb 22 00:59:30 UTC 2018 

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".

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