Hi Mandy, The patch for nestmates [1] could be used as a reference. There a new method was added to class `com.sun.tools.javac.jvm.Target`, named: `hasNestmateAccess` which checks if a target is ready for nestmates or not. I think that you can follow a similar approach here. Thanks, Vicente [1] http://hg.openjdk.java.net/jdk/jdk/rev/2f2af62dfac7 On 3/27/20 12:29 PM, Mandy Chung wrote:

Hi Jan,

Good point.  The javac change only applies to JDK 15 and later and the lambda proxy class is not a nestmate when running on JDK 14 or earlier.

I probably need the help from langtools team to fix this.  I'll give it a try.

Mandy

On 3/27/20 4:31 AM, Jan Lahoda wrote:

...

Hi Mandy,

Regarding the javac changes - should those be switched on/off depending the Target? Or, if one compiles with e.g. --release 14, will the newly generated output still work on JDK 14?

Jan

On 27. 03. 20 0:57, Mandy Chung 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-JVM...

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

Hi Mandy, On 3/27/20 6:29 PM, Mandy Chung wrote:

Hi Vicente,

hasNestmateAccess is about VM supports static nestmates on JDK release

...

= 11.

I was not suggesting the use of `hasNestmateAccess` but to follow the same approach which is adding a new method at class `Target` to check if the new goodies were in the given target

However this is about javac --release 14 and the compiled classes may run on JDK 14 that lambda and string concat spin classes that are not nestmates. I have a patch with Jan's help:

http://cr.openjdk.java.net/~mchung/valhalla/webrevs/8171335/webrev-javac-tar...

which is what the patch above is doing

(you can apply the above patch on valhalla repo "nestmates" branch)

About testing, I wanted to run BridgeMethodsForLambdaTest and TestLambdaBytecode test with --release 14 but it turns out not straight-forward.  Any help would be appreciated.

thanks Mandy

Vicente

On 3/27/20 2:15 PM, Vicente Romero wrote:

...

Hi Mandy,

The patch for nestmates [1] could be used as a reference. There a new method was added to class `com.sun.tools.javac.jvm.Target`, named: `hasNestmateAccess` which checks if a target is ready for nestmates or not. I think that you can follow a similar approach here.

Thanks, Vicente

[1] http://hg.openjdk.java.net/jdk/jdk/rev/2f2af62dfac7

On 3/27/20 12:29 PM, Mandy Chung wrote:

...

Hi Jan,

Good point.  The javac change only applies to JDK 15 and later and the lambda proxy class is not a nestmate when running on JDK 14 or earlier.

I probably need the help from langtools team to fix this. I'll give it a try.

Mandy

On 3/27/20 4:31 AM, Jan Lahoda wrote:

...

Hi Mandy,

Regarding the javac changes - should those be switched on/off depending the Target? Or, if one compiles with e.g. --release 14, will the newly generated output still work on JDK 14?

Jan

On 27. 03. 20 0:57, Mandy Chung 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-JVM...

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

----- Mail original -----

De: "David Holmes" <david.holmes@oracle.com> À: "mandy chung" <mandy.chung@oracle.com>, "Vicente Romero" <vicente.romero@oracle.com>, "jan lahoda" <jan.lahoda@oracle.com> Cc: "serviceability-dev" <serviceability-dev@openjdk.java.net>, "hotspot-dev" <hotspot-dev@openjdk.java.net>, "core-libs-dev" <core-libs-dev@openjdk.java.net>, "valhalla-dev" <valhalla-dev@openjdk.java.net> Envoyé: Samedi 28 Mars 2020 00:01:58 Objet: Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

Hi Mandy,

Hi David,

On 28/03/2020 8:29 am, Mandy Chung wrote:

...

Hi Vicente,

hasNestmateAccess is about VM supports static nestmates on JDK release

...

= 11.

However this is about javac --release 14 and the compiled classes may run on JDK 14 that lambda and string concat spin classes that are not nestmates. I have a patch with Jan's help:

http://cr.openjdk.java.net/~mchung/valhalla/webrevs/8171335/webrev-javac-tar...

+ /** + * The VM does not support access across nested classes (8010319). + * Were that ever to change, this should be removed. + */ + boolean isPrivateInOtherClass() {

I'm not at all sure what this means - access across different nests? (I'm not even sure what that means.)

Access inside the same nest. As you know, until now, a lambda proxy is a VM anonymous class that can only see the private fields of the class declaring the lambda (the host class) and not the private fields of a class of the nest (the enclosing classes in term of Java the language). Rémi

Thanks, David -----

...

(you can apply the above patch on valhalla repo "nestmates" branch)

About testing, I wanted to run BridgeMethodsForLambdaTest and TestLambdaBytecode test with --release 14 but it turns out not straight-forward.  Any help would be appreciated.

thanks Mandy

On 3/27/20 2:15 PM, Vicente Romero wrote:

...

Hi Mandy,

The patch for nestmates [1] could be used as a reference. There a new method was added to class `com.sun.tools.javac.jvm.Target`, named: `hasNestmateAccess` which checks if a target is ready for nestmates or not. I think that you can follow a similar approach here.

Thanks, Vicente

[1] http://hg.openjdk.java.net/jdk/jdk/rev/2f2af62dfac7

On 3/27/20 12:29 PM, Mandy Chung wrote:

...

Hi Jan,

Good point.  The javac change only applies to JDK 15 and later and the lambda proxy class is not a nestmate when running on JDK 14 or earlier.

I probably need the help from langtools team to fix this.  I'll give it a try.

Mandy

On 3/27/20 4:31 AM, Jan Lahoda wrote:

...

Hi Mandy,

Regarding the javac changes - should those be switched on/off depending the Target? Or, if one compiles with e.g. --release 14, will the newly generated output still work on JDK 14?

Jan

On 27. 03. 20 0:57, Mandy Chung 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-JVM...

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

Hi Mandy, On 28/03/2020 9:46 am, Mandy Chung wrote:

On 3/27/20 4:01 PM, David Holmes wrote:

...

Hi Mandy,

On 28/03/2020 8:29 am, Mandy Chung wrote:

...

Hi Vicente,

hasNestmateAccess is about VM supports static nestmates on JDK release  >= 11.

However this is about javac --release 14 and the compiled classes may run on JDK 14 that lambda and string concat spin classes that are not nestmates. I have a patch with Jan's help:

http://cr.openjdk.java.net/~mchung/valhalla/webrevs/8171335/webrev-javac-tar...

+             /** +              * The VM does not support access across nested classes (8010319). +              * Were that ever to change, this should be removed. +              */ +             boolean isPrivateInOtherClass() {

I'm not at all sure what this means - access across different nests? (I'm not even sure what that means.)

This just reverts  the old code that I removed.

Ah I see. This is ancient pre-nestmate code. Can we at least fix the comment as it really doesn't make any sense

What this method is trying to determine if it accesses a private in another class in the same nest (nested classes) that needs a synthetic bridge method to access.

That would be a good comment to add. Something like: If compiling for a release where the VM does not support access between nested classes, this method indicates if a synthetic bridge method is needed for access. Thanks, David

Mandy

On 3/27/20 11:59 AM, Paul Sandoz wrote:

Hi Mandy,

Very thorough, bravo!

Thanks.

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.

Already in the class spec of this Constants class.  The values of all constants defined in this Constants class are verified in sync with VM (see verifyConstants).

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.

Great to know.  Thanks.

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.

Ok.  I will move the className out.

3797 public enum ClassOption {

Shuffle up to be closer to the defineHiddenClass

Moved before 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 …"

Specifies that a hidden class has 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

?

Right, I should have explained this [1]. This @ForceInline is used by BytecodeStringBuilderStrategy that generates code to have the same StringBuilder chain javac would emit. It uses `@ForceInline` annotation which may probably be for performance.  It's believed people rarely uses this non-default strategy.  This patch changes StringConcatFactory to the standard defineHiddenClass method and hence `@ForceInline` has no effect in the generated class for this non-default strategy.  If it turns out to be an issue, then we will determine if it should enable the access to VM annotations (I doubt this is supported strategy). [1] https://bugs.openjdk.java.net/browse/JDK-8241548

+1 Paul.

On Mar 27, 2020, at 3:22 PM, Mandy Chung <mandy.chung@oracle.com> wrote:

Hi Paul,

This is the delta incorporating your comment: http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03... <http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03-delta-psandoz/>

This patch also took Alex's comment to make it clear that the hidden class is the lookup class of the returned Lookup object and drops the sentence you commented on:

On 3/27/20 1:18 PM, Mandy Chung wrote:

...

...

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.) “

Mandy

Hi Mandy and Chris, On 3/29/20 19:17, Mandy Chung wrote:

On 3/27/20 8:51 PM, Chris Plummer wrote:

...

Hi Mandy,

A couple of very minor nits in the jvmtiRedefineClasses.cpp comments:

 153     // classes for primitives, arrays, hidden and vm unsafe anonymous classes  154     // cannot be redefined.  Check here so following code can assume these classes  155     // are InstanceKlass.  156     if (!is_modifiable_class(mirror)) {  157       _res = JVMTI_ERROR_UNMODIFIABLE_CLASS;  158       return false;  159     }

I think this code and comment predate anonymous classes. Probably before anonymous classes the check was not for !is_modifiable_class() but instead was just a check for primitive or array class types since they are not an InstanceKlass, and would cause issues when cast to one in the code that lies below this section. When anonymous classes were added, the code got changed to use !is_modifiable_class() and the comment was not correctly updated (anonymous classes are an InstanceKlass). Then with this webrev the mention of hidden classes was added, also incorrectly implying they are not an InstanceKlass. I think you should just leave off the last sentence of the comment.

I agree with you that this comment needs update.   Perhaps it should say "primitive, array types and hidden classes are non-modifiable. A modifiable class must be an InstanceKlass."

I leave it to Serguei who may have other opinion.

We already had a chat with Chris about this. This suggestion looks right.

...

There's some ambiguity in the application of adjectives in the following:

 297   // Cannot redefine or retransform a hidden or an unsafe anonymous class.

I'd suggest:

 297   // Cannot redefine or retransform a hidden class or an unsafe anonymous class.

+1

+1

...

There are some places in libjdwp that need to be fixed. I spoke to Serguei about those this afternoon. Basically the convertSignatureToClassname() function needs to be fixed to handle hidden classes. Without the fix classname filtering will have problems if the filter contains a pattern with a '/' to filter on hidden classes. Also CLASS_UNLOAD events will not properly convert hidden class names. We also need tests for these cases. I think these are all things that can be addressed later.

Good catch.  I have created a subtask under JDK-8230502:    https://bugs.openjdk.java.net/browse/JDK-8230502

Yes, it is good catch. Thank you for filing the subtask. We discussed this with Chris. This was expected to be found with new test coverage and fixed in the JDI chunk of work which we have decided to separate from JEP 371. Thanks, Serguei

...

I still need to look over the JVMTI tests.

Thanks Mandy

...

thanks,

Chris

On 3/26/20 4:57 PM, Mandy Chung 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-JVM...

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

Sorry to jump in on this but it caught my eye though I may be missing a larger context ... On 30/03/2020 7:30 pm, serguei.spitsyn@oracle.com wrote:

Hi Mandy,

I have just one comment so far.

http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03...

 356   void add_classes(LoadedClassInfo* first_class, int num_classes, bool has_class_mirror_holder) {  357     LoadedClassInfo** p_list_to_add_to;  358     bool is_hidden = first_class->_klass->is_hidden();  359     if (has_class_mirror_holder) {  360       p_list_to_add_to = is_hidden ? &_hidden_weak_classes : &_anon_classes;  361     } else {  362       p_list_to_add_to = &_classes;  363     }  364     // Search tail.  365     while ((*p_list_to_add_to) != NULL) {  366       p_list_to_add_to = &(*p_list_to_add_to)->_next;  367     }  368     *p_list_to_add_to = first_class;  369     if (has_class_mirror_holder) {  370       if (is_hidden) {  371         _num_hidden_weak_classes += num_classes;

Why does hidden imply weak here? David -----

 372       } else {  373         _num_anon_classes += num_classes;  374       }  375     } else {  376       _num_classes += num_classes;  377     }  378   }

 Q1: I'm just curious, what happens if a cld has arrays of hidden classes?      Is the bottom_klass always expected to be the first?

Thanks, Serguei

On 3/26/20 16:57, Mandy Chung 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-JVM...

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

On 3/30/20 02:30, serguei.spitsyn@oracle.com wrote:

Hi Mandy,

I have just one comment so far.

http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03...

 356   void add_classes(LoadedClassInfo* first_class, int num_classes, bool has_class_mirror_holder) {  357     LoadedClassInfo** p_list_to_add_to;  358     bool is_hidden = first_class->_klass->is_hidden();  359     if (has_class_mirror_holder) {  360       p_list_to_add_to = is_hidden ? &_hidden_weak_classes : &_anon_classes;  361     } else {  362       p_list_to_add_to = &_classes;  363     }  364     // Search tail.  365     while ((*p_list_to_add_to) != NULL) {  366       p_list_to_add_to = &(*p_list_to_add_to)->_next;  367     }  368     *p_list_to_add_to = first_class;  369     if (has_class_mirror_holder) {  370       if (is_hidden) {  371         _num_hidden_weak_classes += num_classes;  372       } else {  373         _num_anon_classes += num_classes;  374       }  375     } else {  376       _num_classes += num_classes;  377     }  378   }

 Q1: I'm just curious, what happens if a cld has arrays of hidden classes?      Is the bottom_klass always expected to be the first?

Please, skip it. I've got the answer. The array classes were not included into the LoadedClassInfo* by the classes_do. Thanks, Serguei

Thanks, Serguei

On 3/26/20 16:57, Mandy Chung 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-JVM...

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

Alex's feedback:  rename isHiddenClass to isHidden as it can be a hidden class or interface. `isLocalClass` and `sAnonymousClass` are correct because the Java language only has local classes and anon classes, not local interfaces or anon. interfaces.  `isHidden` is like `isSynthetic`, it could be a class or interface. Although isHiddenClass seems clearer, I'm okay to rename it to `isHidden`. Mandy On 3/31/20 11:06 AM, Mandy Chung wrote:

This patch addresses Joe's feedback on the CSR [1]:

http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03...

Specifically, it adds to the class specification of java.lang.Class to describe how the relevant methods behave for hidden classes.  In addition, use the new inline @jvms tag.

Thanks Mandy [1] https://bugs.openjdk.java.net/browse/JDK-8238359

On 3/26/20 4:57 PM, Mandy Chung 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-JVM...

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

On 4/1/20 7:26 AM, Alan Bateman wrote:

Maybe I missed it going by, but why is the isHidden check in the field offset methods on sun.misc.Unsafe rather than the internal Unsafe?

The reflection and VarHandle implementation uses jdk.internal.misc.Unsafe to do field access (both read and write). For fields of a hidden declaring class, Field::get works on final and non-final fields.  Field::set will work on non-final fields and throw IAE on final fields.   That's why no change to the internal Unsafe to support reflective field access. The check in the field offset methods in `sun.misc.Unsafe` intends to constrain the existing use of jdk.unsupported unsafe field offset methods to existing classes but not hidden classes (perhaps same to apply to inline classes) moving toward "trusted non-instance finals". Mandy

Hi Mandy, On 1/04/2020 1:01 pm, Mandy Chung wrote:

Thanks to the review feedbacks.

Updated webrev: http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.04...

I've had a good look through all the hotspot related changes. All looks good. A few minor comments: src/hotspot/share/ci/ciField.cpp // Trust VM hidden and unsafe anonymous classes. should say // Trust hidden and VM unsafe anonymous classes. // the private API (jdk.internal.misc.Unsafe) s/the/a/ or else change the () to commas or perhaps even better: // the private jdk.internal.misc.Unsafe API, --- src/hotspot/share/ci/ciInstanceKlass.cpp // VM weak hidden and unsafe anonymous classes should say // weak hidden and VM unsafe anonymous classes --- src/hotspot/share/classfile/classLoaderData.hpp ! // the CLDs lifecycle. For example, a non-strong hidden class or an ... ! // Used for weak hidden classes, unsafe anonymous classes and the There is an inconsistency in terminology, so far most code comments refer to "weak hidden" but now you are using "non-strong hidden". ! for an weak hidden s/an/a/ multiple locations --- src/hotspot/share/interpreter/linkResolver.cpp Can you fix one of my comments please (in two places) :) + // For private access check if there was a problem with nest host would read better as: + // For private access see if there was a problem with nest host --- Thanks, David ------

Delta between webrev.03 and webrev.04: http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03...

Summary of changes is: 1. Update javac to retain the previous behavior when compiling to target release <= 14 where lambda proxy class is not a nestmate 2. Rename Class::isHiddenClass to Class::isHidden 3. Address Joe's feedback on the CSR to document the behavior of the relevant methods in java.lang.Class for hidden classes 4. Add test case for unloadable class with nest host error 5. Add test cases for hidden classes with different kinds of class or interface 6. Update dcmd to drop "weak hidden class" and refer it as "hidden class" 7. Small changes in response to Remi, Coleen, Paul's review comments 8. Fix copyright headers 9. Fix @modules in tests

Most of the changes above have also been reviewed as separate patches.

Thanks Mandy

On 3/26/20 4:57 PM, Mandy Chung 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

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

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

Please review the delta patch: http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06... Incremental specdiff: http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff-... This is to follow a discussion [1] on Class::descriptorString and MethodType::descriptorString for hidden classes.   The proposal is to define the descriptor string for a hidden class of this form:     "L" + N + ";" + "/" + <suffix> The spec of `Lookup::defineHiddenClass`, `Class::descriptorString` and `MethodType::descriptorString` are updated to return the descriptor of this form for a hidden class.   To support hidden class, `java.lang.invoke.TypeDescriptor` spec is revised such that a `TypeDescriptor` object can represent an entity that may not be described in nominal form.     This change affects JVM TI, JDWP and JDI.    The spec change includes a couple other JVM TI and java.instrument spec clarification w.r.t. hidden classes that Serguei has been working on. Mandy [1] https://mail.openjdk.java.net/pipermail/valhalla-dev/2020-April/007093.html ---------------- As a record, the above patch is applied on the top: http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06... webrev.06 includes the following changes that have been reviewed: 1. rename "weak hidden" and Klass::is_hidden_weak to is_non_strong_hidden 2. remove unused `setImplMethod` method from lambda proxy class 3. include Serguei's patch to fix JDK-8242166: regression in JDI ClassUnload events 4. drop the uniqueness guarantee of the suffix of the hidden class's name On 3/31/20 8:01 PM, Mandy Chung wrote:

Thanks to the review feedbacks.

Updated webrev: http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.04...

Delta between webrev.03 and webrev.04: http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03...

Summary of changes is: 1. Update javac to retain the previous behavior when compiling to target release <= 14 where lambda proxy class is not a nestmate 2. Rename Class::isHiddenClass to Class::isHidden 3. Address Joe's feedback on the CSR to document the behavior of the relevant methods in java.lang.Class for hidden classes 4. Add test case for unloadable class with nest host error 5. Add test cases for hidden classes with different kinds of class or interface 6. Update dcmd to drop "weak hidden class" and refer it as "hidden class" 7. Small changes in response to Remi, Coleen, Paul's review comments 8. Fix copyright headers 9. Fix @modules in tests

Most of the changes above have also been reviewed as separate patches.

Thanks Mandy

On 3/26/20 4:57 PM, Mandy Chung 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

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

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

On 4/14/20 11:51 AM, Paul Sandoz wrote:

Looks good to me (not familiar with all the code areas.

Minor suggestion:

MethodHandles.java

1811 * ASCII periods. For the instance of {@link java.lang.Class} representing {@code C}: 1812 * <ul> 1813 * <li> {@link Class#getName()} returns the string {@code GN + "/" + <suffix>}, 1814 * even though this is not a valid binary class or interface name.</li> 1815 * <li> {@link Class#descriptorString()} returns the string 1816 * {@code "L" + N + ";" + "/" + <suffix> }, 1817 * even though this is not a valid type descriptor name.</li> 1818 * </ul>

Add another bullet:

“ even though this is not a valid type descriptor name; and

- therefore {@link Class#describeConstable} returns an empty {@code Optional}. “

?

OK.   I add this bullet: - Class.describeConstable() returns an empty optional as C cannot be described in nominal form. The webrev and spec was updated [1] for descriptor string to be of the form "Lfoo/Foo.1234;" to mitigate the compatibility risk.  Th Specdiff with serviceability spec change: http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff-... Specdiff without svc spec change: http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff-... Webrev: http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06... Svc spec change webrev: http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06... thanks Mandy [1] https://mail.openjdk.java.net/pipermail/valhalla-dev/2020-April/007155.html

Paul.

...

On Apr 11, 2020, at 8:13 PM, Mandy Chung <mandy.chung@oracle.com> wrote:

Please review the delta patch: http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06...

Incremental specdiff: http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff-... This is to follow a discussion [1] on Class::descriptorString and MethodType::descriptorString for hidden classes. The proposal is to define the descriptor string for a hidden class of this form: "L" + N + ";" + "/" + <suffix>

The spec of `Lookup::defineHiddenClass`, `Class::descriptorString` and `MethodType::descriptorString` are updated to return the descriptor of this form for a hidden class. To support hidden class, `java.lang.invoke.TypeDescriptor` spec is revised such that a `TypeDescriptor` object can represent an entity that may not be described in nominal form. This change affects JVM TI, JDWP and JDI. The spec change includes a couple other JVM TI and java.instrument spec clarification w.r.t. hidden classes that Serguei has been working on.

Mandy [1] https://mail.openjdk.java.net/pipermail/valhalla-dev/2020-April/007093.html

---------------- As a record, the above patch is applied on the top: http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06...

webrev.06 includes the following changes that have been reviewed: 1. rename "weak hidden" and Klass::is_hidden_weak to is_non_strong_hidden 2. remove unused `setImplMethod` method from lambda proxy class 3. include Serguei's patch to fix JDK-8242166: regression in JDI ClassUnload events 4. drop the uniqueness guarantee of the suffix of the hidden class's name

On 3/31/20 8:01 PM, Mandy Chung wrote:

...

Thanks to the review feedbacks.

Updated webrev: http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.04... Delta between webrev.03 and webrev.04: http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03...

Summary of changes is: 1. Update javac to retain the previous behavior when compiling to target release <= 14 where lambda proxy class is not a nestmate 2. Rename Class::isHiddenClass to Class::isHidden 3. Address Joe's feedback on the CSR to document the behavior of the relevant methods in java.lang.Class for hidden classes 4. Add test case for unloadable class with nest host error 5. Add test cases for hidden classes with different kinds of class or interface 6. Update dcmd to drop "weak hidden class" and refer it as "hidden class" 7. Small changes in response to Remi, Coleen, Paul's review comments 8. Fix copyright headers 9. Fix @modules in tests

Most of the changes above have also been reviewed as separate patches.

Thanks Mandy

On 3/26/20 4:57 PM, Mandy Chung 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

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

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

On 4/18/20 12:47 AM, Chris Plummer wrote:

...

It's a link to https://download.java.net/java/early_access/jdk15/docs/specs/jni/types.html#.... 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.

Right, they are existing reference.  I'll let Serguei to file a JBS issue to follow 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"?

I agree that the svc specs should be examined and use the terminologies consistently.

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

Mandy

Ok, I think I found one such use-case. In the following example: package test; public class LambdaTest {     protected void m() {     } } package test.sub; public class LambdaTestSub extends test.LambdaTest {     public void test() {         Runnable r = this::m;         r.run();     } } ...when compiled with JDK 14 javac. In this case the implClass is test.sub.LambdaTestSub while implInfo is "invokeVirtual test.LambdaTest.m:()void" and the method is not public. Anyway, the name of the proxy class is derived from the targetClass (and therefore shares the same package with targetClass) which is caller's lookup class. Is targetClass always the same as implClass in the invokeVirtual/invokeInterface case? I also noticed that JDK 15 patched javac transforms method reference in above code into a lambda method. But looking at the javac changes in the patch, I don't quite see where this distinction between JDK 14 and patched JDK 15 javac comes from. From the changes to method com.sun.tools.javac.comp.LambdaToMethod.LambdaAnalyzerPreprocessor.ReferenceTranslationContext#needsConversionToLambda:             final boolean needsConversionToLambda() {                 return interfaceParameterIsIntersectionOrUnionType() ||                         isSuper ||                         needsVarArgsConversion() ||                         isArrayOp() || #                        isPrivateInOtherClass() || isProtectedInSuperClassOfEnclosingClassInOtherPackage() ||                         !receiverAccessible() ||                         (tree.getMode() == ReferenceMode.NEW &&                           tree.kind != ReferenceKind.ARRAY_CTOR &&                           (tree.sym.owner.isLocal() || tree.sym.owner.isInner()));             } ...I would draw the conclusion that conversion to lambda is performed in less cases not in more. Hm. Regards, Peter On 4/3/20 11:11 AM, Peter Levart wrote:

Hi Mandy,

Good work.

I'm trying to find out which language use-case is covered by the InnerClassLambdaMetafactory needing to inject method handle into the generated proxy class to be invoked instead of proxy class directly invoking the method:

        useImplMethodHandle = !implClass.getPackageName().equals(implInfo.getDeclaringClass().getPackageName())                                 && !Modifier.isPublic(implInfo.getModifiers());

If I check what implClass and implInfo get resolved to in AbstractValidatingLambdaMetafactory, there are several cases:

        this.implInfo = caller.revealDirect(implMethod);         switch (implInfo.getReferenceKind()) {             case REF_invokeVirtual:             case REF_invokeInterface:                 this.implClass = implMethodType.parameterType(0);                 // reference kind reported by implInfo may not match implMethodType's first param                 // Example: implMethodType is (Cloneable)String, implInfo is for Object.toString                 this.implKind = implClass.isInterface() ? REF_invokeInterface : REF_invokeVirtual;                 this.implIsInstanceMethod = true;                 break;             case REF_invokeSpecial:                 // JDK-8172817: should use referenced class here, but we don't know what it was                 this.implClass = implInfo.getDeclaringClass();                 this.implIsInstanceMethod = true;

                // Classes compiled prior to dynamic nestmate support invokes a private instance                 // method with REF_invokeSpecial.                 //                 // invokespecial should only be used to invoke private nestmate constructors.                 // The lambda proxy class will be defined as a nestmate of targetClass.                 // If the method to be invoked is an instance method of targetClass, then                 // convert to use invokevirtual or invokeinterface.                 if (targetClass == implClass && !implInfo.getName().equals("<init>")) {                     this.implKind = implClass.isInterface() ? REF_invokeInterface : REF_invokeVirtual;                 } else {                     this.implKind = REF_invokeSpecial;                 }                 break;             case REF_invokeStatic:             case REF_newInvokeSpecial:                 // JDK-8172817: should use referenced class here for invokestatic, but we don't know what it was                 this.implClass = implInfo.getDeclaringClass();                 this.implKind = implInfo.getReferenceKind();                 this.implIsInstanceMethod = false;                 break;             default:                 throw new LambdaConversionException(String.format("Unsupported MethodHandle kind: %s", implInfo));         }

For majority of cases (REF_invokeSpecial, REF_invokeStatic, REF_newInvokeSpecial) the this.implClass = implInfo.getDeclaringClass();

Only REF_invokeVirtual and REF_invokeInterface are possible kandidates, right?

So when does the implMethod type of parameter 0 differ from the declaring class of the MethodHandleInfo cracked from that same implMethod and in addition those two types leave in different packages?

Regards, Peter

On 3/27/20 12:57 AM, Mandy Chung 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-JVM...

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

Hi Mandy, On 4/3/20 11:32 PM, Mandy Chung wrote:

Hi Peter,

I added a JBS comment [1] to describe this special case trying to put the story together (let me know if it needs more explanation).   More comment inline below.

Thanks, this helps in establishing the historical context.

On 4/3/20 4:40 AM, Peter Levart wrote:

...

Ok, I think I found one such use-case. In the following example:

package test; public class LambdaTest {     protected void m() {     } }

package test.sub; public class LambdaTestSub extends test.LambdaTest {     public void test() {         Runnable r = this::m;         r.run();     } }

Yes.

This is specific for binary compatibility.   the invocation of a protected method inherited from its supertype in a different package.

The lambda proxy is in the same package as the target class (`test.sub` in the example above) but it has no access to `test.LambdaTest::m`.

...

...when compiled with JDK 14 javac. In this case the implClass is test.sub.LambdaTestSub while implInfo is "invokeVirtual test.LambdaTest.m:()void" and the method is not public.

In JDK 14, a lambda proxy `test.sub.LambdaTestSub$Lambda$$1234` is VM anonymous class which has a special powerful access as if the host class.   This proxy class, even though it's not an instance of `test.LambdaTest`, can invoke  the protected`test.LambdaTest.m:()void` member.

Right, the VM anonymous class "inherits" all access rights from the host class, which in above example is the subclass of test.LambdaTes.

...

Anyway, the name of the proxy class is derived from the targetClass (and therefore shares the same package with targetClass) which is caller's lookup class. Is targetClass always the same as implClass in the invokeVirtual/invokeInterface case?

implMethod is the direct method handle describing the implementation method resolved by the VM.   So it depends.

In the above example, it's `test.LambdaTest.m:()void` and implClass is test.LambdaTest.

Here I think, you are not quite right. First I need to clarify that we are talking about the case where the method reference in above example is not converted to lambda by javac, so the proxy class needs to invoke the superclass method directly (without the help of lambda bridge). I did an experiment and compiled the example with JDK 13 javac, where the patch for (JDK-8234729) is not applied yet. What I get from this compilation is the following metafactory bootstrap method invocation:   0: #35 REF_invokeStatic java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite;     Method arguments:       #42 ()V       #43 REF_invokeVirtual test/LambdaTest.m:()V       #42 ()V The #43 is the implMethod method handle and it is the following:   #43 = MethodHandle       5:#44          // REF_invokeVirtual test/LambdaTest.m:()V   #44 = Methodref          #2.#45         // test/LambdaTest.m:()V   #45 = NameAndType        #46:#6         // m:()V   #46 = Utf8               m    #2 = Class              #4             // test/LambdaTest    #4 = Utf8               test/LambdaTest *BUT* the class that looks up this MH from the constant pool is the subclass (test.sub.LambdaTestSub) which is equivalent to the following programmatic lookup:         var mh = MethodHandles.lookup().findVirtual(LambdaTest.class, "m", MethodType.methodType(void.class));         System.out.println(mh.type()); and this results in a method handle of the following type: (LambdaTestSub)void which is correct since the method handle *MUST* check that the passed in instance (this) is of type LambdaTestSub or subtype or else the "protected" access would be violated. And since the ref type is REF_invokeVirtual, the AbstractValidatingLambdaMetafactory assigns the following to the implClass:         this.implMethodType = implMethod.type();         this.implInfo = caller.revealDirect(implMethod);         switch (implInfo.getReferenceKind()) {             case REF_invokeVirtual:             case REF_invokeInterface:                 this.implClass = implMethodType.parameterType(0); ...which makes the implClass be LambdaTestSub in this case. Which is correct again since we want InnerClassLambdaMetafactory to decide that this is the special case for proxy to invoke the method via method handle:         useImplMethodHandle = !implClass.getPackageName().equals(implInfo.getDeclaringClass().getPackageName())                                 && !Modifier.isPublic(implInfo.getModifiers()); If implClass was test.LambdaTest as you said above, this condition would evaluate to false, since implInfo is "invokeVirtual test.LambdaTest.m:()void" in above case. So everything is OK, but my original question was the following: The name of the generated proxy class is derived from the targetClass which is the caller's lookup class. In this example the caller is LambdaTestSub and this is the same as implClass in this case. Would those two classes always be the same in the case where the evaluation of the above `useImplMethodHandle` boolean matters? I mean, the decision about whether to base the proxy invocation strategy on method handle or direct bytecode invocation is based on one class (implClass), but the actual package of the proxy class which effectively influences the bytecode invocation rights is taken from another class (targetClass). On one hand the package of the proxy class has to be the same as targetClass if the proxy wants to be the nestmate of the targetClass (for example to have private access to the members of the nest). But OTOH it needs to be the same package also with implClass so that the above decision of the proxy invocation strategy is correct. I have a feeling that for protected virtual methods, this is true, because the type of the 0-argument of such method handle is always the lookup class. But am I right? What do you think of another alternative to invoking the super protected method in other package: What if the LMF would decide to base the name of the proxy class on the implInfo.getDeclaringClass() in such case? It would not have to be a nestmate of any class, just in the same package with the method's declaring class. Consequently it would be in the same module as the declaring class and loaded by the same class loader. Therefore it would have to be WEAKLY referenced from the class loader. And the Lookup instance passed to bootstrap LMF method would not be enough for defining such class. But LMF could obtain special powers since it is JDK internal class... Well, I don't know for myself. Is this situation rare enough so that invoking via method handle is not a drawback? It only happens when running JDK 13- compiled classes with JDK 15+ and in addition the method reference must point to protected method in a distant class.

The targetClass is test.sub.LambdaTestSub which is *NOT* the same as implClass.  That's why we change if they are in the same package.

It can be invoking a method in targetClass or a method in another class in the same package with package access, then implClass may or may not be the same as targetClass.

...

I also noticed that JDK 15 patched javac transforms method reference in above code into a lambda method. But looking at the javac changes in the patch, I don't quite see where this distinction between JDK 14 and patched JDK 15 javac comes from.

javac has been changed in JDK 14 to synthesize a bridge method if it's a method reference to access a protected member in a remote supertype  (JDK-8234729).

Ah, that was the reason I haven't seen the change in your patch. I used the JDK 13 javac and thought it would be the same as JDK 14 javac. My mistake. So this answers the question below... Regards, Peter

BTW, the new tests relevant to this scenario are under test/jdk/java/lang/invoke/lambda/superProtectedMethod.

...

From the changes to method com.sun.tools.javac.comp.LambdaToMethod.LambdaAnalyzerPreprocessor.ReferenceTranslationContext#needsConversionToLambda:

            final boolean needsConversionToLambda() {                 return interfaceParameterIsIntersectionOrUnionType() ||                         isSuper ||                         needsVarArgsConversion() ||                         isArrayOp() || #                        isPrivateInOtherClass() || isProtectedInSuperClassOfEnclosingClassInOtherPackage() ||                         !receiverAccessible() ||                         (tree.getMode() == ReferenceMode.NEW &&                           tree.kind != ReferenceKind.ARRAY_CTOR &&                           (tree.sym.owner.isLocal() || tree.sym.owner.isInner()));             }

...I would draw the conclusion that conversion to lambda is performed in less cases not in more.

Jan and Srikanath may be able to explain this further.

...

Hm.

Regards, Peter

On 4/3/20 11:11 AM, Peter Levart wrote:

...

Hi Mandy,

Good work.

I'm trying to find out which language use-case is covered by the InnerClassLambdaMetafactory needing to inject method handle into the generated proxy class to be invoked instead of proxy class directly invoking the method:

        useImplMethodHandle = !implClass.getPackageName().equals(implInfo.getDeclaringClass().getPackageName())                                 && !Modifier.isPublic(implInfo.getModifiers());

If I check what implClass and implInfo get resolved to in AbstractValidatingLambdaMetafactory, there are several cases:

        this.implInfo = caller.revealDirect(implMethod);         switch (implInfo.getReferenceKind()) {             case REF_invokeVirtual:             case REF_invokeInterface:                 this.implClass = implMethodType.parameterType(0);                 // reference kind reported by implInfo may not match implMethodType's first param                 // Example: implMethodType is (Cloneable)String, implInfo is for Object.toString                 this.implKind = implClass.isInterface() ? REF_invokeInterface : REF_invokeVirtual;                 this.implIsInstanceMethod = true;                 break;             case REF_invokeSpecial:                 // JDK-8172817: should use referenced class here, but we don't know what it was                 this.implClass = implInfo.getDeclaringClass();                 this.implIsInstanceMethod = true;

                // Classes compiled prior to dynamic nestmate support invokes a private instance                 // method with REF_invokeSpecial.                 //                 // invokespecial should only be used to invoke private nestmate constructors.                 // The lambda proxy class will be defined as a nestmate of targetClass.                 // If the method to be invoked is an instance method of targetClass, then                 // convert to use invokevirtual or invokeinterface.                 if (targetClass == implClass && !implInfo.getName().equals("<init>")) {                     this.implKind = implClass.isInterface() ? REF_invokeInterface : REF_invokeVirtual;                 } else {                     this.implKind = REF_invokeSpecial;                 }                 break;             case REF_invokeStatic:             case REF_newInvokeSpecial:                 // JDK-8172817: should use referenced class here for invokestatic, but we don't know what it was                 this.implClass = implInfo.getDeclaringClass();                 this.implKind = implInfo.getReferenceKind();                 this.implIsInstanceMethod = false;                 break;             default:                 throw new LambdaConversionException(String.format("Unsupported MethodHandle kind: %s", implInfo));         }

For majority of cases (REF_invokeSpecial, REF_invokeStatic, REF_newInvokeSpecial) the this.implClass = implInfo.getDeclaringClass();

Only REF_invokeVirtual and REF_invokeInterface are possible kandidates, right?

So when does the implMethod type of parameter 0 differ from the declaring class of the MethodHandleInfo cracked from that same implMethod and in addition those two types leave in different packages?

This is concerning the instance method and so parameter 0 is what it wants to look at.

...

...

Regards, Peter

Hope this helps. Mandy [1] https://bugs.openjdk.java.net/browse/JDK-8239384?focusedCommentId=14328369&p...

On 4/4/20 9:16 AM, Peter Levart wrote:

Hi Mandy,

Just another observation of the code in InnerClassLambdaMetafactory...

For the useImplMethodHandle case you generate the protectedImplMethod static field to hold the MH and a static setter method:

            mv = cw.visitMethod(ACC_PRIVATE + ACC_STATIC,                                 "setImplMethod", DESCR_SET_IMPL_METHOD,                                 null, null);

...but then later after you define the class you inject the MH via a "staticSetter" method handle:

                MethodHandle mh = lookup.findStaticSetter(lookup.lookupClass(), NAME_FIELD_IMPL_METHOD, MethodHandle.class);                 mh.invokeExact(implMethod);

So you don't invoke the generated setter method but set the field via special kind of method handle (equivalent to putstatic bytecode). You can remove the setImplMethod method generation code.

Good catch.   Removed the unused method. Mandy

Regards, Peter