RFR: JDK-8225056 VM support for sealed classes
Harold Seigel
harold.seigel at oracle.com
Thu May 21 18:33:07 UTC 2020
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
>>
>>
More information about the core-libs-dev
mailing list