Skip to content

Commit

Permalink
[NativeAOT] Fix GCDesc computation (#88927)
Browse files Browse the repository at this point in the history
* Fix GCDesc computation

#86877 appears to have introduced a bug in the GCDesc computation.

Consider the following structure layout (we are on 32 bit):

 struct {
     int X1;
     int X2;
     Object Obj;
     int X3;
 }

Crucially, the object reference in this struct is placed at a non-zero offset, which means that in an
array GCDesc, sizeof(X1 + X2) aka 8 will be added to the "base size" of the object. Since we have one
and only series (of GC pointers), it will also be the last. Its "skip" was computed as:

 bitfield.Count (4) - last (3) = 1

Which is clearly incorrect, as we need to skip 3 pointers when considering the shifted array layout:

 <Obj, X3][X1, X2, Obj, X3][X1, X2, Obj, X3]...
     |            |
     [Correct skip]

In effect, for the last series, we must consider the skip to include the delta we have included into
the base size, which code before #86877 did, although wrongly - for MD arrays - as well. This change
restores a fixed version of it.

* Add a test

Verified to fail (hit a GC assert) before and pass after.

* (actually make it compile)
  • Loading branch information
SingleAccretion authored Jul 17, 2023
1 parent 4ebb3ea commit 6bd93db
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -526,17 +526,17 @@ private static unsafe int CreateArrayGCDesc(LowLevelList<bool> bitfield, int ran
int numSeries = 0;
int i = 0;

bool first = true;
int first = -1;
int last = 0;
short numPtrs = 0;
while (i < bitfield.Count)
{
if (bitfield[i])
{
if (first)
if (first == -1)
{
baseOffset += i;
first = false;
first = i;
baseOffset += first;
}
else if (gcdesc != null)
{
Expand Down Expand Up @@ -565,7 +565,7 @@ private static unsafe int CreateArrayGCDesc(LowLevelList<bool> bitfield, int ran
{
if (numSeries > 0)
{
*ptr-- = (short)((bitfield.Count - last) * IntPtr.Size);
*ptr-- = (short)((first + bitfield.Count - last) * IntPtr.Size);
*ptr-- = numPtrs;

*(void**)gcdesc = (void*)-numSeries;
Expand Down
57 changes: 57 additions & 0 deletions src/tests/nativeaot/SmokeTests/DynamicGenerics/B282745.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Collections.Generic;
using CoreFXTestLibrary;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Threading;
using System.Threading.Tasks;
using System.Reflection;
Expand Down Expand Up @@ -166,13 +167,69 @@ public static void test()
}
}

unsafe struct StructWithNonGCValuesAtZeroOffset<T>
{
// Generic structs cannot have explicit layout. We make do with a non-generic one.
public StructWithNonGCValuesAtZeroOffsetImpl v;
}

[StructLayout(LayoutKind.Explicit)]
struct StructWithNonGCValuesAtZeroOffsetImpl
{
[FieldOffset(0)]
public int i;
[FieldOffset(8)]
public object o;
[FieldOffset(16)]
public long l;
}

public class GenericTypeForStructWithNonGCValuesAtZeroOffset<T>
{
public static void test()
{
int[] lengths = { 1, 2, 3 };
StructWithNonGCValuesAtZeroOffset<T>[,,] array = (StructWithNonGCValuesAtZeroOffset<T>[,,])Array.CreateInstance(typeof(StructWithNonGCValuesAtZeroOffset<T>), lengths);

array[0, 0, 0].v.o = null;
array[0, 0, 0].v.i = GetIntPtrOnHeapAsInt();
array[0, 0, 0].v.l = GetIntPtrOnHeapAsLong();

array[0, 1, 2].v.o = null;
array[0, 1, 2].v.i = GetIntPtrOnHeapAsInt();
array[0, 1, 2].v.l = GetIntPtrOnHeapAsLong();

array[0, 1, 1].v.o = null;
array[0, 1, 1].v.i = GetIntPtrOnHeapAsInt();
array[0, 1, 1].v.l = GetIntPtrOnHeapAsLong();

GC.Collect();

GC.KeepAlive(array);

RuntimeTypeHandle arrayTypeHandle = array.GetType().TypeHandle;
#if INTERNAL_CONTRACTS
Assert.IsTrue(RuntimeAugments.IsDynamicType(arrayTypeHandle));
#endif
}
}

[MethodImpl(MethodImplOptions.NoInlining)]
[TestMethod]
public static void testMDArrayWithPointerLikeValuesOfKnownStructTypeLargerType()
{
GenericType1<object>.test();
}

[MethodImpl(MethodImplOptions.NoInlining)]
[TestMethod]
public static void testMDArrayWithPointerLikeValuesOfUnknownStructTypeWithNonGCValuesAtZeroOffset()
{
Type genType = typeof(GenericTypeForStructWithNonGCValuesAtZeroOffset<>).MakeGenericType(TypeOf.String);
MethodInfo m = genType.GetTypeInfo().GetDeclaredMethod("test");
m.Invoke(null, new object[] { });
}

[MethodImpl(MethodImplOptions.NoInlining)]
[TestMethod]
public static void testMDArrayWithPointerLikeValuesOfUnknownStructReferenceType()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ public static int Main(string[] args)
new CoreFXTestLibrary.Internal.TestInfo("B282745.testLongMDArrayWithPointerLikeValues", () => global::B282745.testLongMDArrayWithPointerLikeValues(), null),
new CoreFXTestLibrary.Internal.TestInfo("B282745.testMDArrayWithPointerLikeValuesOfKnownStructType", () => global::B282745.testMDArrayWithPointerLikeValuesOfKnownStructType(), null),
new CoreFXTestLibrary.Internal.TestInfo("B282745.testMDArrayWithPointerLikeValuesOfKnownStructTypeLargerType", () => global::B282745.testMDArrayWithPointerLikeValuesOfKnownStructTypeLargerType(), null),
new CoreFXTestLibrary.Internal.TestInfo("B282745.testMDArrayWithPointerLikeValuesOfUnknownStructTypeWithNonGCValuesAtZeroOffset", () => global::B282745.testMDArrayWithPointerLikeValuesOfUnknownStructTypeWithNonGCValuesAtZeroOffset(), null),
new CoreFXTestLibrary.Internal.TestInfo("B282745.testMDArrayWithPointerLikeValuesOfUnknownStructReferenceType", () => global::B282745.testMDArrayWithPointerLikeValuesOfUnknownStructReferenceType(), null),
new CoreFXTestLibrary.Internal.TestInfo("B282745.testMDArrayWithPointerLikeValuesOfUnknownStructPrimitiveType", () => global::B282745.testMDArrayWithPointerLikeValuesOfUnknownStructPrimitiveType(), null),
new CoreFXTestLibrary.Internal.TestInfo("B282745.testMDArrayWith3Dimensions", () => global::B282745.testMDArrayWith3Dimensions(), null),
Expand Down

0 comments on commit 6bd93db

Please sign in to comment.