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

[serguei.spitsyn at 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 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
>>
>>
>