[release/8.0] JIT: Disallow mismatched GC-ness for physical promotions (#90739)

* 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

* Address feedback

---------

Co-authored-by: Jakob Botsch Nielsen <jakob.botsch.nielsen@gmail.com>

diff --git a/src/coreclr/jit/layout.cpp b/src/coreclr/jit/layout.cpp
index 113414d..918fd4a 100644 (file)
--- 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();
@@ -436,6 +437,39 @@ bool ClassLayout::HasGCByRef() const
 }
 
 //------------------------------------------------------------------------
+// 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
+{
+    if (!HasGCPtr())
+    {
+        return false;
+    }
+
+    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.
 //
 // Arguments:

diff --git a/src/coreclr/jit/layout.h b/src/coreclr/jit/layout.h
index 0e9d6ed..59ecaa9 100644 (file)
--- a/src/coreclr/jit/layout.h
+++ b/src/coreclr/jit/layout.h
@@ -216,6 +216,8 @@ public:
         }
     }
 
+    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 e2c4e79..5982ed7 100644 (file)
--- a/src/coreclr/jit/promotion.cpp
+++ b/src/coreclr/jit/promotion.cpp
@@ -621,6 +621,38 @@ public:
     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 @@ public:
 
         // 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)
         {