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:
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" <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
