Incomplete read ordering in CastCache::Get

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,20 @@ 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'
+#if TARGET_ARM64 || TARGET_ARM
+                        // We can either:
+                        // - use acquires for both _source and _targetAndResults or
+                        // - issue a full fence before reading _version
+                        // benchmarks on available hardware show that use of full fence here is cheaper.
+                        Interlocked.MemoryBarrier();
+#endif
                         if (version != pEntry._version)
                         {
                             // oh, so close, the entry is in inconsistent state.

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/vm/castcache.cpp

@@ -160,7 +160,7 @@ TypeHandle::CastResult CastCache::TryGet(TADDR source, TADDR target)
     {
         CastCacheEntry* pEntry = &Elements(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
         DWORD version1 = VolatileLoad(&pEntry->version);
         TADDR entrySource = pEntry->source;
@@ -171,12 +171,14 @@ TypeHandle::CastResult CastCache::TryGet(TADDR source, TADDR target)
 
         if (entrySource == source)
         {
-            TADDR entryTargetAndResult = VolatileLoad(&pEntry->targetAndResult);
+            TADDR entryTargetAndResult = pEntry->targetAndResult;
             // target never has its lower bit set.
             // a matching entryTargetAndResult would have the 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'
+                VolatileLoadBarrier();
                 if (version1 != pEntry->version)
                 {
                     // oh, so close, the entry is in inconsistent state.