Skip to content

Commit 14d03c7

Browse files
authored
Fix HB array storing generic types (#3522)
***NO_CI***
1 parent 38ffa46 commit 14d03c7

8 files changed

Lines changed: 171 additions & 45 deletions

src/CLR/CorLib/corlib_native_System_ReadOnlySpan_1.cpp

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -65,12 +65,11 @@ HRESULT Library_corlib_native_System_ReadOnlySpan_1::_ctor___VOID__VOIDptr__I4(C
6565
}
6666

6767
// check if T is a reference type or contains references
68-
NANOCLR_CHECK_HRESULT(
69-
RuntimeHelpers::CheckReferenceOrContainsReferences(
70-
element.Class,
71-
element.DataType,
72-
&parser,
73-
isRefContainsRefs));
68+
NANOCLR_CHECK_HRESULT(RuntimeHelpers::CheckReferenceOrContainsReferences(
69+
element.Class,
70+
element.DataType,
71+
&parser,
72+
isRefContainsRefs));
7473

7574
if (isRefContainsRefs)
7675
{
@@ -96,8 +95,13 @@ HRESULT Library_corlib_native_System_ReadOnlySpan_1::_ctor___VOID__VOIDptr__I4(C
9695

9796
{
9897
CLR_RT_HeapBlock &refArray = thisSpan[FIELD___array];
99-
NANOCLR_CHECK_HRESULT(
100-
CLR_RT_HeapBlock_Array::CreateInstanceWithStorage(refArray, length, objectRawPointer, element.Class));
98+
// this ReadOnlySpan wraps raw unmanaged memory, not a managed array - no owner to keep alive/relocate
99+
NANOCLR_CHECK_HRESULT(CLR_RT_HeapBlock_Array::CreateInstanceWithStorage(
100+
refArray,
101+
length,
102+
objectRawPointer,
103+
element.Class,
104+
nullptr));
101105
}
102106

103107
// set length
@@ -175,8 +179,13 @@ HRESULT Library_corlib_native_System_ReadOnlySpan_1::NativeReadOnlySpanConstruct
175179
uintptr_t ptrToStartElement = (uintptr_t)sourceArray->GetElement(start);
176180

177181
CLR_RT_HeapBlock &refArray = thisSpan[FIELD___array];
178-
NANOCLR_CHECK_HRESULT(
179-
CLR_RT_HeapBlock_Array::CreateInstanceWithStorage(refArray, length, ptrToStartElement, sourceType));
182+
// keep sourceArray (which owns the wrapped storage) alive and tracked across GC/compaction
183+
NANOCLR_CHECK_HRESULT(CLR_RT_HeapBlock_Array::CreateInstanceWithStorage(
184+
refArray,
185+
length,
186+
ptrToStartElement,
187+
sourceType,
188+
sourceArray));
180189
}
181190

182191
// set length

src/CLR/CorLib/corlib_native_System_Span_1.cpp

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -64,12 +64,11 @@ HRESULT Library_corlib_native_System_Span_1::_ctor___VOID__VOIDptr__I4(CLR_RT_St
6464
}
6565

6666
// check if T is a reference type or contains references
67-
NANOCLR_CHECK_HRESULT(
68-
RuntimeHelpers::CheckReferenceOrContainsReferences(
69-
element.Class,
70-
element.DataType,
71-
&parser,
72-
isRefContainsRefs));
67+
NANOCLR_CHECK_HRESULT(RuntimeHelpers::CheckReferenceOrContainsReferences(
68+
element.Class,
69+
element.DataType,
70+
&parser,
71+
isRefContainsRefs));
7372

7473
if (isRefContainsRefs)
7574
{
@@ -95,8 +94,13 @@ HRESULT Library_corlib_native_System_Span_1::_ctor___VOID__VOIDptr__I4(CLR_RT_St
9594

9695
{
9796
CLR_RT_HeapBlock &refArray = thisSpan[FIELD___array];
98-
NANOCLR_CHECK_HRESULT(
99-
CLR_RT_HeapBlock_Array::CreateInstanceWithStorage(refArray, length, objectRawPointer, element.Class));
97+
// this Span wraps raw unmanaged memory, not a managed array - no owner to keep alive/relocate
98+
NANOCLR_CHECK_HRESULT(CLR_RT_HeapBlock_Array::CreateInstanceWithStorage(
99+
refArray,
100+
length,
101+
objectRawPointer,
102+
element.Class,
103+
nullptr));
100104
}
101105

102106
// set length
@@ -172,8 +176,13 @@ HRESULT Library_corlib_native_System_Span_1::NativeSpanConstructor___VOID__SZARR
172176
uintptr_t ptrToStartElement = (uintptr_t)sourceArray->GetElement(start);
173177

174178
CLR_RT_HeapBlock &refArray = thisSpan[FIELD___array];
175-
NANOCLR_CHECK_HRESULT(
176-
CLR_RT_HeapBlock_Array::CreateInstanceWithStorage(refArray, length, ptrToStartElement, sourceType));
179+
// keep sourceArray (which owns the wrapped storage) alive and tracked across GC/compaction
180+
NANOCLR_CHECK_HRESULT(CLR_RT_HeapBlock_Array::CreateInstanceWithStorage(
181+
refArray,
182+
length,
183+
ptrToStartElement,
184+
sourceType,
185+
sourceArray));
177186
}
178187

179188
// set length

src/CLR/Core/CLAUDE.md

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -839,3 +839,78 @@ instance.**
839839
pass 2 of `FindVirtualMethodDef`. If `MatchSignatureForVirtualDispatch`
840840
mishandles the VAR↔GENERICINST drain (specifically for multi-argument
841841
generics like `KeyValuePair<,>`), this is the symptom.
842+
843+
---
844+
845+
## 16. `Span<T>`/`ReadOnlySpan<T>` storage-pointer arrays and GC
846+
847+
Not generics-specific, but the same "storage array split/mis-relocated/
848+
swept" family as §9, so documented the same way: rationale here, source
849+
stays terse.
850+
851+
**Background.** `Span<T>`'s native backing (`corlib_native_System_Span_1.cpp`,
852+
`corlib_native_System_ReadOnlySpan_1.cpp`) doesn't reuse the wrapped
853+
`T[]`'s `CLR_RT_HeapBlock_Array` directly — it allocates a second, small
854+
"shell" `CLR_RT_HeapBlock_Array` via `CreateInstanceWithStorage` whose
855+
`ReflectionData().kind == REFLECTION_STORAGE_PTR`. The shell has no
856+
element storage of its own; `GetFirstElement()` returns a raw
857+
`m_StoragePointer` address that points into the *original* array's
858+
element data (or, for the `Span(void*, int)` ctor, into unmanaged
859+
memory). This lets slicing/wrapping avoid copying.
860+
861+
**The bug.** Originally the shell held nothing but that raw address —
862+
no `CLR_RT_HeapBlock` reference back to the array that actually owns the
863+
memory. Two independent failures followed from that:
864+
1. **Reachability.** `ComputeReachabilityGraphForMultipleBlocks`'s
865+
`DATATYPE_SZARRAY` case only marks an array's *elements* reachable,
866+
and only when `m_fReference` is set (never true for a shell, since
867+
`Span<T>` rejects reference-containing `T`). Nothing marked the
868+
*owning* array reachable through the shell, so a temporary like
869+
`new Span<int>(new int[n])` had no live reference to the backing
870+
`int[]` once the constructor returned — `--forcegc` would sweep it,
871+
filling it with `SENTINEL_RECOVERED` (`0xDFDFDFDF`,
872+
`CLR_RT_HeapCluster::RecoverFromGC` in `CLR_RT_HeapCluster.cpp`) while
873+
the shell's raw pointer kept pointing at that now-dead memory. This is
874+
the `CopyTo_WithLargeArray_ShouldCopyAllElements` failure signature:
875+
`Actual:<-538976289>` is `0xDFDFDFDF` as `int32`.
876+
2. **Compaction.** Even had the owner survived sweep, `m_StoragePointer`
877+
is a bare address with no type tag — `CLR_RT_HeapBlock_Array::Relocate()`
878+
had no way to know it needed adjusting when the owner's element data
879+
physically moved, so it would go stale across `--compactionaftergc`
880+
too.
881+
882+
Both require `--forcegc --compactionaftergc` together to reproduce
883+
reliably; either flag alone can miss it depending on allocation timing —
884+
about 1 run in 9–10 failed under both flags before the fix, which is why
885+
a single clean run proves nothing here (loop 15–20×, see §14).
886+
887+
**The fix.** `CreateInstanceWithStorage` now takes an `owner` reference
888+
(the array actually backing the memory — `nullptr` for the unmanaged-
889+
memory ctor) and allocates one extra `sizeof(CLR_RT_HeapBlock)` of
890+
storage beyond the shell's header (`extraBytes` threaded through
891+
`CLR_RT_HeapBlock_Array::CreateInstance` → `CLR_RT_ExecutionEngine::
892+
ExtractHeapBlocksForArray`). That slot — `StorageOwner()`, aliasing the
893+
same `&this[1]` address a normal array would use for element 0, which is
894+
otherwise unused on a storage-pointer shell — holds a real
895+
`SetObjectReference` to the owner. `ComputeReachabilityGraphForMultipleBlocks`
896+
marks it explicitly for `IsStoragePointer()` arrays, so the owner is
897+
reachable transitively through the shell like any other object
898+
reference. `CLR_RT_HeapBlock_Array::Relocate()` relocates that reference
899+
via the normal `Heap_Relocate(CLR_RT_HeapBlock*, 1)` path, then relocates
900+
`m_StoragePointer` itself via the generic `Heap_Relocate(void**)` address-
901+
range lookup — safe because that call only rewrites the stored address
902+
value by table lookup, it never dereferences memory at the (possibly
903+
not-yet-moved) target, so ordering within the compaction pass doesn't
904+
matter. A shell built over a shell (e.g. `Span.Slice` of a `Span`) chains
905+
correctly with no special-casing: each shell only tracks its immediate
906+
`sourceArray`, and marking/relocation recurse through the chain via the
907+
same generic per-object dispatch.
908+
909+
**If you see `SENTINEL_RECOVERED` (`0xDFDFDFDF`) or a stale value read
910+
through a `Span<T>`/`ReadOnlySpan<T>`:**
911+
- Confirm it reproduces only with `--forcegc` (sweep) or needs
912+
`--compactionaftergc` too (relocation) — tells you which of the two
913+
mechanisms above is implicated.
914+
- Check that the shell's `StorageOwner()` was actually set (i.e. the
915+
constructor path went through the array-backed `CreateInstanceWithStorage`
916+
call, not the raw-pointer one, which intentionally has no owner).

src/CLR/Core/CLR_RT_HeapBlock_Array.cpp

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,8 @@
1010
HRESULT CLR_RT_HeapBlock_Array::CreateInstance(
1111
CLR_RT_HeapBlock &reference,
1212
CLR_UINT32 length,
13-
const CLR_RT_ReflectionDef_Index &reflex)
13+
const CLR_RT_ReflectionDef_Index &reflex,
14+
CLR_UINT32 extraBytes)
1415
{
1516
NATIVE_PROFILE_CLR_CORE();
1617
NANOCLR_HEADER();
@@ -52,7 +53,8 @@ HRESULT CLR_RT_HeapBlock_Array::CreateInstance(
5253
NANOCLR_SET_AND_LEAVE(CLR_E_WRONG_TYPE);
5354
}
5455

55-
pArray = (CLR_RT_HeapBlock_Array *)g_CLR_RT_ExecutionEngine.ExtractHeapBlocksForArray(inst, length, reflex);
56+
pArray = (CLR_RT_HeapBlock_Array *)
57+
g_CLR_RT_ExecutionEngine.ExtractHeapBlocksForArray(inst, length, reflex, extraBytes);
5658
CHECK_ALLOCATION(pArray);
5759

5860
reference.SetObjectReference(pArray);
@@ -91,7 +93,8 @@ HRESULT CLR_RT_HeapBlock_Array::CreateInstanceWithStorage(
9193
CLR_RT_HeapBlock &reference,
9294
CLR_UINT32 length,
9395
const uintptr_t storageAddress,
94-
const CLR_RT_TypeDef_Index &cls)
96+
const CLR_RT_TypeDef_Index &cls,
97+
const CLR_RT_HeapBlock *owner)
9598
{
9699
NATIVE_PROFILE_CLR_CORE();
97100
NANOCLR_HEADER();
@@ -103,8 +106,8 @@ HRESULT CLR_RT_HeapBlock_Array::CreateInstanceWithStorage(
103106
reflex.levels = 1;
104107
reflex.data.type = cls;
105108

106-
// create an instance with ZERO length because there is no need to allocate storage
107-
NANOCLR_CHECK_HRESULT(CreateInstance(reference, 0, reflex));
109+
// ZERO length: elements live in someone else's storage. extraBytes reserves the StorageOwner() slot.
110+
NANOCLR_CHECK_HRESULT(CreateInstance(reference, 0, reflex, sizeof(CLR_RT_HeapBlock)));
108111

109112
thisArray = reference.DereferenceArray();
110113

@@ -114,6 +117,9 @@ HRESULT CLR_RT_HeapBlock_Array::CreateInstanceWithStorage(
114117
// adjust the number of elements with the provided length
115118
thisArray->m_numOfElements = length;
116119

120+
// see CLAUDE.md §16
121+
thisArray->StorageOwner()->SetObjectReference(owner);
122+
117123
NANOCLR_NOCLEANUP();
118124
}
119125

@@ -226,11 +232,23 @@ HRESULT CLR_RT_HeapBlock_Array::ClearElements(int index, int length)
226232
void CLR_RT_HeapBlock_Array::Relocate()
227233
{
228234
NATIVE_PROFILE_CLR_CORE();
229-
//
230-
// If the array is full of reference types, relocate each of them.
231-
//
232-
if (m_fReference)
235+
236+
if (IsStoragePointer())
237+
{
238+
// no owner => unmanaged-memory Span, nothing to relocate. See CLAUDE.md §16.
239+
CLR_RT_HeapBlock *owner = StorageOwner();
240+
241+
if (owner->Dereference() != nullptr)
242+
{
243+
CLR_RT_GarbageCollector::Heap_Relocate(owner, 1);
244+
CLR_RT_GarbageCollector::Heap_Relocate((void **)&m_StoragePointer);
245+
}
246+
}
247+
else if (m_fReference)
233248
{
249+
//
250+
// If the array is full of reference types, relocate each of them.
251+
//
234252
CLR_RT_GarbageCollector::Heap_Relocate((CLR_RT_HeapBlock *)GetFirstElement(), m_numOfElements);
235253
}
236254
}

src/CLR/Core/Execution.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1710,13 +1710,14 @@ CLR_INT32 CLR_RT_ExecutionEngine::GetNextThreadId()
17101710
CLR_RT_HeapBlock *CLR_RT_ExecutionEngine::ExtractHeapBlocksForArray(
17111711
CLR_RT_TypeDef_Instance &inst,
17121712
CLR_UINT32 length,
1713-
const CLR_RT_ReflectionDef_Index &reflex)
1713+
const CLR_RT_ReflectionDef_Index &reflex,
1714+
CLR_UINT32 extraBytes)
17141715
{
17151716
NATIVE_PROFILE_CLR_CORE();
17161717
NanoCLRDataType dt = (NanoCLRDataType)inst.target->dataType;
17171718
const CLR_RT_DataTypeLookup &dtl = c_CLR_RT_DataTypeLookup[dt];
17181719

1719-
CLR_UINT32 totLength = (CLR_UINT32)(sizeof(CLR_RT_HeapBlock_Array) + length * dtl.m_sizeInBytes);
1720+
CLR_UINT32 totLength = (CLR_UINT32)(sizeof(CLR_RT_HeapBlock_Array) + length * dtl.m_sizeInBytes + extraBytes);
17201721
CLR_UINT32 lengthHB = CONVERTFROMSIZETOHEAPBLOCKS(totLength);
17211722

17221723
if (lengthHB > CLR_RT_HeapBlock::HB_MaxSize)

src/CLR/Core/GarbageCollector_ComputeReachabilityGraph.cpp

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -232,19 +232,24 @@ bool CLR_RT_GarbageCollector::ComputeReachabilityGraphForMultipleBlocks(CLR_RT_H
232232
break;
233233

234234
case DATATYPE_SZARRAY:
235-
//
236-
// If the array is full of reference types, mark each of them.
237-
//
238-
{
239-
CLR_RT_HeapBlock_Array *array = (CLR_RT_HeapBlock_Array *)ptr;
235+
{
236+
CLR_RT_HeapBlock_Array *array = (CLR_RT_HeapBlock_Array *)ptr;
240237

241-
if (array->m_fReference)
242-
{
243-
lst = (CLR_RT_HeapBlock *)array->GetFirstElement();
244-
num = array->m_numOfElements;
245-
}
238+
if (array->IsStoragePointer())
239+
{
240+
// keep the array owning the wrapped storage alive (e.g. a Span<T>'s
241+
// backing array) - see CreateInstanceWithStorage / StorageOwner()
242+
lst = array->StorageOwner();
243+
num = 1;
246244
}
247-
break;
245+
else if (array->m_fReference)
246+
{
247+
// If the array is full of reference types, mark each of them.
248+
lst = (CLR_RT_HeapBlock *)array->GetFirstElement();
249+
num = array->m_numOfElements;
250+
}
251+
}
252+
break;
248253

249254
case DATATYPE_REFLECTION:
250255
break;

src/CLR/Include/nanoCLR_Runtime.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4281,7 +4281,8 @@ struct CLR_RT_ExecutionEngine
42814281
CLR_RT_HeapBlock *ExtractHeapBlocksForArray(
42824282
CLR_RT_TypeDef_Instance &inst,
42834283
CLR_UINT32 length,
4284-
const CLR_RT_ReflectionDef_Index &reflex);
4284+
const CLR_RT_ReflectionDef_Index &reflex,
4285+
CLR_UINT32 extraBytes = 0);
42854286
CLR_RT_HeapBlock *ExtractHeapBlocksForClassOrValueTypes(
42864287
CLR_UINT32 dataType,
42874288
CLR_UINT32 flags,

src/CLR/Include/nanoCLR_Runtime__HeapBlock.h

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1899,7 +1899,8 @@ struct CLR_RT_HeapBlock_Array : public CLR_RT_HeapBlock
18991899
static HRESULT CreateInstance(
19001900
CLR_RT_HeapBlock &reference,
19011901
CLR_UINT32 length,
1902-
const CLR_RT_ReflectionDef_Index &reflex);
1902+
const CLR_RT_ReflectionDef_Index &reflex,
1903+
CLR_UINT32 extraBytes = 0);
19031904
static HRESULT CreateInstance(CLR_RT_HeapBlock &reference, CLR_UINT32 length, const CLR_RT_TypeDef_Index &cls);
19041905
static HRESULT CreateInstance(
19051906
CLR_RT_HeapBlock &reference,
@@ -1912,7 +1913,8 @@ struct CLR_RT_HeapBlock_Array : public CLR_RT_HeapBlock
19121913
CLR_RT_HeapBlock &reference,
19131914
CLR_UINT32 length,
19141915
const uintptr_t storageAddress,
1915-
const CLR_RT_TypeDef_Index &cls);
1916+
const CLR_RT_TypeDef_Index &cls,
1917+
const CLR_RT_HeapBlock *owner);
19161918

19171919
CLR_UINT8 *GetFirstElement()
19181920
{
@@ -1953,6 +1955,12 @@ struct CLR_RT_HeapBlock_Array : public CLR_RT_HeapBlock
19531955
return (ReflectionData().kind == REFLECTION_STORAGE_PTR);
19541956
}
19551957

1958+
// Valid only when IsStoragePointer() is true. See CLAUDE.md §16.
1959+
CLR_RT_HeapBlock *StorageOwner()
1960+
{
1961+
return (CLR_RT_HeapBlock *)&this[1];
1962+
}
1963+
19561964
HRESULT ClearElements(int index, int length);
19571965

19581966
//--//

0 commit comments

Comments
 (0)