Review Request: 8238358: Implementation of JEP 371: Hidden Classes
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Mon Mar 30 15:23:19 UTC 2020
Adding back hotspot-dev.
On 3/30/20 11:02 AM, coleen.phillimore at oracle.com wrote:
>
> Hi, This is great work! I did a prereview and all of my comments
> were addressed. These are a few minor things I noticed.
>
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/ci/ciInstanceKlass.hpp.udiff.html
>
> Nit. Can you add 'const' to the is_hidden accessor?
>
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/classFileParser.cpp.udiff.html
>
> + ID annotation_index(const ClassLoaderData* loader_data, const
> Symbol* name, const bool can_access_vm_annotations);
>
> 'const' bool is weird and unnecessary. Can you remove const here?
>
> + if (is_hidden()) { // Mark methods in hidden classes as 'hidden'.
> + m->set_hidden(true);
> + }
> +
> Could be:
>
> + // Mark methods in hidden classes as 'hidden'.
> + m->set_hidden(is_hidden());
> +
>
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/javaClasses.cpp.udiff.html
>
> + macro(_classData_offset, k, "classData", object_signature, false); \
>
> Probably should remove trailing backslash here.
>
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/systemDictionary.cpp.udiff.html
>
> I think in a future RFE, we should add a default parameter to
> register_loader to make the code in the beginning of parse_stream()
> cleaner and remove has_class_mirror_holder_cld().
>
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/prims/jvm.cpp.udiff.html
> + jboolean is_nestmate = (flags & NESTMATE) == NESTMATE;
> + jboolean is_hidden = (flags & HIDDEN_CLASS) == HIDDEN_CLASS;
> + jboolean is_strong = (flags & STRONG_LOADER_LINK) == STRONG_LOADER_LINK;
> + jboolean vm_annotations = (flags & ACCESS_VM_ANNOTATIONS) ==
> ACCESS_VM_ANNOTATION
>
> Instead of jboolean, please use C++ bool here.
>
> + oop loader = lookup_k->class_loader();
> + Handle class_loader (THREAD, loader);
> Can you rewrite as this to prevent potential unhandled oop for oop loader.
> + Handle class_loader (THREAD, lookup_k->class_loader());
>
> Here:
> + InstanceKlass::cast(defined_k)->class_loader_data()->dec_keep_alive();
>
> Don't have to cast defined_k to get class_loader_data(), but you
> probably just want to move this up to remove the rest of the
> InstanceKlass::cast().
>
> + InstanceKlass* ik = InstanceKlass::cast(defined_k);
>
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/runtime/vmStructs.cpp.udiff.html
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/classfile/ClassLoaderData.java.udiff.html
>
> We agreed already that these changes aren't needed by the SA. You can
> revert these.
>
> These are minor changes. I don't need to see another webrev.
>
> Thanks,
> Coleen
>
>
>
> On 3/26/20 7: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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20200330/fabe74c8/attachment-0001.htm>
More information about the serviceability-dev
mailing list