From ea3f4034314481acde910dc571da5006a571dbfa Mon Sep 17 00:00:00 2001
From: Peter Sollich <petersol@microsoft.com>
Date: Wed, 23 Jun 2021 12:09:38 +0200
Subject: [PATCH] Fix finalizer issue with regions (#54550)

This fixes an issue in Server GC where an item in the finalizer queue became stale due to not being relocated.

The problem was that a finalizable object was allocated on one heap, but registered in the finalizer queue of another heap (this is possible due to heap balancing). In CFinalize::UpdatePromotedGenerations, we ask for the generation of an object, and move the object to the correct section of the finalizer queue. In the error case, we obtained the wrong result for the generation of the object because it lived on another heap, and that heap hadn't set the final generation for the region containing the object yet. So we ended up moving the finalizer entry to the section corresponding to gen 2, and missed a relocation of the object occurring in a gen 1 collection afterwards.

Fix: It seems best to make sure an object is always registered for finalization on the heap it's allocated from, so the fix simply fetches the heap from the alloc context after the allocation in the case of SOH, or determines it by calling gc_heap::heap_of in the case of LOH and POH. In the case of SOH, I added an assert to ensure that the heap obtained agrees with the result of calling gc_heap::heap_of.

I also added some dprintf calls to the finalizer logic to aid in future investigations.
---
 src/coreclr/gc/gc.cpp | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp
index 6a6c63d..9cb439e 100644
--- a/src/coreclr/gc/gc.cpp
+++ b/src/coreclr/gc/gc.cpp
@@ -42257,6 +42257,15 @@ GCHeap::Alloc(gc_alloc_context* context, size_t size, uint32_t flags REQD_ALIGN_
         newAlloc = (Object*) hp->allocate_uoh_object (size + ComputeMaxStructAlignPadLarge(requiredAlignment), flags, gen_num, acontext->alloc_bytes_uoh);
         ASSERT(((size_t)newAlloc & 7) == 0);
 
+#ifdef MULTIPLE_HEAPS
+        if (flags & GC_ALLOC_FINALIZE)
+        {
+            // the heap may have changed due to heap balancing - it's important
+            // to register the object for finalization on the heap it was allocated on
+            hp = gc_heap::heap_of ((uint8_t*)newAlloc);
+        }
+#endif //MULTIPLE_HEAPS
+
 #ifdef FEATURE_STRUCTALIGN
         newAlloc = (Object*) hp->pad_for_alignment_large ((uint8_t*) newAlloc, requiredAlignment, size);
 #endif // FEATURE_STRUCTALIGN
@@ -42276,6 +42285,16 @@ GCHeap::Alloc(gc_alloc_context* context, size_t size, uint32_t flags REQD_ALIGN_
             newAlloc = (Object*) hp->allocate (size + ComputeMaxStructAlignPad(requiredAlignment), acontext, flags);
         }
 
+#ifdef MULTIPLE_HEAPS
+        if (flags & GC_ALLOC_FINALIZE)
+        {
+            // the heap may have changed due to heap balancing - it's important
+            // to register the object for finalization on the heap it was allocated on
+            hp = acontext->get_alloc_heap()->pGenGCHeap;
+            assert ((newAlloc == nullptr) || (hp == gc_heap::heap_of ((uint8_t*)newAlloc)));
+        }
+#endif //MULTIPLE_HEAPS
+
 #ifdef FEATURE_STRUCTALIGN
         newAlloc = (Object*) hp->pad_for_alignment ((uint8_t*) newAlloc, requiredAlignment, size, acontext);
 #endif // FEATURE_STRUCTALIGN
@@ -44404,6 +44423,9 @@ CFinalize::RelocateFinalizationData (int gen, gc_heap* hp)
     unsigned int Seg = gen_segment (gen);
 
     Object** startIndex = SegQueue (Seg);
+
+    dprintf (3, ("RelocateFinalizationData gen=%d, [%Ix,%Ix[", gen, startIndex, SegQueue (FreeList)));
+
     for (Object** po = startIndex; po < SegQueue (FreeList);po++)
     {
         GCHeap::Relocate (po, &sc);
@@ -44413,6 +44435,8 @@ CFinalize::RelocateFinalizationData (int gen, gc_heap* hp)
 void
 CFinalize::UpdatePromotedGenerations (int gen, BOOL gen_0_empty_p)
 {
+    dprintf(3, ("UpdatePromotedGenerations gen=%d, gen_0_empty_p=%d", gen, gen_0_empty_p));
+
     // update the generation fill pointers.
     // if gen_0_empty is FALSE, test each object to find out if
     // it was promoted or not
@@ -44437,6 +44461,8 @@ CFinalize::UpdatePromotedGenerations (int gen, BOOL gen_0_empty_p)
                 int new_gen = g_theGCHeap->WhichGeneration (*po);
                 if (new_gen != i)
                 {
+                    dprintf (3, ("Moving object %Ix->%Ix from gen %d to gen %d", po, *po, i, new_gen));
+
                     if (new_gen > i)
                     {
                         //promotion
@@ -44468,6 +44494,8 @@ CFinalize::GrowArray()
     }
     memcpy (newArray, m_Array, oldArraySize*sizeof(Object*));
 
+    dprintf (3, ("Grow finalizer array [%Ix,%Ix[ -> [%Ix,%Ix[", m_Array, m_EndArray, newArray, &m_Array[newArraySize]));
+
     //adjust the fill pointers
     for (int i = 0; i < FreeList; i++)
     {
-- 
2.7.4