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 17:54:43 UTC 2019
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.
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