RFR: JDK-8225056 VM support for sealed classes
David Holmes
david.holmes at oracle.com
Thu May 28 06:51:47 UTC 2020
Hi Harold,
On 28/05/2020 6:35 am, Harold Seigel wrote:
> Hi David,
>
> Please review this updated webrev:
>
> Incremental webrev:
> http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
Changes look good - thanks!
One minor comment:
src/hotspot/share/prims/jvm.cpp
2159 if (length != 0) {
2160 for (int i = 0; i < length; i++) {
The if statement is not needed as the loop will be skipped if length is 0.
On testing:
test/hotspot/jtreg/runtime/modules/SealedModuleTest.java
test/hotspot/jtreg/runtime/sealedClasses/SealedUnnamedModuleTest.java
test/hotspot/jtreg/runtime/sealedClasses/SealedUnnamedModuleIntfTest.java
You don't seem to have coverage of the full test matrix. For the
combination of "same module or not" x "same package or not" x "public or
not", there should be 8 test cases: 3 failures and 5 successes. Then you
also have "listed in permits clause" versus "not listed in permits clause".
Then you have all that for classes and interfaces.
---
On the question of whether to use ReflectionData or not that is really
question for the core-libs folk to decide.
Thanks,
David
-----
> 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