RFR (XS): 8046018: JVMTI Spec: can_redefine_any_class capability spec is inconsistent
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed May 22 18:50:43 UTC 2019
On 5/22/19 1:54 PM, serguei.spitsyn at 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... :-)
Dan
>
> Thanks, Dan!
> Serguei
>
>>
>> Dan
>>
>>
>> On 5/22/19 6:55 AM, serguei.spitsyn at 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 at 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 at 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 at 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 at 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 at 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
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>>
>
More information about the serviceability-dev
mailing list