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

[David Holmes]

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.

Further this change should not have been pushed yet as the CSR has not 
been approved.

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