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"
<serviceability-dev-boun...@openjdk.java.net on behalf of chris.plum...@oracle.com> 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
