From 6a7dff30edce7a24400b27bee4d7ddd45eed523d Mon Sep 17 00:00:00 2001 From: Stefan Karlsson Date: Thu, 20 Apr 2023 09:18:28 +0000 Subject: [PATCH] 8305880: Loom: Avoid putting stale object pointers in oops Reviewed-by: eosterlund, aboldtch --- src/hotspot/share/compiler/oopMap.cpp | 12 +++---- src/hotspot/share/compiler/oopMap.hpp | 9 +++--- src/hotspot/share/compiler/oopMap.inline.hpp | 6 ++-- src/hotspot/share/memory/iterator.hpp | 3 +- src/hotspot/share/oops/stackChunkOop.cpp | 31 ++++++++++++------- .../runtime/continuationWrapper.inline.hpp | 6 ++-- src/hotspot/share/runtime/frame.cpp | 8 ++--- .../runtime/stackChunkFrameStream.inline.hpp | 2 +- src/hotspot/share/utilities/devirtualizer.hpp | 2 +- .../share/utilities/devirtualizer.inline.hpp | 6 ++-- 10 files changed, 46 insertions(+), 39 deletions(-) diff --git a/src/hotspot/share/compiler/oopMap.cpp b/src/hotspot/share/compiler/oopMap.cpp index a6b8a156450..58e9daa43a5 100644 --- a/src/hotspot/share/compiler/oopMap.cpp +++ b/src/hotspot/share/compiler/oopMap.cpp @@ -392,7 +392,7 @@ class AddDerivedOop : public DerivedOopClosure { SkipNull = true, NeedsLock = true }; - virtual void do_derived_oop(oop* base, derived_pointer* derived) { + virtual void do_derived_oop(derived_base* base, derived_pointer* derived) { #if COMPILER2_OR_JVMCI DerivedPointerTable::add(derived, base); #endif // COMPILER2_OR_JVMCI @@ -410,7 +410,7 @@ public: SkipNull = true, NeedsLock = true }; - virtual void do_derived_oop(oop* base, derived_pointer* derived) { + virtual void do_derived_oop(derived_base* base, derived_pointer* derived) { // All derived pointers must be processed before the base pointer of any derived pointer is processed. // Otherwise, if two derived pointers use the same base, the second derived pointer will get an obscured // offset, if the base pointer is processed in the first derived pointer. @@ -430,7 +430,7 @@ public: SkipNull = true, NeedsLock = true }; - virtual void do_derived_oop(oop* base, derived_pointer* derived) {} + virtual void do_derived_oop(derived_base* base, derived_pointer* derived) {} }; void OopMapSet::oops_do(const frame* fr, const RegisterMap* reg_map, OopClosure* f, DerivedPointerIterationMode mode) { @@ -915,8 +915,8 @@ void DerivedPointerTable::clear() { _active = true; } -void DerivedPointerTable::add(derived_pointer* derived_loc, oop *base_loc) { - assert(Universe::heap()->is_in_or_null(*base_loc), "not an oop"); +void DerivedPointerTable::add(derived_pointer* derived_loc, derived_base* base_loc) { + assert(Universe::heap()->is_in_or_null((void*)*base_loc), "not an oop"); assert(derived_loc != (void*)base_loc, "Base and derived in same location"); derived_pointer base_loc_as_derived_pointer = static_cast(reinterpret_cast(base_loc)); @@ -933,7 +933,7 @@ void DerivedPointerTable::add(derived_pointer* derived_loc, oop *base_loc) { "Add derived pointer@" INTPTR_FORMAT " - Derived: " INTPTR_FORMAT " Base: " INTPTR_FORMAT " (@" INTPTR_FORMAT ") (Offset: " INTX_FORMAT ")", - p2i(derived_loc), derived_pointer_value(*derived_loc), p2i(*base_loc), p2i(base_loc), offset + p2i(derived_loc), derived_pointer_value(*derived_loc), intptr_t(*base_loc), p2i(base_loc), offset ); } // Set derived oop location to point to base. diff --git a/src/hotspot/share/compiler/oopMap.hpp b/src/hotspot/share/compiler/oopMap.hpp index 3c23943e097..4b20464f69c 100644 --- a/src/hotspot/share/compiler/oopMap.hpp +++ b/src/hotspot/share/compiler/oopMap.hpp @@ -48,6 +48,7 @@ class OopClosure; class CodeBlob; class ImmutableOopMap; +enum class derived_base : intptr_t {}; enum class derived_pointer : intptr_t {}; class OopMapValue: public StackObj { @@ -481,12 +482,12 @@ class DerivedPointerTable : public AllStatic { friend class VMStructs; private: class Entry; - static bool _active; // do not record pointers for verify pass etc. + static bool _active; // do not record pointers for verify pass etc. public: - static void clear(); // Called before scavenge/GC - static void add(derived_pointer* derived, oop *base); // Called during scavenge/GC - static void update_pointers(); // Called after scavenge/GC + static void clear(); // Called before scavenge/GC + static void add(derived_pointer* derived, derived_base* base); // Called during scavenge/GC + static void update_pointers(); // Called after scavenge/GC static bool is_empty(); static bool is_active() { return _active; } static void set_active(bool value) { _active = value; } diff --git a/src/hotspot/share/compiler/oopMap.inline.hpp b/src/hotspot/share/compiler/oopMap.inline.hpp index 3f8a81d773b..c6531b1cd3a 100644 --- a/src/hotspot/share/compiler/oopMap.inline.hpp +++ b/src/hotspot/share/compiler/oopMap.inline.hpp @@ -84,15 +84,15 @@ void OopMapDo::iterate_oops_do(const frame } guarantee(loc != nullptr, "missing saved register"); derived_pointer* derived_loc = (derived_pointer*)loc; - void** base_loc = (void**) fr->oopmapreg_to_location(omv.content_reg(), reg_map); + derived_base* base_loc = (derived_base*) fr->oopmapreg_to_location(omv.content_reg(), reg_map); // Ignore nullptr oops and decoded null narrow oops which // equal to CompressedOops::base() when a narrow oop // implicit null check is used in compiled code. // The narrow_oop_base could be nullptr or be the address // of the page below heap depending on compressed oops mode. - if (base_loc != nullptr && !SkipNullValue::should_skip(*base_loc)) { - _derived_oop_fn->do_derived_oop((oop*)base_loc, derived_loc); + if (base_loc != nullptr && !SkipNullValue::should_skip((void*)*base_loc)) { + _derived_oop_fn->do_derived_oop(base_loc, derived_loc); } } } diff --git a/src/hotspot/share/memory/iterator.hpp b/src/hotspot/share/memory/iterator.hpp index eee59334757..5d5c494ef11 100644 --- a/src/hotspot/share/memory/iterator.hpp +++ b/src/hotspot/share/memory/iterator.hpp @@ -129,11 +129,12 @@ public: virtual void oops_do(OopClosure* cl) = 0; }; +enum class derived_base : intptr_t; enum class derived_pointer : intptr_t; class DerivedOopClosure : public Closure { public: enum { SkipNull = true }; - virtual void do_derived_oop(oop* base, derived_pointer* derived) = 0; + virtual void do_derived_oop(derived_base* base, derived_pointer* derived) = 0; }; class KlassClosure : public Closure { diff --git a/src/hotspot/share/oops/stackChunkOop.cpp b/src/hotspot/share/oops/stackChunkOop.cpp index 76e48cba53d..4e771939dc9 100644 --- a/src/hotspot/share/oops/stackChunkOop.cpp +++ b/src/hotspot/share/oops/stackChunkOop.cpp @@ -38,6 +38,11 @@ #include "runtime/smallRegisterMap.inline.hpp" #include "runtime/stackChunkFrameStream.inline.hpp" +// Note: Some functions in this file work with stale object pointers, e.g. +// DerivedPointerSupport. Be extra careful to not put those pointers into +// variables of the 'oop' type. There's extra GC verification around oops +// that may fail when stale oops are being used. + template class FrameOopIterator : public OopIterator { private: @@ -153,43 +158,45 @@ template void stackChunkOopDesc::do_barriers(base); + uintptr_t offset = derived_int_val - base; *(uintptr_t*)derived_loc = offset; } - static void derelativize(oop* base_loc, derived_pointer* derived_loc) { - oop base = *base_loc; - if (base == nullptr) { + static void derelativize(derived_base* base_loc, derived_pointer* derived_loc) { + uintptr_t base = *(uintptr_t*)base_loc; + if (base == 0) { return; } - assert(!UseCompressedOops || !CompressedOops::is_base(base), ""); + assert(!UseCompressedOops || !CompressedOops::is_base((void*)base), ""); // All derived pointers should have been relativized into offsets uintptr_t offset = *(uintptr_t*)derived_loc; // Restore the original derived pointer - *(uintptr_t*)derived_loc = cast_from_oop(base) + offset; + *(uintptr_t*)derived_loc = base + offset; } struct RelativizeClosure : public DerivedOopClosure { - virtual void do_derived_oop(oop* base_loc, derived_pointer* derived_loc) override { + virtual void do_derived_oop(derived_base* base_loc, derived_pointer* derived_loc) override { DerivedPointersSupport::relativize(base_loc, derived_loc); } }; struct DerelativizeClosure : public DerivedOopClosure { - virtual void do_derived_oop(oop* base_loc, derived_pointer* derived_loc) override { + virtual void do_derived_oop(derived_base* base_loc, derived_pointer* derived_loc) override { DerivedPointersSupport::derelativize(base_loc, derived_loc); } }; diff --git a/src/hotspot/share/runtime/continuationWrapper.inline.hpp b/src/hotspot/share/runtime/continuationWrapper.inline.hpp index fc89d98be64..03b2c726a0e 100644 --- a/src/hotspot/share/runtime/continuationWrapper.inline.hpp +++ b/src/hotspot/share/runtime/continuationWrapper.inline.hpp @@ -92,10 +92,8 @@ public: } inline ~SafepointOp() { // reload oops _cont._continuation = _conth(); - if (_cont._tail != nullptr) { - _cont._tail = jdk_internal_vm_Continuation::tail(_cont._continuation); - } - _cont.disallow_safepoint(); + _cont._tail = jdk_internal_vm_Continuation::tail(_cont._continuation); + _cont.disallow_safepoint(); } }; diff --git a/src/hotspot/share/runtime/frame.cpp b/src/hotspot/share/runtime/frame.cpp index bb4a6cc2e54..d4e7c26f18b 100644 --- a/src/hotspot/share/runtime/frame.cpp +++ b/src/hotspot/share/runtime/frame.cpp @@ -1241,7 +1241,7 @@ class FrameValuesOopClosure: public OopClosure, public DerivedOopClosure { private: GrowableArray* _oops; GrowableArray* _narrow_oops; - GrowableArray* _base; + GrowableArray* _base; GrowableArray* _derived; NoSafepointVerifier nsv; @@ -1249,7 +1249,7 @@ public: FrameValuesOopClosure() { _oops = new (mtThread) GrowableArray(100, mtThread); _narrow_oops = new (mtThread) GrowableArray(100, mtThread); - _base = new (mtThread) GrowableArray(100, mtThread); + _base = new (mtThread) GrowableArray(100, mtThread); _derived = new (mtThread) GrowableArray(100, mtThread); } ~FrameValuesOopClosure() { @@ -1261,7 +1261,7 @@ public: virtual void do_oop(oop* p) override { _oops->push(p); } virtual void do_oop(narrowOop* p) override { _narrow_oops->push(p); } - virtual void do_derived_oop(oop* base_loc, derived_pointer* derived_loc) override { + virtual void do_derived_oop(derived_base* base_loc, derived_pointer* derived_loc) override { _base->push(base_loc); _derived->push(derived_loc); } @@ -1281,7 +1281,7 @@ public: } assert(_base->length() == _derived->length(), "should be the same"); for (int i = 0; i < _base->length(); i++) { - oop* base = _base->at(i); + derived_base* base = _base->at(i); derived_pointer* derived = _derived->at(i); values.describe(frame_no, (intptr_t*)derived, err_msg("derived pointer (base: " INTPTR_FORMAT ") for #%d", p2i(base), frame_no)); } diff --git a/src/hotspot/share/runtime/stackChunkFrameStream.inline.hpp b/src/hotspot/share/runtime/stackChunkFrameStream.inline.hpp index e5d300b0016..0a279c57385 100644 --- a/src/hotspot/share/runtime/stackChunkFrameStream.inline.hpp +++ b/src/hotspot/share/runtime/stackChunkFrameStream.inline.hpp @@ -410,7 +410,7 @@ inline void StackChunkFrameStream::iterate_derived_pointers(DerivedO assert(is_in_oops(base_loc, map), "not found: " INTPTR_FORMAT, p2i(base_loc)); assert(!is_in_oops(derived_loc, map), "found: " INTPTR_FORMAT, p2i(derived_loc)); - Devirtualizer::do_derived_oop(closure, (oop*)base_loc, (derived_pointer*)derived_loc); + Devirtualizer::do_derived_oop(closure, (derived_base*)base_loc, (derived_pointer*)derived_loc); } } diff --git a/src/hotspot/share/utilities/devirtualizer.hpp b/src/hotspot/share/utilities/devirtualizer.hpp index 813a56d3740..b4d444dc5a8 100644 --- a/src/hotspot/share/utilities/devirtualizer.hpp +++ b/src/hotspot/share/utilities/devirtualizer.hpp @@ -38,7 +38,7 @@ class Devirtualizer { template static void do_klass(OopClosureType* closure, Klass* k); template static void do_cld(OopClosureType* closure, ClassLoaderData* cld); template static bool do_metadata(OopClosureType* closure); - template static void do_derived_oop(DerivedOopClosureType* closure, oop* base, derived_pointer* derived); + template static void do_derived_oop(DerivedOopClosureType* closure, derived_base* base, derived_pointer* derived); template static bool do_bit(BitMapClosureType* closure, BitMap::idx_t index); }; diff --git a/src/hotspot/share/utilities/devirtualizer.inline.hpp b/src/hotspot/share/utilities/devirtualizer.inline.hpp index 001596874cc..0cae27f87e9 100644 --- a/src/hotspot/share/utilities/devirtualizer.inline.hpp +++ b/src/hotspot/share/utilities/devirtualizer.inline.hpp @@ -154,18 +154,18 @@ void Devirtualizer::do_cld(OopClosureType* closure, ClassLoaderData* cld) { template static typename EnableIf::value, void>::type -call_do_derived_oop(void (Receiver::*)(oop*, derived_pointer*), void (Base::*)(oop*, derived_pointer*), DerivedOopClosureType* closure, oop* base, derived_pointer* derived) { +call_do_derived_oop(void (Receiver::*)(derived_base*, derived_pointer*), void (Base::*)(derived_base*, derived_pointer*), DerivedOopClosureType* closure, derived_base* base, derived_pointer* derived) { closure->do_derived_oop(base, derived); } template static typename EnableIf::value, void>::type -call_do_derived_oop(void (Receiver::*)(oop*, derived_pointer*), void (Base::*)(oop*, derived_pointer*), DerivedOopClosureType* closure, oop* base, derived_pointer* derived) { +call_do_derived_oop(void (Receiver::*)(derived_base*, derived_pointer*), void (Base::*)(derived_base*, derived_pointer*), DerivedOopClosureType* closure, derived_base* base, derived_pointer* derived) { closure->DerivedOopClosureType::do_derived_oop(base, derived); } template -inline void Devirtualizer::do_derived_oop(DerivedOopClosureType* closure, oop* base, derived_pointer* derived) { +inline void Devirtualizer::do_derived_oop(DerivedOopClosureType* closure, derived_base* base, derived_pointer* derived) { call_do_derived_oop(&DerivedOopClosureType::do_derived_oop, &DerivedOopClosure::do_derived_oop, closure, base, derived); }