8196485: FromCardCache default card index can cause crashes

The default value of -1 for 32 bit card indices is a regular card value at the border of 2TB heap addresses in the from card cache, so G1 may loose remembered set entries. Extend from card cache entries to 64 bits.

Co-authored-by: Jarkko Miettinen <jarkko.miettinen@relex.fi>
Reviewed-by: shade, sjohanss

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

@@ -28,7 +28,7 @@
 #include "memory/padded.inline.hpp"
 #include "utilities/debug.hpp"
 
-int**  G1FromCardCache::_cache = NULL;
+uintptr_t** G1FromCardCache::_cache = NULL;
 uint        G1FromCardCache::_max_regions = 0;
 size_t      G1FromCardCache::_static_mem_size = 0;
 #ifdef ASSERT
@@ -43,7 +43,7 @@ void G1FromCardCache::initialize(uint num_par_rem_sets, uint max_num_regions) {
 #ifdef ASSERT
   _max_workers = num_par_rem_sets;
 #endif
-  _cache = Padded2DArray<int, mtGC>::create_unfreeable(_max_regions,
+  _cache = Padded2DArray<uintptr_t, mtGC>::create_unfreeable(_max_regions,
                                                              num_par_rem_sets,
                                                              &_static_mem_size);
 
@@ -68,7 +68,7 @@ void G1FromCardCache::invalidate(uint start_idx, size_t new_num_regions) {
 void G1FromCardCache::print(outputStream* out) {
   for (uint i = 0; i < G1RemSet::num_par_rem_sets(); i++) {
     for (uint j = 0; j < _max_regions; j++) {
-      out->print_cr("_from_card_cache[%u][%u] = %d.",
+      out->print_cr("_from_card_cache[%u][%u] = " SIZE_FORMAT ".",
                     i, j, at(i, j));
     }
   }

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

@@ -37,7 +37,7 @@ class G1FromCardCache : public AllStatic {
   // This order minimizes the time to clear all entries for a given region during region
   // freeing. I.e. a single clear of a single memory area instead of multiple separate
   // accesses with a large stride per region.
-  static int** _cache;
+  static uintptr_t** _cache;
   static uint _max_regions;
   static size_t _static_mem_size;
 #ifdef ASSERT
@@ -50,16 +50,14 @@ class G1FromCardCache : public AllStatic {
 #endif
 
  public:
-  enum {
+  static const uintptr_t InvalidCard = UINTPTR_MAX;
-    InvalidCard = -1 // Card value of an invalid card, i.e. a card index not otherwise used.
-  };
 
   static void clear(uint region_idx);
 
   // Returns true if the given card is in the cache at the given location, or
   // replaces the card at that location and returns false.
-  static bool contains_or_replace(uint worker_id, uint region_idx, int card) {
+  static bool contains_or_replace(uint worker_id, uint region_idx, uintptr_t card) {
-    int card_in_cache = at(worker_id, region_idx);
+    uintptr_t card_in_cache = at(worker_id, region_idx);
     if (card_in_cache == card) {
       return true;
     } else {
@@ -68,12 +66,12 @@ class G1FromCardCache : public AllStatic {
     }
   }
 
-  static int at(uint worker_id, uint region_idx) {
+  static uintptr_t at(uint worker_id, uint region_idx) {
     DEBUG_ONLY(check_bounds(worker_id, region_idx);)
     return _cache[region_idx][worker_id];
   }
 
-  static void set(uint worker_id, uint region_idx, int val) {
+  static void set(uint worker_id, uint region_idx, uintptr_t val) {
     DEBUG_ONLY(check_bounds(worker_id, region_idx);)
     _cache[region_idx][worker_id] = val;
   }

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

@@ -93,17 +93,8 @@ protected:
     // If the test below fails, then this table was reused concurrently
     // with this operation.  This is OK, since the old table was coarsened,
     // and adding a bit to the new table is never incorrect.
-    // If the table used to belong to a continues humongous region and is
-    // now reused for the corresponding start humongous region, we need to
-    // make sure that we detect this. Thus, we call is_in_reserved_raw()
-    // instead of just is_in_reserved() here.
     if (loc_hr->is_in_reserved(from)) {
-      size_t hw_offset = pointer_delta((HeapWord*)from, loc_hr->bottom());
+      CardIdx_t from_card = OtherRegionsTable::card_within_region(from, loc_hr);
-      CardIdx_t from_card = (CardIdx_t)
-          hw_offset >> (G1CardTable::card_shift - LogHeapWordSize);
-
-      assert((size_t)from_card < HeapRegion::CardsPerRegion,
-             "Must be in range.");
       add_card_work(from_card, par);
     }
   }
@@ -343,10 +334,18 @@ void OtherRegionsTable::unlink_from_all(PerRegionTable* prt) {
          "just checking");
 }
 
+CardIdx_t OtherRegionsTable::card_within_region(OopOrNarrowOopStar within_region, HeapRegion* hr) {
+  assert(hr->is_in_reserved(within_region),
+         "HeapWord " PTR_FORMAT " is outside of region %u [" PTR_FORMAT ", " PTR_FORMAT ")",
+         p2i(within_region), hr->hrm_index(), p2i(hr->bottom()), p2i(hr->end()));
+  CardIdx_t result = (CardIdx_t)(pointer_delta((HeapWord*)within_region, hr->bottom()) >> (CardTable::card_shift - LogHeapWordSize));
+  return result;
+}
+
 void OtherRegionsTable::add_reference(OopOrNarrowOopStar from, uint tid) {
   uint cur_hrm_ind = _hr->hrm_index();
 
-  int from_card = (int)(uintptr_t(from) >> G1CardTable::card_shift);
+  uintptr_t from_card = uintptr_t(from) >> CardTable::card_shift;
 
   if (G1FromCardCache::contains_or_replace(tid, cur_hrm_ind, from_card)) {
     assert(contains_reference(from), "We just found " PTR_FORMAT " in the FromCardCache", p2i(from));
@@ -372,12 +371,8 @@ void OtherRegionsTable::add_reference(OopOrNarrowOopStar from, uint tid) {
     prt = find_region_table(ind, from_hr);
     if (prt == NULL) {
 
-      uintptr_t from_hr_bot_card_index =
+      CardIdx_t card_index = card_within_region(from, from_hr);
-        uintptr_t(from_hr->bottom())
+
-          >> G1CardTable::card_shift;
-      CardIdx_t card_index = from_card - from_hr_bot_card_index;
-      assert((size_t)card_index < HeapRegion::CardsPerRegion,
-             "Must be in range.");
       if (G1HRRSUseSparseTable &&
           _sparse_table.add_card(from_hrm_ind, card_index)) {
         assert(contains_reference_locked(from), "We just added " PTR_FORMAT " to the Sparse table", p2i(from));
@@ -612,14 +607,7 @@ bool OtherRegionsTable::contains_reference_locked(OopOrNarrowOopStar from) const
     return prt->contains_reference(from);
 
   } else {
-    uintptr_t from_card =
+    CardIdx_t card_index = card_within_region(from, hr);
-      (uintptr_t(from) >> G1CardTable::card_shift);
-    uintptr_t hr_bot_card_index =
-      uintptr_t(hr->bottom()) >> G1CardTable::card_shift;
-    assert(from_card >= hr_bot_card_index, "Inv");
-    CardIdx_t card_index = from_card - hr_bot_card_index;
-    assert((size_t)card_index < HeapRegion::CardsPerRegion,
-           "Must be in range.");
     return _sparse_table.contains_card(hr_ind, card_index);
   }
 }

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

@@ -130,6 +130,9 @@ public:
   // be used to ensure consistency.
   OtherRegionsTable(HeapRegion* hr, Mutex* m);
 
+  // Returns the card index of the given within_region pointer relative to the bottom
+  // of the given heap region.
+  static CardIdx_t card_within_region(OopOrNarrowOopStar within_region, HeapRegion* hr);
   // Adds the reference from "from to this remembered set.
   void add_reference(OopOrNarrowOopStar from, uint tid);
 

test/hotspot/jtreg/gc/g1/TestFromCardCacheIndex.java

@@ -0,0 +1,120 @@
+/*
+ * @test TestFromCardCacheIndex.java
+ * @bug 8196485
+ * @summary Ensure that G1 does not miss a remembered set entry due to from card cache default value indices.
+ * @key gc
+ * @requires vm.gc.G1
+ * @requires vm.debug
+ * @requires vm.bits != "32"
+ * @library /test/lib
+ * @modules java.base/jdk.internal.misc
+ *          java.management
+ * @build sun.hotspot.WhiteBox
+ * @run driver ClassFileInstaller sun.hotspot.WhiteBox
+ * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -Xbootclasspath/a:. -Xms20M -Xmx20M -XX:+UseCompressedOops -XX:G1HeapRegionSize=1M -XX:HeapBaseMinAddress=2199011721216 -XX:+UseG1GC -verbose:gc TestFromCardCacheIndex
+ */
+
+import sun.hotspot.WhiteBox;
+
+/**
+ * Repeatedly tries to generate references from objects that contained a card with the same index
+ * of the from card cache default value.
+ */
+public class TestFromCardCacheIndex {
+    private static WhiteBox WB;
+
+    // Shift value to calculate card indices from addresses.
+    private static final int CardSizeShift = 9;
+
+    /**
+     * Returns the last address on the heap within the object.
+     *
+     * @param The Object array to get the last address from.
+     */
+    private static long getObjectLastAddress(Object[] o) {
+        return WB.getObjectAddress(o) + WB.getObjectSize(o) - 1;
+    }
+
+    /**
+     * Returns the (truncated) 32 bit card index for the given address.
+     *
+     * @param The address to get the 32 bit card index from.
+     */
+    private static int getCardIndex32bit(long address) {
+        return (int)(address >> CardSizeShift);
+    }
+
+    // The source arrays that are placed on the heap in old gen.
+    private static int numArrays = 7000;
+    private static int arraySize = 508;
+    // Size of a humongous byte array, a bit less than a 1M region. This makes sure
+    // that we always create a cross-region reference when referencing it.
+    private static int byteArraySize = 1024*1023;
+
+    public static void main(String[] args) {
+        WB = sun.hotspot.WhiteBox.getWhiteBox();
+        for (int i = 0; i < 5; i++) {
+          runTest();
+          WB.fullGC();
+        }
+    }
+
+    public static void runTest() {
+        System.out.println("Starting test");
+
+        // Spray the heap with random object arrays in the hope that we get one
+        // at the proper place.
+        Object[][] arrays = new Object[numArrays][];
+        for (int i = 0; i < numArrays; i++) {
+            arrays[i] = new Object[arraySize];
+        }
+
+        // Make sure that everything is in old gen.
+        WB.fullGC();
+
+        // Find if we got an allocation at the right spot.
+        Object[] arrayWithCardMinus1 = findArray(arrays);
+
+        if (arrayWithCardMinus1 == null) {
+            System.out.println("Array with card -1 not found. Trying again.");
+            return;
+        } else {
+            System.out.println("Array with card -1 found.");
+        }
+
+        System.out.println("Modifying the last card in the array with a new object in a different region...");
+        // Create a target object that is guaranteed to be in a different region.
+        byte[] target = new byte[byteArraySize];
+
+        // Modify the last entry of the object we found.
+        arrayWithCardMinus1[arraySize - 1] = target;
+
+        target = null;
+        // Make sure that the dirty cards are flushed by doing a GC.
+        System.out.println("Doing a GC.");
+        WB.youngGC();
+
+        System.out.println("The crash didn't reproduce. Trying again.");
+    }
+
+    /**
+     * Finds an returns an array that contains a (32 bit truncated) card with value -1.
+     */
+    private static Object[] findArray(Object[][] arrays) {
+        for (int i = 0; i < arrays.length; i++) {
+            Object[] target = arrays[i];
+            if (target == null) {
+                continue;
+            }
+            final long startAddress = WB.getObjectAddress(target);
+            final long lastAddress = getObjectLastAddress(target);
+            final int card = getCardIndex32bit(lastAddress);
+            if (card == -1) {
+                Object[] foundArray = target;
+                return foundArray;
+            }
+        }
+        return null;
+    }
+}
+