Fix GC compaction for static fields (#3521)

***NO_CI***

Author: josesimoes

src/CLR/Core/CLAUDE.md

@@ -352,7 +352,66 @@ slots:
   so a bare-VAR field like `static T DefaultValue` in
   `Foo<int>` is stamped `I4` and in `Foo<string>` is stamped OBJECT.
   Passing no instance here would leave every VAR field wrongly typed as
-  the OBJECT block that `ExtractHeapBlocksForObjects` produced.
+  the OBJECT block that `CLR_AllocateGenericStaticFieldStorage` produced.
+
+### GC contract for the per-instantiation storage
+
+The per-instantiation slots are the one place in the CLR where an *array*
+of heap blocks is addressed by pointer arithmetic
+(`GetGenericStaticField` returns `&ts.genericStaticFields[i]`) while
+living outside a containing object. That forces three rules, and getting
+any of them wrong produces the same symptom: a static field read returns
+the contents of an unrelated heap block. When the block it lands on is a
+generic instance, the value comes back as a `CLR_RT_TypeSpec_Index` bit
+pattern such as `0x01000001` (§13).
+
+**1. The run must not be split.** `CLR_AllocateGenericStaticFieldStorage`
+allocates the slots as the payload of an unmovable
+`DATATYPE_BINARY_BLOB_HEAD` block (`ExtractHeapBlocksForEvents`, which
+adds `HB_Event`, hence `HB_Unmovable`). Compaction moves *maximal
+contiguous groups* of movable blocks into whatever free region is
+current, and ends a group when that region fills
+(`GarbageCollector_Compaction.cpp`, the `freeRegion_Size < len` break).
+So a bare run of N separately-movable size-1 blocks — what
+`ExtractHeapBlocksForObjects` gives you — can be broken in the middle,
+after which `m_fields[1]` points at an unrelated block while
+`m_fields[0]` is still correct. The blob header carries the whole run's
+`DataSize`, so the heap walk steps over the payload and compaction skips
+the run as a unit.
+
+*Rejected: pinning the slots individually.* Compaction tests
+`HB_Unmovable` per block, so every slot would need the flag, and slot
+flags do not survive. `InitializeReference` ends with
+`SetDataId(RAW_ID(dt, HB_Alive, 1))`, and `stsfld` on a reference-typed
+field goes through `AssignAndPreserveType`, which copies the whole `m_id`
+(flags included) from the eval-stack value whenever the slot's datatype
+is above `DATATYPE_LAST_PRIMITIVE_TO_PRESERVE`. The first write to a
+`static T` field would silently unpin the run.
+
+**2. Marking and relocation must walk the same array.** Both are driven
+from the global registry `g_CLR_RT_TypeSystem.m_genericStaticFields[]` —
+marking in `Assembly_Mark`, relocation in `CLR_RT_TypeSystem::Relocate`.
+Marking used to iterate per-assembly `crossReferenceTypeSpec` rows while
+relocation iterated the global array; a record reachable only from the
+registry (`FindOrCreateGenericStaticFields` creates exactly that shape,
+though it currently has no callers) would then be relocated but never
+marked, and freed while live.
+
+**3. Relocation runs once per pass, not once per assembly.**
+`Heap_Relocate(void**)` adds a region offset — it is *not* idempotent.
+`CLR_RT_Assembly::Relocate` is the `DATATYPE_ASSEMBLY` handler
+(`TypeSystemLookup.cpp`), so the heap walk calls it once per loaded
+assembly; driving the global registry from there shifted every pointer
+once per assembly. The hook is now `CLR_RT_ExecutionEngine::Relocate`,
+which `Heap_Relocate_Pass` calls exactly once after the walk.
+
+Because the payload is inside a blob the heap walk steps over, the slots'
+own contents are relocated *only* by
+`RelocateGenericStaticField` — that is why it still calls
+`Heap_Relocate(m_fields, m_count)`. Conversely `m_fields` itself never
+moves, so it is not relocated, and neither are the `tsCross` caches that
+copy it. `m_fieldDefs` is `platform_malloc`'d and was never a heap
+pointer; relocating it was always meaningless.
 
 ### Nested generic construction
 
@@ -744,6 +803,17 @@ When you see one of these symptoms, this is where to look first.
   priority-2 result is being picked up when it shouldn't (because the
   callee's `genericType` is open), the gate is missing or wrong.
 
+**A generic static field reads back as `0x0100000N` (or any unrelated
+value) only when the GC compacts.**
+- Reproduce with **both** `--forcegc` and `--compactionaftergc`;
+  `--forcegc` alone only marks and sweeps, and the bug will not show.
+- The storage array was split, mis-relocated, or swept. Check the three
+  rules in §9 "GC contract for the per-instantiation storage" — the
+  symptom is identical for all three.
+- A crash (host exit code 3) on the *next* generic-static test rather
+  than a bad value is the same bug reaching a slot that holds an object
+  reference instead of an `int`.
+
 **Generic `.cctor` not firing or firing twice.**
 - `FindOrCreateGenericStaticFields` hash mismatch — two TypeSpec rows
   encode the same closed type but hash differently, or vice versa. See §10.

src/CLR/Core/Execution.cpp

@@ -456,6 +456,8 @@ void CLR_RT_ExecutionEngine::Relocate()
     CLR_RT_GarbageCollector::Heap_Relocate((void **)&m_currentUICulture);
 
     m_weakReferences.Relocate();
+
+    g_CLR_RT_TypeSystem.Relocate();
 }
 
 //--//

src/CLR/Core/GarbageCollector.cpp

@@ -734,22 +734,23 @@ void CLR_RT_GarbageCollector::Assembly_Mark()
 
 #if !defined(NANOCLR_APPDOMAINS)
         CheckMultipleBlocks(pASSM->staticFields, pASSM->staticFieldsCount);
-
-        // Mark generic static fields for each TypeSpec
-        for (int i = 0; i < pASSM->tablesSize[TBL_TypeSpec]; i++)
-        {
-            CLR_RT_TypeSpec_CrossReference &ts = pASSM->crossReferenceTypeSpec[i];
-
-            if (ts.genericStaticFields != nullptr && ts.genericStaticFieldsCount > 0)
-            {
-                CheckMultipleBlocks(ts.genericStaticFields, ts.genericStaticFieldsCount);
-            }
-        }
 #endif
 
         CheckSingleBlock(&pASSM->file);
     }
     NANOCLR_FOREACH_ASSEMBLY_END();
+
+    // Generic static fields are rooted in the global registry, not per assembly: marking has to walk
+    // the same array relocation does. See CLAUDE.md "Static fields on generic types".
+    for (CLR_UINT32 i = 0; i < g_CLR_RT_TypeSystem.m_genericStaticFieldsCount; i++)
+    {
+        CLR_RT_GenericStaticFieldRecord &record = g_CLR_RT_TypeSystem.m_genericStaticFields[i];
+
+        if (record.m_fields != nullptr && record.m_count > 0)
+        {
+            CheckMultipleBlocks(record.m_fields, record.m_count);
+        }
+    }
 }
 
 //--//

src/CLR/Core/TypeSystem.cpp

@@ -5860,6 +5860,39 @@ HRESULT CLR_RT_Assembly::ResolveAllocateStaticFields(CLR_RT_HeapBlock *pStaticFi
     NANOCLR_NOCLEANUP();
 }
 
+// The slots are indexed as an array, so the run has to stay contiguous and unmoved: see CLAUDE.md
+// "Static fields on generic types".
+static CLR_RT_HeapBlock *CLR_AllocateGenericStaticFieldStorage(CLR_UINT32 count)
+{
+    NATIVE_PROFILE_CLR_CORE();
+
+    const CLR_UINT32 headerBlocks = CONVERTFROMSIZETOHEAPBLOCKS(sizeof(CLR_RT_HeapBlock_BinaryBlob));
+
+    auto *blob = (CLR_RT_HeapBlock_BinaryBlob *)g_CLR_RT_ExecutionEngine.ExtractHeapBlocksForEvents(
+        DATATYPE_BINARY_BLOB_HEAD,
+        0,
+        headerBlocks + count);
+
+    if (blob == nullptr)
+    {
+        return nullptr;
+    }
+
+    // no handlers: the registry marks and relocates the payload, see CLR_RT_TypeSystem::Relocate
+    blob->SetBinaryBlobHandlers(nullptr, nullptr);
+    blob->m_assembly = nullptr;
+
+    CLR_RT_HeapBlock *fields = (CLR_RT_HeapBlock *)blob + headerBlocks;
+
+    // the record is registered before the slots are typed, so they have to be walkable right away
+    for (CLR_UINT32 i = 0; i < count; i++)
+    {
+        fields[i].SetObjectReference(nullptr);
+    }
+
+    return fields;
+}
+
 HRESULT CLR_RT_Assembly::ResolveAllocateGenericTypeStaticFields()
 {
     NATIVE_PROFILE_CLR_CORE();
@@ -5977,26 +6010,24 @@ HRESULT CLR_RT_Assembly::ResolveAllocateGenericTypeStaticFields()
             g_CLR_RT_TypeSystem.m_genericStaticFieldsMaxCount = newMax;
         }
 
-        // Allocate storage for the static fields
-        CLR_RT_HeapBlock *fields = g_CLR_RT_ExecutionEngine.ExtractHeapBlocksForObjects(
-            DATATYPE_OBJECT, // heapblock kind
-            0,               // flags
-            count);          // number of CLR_RT_HeapBlock entries
+        // Allocate mapping for field definitions first: a platform_malloc failure here is trivial to
+        // unwind, whereas the GC heap blob allocated below has no direct release path and would
+        // otherwise leak until the next GC cycle if allocated first and this failed.
+        CLR_RT_FieldDef_Index *fieldDefs =
+            (CLR_RT_FieldDef_Index *)platform_malloc(sizeof(CLR_RT_FieldDef_Index) * count);
 
-        if (fields == nullptr)
+        if (fieldDefs == nullptr)
         {
             NANOCLR_SET_AND_LEAVE(CLR_E_OUT_OF_MEMORY);
         }
 
-        // Allocate mapping for field definitions
-        CLR_RT_FieldDef_Index *fieldDefs =
-            (CLR_RT_FieldDef_Index *)platform_malloc(sizeof(CLR_RT_FieldDef_Index) * count);
+        // Allocate storage for the static fields
+        CLR_RT_HeapBlock *fields = CLR_AllocateGenericStaticFieldStorage(count);
 
-        if (fieldDefs == nullptr)
+        if (fields == nullptr)
         {
-            // Free already allocated fields
-            // Since we don't have a direct ReleaseHeapBlocksForObjects function,
-            // we'll need to have the GC clean it up later
+            // Field defs mapping isn't published anywhere yet, so it can be freed directly
+            platform_free(fieldDefs);
             NANOCLR_SET_AND_LEAVE(CLR_E_OUT_OF_MEMORY);
         }
 
@@ -6246,22 +6277,22 @@ HRESULT CLR_RT_Assembly::AllocateGenericStaticFieldsOnDemand(
         g_CLR_RT_TypeSystem.m_genericStaticFieldsMaxCount = newMax;
     }
 
-    // Allocate storage for the static fields
-    fields = g_CLR_RT_ExecutionEngine.ExtractHeapBlocksForObjects(
-        DATATYPE_OBJECT, // heapblock kind
-        0,               // flags
-        count);          // number of CLR_RT_HeapBlock entries
+    // Allocate mapping for field definitions first: a platform_malloc failure here is trivial to
+    // unwind, whereas the GC heap blob allocated below has no direct release path.
+    fieldDefs = (CLR_RT_FieldDef_Index *)platform_malloc(sizeof(CLR_RT_FieldDef_Index) * count);
 
-    if (fields == nullptr)
+    if (fieldDefs == nullptr)
     {
         NANOCLR_SET_AND_LEAVE(CLR_E_OUT_OF_MEMORY);
     }
 
-    // Allocate mapping for field definitions
-    fieldDefs = (CLR_RT_FieldDef_Index *)platform_malloc(sizeof(CLR_RT_FieldDef_Index) * count);
+    // Allocate storage for the static fields
+    fields = CLR_AllocateGenericStaticFieldStorage(count);
 
-    if (fieldDefs == nullptr)
+    if (fields == nullptr)
     {
+        // Field defs mapping isn't published anywhere yet, so it can be freed directly
+        platform_free(fieldDefs);
         NANOCLR_SET_AND_LEAVE(CLR_E_OUT_OF_MEMORY);
     }
 
@@ -7200,36 +7231,6 @@ void CLR_RT_Assembly::Relocate()
     CLR_RT_GarbageCollector::Heap_Relocate(staticFields, staticFieldsCount);
 #endif
 
-    // Relocate all generic static field entries
-    for (CLR_UINT32 i = 0; i < g_CLR_RT_TypeSystem.m_genericStaticFieldsCount; i++)
-    {
-        CLR_RT_GarbageCollector::RelocateGenericStaticField(&g_CLR_RT_TypeSystem.m_genericStaticFields[i]);
-    }
-
-    // Resync TypeSpec cross-ref caches into the relocated generic static field arrays
-    // (they are platform_malloc'd, so GC relocation above does not update them).
-    for (CLR_UINT32 i = 0; i < g_CLR_RT_TypeSystem.m_genericStaticFieldsCount; i++)
-    {
-        const CLR_RT_GenericStaticFieldRecord &record = g_CLR_RT_TypeSystem.m_genericStaticFields[i];
-
-        // Walk every assembly, every TypeSpec cross-reference, and update the cache if it was
-        // pointing at this record's old field block.
-        NANOCLR_FOREACH_ASSEMBLY(g_CLR_RT_TypeSystem)
-        {
-            for (int tsIdx = 0; tsIdx < pASSM->tablesSize[TBL_TypeSpec]; tsIdx++)
-            {
-                CLR_RT_TypeSpec_CrossReference &tsCross = pASSM->crossReferenceTypeSpec[tsIdx];
-
-                if (tsCross.genericStaticFields != nullptr && tsCross.genericStaticFieldsCount == record.m_count &&
-                    tsCross.genericStaticFieldDefs == record.m_fieldDefs)
-                {
-                    tsCross.genericStaticFields = record.m_fields;
-                }
-            }
-        }
-        NANOCLR_FOREACH_ASSEMBLY_END();
-    }
-
     CLR_RT_GarbageCollector::Heap_Relocate((void **)&header);
     CLR_RT_GarbageCollector::Heap_Relocate((void **)&name);
     CLR_RT_GarbageCollector::Heap_Relocate((void **)&file);
@@ -7238,6 +7239,20 @@ void CLR_RT_Assembly::Relocate()
 
 ////////////////////////////////////////////////////////////////////////////////////////////////////
 
+void CLR_RT_TypeSystem::Relocate()
+{
+    NATIVE_PROFILE_CLR_CORE();
+
+    // The registry is global, so this runs once per relocation pass, never per assembly:
+    // see CLAUDE.md "Static fields on generic types".
+    for (CLR_UINT32 i = 0; i < m_genericStaticFieldsCount; i++)
+    {
+        CLR_RT_GarbageCollector::RelocateGenericStaticField(&m_genericStaticFields[i]);
+    }
+}
+
+////////////////////////////////////////////////////////////////////////////////////////////////////
+
 void CLR_RT_TypeSystem::TypeSystem_Initialize()
 {
     NATIVE_PROFILE_CLR_CORE();
@@ -9325,34 +9340,24 @@ CLR_RT_GenericStaticFieldRecord *CLR_RT_TypeSystem::FindOrCreateGenericStaticFie
     // Get the type definition record
     const CLR_RECORD_TYPEDEF *ownerTd = ownerAssembly->GetTypeDef(typeDef.Type());
 
-    // Allocate storage for the static fields
-    CLR_RT_HeapBlock *pFields = g_CLR_RT_ExecutionEngine.ExtractHeapBlocksForObjects(
-        DATATYPE_OBJECT, // Use OBJECT type for proper GC management
-        0,               // No special flags
-        staticFieldCount // Number of fields
-    );
-
-    if (pFields == nullptr)
-    {
-        return nullptr; // Out of memory
-    }
-
-    // Allocate mapping for field definitions using platform_malloc since we need to manage this memory separately
+    // Allocate mapping for field definitions first using platform_malloc: if this fails there's
+    // nothing to unwind. Doing this before the GC heap blob avoids leaking GC-managed memory
+    // that has no direct release path.
     CLR_RT_FieldDef_Index *pFieldDefs =
         (CLR_RT_FieldDef_Index *)platform_malloc(sizeof(CLR_RT_FieldDef_Index) * staticFieldCount);
 
     if (pFieldDefs == nullptr)
     {
-        // Unable to allocate field definitions, must clean up the fields we already allocated
-        // Since ExtractHeapBlocksForObjects allocates memory that's managed by the GC,
-        // we don't explicitly free it. The next GC cycle will reclaim it.
+        return nullptr; // Out of memory
+    }
 
-        // Reset the allocated fields to null to ensure no dangling references
-        for (CLR_UINT32 i = 0; i < staticFieldCount; i++)
-        {
-            pFields[i].SetObjectReference(nullptr);
-        }
+    // Allocate storage for the static fields
+    CLR_RT_HeapBlock *pFields = CLR_AllocateGenericStaticFieldStorage(staticFieldCount);
 
+    if (pFields == nullptr)
+    {
+        // Field defs mapping isn't published anywhere yet, so it can be freed directly
+        platform_free(pFieldDefs);
         return nullptr; // Out of memory
     }
 

src/CLR/Include/nanoCLR_Runtime.h

@@ -2077,6 +2077,8 @@ struct CLR_RT_TypeSystem // EVENT HEAP - NO RELOCATION -
     void TypeSystem_Initialize();
     void TypeSystem_Cleanup();
 
+    void Relocate();
+
     void Link(CLR_RT_Assembly *assm);
     void PostLinkageProcessing(CLR_RT_Assembly *assm);
 
@@ -3228,18 +3230,9 @@ struct CLR_RT_GarbageCollector
     {
         if (field->m_fields)
         {
-            // Relocate the internal pointers within each HeapBlock in the array
-            // (must be done before updating m_fields, while it still points to the old location)
+            // The slots sit inside an unmovable blob the heap walk steps over, so they are relocated
+            // here and nowhere else. See CLAUDE.md "Static fields on generic types".
             CLR_RT_GarbageCollector::Heap_Relocate(field->m_fields, field->m_count);
-
-            // Update m_fields pointer itself to wherever the block array moved after compaction.
-            // Without this, m_fields becomes a dangling pointer after any GC compaction.
-            CLR_RT_GarbageCollector::Heap_Relocate((void **)&field->m_fields);
-        }
-
-        if (field->m_fieldDefs)
-        {
-            CLR_RT_GarbageCollector::Heap_Relocate((void **)&field->m_fieldDefs);
         }
     }