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

[Mandy Chung]

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#type-signatures. 
This is how the current JVM TI spec defines.
Since it looks like this reference was present before your changes, I 
guess it's ok to leave it, but like JDWP maybe a bug should be filed 
to clean it up.




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

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

[Chris Plummer]

Hi Mandy,

Comments inline.

On 4/17/20 4:52 PM, Mandy Chung wrote:



On 4/17/20 3:51 PM, Chris Plummer wrote:

Hi Mandy,

Thanks for updating the svc specs. Some comments below:


In the JDWP spec update, you changed "JNI signature" to "type 
signature" in one place, but left it as "JNI signature" everywhere 
else. Should they all be changed?




JDWP signature is changed because there is no JNI signature 
representing a hidden class.    I leave other references to JNI 
signature as is since those only apply for normal classes as I wanted 
to keep this change specifically due to hidden classes.  I think it's 
good to file a JBS issue to follow up on JDWP and JDI to determine if 
the spec should be upgraded to reference the new TypeDescriptor API.




In the JDWP spec for ClassLoaderReference.VisibleClasses:

 "That is, this class loader has been recorded as an 
initiating loader of the returned classes." -> "That is, all 
classes for which this class loader has been recorded as an 
initiating loader."


This seems like too much detail to be put here. Basically the term 
"initiating ClassLoader" has turned into a short essay. Is it 
possible that all this detail could be put elsewhere and referenced? 


Any suggestion?   We attempted to place those description in JVM TI 
Class section or ClassLoad event.   However, that's not ideal place 
since that's needed by JDWP, JDI and Instrumentation.   I found 
inlining this description is not ideal but it provides adequate 
clarification.

See other thread.


Aren't there other places in other specs where a similar 
clarification of "initiating ClassLoader" is needed (I see now that 
ClassLoaderClasses in the JVMTI spec, 
ClassLoaderReference,visibleClasses in the JDI spec, and 
Instrumentation.getInitiatedClasses are all dealing with this, but 
not all in the exact same way).




I took the conservative side and make sure the clarification is in 
place for all APIs.  I'm open to any suggestion for example having 
JDWP and JDI to link to JVM TI spec if you think appropriate.




In the JVMTI spec for GetLoadedClasses:

This suffers in a way similar to ClassLoaderReference.VisibleClasses 
in the JDWP spec, although not as badly. A simple concept ends up 
with a complex description, and it seems that description should 
really be in a more centralized place.  I would also suggest a bit of 
cleanup of these lines:


6866 An array class is created directly by Java virtual 
machine.  An array class
6867 creation can be triggered by using class loaders or by 
invoking methods in certain

6868 Java SE Platform API such as reflection.

"Created by [the] Java virtual machine" (add "the")
Change "An array class creation" to "The creation" since your are 
repeating "An array class" from the previous sentence.



In the JVMTI spec ClassLoaderClasses section:

"That is, initiating_loader has been recorded as an initiating loader 
of the returned classes." -> "That is, all classes for which 
initiating_loader has been recorded as an initiating loader."



In the JVMTI spec GetClassSignature section:

"If the class indicated by klass is ..." -> "If the class ..." (you 
just finished the previous sentence with "class indicated by Klass").


"the returned name is [the] JNI type signature" (add "the"). Also, is 
"JNI type signature" formally defined somewhere? This relates to my 
JDWP spec comment above.




It's a link to 
https://download.java.net/java/early_access/jdk15/docs/specs/jni/types.html#type-signatures. 
This is how the current JVM TI spec defines.
Since it looks like this reference was present before your changes, I 
guess it's ok to leave it, but like JDWP maybe a bug should be filed to 
clean it up.


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


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




" where N is the binary name encoded in internal form indicated by 
the class file". Is "binary name encoded in internal form" explained 
somewhere? 


JVMS 4.2.1

https://docs.oracle.com/javase/specs/jvms/se14/html/jvms-4.html#jvms-4.2.1

Thanks. I see you've also added a reference in your edits below.



Also, can you add an example of a returned hidden class signature?


OK



In the JVMTI spec ClassLoad section:

"representation using [a] class loader" (add "a")

"By invoking Lookup::defineHiddenClass, that creates ..."  -> "By 

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

[Chris Plummer]

On 4/17/20 9:37 PM, serguei.spit...@oracle.com wrote:

On 4/17/20 16:52, Mandy Chung wrote:



On 4/17/20 3:51 PM, Chris Plummer wrote:

Hi Mandy,

Thanks for updating the svc specs. Some comments below:


In the JDWP spec update, you changed "JNI signature" to "type 
signature" in one place, but left it as "JNI signature" everywhere 
else. Should they all be changed?




JDWP signature is changed because there is no JNI signature 
representing a hidden class.    I leave other references to JNI 
signature as is since those only apply for normal classes as I wanted 
to keep this change specifically due to hidden classes. I think it's 
good to file a JBS issue to follow up on JDWP and JDI to determine if 
the spec should be upgraded to reference the new TypeDescriptor API.




In the JDWP spec for ClassLoaderReference.VisibleClasses:

 "That is, this class loader has been recorded as an 
initiating loader of the returned classes." -> "That is, all 
classes for which this class loader has been recorded as an 
initiating loader."


This seems like too much detail to be put here. Basically the term 
"initiating ClassLoader" has turned into a short essay. Is it 
possible that all this detail could be put elsewhere and referenced? 


Any suggestion?   We attempted to place those description in JVM TI 
Class section or ClassLoad event.   However, that's not ideal place 
since that's needed by JDWP, JDI and Instrumentation.   I found 
inlining this description is not ideal but it provides adequate 
clarification.


The JDI (transitively via JDWP), JDWP and Instrumentation 
implementations are based on the JVMTI.

I've tried to suggest once to link these API's to the JVMTI.
The problem is there was no such practice in the specs of these API's 
before but we can make a step to introduce it now.
Placing this description either in JVM TI Class section or ClassLoad 
event would be good enough.


An alternative approach is to make JVMTI/JDI/JDWP/Instrument to refer 
to the java.lang.Class spec for
general information about class loading and classes defined and 
initiated by class loaders.
I'd first like to clarify on my comment above just in case there was any 
confusion. At the last minute I re-ordered the two paragraph and now I 
see it makes it seem like I was only referring to to the one sentence 
about initiating loaders. I was referring to pretty much all the changes 
that were made in this section and other similar APIs in the other specs.


Yes. I'd like to see all this as part of the Class/Classloading spec in 
some sort of section that gives an overview of all these topics, but 
mostly clarifies what an initiating loader is, and the (non) 
relationship to hidden classes.


thanks,

Chris


Thanks,
Serguei

Aren't there other places in other specs where a similar 
clarification of "initiating ClassLoader" is needed (I see now that 
ClassLoaderClasses in the JVMTI spec, 
ClassLoaderReference,visibleClasses in the JDI spec, and 
Instrumentation.getInitiatedClasses are all dealing with this, but 
not all in the exact same way).




I took the conservative side and make sure the clarification is in 
place for all APIs.  I'm open to any suggestion for example having 
JDWP and JDI to link to JVM TI spec if you think appropriate.




In the JVMTI spec for GetLoadedClasses:

This suffers in a way similar to ClassLoaderReference.VisibleClasses 
in the JDWP spec, although not as badly. A simple concept ends up 
with a complex description, and it seems that description should 
really be in a more centralized place.  I would also suggest a bit 
of cleanup of these lines:


6866 An array class is created directly by Java virtual 
machine.  An array class
6867 creation can be triggered by using class loaders or by 
invoking methods in certain

6868 Java SE Platform API such as reflection.

"Created by [the] Java virtual machine" (add "the")
Change "An array class creation" to "The creation" since your are 
repeating "An array class" from the previous sentence.



In the JVMTI spec ClassLoaderClasses section:

"That is, initiating_loader has been recorded as an initiating 
loader of the returned classes." -> "That is, all classes for which 
initiating_loader has been recorded as an initiating loader."



In the JVMTI spec GetClassSignature section:

"If the class indicated by klass is ..." -> "If the class ..." (you 
just finished the previous sentence with "class indicated by Klass").


"the returned name is [the] JNI type signature" (add "the"). Also, 
is "JNI type signature" formally defined somewhere? This relates to 
my JDWP spec comment above.




It's a link to 
https://download.java.net/java/early_access/jdk15/docs/specs/jni/types.html#type-signatures. 
This is how the current JVM TI spec defines.


" where N is the binary name encoded in internal form indicated by 
the class file". Is "binary name encoded in internal form" explained 
somewhere? 


JVMS 4.2.1

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

[serguei.spit...@oracle.com]

On 4/17/20 16:52, Mandy Chung wrote:



On 4/17/20 3:51 PM, Chris Plummer wrote:

Hi Mandy,

Thanks for updating the svc specs. Some comments below:


In the JDWP spec update, you changed "JNI signature" to "type 
signature" in one place, but left it as "JNI signature" everywhere 
else. Should they all be changed?




JDWP signature is changed because there is no JNI signature 
representing a hidden class.    I leave other references to JNI 
signature as is since those only apply for normal classes as I wanted 
to keep this change specifically due to hidden classes.  I think it's 
good to file a JBS issue to follow up on JDWP and JDI to determine if 
the spec should be upgraded to reference the new TypeDescriptor API.




In the JDWP spec for ClassLoaderReference.VisibleClasses:

 "That is, this class loader has been recorded as an 
initiating loader of the returned classes." -> "That is, all 
classes for which this class loader has been recorded as an 
initiating loader."


This seems like too much detail to be put here. Basically the term 
"initiating ClassLoader" has turned into a short essay. Is it 
possible that all this detail could be put elsewhere and referenced? 


Any suggestion?   We attempted to place those description in JVM TI 
Class section or ClassLoad event.   However, that's not ideal place 
since that's needed by JDWP, JDI and Instrumentation.   I found 
inlining this description is not ideal but it provides adequate 
clarification.


The JDI (transitively via JDWP), JDWP and Instrumentation 
implementations are based on the JVMTI.

I've tried to suggest once to link these API's to the JVMTI.
The problem is there was no such practice in the specs of these API's 
before but we can make a step to introduce it now.
Placing this description either in JVM TI Class section or ClassLoad 
event would be good enough.


An alternative approach is to make JVMTI/JDI/JDWP/Instrument to refer to 
the java.lang.Class spec for
general information about class loading and classes defined and 
initiated by class loaders.


Thanks,
Serguei

Aren't there other places in other specs where a similar 
clarification of "initiating ClassLoader" is needed (I see now that 
ClassLoaderClasses in the JVMTI spec, 
ClassLoaderReference,visibleClasses in the JDI spec, and 
Instrumentation.getInitiatedClasses are all dealing with this, but 
not all in the exact same way).




I took the conservative side and make sure the clarification is in 
place for all APIs.  I'm open to any suggestion for example having 
JDWP and JDI to link to JVM TI spec if you think appropriate.




In the JVMTI spec for GetLoadedClasses:

This suffers in a way similar to ClassLoaderReference.VisibleClasses 
in the JDWP spec, although not as badly. A simple concept ends up 
with a complex description, and it seems that description should 
really be in a more centralized place.  I would also suggest a bit of 
cleanup of these lines:


6866 An array class is created directly by Java virtual 
machine.  An array class
6867 creation can be triggered by using class loaders or by 
invoking methods in certain

6868 Java SE Platform API such as reflection.

"Created by [the] Java virtual machine" (add "the")
Change "An array class creation" to "The creation" since your are 
repeating "An array class" from the previous sentence.



In the JVMTI spec ClassLoaderClasses section:

"That is, initiating_loader has been recorded as an initiating loader 
of the returned classes." -> "That is, all classes for which 
initiating_loader has been recorded as an initiating loader."



In the JVMTI spec GetClassSignature section:

"If the class indicated by klass is ..." -> "If the class ..." (you 
just finished the previous sentence with "class indicated by Klass").


"the returned name is [the] JNI type signature" (add "the"). Also, is 
"JNI type signature" formally defined somewhere? This relates to my 
JDWP spec comment above.




It's a link to 
https://download.java.net/java/early_access/jdk15/docs/specs/jni/types.html#type-signatures. 
This is how the current JVM TI spec defines.


" where N is the binary name encoded in internal form indicated by 
the class file". Is "binary name encoded in internal form" explained 
somewhere? 


JVMS 4.2.1

https://docs.oracle.com/javase/specs/jvms/se14/html/jvms-4.html#jvms-4.2.1 




Also, can you add an example of a returned hidden class signature?


OK



In the JVMTI spec ClassLoad section:

"representation using [a] class loader" (add "a")

"By invoking Lookup::defineHiddenClass, that creates ..."  -> "By 
invoking Lookup::defineHiddenClass to create ..."


"certain Java SE Platform API" -> Should be "APIs"


In JDI ClassLoaderReference.definedClasses()

"loaded at least to the point of preparation and types ..." -> 
"loaded at least to the point of preparation, and types ..." (Note, 
this not a new issue with your edits)



In Instrumentation.getAllLoadedClasses:

The reference to `class` did 

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

[Mandy Chung]

On 4/17/20 3:51 PM, Chris Plummer wrote:

Hi Mandy,

Thanks for updating the svc specs. Some comments below:


In the JDWP spec update, you changed "JNI signature" to "type 
signature" in one place, but left it as "JNI signature" everywhere 
else. Should they all be changed?




JDWP signature is changed because there is no JNI signature representing 
a hidden class.    I leave other references to JNI signature as is since 
those only apply for normal classes as I wanted to keep this change 
specifically due to hidden classes.  I think it's good to file a JBS 
issue to follow up on JDWP and JDI to determine if the spec should be 
upgraded to reference the new TypeDescriptor API.




In the JDWP spec for ClassLoaderReference.VisibleClasses:

 "That is, this class loader has been recorded as an initiating 
loader of the returned classes." -> "That is, all classes for which 
this class loader has been recorded as an initiating loader."


This seems like too much detail to be put here. Basically the term 
"initiating ClassLoader" has turned into a short essay. Is it possible 
that all this detail could be put elsewhere and referenced? 


Any suggestion?   We attempted to place those description in JVM TI 
Class section or ClassLoad event.   However, that's not ideal place 
since that's needed by JDWP, JDI and Instrumentation.   I found inlining 
this description is not ideal but it provides adequate clarification.


Aren't there other places in other specs where a similar clarification 
of "initiating ClassLoader" is needed (I see now that 
ClassLoaderClasses in the JVMTI spec, 
ClassLoaderReference,visibleClasses in the JDI spec, and 
Instrumentation.getInitiatedClasses are all dealing with this, but not 
all in the exact same way).




I took the conservative side and make sure the clarification is in place 
for all APIs.  I'm open to any suggestion for example having JDWP and 
JDI to link to JVM TI spec if you think appropriate.




In the JVMTI spec for GetLoadedClasses:

This suffers in a way similar to ClassLoaderReference.VisibleClasses 
in the JDWP spec, although not as badly. A simple concept ends up with 
a complex description, and it seems that description should really be 
in a more centralized place.  I would also suggest a bit of cleanup of 
these lines:


6866 An array class is created directly by Java virtual 
machine.  An array class
6867 creation can be triggered by using class loaders or by 
invoking methods in certain

6868 Java SE Platform API such as reflection.

"Created by [the] Java virtual machine" (add "the")
Change "An array class creation" to "The creation" since your are 
repeating "An array class" from the previous sentence.



In the JVMTI spec ClassLoaderClasses section:

"That is, initiating_loader has been recorded as an initiating loader 
of the returned classes." -> "That is, all classes for which 
initiating_loader has been recorded as an initiating loader."



In the JVMTI spec GetClassSignature section:

"If the class indicated by klass is ..." -> "If the class ..." (you 
just finished the previous sentence with "class indicated by Klass").


"the returned name is [the] JNI type signature" (add "the"). Also, is 
"JNI type signature" formally defined somewhere? This relates to my 
JDWP spec comment above.




It's a link to 
https://download.java.net/java/early_access/jdk15/docs/specs/jni/types.html#type-signatures. 
This is how the current JVM TI spec defines.


" where N is the binary name encoded in internal form indicated by the 
class file". Is "binary name encoded in internal form" explained 
somewhere? 


JVMS 4.2.1

https://docs.oracle.com/javase/specs/jvms/se14/html/jvms-4.html#jvms-4.2.1


Also, can you add an example of a returned hidden class signature?


OK



In the JVMTI spec ClassLoad section:

"representation using [a] class loader" (add "a")

"By invoking Lookup::defineHiddenClass, that creates ..."  -> "By 
invoking Lookup::defineHiddenClass to create ..."


"certain Java SE Platform API" -> Should be "APIs"


In JDI ClassLoaderReference.definedClasses()

"loaded at least to the point of preparation and types ..." -> "loaded 
at least to the point of preparation, and types ..." (Note, this not a 
new issue with your edits)



In Instrumentation.getAllLoadedClasses:

The reference to `class` did not format properly.



Serguei caught that one too.  I fixed it in my local repo.

"by invoking Lookup::defineHiddenClass that creates" -> "by invoking 
Lookup::defineHiddenClass, which creates"


"An array class is created directly by Java virtual machine. An array 
class creation can be triggered ..." ->"An array class is created 
directly by the Java virtual machine. Array class creation can be 
triggered ..."



In Instrumentation.getInitiatedClasses:

"That is, loader has been recorded as an initiating loader of these 
classes." -> "That is, all classes for which loader has been recorded 
as an initiating loader."


thanks,

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

[Chris Plummer]

On 4/16/20 9:45 AM, Mandy Chung wrote:

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  * 
1813  *  {@link Class#getName()} returns the string 
{@code GN + "/" + },
1814  *  even though this is not a valid binary class or 
interface name.

1815  *  {@link Class#descriptorString()} returns the string
1816  *  {@code "L" + N + ";" + "/" +  },
1817  *  even though this is not a valid type descriptor 
name.

1818  * 

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-inc/ 


Specdiff without svc spec change:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff-descriptor-string/overview-summary.html 



Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-descriptor-string/ 



Svc spec change webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/ 



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


Hi Mandy,

Thanks for updating the svc specs. Some comments below:


In the JDWP spec update, you changed "JNI signature" to "type signature" 
in one place, but left it as "JNI signature" everywhere else. Should 
they all be changed?



In the JDWP spec for ClassLoaderReference.VisibleClasses:

 "That is, this class loader has been recorded as an initiating 
loader of the returned classes." -> "That is, all classes for which this 
class loader has been recorded as an initiating loader."


This seems like too much detail to be put here. Basically the term 
"initiating ClassLoader" has turned into a short essay. Is it possible 
that all this detail could be put elsewhere and referenced? Aren't there 
other places in other specs where a similar clarification of "initiating 
ClassLoader" is needed (I see now that ClassLoaderClasses in the JVMTI 
spec, ClassLoaderReference,visibleClasses in the JDI spec, and 
Instrumentation.getInitiatedClasses are all dealing with this, but not 
all in the exact same way).



In the JVMTI spec for GetLoadedClasses:

This suffers in a way similar to ClassLoaderReference.VisibleClasses in 
the JDWP spec, although not as badly. A simple concept ends up with a 
complex description, and it seems that description should really be in a 
more centralized place. I would also suggest a bit of cleanup of these 
lines:


6866 An array class is created directly by Java virtual 
machine.  An array class
6867 creation can be triggered by using class loaders or by 
invoking methods in certain

6868 Java SE Platform API such as reflection.

"Created by [the] Java virtual machine" (add "the")

Change "An array class creation" to "The creation" since your are 
repeating "An array class" from the previous sentence.



In the JVMTI spec ClassLoaderClasses section:

"That is, initiating_loader has been recorded as an initiating loader of 
the returned classes." -> "That is, all classes for which 
initiating_loader has been recorded as an initiating loader."



In the JVMTI spec GetClassSignature section:

"If the class indicated by klass is ..." -> "If the class ..." (you just 
finished the previous sentence with "class indicated by Klass").


"the returned name is [the] JNI type signature" (add "the"). Also, is 
"JNI type signature" formally defined somewhere? This relates to my JDWP 
spec comment above.


" where N is the binary name encoded in internal form indicated by the 
class file". Is "binary name encoded in internal form" explained 
somewhere? Also, can you add an example of a returned hidden class 
signature?



In the JVMTI spec ClassLoad section:

"representation using [a] class loader" (add "a")

"By invoking Lookup::defineHiddenClass, that creates ..."  -> "By 
invoking Lookup::defineHiddenClass to create ..."


"certain Java SE Platform API" -> Should be "APIs"


In JDI ClassLoaderReference.definedClasses()

"loaded at least to the point of preparation and types ..." -> "loaded 
at least to the point of preparation, and types ..." (Note, this not a 
new issue with your edits)



In Instrumentation.getAllLoadedClasses:

The reference to `class` did not format properly.

"by invoking Lookup::defineHiddenClass that creates" -> "by invoking 
Lookup::defineHiddenClass, which 

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

[Mandy Chung]

On 4/16/20 11:42 AM, serguei.spit...@oracle.com wrote:

Hi Mandy,

I have a couple of minor comments on the Serviceability spec update.

http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/src/jdk.jdi/share/classes/com/sun/jdi/ReferenceType.java.udiff.html

 Replace: "The returned name is the same form as ..." => "The returned 
name is of the same form as ..."



Example is:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/src/jdk.jdi/share/classes/com/sun/jdi/Type.java.udiff.html

  + * Returns the name of this type. The result is of the same form as


http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java.udiff.html
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java.udiff.html

Both files above have this: "a `class` file representation".
It feels like something is wrong with the `class` part.
Should it say `class file` or maybe use some other formatting?

Otherwise, the Serviceability spec update looks great.
I'll look at the non-Serviceability spec changes too.


Thanks for catching these typo/format issue.  I have fixed them in my 
local repo:


diff --git 
a/src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java 
b/src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java
--- 
a/src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java
+++ 
b/src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java

@@ -391,7 +391,7 @@
  * 
  * A class or interface creation can be triggered by one of the 
following:

  * 
- * by loading and deriving a class from a `class` file 
representation
+ * by loading and deriving a class from a {@code class} file 
representation

  * using class loader (see JVMS {@jvms 5.3}).
  * by invoking {@link 
java.lang.invoke.MethodHandles.Lookup#defineHiddenClass(byte[], boolean, 
java.lang.invoke.MethodHandles.Lookup.ClassOption...)

  * Lookup::defineHiddenClass} that creates a {@link Class#isHidden
diff --git a/src/jdk.jdi/share/classes/com/sun/jdi/ReferenceType.java 
b/src/jdk.jdi/share/classes/com/sun/jdi/ReferenceType.java

--- a/src/jdk.jdi/share/classes/com/sun/jdi/ReferenceType.java
+++ b/src/jdk.jdi/share/classes/com/sun/jdi/ReferenceType.java
@@ -85,7 +85,7 @@
 {
 /**
  * Returns the name of this {@code ReferenceType} object.
- * The returned name is the same form as the name returned by
+ * The returned name is of the same form as the name returned by
  * {@link Class#getName()}.
  *
  * @return a string containing the type name.
diff --git a/src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java 
b/src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java

--- a/src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java
+++ b/src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java
@@ -137,7 +137,7 @@
  * 
  * A class or interface creation can be triggered by one of the 
following:

  * 
- * by loading and deriving a class from a `class` file 
representation
+ * by loading and deriving a class from a {@code class} file 
representation

  * using class loader (see JVMS {@jvms 5.3}).
  * by invoking {@link 
java.lang.invoke.MethodHandles.Lookup#defineHiddenClass(byte[], boolean, 
java.lang.invoke.MethodHandles.Lookup.ClassOption...)

  * Lookup::defineHiddenClass} that creates a {@link Class#isHidden

Mandy

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

[Mandy Chung]

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  * 
1813  *  {@link Class#getName()} returns the string {@code GN + "/" + 
},
1814  *  even though this is not a valid binary class or interface 
name.
1815  *  {@link Class#descriptorString()} returns the string
1816  *  {@code "L" + N + ";" + "/" +  },
1817  *  even though this is not a valid type descriptor name.
1818  * 

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-inc/
Specdiff without svc spec change:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff-descriptor-string/overview-summary.html

Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-descriptor-string/

Svc spec change webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/

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  wrote:

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

Incremental specdiff:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff-inc/overview-summary.html
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 + ";" + "/" + 

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-delta/

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/

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

[Paul Sandoz]

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  * 
1813  *  {@link Class#getName()} returns the string {@code GN + "/" 
+ },
1814  *  even though this is not a valid binary class or interface 
name.
1815  *  {@link Class#descriptorString()} returns the string
1816  *  {@code "L" + N + ";" + "/" +  },
1817  *  even though this is not a valid type descriptor name.
1818  * 

Add another bullet:

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

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

?

Paul.

> On Apr 11, 2020, at 8:13 PM, Mandy Chung  wrote:
> 
> Please review the delta patch:
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-delta/
> 
> Incremental specdiff:
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff-inc/overview-summary.html
> 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 + ";" + "/" + 
> 
> 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-delta/
>>  
>> 
>> 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-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
>> 
> 

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

[Mandy Chung]

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

Incremental specdiff:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff-inc/overview-summary.html
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 + ";" + "/" + 

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-delta/ 



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

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

[serguei.spit...@oracle.com]

On 4/6/20 11:54, Mandy Chung wrote:

On 4/6/20 9:56 AM, serguei.spit...@oracle.com wrote:


The suggested fix is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-regression-8242166.1/



This patch looks okay. I'll include in my local patch.

On 4/6/20 11:00 AM, Chris Plummer wrote:


I think that's fine but I don't think it should be done in the 
context of this Vahalla webrev since it has nothing to do with 
Vahalla. I'd suggest filing an RFE and pushing it to jdk/jdk. Easier 
to track that way if there are issues down the road.




I am okay to follow up as a separate RFE.


Filed RFE:
  https://bugs.openjdk.java.net/browse/JDK-8242241
    add assert to ClassUnloadEventImpl::className

Thanks,
Serguei



thanks
Mandy

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

[Mandy Chung]

On 4/6/20 9:56 AM, serguei.spit...@oracle.com wrote:


The suggested fix is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-regression-8242166.1/



This patch looks okay. I'll include in my local patch.

On 4/6/20 11:00 AM, Chris Plummer wrote:


I think that's fine but I don't think it should be done in the context 
of this Vahalla webrev since it has nothing to do with Vahalla. I'd 
suggest filing an RFE and pushing it to jdk/jdk. Easier to track that 
way if there are issues down the road.




I am okay to follow up as a separate RFE.

thanks
Mandy

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

[Mandy Chung]

Hi Peter,

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


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.




My bad.  I mixed up with implClass and implInfo cracked from implMethod 
in your question.


implInfo::getDeclaringClass() returns the declaring class of the 
resolved method, which is the superclass (which is what I have been 
thinking for test.LambdaTest).


implClass is the target type of the method reference. 
AbstractValidatingLambdaMetafactory has clear comment:


    final MethodType implMethodType;  // Type of the implMethod 
MethodHandle "(CC,int)String"
    final Class implClass; // Class for referencing 
the implementation method "class CC"



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.


Yes.

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 

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

[Mandy Chung]

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

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

[Peter Levart]

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.

Regards, Peter

On 4/4/20 12:58 PM, Peter Levart wrote:

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);

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

[Peter Levart]

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 

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

[Mandy Chung]

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.


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.


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


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();
    

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

[Peter Levart]

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("")) {
    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 

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

[Peter Levart]

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("")) {
    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. 

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

[David Holmes]

Hi Mandy,

Update seems fine.

Thanks,
David

On 3/04/2020 4:56 am, Mandy Chung wrote:

Hi David,

Thank you for checking thoroughly.   I now grep on src/hotspot and clean 
all of them.


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

On 4/1/20 11:21 PM, David Holmes wrote:

Hi Mandy,

On 2/04/2020 3:17 pm, Mandy Chung wrote:

Hi David,

Thanks for the edits to the comments.   "weak hidden" were missed to 
be changed to "non-strong hidden".  Here is the patch fixing the 
comments.


There are 33 cases of "weak hidden" in the patch I reviewed and also 
some "hidden weak". Should they all be "non-strong hidden" now? 


yes, supposed to.   All should be fixed in webrev.04-delta.


In some cases it also appears in variables and associated logic ie.:

src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp

+  _hidden_weak_classes(NULL), _num_hidden_weak_classes(0),



Are you reading webrev.03?    This has been fixed in webrev.04.

I found Klass::is_hidden_weak that should have been renamed too.


I'm not clear how far this terminology change needs to extend ??



I hope consistently used in the source.


Otherwise patch below seems fine.



Revised the patch.

Thanks
Mandy

Thanks,
David
-


diff --git a/src/hotspot/share/ci/ciField.cpp 
b/src/hotspot/share/ci/ciField.cpp

--- a/src/hotspot/share/ci/ciField.cpp
+++ b/src/hotspot/share/ci/ciField.cpp
@@ -223,8 +223,8 @@
    holder->is_in_package("jdk/internal/foreign") || 
holder->is_in_package("jdk/incubator/foreign") ||

    holder->is_in_package("java/lang"))
  return true;
-  // Trust VM hidden and unsafe anonymous classes. They are created 
via Lookup.defineClass or
-  // the private API (jdk.internal.misc.Unsafe) and can't be 
serialized, so there is no hacking
+  // Trust hidden and VM unsafe anonymous classes. They are created 
via Lookup.defineClass or
+  // the private jdk.internal.misc.Unsafe API and can't be 
serialized, so there is no hacking

    // of finals going on with them.
    if (holder->is_hidden() || holder->is_unsafe_anonymous())
  return true;
diff --git a/src/hotspot/share/ci/ciInstanceKlass.cpp 
b/src/hotspot/share/ci/ciInstanceKlass.cpp

--- a/src/hotspot/share/ci/ciInstanceKlass.cpp
+++ b/src/hotspot/share/ci/ciInstanceKlass.cpp
@@ -76,7 +76,7 @@
    oop holder = ik->klass_holder();
    if (ik->class_loader_data()->has_class_mirror_holder()) {
  // Though ciInstanceKlass records class loader oop, it's not 
enough to keep
-    // VM weak hidden and unsafe anonymous classes alive (loader == 
NULL). Klass holder should
+    // non-strong hidden class and VM unsafe anonymous classes alive 
(loader == NULL). Klass holder should
  // be used instead. It is enough to record a ciObject, since 
cached elements are never removed
  // during ciObjectFactory lifetime. ciObjectFactory itself is 
created for
  // every compilation and lives for the whole duration of the 
compilation.
diff --git a/src/hotspot/share/classfile/classLoaderData.hpp 
b/src/hotspot/share/classfile/classLoaderData.hpp

--- a/src/hotspot/share/classfile/classLoaderData.hpp
+++ b/src/hotspot/share/classfile/classLoaderData.hpp
@@ -127,9 +127,10 @@
    bool _accumulated_modified_oops; // Mod Union Equivalent (CMS 
support)


    int _keep_alive; // if this CLD is kept alive.
-   // Used for weak hidden classes, unsafe 
anonymous classes and the
+   // Used for non-strong hidden classes, 
unsafe anonymous classes and the
 // boot class loader. _keep_alive does 
not need to be volatile or
-   // atomic since there is one unique CLD 
per weak hidden or unsafe anonymous class.
+   // atomic since there is one unique CLD 
per non-strong hidden class

+   // or unsafe anonymous class.

    volatile int _claim; // non-zero if claimed, for example during 
GC traces.
 // To avoid applying oop closure more than 
once.

@@ -242,15 +243,15 @@
    }

    // Returns true if this class loader data is for the system class 
loader.
-  // (Note that the class loader data may be for an weak hidden or 
unsafe anonymous class)
+  // (Note that the class loader data may be for a non-strong hidden 
class or unsafe anonymous class)

    bool is_system_class_loader_data() const;

    // Returns true if this class loader data is for the platform 
class loader.
-  // (Note that the class loader data may be for an weak hidden or 
unsafe anonymous class)
+  // (Note that the class loader data may be for a non-strong hidden 
class or unsafe anonymous class)

    bool is_platform_class_loader_data() const;

    // Returns true if this class loader data is for the boot class 
loader.
-  // (Note that the class loader data may be for an weak hidden 
unsafe anonymous class)
+  // (Note that the class loader data may be for a 

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

[Lois Foltan]

On 4/2/2020 2:56 PM, Mandy Chung wrote:

Hi David,

Thank you for checking thoroughly.   I now grep on src/hotspot and 
clean all of them.


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



Patch looks good Mandy.
Lois



On 4/1/20 11:21 PM, David Holmes wrote:

Hi Mandy,

On 2/04/2020 3:17 pm, Mandy Chung wrote:

Hi David,

Thanks for the edits to the comments.   "weak hidden" were missed to 
be changed to "non-strong hidden".  Here is the patch fixing the 
comments.


There are 33 cases of "weak hidden" in the patch I reviewed and also 
some "hidden weak". Should they all be "non-strong hidden" now? 


yes, supposed to.   All should be fixed in webrev.04-delta.


In some cases it also appears in variables and associated logic ie.:

src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp

+  _hidden_weak_classes(NULL), _num_hidden_weak_classes(0),



Are you reading webrev.03?    This has been fixed in webrev.04.

I found Klass::is_hidden_weak that should have been renamed too.


I'm not clear how far this terminology change needs to extend ??



I hope consistently used in the source.


Otherwise patch below seems fine.



Revised the patch.

Thanks
Mandy

Thanks,
David
-


diff --git a/src/hotspot/share/ci/ciField.cpp 
b/src/hotspot/share/ci/ciField.cpp

--- a/src/hotspot/share/ci/ciField.cpp
+++ b/src/hotspot/share/ci/ciField.cpp
@@ -223,8 +223,8 @@
    holder->is_in_package("jdk/internal/foreign") || 
holder->is_in_package("jdk/incubator/foreign") ||

    holder->is_in_package("java/lang"))
  return true;
-  // Trust VM hidden and unsafe anonymous classes. They are created 
via Lookup.defineClass or
-  // the private API (jdk.internal.misc.Unsafe) and can't be 
serialized, so there is no hacking
+  // Trust hidden and VM unsafe anonymous classes. They are created 
via Lookup.defineClass or
+  // the private jdk.internal.misc.Unsafe API and can't be 
serialized, so there is no hacking

    // of finals going on with them.
    if (holder->is_hidden() || holder->is_unsafe_anonymous())
  return true;
diff --git a/src/hotspot/share/ci/ciInstanceKlass.cpp 
b/src/hotspot/share/ci/ciInstanceKlass.cpp

--- a/src/hotspot/share/ci/ciInstanceKlass.cpp
+++ b/src/hotspot/share/ci/ciInstanceKlass.cpp
@@ -76,7 +76,7 @@
    oop holder = ik->klass_holder();
    if (ik->class_loader_data()->has_class_mirror_holder()) {
  // Though ciInstanceKlass records class loader oop, it's not 
enough to keep
-    // VM weak hidden and unsafe anonymous classes alive (loader == 
NULL). Klass holder should
+    // non-strong hidden class and VM unsafe anonymous classes 
alive (loader == NULL). Klass holder should
  // be used instead. It is enough to record a ciObject, since 
cached elements are never removed
  // during ciObjectFactory lifetime. ciObjectFactory itself is 
created for
  // every compilation and lives for the whole duration of the 
compilation.
diff --git a/src/hotspot/share/classfile/classLoaderData.hpp 
b/src/hotspot/share/classfile/classLoaderData.hpp

--- a/src/hotspot/share/classfile/classLoaderData.hpp
+++ b/src/hotspot/share/classfile/classLoaderData.hpp
@@ -127,9 +127,10 @@
    bool _accumulated_modified_oops; // Mod Union Equivalent (CMS 
support)


    int _keep_alive; // if this CLD is kept alive.
-   // Used for weak hidden classes, unsafe 
anonymous classes and the
+   // Used for non-strong hidden classes, 
unsafe anonymous classes and the
 // boot class loader. _keep_alive does 
not need to be volatile or
-   // atomic since there is one unique CLD 
per weak hidden or unsafe anonymous class.
+   // atomic since there is one unique CLD 
per non-strong hidden class

+   // or unsafe anonymous class.

    volatile int _claim; // non-zero if claimed, for example during 
GC traces.
 // To avoid applying oop closure more than 
once.

@@ -242,15 +243,15 @@
    }

    // Returns true if this class loader data is for the system 
class loader.
-  // (Note that the class loader data may be for an weak hidden or 
unsafe anonymous class)
+  // (Note that the class loader data may be for a non-strong 
hidden class or unsafe anonymous class)

    bool is_system_class_loader_data() const;

    // Returns true if this class loader data is for the platform 
class loader.
-  // (Note that the class loader data may be for an weak hidden or 
unsafe anonymous class)
+  // (Note that the class loader data may be for a non-strong 
hidden class or unsafe anonymous class)

    bool is_platform_class_loader_data() const;

    // Returns true if this class loader data is for the boot class 
loader.
-  // (Note that the class loader data may be for an weak hidden 
unsafe anonymous class)
+  // (Note that the class loader data may be for a non-strong 

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

[Mandy Chung]

Hi David,

Thank you for checking thoroughly.   I now grep on src/hotspot and clean 
all of them.


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

On 4/1/20 11:21 PM, David Holmes wrote:

Hi Mandy,

On 2/04/2020 3:17 pm, Mandy Chung wrote:

Hi David,

Thanks for the edits to the comments.   "weak hidden" were missed to 
be changed to "non-strong hidden".  Here is the patch fixing the 
comments.


There are 33 cases of "weak hidden" in the patch I reviewed and also 
some "hidden weak". Should they all be "non-strong hidden" now? 


yes, supposed to.   All should be fixed in webrev.04-delta.


In some cases it also appears in variables and associated logic ie.:

src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp

+  _hidden_weak_classes(NULL), _num_hidden_weak_classes(0),



Are you reading webrev.03?    This has been fixed in webrev.04.

I found Klass::is_hidden_weak that should have been renamed too.


I'm not clear how far this terminology change needs to extend ??



I hope consistently used in the source.


Otherwise patch below seems fine.



Revised the patch.

Thanks
Mandy

Thanks,
David
-


diff --git a/src/hotspot/share/ci/ciField.cpp 
b/src/hotspot/share/ci/ciField.cpp

--- a/src/hotspot/share/ci/ciField.cpp
+++ b/src/hotspot/share/ci/ciField.cpp
@@ -223,8 +223,8 @@
    holder->is_in_package("jdk/internal/foreign") || 
holder->is_in_package("jdk/incubator/foreign") ||

    holder->is_in_package("java/lang"))
  return true;
-  // Trust VM hidden and unsafe anonymous classes. They are created 
via Lookup.defineClass or
-  // the private API (jdk.internal.misc.Unsafe) and can't be 
serialized, so there is no hacking
+  // Trust hidden and VM unsafe anonymous classes. They are created 
via Lookup.defineClass or
+  // the private jdk.internal.misc.Unsafe API and can't be 
serialized, so there is no hacking

    // of finals going on with them.
    if (holder->is_hidden() || holder->is_unsafe_anonymous())
  return true;
diff --git a/src/hotspot/share/ci/ciInstanceKlass.cpp 
b/src/hotspot/share/ci/ciInstanceKlass.cpp

--- a/src/hotspot/share/ci/ciInstanceKlass.cpp
+++ b/src/hotspot/share/ci/ciInstanceKlass.cpp
@@ -76,7 +76,7 @@
    oop holder = ik->klass_holder();
    if (ik->class_loader_data()->has_class_mirror_holder()) {
  // Though ciInstanceKlass records class loader oop, it's not 
enough to keep
-    // VM weak hidden and unsafe anonymous classes alive (loader == 
NULL). Klass holder should
+    // non-strong hidden class and VM unsafe anonymous classes alive 
(loader == NULL). Klass holder should
  // be used instead. It is enough to record a ciObject, since 
cached elements are never removed
  // during ciObjectFactory lifetime. ciObjectFactory itself is 
created for
  // every compilation and lives for the whole duration of the 
compilation.
diff --git a/src/hotspot/share/classfile/classLoaderData.hpp 
b/src/hotspot/share/classfile/classLoaderData.hpp

--- a/src/hotspot/share/classfile/classLoaderData.hpp
+++ b/src/hotspot/share/classfile/classLoaderData.hpp
@@ -127,9 +127,10 @@
    bool _accumulated_modified_oops; // Mod Union Equivalent (CMS 
support)


    int _keep_alive; // if this CLD is kept alive.
-   // Used for weak hidden classes, unsafe 
anonymous classes and the
+   // Used for non-strong hidden classes, 
unsafe anonymous classes and the
 // boot class loader. _keep_alive does 
not need to be volatile or
-   // atomic since there is one unique CLD 
per weak hidden or unsafe anonymous class.
+   // atomic since there is one unique CLD 
per non-strong hidden class

+   // or unsafe anonymous class.

    volatile int _claim; // non-zero if claimed, for example during 
GC traces.
 // To avoid applying oop closure more than 
once.

@@ -242,15 +243,15 @@
    }

    // Returns true if this class loader data is for the system class 
loader.
-  // (Note that the class loader data may be for an weak hidden or 
unsafe anonymous class)
+  // (Note that the class loader data may be for a non-strong hidden 
class or unsafe anonymous class)

    bool is_system_class_loader_data() const;

    // Returns true if this class loader data is for the platform 
class loader.
-  // (Note that the class loader data may be for an weak hidden or 
unsafe anonymous class)
+  // (Note that the class loader data may be for a non-strong hidden 
class or unsafe anonymous class)

    bool is_platform_class_loader_data() const;

    // Returns true if this class loader data is for the boot class 
loader.
-  // (Note that the class loader data may be for an weak hidden 
unsafe anonymous class)
+  // (Note that the class loader data may be for a non-strong hidden 
class or unsafe anonymous class)

    inline bool 

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

[David Holmes]

Hi Mandy,

On 2/04/2020 3:17 pm, Mandy Chung wrote:

Hi David,

Thanks for the edits to the comments.   "weak hidden" were missed to be 
changed to "non-strong hidden".  Here is the patch fixing the comments.


There are 33 cases of "weak hidden" in the patch I reviewed and also 
some "hidden weak". Should they all be "non-strong hidden" now? In some 
cases it also appears in variables and associated logic ie.:


src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp

+  _hidden_weak_classes(NULL), _num_hidden_weak_classes(0),

I'm not clear how far this terminology change needs to extend ??

Otherwise patch below seems fine.

Thanks,
David
-


diff --git a/src/hotspot/share/ci/ciField.cpp 
b/src/hotspot/share/ci/ciField.cpp

--- a/src/hotspot/share/ci/ciField.cpp
+++ b/src/hotspot/share/ci/ciField.cpp
@@ -223,8 +223,8 @@
    holder->is_in_package("jdk/internal/foreign") || 
holder->is_in_package("jdk/incubator/foreign") ||

    holder->is_in_package("java/lang"))
  return true;
-  // Trust VM hidden and unsafe anonymous classes. They are created via 
Lookup.defineClass or
-  // the private API (jdk.internal.misc.Unsafe) and can't be 
serialized, so there is no hacking
+  // Trust hidden and VM unsafe anonymous classes. They are created via 
Lookup.defineClass or
+  // the private jdk.internal.misc.Unsafe API and can't be serialized, 
so there is no hacking

    // of finals going on with them.
    if (holder->is_hidden() || holder->is_unsafe_anonymous())
  return true;
diff --git a/src/hotspot/share/ci/ciInstanceKlass.cpp 
b/src/hotspot/share/ci/ciInstanceKlass.cpp

--- a/src/hotspot/share/ci/ciInstanceKlass.cpp
+++ b/src/hotspot/share/ci/ciInstanceKlass.cpp
@@ -76,7 +76,7 @@
    oop holder = ik->klass_holder();
    if (ik->class_loader_data()->has_class_mirror_holder()) {
  // Though ciInstanceKlass records class loader oop, it's not 
enough to keep
-    // VM weak hidden and unsafe anonymous classes alive (loader == 
NULL). Klass holder should
+    // non-strong hidden class and VM unsafe anonymous classes alive 
(loader == NULL). Klass holder should
  // be used instead. It is enough to record a ciObject, since 
cached elements are never removed
  // during ciObjectFactory lifetime. ciObjectFactory itself is 
created for
  // every compilation and lives for the whole duration of the 
compilation.
diff --git a/src/hotspot/share/classfile/classLoaderData.hpp 
b/src/hotspot/share/classfile/classLoaderData.hpp

--- a/src/hotspot/share/classfile/classLoaderData.hpp
+++ b/src/hotspot/share/classfile/classLoaderData.hpp
@@ -127,9 +127,10 @@
    bool _accumulated_modified_oops; // Mod Union Equivalent (CMS support)

    int _keep_alive; // if this CLD is kept alive.
-   // Used for weak hidden classes, unsafe 
anonymous classes and the
+   // Used for non-strong hidden classes, 
unsafe anonymous classes and the
     // boot class loader. _keep_alive does not 
need to be volatile or
-   // atomic since there is one unique CLD per 
weak hidden or unsafe anonymous class.
+   // atomic since there is one unique CLD per 
non-strong hidden class

+   // or unsafe anonymous class.

    volatile int _claim; // non-zero if claimed, for example during GC 
traces.

     // To avoid applying oop closure more than once.
@@ -242,15 +243,15 @@
    }

    // Returns true if this class loader data is for the system class 
loader.
-  // (Note that the class loader data may be for an weak hidden or 
unsafe anonymous class)
+  // (Note that the class loader data may be for a non-strong hidden 
class or unsafe anonymous class)

    bool is_system_class_loader_data() const;

    // Returns true if this class loader data is for the platform class 
loader.
-  // (Note that the class loader data may be for an weak hidden or 
unsafe anonymous class)
+  // (Note that the class loader data may be for a non-strong hidden 
class or unsafe anonymous class)

    bool is_platform_class_loader_data() const;

    // Returns true if this class loader data is for the boot class loader.
-  // (Note that the class loader data may be for an weak hidden unsafe 
anonymous class)
+  // (Note that the class loader data may be for a non-strong hidden 
class or unsafe anonymous class)

    inline bool is_boot_class_loader_data() const;

    bool is_builtin_class_loader_data() const;
@@ -271,7 +272,7 @@
  return _unloading;
    }

-  // Used to refcount an weak hidden or unsafe anonymous class's CLD in 
order to
+  // Used to refcount a non-strong hidden class's or unsafe anonymous 
class's CLD in order to

    // indicate their aliveness.
    void inc_keep_alive();
    void dec_keep_alive();

diff --git a/src/hotspot/share/interpreter/linkResolver.cpp 
b/src/hotspot/share/interpreter/linkResolver.cpp

--- 

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

[Mandy Chung]

Hi David,

Thanks for the edits to the comments.   "weak hidden" were missed to be 
changed to "non-strong hidden".  Here is the patch fixing the comments.


diff --git a/src/hotspot/share/ci/ciField.cpp 
b/src/hotspot/share/ci/ciField.cpp

--- a/src/hotspot/share/ci/ciField.cpp
+++ b/src/hotspot/share/ci/ciField.cpp
@@ -223,8 +223,8 @@
   holder->is_in_package("jdk/internal/foreign") || 
holder->is_in_package("jdk/incubator/foreign") ||

   holder->is_in_package("java/lang"))
 return true;
-  // Trust VM hidden and unsafe anonymous classes. They are created via 
Lookup.defineClass or
-  // the private API (jdk.internal.misc.Unsafe) and can't be 
serialized, so there is no hacking
+  // Trust hidden and VM unsafe anonymous classes. They are created via 
Lookup.defineClass or
+  // the private jdk.internal.misc.Unsafe API and can't be serialized, 
so there is no hacking

   // of finals going on with them.
   if (holder->is_hidden() || holder->is_unsafe_anonymous())
 return true;
diff --git a/src/hotspot/share/ci/ciInstanceKlass.cpp 
b/src/hotspot/share/ci/ciInstanceKlass.cpp

--- a/src/hotspot/share/ci/ciInstanceKlass.cpp
+++ b/src/hotspot/share/ci/ciInstanceKlass.cpp
@@ -76,7 +76,7 @@
   oop holder = ik->klass_holder();
   if (ik->class_loader_data()->has_class_mirror_holder()) {
 // Though ciInstanceKlass records class loader oop, it's not 
enough to keep
-    // VM weak hidden and unsafe anonymous classes alive (loader == 
NULL). Klass holder should
+    // non-strong hidden class and VM unsafe anonymous classes alive 
(loader == NULL). Klass holder should
 // be used instead. It is enough to record a ciObject, since 
cached elements are never removed
 // during ciObjectFactory lifetime. ciObjectFactory itself is 
created for
 // every compilation and lives for the whole duration of the 
compilation.
diff --git a/src/hotspot/share/classfile/classLoaderData.hpp 
b/src/hotspot/share/classfile/classLoaderData.hpp

--- a/src/hotspot/share/classfile/classLoaderData.hpp
+++ b/src/hotspot/share/classfile/classLoaderData.hpp
@@ -127,9 +127,10 @@
   bool _accumulated_modified_oops; // Mod Union Equivalent (CMS support)

   int _keep_alive; // if this CLD is kept alive.
-   // Used for weak hidden classes, unsafe 
anonymous classes and the
+   // Used for non-strong hidden classes, 
unsafe anonymous classes and the
    // boot class loader. _keep_alive does not 
need to be volatile or
-   // atomic since there is one unique CLD per 
weak hidden or unsafe anonymous class.
+   // atomic since there is one unique CLD per 
non-strong hidden class

+   // or unsafe anonymous class.

   volatile int _claim; // non-zero if claimed, for example during GC 
traces.

    // To avoid applying oop closure more than once.
@@ -242,15 +243,15 @@
   }

   // Returns true if this class loader data is for the system class 
loader.
-  // (Note that the class loader data may be for an weak hidden or 
unsafe anonymous class)
+  // (Note that the class loader data may be for a non-strong hidden 
class or unsafe anonymous class)

   bool is_system_class_loader_data() const;

   // Returns true if this class loader data is for the platform class 
loader.
-  // (Note that the class loader data may be for an weak hidden or 
unsafe anonymous class)
+  // (Note that the class loader data may be for a non-strong hidden 
class or unsafe anonymous class)

   bool is_platform_class_loader_data() const;

   // Returns true if this class loader data is for the boot class loader.
-  // (Note that the class loader data may be for an weak hidden unsafe 
anonymous class)
+  // (Note that the class loader data may be for a non-strong hidden 
class or unsafe anonymous class)

   inline bool is_boot_class_loader_data() const;

   bool is_builtin_class_loader_data() const;
@@ -271,7 +272,7 @@
 return _unloading;
   }

-  // Used to refcount an weak hidden or unsafe anonymous class's CLD in 
order to
+  // Used to refcount a non-strong hidden class's or unsafe anonymous 
class's CLD in order to

   // indicate their aliveness.
   void inc_keep_alive();
   void dec_keep_alive();

diff --git a/src/hotspot/share/interpreter/linkResolver.cpp 
b/src/hotspot/share/interpreter/linkResolver.cpp

--- a/src/hotspot/share/interpreter/linkResolver.cpp
+++ b/src/hotspot/share/interpreter/linkResolver.cpp
@@ -611,7 +611,7 @@
  (same_module) ? "" : sel_klass->class_in_module_of_loader()
  );

-    // For private access check if there was a problem with nest host
+    // For private access see if there was a problem with nest host
 // resolution, and if so report that as part of the message.
 if (sel_method->is_private()) {
   print_nest_host_error_on(, ref_klass, sel_klass, THREAD);
@@ -951,7 +951,7 @@
  (same_module) 

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

[David Holmes]

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-delta/

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

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

[Mandy Chung]

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

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

[Alan Bateman]

On 31/03/2020 20:25, Mandy Chung wrote:
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`.
I went through the java.base changes in webrev.03-delta. The rename to 
isHidden and the updated javadoc looks good. There are a few additional 
drive-by updates to the javadoc, including isSynthetic, they all look 
good too.


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?


-Alan.

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

[Mandy Chung]

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-delta/

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

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

[Mandy Chung]

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-delta-jdarcy/ 



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

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

[Mandy Chung]

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

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

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

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

[Mandy Chung]

This is the patch to keep the JDK 14 behavior if target release to 14 
(thanks to Jan for helping making change in javac to get the tests working)

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

Mandy

On 3/27/20 9:29 AM, 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

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

[serguei.spit...@oracle.com]

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

Hi Mandy,

I have just one comment so far.

http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp.frames.html 



 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:

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

[coleen . phillimore]

On 3/30/20 5:54 AM, David Holmes wrote:
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.spit...@oracle.com wrote:

Hi Mandy,

I have just one comment so far.

http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp.frames.html 



  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?


has_class_mirror_holder() implies weak.

Coleen


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

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

[David Holmes]

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.spit...@oracle.com wrote:

Hi Mandy,

I have just one comment so far.

http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp.frames.html 



  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 

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

[serguei.spit...@oracle.com]

Hi Mandy,

I have just one comment so far.

http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp.frames.html

 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?


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-JVMS-HiddenClasses.pdf 



CSR:
https://bugs.openjdk.java.net/browse/JDK-8238359

Thanks
Mandy
[1] 

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

[serguei.spit...@oracle.com]

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 

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

[Mandy Chung]

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.


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

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


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

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

[Chris Plummer]

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.


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.


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.


I still need to look over the JVMTI tests.

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 

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

[Dean Long]

I looked at the AOT, C2, and JVMCI changes and I didn't find any issues.

dl

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

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

[Paul Sandoz]

+1
Paul.

> On Mar 27, 2020, at 3:22 PM, Mandy Chung  wrote:
> 
> Hi Paul,
> 
> This is the delta incorporating your comment:
>   
> 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

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

[David Holmes]

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-target-release-14/index.html 



+ /**
+  * 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

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

[Mandy Chung]

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-target-release-14/index.html 



+ /**
+  * 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.

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.


Mandy

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

[Remi Forax]

- Mail original -
> De: "David Holmes" 
> À: "mandy chung" , "Vicente Romero" 
> , "jan lahoda"
> 
> Cc: "serviceability-dev" , "hotspot-dev" 
> ,
> "core-libs-dev" , "valhalla-dev" 
> 
> 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-target-release-14/index.html
> 
> + /**
> +  * 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
>>>>&

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

[David Holmes]

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-target-release-14/index.html 


+ /**
+  * 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.)


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 

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

[Vicente Romero]

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-target-release-14/index.html


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 

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

[Mandy Chung]

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-target-release-14/index.html

(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

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

[Mandy Chung]

Hi Paul,

This is the delta incorporating your comment:
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

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

[Vicente Romero]

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

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

[Mandy Chung]

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= 0x0001,
  148 HIDDEN_CLASS  = 0x0002,
  149 STRONG_LOADER_LINK= 0x0004,
  150 ACCESS_VM_ANNOTATIONS = 0x0008;
  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 opts = options.length > 0 ? Set.of(options) : 
Set.of();

You can just do:

   Set 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 
strong

“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

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

[Paul Sandoz]

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= 0x0001,
 148 HIDDEN_CLASS  = 0x0002,
 149 STRONG_LOADER_LINK= 0x0004,
 150 ACCESS_VM_ANNOTATIONS = 0x0008;
 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 opts = options.length > 0 ? Set.of(options) : 
Set.of();

You can just do:

  Set 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 
strong

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

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

[Mandy Chung]

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

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

[forax]

> De: "mandy chung" 
> À: "Remi Forax" 
> Cc: "valhalla-dev" , "core-libs-dev"
> , "serviceability-dev"
> , "hotspot-dev"
> 
> Envoyé: Vendredi 27 Mars 2020 16:50:55
> Objet: Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

> On 3/27/20 5:00 AM, Remi Forax wrote:

>> Hi Mandy,
>> in ReflectionFactory, why in the case of a constructor the check to the
>> anonymous class is removed ?

> Good catch. Fixed

>> in BytecodeGenerator, the comment "// bootstrapping issue if using condy"
>> can be promoted on top of clinit, because i ask myself the same question 
>> seeing
>> a static block was generated

> OK, that's clearer.

>> in AbstractValidatingLambdaMetafactory.java, the field caller is not used 
>> after
>> all ?

> Thanks. Removed. It was left behind from an early prototype.

> Below is the patch. I will send out a new webrev and delta webrev in the next
> revision.
Thanks Mandy, 
Looks good. 

Rémi 

> thanks
> Mandy

> diff --git
> a/src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java
> b/src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java
> ---
> a/src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java
> +++
> b/src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java
> @@ -51,7 +51,6 @@
> * System.out.printf(">>> %s\n", iii.foo(44));
> * }}
> */
> - final MethodHandles.Lookup caller;
> final Class targetClass; // The class calling the meta-factory via
> invokedynamic "class X"
> final MethodType invokedType; // The type of the invoked method "(CC)II"
> final Class samBase; // The type of the returned instance "interface JJ"
> @@ -121,7 +120,6 @@
> "Invalid caller: %s",
> caller.lookupClass().getName()));
> }
> - this.caller = caller;
> this.targetClass = caller.lookupClass();
> this.invokedType = invokedType;

> diff --git
> a/src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
> b/src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
> --- 
> a/src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
> +++ 
> b/src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
> @@ -363,6 +363,10 @@
> clinit(cw, className(), classData);
> }

> + /*
> + *  to initialize the static final fields with the live class data
> + * LambdaForms can't use condy due to bootstrapping issue.
> + */
> static void clinit(ClassWriter cw, String className, List 
> classData)
> {
> if (classData.isEmpty())
> return;
> @@ -375,7 +379,6 @@

> MethodVisitor mv = cw.visitMethod(Opcodes.ACC_STATIC, "", "()V", null,
> null);
> mv.visitCode();
> - // bootstrapping issue if using condy
> mv.visitLdcInsn(Type.getType("L" + className + ";"));
> mv.visitMethodInsn(Opcodes.INVOKESTATIC, 
> "java/lang/invoke/MethodHandleNatives",
> "classData", "(Ljava/lang/Class;)Ljava/lang/Object;", false);
> diff --git
> a/src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java
> b/src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java
> --- a/src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java
> +++ b/src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java
> @@ -245,7 +245,8 @@
> return new BootstrapConstructorAccessorImpl(c);
> }

> - if (noInflation && !c.getDeclaringClass().isHiddenClass()) {
> + if (noInflation && !c.getDeclaringClass().isHiddenClass()
> + && !ReflectUtil.isVMAnonymousClass(c.getDeclaringClass())) {
> return new MethodAccessorGenerator().
> generateConstructor(c.getDeclaringClass(),
> c.getParameterTypes(),

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

[Mandy Chung]

On 3/27/20 5:00 AM, Remi Forax wrote:

Hi Mandy,
in ReflectionFactory, why in the case of a constructor the check to the 
anonymous class is removed ?


Good catch.  Fixed


in BytecodeGenerator, the comment "// bootstrapping issue if using condy"
can be promoted on top of clinit, because i ask myself the same question seeing 
a static block was generated


OK, that's clearer.


in AbstractValidatingLambdaMetafactory.java, the field caller is not used after 
all ?


Thanks.  Removed.  It was left behind from an early prototype.

Below is the patch.  I will send out a new webrev and delta webrev in 
the next revision.


thanks
Mandy

diff --git 
a/src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java 
b/src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java
--- 
a/src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java
+++ 
b/src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java

@@ -51,7 +51,6 @@
  * System.out.printf(">>> %s\n", iii.foo(44));
  * }}
  */
-    final MethodHandles.Lookup caller;
 final Class targetClass;   // The class calling the 
meta-factory via invokedynamic "class X"
 final MethodType invokedType; // The type of the 
invoked method "(CC)II"
 final Class samBase;   // The type of the 
returned instance "interface JJ"

@@ -121,7 +120,6 @@
 "Invalid caller: %s",
 caller.lookupClass().getName()));
 }
-    this.caller = caller;
 this.targetClass = caller.lookupClass();
 this.invokedType = invokedType;

diff --git 
a/src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java 
b/src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
--- 
a/src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
+++ 
b/src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java

@@ -363,6 +363,10 @@
 clinit(cw, className(), classData);
 }

+    /*
+ *  to initialize the static final fields with the live 
class data

+ * LambdaForms can't use condy due to bootstrapping issue.
+ */
 static void clinit(ClassWriter cw, String className, 
List classData) {

 if (classData.isEmpty())
 return;
@@ -375,7 +379,6 @@

 MethodVisitor mv = cw.visitMethod(Opcodes.ACC_STATIC, 
"", "()V", null, null);

 mv.visitCode();
-    // bootstrapping issue if using condy
 mv.visitLdcInsn(Type.getType("L" + className + ";"));
 mv.visitMethodInsn(Opcodes.INVOKESTATIC, 
"java/lang/invoke/MethodHandleNatives",
    "classData", 
"(Ljava/lang/Class;)Ljava/lang/Object;", false);
diff --git 
a/src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java 
b/src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java
--- 
a/src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java
+++ 
b/src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java

@@ -245,7 +245,8 @@
 return new BootstrapConstructorAccessorImpl(c);
 }

-    if (noInflation && !c.getDeclaringClass().isHiddenClass()) {
+    if (noInflation && !c.getDeclaringClass().isHiddenClass()
+    && 
!ReflectUtil.isVMAnonymousClass(c.getDeclaringClass())) {

 return new MethodAccessorGenerator().
 generateConstructor(c.getDeclaringClass(),
 c.getParameterTypes(),

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

[Remi Forax]

Hi Mandy,
in ReflectionFactory, why in the case of a constructor the check to the 
anonymous class is removed ?

in BytecodeGenerator, the comment "// bootstrapping issue if using condy"
can be promoted on top of clinit, because i ask myself the same question seeing 
a static block was generated

in AbstractValidatingLambdaMetafactory.java, the field caller is not used after 
all ?

regards,
Rémi

- Mail original -
> De: "mandy chung" 
> À: "valhalla-dev" , "core-libs-dev" 
> ,
> "serviceability-dev" , "hotspot-dev" 
> 
> Envoyé: Vendredi 27 Mars 2020 00:57:39
> Objet: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

> 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

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

[Jan Lahoda]

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