Hi David,
Thanks for looking at this! Please review this new webrev:
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
This webrev contains the following significant changes:
1. The format/indentation issues in classFileParser.cpp were fixed.
2. Unneeded checks in InstanceKlass::has_as_permitted_subclass() were
removed and the TRAPS parameter was removed.
3. The changes to klassVtable.* and method.* were reverted. Those
changes were from when sealed classes were marked as final, and are
no longer valid.
4. Method getPermittedSubclasses() in Class.java was renamed to
permittedSubclasses() and its implementation was changed.
5. Variables and methods for 'asm' were renamed from
'permittedSubtypes' to 'permittedSubclasses'.
6. Classes for sealed classes tests were changed to start with capital
letters.
7. Changes to langtools tests were removed from this webrev. They are
covered by the javac webrev (JDK-8244556).
8. The CSR's for JVMTI, JDWP, and JDI are in progress.
Please also see comments inline.
On 5/19/2020 1:26 AM, David Holmes wrote:
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.
The javac changes are being reviewed as bug JDK-8227406. We understand
the need to do the reviews under one bug.
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.
All of the above classFileParser.cpp changes were done.
---
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.
Done.
---
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.
This was discussed as part of the CSR and hopefully clarified.
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.
The comparison of class loaders was removed because checking that the
two classes are in the same module ensures that they have the same class
loader.
The traps parameter was removed. The CHECK macro was replaced with THREAD.
---
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 ??
This change was removed. See item #3 at the beginning of this email.
---
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.
Done.
---
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. :)
Serguei filed the RFE.
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.
The CSR is JDK-8244556.
+ * 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).
Discussed off-line and was decided that this text isn't needed.
+ * @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.
Done.
+ public ClassDesc[] getPermittedSubclasses() {
As mentioned for jvm.cpp this Java code should do the isArray() and
isPrimitive() check before calling the VM.
Done.
+ 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(...).
We tried using ClassDesc.ofDescriptor() but encountered problems. The
variable 'descriptors' was renamed 'subclassNames'.
+ if (descriptors == null
The VM never returns null.
The check was removed.
+ 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.
Done.
---
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.
Done.
---
src/java.base/share/native/libjava/Class.c
{"isRecord0", "()Z", (void *)&JVM_IsRecord},
+ {"getPermittedSubclasses0", "()[" STR, (void
*)&JVM_GetPermittedSubclasses},
please align (void
Done.
---
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?
'non-sealed' is a keyword but not a restricted keyword. So, it should
not be in the list.
---
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).
Done.
Please ensure all new tests have an @bug 8225056 (or whatever the
actual JBS issue will be)
Done.
All test classes (and thus files) should be named in camel-case i.e.
C1 not c1, C2 not c2, SuperClass not superClass etc.
Done.
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.
Done.
Thanks! Harold
---
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
