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
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
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
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
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, , _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
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
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
