RFR (XS): 8046018: JVMTI Spec: can_redefine_any_class capability spec is inconsistent
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed May 22 14:00:38 UTC 2019
> 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...
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