From a9c3c58e8b31baf8d4abae33a45bb68c3bfc4e73 Mon Sep 17 00:00:00 2001 From: tstuefe Date: Tue, 8 Jul 2025 15:46:59 +0200 Subject: [PATCH 1/4] start --- .../share/gc/shenandoah/shenandoahAsserts.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp b/src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp index ffafcc5840d78..037f344ee7208 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp @@ -269,7 +269,18 @@ void ShenandoahAsserts::assert_correct(void* interior_loc, oop obj, const char* // We want to check class loading/unloading did not corrupt them. if (Universe::is_fully_initialized() && (obj_klass == vmClasses::Class_klass())) { - Metadata* klass = obj->metadata_field(java_lang_Class::klass_offset()); + // Note: class redefinition involves creating a new Klass structure and a new associated mirror object. + // The old Klass is discarded and thus made invalid. The old class mirror's Klass field is nulled + // out during redefinition (correctly) - hence the need for the "klass != nullptr" condition below. + // + // However, if this is happening during concurrent evacuation, the class mirror have been forwarded. + // In that case, class redefinition only nulls out only the Klass reference in the forwardee. The forwarded + // oop still contains the old, invalid Klass pointer. This should not matter since it is eventually + // reclaimed by GC, and all JVM code should consistently access the forwardee's Klass reference only. + // + // Here, we need to check the Klass reference in the forwardee version of the mirror oop. + + const Metadata* klass = fwd->metadata_field(java_lang_Class::klass_offset()); if (klass != nullptr && !Metaspace::contains(klass)) { print_failure(_safe_all, obj, interior_loc, nullptr, "Shenandoah assert_correct failed", "Instance class mirror should point to Metaspace", From 3d07a9d9e8d7790d25401067a0a3b945fb0878c8 Mon Sep 17 00:00:00 2001 From: tstuefe Date: Tue, 8 Jul 2025 16:24:06 +0200 Subject: [PATCH 2/4] also fix ShenandoahVerifier --- .../share/gc/shenandoah/shenandoahAsserts.cpp | 19 ++++++++----------- .../gc/shenandoah/shenandoahVerifier.cpp | 10 +++++++++- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp b/src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp index 037f344ee7208..7a08379da50b6 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp @@ -269,17 +269,14 @@ void ShenandoahAsserts::assert_correct(void* interior_loc, oop obj, const char* // We want to check class loading/unloading did not corrupt them. if (Universe::is_fully_initialized() && (obj_klass == vmClasses::Class_klass())) { - // Note: class redefinition involves creating a new Klass structure and a new associated mirror object. - // The old Klass is discarded and thus made invalid. The old class mirror's Klass field is nulled - // out during redefinition (correctly) - hence the need for the "klass != nullptr" condition below. - // - // However, if this is happening during concurrent evacuation, the class mirror have been forwarded. - // In that case, class redefinition only nulls out only the Klass reference in the forwardee. The forwarded - // oop still contains the old, invalid Klass pointer. This should not matter since it is eventually - // reclaimed by GC, and all JVM code should consistently access the forwardee's Klass reference only. - // - // Here, we need to check the Klass reference in the forwardee version of the mirror oop. - + // During class redefinition the old Klass gets reclaimed and the old mirror oop's Klass reference + // nulled out (hence the "klass != nullptr" condition below). However, the mirror oop may have been + // forwarded if we are in the mids of an evacuation. In that case, the forwardee's Klass reference + // is nulled out. The old, forwarded, still still carries the old invalid Klass pointer. It will be + // eventually collected. + // This checking code may encounter the old copy of the mirror, and its referee Klass pointer may + // already be reclaimed and therefore be invalid. We must therefore check the forwardee's Klass + // reference. const Metadata* klass = fwd->metadata_field(java_lang_Class::klass_offset()); if (klass != nullptr && !Metaspace::contains(klass)) { print_failure(_safe_all, obj, interior_loc, nullptr, "Shenandoah assert_correct failed", diff --git a/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp b/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp index cdf7848520765..2caea2b77f892 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp @@ -247,7 +247,15 @@ class ShenandoahVerifyOopClosure : public BasicOopIterateClosure { // We want to check class loading/unloading did not corrupt them. if (obj_klass == vmClasses::Class_klass()) { - Metadata* klass = obj->metadata_field(java_lang_Class::klass_offset()); + // During class redefinition the old Klass gets reclaimed and the old mirror oop's Klass reference + // nulled out (hence the "klass != nullptr" condition below). However, the mirror oop may have been + // forwarded if we are in the mids of an evacuation. In that case, the forwardee's Klass reference + // is nulled out. The old, forwarded, still still carries the old invalid Klass pointer. It will be + // eventually collected. + // This checking code may encounter the old copy of the mirror, and its referee Klass pointer may + // already be reclaimed and therefore be invalid. We must therefore check the forwardee's Klass + // reference. + Metadata* klass = fwd->metadata_field(java_lang_Class::klass_offset()); check(ShenandoahAsserts::_safe_oop, obj, klass == nullptr || Metaspace::contains(klass), "Instance class mirror should point to Metaspace"); From fba14a0d26b7fcb362c38e75c5f2b6c4447c0881 Mon Sep 17 00:00:00 2001 From: tstuefe Date: Wed, 9 Jul 2025 08:17:37 +0200 Subject: [PATCH 3/4] Also check array klass --- src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp | 6 +++--- src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp b/src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp index d415d5aedcbc8..e6da01c77bbd5 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp @@ -282,14 +282,14 @@ void ShenandoahAsserts::assert_correct(void* interior_loc, oop obj, const char* const Metadata* klass = fwd->metadata_field(java_lang_Class::klass_offset()); if (klass != nullptr && !Metaspace::contains(klass)) { print_failure(_safe_all, obj, interior_loc, nullptr, "Shenandoah assert_correct failed", - "Instance class mirror should point to Metaspace", + "Mirrored instance class should point to Metaspace", file, line); } - Metadata* array_klass = obj->metadata_field(java_lang_Class::array_klass_offset()); + const Metadata* array_klass = fwd->metadata_field(java_lang_Class::array_klass_offset()); if (array_klass != nullptr && !Metaspace::contains(array_klass)) { print_failure(_safe_all, obj, interior_loc, nullptr, "Shenandoah assert_correct failed", - "Array class mirror should point to Metaspace", + "Mirrored array class should point to Metaspace", file, line); } } diff --git a/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp b/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp index 2caea2b77f892..0c899844d8fa8 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp @@ -255,15 +255,15 @@ class ShenandoahVerifyOopClosure : public BasicOopIterateClosure { // This checking code may encounter the old copy of the mirror, and its referee Klass pointer may // already be reclaimed and therefore be invalid. We must therefore check the forwardee's Klass // reference. - Metadata* klass = fwd->metadata_field(java_lang_Class::klass_offset()); + const Metadata* klass = fwd->metadata_field(java_lang_Class::klass_offset()); check(ShenandoahAsserts::_safe_oop, obj, klass == nullptr || Metaspace::contains(klass), - "Instance class mirror should point to Metaspace"); + "Mirrored instance class should point to Metaspace"); - Metadata* array_klass = obj->metadata_field(java_lang_Class::array_klass_offset()); + const Metadata* array_klass = obj->metadata_field(java_lang_Class::array_klass_offset()); check(ShenandoahAsserts::_safe_oop, obj, array_klass == nullptr || Metaspace::contains(array_klass), - "Array class mirror should point to Metaspace"); + "Mirrored array class should point to Metaspace"); } // ------------ obj and fwd are safe at this point -------------- From c33cf248f5404094eec6a35c56da3c9fbf43e68c Mon Sep 17 00:00:00 2001 From: tstuefe Date: Thu, 10 Jul 2025 08:27:07 +0200 Subject: [PATCH 4/4] Alekseys additions --- .../share/gc/shenandoah/shenandoahAsserts.cpp | 16 +++++++--------- .../share/gc/shenandoah/shenandoahVerifier.cpp | 12 +++--------- 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp b/src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp index e6da01c77bbd5..7c9fa7598355f 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp @@ -77,6 +77,10 @@ void ShenandoahAsserts::print_obj(ShenandoahMessageBuffer& msg, oop obj) { } msg.append(" mark:%s\n", mw_ss.freeze()); msg.append(" region: %s", ss.freeze()); + if (obj_klass == vmClasses::Class_klass()) { + msg.append(" mirrored klass: " PTR_FORMAT "\n", p2i(obj->metadata_field(java_lang_Class::klass_offset()))); + msg.append(" mirrored array klass: " PTR_FORMAT "\n", p2i(obj->metadata_field(java_lang_Class::array_klass_offset()))); + } } void ShenandoahAsserts::print_non_obj(ShenandoahMessageBuffer& msg, void* loc) { @@ -268,17 +272,11 @@ void ShenandoahAsserts::assert_correct(void* interior_loc, oop obj, const char* } // Do additional checks for special objects: their fields can hold metadata as well. - // We want to check class loading/unloading did not corrupt them. + // We want to check class loading/unloading did not corrupt them. We can only reasonably + // trust the forwarded objects, as the from-space object can have the klasses effectively + // dead. if (Universe::is_fully_initialized() && (obj_klass == vmClasses::Class_klass())) { - // During class redefinition the old Klass gets reclaimed and the old mirror oop's Klass reference - // nulled out (hence the "klass != nullptr" condition below). However, the mirror oop may have been - // forwarded if we are in the mids of an evacuation. In that case, the forwardee's Klass reference - // is nulled out. The old, forwarded, still still carries the old invalid Klass pointer. It will be - // eventually collected. - // This checking code may encounter the old copy of the mirror, and its referee Klass pointer may - // already be reclaimed and therefore be invalid. We must therefore check the forwardee's Klass - // reference. const Metadata* klass = fwd->metadata_field(java_lang_Class::klass_offset()); if (klass != nullptr && !Metaspace::contains(klass)) { print_failure(_safe_all, obj, interior_loc, nullptr, "Shenandoah assert_correct failed", diff --git a/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp b/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp index 0c899844d8fa8..727b90e82a2a5 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp @@ -244,17 +244,11 @@ class ShenandoahVerifyOopClosure : public BasicOopIterateClosure { } // Do additional checks for special objects: their fields can hold metadata as well. - // We want to check class loading/unloading did not corrupt them. + // We want to check class loading/unloading did not corrupt them. We can only reasonably + // trust the forwarded objects, as the from-space object can have the klasses effectively + // dead. if (obj_klass == vmClasses::Class_klass()) { - // During class redefinition the old Klass gets reclaimed and the old mirror oop's Klass reference - // nulled out (hence the "klass != nullptr" condition below). However, the mirror oop may have been - // forwarded if we are in the mids of an evacuation. In that case, the forwardee's Klass reference - // is nulled out. The old, forwarded, still still carries the old invalid Klass pointer. It will be - // eventually collected. - // This checking code may encounter the old copy of the mirror, and its referee Klass pointer may - // already be reclaimed and therefore be invalid. We must therefore check the forwardee's Klass - // reference. const Metadata* klass = fwd->metadata_field(java_lang_Class::klass_offset()); check(ShenandoahAsserts::_safe_oop, obj, klass == nullptr || Metaspace::contains(klass),