Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
Hi Dannil, On 22/05/2020 3:06 pm, Daniil Titov wrote: Please review a webrev [1] that reverts the changes done in jdk.test.lib.process.OutputAnalyzer in [3]. Change [3] modified OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() methods to ignore also VM version strings . The current webrev [1] reverts this change Thank you for reverting the OutputAnalyzer change. and instead makes the tests that expects an empty stderr from launched j-* tools to filter out '-showversion' from test options when forwarding test VM option to j*-tools. That seems workable, though I'm unclear what is adding the "-showversion" in the first place? Thanks, David - Testing: Mach5 tier1-tier5 tests passed successfully. Tier6-tier7 tests are in progress. [1] Webrev: http://cr.openjdk.java.net/~dtitov/8244993/webrev.01 [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993 [3] https://bugs.openjdk.java.net/browse/JDK-8242009 Thank you, Daniil
Re: RFR (XS): 8245392: Remove duplication in class redefinition and retransformation specs
Hi Serguei, On 22/05/2020 3:02 pm, serguei.spit...@oracle.com wrote: Please, review a fix for: https://bugs.openjdk.java.net/browse/JDK-8245392 CSR draft (one CSR reviewer is needed before finalizing it): https://bugs.openjdk.java.net/browse/JDK-8245433 I've made some updates and comments on the CSR request. Only minor suggestions. Once that is addressed I'll add my review there and here. Thanks, David Webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/src/ Updated specs: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/docs/api/java.instrument/java/lang/instrument/Instrumentation.html http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/docs/specs/jdwp/jdwp-protocol.html http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/docs/api/jdk.jdi/com/sun/jdi/VirtualMachine.html Summary: The fix is to replace in Instrumentation, JDI and JDWP spec a description of class redefinition or retransformation restriction with a link to the supporting JVM TI function where it has been already documented. This spec refactoring should help in cases when new 'unmodifiable in redefinition' class file attributes are added. Testing: Built docs and checked the link works as expected. Thanks, Serguei
RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
Please review a webrev [1] that reverts the changes done in jdk.test.lib.process.OutputAnalyzer in [3]. Change [3] modified OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() methods to ignore also VM version strings . The current webrev [1] reverts this change and instead makes the tests that expects an empty stderr from launched j-* tools to filter out '-showversion' from test options when forwarding test VM option to j*-tools. Testing: Mach5 tier1-tier5 tests passed successfully. Tier6-tier7 tests are in progress. [1] Webrev: http://cr.openjdk.java.net/~dtitov/8244993/webrev.01 [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993 [3] https://bugs.openjdk.java.net/browse/JDK-8242009 Thank you, Daniil
RFR (XS): 8245392: Remove duplication in class redefinition and retransformation specs
Please, review a fix for: https://bugs.openjdk.java.net/browse/JDK-8245392 CSR draft (one CSR reviewer is needed before finalizing it): https://bugs.openjdk.java.net/browse/JDK-8245433 Webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/src/ Updated specs: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/docs/api/java.instrument/java/lang/instrument/Instrumentation.html http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/docs/specs/jdwp/jdwp-protocol.html http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/docs/api/jdk.jdi/com/sun/jdi/VirtualMachine.html Summary: The fix is to replace in Instrumentation, JDI and JDWP spec a description of class redefinition or retransformation restriction with a link to the supporting JVM TI function where it has been already documented. This spec refactoring should help in cases when new 'unmodifiable in redefinition' class file attributes are added. Testing: Built docs and checked the link works as expected. Thanks, Serguei
Re: RFR(XS): 8245521: Remove STACK_BIAS
Hi Mikael, Looks good. I assume the change to GraalHotSpotVMConfig.java is to allow it to work with older VMs? Thanks, David On 22/05/2020 1:36 pm, Mikael Vidstedt wrote: Please review this small change which removes the STACK_BIAS constant and its uses: JBS: https://bugs.openjdk.java.net/browse/JDK-8245521 webrev: http://cr.openjdk.java.net/~mikael/webrevs/8245521/webrev.00/open/webrev/ Background (from JBS): With Solaris/SPARC removed the STACK_BIAS definition in src/hotspot/share/utilities/globalDefinitions.hpp is now always 0 and can be removed. Testing: Tier1 Cheers, Mikael
RFR(XS): 8245521: Remove STACK_BIAS
Please review this small change which removes the STACK_BIAS constant and its uses: JBS: https://bugs.openjdk.java.net/browse/JDK-8245521 webrev: http://cr.openjdk.java.net/~mikael/webrevs/8245521/webrev.00/open/webrev/ Background (from JBS): With Solaris/SPARC removed the STACK_BIAS definition in src/hotspot/share/utilities/globalDefinitions.hpp is now always 0 and can be removed. Testing: Tier1 Cheers, Mikael
Re: RFR(S): 8244203: sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java fails with NullPointerException
Hi Serguei,
Your point about about the JVM state being read-only and
unchanging while SA is attached is valid. I had to clarify this
not so obvious point a couple times during internal discussions.
Basically any hotspot state that SA has cached is thrown away
every time you detach, and while attached the hotspot state cannot
change. But in the end this is something fundamental to how SA
works that you just need to know in order to understand SA code. I
don't think we can clarify it with comments everywhere in SA where
it matters (which is just about everywhere).
thanks,
Chris
On 5/21/20 1:22 PM, serguei.spit...@oracle.com wrote:
Hi Chris,
With the below fixed this looks good to me.
One comment about the constructor:
146 public InstanceKlass(Address addr) {
147 super(addr);
148
149 // If the class hasn't yet reached the "loaded" init state, then don't go any further
150 // or we'll run into problems trying to look at fields that are not yet setup.
151 // Attempted lookups of this InstanceKlass via ClassLoaderDataGraph, ClassLoaderData,
152 // and Dictionary will all refuse to return it. The main purpose of allowing this
153 // InstanceKlass to initialize is so ClassLoaderData.getKlasses() will succeed, allowing
154 // ClassLoaderData.classesDo() to iterate over all Klasses (skipping those that are
155 // not yet fully loaded).
156 if (!isLoaded()) {
157 return;
158 }
159
160 if (getJavaFieldsCount() != getAllFieldsCount()) {
161 // Exercise the injected field logic
162 for (int i = getJavaFieldsCount(); i < getAllFieldsCount(); i++) {
163 getFieldName(i);
164 getFieldSignature(i);
165 }
166 }
167 }
The remote process is read-only for SA and is not progressing.
It means, if the class is not fully loaded yet then SA will
never see it loaded later. :-)
So, the InstanceKlass object created by this constructor does
not need to be ever updated (see lines: 160-165).
I'm not sure we need any comment about it as it has to be a
general assumption (which is not always clear though).
Thanks,
Serguei
On 5/21/20 12:10, Chris Plummer wrote:
Yes,
you are correct. As I mentioned earlier, it turns out this
functionality isn't used, other wise it also would have been
exposed by the other bug I fixed (the unnecessary outter loop).
I'll fix this to use !isLoaded().
thanks,
Chris
On 5/21/20 10:44 AM, Daniil Titov wrote:
Hi Chris,
I have a question about a change in
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/memory/Dictionary.java.
61 /** All classes, and their initiating class loader,
passed in. */
62 public void
allEntriesDo(ClassLoaderDataGraph.ClassAndLoaderVisitor v, Oop
loader) {
63 int tblSize = tableSize();
64 for (int index = 0; index < tblSize; index++) {
65 for (DictionaryEntry probe = (DictionaryEntry)
bucket(index); probe != null;
66 probe =
(DictionaryEntry) probe.next()) {
67 Klass k = probe.klass();
68 // Only visit InstanceKlasses that are at least
in the "loaded" init_state. Otherwise
69 // the InstanceKlass won't have some required
fields initialized, which can cause problems.
70 if (k instanceof InstanceKlass &&
((InstanceKlass)k).isLoaded()) {
71 continue;
72 }
73 v.visit(k, loader);
74 }
75 }
76 }
Based on the comment at lines 68-69 should not condition on
line 70 be
70 if (k instanceof InstanceKlass &&
!((InstanceKlass)k).isLoaded()) {
in order to we skip InstanceKlasses that are not in the
"loaded" state?
Thank you,
Daniil
On 5/19/20, 12:47 PM, "serviceability-dev on behalf of Chris
Plummer" wrote:
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
Re: RFR(S): 8244203: sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java fails with NullPointerException
Hi Chris,
With the below fixed this looks good to me.
One comment about the constructor:
146 public InstanceKlass(Address addr) {
147 super(addr);
148
149 // If the class hasn't yet reached the "loaded" init state, then don't go any further
150 // or we'll run into problems trying to look at fields that are not yet setup.
151 // Attempted lookups of this InstanceKlass via ClassLoaderDataGraph, ClassLoaderData,
152 // and Dictionary will all refuse to return it. The main purpose of allowing this
153 // InstanceKlass to initialize is so ClassLoaderData.getKlasses() will succeed, allowing
154 // ClassLoaderData.classesDo() to iterate over all Klasses (skipping those that are
155 // not yet fully loaded).
156 if (!isLoaded()) {
157 return;
158 }
159
160 if (getJavaFieldsCount() != getAllFieldsCount()) {
161 // Exercise the injected field logic
162 for (int i = getJavaFieldsCount(); i < getAllFieldsCount(); i++) {
163 getFieldName(i);
164 getFieldSignature(i);
165 }
166 }
167 }
The remote process is read-only for SA and is not progressing.
It means, if the class is not fully loaded yet then SA will never
see it loaded later. :-)
So, the InstanceKlass object created by this constructor does not
need to be ever updated (see lines: 160-165).
I'm not sure we need any comment about it as it has to be a
general assumption (which is not always clear though).
Thanks,
Serguei
On 5/21/20 12:10, Chris Plummer wrote:
Yes,
you are correct. As I mentioned earlier, it turns out this
functionality isn't used, other wise it also would have been
exposed by the other bug I fixed (the unnecessary outter loop).
I'll fix this to use !isLoaded().
thanks,
Chris
On 5/21/20 10:44 AM, Daniil Titov wrote:
Hi Chris,
I have a question about a change in
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/memory/Dictionary.java.
61 /** All classes, and their initiating class loader, passed
in. */
62 public void
allEntriesDo(ClassLoaderDataGraph.ClassAndLoaderVisitor v, Oop
loader) {
63 int tblSize = tableSize();
64 for (int index = 0; index < tblSize; index++) {
65 for (DictionaryEntry probe = (DictionaryEntry)
bucket(index); probe != null;
66 probe =
(DictionaryEntry) probe.next()) {
67 Klass k = probe.klass();
68 // Only visit InstanceKlasses that are at least in
the "loaded" init_state. Otherwise
69 // the InstanceKlass won't have some required
fields initialized, which can cause problems.
70 if (k instanceof InstanceKlass &&
((InstanceKlass)k).isLoaded()) {
71 continue;
72 }
73 v.visit(k, loader);
74 }
75 }
76 }
Based on the comment at lines 68-69 should not condition on line
70 be
70 if (k instanceof InstanceKlass &&
!((InstanceKlass)k).isLoaded()) {
in order to we skip InstanceKlasses that are not in the
"loaded" state?
Thank you,
Daniil
On 5/19/20, 12:47 PM, "serviceability-dev on behalf of Chris
Plummer" wrote:
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.
Re: RFR(S): 8244203: sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java fails with NullPointerException
Yes, you are correct. As I mentioned earlier, it turns out this
functionality isn't used, other wise it also would have been exposed by
the other bug I fixed (the unnecessary outter loop). I'll fix this to
use !isLoaded().
thanks,
Chris
On 5/21/20 10:44 AM, Daniil Titov wrote:
Hi Chris,
I have a question about a change in
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/memory/Dictionary.java.
61 /** All classes, and their initiating class loader, passed in. */
62 public void allEntriesDo(ClassLoaderDataGraph.ClassAndLoaderVisitor v,
Oop loader) {
63 int tblSize = tableSize();
64 for (int index = 0; index < tblSize; index++) {
65 for (DictionaryEntry probe = (DictionaryEntry) bucket(index); probe
!= null;
66 probe = (DictionaryEntry)
probe.next()) {
67 Klass k = probe.klass();
68 // Only visit InstanceKlasses that are at least in the "loaded"
init_state. Otherwise
69 // the InstanceKlass won't have some required fields initialized,
which can cause problems.
70 if (k instanceof InstanceKlass && ((InstanceKlass)k).isLoaded()) {
71 continue;
72 }
73 v.visit(k, loader);
74 }
75 }
76 }
Based on the comment at lines 68-69 should not condition on line 70 be
70 if (k instanceof InstanceKlass && !((InstanceKlass)k).isLoaded()) {
in order to we skip InstanceKlasses that are not in the "loaded" state?
Thank you,
Daniil
On 5/19/20, 12:47 PM, "serviceability-dev on behalf of Chris Plummer"
wrote:
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
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Mandy,
Thanks for the suggestions. They have been incorporated in the revised
webrev.
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
Harold
On 5/20/2020 1:05 PM, Mandy Chung wrote:
Hi Vicente,
On 5/20/20 8:40 AM, Vicente Romero wrote:
Hi David,
src/java.base/share/classes/java/lang/Class.java
There needs to be a CSR request for these changes.
yes there is one already:
https://bugs.openjdk.java.net/browse/JDK-8244556
Adding to David's comment w.r.t. @throws IAE:
The Class::getXXX APIs returns `Class` or `Class[]` if the result is
about type(s). This new `getPermittedSubclasses` returns
`ClassDesc[]`. I wonder if this should be renamed to
`permittedSubclasses` to follow the convention of the new APIs added
to support descriptors e.g. `describeConstable`
Nit: {@linkplain Class} should be {@code Class}
Mandy
Re: RFR: JDK-8225056 VM support for sealed classes
Hi David,
Thanks for looking at this! Please review this new webrev:
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
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,
+
Re: RFR(S): 8244203: sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java fails with NullPointerException
Hi Chris,
I have a question about a change in
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/memory/Dictionary.java.
61 /** All classes, and their initiating class loader, passed in. */
62 public void allEntriesDo(ClassLoaderDataGraph.ClassAndLoaderVisitor v,
Oop loader) {
63 int tblSize = tableSize();
64 for (int index = 0; index < tblSize; index++) {
65 for (DictionaryEntry probe = (DictionaryEntry) bucket(index); probe
!= null;
66 probe = (DictionaryEntry)
probe.next()) {
67 Klass k = probe.klass();
68 // Only visit InstanceKlasses that are at least in the "loaded"
init_state. Otherwise
69 // the InstanceKlass won't have some required fields initialized,
which can cause problems.
70 if (k instanceof InstanceKlass && ((InstanceKlass)k).isLoaded()) {
71 continue;
72 }
73 v.visit(k, loader);
74 }
75 }
76 }
Based on the comment at lines 68-69 should not condition on line 70 be
70 if (k instanceof InstanceKlass && !((InstanceKlass)k).isLoaded()) {
in order to we skip InstanceKlasses that are not in the "loaded" state?
Thank you,
Daniil
On 5/19/20, 12:47 PM, "serviceability-dev on behalf of Chris Plummer"
wrote:
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
Fwd: RFR(S): 8244203: sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java fails with NullPointerException
Ping!
Forwarded Message
Subject:
RFR(S): 8244203:
sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java fails
with NullPointerException
Date:
Tue, 19 May 2020 12:47:17 -0700
From:
Chris Plummer
To:
serviceability-dev
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
