[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR (XS): 8046018: JVMTI Spec: can_redefine_any_class capability spec is inconsistent
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2019-05-23 8:18:13
Message-ID: 068061f2-9a94-371f-9b0d-134939ff518d () oracle ! com
[Download RAW message or body]
On 5/22/19 11:50, Daniel D. Daugherty wrote:
> On 5/22/19 1:54 PM, serguei.spitsyn@oracle.com wrote:
>>
>>
>>
>> On 5/22/19 07:00, Daniel D. Daugherty wrote:
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8046018-jvmti-cap-spec.3/
>>>
>>>
>>>
>>>
>>>
>>> Thumbs up on this version. I did add a historical note to the CSR...
>>
>> Good comment.
>> I've added a note with a small correction/addition.
>
> Sorry, I had to correct your correction... :-)
Okay, thanks!
I still hope, Alan will have a chance to look at this CSR and fix.
At least, Alan wanted to review it a week ago.
Thanks,
Serguei
>
> Dan
>
>
>>
>> Thanks, Dan!
>> Serguei
>>
>>>
>>> Dan
>>>
>>>
>>> On 5/22/19 6:55 AM, serguei.spitsyn@oracle.com wrote:
>>>> Hi David,
>>>>
>>>>
>>>> On 5/21/19 22:36, David Holmes wrote:
>>>>> Hi Serguei,
>>>>>
>>>>> Apologies for the confusion around this - and double apologies
>>>>> that you wrote all the below while I was busy going through the
>>>>> history and updating the CSR request with all the relevant details.
>>>>
>>>>
>>>> No apologies are accepted. :)
>>>> The topic is confusing for me too.
>>>> Thank you for checking all this as it is the best way to make it
>>>> right!
>>>> I really appreciate it.
>>>>
>>>>
>>>>> As I've finally written in the CSR request your change here is
>>>>> correct as it fixes an error/confusion introduced by JDK-6328530
>>>>> (which itself was confused because of an omission introduced by
>>>>> JDK-6306942; and that omission was rectified by JDK-8145964.)
>>>>>
>>>>> They key end result here is that can_redefine_any_class and
>>>>> can_retransform_any class should be described in exactly the same
>>>>> way other than the redefine/retransform part. This means:
>>>>>
>>>>> - their usage with IsModifiableClass
>>>>> - their usage with RedefineClasses/RetransformClasses
>>>>> - their definitions in the capabilities structure
>>>>>
>>>>> You've addressed the first part here, and the second was already
>>>>> fine, but your change to the third part, while not wrong per-se,
>>>>> uses a different form. You now have:
>>>>>
>>>>> can_redefine_any_class: Can redefine any modifiable class. See
>>>>> IsModifiableClass. (can_redefine_classes must also be set)
>>>>>
>>>>> whereas we already have:
>>>>>
>>>>> can_retransform_any_class: RetransformClasses can be called on any
>>>>> modifiable class. See IsModifiableClass. (can_retransform_classes
>>>>> must also be set)
>>>>>
>>>>> So I recommend instead:
>>>>>
>>>>> can_redefine_any_class: RedefineClasses can be called on any
>>>>> modifiable class. See IsModifiableClass. (can_redefine_classes
>>>>> must also be set)
>>>>
>>>> Agreed, thanks!
>>>> When looked at the spec again I came to the same conclusion.
>>>> I've updated the CSR with this suggestion.
>>>>
>>>> Also, please, find the latest versions below:
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8046018-jvmti-cap-spec.3/
>>>>
>>>>
>>>> JVMTI spec:
>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8046018-jvmti-cap-spec.3/jvmti.html
>>>>
>>>>
>>>> JVMTI spec diff:
>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8046018-jvmti-cap-spec.3/jvmti-specdiff/
>>>>
>>>>
>>>> Enhancement:
>>>> https://bugs.openjdk.java.net/browse/JDK-8046018
>>>>
>>>> Related CSR:
>>>> https://bugs.openjdk.java.net/browse/JDK-8223915
>>>>
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>> On 22/05/2019 1:59 pm, serguei.spitsyn@oracle.com wrote:
>>>>>> Hi David,
>>>>>>
>>>>>>
>>>>>> On 5/21/19 17:25, David Holmes wrote:
>>>>>>> Hi Serguei,
>>>>>>>
>>>>>>> Sorry but I think this "fix" is inappropriate. The capability
>>>>>>> may be badly named but the intent was to have it apply to both
>>>>>>> redefine and retransform.
>>>>>>
>>>>>> Not sure, I fully understand you here.
>>>>>> This intent seemed to be wrong or, at least, it is really
>>>>>> confusing to everybody.
>>>>>>
>>>>>> To understand it better, let's do some analysis first and start
>>>>>> from what we know.
>>>>>>
>>>>>> 1) The JVMTI RedefineClasses is implemented with using a
>>>>>> VM_RedefineClasses operation.
>>>>>> So, the VM_RedefineClasses is triggered on behave of a
>>>>>> RedefineClasses call from agent code.
>>>>>> The capability "can_redefine_classes" has to be possessed to
>>>>>> call RedefineClasses.
>>>>>>
>>>>>> 2) The JVMTI RetransformClasses is also implemented with using a
>>>>>> VM_RedefineClasses operation.
>>>>>> So, the VM_RedefineClasses is triggered on behave of a
>>>>>> RetransformClasses call from agent code.
>>>>>> The capability "can_retransform_classes" has to be possessed
>>>>>> to call RetransformClasses.
>>>>>> There is no check (and no intent to check) for the capability
>>>>>> "can_redefine_classes".
>>>>>>
>>>>>> 3) The VM_RedefineClasses starts a chain of CFLH's (per each
>>>>>> JvmtiEnv) in both cases above.
>>>>>> So that the retransformations (with CFLH's) both
>>>>>> RedefineClasses and RetransformClasses calls.
>>>>>> If the capability "can_retransform_classes" was not possessed
>>>>>> the retransformations
>>>>>> caused by RedefineClasses will be processed without problems.
>>>>>> It is because both redefine and retransform capabilities are
>>>>>> not required for CFLH's.
>>>>>>
>>>>>> 4) The capability "can_retransform_any_class" was added to allow
>>>>>> RetransformClasses calls
>>>>>> for any classes including non-modifiable (if the
>>>>>> implementation has such a notion).
>>>>>> It controls RetransformClasses calls only.
>>>>>>
>>>>>> 5) As I understand, the capability "can_redefine_any_class" was
>>>>>> added to allow
>>>>>> RedefineClasses calls for any classes including non-modifiable
>>>>>> (if the implementation has such a notion).
>>>>>> It should control RedefineClasses only.
>>>>>> I do not see why it has to apply to RetransformClasses as well.
>>>>>>
>>>>>>
>>>>>> Could you, explain, please, what is the advantages to apply it to
>>>>>> both retransform and redefine?
>>>>>>
>>>>>> I see only problems/disadvantages with it:
>>>>>>
>>>>>> D1: The terms "retransform" and "redefine" in this context are
>>>>>> confusing.
>>>>>> The RetransformClasses is using a VM_RedefineClasses operation.
>>>>>> Can we treat this VM_RedefineClasses operation as "redefine"?
>>>>>> My guess is that this was initial motivation to list both
>>>>>> "retransform" and "redefine"
>>>>>> for "can_redefine_any_class" capability.
>>>>>>
>>>>>> D2: The "can_redefine_classes" and "can_retransform_classes" are
>>>>>> equal,
>>>>>> but "can_redefine_any_class" and "can_retransform_any_class"
>>>>>> are not.
>>>>>> There is no symmetry here which adds complexity and
>>>>>> generates confusion.
>>>>>>
>>>>>> D3: The RedefineClasses is controlled with can_redefine_classes
>>>>>> and can_redefine_any_class.
>>>>>> But the RetransformClasses is controlled with:
>>>>>> can_retransform_classes, can_retransform_any_class and
>>>>>> can_redefine_any_class.
>>>>>> There is no symmetry here which adds complexity and
>>>>>> generates confusion.
>>>>>>
>>>>>>
>>>>>>> Further this change should not have been pushed yet as the CSR
>>>>>>> has not been approved.
>>>>>>
>>>>>> Sorry, I confused this bug with another one.
>>>>>> This was not pushed yet.
>>>>>> Thank you for pointing it out!
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>> On 22/05/2019 9:53 am, serguei.spitsyn@oracle.com wrote:
>>>>>>>> Hi Dan and David,
>>>>>>>>
>>>>>>>> On 5/21/19 15:58, David Holmes wrote:
>>>>>>>>> Hi Dan,
>>>>>>>>>
>>>>>>>>> On 22/05/2019 2:34 am, Daniel D. Daugherty wrote:
>>>>>>>>>> On 5/21/19 2:19 AM, serguei.spitsyn@oracle.com wrote:
>>>>>>>>>>> Hi guys,
>>>>>>>>>>>
>>>>>>>>>>> I've found one more fragment in the IsModifiableClass spec
>>>>>>>>>>> which has to be fixed.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Updated webrev v2:
>>>>>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8046018-jvmti-cap-spec.2/
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> src/hotspot/share/prims/jvmti.xml
>>>>>>>>>> No comments.
>>>>>>>>>>
>>>>>>>>>> Looks like there was a specific update to the spec to allow
>>>>>>>>>> can_redefine_any_class
>>>>>>>>>> to include retransform (in addition to redefine):
>>>>>>>>>>
>>>>>>>>>> 1.1.82
>>>>>>>>>> 13 February 2006 Doc fixes: update can_redefine_any_class
>>>>>>>>>> to include retransform. Clarify that exception events cover
>>>>>>>>>> all Throwables. In GetStackTrace, no test is done for
>>>>>>>>>> start_depth too big if start_depth is zero, Clarify fields
>>>>>>>>>> reported in Primitive Field Callback -- static vs instance.
>>>>>>>>>> Repair confusing names of heap types, including callback
>>>>>>>>>> names. Require consistent usage of stack depth in the face of
>>>>>>>>>> thread launch methods. Note incompatibility of JVM TI memory
>>>>>>>>>> management with other systems.
>>>>>>>>>>
>>>>>>>>>> I can't tell if you've chased down that change and why you no
>>>>>>>>>> longer
>>>>>>>>>> think that change is necessary.
>>>>>>>>>>
>>>>>>>>>> I'm okay with the change, but I think you have more research
>>>>>>>>>> to do here.
>>>>>>>>>
>>>>>>>>> I already chased that down and mentioned it in the CSR. It was
>>>>>>>>> done under:
>>>>>>>>>
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-6328530
>>>>>>>>>
>>>>>>>>> Unfortunately I misread the original bug description. In
>>>>>>>>> relation to can_redefine_any_class it states:
>>>>>>>>>
>>>>>>>>> A more precise definition would be:
>>>>>>>>> Can retransform or redefine any non-primitive non-array
>>>>>>>>> class.
>>>>>>>>>
>>>>>>>>> It appears to me that they did consider can_redefine_any_class
>>>>>>>>> to be a "super" capability that could be added on top of
>>>>>>>>> can_redefine_classes to extend it to "any" class; or on top of
>>>>>>>>> can_retransform_classes to extend it to "any" class. If so the
>>>>>>>>> name choice was unfortunate.
>>>>>>>>>
>>>>>>>>> Further it seems the implementation never checked this anyway.
>>>>>>>>
>>>>>>>> Right.
>>>>>>>> The approach taken for JDK-6328530 is non-symmetrical and
>>>>>>>> confusing.
>>>>>>>> But, I think, I understand why this decision was made. :)
>>>>>>>>
>>>>>>>> If we ever decide to make some (non-primitive and non-array)
>>>>>>>> classes to be non-modifiable then
>>>>>>>> I do not see problems to implement it in a way that
>>>>>>>> can_redefine_any_class will be checked on
>>>>>>>> RedefineClasses only and can_retransform_any_class will be
>>>>>>>> checked on RetransformClasses only.
>>>>>>>> We will get a symmetry (and so, a simplification as well) in
>>>>>>>> two dimensions:
>>>>>>>> - between redefine and retransform capabilities
>>>>>>>> - between can_redefine_classes and can_redefine_any_class
>>>>>>>> capabilities
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Serguei
>>>>>>>>
>>>>>>>>>
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>>> Dan
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Specdiff:
>>>>>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8046018-jvmti-cap-spec.2/jvmti-specdiff/
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Enhancement:
>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8046018
>>>>>>>>>>>
>>>>>>>>>>> Related CSR:
>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8223915
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Serguei
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 5/20/19 21:43, serguei.spitsyn@oracle.com wrote:
>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>
>>>>>>>>>>>> Thank you for looking at this!
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 5/20/19 20:53, David Holmes wrote:
>>>>>>>>>>>>> Hi Serguei,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 21/05/2019 4:07 am, serguei.spitsyn@oracle.com wrote:
>>>>>>>>>>>>>> Please, review a fix for enhancement:
>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8046018
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Related CSR:
>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8223915
>>>>>>>>>>>>>
>>>>>>>>>>>>> I have some comments on the CSR and about this change
>>>>>>>>>>>>> overall as to me it is not a simple clarification at all,
>>>>>>>>>>>>> but potentially a significant change in the meaning of the
>>>>>>>>>>>>> capability.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> I've answered your question in the CSR with my comment.
>>>>>>>>>>>>
>>>>>>>>>>>>>> Webrev:
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8046018-jvmti-cap-spec.1/
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> You introduced a typo: modifialble
>>>>>>>>>>>>>
>>>>>>>>>>>>> Assuming this proceeds a similar change is needed earlier:
>>>>>>>>>>>>>
>>>>>>>>>>>>> 7444 <capability id="can_redefine_any_class">
>>>>>>>>>>>>> 7445 If possessed then all classes (except
>>>>>>>>>>>>> primitive, array, and some implementation defined
>>>>>>>>>>>>> 7446 classes) are modifiable (redefine or
>>>>>>>>>>>>> retransform).
>>>>>>>>>>>>
>>>>>>>>>>>> Good catch, thanks!
>>>>>>>>>>>> I've updated the webrev in place.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Serguei
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> David
>>>>>>>>>>>>> -----
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Summary:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The fix is to make the JVMTI can_redefine_any_class
>>>>>>>>>>>>>> capability spec more inconsistent.
>>>>>>>>>>>>>> It is just about a couple of lines.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> Serguei
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>
>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic