Review Request: 8238358: Implementation of JEP 371: Hidden Classes
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Mon Mar 30 14:20:01 UTC 2020
Adding back serviceability-dev. Sometimes reply (and myself) remembers
it and sometimes it strips it off....
Coleen
On 3/30/20 10:16 AM, coleen.phillimore at oracle.com wrote:
>
>
> 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
>>>
>>>
>>
>
More information about the serviceability-dev
mailing list