From 2d606a6215a4911d7679f85a4905cf2f02b405f7 Mon Sep 17 00:00:00 2001 From: Xiaolong Peng Date: Fri, 16 Jan 2026 16:24:22 -0800 Subject: [PATCH] Enforce memory ordering for read/write of _atomic_top to fix crash on MacOS --- .../gc/shenandoah/shenandoahAllocator.cpp | 4 +++ .../share/gc/shenandoah/shenandoahFreeSet.cpp | 2 ++ .../shenandoahGenerationalEvacuationTask.cpp | 1 + .../gc/shenandoah/shenandoahHeapRegion.hpp | 34 +++++++++++++------ .../shenandoahHeapRegion.inline.hpp | 13 ++++--- 5 files changed, 39 insertions(+), 15 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahAllocator.cpp b/src/hotspot/share/gc/shenandoah/shenandoahAllocator.cpp index ad7d7026803..b8529d66e31 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahAllocator.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahAllocator.cpp @@ -256,6 +256,7 @@ HeapWord* ShenandoahAllocator::allocate_in(ShenandoahHeapRegion assert(ready_for_retire == false, "Sanity check"); if (!IS_SHARED_ALLOC_REGION) { shenandoah_assert_heaplocked(); + assert(!region->is_active_alloc_region(), "Must not"); } HeapWord* obj = nullptr; size_t actual_size = req.size(); @@ -339,6 +340,9 @@ int ShenandoahAllocator::refresh_alloc_regions(ShenandoahAllocR satisfy_alloc_req_first = false; } reserved[i]->set_active_alloc_region(); + // Enforce order here, + // set_active_alloc_region must be executed before storing the region to the shared address + OrderAccess::fence(); log_debug(gc, alloc)("%sAllocator: Storing heap region %li to alloc region %i", _alloc_partition_name, reserved[i]->index(), refreshable[i]->alloc_region_index); AtomicAccess::store(&refreshable[i]->address, reserved[i]); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp b/src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp index 80e28980f58..bbf03e169c2 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp @@ -3101,6 +3101,7 @@ int ShenandoahFreeSet::reserve_alloc_regions_internal(Iter iterator, int const r for (idx_t idx = iterator.current(); iterator.has_next() && reserved_regions_count < regions_to_reserve; idx = iterator.next()) { ShenandoahHeapRegion* r = _heap->get_region(idx); + assert(r->get_top_before_promote() == nullptr, "Must not be PIP region"); if (_heap->is_concurrent_weak_root_in_progress() && r->is_trash()) { continue; } @@ -3234,6 +3235,7 @@ ShenandoahHeapRegion* ShenandoahFreeSet::find_heap_region_for_allocation_interna bool const use_affiliated_region_first = ALLOC_PARTITION != ShenandoahFreeSetPartitionId::Mutator; for (idx_t idx = iterator.current(); iterator.has_next(); idx = iterator.next()) { ShenandoahHeapRegion* r = _heap->get_region(idx); + assert(r->get_top_before_promote() == nullptr, "Must not be PIP region"); if (r->is_trash() && _heap->is_concurrent_weak_root_in_progress()) { continue; } diff --git a/src/hotspot/share/gc/shenandoah/shenandoahGenerationalEvacuationTask.cpp b/src/hotspot/share/gc/shenandoah/shenandoahGenerationalEvacuationTask.cpp index a6248848b33..cc5796684b7 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahGenerationalEvacuationTask.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahGenerationalEvacuationTask.cpp @@ -186,6 +186,7 @@ void ShenandoahGenerationalEvacuationTask::promote_in_place(ShenandoahHeapRegion assert(region->is_regular(), "Use different service to promote humongous regions"); assert(_heap->is_tenurable(region), "Only promote regions that are sufficiently aged"); assert(region->get_top_before_promote() == tams, "Region %zu has been used for allocations before promotion", region->index()); + assert(!region->is_active_alloc_region(), "Must not be atomic alloc region"); } ShenandoahOldGeneration* const old_gen = _heap->old_generation(); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.hpp b/src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.hpp index 9a6e33377c7..b62e545b480 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.hpp @@ -459,14 +459,21 @@ public: ShenandoahHeapRegion* humongous_start_region() const; HeapWord* atomic_top() const { - return AtomicAccess::load(&_atomic_top); + return AtomicAccess::load_acquire(&_atomic_top); } + template HeapWord* top() const { - HeapWord* v_top = atomic_top(); - return v_top == nullptr ? AtomicAccess::load(&_top) : v_top; + if (CHECK_ATOMIC_TOP) { + HeapWord* v_top = atomic_top(); + return v_top == nullptr ? AtomicAccess::load(&_top) : v_top; + } + assert(atomic_top() == nullptr, "Must be"); + return AtomicAccess::load(&_top); } + void set_top(HeapWord* v) { + assert(!is_active_alloc_region(), "Sanity check"); AtomicAccess::store(&_top, v); } @@ -555,10 +562,11 @@ public: } inline void set_active_alloc_region() { + shenandoah_assert_heaplocked(); assert(_active_alloc_region.is_unset(), "Must be"); // Sync _top to _atomic_top before setting _active_alloc_region flag to prepare for CAS allocation assert(atomic_top() == nullptr, "Must be"); - AtomicAccess::store(&_atomic_top, _top); + AtomicAccess::release_store(&_atomic_top, top()); _active_alloc_region.set(); } @@ -572,17 +580,21 @@ public: // Before unset _active_alloc_region flag, _atomic_top needs to be set to sentinel value using AtomicAccess::cmpxchg, // this avoids race condition when the alloc region removed from the alloc regions array used by lock-free allocation in allocator; // meanwhile the previous value of _atomic_top needs to be synced back to _top. - HeapWord* previous_atomic_top = nullptr; + HeapWord* prior_atomic_top = nullptr; + HeapWord* current_atomic_top = atomic_top(); while (true /*always break out in the loop*/) { - previous_atomic_top = atomic_top(); - assert(previous_atomic_top != nullptr, "Must not"); - set_top(previous_atomic_top); // Sync current _atomic_top back to _top - if (AtomicAccess::cmpxchg(&_atomic_top, previous_atomic_top, (HeapWord*) nullptr) == previous_atomic_top) { - // break when successfully exchange _atomic_top to nullptr + assert(current_atomic_top != nullptr, "Must not"); + AtomicAccess::store(&_top, current_atomic_top); // Sync current _atomic_top back to _top + OrderAccess::fence(); + prior_atomic_top = AtomicAccess::cmpxchg(&_atomic_top, current_atomic_top, (HeapWord*) nullptr, memory_order_release); + if (prior_atomic_top == current_atomic_top) { + // break out the loop when successfully exchange _atomic_top to nullptr break; } + current_atomic_top = prior_atomic_top; } - assert(_top == previous_atomic_top, "Must be"); + OrderAccess::fence(); + assert(top() == current_atomic_top, "Value of _atomic_top must have synced to _top."); _active_alloc_region.unset(); } diff --git a/src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.inline.hpp index c6879cc716f..3b3865b57a3 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.inline.hpp @@ -107,9 +107,10 @@ HeapWord* ShenandoahHeapRegion::allocate_fill(size_t size) { HeapWord* ShenandoahHeapRegion::allocate(size_t size, const ShenandoahAllocRequest& req) { shenandoah_assert_heaplocked_or_safepoint(); + assert(!is_active_alloc_region(), "Must not"); assert(is_object_aligned(size), "alloc size breaks alignment: %zu", size); - HeapWord* obj = top(); + HeapWord* obj = top(); if (pointer_delta(end(), obj) >= size) { make_regular_allocation(req.affiliation()); adjust_alloc_metadata(req, size); @@ -133,7 +134,7 @@ HeapWord* ShenandoahHeapRegion::allocate_lab(const ShenandoahAllocRequest& req, size_t adjusted_size = req.size(); HeapWord* obj = nullptr; - HeapWord* old_top = top(); + HeapWord* old_top = top(); size_t free_words = align_down(byte_size(old_top, end()) >> LogHeapWordSize, MinObjAlignment); if (adjusted_size > free_words) { adjusted_size = free_words; @@ -238,7 +239,7 @@ HeapWord* ShenandoahHeapRegion::allocate_lab_atomic(const ShenandoahAllocRequest bool ShenandoahHeapRegion::try_allocate(HeapWord* const obj, size_t const size, HeapWord* &prior_atomic_top) { HeapWord* new_top = obj + size; - if ((prior_atomic_top = AtomicAccess::cmpxchg(&_atomic_top, obj, new_top)) == obj) { + if ((prior_atomic_top = AtomicAccess::cmpxchg(&_atomic_top, obj, new_top, memory_order_release)) == obj) { assert(is_object_aligned(new_top), "new top breaks alignment: " PTR_FORMAT, p2i(new_top)); assert(is_object_aligned(obj), "obj is not aligned: " PTR_FORMAT, p2i(obj)); return true; @@ -357,10 +358,14 @@ inline bool ShenandoahHeapRegion::is_affiliated() const { } inline void ShenandoahHeapRegion::save_top_before_promote() { - _top_before_promoted = top(); + assert(!is_active_alloc_region(), "Must not"); + assert(atomic_top() == nullptr, "Must be"); + _top_before_promoted = top(); } inline void ShenandoahHeapRegion::restore_top_before_promote() { + assert(!is_active_alloc_region(), "Must not"); + assert(atomic_top() == nullptr, "Must be"); _top = _top_before_promoted; _top_before_promoted = nullptr; }