RFR (XS): 8046018: JVMTI Spec: can_redefine_any_class capability spec is inconsistent
David Holmes
david.holmes at oracle.com
Wed May 22 05:36:42 UTC 2019
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.
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)
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