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

[coleen.phillimore at oracle.com]

On 3/29/20 10:17 PM, 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 may have written the last part of that comment (or remember it at 
least).  I think Chris's suggestion to remove the last sentence makes 
sense.  Anything further will just adds unnecessary confusion to the 
reader.  Anyone modifying this will get the InstanceKlass::cast() assert 
soon after if they mess up.

Coleen

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