Skip to content

Commit

Permalink
Fix finalizer issue with regions (#54550)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
PeterSolMS authored Jun 23, 2021
1 parent 97de5c5 commit ea3f403
Showing 1 changed file with 28 additions and 0 deletions.
28 changes: 28 additions & 0 deletions src/coreclr/gc/gc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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++)
{
Expand Down

0 comments on commit ea3f403

Please sign in to comment.