Hi Harold,
Adding serviceability-dev for the serviceability related changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included here
so when and where will they be reviewed and under what bug id? Ideally
there will be a single JBS issue for "Implementation of JEP 360:
Sealed types". It's okay to break up the RFRs across different areas,
but it should be done under one bug id.
Overall this looks good. I've looked at all files and mainly have some
style nits in various places. But there are some more significant
items below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for the JEP
360 Sealed Classes preview feature. Code changes include the following:
* Processing of the new PermittedSubclasses attribute to enforce that
a class or interface, whose super is a sealed class or interface,
must be listed in the super's PermittedSubclasses attribute.
* Disallow redefinition of a sealed class or interface if its
redefinition would change its PermittedSubclasses attribute.
* Support API's to determine if a class or interface is sealed and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But also
see below ...
Open Webrev:
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for this JDWP
spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong place.
It needs to be before
3788 if (tag == vmSymbols::tag_permitted_subclasses()) {
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute(
3878 cfs,
3879 permitted_subclasses_attribute_start,
3880 CHECK);
Although it looks odd the preceding, similarly shaped, sections all
indent to the same absolute position. Can you make L3878/78/80 match
please.
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884 sizeof(num_of_subclasses) + sizeof(u2) *
num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class file
%s", CHECK);
Nits: please reformat as:
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
sizeof(num_of_subclasses) + sizeof(u2) * num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class file
%s", CHECK);
It would also look slightly better if you shortened the name of the
num_of_subclasses variable.
---
src/hotspot/share/classfile/classFileParser.hpp
+ u2 parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
+ const u1* const
permitted_subclasses_attribute_start,
+ TRAPS);
Please fix indentation after copy'n'edit.
---
src/hotspot/share/oops/instanceKlass.cpp
247 if (classloader1 != classloader2) {
I'm not clear what rule this is verifying. The same module check
follows this one. The rule is that the subclass must be accessible to
the superclass implying:
1. same named module (regardless of class access modifiers); or
2. (implicitly in un-named module) same package if subclass not
public; or
3. public subclass
Having the same classloader implies same package, but that alone
doesn't address 2 or 3. So this doesn't conform to proposed JVMS rules.
264 if (_constants->tag_at(cp_index).is_klass()) {
265 Klass* k2 = _constants->klass_at(cp_index, CHECK_false);
You've copied this code from the nestmember checks but your changes
don't quite make sense to me. If we have already checked is_klass()
then klass_at() cannot lead to any exceptions.
272 if (name == k->name()) {
273 log_trace(class, sealed)("- Found it at
permitted_subclasses[%d] => cp[%d]", i, cp_index);
274 return true;
I was wondering why you don't resolve the cp entry when you find the
name matches, as we do for nest members, but realized that unlike the
nest membership check, which can happen many times for a given class,
this permitted subclass check can only happen once per class. As you
don't actually resolve here, and given that the earlier check cannot
throw exceptions, it follows that the entire method never results in
any exceptions and so callers should not be using the CHECK macro.
---
src/hotspot/share/oops/method.cpp
I don't understand how knowing the class is sealed allows you to infer
that a non-final method is actually final ??
---
src/hotspot/share/prims/jvm.cpp
It would be simpler (and cheaper) if the Java side of this ensures it
doesn't call into the VM with an array or primitive class.
---
src/hotspot/share/prims/jvmti.xml
The JVM TI spec changes also need to be covered by a CSR request.
---
src/hotspot/share/prims/jvmtiRedefineClasses.cpp
We should file a RFE to refactor the logic that checks that an
attribute consisting of a list of classes has not changed. :)
Aside: I spotted a bug in the nest member code (missing NULL check!)
thanks to your change :)
---
src/java.base/share/classes/java/lang/Class.java
There needs to be a CSR request for these changes.
+ * Returns an array containing {@code ClassDesc} objects
representing all the
+ * permitted subclasses of this {@linkplain Class} if it is
sealed. Returns an empty array if this
+ * {@linkplain Class} is not sealed.
should add "or this class represents an array or primitive type"
(using the standard wording for such cases).
+ * @throws IllegalArgumentException if a class descriptor is not
in the correct format
IllegalArgumentException is not an appropriate exception to use as
this method takes no arguments. If the class descriptor is not valid
and it comes from the VM then I think we have a problem with how the
VM validates class descriptors. Any IAE from ClassDesc.of should be
caught and converted to a more suitable exception type - preferably
InternalError if the VM should always return valid strings.
+ public ClassDesc[] getPermittedSubclasses() {
As mentioned for jvm.cpp this Java code should do the isArray() and
isPrimitive() check before calling the VM.
+ String[] descriptors = getPermittedSubclasses0();
Nit: what you get from the VM are not descriptors, just name strings
in internal form. This wouldn't really matter except it then looks
strange to call ClassDesc.of(...) instead of ClassDesc.ofDescriptor(...).
+ if (descriptors == null
The VM never returns null.
+ return getPermittedSubclasses().length != 0;
It's grossly inefficient to create the ClassDesc array and then throw
it away IMO. The result should be cached either in a field of Class or
in the ReflectionData of the class.
---
src/java.base/share/classes/jdk/internal/org/objectweb/asm/ClassReader.java
! // - The offset of the PermittedSubclasses attribute, or 0
int permittedSubtypesOffset = 0;
Obviously ASM already has some prelim support for sealed classes, but
now that the attribute has been renamed that should also flow through
to the ASM code ie the variable, not just the comment.
Ditto for ClassWriter.java and its fields.
---
src/java.base/share/native/libjava/Class.c
{"isRecord0", "()Z", (void *)&JVM_IsRecord},
+ {"getPermittedSubclasses0", "()[" STR, (void
*)&JVM_GetPermittedSubclasses},
please align (void
---
src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java
src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java
There needs to be a CSR for these changes too.
---
test/langtools/tools/javac/processing/model/TestSourceVersion.java
! // Assume "record" and "sealed" will be
restricted keywords.
! "record", "sealed");
What about the non-sealed keyword defined in the JEP?
---
In the tests you don't need to explicitly include
sun.hotspot.WhiteBox$WhiteBoxPermission on the ClassFileInstaller
invocation. (previous RFE's have been removing existing occurrences
after the CFI was fixed to handle it internally).
Please ensure all new tests have an @bug 8225056 (or whatever the
actual JBS issue will be)
All test classes (and thus files) should be named in camel-case i.e.
C1 not c1, C2 not c2, SuperClass not superClass etc.
test/hotspot/jtreg/runtime/modules/sealedP1/superClass.jcod
test/hotspot/jtreg/runtime/sealedClasses/Pkg/sealedInterface.jcod
Please add comments clarifying why these must be jcod files.
---
That's it from me.
Thanks,
David
-----
JBS bug: https://bugs.openjdk.java.net/browse/JDK-8225056
Java Language Spec changes:
http://cr.openjdk.java.net/~gbierman/jep360/jep360-20200513/specs/sealed-classes-jls.html
JVM Spec changes:
http://cr.openjdk.java.net/~gbierman/jep360/jep360-20200513/specs/sealed-classes-jvms.html
JEP 360: https://bugs.openjdk.java.net/browse/JDK-8227043
JVM CSR: https://bugs.openjdk.java.net/browse/JDK-8242578
Changes to javac and other language tools will be reviewed separately.
Thanks, Harold
