Cast cache Add should handle odd entry versions as inconsistent/taken. (#41109)

* Cast cache Add should handle odd entry versions as inconsistent/taken

* PR feedback

diff --git a/src/coreclr/src/vm/castcache.cpp b/src/coreclr/src/vm/castcache.cpp
index 6613c6f..bc5831a 100644 (file)
--- a/src/coreclr/src/vm/castcache.cpp
+++ b/src/coreclr/src/vm/castcache.cpp
@@ -244,7 +244,23 @@ void CastCache::TrySet(TADDR source, TADDR target, BOOL result)
             //        We improve average lookup by giving preference to the "richer" entries.
             //        If we used Robin Hood strategy we could eventually end up with all
             //        entries in the table being maximally "poor".
-            DWORD version = pEntry->version;
+
+            // VolatileLoadWithoutBarrier is to ensure that the version cannot be re-fetched between here and CompareExchange.
+            DWORD version = VolatileLoadWithoutBarrier(&pEntry->version);
+
+            // mask the lower version bit to make it even.
+            // This way we will detect both if version is changing (odd) or has changed (even, but different).
+            version &= ~1;
+
+            if ((version & VERSION_NUM_MASK) >= (VERSION_NUM_MASK - 2))
+            {
+                // If exactly VERSION_NUM_MASK updates happens between here and publishing, we may not recognise a race.
+                // It is extremely unlikely, but to not worry about the possibility, lets not allow version to go this high and just get a new cache.
+                // This will not happen often.
+                FlushCurrentCache();
+                return;
+            }
+
             if (version == 0 || (version >> VERSION_NUM_SIZE) > i)
             {
                 DWORD newVersion = (i << VERSION_NUM_SIZE) + (version & VERSION_NUM_MASK) + 1;
@@ -294,11 +310,18 @@ void CastCache::TrySet(TADDR source, TADDR target, BOOL result)
     {
         CastCacheEntry* pEntry = &Elements(tableData)[(bucket + victim) & TableMask(tableData)];
 
-        DWORD version = pEntry->version;
+        // VolatileLoadWithoutBarrier is to ensure that the version cannot be re-fetched between here and CompareExchange.
+        DWORD version = VolatileLoadWithoutBarrier(&pEntry->version);
+
+        // mask the lower version bit to make it even.
+        // This way we will detect both if version is changing (odd) or has changed (even, but different).
+        version &= ~1;
+
         if ((version & VERSION_NUM_MASK) >= (VERSION_NUM_MASK - 2))
         {
-            // It is unlikely for a reader to sit between versions while exactly 2^VERSION_NUM_SIZE updates happens.
-            // Anyways, to not bother about the possibility, lets get a new cache. It will not happen often, if ever.
+            // If exactly VERSION_NUM_MASK updates happens between here and publishing, we may not recognise a race.
+            // It is extremely unlikely, but to not worry about the possibility, lets not allow version to go this high and just get a new cache.
+            // This will not happen often.
             FlushCurrentCache();
             return;
         }