Fix token memory ordering issue reading from the MethodDef and FieldDef token tables (#123557)

We have logic which loads a typedef, and if the typedef is loaded then
we assume the method can be loaded via lookup in the MethodDef/FieldDef
token map tables. HOWEVER, what this boils down to is we do a load from
the TypeDef table, and if that succeeds we will do a load from the
FieldDef/MethodDef token map tables, but that second load is done
without an explicit memory barrier. Unfortunately, through the wonder of
memory ordering behavior, its legal (and not actually all that uncommon)
for the CPU to do the load from the FieldDef/MethodDef tables BEFORE it
actually loads from the TypeDef table. So what can happen is that the
fielddef table read can happen, then the typedef table read can happen,
then the CPU does the if check to see if its ok to do the fielddef table
read, then we use the value from the fielddef table read which happened
earlier.

This fix should fix that problem by using a VolatileLoad for all reads
from the lookup maps. This is slightly slower but avoids the extremely
easy to mess up dependency ordering rules.

Fixes #120754

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Aaron R Robinson <arobins@microsoft.com>

Author: 3 people

src/coreclr/vm/ceeload.inl

@@ -18,7 +18,13 @@ TYPE LookupMap<TYPE>::GetValueAt(PTR_TADDR pValue, TADDR* pFlags, TADDR supporte
     WRAPPER_NO_CONTRACT;
     SUPPORTS_DAC;
 #ifndef DACCESS_COMPILE
-    TYPE value = dac_cast<TYPE>(VolatileLoadWithoutBarrier(pValue)); // LookupMap's hold pointers, so we can use a data dependency instead of an explicit barrier here.
+    // LookupMap's hold pointers to data which is immutable, so normally we could use a data
+    // dependency instead of an explicit barrier here. However, the access pattern between
+    // m_TypeDefToMethodTableMap and m_MethodDefToDescMap/m_FieldDefToDescMap is such that a
+    // data dependency is not sufficient to ensure that the MethodTable is visible when we
+    // access the MethodDesc/FieldDesc. Since those loads are independent, we use
+    // VolatileLoad here to ensure proper ordering.
+    TYPE value = dac_cast<TYPE>(VolatileLoad(pValue));
 #else
     TYPE value = dac_cast<TYPE>(*pValue);
 #endif
@@ -51,7 +57,13 @@ SIZE_T LookupMap<SIZE_T>::GetValueAt(PTR_TADDR pValue, TADDR* pFlags, TADDR supp
 {
     WRAPPER_NO_CONTRACT;
 
-    TADDR value = VolatileLoadWithoutBarrier(pValue); // LookupMap's hold pointers, so we can use a data dependency instead of an explicit barrier here.
+    // LookupMap's hold pointers to data which is immutable, so normally we could use a data
+    // dependency instead of an explicit barrier here. However, the access pattern between
+    // m_TypeDefToMethodTableMap and m_MethodDefToDescMap/m_FieldDefToDescMap is such that a
+    // data dependency is not sufficient to ensure that the MethodTable is visible when we
+    // access the MethodDesc/FieldDesc. Since those loads are independent, we use
+    // VolatileLoad here to ensure proper ordering.
+    TADDR value = VolatileLoad(pValue);
 
     if (pFlags)
         *pFlags = value & supportedFlags;