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

[David Holmes]

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.

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