Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Mandy Chung]

On 4/20/20 1:06 PM, Chris Plummer wrote:
> On 4/18/20 3:28 PM, Mandy Chung wrote:
>> Hi Chris, Serguei,
>>
>> On 4/18/20 12:18 AM, Chris Plummer wrote:
>>>
>>> Yes. I'd like to see all this as part of the Class/Classloading spec 
>>> in some sort of section that gives an overview of all these topics, 
>>> but mostly clarifies what an initiating loader is, and the (non) 
>>> relationship to hidden classes. 
>>
>> We should refer initiating loader and class loading from JVMS 5.3.   
>> JVM TI needs the clarification w.r.t. GetClassLoaderClasses that does 
>> not include hidden classes because the initiating class loader cannot 
>> find it.
>>
>> GetLoadedClasses is about class creation.   While it seems tempted to 
>> put the descriptions in some sort of overview section, I found the 
>> clarification is specific for each function and hence inlining them 
>> in each function helps the readers directly see the difference.
>>
>> I made a few changes that should ease your concern of duplication:
>> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes.01/ 
>>
>>
>> - The class creation description added in GetLoadedClasses.  Not in 
>> JDWP, JDI and Instrumentation.
>> - The description in GetClassLoaderClasses, 
>> Instrumentation::getInitiatedClasses, 
>> ClassLoaderReference::visibleClasses is revised to take out the 
>> details.   Add links from JDWP and JDI to GetClassLoaderClasses.
>> - The details about hidden classes can be found from 
>> `Class::isHidden` and      `Lookup::defineHiddenClass`.
>>
>> Mandy
> Hi Mandy,
>
> This looks much better. Thank you for the consolidating and the cross 
> references. I think that's a big improvement.
>
> One minor thing I noticed is a few "JNI-style signature" references. 
> In LocalVariable.java you renamed "JNI signature" to "JNI-style 
> signature", but in JDI Mirror.java you renamed "JNI-style signature" 
> to "type signature". 

You meant Type::signature.   Type::signature may return the type 
signature of a hidden class whereas LocalVariable::signature and 
TypeComponent::signature return the field/method descriptor where no 
hidden class will appear in a resolved field/variable/method.

> And then in JDI TypeComponent.java you left the "JNI-style signature" 
> reference unchanged. Why the inconsistency?
>
I made LocalVariable::signature consistent with TypeComponent::signature 
whereas Type::signature returns the string of the form described in 
Class::descriptorString.

I tried to keep existing reference to use "JNI-style signature". Since I 
touched on these files, I can change them to "type signature" which 
applies to these cases but I will leave the link to the JNI spec as it is.

> In VirtualMachine.java:
>
>  145      * @see Class or interface creation specified in
>  146      * <a 
> href="{@docRoot}/../specs/jvmti/jvmti.html#GetLoadedClasses">JVM TI 
> GetLoadedClasses</a>
>
> How about:
>
>  145      * @a description of how Class and interface creation can be 
> triggered is specified in
>  146      * <a 
> href="{@docRoot}/../specs/jvmti/jvmti.html#GetLoadedClasses">JVM TI 
> GetLoadedClasses</a>
>

      * @see <a 
href="{@docRoot}/../specs/jvmti/jvmti.html#GetLoadedClasses">
      * JVM TI GetLoadedClasses</a> regarding how class and interface 
creation can be triggered

> I read the following incorrectly a few times before I finally 
> understood the meaning:
>
> 44     /**
>   45      * Returns the {@linkplain com.sun.jdi.Type#name() name of 
> the class}
>   46      * that has been unloaded. The returned string may not be a
>   47      * <a 
> href="${docRoot}/java.base/java/lang/ClassLoader.html#binary-name">binary 
> name</a>.
>   48      *
>   49      * @see Class#getName()
>   50      */
>   51     public String className();
>   52
>   53     /**
>   54      * Returns the {@linkplain com.sun.jdi.Type#signature() type 
> signature of the class}
>   55      * that has been unloaded.  The returned string may not be a 
> type descriptor
>   56      * specified in JVMS {@jvms 4.3.2}.
>   57      *
>   58      * @see java.lang.invoke.TypeDescriptor#descriptorString()
>   59      */
>   60     public String classSignature();
>
> "may not be" through me off. I read it as "is not allowed not be". 
> Your intent was "might not be", which might be a better choice of words.
>

I make it clearer as:

      * Returns the {@linkplain com.sun.jdi.Type#signature() type 
signature of the class}
      * that has been unloaded.  The result is of the same
      * form as the string returned by {@link Class#descriptorString()}.
      * If this class can be described nominally, the returned string is a
      * type descriptor conforming to JVMS {@jvms 4.3.2}; otherwise, the 
returned string
      * is not a type descriptor.

Similar clarification is also made in Type::signature:

      * Returns the type signature for this type.  The result is of the same
      * form as the string returned by {@link Class#descriptorString()}.
      * The returned string is a type descriptor conforming to JVMS 
{@jvms 4.3.2}
      * if this type can be described nominally.  Otherwise, the 
returned string
      * is not a type descriptor.

Mandy
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20200420/c49cf200/attachment.htm>