RFR (XS): 8046018: JVMTI Spec: can_redefine_any_class capability spec is inconsistent

[Daniel D. Daugherty]

On 5/21/19 6:58 PM, 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.

Thanks for closing the loop on this query. When I read the CSR,
I missed that bit of research.

Dan


>
> 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
>>>>
>>>
>>