7200261: G1: Liveness counting inconsistencies during marking verification

Summary: The clipping code in the routine that sets the bits for a range of cards, in the liveness accounting verification code was incorrect. It set all the bits in the card bitmap from the given starting index which would lead to spurious marking verification failures.
Reviewed-by: brutisso, jwilhelm, jmasa

src/share/vm/gc_implementation/g1/concurrentMark.cpp

@@ -1188,29 +1188,14 @@ void ConcurrentMark::checkpointRootsFinal(bool clear_all_soft_refs) {
 // liveness counting data.
 class CMCountDataClosureBase: public HeapRegionClosure {
 protected:
+  G1CollectedHeap* _g1h;
   ConcurrentMark* _cm;
+  CardTableModRefBS* _ct_bs;
+
   BitMap* _region_bm;
   BitMap* _card_bm;
 
-  void set_card_bitmap_range(BitMap::idx_t start_idx, BitMap::idx_t last_idx) {
-    assert(start_idx <= last_idx, "sanity");
-
-    // Set the inclusive bit range [start_idx, last_idx].
-    // For small ranges (up to 8 cards) use a simple loop; otherwise
-    // use par_at_put_range.
-    if ((last_idx - start_idx) < 8) {
-      for (BitMap::idx_t i = start_idx; i <= last_idx; i += 1) {
-        _card_bm->par_set_bit(i);
-      }
-    } else {
-      assert(last_idx < _card_bm->size(), "sanity");
-      // Note BitMap::par_at_put_range() is exclusive.
-      BitMap::idx_t max_idx = MAX2(last_idx+1, _card_bm->size());
-      _card_bm->par_at_put_range(start_idx, max_idx, true);
-    }
-  }
-
-  // It takes a region that's not empty (i.e., it has at least one
+  // Takes a region that's not empty (i.e., it has at least one
   // live object in it and sets its corresponding bit on the region
   // bitmap to 1. If the region is "starts humongous" it will also set
   // to 1 the bits on the region bitmap that correspond to its
@@ -1231,9 +1216,11 @@ protected:
   }
 
 public:
-  CMCountDataClosureBase(ConcurrentMark *cm,
+  CMCountDataClosureBase(G1CollectedHeap* g1h,
                          BitMap* region_bm, BitMap* card_bm):
-    _cm(cm), _region_bm(region_bm), _card_bm(card_bm) { }
+    _g1h(g1h), _cm(g1h->concurrent_mark()),
+    _ct_bs((CardTableModRefBS*) (g1h->barrier_set())),
+    _region_bm(region_bm), _card_bm(card_bm) { }
 };
 
 // Closure that calculates the # live objects per region. Used
@@ -1243,9 +1230,9 @@ class CalcLiveObjectsClosure: public CMCountDataClosureBase {
   size_t _region_marked_bytes;
 
 public:
-  CalcLiveObjectsClosure(CMBitMapRO *bm, ConcurrentMark *cm,
+  CalcLiveObjectsClosure(CMBitMapRO *bm, G1CollectedHeap* g1h,
                          BitMap* region_bm, BitMap* card_bm) :
-    CMCountDataClosureBase(cm, region_bm, card_bm),
+    CMCountDataClosureBase(g1h, region_bm, card_bm),
     _bm(bm), _region_marked_bytes(0) { }
 
   bool doHeapRegion(HeapRegion* hr) {
@@ -1261,44 +1248,63 @@ public:
       return false;
     }
 
-    HeapWord* nextTop = hr->next_top_at_mark_start();
-    HeapWord* start   = hr->bottom();
+    HeapWord* ntams = hr->next_top_at_mark_start();
+    HeapWord* start = hr->bottom();
 
-    assert(start <= hr->end() && start <= nextTop && nextTop <= hr->end(),
+    assert(start <= hr->end() && start <= ntams && ntams <= hr->end(),
            err_msg("Preconditions not met - "
-                   "start: "PTR_FORMAT", nextTop: "PTR_FORMAT", end: "PTR_FORMAT,
-                   start, nextTop, hr->end()));
+                   "start: "PTR_FORMAT", ntams: "PTR_FORMAT", end: "PTR_FORMAT,
+                   start, ntams, hr->end()));
 
     // Find the first marked object at or after "start".
-    start = _bm->getNextMarkedWordAddress(start, nextTop);
+    start = _bm->getNextMarkedWordAddress(start, ntams);
 
     size_t marked_bytes = 0;
 
-    while (start < nextTop) {
+    while (start < ntams) {
       oop obj = oop(start);
       int obj_sz = obj->size();
-      HeapWord* obj_last = start + obj_sz - 1;
+      HeapWord* obj_end = start + obj_sz;
 
       BitMap::idx_t start_idx = _cm->card_bitmap_index_for(start);
-      BitMap::idx_t last_idx = _cm->card_bitmap_index_for(obj_last);
+      BitMap::idx_t end_idx = _cm->card_bitmap_index_for(obj_end);
+
+      // Note: if we're looking at the last region in heap - obj_end
+      // could be actually just beyond the end of the heap; end_idx
+      // will then correspond to a (non-existent) card that is also
+      // just beyond the heap.
+      if (_g1h->is_in_g1_reserved(obj_end) && !_ct_bs->is_card_aligned(obj_end)) {
+        // end of object is not card aligned - increment to cover
+        // all the cards spanned by the object
+        end_idx += 1;
+      }
 
-      // Set the bits in the card BM for this object (inclusive).
-      set_card_bitmap_range(start_idx, last_idx);
+      // Set the bits in the card BM for the cards spanned by this object.
+      _cm->set_card_bitmap_range(_card_bm, start_idx, end_idx, true /* is_par */);
 
       // Add the size of this object to the number of marked bytes.
       marked_bytes += (size_t)obj_sz * HeapWordSize;
 
       // Find the next marked object after this one.
-      start = _bm->getNextMarkedWordAddress(obj_last + 1, nextTop);
+      start = _bm->getNextMarkedWordAddress(obj_end, ntams);
     }
 
     // Mark the allocated-since-marking portion...
     HeapWord* top = hr->top();
-    if (nextTop < top) {
-      BitMap::idx_t start_idx = _cm->card_bitmap_index_for(nextTop);
-      BitMap::idx_t last_idx = _cm->card_bitmap_index_for(top - 1);
-
-      set_card_bitmap_range(start_idx, last_idx);
+    if (ntams < top) {
+      BitMap::idx_t start_idx = _cm->card_bitmap_index_for(ntams);
+      BitMap::idx_t end_idx = _cm->card_bitmap_index_for(top);
+
+      // Note: if we're looking at the last region in heap - top
+      // could be actually just beyond the end of the heap; end_idx
+      // will then correspond to a (non-existent) card that is also
+      // just beyond the heap.
+      if (_g1h->is_in_g1_reserved(top) && !_ct_bs->is_card_aligned(top)) {
+        // end of object is not card aligned - increment to cover
+        // all the cards spanned by the object
+        end_idx += 1;
+      }
+      _cm->set_card_bitmap_range(_card_bm, start_idx, end_idx, true /* is_par */);
 
       // This definitely means the region has live objects.
       set_bit_for_region(hr);
@@ -1325,6 +1331,7 @@ public:
 // regions during the STW cleanup pause.
 
 class VerifyLiveObjectDataHRClosure: public HeapRegionClosure {
+  G1CollectedHeap* _g1h;
   ConcurrentMark* _cm;
   CalcLiveObjectsClosure _calc_cl;
   BitMap* _region_bm;   // Region BM to be verified
@@ -1337,14 +1344,14 @@ class VerifyLiveObjectDataHRClosure: public HeapRegionClosure {
   int _failures;
 
 public:
-  VerifyLiveObjectDataHRClosure(ConcurrentMark* cm,
+  VerifyLiveObjectDataHRClosure(G1CollectedHeap* g1h,
                                 BitMap* region_bm,
                                 BitMap* card_bm,
                                 BitMap* exp_region_bm,
                                 BitMap* exp_card_bm,
                                 bool verbose) :
-    _cm(cm),
-    _calc_cl(_cm->nextMarkBitMap(), _cm, exp_region_bm, exp_card_bm),
+    _g1h(g1h), _cm(g1h->concurrent_mark()),
+    _calc_cl(_cm->nextMarkBitMap(), g1h, exp_region_bm, exp_card_bm),
     _region_bm(region_bm), _card_bm(card_bm), _verbose(verbose),
     _exp_region_bm(exp_region_bm), _exp_card_bm(exp_card_bm),
     _failures(0) { }
@@ -1491,7 +1498,7 @@ public:
   void work(uint worker_id) {
     assert(worker_id < _n_workers, "invariant");
 
-    VerifyLiveObjectDataHRClosure verify_cl(_cm,
+    VerifyLiveObjectDataHRClosure verify_cl(_g1h,
                                             _actual_region_bm, _actual_card_bm,
                                             _expected_region_bm,
                                             _expected_card_bm,
@@ -1521,10 +1528,10 @@ public:
 
 class FinalCountDataUpdateClosure: public CMCountDataClosureBase {
  public:
-  FinalCountDataUpdateClosure(ConcurrentMark* cm,
+  FinalCountDataUpdateClosure(G1CollectedHeap* g1h,
                               BitMap* region_bm,
                               BitMap* card_bm) :
-    CMCountDataClosureBase(cm, region_bm, card_bm) { }
+    CMCountDataClosureBase(g1h, region_bm, card_bm) { }
 
   bool doHeapRegion(HeapRegion* hr) {
 
@@ -1548,24 +1555,29 @@ class FinalCountDataUpdateClosure: public CMCountDataClosureBase {
     if (ntams < top) {
       // This definitely means the region has live objects.
       set_bit_for_region(hr);
-    }
 
-    // Now set the bits for [ntams, top]
-    BitMap::idx_t start_idx = _cm->card_bitmap_index_for(ntams);
-    // set_card_bitmap_range() expects the last_idx to be with
-    // the range of the bit map (see assertion in set_card_bitmap_range()),
-    // so limit it to that range with this application of MIN2.
-    BitMap::idx_t last_idx = MIN2(_cm->card_bitmap_index_for(top),
-                                  _card_bm->size()-1);
-    if (start_idx < _card_bm->size()) {
-    set_card_bitmap_range(start_idx, last_idx);
-    } else {
-      // To reach here start_idx must be beyond the end of
-      // the bit map and last_idx must have been limited by
-      // the MIN2().
-      assert(start_idx == last_idx + 1,
-        err_msg("Not beyond end start_idx " SIZE_FORMAT " last_idx "
-                SIZE_FORMAT, start_idx, last_idx));
+      // Now set the bits in the card bitmap for [ntams, top)
+      BitMap::idx_t start_idx = _cm->card_bitmap_index_for(ntams);
+      BitMap::idx_t end_idx = _cm->card_bitmap_index_for(top);
+
+      // Note: if we're looking at the last region in heap - top
+      // could be actually just beyond the end of the heap; end_idx
+      // will then correspond to a (non-existent) card that is also
+      // just beyond the heap.
+      if (_g1h->is_in_g1_reserved(top) && !_ct_bs->is_card_aligned(top)) {
+        // end of object is not card aligned - increment to cover
+        // all the cards spanned by the object
+        end_idx += 1;
+      }
+
+      assert(end_idx <= _card_bm->size(),
+             err_msg("oob: end_idx=  "SIZE_FORMAT", bitmap size= "SIZE_FORMAT,
+                     end_idx, _card_bm->size()));
+      assert(start_idx < _card_bm->size(),
+             err_msg("oob: start_idx=  "SIZE_FORMAT", bitmap size= "SIZE_FORMAT,
+                     start_idx, _card_bm->size()));
+
+      _cm->set_card_bitmap_range(_card_bm, start_idx, end_idx, true /* is_par */);
     }
 
     // Set the bit for the region if it contains live data
@@ -1606,7 +1618,7 @@ public:
   void work(uint worker_id) {
     assert(worker_id < _n_workers, "invariant");
 
-    FinalCountDataUpdateClosure final_update_cl(_cm,
+    FinalCountDataUpdateClosure final_update_cl(_g1h,
                                                 _actual_region_bm,
                                                 _actual_card_bm);
 
@@ -2846,20 +2858,19 @@ void ConcurrentMark::clear_marking_state(bool clear_overflow) {
 // Aggregate the counting data that was constructed concurrently
 // with marking.
 class AggregateCountDataHRClosure: public HeapRegionClosure {
+  G1CollectedHeap* _g1h;
   ConcurrentMark* _cm;
+  CardTableModRefBS* _ct_bs;
   BitMap* _cm_card_bm;
   size_t _max_task_num;
 
  public:
-  AggregateCountDataHRClosure(ConcurrentMark *cm,
+  AggregateCountDataHRClosure(G1CollectedHeap* g1h,
                               BitMap* cm_card_bm,
                               size_t max_task_num) :
-    _cm(cm), _cm_card_bm(cm_card_bm),
-    _max_task_num(max_task_num) { }
-
-  bool is_card_aligned(HeapWord* p) {
-    return ((uintptr_t(p) & (CardTableModRefBS::card_size - 1)) == 0);
-  }
+    _g1h(g1h), _cm(g1h->concurrent_mark()),
+    _ct_bs((CardTableModRefBS*) (g1h->barrier_set())),
+    _cm_card_bm(cm_card_bm), _max_task_num(max_task_num) { }
 
   bool doHeapRegion(HeapRegion* hr) {
     if (hr->continuesHumongous()) {
@@ -2890,16 +2901,23 @@ class AggregateCountDataHRClosure: public HeapRegionClosure {
       return false;
     }
 
-    assert(is_card_aligned(start), "sanity");
-    assert(is_card_aligned(end), "sanity");
+    // 'start' should be in the heap.
+    assert(_g1h->is_in_g1_reserved(start) && _ct_bs->is_card_aligned(start), "sanity");
+    // 'end' *may* be just beyone the end of the heap (if hr is the last region)
+    assert(!_g1h->is_in_g1_reserved(end) || _ct_bs->is_card_aligned(end), "sanity");
 
     BitMap::idx_t start_idx = _cm->card_bitmap_index_for(start);
     BitMap::idx_t limit_idx = _cm->card_bitmap_index_for(limit);
     BitMap::idx_t end_idx = _cm->card_bitmap_index_for(end);
 
-    // If ntams is not card aligned then we bump the index for
-    // limit so that we get the card spanning ntams.
-    if (!is_card_aligned(limit)) {
+    // If ntams is not card aligned then we bump card bitmap index
+    // for limit so that we get the all the cards spanned by
+    // the object ending at ntams.
+    // Note: if this is the last region in the heap then ntams
+    // could be actually just beyond the end of the the heap;
+    // limit_idx will then  correspond to a (non-existent) card
+    // that is also outside the heap.
+    if (_g1h->is_in_g1_reserved(limit) && !_ct_bs->is_card_aligned(limit)) {
       limit_idx += 1;
     }
 
@@ -2928,7 +2946,7 @@ class AggregateCountDataHRClosure: public HeapRegionClosure {
 
         // BitMap::get_next_one_offset() can handle the case when
         // its left_offset parameter is greater than its right_offset
-        // parameter. If does, however, have an early exit if
+        // parameter. It does, however, have an early exit if
         // left_offset == right_offset. So let's limit the value
         // passed in for left offset here.
         BitMap::idx_t next_idx = MIN2(scan_idx + 1, limit_idx);
@@ -2964,7 +2982,7 @@ public:
     _active_workers(n_workers) { }
 
   void work(uint worker_id) {
-    AggregateCountDataHRClosure cl(_cm, _cm_card_bm, _max_task_num);
+    AggregateCountDataHRClosure cl(_g1h, _cm_card_bm, _max_task_num);
 
     if (G1CollectedHeap::use_parallel_gc_threads()) {
       _g1h->heap_region_par_iterate_chunked(&cl, worker_id,

src/share/vm/gc_implementation/g1/concurrentMark.hpp

@@ -806,7 +806,14 @@ public:
     return _MARKING_VERBOSE_ && _verbose_level >= high_verbose;
   }
 
-  // Counting data structure accessors
+  // Liveness counting
+
+  // Utility routine to set an exclusive range of cards on the given
+  // card liveness bitmap
+  inline void set_card_bitmap_range(BitMap* card_bm,
+                                    BitMap::idx_t start_idx,
+                                    BitMap::idx_t end_idx,
+                                    bool is_par);
 
   // Returns the card number of the bottom of the G1 heap.
   // Used in biasing indices into accounting card bitmaps.

src/share/vm/gc_implementation/g1/concurrentMark.inline.hpp

@@ -28,6 +28,42 @@
 #include "gc_implementation/g1/concurrentMark.hpp"
 #include "gc_implementation/g1/g1CollectedHeap.inline.hpp"
 
+// Utility routine to set an exclusive range of cards on the given
+// card liveness bitmap
+inline void ConcurrentMark::set_card_bitmap_range(BitMap* card_bm,
+                                                  BitMap::idx_t start_idx,
+                                                  BitMap::idx_t end_idx,
+                                                  bool is_par) {
+
+  // Set the exclusive bit range [start_idx, end_idx).
+  assert((end_idx - start_idx) > 0, "at least one card");
+  assert(end_idx <= card_bm->size(), "sanity");
+
+  // Silently clip the end index
+  end_idx = MIN2(end_idx, card_bm->size());
+
+  // For small ranges use a simple loop; otherwise use set_range or
+  // use par_at_put_range (if parallel). The range is made up of the
+  // cards that are spanned by an object/mem region so 8 cards will
+  // allow up to object sizes up to 4K to be handled using the loop.
+  if ((end_idx - start_idx) <= 8) {
+    for (BitMap::idx_t i = start_idx; i < end_idx; i += 1) {
+      if (is_par) {
+        card_bm->par_set_bit(i);
+      } else {
+        card_bm->set_bit(i);
+      }
+    }
+  } else {
+    // Note BitMap::par_at_put_range() and BitMap::set_range() are exclusive.
+    if (is_par) {
+      card_bm->par_at_put_range(start_idx, end_idx, true);
+    } else {
+      card_bm->set_range(start_idx, end_idx);
+    }
+  }
+}
+
 // Returns the index in the liveness accounting card bitmap
 // for the given address
 inline BitMap::idx_t ConcurrentMark::card_bitmap_index_for(HeapWord* addr) {
@@ -35,7 +71,6 @@ inline BitMap::idx_t ConcurrentMark::card_bitmap_index_for(HeapWord* addr) {
   // by the card shift -- address 0 corresponds to card number 0.  One
   // must subtract the card num of the bottom of the heap to obtain a
   // card table index.
-
   intptr_t card_num = intptr_t(uintptr_t(addr) >> CardTableModRefBS::card_shift);
   return card_num - heap_bottom_card_num();
 }
@@ -46,8 +81,10 @@ inline void ConcurrentMark::count_region(MemRegion mr, HeapRegion* hr,
                                          size_t* marked_bytes_array,
                                          BitMap* task_card_bm) {
   G1CollectedHeap* g1h = _g1h;
+  CardTableModRefBS* ct_bs = (CardTableModRefBS*) (g1h->barrier_set());
+
   HeapWord* start = mr.start();
-  HeapWord* last = mr.last();
+  HeapWord* end = mr.end();
   size_t region_size_bytes = mr.byte_size();
   uint index = hr->hrs_index();
 
@@ -61,24 +98,21 @@ inline void ConcurrentMark::count_region(MemRegion mr, HeapRegion* hr,
   marked_bytes_array[index] += region_size_bytes;
 
   BitMap::idx_t start_idx = card_bitmap_index_for(start);
-  BitMap::idx_t last_idx = card_bitmap_index_for(last);
-
-  // The card bitmap is task/worker specific => no need to use 'par' routines.
-  // Set bits in the inclusive bit range [start_idx, last_idx].
-  //
-  // For small ranges use a simple loop; otherwise use set_range
-  // The range are the cards that are spanned by the object/region
-  // so 8 cards will allow objects/regions up to 4K to be handled
-  // using the loop.
-  if ((last_idx - start_idx) <= 8) {
-    for (BitMap::idx_t i = start_idx; i <= last_idx; i += 1) {
-     task_card_bm->set_bit(i);
-    }
-  } else {
-    assert(last_idx < task_card_bm->size(), "sanity");
-    // Note: BitMap::set_range() is exclusive.
-    task_card_bm->set_range(start_idx, last_idx+1);
+  BitMap::idx_t end_idx = card_bitmap_index_for(end);
+
+  // Note: if we're looking at the last region in heap - end
+  // could be actually just beyond the end of the heap; end_idx
+  // will then correspond to a (non-existent) card that is also
+  // just beyond the heap.
+  if (g1h->is_in_g1_reserved(end) && !ct_bs->is_card_aligned(end)) {
+    // end of region is not card aligned - incremement to cover
+    // all the cards spanned by the region.
+    end_idx += 1;
   }
+  // The card bitmap is task/worker specific => no need to use
+  // the 'par' BitMap routines.
+  // Set bits in the exclusive bit range [start_idx, end_idx).
+  set_card_bitmap_range(task_card_bm, start_idx, end_idx, false /* is_par */);
 }
 
 // Counts the given memory region in the task/worker counting