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