[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: PING: Re: RFR(XS): 8222005: ClassRedefinition crashes with: guarantee(false) failed: OLD and/or OBSO
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2020-05-22 6:19:31
Message-ID: 4db91c1c-fd02-7e69-4778-7569ac65c989 () 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">PING: I'm still looking for reviewers
for this fix!<br>
<br>
Thanks!<br>
Serguei<br>
<br>
<br>
On 5/18/20 00:34, <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:bccf5f91-4350-cc02-48f8-22b9f421593b@oracle.com">
<div class="moz-cite-prefix">On 5/18/20 00:30, <a
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:f97d9630-df42-e874-46f6-e33ac34fad1a@oracle.com">
<div class="moz-cite-prefix">Hi Coleen and potential reviewers,<br>
<br>
Now, the webrev:<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/"
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/</a><br>
<br>
has a complete fix for all three failure modes related to the
guarantee about OLD and OBSOLETE methods.<br>
<br>
The root cause are the following optimizations:<br>
<br>
1) Optimization based on the flag
ik->is_being_redefined():<br>
The problem is that the cpcache method entries of such
classes are not being adjusted.<br>
It is explained below in the initial RFR summary.<br>
The fix is to get rid of this optimization.<br>
<br>
2) Optimization for array classes based on the flag
_has_redefined_Object.<br>
The problem is that the vtable method entries are not
adjusted for array classes.<br>
The array classes have to be adjusted even if the
java.lang.Object was redefined<br>
by one of previous VM_RedefineClasses operation, not only
if it was redefined in<br>
the current VM_RedefineClasses operation. The fix is is
follow this requirement.<br>
<br>
3) Optimization based on the flag <span \
class="removed">_has_null_class_loader which assumes that the Hotspot<br>
does not support delegation </span><span
class="removed">from the bootstrap class loader to a</span><span
class="removed"> user-defined class<br>
loader.</span><span class="removed"> The assumption is
that if the current class being redefined has a user-defined<br>
class</span><span class="removed"> loader as its
defining class loader, then all</span><span class="removed">
classes loaded by the bootstrap<br>
class loader can be skipped for vtable/itable method
entries adjustment.<br>
The problem is that this assumption is not really
correct. There are classes that<br>
still need the adjustment. For instance, the class
java.util.IdentityHashMap$KeyIterator<br>
loaded by the bootstrap class loader has the
vtable/itable references to the method:<br>
</span><span class="removed"><span \
class="removed"></span>java.util.Iterator.forEachRemaining(java.util.function.Consumer)<br>
The class </span><span class="removed"></span><span
class="removed"><span class="removed"></span>java.util.Iterator
is defined by a user-defined class loader.<br>
The fix is to get rid of this optimization.<br>
</span></div>
</blockquote>
<br>
I've pushed the "Send" button too early, sorry.<br>
The fix also has some adjustments for log messages in cpCache.cpp
and klassVtable.cpp<br>
to easier log information for these types of failures.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<blockquote type="cite"
cite="mid:f97d9630-df42-e874-46f6-e33ac34fad1a@oracle.com">
<div class="moz-cite-prefix"><span class="removed"> <br>
All three failure modes are observed with the -Xcomp flag.<br>
With all three fixes above in place, the Kitchensink does
not fail with this guarantee anymore.<br>
</span><span class="removed"></span><span class="removed"></span><span
class="removed"></span><br>
There is still a JIT compiler relted failure:<br>
<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8245128"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8245128</a><br>
Kitchensink fails with: assert(destination == (address)-1
|| destination == entry) failed: b) MT-unsafe modification of
inline cache<br>
<br>
I also saw this failure but just once:<br>
<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8245126"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8245126</a><br>
Kitchensink fails with: assert(!method->is_old())
failed: Should not be installing old methods<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 5/15/20 15:14, <a class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:52ba0f0f-a705-2043-1c1d-15ba4a441aba@oracle.com">
<div class="moz-cite-prefix">Hi Coleen,<br>
<br>
Thanks a lot for review!<br>
Good suggestion, will use it.<br>
<br>
In fact, I've found two more related problems with the same
guarantee.<br>
One is with vtable method entries adjustment and another
with itable.<br>
This webrev version includes a fix for the vtable related
issue:<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/"
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/</a><br>
<br>
I'm still investigating the itable related issue.<br>
<br>
It is interesting that the Kitchensink with Instrumentation
modules enabled is like a Pandora box full of surprises.<br>
New problems are getting discovered after some road blocks
are removed.<br>
I've just filed a couple of compiler bugs discovered in this
mode of testing:<br>
<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8245126"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8245126</a><br>
Kitchensink fails with: assert(!method->is_old())
failed: Should not be installing old methods<br>
<br>
<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8245128"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8245128</a><br>
Kitchensink fails with: assert(destination ==
(address)-1 || destination == entry) failed: b) MT-unsafe
modification of inline cache<br>
<br>
<br>
Thanks,<br>
Serguei <br>
<br>
<br>
On 5/15/20 05:12, <a class="moz-txt-link-abbreviated"
href="mailto:coleen.phillimore@oracle.com"
moz-do-not-send="true">coleen.phillimore@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:2f9aa92c-18f5-1203-1523-3c1fd9ba9ad1@oracle.com">
<br>
Serguei,<br>
<br>
Good find!! The fix looks good. I'm sure the optimization
wasn't noticeable and thank you for the additional comments.<br>
<br>
There is a Method::external_name() function that I believe
prints all the things you want in the logging here:<br>
<br>
<a
href="http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.1/src/hotspot/share/oops/cpCache.cpp.udiff.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.1/src/hotspot/share/oops/cpCache.cpp.udiff.html</a><br>
<br>
I don't need to see another webrev if you make this change.<br>
<br>
Thanks,<br>
Coleen<br>
<br>
<div class="moz-cite-prefix">On 5/14/20 12:26 PM, <a
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:5942b42c-b9b3-f1d4-6c13-774649fca32b@oracle.com">
Please, review a fix for The Kitchensink bug:<br>
<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8222005"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8222005</a><br>
<br>
Webrev:<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.1/"
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.1/</a><br>
<br>
Summary:<br>
The VM_RedefineClasses::doit() uses two helper classes
to walk all VM classes.<br>
First is AdjustAndCleanMetadata to adjust method entries
in the vtables/itables/cpcaches.<br>
Second is CheckClass to check that adjustments for all
method entries are correct.<br>
The Kitchensink test is failing with two modes:<br>
- guarantee(false) failed: OLD and/or OBSOLETE
method(s) found in the<br>
VM_RedefineClasses::CheckClass::do_klass()<br>
- SIGSEGV in the
ConstantPoolCacheEntry::get_interesting_method_entry() in
context<br>
of VM_RedefineClasses::CheckClass::do_klass()
execution<br>
<br>
The second failure mode is rare. In is before the first
one in the code path.<br>
The root cause of both is that the
VM_RedefineClasses::AdjustAndCleanMetadata::do_klass()<br>
is skipping the cpcache update for classes that are
being redefined assuming they are<br>
being redefined by the current VM_RedefineClasses
operation. In such cases, the adjustment<br>
is not needed as the cpcache is empty. The problem is
that the assumption above is wrong.<br>
The class can also be redefined by another
VM_RedefineClasses operation <span class="changed">which
has already<br>
executed its doit_prologue</span>. The cpcache
djustment for such class is necessary.<br>
The fix is to always call the
cp_cache->adjust_method_entries() even if the class is<br>
being redefined by the current VM_RedefineClasses
operation. It is possible to skip it<br>
but it will add extra complexity to the code.<br>
The fix also includes minor tweak in the cpCache.cpp to
include method's class name to<br>
the redefinition cpcache log.<br>
<br>
Testing:<br>
Ran Kitchensink test locally on a Linux server with the
Instrumentation module enabled.<br>
The test does not fail anymore.<br>
In progress, a mach5 tiers 1-5 and runs and separate
mach5 Kitchensink run.<br>
<br>
Thanks,<br>
Serguei<br>
</blockquote>
<br>
</blockquote>
<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