Skip to content

Commit

Permalink
JIT: Disallow mismatched GC-ness for physical promotions
Browse files Browse the repository at this point in the history
Physical promotion was working under the assumption that reinterpreting
GC pointers is undefined behavior, and would happily promote GC pointers
as integers if it saw such accesses. However, physical promotion is
function wide while the UB accesses can be happening in a restricted
(dynamically unreachable) scope. This exact situation happens in
MemoryExtensions.Contains. The issue was uncovered under jit stress
where we did not fold away the guard early enough, meaning that
promotion then saw a `TYP_LONG` access of a `struct { object, int }` and
proceeded to promote it as such.

Fix #90602
  • Loading branch information
jakobbotsch authored and github-actions committed Aug 17, 2023
1 parent 68213d0 commit 62946ee
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 3 deletions.
29 changes: 29 additions & 0 deletions src/coreclr/jit/layout.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,7 @@ void ClassLayout::InitializeGCPtrs(Compiler* compiler)
//
// Return value:
// true if at least one GC ByRef, false otherwise.
//
bool ClassLayout::HasGCByRef() const
{
unsigned slots = GetSlotCount();
Expand All @@ -435,6 +436,34 @@ bool ClassLayout::HasGCByRef() const
return false;
}

//------------------------------------------------------------------------
// IntersectsGCPtr: check if the specified interval intersects with a GC
// pointer.
//
// Parameters:
// offset - The start offset of the interval
// size - The size of the interval
//
// Return value:
// True if it does.
//
bool ClassLayout::IntersectsGCPtr(unsigned offset, unsigned size) const
{
unsigned startSlot = offset / TARGET_POINTER_SIZE;
unsigned endSlot = (offset + size - 1) / TARGET_POINTER_SIZE;
assert((startSlot < GetSlotCount()) && (endSlot < GetSlotCount()));

for (unsigned i = startSlot; i <= endSlot; i++)
{
if (IsGCPtr(i))
{
return true;
}
}

return false;
}

//------------------------------------------------------------------------
// AreCompatible: check if 2 layouts are the same for copying.
//
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/jit/layout.h
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,8 @@ class ClassLayout
}
}

bool IntersectsGCPtr(unsigned offset, unsigned size) const;

static bool AreCompatible(const ClassLayout* layout1, const ClassLayout* layout2);

private:
Expand Down
37 changes: 34 additions & 3 deletions src/coreclr/jit/promotion.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -621,6 +621,38 @@ class LocalUses
bool EvaluateReplacement(
Compiler* comp, unsigned lclNum, const Access& access, unsigned inducedCount, weight_t inducedCountWtd)
{
// Verify that this replacement has proper GC ness compared to the
// layout. While reinterpreting GC fields to integers can be considered
// UB, there are scenarios where it can happen safely:
//
// * The user code could have guarded the access with a dynamic check
// that it doesn't contain a GC pointer, so that the access is actually
// in dead code. This happens e.g. in span functions in SPC.
//
// * For byrefs, reinterpreting as an integer could be ok in a
// restricted scope due to pinning.
//
// In theory we could allow these promotions in the restricted scope,
// but currently physical promotion works on a function-wide basis.

LclVarDsc* lcl = comp->lvaGetDesc(lclNum);
ClassLayout* layout = lcl->GetLayout();
if (layout->IntersectsGCPtr(access.Offset, genTypeSize(access.AccessType)))
{
if (((access.Offset % TARGET_POINTER_SIZE) != 0) ||
(layout->GetGCPtrType(access.Offset / TARGET_POINTER_SIZE) != access.AccessType))
{
return false;
}
}
else
{
if (varTypeIsGC(access.AccessType))
{
return false;
}
}

unsigned countOverlappedCallArg = 0;
unsigned countOverlappedStoredFromCall = 0;

Expand Down Expand Up @@ -678,9 +710,8 @@ class LocalUses

// Now look at the overlapping struct uses that promotion will make more expensive.

unsigned countReadBacks = 0;
weight_t countReadBacksWtd = 0;
LclVarDsc* lcl = comp->lvaGetDesc(lclNum);
unsigned countReadBacks = 0;
weight_t countReadBacksWtd = 0;
// For parameters or OSR locals we always need one read back.
if (lcl->lvIsParam || lcl->lvIsOSRLocal)
{
Expand Down

0 comments on commit 62946ee

Please sign in to comment.