From a90d26db22af761084a5812c7163cb9daee47238 Mon Sep 17 00:00:00 2001 From: Peter Sollich Date: Fri, 14 Jan 2022 15:39:05 +0100 Subject: [PATCH 1/3] Fix issues with verify_regions, clear_batch_mark_array_bits. Details: - we cannot verify the tail of the region list from background GC, as it may be updated by threads allocating. - fix case in clear_batch_mark_array_bits where end is equal to the very end of a segment and we write into uncommitted memory in the mark_array. --- src/coreclr/gc/gc.cpp | 30 ++++++++++++++++++++---------- src/coreclr/gc/gcpriv.h | 4 ++-- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index ace66eac266709..1f9bf7fff517af 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -10914,8 +10914,12 @@ void gc_heap::clear_batch_mark_array_bits (uint8_t* start, uint8_t* end) if (startwrd == endwrd) { - unsigned int wrd = firstwrd | lastwrd; - mark_array[startwrd] &= wrd; + assert (startbit <= endbit); + if (endbit) + { + unsigned int wrd = firstwrd | lastwrd; + mark_array[startwrd] &= wrd; + } return; } @@ -29701,7 +29705,7 @@ void gc_heap::thread_final_regions (bool compact_p) } } - verify_regions (true); + verify_regions (true, false); } void gc_heap::thread_start_region (generation* gen, heap_segment* region) @@ -29752,7 +29756,7 @@ heap_segment* gc_heap::get_new_region (int gen_number, size_t size) heap_segment_next (generation_tail_region (gen)) = new_region; generation_tail_region (gen) = new_region; - verify_regions (gen_number, false); + verify_regions (gen_number, false, settings.concurrent); } return new_region; @@ -29815,7 +29819,7 @@ void gc_heap::update_start_tail_regions (generation* gen, (size_t)prev_region, heap_segment_mem (prev_region))); } - verify_regions (false); + verify_regions (false, settings.concurrent); } // There's one complication with deciding whether we can make a region SIP or not - if the plan_gen_num of @@ -42103,7 +42107,7 @@ gc_heap::verify_free_lists () } } -void gc_heap::verify_regions (int gen_number, bool can_verify_gen_num) +void gc_heap::verify_regions (int gen_number, bool can_verify_gen_num, bool can_verify_tail) { #ifdef USE_REGIONS // For the given generation, verify that @@ -42166,7 +42170,7 @@ void gc_heap::verify_regions (int gen_number, bool can_verify_gen_num) FATAL_GC_ERROR(); } - if (tail_region != prev_region_in_gen) + if (can_verify_tail && (tail_region != prev_region_in_gen)) { dprintf (REGIONS_LOG, ("h%d gen%d tail region is %Ix(%Ix), diff from last region %Ix(%Ix)!!", heap_number, gen_number, @@ -42177,12 +42181,18 @@ void gc_heap::verify_regions (int gen_number, bool can_verify_gen_num) #endif //USE_REGIONS } -void gc_heap::verify_regions (bool can_verify_gen_num) +inline bool is_user_alloc_gen (int gen_number) +{ + return ((gen_number == soh_gen0) || (gen_number == loh_generation) || (gen_number == poh_generation)); +} + +void gc_heap::verify_regions (bool can_verify_gen_num, bool concurrent_p) { #ifdef USE_REGIONS for (int i = 0; i < total_generation_count; i++) { - verify_regions (i, can_verify_gen_num); + bool can_verify_tail = (concurrent_p ? !is_user_alloc_gen (i) : true); + verify_regions (i, can_verify_gen_num, can_verify_tail); } #endif //USE_REGIONS } @@ -42313,7 +42323,7 @@ void gc_heap::verify_heap (BOOL begin_gc_p) //verify that the generation structures makes sense { #ifdef USE_REGIONS - verify_regions (true); + verify_regions (true, settings.concurrent); #else //USE_REGIONS generation* gen = generation_of (max_generation); diff --git a/src/coreclr/gc/gcpriv.h b/src/coreclr/gc/gcpriv.h index 081b9a539c073c..069eaacd2ad41b 100644 --- a/src/coreclr/gc/gcpriv.h +++ b/src/coreclr/gc/gcpriv.h @@ -1286,9 +1286,9 @@ class gc_heap PER_HEAP void verify_free_lists(); PER_HEAP - void verify_regions (int gen_number, bool can_verify_gen_num); + void verify_regions (int gen_number, bool can_verify_gen_num, bool can_verify_tail); PER_HEAP - void verify_regions (bool can_verify_gen_num); + void verify_regions (bool can_verify_gen_num, bool concurrent_p); PER_HEAP_ISOLATED void enter_gc_lock_for_verify_heap(); PER_HEAP_ISOLATED From d6082e608c676377e764cfc41a2b43d0d03b68ea Mon Sep 17 00:00:00 2001 From: Peter Sollich Date: Mon, 17 Jan 2022 10:52:08 +0100 Subject: [PATCH 2/3] Address code review feedback. --- src/coreclr/gc/gc.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index 1f9bf7fff517af..2c0c8d06153974 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -10914,12 +10914,15 @@ void gc_heap::clear_batch_mark_array_bits (uint8_t* start, uint8_t* end) if (startwrd == endwrd) { - assert (startbit <= endbit); - if (endbit) + if (startbit != endbit) { unsigned int wrd = firstwrd | lastwrd; mark_array[startwrd] &= wrd; } + else + { + assert (start == end); + } return; } From aa0c72fd06d82aaa9eb42d46d889e445b1978a11 Mon Sep 17 00:00:00 2001 From: Peter Sollich Date: Wed, 19 Jan 2022 10:36:01 +0100 Subject: [PATCH 3/3] Elimiinate redundant call layer and duplicate checks . Details: bgc_clear_batch_mark_array_bits did some checks and then called clear_batch_mark_array_bits which repeated the exact same checks. Renamed clear_batch_mark_array_bits to bgc_batch_mark_array_bits and removed the old copy, removed the declaration for clear_batch_mark_array_bits. --- src/coreclr/gc/gc.cpp | 14 +------------- src/coreclr/gc/gcpriv.h | 2 -- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index 2c0c8d06153974..a14e6b2ed452d7 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -10890,7 +10890,7 @@ void gc_heap::seg_clear_mark_array_bits_soh (heap_segment* seg) } } -void gc_heap::clear_batch_mark_array_bits (uint8_t* start, uint8_t* end) +void gc_heap::bgc_clear_batch_mark_array_bits (uint8_t* start, uint8_t* end) { if ((start < background_saved_highest_address) && (end > background_saved_lowest_address)) @@ -10945,18 +10945,6 @@ void gc_heap::clear_batch_mark_array_bits (uint8_t* start, uint8_t* end) } } } - -void gc_heap::bgc_clear_batch_mark_array_bits (uint8_t* start, uint8_t* end) -{ - if ((start < background_saved_highest_address) && - (end > background_saved_lowest_address)) - { - start = max (start, background_saved_lowest_address); - end = min (end, background_saved_highest_address); - - clear_batch_mark_array_bits (start, end); - } -} #endif //BACKGROUND_GC inline diff --git a/src/coreclr/gc/gcpriv.h b/src/coreclr/gc/gcpriv.h index 069eaacd2ad41b..e8e6351b42ea8b 100644 --- a/src/coreclr/gc/gcpriv.h +++ b/src/coreclr/gc/gcpriv.h @@ -2156,8 +2156,6 @@ class gc_heap PER_HEAP void seg_clear_mark_array_bits_soh (heap_segment* seg); PER_HEAP - void clear_batch_mark_array_bits (uint8_t* start, uint8_t* end); - PER_HEAP void bgc_clear_batch_mark_array_bits (uint8_t* start, uint8_t* end); #ifdef VERIFY_HEAP PER_HEAP