RFR: JDK-8225056 VM support for sealed classes
David Holmes
david.holmes at oracle.com
Mon May 25 02:28:33 UTC 2020
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();
+ }
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;
---
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.
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.
---
src/hotspot/share/prims/jvm.cpp
2159 objArrayHandle result (THREAD, r);
Please remove space after "result".
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.
---
src/hotspot/share/prims/jvmtiRedefineClasses.cpp
857 static jvmtiError
check_permitted_subclasses_attribute(InstanceKlass* the_class,
858 InstanceKlass*
scratch_class) {
Please align.
---
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.
---
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;
I'm wondering now whether using Reflectiondata as the cache for this is
actually the best way to cache it?
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 amber-dev
mailing list