PING: Re: RFR(XS): 8222005: ClassRedefinition crashes with: guarantee(false) failed: OLD and/or OBSOLETE method(s) found
PING: One more review for this fix is needed. Thanks, Serguei On 6/3/20 09:52, serguei.spit...@oracle.com wrote: Thank you a lot for review, Coleen! Serguei On 6/3/20 08:50, coleen.phillim...@oracle.com wrote: Hi Serguei, This change looks great. Thank you for fixing this! Coleen On 5/28/20 7:16 PM, serguei.spit...@oracle.com wrote: Hi Coleen, The updated webrev version is: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.3/ It has your suggestions addressed: - remove log_is_enabled conditions - move ResourceMark's out of loops Thanks, Serguei On 5/28/20 14:44, serguei.spit...@oracle.com wrote: Hi Coleen, Thank you a lot for reviewing this! On 5/28/20 12:48, coleen.phillim...@oracle.com wrote: Hi Serguei, Sorry for the delay reviewing this again. On 5/18/20 3:30 AM, serguei.spit...@oracle.com wrote: Hi Coleen and potential reviewers, Now, the webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/ has a complete fix for all three failure modes related to the guarantee about OLD and OBSOLETE methods. The root cause are the following optimizations: 1) Optimization based on the flag ik->is_being_redefined(): The problem is that the cpcache method entries of such classes are not being adjusted. It is explained below in the initial RFR summary. The fix is to get rid of this optimization. This seems like a good thing to do even though (actually especially because) I can't re-imagine the logic that went into this optimization. Probably, I've not explained it well enough. The logic was that the class marked as is_being_redefined was considered as being redefined in the current redefinition operation. For classes redefined in current redefinition the cpcache is empty, so there is nothing to adjust. The problem is that classes can be marked as is_being_redefined by doit_prologue of one of the following redefinition operations. In such a case, the VM_RedefineClasses::CheckClass::do_klass fails with this guarantee. It is because the VM_RedefineClasses::CheckClass::do_klass does not have this optimization and does not skip such classes as the VM_RedefineClasses::AdjustAndCleanMetadata::do_class. Without this catch this issue could have unknown consequences in the future execution far away from the root cause. 2) Optimization for array classes based on the flag _has_redefined_Object. The problem is that the vtable method entries are not adjusted for array classes. The array classes have to be adjusted even if the java.lang.Object was redefined by one of previous VM_RedefineClasses operation, not only if it was redefined in the current VM_RedefineClasses operation. The fix is is follow this requirement. This I can't understand. The redefinitions are serialized in safepoints, so why would you need to replace vtable entries for arrays if java.lang.Object isn't redefined in this safepoint? The VM_RedefineClasses::CheckClass::do_klass fails with the same guarantee because of this. It never fails this way with this optimization relaxed. I've already broke my head trying to understand it. It can be because of another bug we don't know yet. 3) Optimization based on the flag _has_null_class_loader which assumes that the Hotspot
PING: Re: RFR(XS): 8222005: ClassRedefinition crashes with: guarantee(false) failed: OLD and/or OBSOLETE method(s) found
PING: I'm still looking for reviewers for this fix! Thanks! Serguei On 5/18/20 00:34, serguei.spit...@oracle.com wrote: On 5/18/20 00:30, serguei.spit...@oracle.com wrote: Hi Coleen and potential reviewers, Now, the webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/ has a complete fix for all three failure modes related to the guarantee about OLD and OBSOLETE methods. The root cause are the following optimizations: 1) Optimization based on the flag ik->is_being_redefined(): The problem is that the cpcache method entries of such classes are not being adjusted. It is explained below in the initial RFR summary. The fix is to get rid of this optimization. 2) Optimization for array classes based on the flag _has_redefined_Object. The problem is that the vtable method entries are not adjusted for array classes. The array classes have to be adjusted even if the java.lang.Object was redefined by one of previous VM_RedefineClasses operation, not only if it was redefined in the current VM_RedefineClasses operation. The fix is is follow this requirement. 3) Optimization based on the flag _has_null_class_loader which assumes that the Hotspot does not support delegation from the bootstrap class loader to a user-defined class loader. The assumption is that if the current class being redefined has a user-defined class loader as its defining class loader, then all classes loaded by the bootstrap class loader can be skipped for vtable/itable method entries adjustment. The problem is that this assumption is not really correct. There are classes that still need the adjustment. For instance, the class java.util.IdentityHashMap$KeyIterator loaded by the bootstrap class loader has the vtable/itable references to the method: java.util.Iterator.forEachRemaining(java.util.function.Consumer) The class java.util.Iterator is defined by a user-defined class loader. The fix is to get rid of this optimization. I've pushed the "Send" button too early, sorry. The fix also has some adjustments for log messages in cpCache.cpp and klassVtable.cpp to easier log information for these types of failures. Thanks, Serguei All three failure modes are observed with the -Xcomp flag. With all three fixes above in place, the Kitchensink does not fail with this guarantee anymore. There is still a JIT compiler relted failure: 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 I also saw this failure but just once: https://bugs.openjdk.java.net/browse/JDK-8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods Thanks, Serguei On 5/15/20 15:14, serguei.spit...@oracle.com wrote: 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
