RFR: JDK-8225056 VM support for sealed classes
Harold Seigel
harold.seigel at oracle.com
Wed May 27 20:35:47 UTC 2020
Hi David,
Please review this updated webrev:
Incremental webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
It includes the following changes:
* Indentation and simplification changes as suggested below
* If a class is not a valid permitted subclass of its sealed super
then an IncompatibleClassChangeError exception is thrown (as
specified in the JVM Spec) instead of VerifyError.
* Added a check that a non-public subclass must be in the same package
as its sealed super. And added appropriate testing.
* Method Class.permittedSubtypes() was changed.
See also inline comments.
On 5/24/2020 10:28 PM, David Holmes wrote:
> Hi Harold,
>
> On 22/05/2020 4:33 am, Harold Seigel wrote:
>> Hi David,
>>
>> Thanks for looking at this! Please review this new webrev:
>>
>> http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
>
> I'll list all relevant commens here rather than interspersing below so
> that it is easier to track. Mostly nits below, other than the
> is_permitted_subclass check in the VM, and the use of ReflectionData
> in java.lang.Class.
>
> --
>
> src/hotspot/share/classfile/classFileParser.cpp
>
> + bool ClassFileParser::supports_sealed_types() {
> + return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
> + _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
> + Arguments::enable_preview();
> + }
>
> Nowe there is too little indentation - the subclauses of the
> conjunction expression should align[1]
>
> + bool ClassFileParser::supports_sealed_types() {
> + return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
> + _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
> + Arguments::enable_preview();
> + }
Fixed. Along with indentation of supports_records().
>
> 3791 if (parsed_permitted_subclasses_attribute) {
> 3792 classfile_parse_error("Multiple
> PermittedSubclasses attributes in class file %s", CHECK);
> 3793 // Classes marked ACC_FINAL cannot have a
> PermittedSubclasses attribute.
> 3794 } else if (_access_flags.is_final()) {
> 3795 classfile_parse_error("PermittedSubclasses
> attribute in final class file %s", CHECK);
> 3796 } else {
> 3797 parsed_permitted_subclasses_attribute = true;
> 3798 }
>
> The indent of the comment at L3793 is wrong, and its placement is
> awkward because it relates to the next condition. But we don't have to
> use if-else here as any parse error results in immediate return due to
> the CHECK macro. So the above can be reformatted as:
>
> 3791 if (parsed_permitted_subclasses_attribute) {
> 3792 classfile_parse_error("Multiple
> PermittedSubclasses attributes in class file %s", CHECK);
> 3793 }
> 3794 // Classes marked ACC_FINAL cannot have a
> PermittedSubclasses attribute.
> 3795 if (_access_flags.is_final()) {
> 3796 classfile_parse_error("PermittedSubclasses
> attribute in final class file %s", CHECK);
> 3797 }
> 3798 parsed_permitted_subclasses_attribute = true;
Done.
>
> ---
>
> src/hotspot/share/oops/instanceKlass.cpp
>
> The logic in InstanceKlass::has_as_permitted_subclass still does not
> implement the rules specified in the JVMS. It only implements a "same
> module" check, whereas the JVMS specifies an accessibility requirement
> as well.
Done.
>
> 730 bool InstanceKlass::is_sealed() const {
> 731 return _permitted_subclasses != NULL &&
> 732 _permitted_subclasses !=
> Universe::the_empty_short_array() &&
> 733 _permitted_subclasses->length() > 0;
> 734 }
>
> Please align subclauses.
Done.
>
> ---
>
> src/hotspot/share/prims/jvm.cpp
>
> 2159 objArrayHandle result (THREAD, r);
>
> Please remove space after "result".
Done.
>
> As we will always create and return an arry, if you reverse these two
> statements:
>
> 2156 if (length != 0) {
> 2157 objArrayOop r =
> oopFactory::new_objArray(SystemDictionary::String_klass(),
> 2158 length, CHECK_NULL);
>
> and these two:
>
> 2169 return (jobjectArray)JNIHandles::make_local(THREAD, result());
> 2170 }
>
> then you can delete
>
> 2172 // if it gets to here return an empty array, cases will be: the
> class is primitive, or an array, or just not sealed
> 2173 objArrayOop result =
> oopFactory::new_objArray(SystemDictionary::String_klass(), 0,
> CHECK_NULL);
> 2174 return (jobjectArray)JNIHandles::make_local(env, result);
>
> The comment there is no longer accurate anyway.
Done.
>
> ---
>
> src/hotspot/share/prims/jvmtiRedefineClasses.cpp
>
> 857 static jvmtiError
> check_permitted_subclasses_attribute(InstanceKlass* the_class,
> 858 InstanceKlass* scratch_class) {
>
> Please align.
Done.
>
> ---
>
> src/hotspot/share/prims/jvmtiRedefineClasses.cpp
>
> 2007 if (permitted_subclasses != NULL) {
>
> permitted_subclasses cannot be NULL. I initially thought the bug was
> in the nest_members version of this code, but they both have the same
> properties: the member is initialized to NULL when the InstanceKlass
> is constructed, and set to either the proper array or the
> empty_array() when classfile parsing is complete. So redefinition
> cannot encounter a NULL value here.
Changed the 'if' statement to an assert.
>
> ---
>
> src/java.base/share/classes/java/lang/Class.java
>
> The use of ReflectionData is not correctly implemented. The
> ReflectionData instance is not constant but can be replaced when class
> redefinition operates. So you cannot do this:
>
> if (rd.permittedSubclasses != null) {
> return rd.permittedSubclasses;
> }
>
> because you may be returning the permittedSubclasses field of a
> different Reflectiondata instance. You need to read the field once
> into a local and thereafter use it. Similarly with:
>
> rd.permittedSubclasses = new ClassDesc[0];
> return rd.permittedSubclasses;
>
> you need to do:
>
> temp = new ClassDesc[0];
> rd.permittedSubclasses = temp;
> return temp;
Done.
>
> I'm wondering now whether using Reflectiondata as the cache for this
> is actually the best way to cache it?
Perhaps Reflectiondata could be used now and an RFE could be filed to
look at this more closely?
Thanks, Harold
>
> Aside: The JEP should explicitly point out that because the
> sealed/non-sealed modifiers are not represented in the classfile
> directly, they are also not exposed via the java.lang.reflect.Modifier
> API.
>
> ---
>
> That's it form me.
>
> Thanks,
> David
>
> [1] https://wiki.openjdk.java.net/display/HotSpot/StyleGuide
> "Use good taste to break lines and align corresponding tokens on
> adjacent lines."
>
>> 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