[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: JVMTI retransformation and addition of private methods
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2018-02-22 1:13:18
Message-ID: e4e7f454-d395-f86d-7f9e-7d35d8b50c41 () oracle ! com
[Download RAW message or body]
On 2/21/18 16:59, David Holmes wrote:
> On 22/02/2018 7:44 AM, serguei.spitsyn@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".
There are multiple negations above, so this needs to be interpreted in a
right context:
Return error if deleted method is !PRIVATE or (!FINAL and !STATIC)
After inversion:
Allow to delete if method is PRIVATE and (FINAL or STATIC)
I feel myself stupid when reading this code. :)
Thanks,
Serguei
> 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@oracle.com <mailto:daniel.daugherty@oracle.com>>
>>>> wrote:
>>>>
>>>> On 2/21/18 2:45 AM, serguei.spitsyn@oracle.com wrote:
>>>>> On 2/20/18 23:01, David Holmes wrote:
>>>>>> On 21/02/2018 4:50 PM, serguei.spitsyn@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>
>>>
>>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic