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

Chris Plummer chris.plummer at oracle.com
Sat Apr 18 07:47:20 UTC 2020


Hi Mandy,

Comments inline.

On 4/17/20 4:52 PM, Mandy Chung wrote:
>
>
> On 4/17/20 3:51 PM, Chris Plummer wrote:
>> Hi Mandy,
>>
>> Thanks for updating the svc specs. Some comments below:
>>
>>
>> In the JDWP spec update, you changed "JNI signature" to "type 
>> signature" in one place, but left it as "JNI signature" everywhere 
>> else. Should they all be changed?
>>
>
> JDWP signature is changed because there is no JNI signature 
> representing a hidden class.    I leave other references to JNI 
> signature as is since those only apply for normal classes as I wanted 
> to keep this change specifically due to hidden classes.  I think it's 
> good to file a JBS issue to follow up on JDWP and JDI to determine if 
> the spec should be upgraded to reference the new TypeDescriptor API.
>
>>
>> In the JDWP spec for ClassLoaderReference.VisibleClasses:
>>
>>  "That is, this class loader has been recorded as an 
>> <i>initiating</i> loader of the returned classes." -> "That is, all 
>> classes for which this class loader has been recorded as an 
>> <i>initiating</i> loader."
>>
>> This seems like too much detail to be put here. Basically the term 
>> "initiating ClassLoader" has turned into a short essay. Is it 
>> possible that all this detail could be put elsewhere and referenced? 
>
> Any suggestion?   We attempted to place those description in JVM TI 
> Class section or ClassLoad event.   However, that's not ideal place 
> since that's needed by JDWP, JDI and Instrumentation.   I found 
> inlining this description is not ideal but it provides adequate 
> clarification.
See other thread.
>
>> Aren't there other places in other specs where a similar 
>> clarification of "initiating ClassLoader" is needed (I see now that 
>> ClassLoaderClasses in the JVMTI spec, 
>> ClassLoaderReference,visibleClasses in the JDI spec, and 
>> Instrumentation.getInitiatedClasses are all dealing with this, but 
>> not all in the exact same way).
>>
>
> I took the conservative side and make sure the clarification is in 
> place for all APIs.  I'm open to any suggestion for example having 
> JDWP and JDI to link to JVM TI spec if you think appropriate.
>
>>
>> In the JVMTI spec for GetLoadedClasses:
>>
>> This suffers in a way similar to ClassLoaderReference.VisibleClasses 
>> in the JDWP spec, although not as badly. A simple concept ends up 
>> with a complex description, and it seems that description should 
>> really be in a more centralized place.  I would also suggest a bit of 
>> cleanup of these lines:
>>
>> 6866         An array class is created directly by Java virtual 
>> machine.  An array class
>> 6867         creation can be triggered by using class loaders or by 
>> invoking methods in certain
>> 6868         Java SE Platform API such as reflection.
>>
>> "Created by [the] Java virtual machine" (add "the")
>> Change "An array class creation" to "The creation" since your are 
>> repeating "An array class" from the previous sentence.
>>
>>
>> In the JVMTI spec ClassLoaderClasses section:
>>
>> "That is, initiating_loader has been recorded as an initiating loader 
>> of the returned classes." -> "That is, all classes for which 
>> initiating_loader has been recorded as an initiating loader."
>>
>>
>> In the JVMTI spec GetClassSignature section:
>>
>> "If the class indicated by klass is ..." -> "If the class ..." (you 
>> just finished the previous sentence with "class indicated by Klass").
>>
>> "the returned name is [the] JNI type signature" (add "the"). Also, is 
>> "JNI type signature" formally defined somewhere? This relates to my 
>> JDWP spec comment above.
>>
>
> It's a link to 
> https://download.java.net/java/early_access/jdk15/docs/specs/jni/types.html#type-signatures. 
> This is how the current JVM TI spec defines.
Since it looks like this reference was present before your changes, I 
guess it's ok to leave it, but like JDWP maybe a bug should be filed to 
clean it up.

Regarding your JNI spec reference, it say "The JNI uses the Java VM’s 
representation of type signatures" and then has a table called "Java VM 
Type Signatures". No where in the JNI spec do you see "JNI Signature" or 
"JNI Type Signature". It seems we should always just use "Type Signature"?

Even the JVMTI spec is not consistent. It has a couple of references to 
JNI Type Signature as links to the JNI spec, but also says "type 
signatures" in a couple of places, including for GetLocalVariableTable() 
which refers the the JVMS: "The local variable's type signature, encoded 
as a modified UTF-8 string. The signature format is the same as that 
defined in The Java™ Virtual Machine Specification, Chapter 4.3.2. "

>
>> " where N is the binary name encoded in internal form indicated by 
>> the class file". Is "binary name encoded in internal form" explained 
>> somewhere? 
>
> JVMS 4.2.1
>
> https://docs.oracle.com/javase/specs/jvms/se14/html/jvms-4.html#jvms-4.2.1
Thanks. I see you've also added a reference in your edits below.
>
>> Also, can you add an example of a returned hidden class signature?
>>
> OK
>
>>
>> In the JVMTI spec ClassLoad section:
>>
>> "representation using [a] class loader" (add "a")
>>
>> "By invoking Lookup::defineHiddenClass, that creates ..."  -> "By 
>> invoking Lookup::defineHiddenClass to create ..."
>>
>> "certain Java SE Platform API" -> Should be "APIs"
>>
>>
>> In JDI ClassLoaderReference.definedClasses()
>>
>> "loaded at least to the point of preparation and types ..." -> 
>> "loaded at least to the point of preparation, and types ..." (Note, 
>> this not a new issue with your edits)
>>
>>
>> In Instrumentation.getAllLoadedClasses:
>>
>> The reference to `class` did not format properly.
>>
>
> Serguei caught that one too.  I fixed it in my local repo.
>
>> "by invoking Lookup::defineHiddenClass that creates" -> "by invoking 
>> Lookup::defineHiddenClass, which creates"
>>
>> "An array class is created directly by Java virtual machine. An array 
>> class creation can be triggered ..." ->"An array class is created 
>> directly by the Java virtual machine. Array class creation can be 
>> triggered ..."
>>
>>
>> In Instrumentation.getInitiatedClasses:
>>
>> "That is, loader has been recorded as an initiating loader of these 
>> classes." -> "That is, all classes for which loader has been recorded 
>> as an initiating loader."
>>
>> thanks,
>>
>> Chris
>
> Thanks for the review.   See this patch of the editorial fixes.
>
> diff --git a/make/data/jdwp/jdwp.spec b/make/data/jdwp/jdwp.spec
> --- a/make/data/jdwp/jdwp.spec
> +++ b/make/data/jdwp/jdwp.spec
> @@ -2265,8 +2265,8 @@
>          "Returns a list of all classes which this class loader can find "
>          "by name via <code>ClassLoader::loadClass</code>, "
>          "<code>Class::forName</code> and bytecode linkage. That is, "
> -        "this class loader has been recorded as an <i>initiating</i> "
> -        "loader of the returned classes. The list contains each
> +        "all classes for which this class loader has been recorded as 
> an "
> +        "<i>initiating</i> loader. The list contains each "
>          "reference type created by this loader and any types for which "
>          "loading was delegated by this class loader to another class 
> loader. "
>          "The list does not include hidden classes or interfaces 
> created by "
> diff --git a/src/hotspot/share/prims/jvmti.xml 
> b/src/hotspot/share/prims/jvmti.xml
> --- a/src/hotspot/share/prims/jvmti.xml
> +++ b/src/hotspot/share/prims/jvmti.xml
> @@ -6857,15 +6857,15 @@
>          A class or interface creation can be triggered by one of the 
> following:
>          <ul>
>          <li>By loading and deriving a class from a <code>class</code> 
> file representation
> -            using class loader (see <vmspec chapter="5.3"/>).</li>
> +            using a class loader (see <vmspec chapter="5.3"/>).</li>
>          <li>By invoking <externallink 
> id="../api/java.base/java/lang/invoke/MethodHandles.Lookup.html#defineHiddenClass(byte[],boolean,java.lang.invoke.MethodHandles.Lookup.ClassOption...)">Lookup::defineHiddenClass</externallink>
>              that creates a hidden class or interface from a 
> <code>class</code> file representation.</li>
> -        <li>By invoking methods in certain Java SE Platform API such 
> as reflection.</li>
> +        <li>By invoking methods in certain Java SE Platform APIs such 
> as reflection.</li>
>           </ul>
>          <p/>
> -        An array class is created directly by Java virtual machine.  
> An array class
> -        creation can be triggered by using class loaders or by 
> invoking methods in certain
> -        Java SE Platform API such as reflection.
> +        An array class is created directly by the Java virtual 
> machine.  The creation
> +        can be triggered by using class loaders or by invoking 
> methods in certain
> +        Java SE Platform APIs such as reflection.
>          <p/>
>          The returned list includes all classes and interfaces, including
>          <externallink 
> id="../api/java.base/java/lang/Class.html#isHidden()">
> @@ -6904,8 +6904,8 @@
>          can find by name via
>          <externallink 
> id="../api/java.base/java/lang/ClassLoader.html#loadClass(java.lang.String,boolean)">ClassLoader::loadClass</externallink>,
>          <externallink 
> id="../api/java.base/java/lang/Class.html#forName(java.lang.String,boolean,java.lang.ClassLoader)">Class::forName</externallink> 
> and bytecode linkage.
> -        That is, <code>initiating_loader</code>
> -        has been recorded as an initiating loader of the returned 
> classes.
> +        That is, all classes for which <code>initiating_loader</code>
> +        has been recorded as an initiating loader.
>          Each class in the returned array was created by this class 
> loader,
>          either by defining it directly or by delegation to another 
> class loader.
>          See <vmspec chapter="5.3"/>.
> @@ -6964,21 +6964,22 @@
>        <description>
>          Return the name and the generic signature of the class 
> indicated by <code>klass</code>.
>          <p/>
> -        If the class indicated by <code>klass</code> is a class or 
> interface, then:
> +        If the class is a class or interface, then:
>          <ul>
>          <li>If the class or interface is not <externallink 
> id="../api/java.base/java/lang/Class.html#isHidden()">hi
> dden</externallink>,
> -          then the returned name is <externallink 
> id="jni/types.html#type-signatures">
> +          then the returned name is the <externallink 
> id="jni/types.html#type-signatures">
>            JNI type signature</externallink>.
>            For example, java.util.List is "Ljava/util/List;"
>          </li>
>          <li>If the class or interface is <externallink 
> id="../api/java.base/java/lang/Class.html#isHidden()">hidden
> </externallink>,
>            then the returned name is a string of the form:
>            <code>"L" + N + "." +  S + ";"</code>
> -          where <code>N</code> is the binary name encoded in internal 
> form
> +          where <code>N</code> is the binary name encoded in internal 
> form (JVMS 4.2.1)
>            indicated by the <code>class</code> file passed to
>            <externallink 
> id="../api/java.base/java/lang/invoke/MethodHandles.Lookup.html#defineHiddenClass(byte[],boolean,java.lang.invoke.MethodHandles.Lookup.ClassOption...)">Lookup::defineHiddenClass</externallink>,
>            and <code>S</code> is an unqualified name.
> -          The returned name is not a valid type descriptor.
> +          The returned name is not a type descriptor and does not 
> conform to JVMS 4.3.2.
> +          For example, com.foo.Foo/AnySuffix is "Lcom/foo/Foo.AnySuffix;"
>          </li>
>          </ul>
>          <p/>
> @@ -12768,10 +12769,10 @@
>        A class or interface is created by one of the following:
>        <ul>
>        <li>By loading and deriving a class from a <code>class</code> 
> file representation
> -          using class loader.</li>
> +          using a class loader.</li>
>        <li>By invoking <externallink 
> id="../api/java.base/java/lang/invoke/MethodHandles.Lookup.html#defineHiddenClass(byte[],boolean,java.lang.invoke.MethodHandles.Lookup.ClassOption...)">Lookup::defineHiddenClass</externallink>,
>            that creates a hidden class or interface from a 
> <code>class</code> file representation.</li>
> -      <li>By invoking methods in certain Java SE Platform API such as 
> reflection.</li>
> +      <li>By invoking methods in certain Java SE Platform APIs such 
> as reflection.</li>
>        </ul>
>        <p/>
>        Array class creation does not generate a class load event.
> diff --git 
> a/src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java 
> b/src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java
> --- 
> a/src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java
> +++ 
> b/src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java
> @@ -392,15 +392,15 @@
>       * A class or interface creation can be triggered by one of the 
> following:
>       * <ul>
>       * <li>by loading and deriving a class from a {@code class} file 
> representation
> -     *     using class loader (see JVMS {@jvms 5.3}).
> +     *     using a class loader (see JVMS {@jvms 5.3}).
>       * <li>by invoking {@link 
> java.lang.invoke.MethodHandles.Lookup#defineHiddenClass(byte[], 
> boolean, java.lang.invoke.MethodHandles.Lookup.ClassOption...)
> -     *     Lookup::defineHiddenClass} that creates a {@link 
> Class#isHidden
> +     *     Lookup::defineHiddenClass}, which creates a {@link 
> Class#isHidden
>       *     hidden class or interface} from a {@code class} file 
> representation.
>       * <li>by invoking methods in certain reflection APIs such as
>       *     {@link Class#forName(String) Class::forName}.
>       * </ul>
>       * <p>
> -     * An array class is created directly by Java virtual machine.  
> An array
> +     * An array class is created directly by the Java virtual 
> machine.  Array
>       * class creation can be triggered by using class loaders or by 
> invoking
>       * methods in certain reflection APIs such as
>       * {@link java.lang.reflect.Array#newInstance(Class, int) 
> Array::newInstance}
> @@ -420,8 +420,8 @@
>       * Returns an array of all classes which {@code loader} can find 
> by name
>       * via {@link ClassLoader#loadClass(String, boolean) 
> ClassLoader::loadClass},
>       * {@link Class#forName(String) Class::forName} and bytecode linkage.
> -     * That is, {@code loader} has been recorded as an initiating loader
> -     * of these classes. If the supplied {@code loader} is {@code null},
> +     * That is, all classes for which {@code loader} has been recorded as
> +     * an initiating loader. If the supplied {@code loader} is {@code 
> null},
>       * classes that the bootstrap class loader can find by name are 
> returned.
>       *
>       * <p>
> diff --git 
> a/src/jdk.jdi/share/classes/com/sun/jdi/ClassLoaderReference.java 
> b/src/jdk.jdi/share/classes/com/sun/jdi/ClassLoaderReference.java
> --- a/src/jdk.jdi/share/classes/com/sun/jdi/ClassLoaderReference.java
> +++ b/src/jdk.jdi/share/classes/com/sun/jdi/ClassLoaderReference.java
> @@ -46,7 +46,7 @@
>       * No ordering of this list guaranteed.
>       * The returned list will include all reference types, including
>       * {@link Class#isHidden hidden classes or interfaces}, loaded
> -     * at least to the point of preparation and types (like array)
> +     * at least to the point of preparation, and types (like array)
>       * for which preparation is not defined.
>       *
>       * @return a {@code List} of {@link ReferenceType} objects 
> mirroring types
> @@ -59,8 +59,8 @@
>       * Returns a list of all classes which this class loader
>       * can find by name via {@link ClassLoader#loadClass(String, boolean)
>       * ClassLoader::loadClass}, {@link Class#forName(String) 
> Class::forName}
> -     * and bytecode linkage in the target VM.  That is, this class loader
> -     * has been recorded as an initiating loader of these classes.
> +     * and bytecode linkage in the target VM.  That is, all classes for
> +     * which this class loader has been recorded as an initiating loader.
>       * <p>
>       * Each class in the returned list was created by this class loader
>       * either by defining it directly or by delegation to another 
> class loader
> diff --git a/src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java 
> b/src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java
> --- a/src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java
> +++ b/src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java
> @@ -138,9 +138,9 @@
>       * A class or interface creation can be triggered by one of the 
> following:
>       * <ul>
>       * <li>by loading and deriving a class from a {@code class} file 
> representation
> -     *     using class loader (see JVMS {@jvms 5.3}).
> +     *     using a class loader (see JVMS {@jvms 5.3}).
>       * <li>by invoking {@link 
> java.lang.invoke.MethodHandles.Lookup#defineHiddenClass(byte[], 
> boolean, java.lang.invoke.MethodHandles.Lookup.ClassOption...)
> -     *     Lookup::defineHiddenClass} that creates a {@link 
> Class#isHidden
> +     *     Lookup::defineHiddenClass}, which creates a {@link 
> Class#isHidden
>       *     hidden class or interface} from a {@code class} file 
> representation.
>       * <li>by invoking methods in certain reflection APIs such as
>       *     {@link Class#forName(String) Class::forName}.
>
Above changes all look good.

thanks,

Chris
> Mandy
>
>



More information about the valhalla-dev mailing list