[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(S): 8244203: sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java fails with
From: Chris Plummer <chris.plummer () oracle ! com>
Date: 2020-05-21 21:39:01
Message-ID: 22c87f00-7ddd-5b82-e547-40142bc5431a () oracle ! com
[Download RAW message or body]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<div class="moz-cite-prefix">Hi Serguei,<br>
<br>
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).<br>
<br>
thanks,<br>
<br>
Chris<br>
<br>
<br>
On 5/21/20 1:22 PM, <a class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:<br> \
</div> <blockquote type="cite"
cite="mid:e75321e3-6af1-0342-635f-625a6349da52@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<div class="moz-cite-prefix">Hi Chris,<br>
<br>
With the below fixed this looks good to me.<br>
<br>
One comment about the constructor:<br>
<pre> 146 public InstanceKlass(Address addr) {
147 super(addr);
<span class="new"> 148 </span>
<span class="new"> 149 // If the class hasn't yet reached the "loaded" init \
state, then don't go any further</span> <span class="new"> 150 // or we'll run \
into problems trying to look at fields that are not yet setup.</span> <span \
class="new"> 151 // Attempted lookups of this InstanceKlass via \
ClassLoaderDataGraph, ClassLoaderData,</span> <span class="new"> 152 // and \
Dictionary will all refuse to return it. The main purpose of allowing this</span> \
<span class="new"> 153 // InstanceKlass to initialize is so \
ClassLoaderData.getKlasses() will succeed, allowing</span> <span class="new"> 154 \
// ClassLoaderData.classesDo() to iterate over all Klasses (skipping those that \
are</span> <span class="new"> 155 // not yet fully loaded).</span>
<span class="new"> 156 if (!isLoaded()) {</span>
<span class="new"> 157 return;</span>
<span class="new"> 158 }</span>
<span class="new"> 159 </span>
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 }</pre>
<br>
The remote process is read-only for SA and is not progressing.<br>
It means, if the class is not fully loaded yet then SA will
never see it loaded later. :-)<br>
So, the InstanceKlass object created by this constructor does
not need to be ever updated (see lines: 160-165).<br>
I'm not sure we need any comment about it as it has to be a
general assumption (which is not always clear though).<br>
<br>
Thanks,<br>
Serguei<br>
<br>
On 5/21/20 12:10, Chris Plummer wrote:<br>
</div>
<blockquote type="cite"
cite="mid:23ecf9af-2019-ff03-8a53-19a1623bad7b@oracle.com">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(). <br>
<br>
thanks, <br>
<br>
Chris <br>
<br>
On 5/21/20 10:44 AM, Daniil Titov wrote: <br>
<blockquote type="cite">Hi Chris, <br>
<br>
I have a question about a change in <br>
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/memory/Dictionary.java.
<br>
<br>
61 /** All classes, and their initiating class loader,
passed in. */ <br>
62 public void
allEntriesDo(ClassLoaderDataGraph.ClassAndLoaderVisitor v, Oop
loader) { <br>
63 int tblSize = tableSize(); <br>
64 for (int index = 0; index < tblSize; index++) { <br>
65 for (DictionaryEntry probe = (DictionaryEntry)
bucket(index); probe != null; <br>
66 \
probe = (DictionaryEntry) probe.next()) { <br>
67 Klass k = probe.klass(); <br>
68 // Only visit InstanceKlasses that are at least
in the "loaded" init_state. Otherwise <br>
69 // the InstanceKlass won't have some required
fields initialized, which can cause problems. <br>
70 if (k instanceof InstanceKlass &&
((InstanceKlass)k).isLoaded()) { <br>
71 continue; <br>
72 } <br>
73 v.visit(k, loader); <br>
74 } <br>
75 } <br>
76 } <br>
<br>
<br>
Based on the comment at lines 68-69 should not condition on
line 70 be <br>
<br>
70 if (k instanceof InstanceKlass &&
!((InstanceKlass)k).isLoaded()) { <br>
<br>
in order to we skip InstanceKlasses that are not in the
"loaded" state? <br>
<br>
Thank you, <br>
Daniil <br>
<br>
On 5/19/20, 12:47 PM, "serviceability-dev on behalf of Chris
Plummer" <a class="moz-txt-link-rfc2396E"
href="mailto:serviceability-dev-bounces@openjdk.java.netonbehalfofchris.plummer@oracle.com"
moz-do-not-send="true"><serviceability-dev-bounces@openjdk.java.net
on behalf of chris.plummer@oracle.com></a> wrote: <br>
<br>
Hello, <br>
<br>
Please review the following: <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~cjplummer/8244203/webrev.00/index.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8244203/webrev.00/index.html</a>
<br>
<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8244203"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8244203</a>
<br>
<br>
The root of the problem is that SA is trying iterate over
all loaded <br>
classes by using ClassLoaderDataGraph, but it possible
that a class that <br>
ClassLoaderDataGraph knows about can be in a state that
makes it <br>
findable by SA, but not yet initialized far enough to
make is usable by SA. <br>
<br>
The first issue I tracked down in this area was a case
where an <br>
InstanceKlass did not yet have its java_mirror. This
resulted in the NPE <br>
you see reported in the bug, because there is some SA
code that assumes <br>
all InstanceKlass's have a java_mirror. I fixed this by
not having <br>
ClassLoaderData.classesDo() return any InstanceKlass that
was not yet <br>
initialized to the point of being considered "loaded".
That fixed this <br>
particular problem, but there was another still lurking
that was similar.. <br>
<br>
The second issue was that event attempting to iterate
over the set of <br>
loaded classes could cause an NPE, so you couldn't even
get to the point <br>
of being able to reject an InstanceKlass if it was not
yet int he <br>
"loaded" state. ClassLoaderData.classesDo() needs to get
the first <br>
Klass in its list: <br>
<br>
public void
classesDo(ClassLoaderDataGraph.ClassVisitor v) { <br>
for (Klass l = getKlasses(); l != null; l =
l.getNextLinkKlass()) { <br>
v.visit(l); <br>
} <br>
} <br>
<br>
Since the first Klass is the one most recently added, its
also subject <br>
to sometimes not yet being fully loaded. Calling
getKlasses() will <br>
instantiate (wrap) the first Klass in the list, which in
this case is a <br>
partially loaded InstanceKlass which was so early in its
initialization <br>
that InstanceKlass::_fields had not yet been setup.
Creating an <br>
InstanceKlass (on the SA side) requires _fields to be
set, otherwise it <br>
gets an NPE. I fixed this by allowing the InstanceKlass
to still be <br>
created if not yet "loaded", but skipped any further
initialization of <br>
the InstanceKlass that would rely on _fields. This allows
the <br>
InstanceKlass to still be used by
ClassLoaderData.classesDo() to iterate <br>
over the classes. However, I also wanted to make sure
this uninitialized <br>
InstanceKlass wasn't leaked out to SA beyond its use by
classesDo(). It <br>
looks like the only other way this is possible is with <br>
ClassLoaderData.find() and Dictionary.allEntriesDo(), so
I put an <br>
InstanceKlass.isLoaded() checks there also. <br>
<br>
One way I tested these fixes was to by introducing short
sleep in <br>
ClassFileParser::fill_instance_klass() right after the
Klass gets added <br>
to the ClassLoaderData: <br>
<br>
_loader_data->add_class(ik, publicize); <br>
+ os::naked_short_sleep(2); <br>
<br>
By doing this the bug reproduced almost every time, and
the fixes <br>
resolved it. <br>
<br>
You'll also notice a bug fix in
ClassLoaderData.allEntriesDo(). The <br>
outside loop is not only unnecessary, but results in the
same Klass <br>
being visited multiple times. It turns out no one uses
this <br>
functionality, but I fixed it anyway rather than rip it
out because it <br>
seemed it might be useful in the future. <br>
<br>
The changes in ClhsdbClasses.java test are to better
check for expected <br>
classes when dumping the set of all classes. I added
these after <br>
realizing I had introduced a bug that caused only
InstanceKlasses to be <br>
dumped, not interfaces or arrays, so I added a few more
types to the <br>
list that we check. <br>
<br>
thanks, <br>
<br>
Chris <br>
<br>
<br>
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic