From 62946ee15e59d36500d66fcbc066e8388d7af9bd Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 16 Aug 2023 22:31:11 +0200 Subject: [PATCH 1/2] JIT: Disallow mismatched GC-ness for physical promotions 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 --- src/coreclr/jit/layout.cpp | 29 +++++++++++++++++++++++++++ src/coreclr/jit/layout.h | 2 ++ src/coreclr/jit/promotion.cpp | 37 ++++++++++++++++++++++++++++++++--- 3 files changed, 65 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/layout.cpp b/src/coreclr/jit/layout.cpp index 113414ddfd7f7..d9069224aff1c 100644 --- a/src/coreclr/jit/layout.cpp +++ b/src/coreclr/jit/layout.cpp @@ -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(); @@ -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. // diff --git a/src/coreclr/jit/layout.h b/src/coreclr/jit/layout.h index 0e9d6ed65d03d..59ecaa9405485 100644 --- a/src/coreclr/jit/layout.h +++ b/src/coreclr/jit/layout.h @@ -216,6 +216,8 @@ class ClassLayout } } + bool IntersectsGCPtr(unsigned offset, unsigned size) const; + static bool AreCompatible(const ClassLayout* layout1, const ClassLayout* layout2); private: diff --git a/src/coreclr/jit/promotion.cpp b/src/coreclr/jit/promotion.cpp index e2c4e797a3c9d..5982ed7928335 100644 --- a/src/coreclr/jit/promotion.cpp +++ b/src/coreclr/jit/promotion.cpp @@ -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; @@ -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) { From ada908fa1724fe2844e346bfcb221adf124477e7 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 17 Aug 2023 11:39:15 +0200 Subject: [PATCH 2/2] Address feedback --- src/coreclr/jit/layout.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/coreclr/jit/layout.cpp b/src/coreclr/jit/layout.cpp index d9069224aff1c..918fd4ab6521d 100644 --- a/src/coreclr/jit/layout.cpp +++ b/src/coreclr/jit/layout.cpp @@ -449,6 +449,11 @@ bool ClassLayout::HasGCByRef() const // bool ClassLayout::IntersectsGCPtr(unsigned offset, unsigned size) const { + if (!HasGCPtr()) + { + return false; + } + unsigned startSlot = offset / TARGET_POINTER_SIZE; unsigned endSlot = (offset + size - 1) / TARGET_POINTER_SIZE; assert((startSlot < GetSlotCount()) && (endSlot < GetSlotCount()));