Skip to content

Commit 951bc70

Browse files
authored
Move assert checking the size of structs with GC pointers. (#34053)
Codegen for CpObj assumes that we cannot have a struct with GC pointers whose size is not a multiple of the register size. We had an assert that verified that in `ClassLayout::InitializeGCPtrs`. The assert fired in the new pipeline that tests off-by-default features in the leg that turns on object stack allocation. Stack-allocated objects are never copied so the assert shouldn't apply to them. I moved the assert to `Compiler::gtNewCpObjNode`. I clarified the comment for the assert and changed it from `noway_assert` to a regular `assert` since recompilation with minopts will not help if we hit the assert. I also added a repro case to ObjectStackAllocationTests that run with COMPlus_JitObjectStackAllocation=1 in regular test runs.
1 parent 1926a3c commit 951bc70

File tree

3 files changed

+34
-9
lines changed

3 files changed

+34
-9
lines changed

src/coreclr/src/jit/gentree.cpp

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6579,7 +6579,19 @@ GenTree* Compiler::gtNewCpObjNode(GenTree* dstAddr, GenTree* srcAddr, CORINFO_CL
65796579

65806580
if (lhs->OperIs(GT_OBJ))
65816581
{
6582-
gtSetObjGcInfo(lhs->AsObj());
6582+
GenTreeObj* lhsObj = lhs->AsObj();
6583+
#if DEBUG
6584+
// Codegen for CpObj assumes that we cannot have a struct with GC pointers whose size is not a multiple
6585+
// of the register size. The EE currently does not allow this to ensure that GC pointers are aligned
6586+
// if the struct is stored in an array. Note that this restriction doesn't apply to stack-allocated objects:
6587+
// they are never stored in arrays. We should never get to this method with stack-allocated objects since they
6588+
// are never copied so we don't need to exclude them from the assert below.
6589+
// Let's assert it just to be safe.
6590+
ClassLayout* layout = lhsObj->GetLayout();
6591+
unsigned size = layout->GetSize();
6592+
assert((layout->GetGCPtrCount() == 0) || (roundUp(size, REGSIZE_BYTES) == size));
6593+
#endif
6594+
gtSetObjGcInfo(lhsObj);
65836595
}
65846596

65856597
if (srcAddr->OperGet() == GT_ADDR)

src/coreclr/src/jit/layout.cpp

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -373,12 +373,6 @@ void ClassLayout::InitializeGCPtrs(Compiler* compiler)
373373
// so it should be safe to fit this into a 30 bits bit field.
374374
assert(gcPtrCount < (1 << 30));
375375

376-
// We assume that we cannot have a struct with GC pointers that is not a multiple
377-
// of the register size.
378-
// The EE currently does not allow this, but it could change.
379-
// Let's assert it just to be safe.
380-
noway_assert((gcPtrCount == 0) || (roundUp(m_size, REGSIZE_BYTES) == m_size));
381-
382376
m_gcPtrCount = gcPtrCount;
383377
}
384378

src/coreclr/tests/src/JIT/opt/ObjectStackAllocation/ObjectStackAllocationTests.cs

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,18 @@ public SimpleClassWithGCField(int f1, int f2, object o) : base(f1, f2)
4343
}
4444
}
4545

46+
class ClassWithGCFieldAndInt
47+
{
48+
public object o;
49+
public int i;
50+
51+
public ClassWithGCFieldAndInt(int i, object o)
52+
{
53+
this.o = o;
54+
this.i = i;
55+
}
56+
}
57+
4658
class ClassWithNestedStruct
4759
{
4860
public ClassWithNestedStruct(int f1, int f2)
@@ -137,6 +149,8 @@ public static int Main()
137149
// We don't have to use a helper in this case (even for R2R), https://github.com/dotnet/coreclr/issues/22086 tracks fixing that.
138150
CallTestAndVerifyAllocation(AllocateSimpleClassAndCheckTypeNoHelper, 1, AllocationKind.Undefined);
139151

152+
CallTestAndVerifyAllocation(AllocateClassWithGcFieldAndInt, 5, expectedAllocationKind);
153+
140154
// The remaining tests currently never allocate on the stack
141155
if (expectedAllocationKind == AllocationKind.Stack) {
142156
expectedAllocationKind = AllocationKind.Heap;
@@ -170,8 +184,6 @@ static bool GCStressEnabled()
170184

171185
static void CallTestAndVerifyAllocation(Test test, int expectedResult, AllocationKind expectedAllocationsKind)
172186
{
173-
// Run the test once to exclude any allocations during jitting, etc.
174-
//test();
175187
long allocatedBytesBefore = GC.GetAllocatedBytesForCurrentThread();
176188
int testResult = test();
177189
long allocatedBytesAfter = GC.GetAllocatedBytesForCurrentThread();
@@ -326,5 +338,12 @@ static int TestMixOfReportingAndWriteBarriers()
326338

327339
return c1.o.ToString().Length + c2.o.ToString().Length + c3.o.ToString().Length + c4.o.ToString().Length;
328340
}
341+
342+
static int AllocateClassWithGcFieldAndInt()
343+
{
344+
ClassWithGCFieldAndInt c = new ClassWithGCFieldAndInt(f1, null);
345+
GC.Collect();
346+
return c.i;
347+
}
329348
}
330349
}

0 commit comments

Comments
 (0)