8352508: [Redo] G1: Pinned regions with pinned objects only reachable by native code crash VM

Reviewed-by: ayang, iwalulya

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp

@@ -1273,7 +1273,8 @@ class G1UpdateRegionLivenessAndSelectForRebuildTask : public WorkerTask {
         // The liveness of this humongous obj decided by either its allocation
         // time (allocated after conc-mark-start, i.e. live) or conc-marking.
         const bool is_live = _cm->top_at_mark_start(hr) == hr->bottom()
-                             || _cm->contains_live_object(hr->hrm_index());
+                          || _cm->contains_live_object(hr->hrm_index())
+                          || hr->has_pinned_objects();
         if (is_live) {
           const bool selected_for_rebuild = tracker->update_humongous_before_rebuild(hr);
           auto on_humongous_region = [&] (G1HeapRegion* hr) {
@@ -1291,7 +1292,9 @@ class G1UpdateRegionLivenessAndSelectForRebuildTask : public WorkerTask {
         uint region_idx = hr->hrm_index();
         hr->note_end_of_marking(_cm->top_at_mark_start(hr), _cm->live_bytes(region_idx), _cm->incoming_refs(region_idx));
 
-        if (hr->live_bytes() != 0) {
+        const bool is_live = hr->live_bytes() != 0
+                          || hr->has_pinned_objects();
+        if (is_live) {
           if (tracker->update_old_before_rebuild(hr)) {
             _num_selected_for_rebuild++;
           }

src/hotspot/share/gc/g1/g1ParScanThreadState.cpp

@@ -614,6 +614,12 @@ void G1ParScanThreadStateSet::record_unused_optional_region(G1HeapRegion* hr) {
   }
 }
 
+void G1ParScanThreadState::record_evacuation_failed_region(G1HeapRegion* r, uint worker_id, bool cause_pinned) {
+  if (_evac_failure_regions->record(worker_id, r->hrm_index(), cause_pinned)) {
+    G1HeapRegionPrinter::evac_failure(r);
+  }
+}
+
 NOINLINE
 oop G1ParScanThreadState::handle_evacuation_failure_par(oop old, markWord m, size_t word_sz, bool cause_pinned) {
   assert(_g1h->is_in_cset(old), "Object " PTR_FORMAT " should be in the CSet", p2i(old));
@@ -623,9 +629,7 @@ oop G1ParScanThreadState::handle_evacuation_failure_par(oop old, markWord m, siz
     // Forward-to-self succeeded. We are the "owner" of the object.
     G1HeapRegion* r = _g1h->heap_region_containing(old);
 
-    if (_evac_failure_regions->record(_worker_id, r->hrm_index(), cause_pinned)) {
-      G1HeapRegionPrinter::evac_failure(r);
-    }
+    record_evacuation_failed_region(r, _worker_id, cause_pinned);
 
     // Mark the failing object in the marking bitmap and later use the bitmap to handle
     // evacuation failure recovery.

src/hotspot/share/gc/g1/g1ParScanThreadState.hpp

@@ -228,6 +228,7 @@ public:
   Tickspan trim_ticks() const;
   void reset_trim_ticks();
 
+  void record_evacuation_failed_region(G1HeapRegion* r, uint worker_id, bool cause_pinned);
   // An attempt to evacuate "obj" has failed; take necessary steps.
   oop handle_evacuation_failure_par(oop obj, markWord m, size_t word_sz, bool cause_pinned);
 

src/hotspot/share/gc/g1/g1Policy.cpp

@@ -660,13 +660,6 @@ void G1Policy::record_concurrent_refinement_stats(size_t pending_cards,
 
 bool G1Policy::should_retain_evac_failed_region(uint index) const {
   size_t live_bytes = _g1h->region_at(index)->live_bytes();
-
-#ifdef ASSERT
-  G1HeapRegion* r = _g1h->region_at(index);
-  assert(live_bytes != 0,
-         "live bytes not set for %u used %zu garbage %zu cm-live %zu pinned %d",
-         index, r->used(), r->garbage_bytes(), live_bytes, r->has_pinned_objects());
-#endif
   size_t threshold = G1RetainRegionLiveThresholdPercent * G1HeapRegion::GrainBytes / 100;
   return live_bytes < threshold;
 }

src/hotspot/share/gc/g1/g1YoungCollector.cpp

@@ -587,6 +587,30 @@ public:
 };
 
 class G1EvacuateRegionsBaseTask : public WorkerTask {
+
+  // All pinned regions in the collection set must be registered as failed
+  // regions as there is no guarantee that there is a reference reachable by
+  // Java code (i.e. only by native code) that adds it to the evacuation failed
+  // regions.
+  void record_pinned_regions(G1ParScanThreadState* pss, uint worker_id) {
+    class RecordPinnedRegionClosure : public G1HeapRegionClosure {
+      G1ParScanThreadState* _pss;
+      uint _worker_id;
+
+    public:
+      RecordPinnedRegionClosure(G1ParScanThreadState* pss, uint worker_id) : _pss(pss), _worker_id(worker_id) { }
+
+      bool do_heap_region(G1HeapRegion* r) {
+        if (r->has_pinned_objects()) {
+          _pss->record_evacuation_failed_region(r, _worker_id, true /* cause_pinned */);
+        }
+        return false;
+      }
+    } cl(pss, worker_id);
+
+    _g1h->collection_set_iterate_increment_from(&cl, worker_id);
+  }
+
 protected:
   G1CollectedHeap* _g1h;
   G1ParScanThreadStateSet* _per_thread_states;
@@ -594,8 +618,6 @@ protected:
   G1ScannerTasksQueueSet* _task_queues;
   TaskTerminator _terminator;
 
-  uint _num_workers;
-
   void evacuate_live_objects(G1ParScanThreadState* pss,
                              uint worker_id,
                              G1GCPhaseTimes::GCParPhases objcopy_phase,
@@ -631,6 +653,9 @@ protected:
 
   virtual void evacuate_live_objects(G1ParScanThreadState* pss, uint worker_id) = 0;
 
+private:
+  volatile bool _pinned_regions_recorded;
+
 public:
   G1EvacuateRegionsBaseTask(const char* name,
                             G1ParScanThreadStateSet* per_thread_states,
@@ -641,7 +666,7 @@ public:
     _per_thread_states(per_thread_states),
     _task_queues(task_queues),
     _terminator(num_workers, _task_queues),
-    _num_workers(num_workers)
+    _pinned_regions_recorded(false)
   { }
 
   void work(uint worker_id) {
@@ -653,6 +678,9 @@ public:
       G1ParScanThreadState* pss = _per_thread_states->state_for_worker(worker_id);
       pss->set_ref_discoverer(_g1h->ref_processor_stw());
 
+      if (!Atomic::cmpxchg(&_pinned_regions_recorded, false, true)) {
+        record_pinned_regions(pss, worker_id);
+      }
       scan_roots(pss, worker_id);
       evacuate_live_objects(pss, worker_id);
     }

test/hotspot/jtreg/gc/g1/pinnedobjs/TestPinnedEvacEmpty.java

@@ -0,0 +1,112 @@
+/*
+ * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/* @test
+ * @bug 8351921 8352508
+ * @summary Test that pinned regions with no Java references into them
+ *          do not make the garbage collector reclaim that region.
+ *          This test simulates this behavior using Whitebox methods
+ *          to pin a Java object in a region with no other pinnable objects and
+*           lose the reference to it before the garbage collection.
+ * @requires vm.gc.G1
+ * @requires vm.debug
+ * @library /test/lib /
+ * @modules java.management
+ * @build jdk.test.whitebox.WhiteBox
+ * @run driver jdk.test.lib.helpers.ClassFileInstaller jdk.test.whitebox.WhiteBox
+ * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -XX:+UseG1GC
+ *      -Xbootclasspath/a:. -Xlog:gc=debug,gc+ergo+cset=trace,gc+phases=debug -XX:G1HeapRegionSize=1m -Xms30m  gc.g1.pinnedobjs.TestPinnedEvacEmpty regular
+ *
+ * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -XX:+UseG1GC
+ *      -Xbootclasspath/a:. -Xlog:gc=debug,gc+ergo+cset=trace,gc+phases=debug -XX:G1HeapRegionSize=1m -Xms30m  gc.g1.pinnedobjs.TestPinnedEvacEmpty humongous
+ */
+
+package gc.g1.pinnedobjs;
+
+import gc.testlibrary.Helpers;
+
+import jdk.test.lib.Asserts;
+import jdk.test.lib.process.OutputAnalyzer;
+import jdk.test.lib.process.ProcessTools;
+import jdk.test.whitebox.WhiteBox;
+
+public class TestPinnedEvacEmpty {
+
+    private static final WhiteBox wb = WhiteBox.getWhiteBox();
+
+    private static final int objSize = (int)wb.getObjectSize(new Object());
+
+    private static Object allocHumongous() {
+        final int M = 1024 * 1024;
+        // The target object to pin. Since it is humongous, it will always be in its
+        // own regions.
+        return new int[M];
+    }
+
+    private static Object allocRegular() {
+        // How many j.l.Object should we allocate when creating garbage.
+        final int numAllocations = 1024 * 1024 * 3 / objSize;
+
+        // Allocate garbage so that the target object will be in a new region.
+        for (int i = 0; i < numAllocations; i++) {
+          Object z = new Object();
+        }
+        int[] o = new int[100];  // The target object to pin.
+        // Further allocate garbage so that any additional allocations of potentially
+        // pinned objects can not be allocated in the same region as the target object.
+        for (int i = 0; i < numAllocations; i++) {
+          Object z = new Object();
+        }
+
+        Asserts.assertTrue(!wb.isObjectInOldGen(o), "should not be in old gen already");
+        return o;
+    }
+
+    public static void main(String[] args) throws Exception {
+        System.out.println("Testing " + args[0] + " objects...");
+
+        // Remove garbage from VM initialization.
+        wb.fullGC();
+
+        // Allocate target object according to arguments.
+        Object o = args[0].equals("regular") ? allocRegular() : allocHumongous();
+
+        // Pin the object.
+        wb.pinObject(o);
+
+        // And forget the (Java) reference to the int array. After this, the garbage
+        // collection should find a completely empty pinned region. The collector
+        // must not collect/free it.
+        o = null;
+
+        // Full collection should not crash the VM in case of "empty" pinned regions.
+        wb.fullGC();
+
+        // Do a young garbage collection to zap the data in the pinned region. This test
+        // achieves that by executing a concurrent cycle that both performs both a young
+        // garbage collection as well as checks that errorneous reclamation does not occur
+        // in the Remark pause.
+        wb.g1RunConcurrentGC();
+        System.out.println("Done");
+    }
+}