Hi Coleen,
Thanks a lot for review!
Good suggestion, will use it.
In fact, I've found two more related problems with the same guarantee.
One is with vtable method entries adjustment and another with itable.
This webrev version includes a fix for the vtable related issue:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/
I'm still investigating the itable related issue.
It is interesting that the Kitchensink with Instrumentation modules enabled is like a Pandora box full of surprises.
New problems are getting discovered after some road blocks are removed.
I've just filed a couple of compiler bugs discovered in this mode of testing:
https://bugs.openjdk.java.net/browse/JDK-8245126
Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
https://bugs.openjdk.java.net/browse/JDK-8245128
Kitchensink fails with: assert(destination == (address)-1 || destination == entry) failed: b) MT-unsafe modification of inline cache
Thanks,
Serguei
On 5/15/20 05:12, coleen.phillim...@oracle.com wrote:
Thanks a lot for review!
Good suggestion, will use it.
In fact, I've found two more related problems with the same guarantee.
One is with vtable method entries adjustment and another with itable.
This webrev version includes a fix for the vtable related issue:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/
I'm still investigating the itable related issue.
It is interesting that the Kitchensink with Instrumentation modules enabled is like a Pandora box full of surprises.
New problems are getting discovered after some road blocks are removed.
I've just filed a couple of compiler bugs discovered in this mode of testing:
https://bugs.openjdk.java.net/browse/JDK-8245126
Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
https://bugs.openjdk.java.net/browse/JDK-8245128
Kitchensink fails with: assert(destination == (address)-1 || destination == entry) failed: b) MT-unsafe modification of inline cache
Thanks,
Serguei
On 5/15/20 05:12, coleen.phillim...@oracle.com wrote:
Serguei,
Good find!! The fix looks good. I'm sure the optimization wasn't noticeable and thank you for the additional comments.
There is a Method::external_name() function that I believe prints all the things you want in the logging here:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.1/src/hotspot/share/oops/cpCache.cpp.udiff.html
I don't need to see another webrev if you make this change.
Thanks,
Coleen
On 5/14/20 12:26 PM, serguei.spit...@oracle.com wrote:
Please, review a fix for The Kitchensink bug:
https://bugs.openjdk.java.net/browse/JDK-8222005
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.1/
Summary:
The VM_RedefineClasses::doit() uses two helper classes to walk all VM classes.
First is AdjustAndCleanMetadata to adjust method entries in the vtables/itables/cpcaches.
Second is CheckClass to check that adjustments for all method entries are correct.
The Kitchensink test is failing with two modes:
- guarantee(false) failed: OLD and/or OBSOLETE method(s) found in the
VM_RedefineClasses::CheckClass::do_klass()
- SIGSEGV in the ConstantPoolCacheEntry::get_interesting_method_entry() in context
of VM_RedefineClasses::CheckClass::do_klass() execution
The second failure mode is rare. In is before the first one in the code path.
The root cause of both is that the VM_RedefineClasses::AdjustAndCleanMetadata::do_klass()
is skipping the cpcache update for classes that are being redefined assuming they are
being redefined by the current VM_RedefineClasses operation. In such cases, the adjustment
is not needed as the cpcache is empty. The problem is that the assumption above is wrong.
The class can also be redefined by another VM_RedefineClasses operation which has already
executed its doit_prologue. The cpcache djustment for such class is necessary.
The fix is to always call the cp_cache->adjust_method_entries() even if the class is
being redefined by the current VM_RedefineClasses operation. It is possible to skip it
but it will add extra complexity to the code.
The fix also includes minor tweak in the cpCache.cpp to include method's class name to
the redefinition cpcache log.
Testing:
Ran Kitchensink test locally on a Linux server with the Instrumentation module enabled.
The test does not fail anymore.
In progress, a mach5 tiers 1-5 and runs and separate mach5 Kitchensink run.
Thanks,
Serguei