Sergeui,
You were right - I read the sources incorrectly. Would still help to understand
both
the motivation and the reason to not add to the spec.
Robert - do you remember why we didn’t add this to the specification? (6404550)
> On Feb 21, 2018, at 4:44 PM, serguei.spit...@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”.
Sergeui - you are right - I misread this today.
And I played with the tests a bit - thanks Dan - and it matches the way you
read this.
So today, private methods that are not marked as final in the source code can
not be added -
I tried that variation by modifying the RedefineMethodAddInvokeTarget_1.java
and changing
private final void myMethod1() to private void myMethod1() and got an
UnsupportedOperationException.
So - private methods are not marked as ACC_FINAL today so I think the
simplification doesn’t
apply, so we are left with the ability to ADD or DELETE
PRIVATE && (STATIC || FINAL) methods - at least that is what we support
today.
>
>> 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.
I tried the test. And it works because of the requirement that FINAL or STATIC
are set,
which therefore means no vtable entry.
>
>
>> 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.
We do create a vtable entry for private methods; however if FINAL or STATIC is
set, then
we do not create a vtable entry.
That is why we don’t ever get here.
thanks,
Karen
>
> Thanks,
> Serguei
>
>> thanks,
>> Karen
>>
>>> On Feb 21, 2018, at 10:40 AM, Daniel D. Daugherty
>>> <daniel.daughe...@oracle.com <mailto:daniel.daughe...@oracle.com>> wrote:
>>>
>>> On 2/21/18 2:45 AM, serguei.spit...@oracle.com
>>> <mailto:serguei.spit...@oracle.com> wrote:
>>>> On 2/20/18 23:01, David Holmes wrote:
>>>>> On 21/02/2018 4:50 PM, serguei.spit...@oracle.com
>>>>> <mailto:serguei.spit...@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
>>>>>>> <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
>>>>>> <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
>>>>> <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:
>>>
>>>> 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>
>>
>
