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.ch...@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
