Incomplete read ordering in CastCache::Get (#35597)

* Incomplete read ordering in CastCache::Get

* Use LoadBarrier on the managed side as well.

* Rename to ReadMemoryBarrier

* PR feedback in the JIT

Author: VSadov

src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs

@@ -113,7 +113,7 @@ private static CastResult TryGet(nuint source, nuint target)
             {
                 ref CastCacheEntry pEntry = ref Element(ref tableData, index);
 
-                // must read in this order: version -> entry parts -> version
+                // must read in this order: version -> [entry parts] -> version
                 // if version is odd or changes, the entry is inconsistent and thus ignored
                 int version = Volatile.Read(ref pEntry._version);
                 nuint entrySource = pEntry._source;
@@ -124,12 +124,19 @@ private static CastResult TryGet(nuint source, nuint target)
 
                 if (entrySource == source)
                 {
-                    nuint entryTargetAndResult = Volatile.Read(ref pEntry._targetAndResult);
+                    nuint entryTargetAndResult = pEntry._targetAndResult;
                     // target never has its lower bit set.
                     // a matching entryTargetAndResult would the have same bits, except for the lowest one, which is the result.
                     entryTargetAndResult ^= target;
                     if (entryTargetAndResult <= 1)
                     {
+                        // make sure 'version' is loaded after 'source' and 'targetAndResults'
+                        //
+                        // We can either:
+                        // - use acquires for both _source and _targetAndResults or
+                        // - issue a load barrier before reading _version
+                        // benchmarks on available hardware show that use of a read barrier is cheaper.
+                        Interlocked.ReadMemoryBarrier();
                         if (version != pEntry._version)
                         {
                             // oh, so close, the entry is in inconsistent state.

src/coreclr/src/System.Private.CoreLib/src/System/Threading/Interlocked.CoreCLR.cs

@@ -232,6 +232,15 @@ public static long Read(ref long location) =>
         [MethodImpl(MethodImplOptions.InternalCall)]
         public static extern void MemoryBarrier();
 
+        /// <summary>
+        /// Synchronizes memory access as follows:
+        /// The processor that executes the current thread cannot reorder instructions in such a way that memory reads before
+        /// the call to <see cref="ReadMemoryBarrier"/> execute after memory accesses that follow the call to <see cref="ReadMemoryBarrier"/>.
+        /// </summary>
+        [Intrinsic]
+        [MethodImpl(MethodImplOptions.InternalCall)]
+        internal static extern void ReadMemoryBarrier();
+
         [DllImport(RuntimeHelpers.QCall, CharSet = CharSet.Unicode)]
         private static extern void _MemoryBarrierProcessWide();
 

src/coreclr/src/inc/corinfo.h

@@ -947,6 +947,7 @@ enum CorInfoIntrinsics
     CORINFO_INTRINSIC_InterlockedCmpXchg32,
     CORINFO_INTRINSIC_InterlockedCmpXchg64,
     CORINFO_INTRINSIC_MemoryBarrier,
+    CORINFO_INTRINSIC_MemoryBarrierLoad,
     CORINFO_INTRINSIC_GetCurrentManagedThread,
     CORINFO_INTRINSIC_GetManagedThreadId,
     CORINFO_INTRINSIC_ByReference_Ctor,

src/coreclr/src/inc/volatile.h

@@ -286,6 +286,27 @@ void VolatileStoreWithoutBarrier(T* pt, T val)
 #endif
 }
 
+//
+// Memory ordering barrier that waits for loads in progress to complete.
+// Any effects of loads or stores that appear after, in program order, will "happen after" relative to this.
+// Other operations such as computation or instruction prefetch are not affected.
+//
+// Architectural mapping:
+//   arm64  : dmb ishld
+//   arm    : dmb ish
+//   x86/64 : compiler fence
+inline
+void VolatileLoadBarrier()
+{
+#if defined(HOST_ARM64) && defined(__GNUC__)
+    asm volatile ("dmb ishld" : : : "memory");
+#elif defined(HOST_ARM64) && defined(_MSC_VER)
+    __dmb(_ARM64_BARRIER_ISHLD);
+#else
+    VOLATILE_MEMORY_BARRIER();
+#endif   
+}
+
 //
 // Volatile<T> implements accesses with our volatile semantics over a variable of type T.
 // Wherever you would have used a "volatile Foo" or, equivalently, "Foo volatile", use Volatile<Foo>

src/coreclr/src/jit/codegen.h

@@ -1465,11 +1465,13 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 
     void instGen_Return(unsigned stkArgSize);
 
-#ifdef TARGET_ARM64
-    void instGen_MemoryBarrier(insBarrier barrierType = INS_BARRIER_ISH);
-#else
-    void instGen_MemoryBarrier();
-#endif
+    enum BarrierKind
+    {
+        BARRIER_FULL,      // full barrier
+        BARRIER_LOAD_ONLY, // load barier
+    };
+
+    void instGen_MemoryBarrier(BarrierKind barrierKind = BARRIER_FULL);
 
     void instGen_Set_Reg_To_Zero(emitAttr size, regNumber reg, insFlags flags = INS_FLAGS_DONT_CARE);
 

src/coreclr/src/jit/codegenarm64.cpp

@@ -2665,8 +2665,8 @@ void CodeGen::genCodeForCpObj(GenTreeObj* cpObjNode)
 
     if (cpObjNode->gtFlags & GTF_BLK_VOLATILE)
     {
-        // issue a INS_BARRIER_ISHLD after a volatile CpObj operation
-        instGen_MemoryBarrier(INS_BARRIER_ISHLD);
+        // issue a load barrier after a volatile CpObj operation
+        instGen_MemoryBarrier(BARRIER_LOAD_ONLY);
     }
 
     // Clear the gcInfo for REG_WRITE_BARRIER_SRC_BYREF and REG_WRITE_BARRIER_DST_BYREF.
@@ -2775,7 +2775,7 @@ void CodeGen::genLockedInstructions(GenTreeOp* treeNode)
                 assert(!"Unexpected treeNode->gtOper");
         }
 
-        instGen_MemoryBarrier(INS_BARRIER_ISH);
+        instGen_MemoryBarrier();
     }
     else
     {
@@ -2855,7 +2855,7 @@ void CodeGen::genLockedInstructions(GenTreeOp* treeNode)
 
         GetEmitter()->emitIns_J_R(INS_cbnz, EA_4BYTE, labelRetry, exResultReg);
 
-        instGen_MemoryBarrier(INS_BARRIER_ISH);
+        instGen_MemoryBarrier();
 
         gcInfo.gcMarkRegSetNpt(addr->gtGetRegMask());
     }
@@ -2904,7 +2904,7 @@ void CodeGen::genCodeForCmpXchg(GenTreeCmpXchg* treeNode)
         }
         GetEmitter()->emitIns_R_R_R(INS_casal, dataSize, targetReg, dataReg, addrReg);
 
-        instGen_MemoryBarrier(INS_BARRIER_ISH);
+        instGen_MemoryBarrier();
     }
     else
     {
@@ -2984,7 +2984,7 @@ void CodeGen::genCodeForCmpXchg(GenTreeCmpXchg* treeNode)
 
         genDefineTempLabel(labelCompareFail);
 
-        instGen_MemoryBarrier(INS_BARRIER_ISH);
+        instGen_MemoryBarrier();
 
         gcInfo.gcMarkRegSetNpt(addr->gtGetRegMask());
     }

src/coreclr/src/jit/codegenarmarch.cpp

@@ -398,8 +398,13 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode)
             break;
 
         case GT_MEMORYBARRIER:
-            instGen_MemoryBarrier();
+        {
+            CodeGen::BarrierKind barrierKind =
+                treeNode->gtFlags & GTF_MEMORYBARRIER_LOAD ? BARRIER_LOAD_ONLY : BARRIER_FULL;
+
+            instGen_MemoryBarrier(barrierKind);
             break;
+        }
 
 #ifdef TARGET_ARM64
         case GT_XCHG:
@@ -1944,11 +1949,9 @@ void CodeGen::genCodeForIndir(GenTreeIndir* tree)
 
     if (emitBarrier)
     {
-#ifdef TARGET_ARM64
-        instGen_MemoryBarrier(INS_BARRIER_ISHLD);
-#else
-        instGen_MemoryBarrier();
-#endif
+        // when INS_ldar* could not be used for a volatile load,
+        // we use an ordinary load followed by a load barrier.
+        instGen_MemoryBarrier(BARRIER_LOAD_ONLY);
     }
 
     genProduceReg(tree);
@@ -1980,13 +1983,8 @@ void CodeGen::genCodeForCpBlkHelper(GenTreeBlk* cpBlkNode)
 
     if (cpBlkNode->gtFlags & GTF_BLK_VOLATILE)
     {
-#ifdef TARGET_ARM64
-        // issue a INS_BARRIER_ISHLD after a volatile CpBlk operation
-        instGen_MemoryBarrier(INS_BARRIER_ISHLD);
-#else
-        // issue a full memory barrier after a volatile CpBlk operation
-        instGen_MemoryBarrier();
-#endif // TARGET_ARM64
+        // issue a load barrier after a volatile CpBlk operation
+        instGen_MemoryBarrier(BARRIER_LOAD_ONLY);
     }
 }
 
@@ -2207,6 +2205,7 @@ void CodeGen::genCodeForCpBlkUnroll(GenTreeBlk* node)
 
     if (node->IsVolatile())
     {
+        // issue a full memory barrier before a volatile CpBlk operation
         instGen_MemoryBarrier();
     }
 
@@ -2304,11 +2303,8 @@ void CodeGen::genCodeForCpBlkUnroll(GenTreeBlk* node)
 
     if (node->IsVolatile())
     {
-#ifdef TARGET_ARM64
-        instGen_MemoryBarrier(INS_BARRIER_ISHLD);
-#else
-        instGen_MemoryBarrier();
-#endif
+        // issue a load barrier after a volatile CpBlk operation
+        instGen_MemoryBarrier(BARRIER_LOAD_ONLY);
     }
 }
 

src/coreclr/src/jit/codegenxarch.cpp

@@ -1862,8 +1862,13 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode)
             break;
 
         case GT_MEMORYBARRIER:
-            instGen_MemoryBarrier();
+        {
+            CodeGen::BarrierKind barrierKind =
+                treeNode->gtFlags & GTF_MEMORYBARRIER_LOAD ? BARRIER_LOAD_ONLY : BARRIER_FULL;
+
+            instGen_MemoryBarrier(barrierKind);
             break;
+        }
 
         case GT_CMPXCHG:
             genCodeForCmpXchg(treeNode->AsCmpXchg());

src/coreclr/src/jit/gentree.h

@@ -814,6 +814,8 @@ struct GenTree
 #define GTF_CALL_POP_ARGS           0x04000000 // GT_CALL -- caller pop arguments?
 #define GTF_CALL_HOISTABLE          0x02000000 // GT_CALL -- call is hoistable
 
+#define GTF_MEMORYBARRIER_LOAD      0x40000000 // GT_MEMORYBARRIER -- Load barrier
+
 #define GTF_NOP_DEATH               0x40000000 // GT_NOP -- operand dies here
 
 #define GTF_FLD_VOLATILE            0x40000000 // GT_FIELD/GT_CLS_VAR -- same as GTF_IND_VOLATILE

src/coreclr/src/jit/importer.cpp

@@ -3627,11 +3627,20 @@ GenTree* Compiler::impIntrinsic(GenTree*                newobjThis,
 #endif // defined(TARGET_XARCH) || defined(TARGET_ARM64)
 
         case CORINFO_INTRINSIC_MemoryBarrier:
+        case CORINFO_INTRINSIC_MemoryBarrierLoad:
 
             assert(sig->numArgs == 0);
 
             op1 = new (this, GT_MEMORYBARRIER) GenTree(GT_MEMORYBARRIER, TYP_VOID);
             op1->gtFlags |= GTF_GLOB_REF | GTF_ASG;
+
+            // On XARCH `CORINFO_INTRINSIC_MemoryBarrierLoad` fences need not be emitted.
+            // However, we still need to capture the effect on reordering.
+            if (intrinsicID == CORINFO_INTRINSIC_MemoryBarrierLoad)
+            {
+                op1->gtFlags |= GTF_MEMORYBARRIER_LOAD;
+            }
+
             retNode = op1;
             break;