Re: RFR: JDK-8225056 VM support for sealed classes

[David Holmes]

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