RFR: JDK-8225056 VM support for sealed classes
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue May 19 15:53:54 UTC 2020
Hi Harold,
The Serviceability part including the tests looks good to me.
I can file a JVMTI enhancement on the jvmtiRedefineClasses.cpp
refactoring if you are okay with it.
Thanks,
Serguei
On 5/18/20 22:26, 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.
>
> 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
>>
>>
More information about the amber-dev
mailing list