RFR (XS): 8046018: JVMTI Spec: can_redefine_any_class capability spec is inconsistent
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed May 22 10:55:53 UTC 2019
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