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
