Re: RFR: JDK-8225056 VM support for sealed classes
Hi David, On 5/19/20 19:31, David Holmes wrote: Hi Serguei, On 20/05/2020 1:49 am, serguei.spit...@oracle.com wrote: Hi Harold and David, Just one comment. We could save on the CSR's for: make/data/jdwp/jdwp.spec || src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java || src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java Just to be clear, you don't need a CSR request per change but each change must be covered by some CSR request. Oh, right. I was confused by your comments against each change. As we discussed with David, it could make sense to remove duplication in the Serviceability class redefinition and retransformation specs. I'd suggest something like this webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/ If the direction looks okay then I could file RFE+CSR (and post an RFR). I like the idea of keeping one list referred to by the other specs! Thanks, I've started this. Probably, it is better to wait until this fix is pushed. Thanks, Serguei Thanks, David - 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 fi
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Serguei, On 20/05/2020 1:49 am, serguei.spit...@oracle.com wrote: Hi Harold and David, Just one comment. We could save on the CSR's for: make/data/jdwp/jdwp.spec || src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java || src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java Just to be clear, you don't need a CSR request per change but each change must be covered by some CSR request. As we discussed with David, it could make sense to remove duplication in the Serviceability class redefinition and retransformation specs. I'd suggest something like this webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/ If the direction looks okay then I could file RFE+CSR (and post an RFR). I like the idea of keeping one list referred to by the other specs! Thanks, David - 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 tha
RFR(S): 8244203: sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java fails with NullPointerException
Hello, Please review the following: http://cr.openjdk.java.net/~cjplummer/8244203/webrev.00/index.html https://bugs.openjdk.java.net/browse/JDK-8244203 The root of the problem is that SA is trying iterate over all loaded classes by using ClassLoaderDataGraph, but it possible that a class that ClassLoaderDataGraph knows about can be in a state that makes it findable by SA, but not yet initialized far enough to make is usable by SA. The first issue I tracked down in this area was a case where an InstanceKlass did not yet have its java_mirror. This resulted in the NPE you see reported in the bug, because there is some SA code that assumes all InstanceKlass's have a java_mirror. I fixed this by not having ClassLoaderData.classesDo() return any InstanceKlass that was not yet initialized to the point of being considered "loaded". That fixed this particular problem, but there was another still lurking that was similar.. The second issue was that event attempting to iterate over the set of loaded classes could cause an NPE, so you couldn't even get to the point of being able to reject an InstanceKlass if it was not yet int he "loaded" state. ClassLoaderData.classesDo() needs to get the first Klass in its list: public void classesDo(ClassLoaderDataGraph.ClassVisitor v) { for (Klass l = getKlasses(); l != null; l = l.getNextLinkKlass()) { v.visit(l); } } Since the first Klass is the one most recently added, its also subject to sometimes not yet being fully loaded. Calling getKlasses() will instantiate (wrap) the first Klass in the list, which in this case is a partially loaded InstanceKlass which was so early in its initialization that InstanceKlass::_fields had not yet been setup. Creating an InstanceKlass (on the SA side) requires _fields to be set, otherwise it gets an NPE. I fixed this by allowing the InstanceKlass to still be created if not yet "loaded", but skipped any further initialization of the InstanceKlass that would rely on _fields. This allows the InstanceKlass to still be used by ClassLoaderData.classesDo() to iterate over the classes. However, I also wanted to make sure this uninitialized InstanceKlass wasn't leaked out to SA beyond its use by classesDo(). It looks like the only other way this is possible is with ClassLoaderData.find() and Dictionary.allEntriesDo(), so I put an InstanceKlass.isLoaded() checks there also. One way I tested these fixes was to by introducing short sleep in ClassFileParser::fill_instance_klass() right after the Klass gets added to the ClassLoaderData: _loader_data->add_class(ik, publicize); + os::naked_short_sleep(2); By doing this the bug reproduced almost every time, and the fixes resolved it. You'll also notice a bug fix in ClassLoaderData.allEntriesDo(). The outside loop is not only unnecessary, but results in the same Klass being visited multiple times. It turns out no one uses this functionality, but I fixed it anyway rather than rip it out because it seemed it might be useful in the future. The changes in ClhsdbClasses.java test are to better check for expected classes when dumping the set of all classes. I added these after realizing I had introduced a bug that caused only InstanceKlasses to be dumped, not interfaces or arrays, so I added a few more types to the list that we check. thanks, Chris
Re: RFR: JDK-8225056 VM support for sealed classes
Filed the enhancement: https://bugs.openjdk.java.net/browse/JDK-8245392 Remove deduplication in class redefinition and retransformation specs Will also create CSR and post RFR soon. Thanks, Serguei On 5/19/20 09:46, Harold Seigel wrote: I think that's a good idea. Thanks, Harold On 5/19/2020 11:49 AM, serguei.spit...@oracle.com wrote: Hi Harold and David, Just one comment. We could save on the CSR's for: make/data/jdwp/jdwp.spec src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java As we discussed with David, it could make sense to remove duplication in the Serviceability class redefinition and retransformation specs. I'd suggest something like this webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/ If the direction looks okay then I could file RFE+CSR (and post an RFR). 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) {
Re: RFR: JDK-8225056 VM support for sealed classes
On 5/19/20 09:46, Harold Seigel wrote: That sounds good! Thanks, Harold On 5/19/2020 11:53 AM, serguei.spit...@oracle.com wrote: 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. Filed enhancement and assigned to myself: https://bugs.openjdk.java.net/browse/JDK-8245321 refactor the redefine check that an attribute consisting of a list of classes has not changed Thanks, Serguei 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
Re: RFR: JDK-8225056 VM support for sealed classes
That sounds good! Thanks, Harold On 5/19/2020 11:53 AM, serguei.spit...@oracle.com wrote: 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 m
Re: RFR: JDK-8225056 VM support for sealed classes
I think that's a good idea. Thanks, Harold On 5/19/2020 11:49 AM, serguei.spit...@oracle.com wrote: Hi Harold and David, Just one comment. We could save on the CSR's for: make/data/jdwp/jdwp.spec || src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java || src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java As we discussed with David, it could make sense to remove duplication in the Serviceability class redefinition and retransformation specs. I'd suggest something like this webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/ If the direction looks okay then I could file RFE+CSR (and post an RFR). 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 pack
Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools
Hi Chris and Serguei, Thank you for reviewing this change. Best regards, Daniil On 5/18/20, 12:41 PM, "Chris Plummer" wrote: Looks good. thanks, Chris On 5/14/20 1:33 PM, Daniil Titov wrote: > Hi Serguei and Chris, > > Please review a new version of the change [1] that addresses your comments. > > Testing: Mach5 tier1-tier5 tests successfully passed. > > Regarding CR for the JDWP spec issues related to missing type information in some of the protocol packet descriptions [3], as Chris has just noticed > we really don't need it, so I withdrew this CR. > > [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.02 > [2] https://bugs.openjdk.java.net/browse/JDK-8241080 > [3] https://bugs.openjdk.java.net/browse/JDK-8245057 > > Thank you, > Daniil > > > From: "serguei.spit...@oracle.com" > Date: Monday, May 11, 2020 at 11:53 AM > To: Daniil Titov , serviceability-dev > Subject: Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools > > Hi Daniil, > > It looks pretty good in general. > A couple of nits below. > > http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c.udiff.html > +void *cursor; > +jbyte argumentTag; > +jint argIndex = 0; > +jvalue *argument = request->arguments;; > . . . > void *cursor; > jint argIndex = 0; > +jbyte argumentTag; > jvalue *argument = request->arguments; > > It is better if the local variables 'cursor' and 'argumentTag' get initialized. > There is double semicolon: "arguments;;" > > > http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/signature.h.html >43 static inline jbyte basicType(const char *signature) { > > It'd be nice to rename it to basicTypeTag() to get it unified with other functions below. > > It is more safe to run tier5 as well. > > Thanks, > Serguei > > > On 5/9/20 09:29, Daniil Titov wrote: > Please review a change[1] that centralizes the signature processing in serviceability tools to make it capable of being easily extensible in the future. > > Testing: Mach5 tier1-tier3 tests successfully passed. > > [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.01 > [2] https://bugs.openjdk.java.net/browse/JDK-8241080 > > Thank you, > Daniil > > > > > >
RFR (XS): 8244571: assert(!_thread->is_pending_jni_exception_check()) failed: Pending JNI Exception Check during class loading
Please, review fix for: https://bugs.openjdk.java.net/browse/JDK-8244571 Webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.1/ Summary: There are two places in the native part of test that cause assert and WARNING with the -Xcheck:jni. The assert is because there is no check for pending exception after the call to: jni->CallBooleanMethod(klass, is_hid_mid); Using a JNI ExceptionCheck() after the call fixes the issue. The following call to the JVM TI function: err = jvmti->GetClassLoaderClasses(loader, &count, &loader_classes); produces the warning (with a java level stack trace): WARNING: JNI local refs: 94, exceeds capacity: 32 It is because the GetClassLoaderClasses returns an array of local references to the loader classes. Using a JNI EnsureLocalCapacity() before the JVM TI call also fixes the issue. Testing: Running the test test/hotspot/jtreg/serviceability/jvmti/HiddenClass locally. Will run a mach5 job as well. Thanks, Serguei
Re: RFR: JDK-8225056 VM support for sealed classes
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 excep
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Harold and David, Just one comment. We could save on the CSR's for: make/data/jdwp/jdwp.spec src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java As we discussed with David, it could make sense to remove duplication in the Serviceability class redefinition and retransformation specs. I'd suggest something like this webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/ If the direction looks okay then I could file RFE+CSR (and post an RFR). 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 attribu
Re: RFR: JDK-8225056 VM support for sealed classes
- Mail original - > De: "David Holmes" > À: "Harold David Seigel" , "hotspot-runtime-dev" > , > "amber-dev" , "core-libs-dev" > , "serviceability-dev" > > Envoyé: Mardi 19 Mai 2020 07:26:38 > Objet: Re: RFR: JDK-8225056 VM support for sealed classes > 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 ... Currently the version of ASM used JDK already supports sealed classes but with the attribute named PermittedSubtypes instead of PermittedSubclasses. This patch renames the attribute which is the right thing to do. Then when JDK 15 will be released, we will release ASM 9 with full support of PermittedSubclasses, with the right method names, docs, etc, that will be then integrated in JDK 16. So with my ASM hat, the changes to the 3 ASM classes are good. Rémi