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

Fri Mar 27 18:59:10 UTC 2020

Hi Mandy,

Very thorough, bravo!

Minor suggestions below.

Paul.

MethodHandleNatives.java
—

 142 
 143         /**
 144          * Flags for Lookup.ClassOptions
 145          */
 146         static final int
 147             NESTMATE_CLASS            = 0x00000001,
 148             HIDDEN_CLASS              = 0x00000002,
 149             STRONG_LOADER_LINK        = 0x00000004,
 150             ACCESS_VM_ANNOTATIONS     = 0x00000008;
 151     }

Suggest you add a comment to keep the values in sync with the VM component.

MethodHandles.java
—

1786          * (Given the {@code Lookup} object returned this method, its lookup class
1787          * is a {@code Class} object for which {@link Class#getName()} returns a string
1788          * that is not a binary name.)

“
(The {@code Lookup} object returned from this method has a lookup class that is 
a {@code Class} object whose {@link Class#getName()} returns a string
that is not a binary name.)
“

1902             Set<ClassOption> opts = options.length > 0 ? Set.of(options) : Set.of();

You can just do:

  Set<ClassOption> opts = Set.of(options)

And/or inline it into the subsequent method call.  The implementation of Set.of checks the array length.

2001         ClassDefiner makeHiddenClassDefiner(byte[] bytes,

I think you can telescope the methods for non-name and name accepting since IIUC the name is derived from the byte[].  Thereby you can remove some code duplication. i.e. pull ClassDefiner.className out from ClassDefiner and place the logic in the factory methods.  Alternative push the factory methods into ClassDefiner to keep all the logic together.

3797         public enum ClassOption {

Shuffle up to be closer to the defineHiddenClass

3798             /**
3799              * This class option specifies the hidden class be added to
3800              * {@linkplain Class#getNestHost nest} of a lookup class as
3801              * a nestmate.

Suggest:

"This class option specifies the hidden class “ -> “Specifies that a hidden class 

3812              * This class option specifies the hidden class to have a <em>strong</em>

“Specifies that a hidden class have a …"

3813              * relationship with the class loader marked as its defining loader,
3814              * as a normal class or interface has with its own defining loader.
3815              * This means that the hidden class may be unloaded if and only if
3816              * its defining loader is not reachable and thus may be reclaimed
3817              * by a garbage collector (JLS 12.7).

StringConcatFactory.java
—

 861             // use of @ForceInline no longer has any effect

?

 862             mv.visitAnnotation("Ljdk/internal/vm/annotation/ForceInline;", true);
 863             mv.visitCode();

> On Mar 26, 2020, at 4:57 PM, Mandy Chung <mandy.chung at oracle.com> wrote:
> 
> Please review the implementation of JEP 371: Hidden Classes. The main changes are in core-libs and hotspot runtime area.  Small changes are made in javac, VM compiler (intrinsification of Class::isHiddenClass), JFR, JDI, and jcmd.  CSR [1]has been reviewed and is in the finalized state (see specdiff and javadoc below for reference).
> 
> Webrev:
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03
> 
> Hidden class is created via `Lookup::defineHiddenClass`. From JVM's point
> of view, a hidden class is a normal class except the following:
> 
> - A hidden class has no initiating class loader and is not registered in any dictionary.
> - A hidden class has a name containing an illegal character `Class::getName` returns `p.Foo/0x1234` whereas `GetClassSignature` returns "Lp/Foo.0x1234;".
> - A hidden class is not modifiable, i.e. cannot be redefined or retransformed. JVM TI IsModifableClass returns false on a hidden.
> - Final fields in a hidden class is "final".  The value of final fields cannot be overriden via reflection.  setAccessible(true) can still be called on reflected objects representing final fields in a hidden class and its access check will be suppressed but only have read-access (i.e. can do Field::getXXX but not setXXX).
> 
> Brief summary of this patch:
> 
> 1. A new Lookup::defineHiddenClass method is the API to create a hidden class.
> 2. A new Lookup.ClassOption enum class defines NESTMATE and STRONG option that
>    can be specified when creating a hidden class.
> 3. A new Class::isHiddenClass method tests if a class is a hidden class.
> 4. Field::setXXX method will throw IAE on a final field of a hidden class
>    regardless of the value of the accessible flag.
> 5. JVM_LookupDefineClass is the new JVM entry point for Lookup::defineClass
>    and defineHiddenClass to create a class from the given bytes.
> 6. ClassLoaderData implementation is not changed.  There is one primary CLD
>    that holds the classes strongly referenced by its defining loader.  There
>    can be zero or more additional CLDs - one per weak class.
> 7. Nest host determination is updated per revised JVMS 5.4.4. Access control
>    check no longer throws LinkageError but instead it will throw IAE with
>    a clear message if a class fails to resolve/validate the nest host declared
>    in NestHost/NestMembers attribute.
> 8. JFR, jcmd, JDI are updated to support hidden classes.
> 9. update javac LambdaToMethod as lambda proxy starts using nestmates
>    and generate a bridge method to desuger a method reference to a protected
>    method in its supertype in a different package
> 
> This patch also updates StringConcatFactory, LambdaMetaFactory, and LambdaForms
> to use hidden classes.  The webrev includes changes in nashorn to hidden class
> and I will update the webrev if JEP 372 removes it any time soon.
> 
> We uncovered a bug in Lookup::defineClass spec throws LinkageError and intends
> to have the newly created class linked.  However, the implementation in 14
> does not link the class.  A separate CSR [2] proposes to update the
> implementation to match the spec.  This patch fixes the implementation.
> 
> The spec update on JVM TI, JDI and Instrumentation will be done as
> a separate RFE [3].  This patch includes new tests for JVM TI and
> java.instrument that validates how the existing APIs work for hidden classes.
> 
> javadoc/specdiff
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/api/
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff/
> 
> JVMS 5.4.4 change:
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/Draft-JVMS-HiddenClasses.pdf
> 
> CSR:
> https://bugs.openjdk.java.net/browse/JDK-8238359
> 
> Thanks
> Mandy
> [1] https://bugs.openjdk.java.net/browse/JDK-8238359
> [2] https://bugs.openjdk.java.net/browse/JDK-8240338
> [3] https://bugs.openjdk.java.net/browse/JDK-8230502