Synchronize managed and unmanaged Variant fields (dotnet/coreclr#24951)

* Synchronize managed and unmanaged Variant fields

Fixes dotnet/coreclr#24948

Commit migrated from https://github.com/dotnet/coreclr/commit/c57822327a17408c44853ae1ce22d581b5047ab6

src/coreclr/src/System.Private.CoreLib/src/System/Variant.cs

@@ -22,15 +22,13 @@
 
 namespace System
 {
-    [StructLayout(LayoutKind.Sequential)]
     internal struct Variant
     {
         //Do Not change the order of these fields.
         //They are mapped to the native VariantData * data structure.
-        private object? m_objref;
-        private int m_data1;
-        private int m_data2;
-        private int m_flags;
+        private object? _objref;
+        private long _data;
+        private int _flags;
 
         // The following bits have been taken up as follows
         // bits 0-15    - Type code
@@ -112,180 +110,143 @@ internal struct Variant
         };
 
         internal static readonly Variant Empty = new Variant();
-        internal static readonly Variant Missing = new Variant(Variant.CV_MISSING, Type.Missing, 0, 0);
-        internal static readonly Variant DBNull = new Variant(Variant.CV_NULL, System.DBNull.Value, 0, 0);
+        internal static readonly Variant Missing = new Variant(Variant.CV_MISSING, Type.Missing, 0);
+        internal static readonly Variant DBNull = new Variant(Variant.CV_NULL, System.DBNull.Value, 0);
 
         //
         // Native Methods
         //
         [MethodImplAttribute(MethodImplOptions.InternalCall)]
-        internal extern double GetR8FromVar();
-        [MethodImplAttribute(MethodImplOptions.InternalCall)]
-        internal extern float GetR4FromVar();
-        [MethodImplAttribute(MethodImplOptions.InternalCall)]
-        internal extern void SetFieldsR4(float val);
-        [MethodImplAttribute(MethodImplOptions.InternalCall)]
-        internal extern void SetFieldsR8(double val);
-        [MethodImplAttribute(MethodImplOptions.InternalCall)]
         internal extern void SetFieldsObject(object val);
 
-        // Use this function instead of an ECALL - saves about 150 clock cycles
-        // by avoiding the ecall transition and because the JIT inlines this.
-        // Ends up only taking about 1/8 the time of the ECALL version.
-        internal long GetI8FromVar()
-        {
-            return ((long)m_data2 << 32 | ((long)m_data1 & 0xFFFFFFFFL));
-        }
-
         //
         // Constructors
         //
 
-        internal Variant(int flags, object or, int data1, int data2)
+        internal Variant(int flags, object or, long data)
         {
-            m_flags = flags;
-            m_objref = or;
-            m_data1 = data1;
-            m_data2 = data2;
+            _flags = flags;
+            _objref = or;
+            _data = data;
         }
 
         public Variant(bool val)
         {
-            m_objref = null;
-            m_flags = CV_BOOLEAN;
-            m_data1 = (val) ? bool.True : bool.False;
-            m_data2 = 0;
+            _objref = null;
+            _flags = CV_BOOLEAN;
+            _data = (val) ? bool.True : bool.False;
         }
 
         public Variant(sbyte val)
         {
-            m_objref = null;
-            m_flags = CV_I1;
-            m_data1 = (int)val;
-            m_data2 = (int)(((long)val) >> 32);
+            _objref = null;
+            _flags = CV_I1;
+            _data = val;
         }
 
 
         public Variant(byte val)
         {
-            m_objref = null;
-            m_flags = CV_U1;
-            m_data1 = (int)val;
-            m_data2 = 0;
+            _objref = null;
+            _flags = CV_U1;
+            _data = val;
         }
 
         public Variant(short val)
         {
-            m_objref = null;
-            m_flags = CV_I2;
-            m_data1 = (int)val;
-            m_data2 = (int)(((long)val) >> 32);
+            _objref = null;
+            _flags = CV_I2;
+            _data = val;
         }
 
         public Variant(ushort val)
         {
-            m_objref = null;
-            m_flags = CV_U2;
-            m_data1 = (int)val;
-            m_data2 = 0;
+            _objref = null;
+            _flags = CV_U2;
+            _data = val;
         }
 
         public Variant(char val)
         {
-            m_objref = null;
-            m_flags = CV_CHAR;
-            m_data1 = (int)val;
-            m_data2 = 0;
+            _objref = null;
+            _flags = CV_CHAR;
+            _data = val;
         }
 
         public Variant(int val)
         {
-            m_objref = null;
-            m_flags = CV_I4;
-            m_data1 = val;
-            m_data2 = val >> 31;
+            _objref = null;
+            _flags = CV_I4;
+            _data = val;
         }
 
         public Variant(uint val)
         {
-            m_objref = null;
-            m_flags = CV_U4;
-            m_data1 = (int)val;
-            m_data2 = 0;
+            _objref = null;
+            _flags = CV_U4;
+            _data = val;
         }
 
         public Variant(long val)
         {
-            m_objref = null;
-            m_flags = CV_I8;
-            m_data1 = (int)val;
-            m_data2 = (int)(val >> 32);
+            _objref = null;
+            _flags = CV_I8;
+            _data = val;
         }
 
         public Variant(ulong val)
         {
-            m_objref = null;
-            m_flags = CV_U8;
-            m_data1 = (int)val;
-            m_data2 = (int)(val >> 32);
+            _objref = null;
+            _flags = CV_U8;
+            _data = (long)val;
         }
 
         public Variant(float val)
         {
-            m_objref = null;
-            m_flags = CV_R4;
-            m_data1 = 0;
-            m_data2 = 0;
-            SetFieldsR4(val);
+            _objref = null;
+            _flags = CV_R4;
+            _data = (uint)BitConverter.SingleToInt32Bits(val);
         }
 
         public Variant(double val)
         {
-            m_objref = null;
-            m_flags = CV_R8;
-            m_data1 = 0;
-            m_data2 = 0;
-            SetFieldsR8(val);
+            _objref = null;
+            _flags = CV_R8;
+            _data = BitConverter.DoubleToInt64Bits(val);
         }
 
         public Variant(DateTime val)
         {
-            m_objref = null;
-            m_flags = CV_DATETIME;
-            ulong ticks = (ulong)val.Ticks;
-            m_data1 = (int)ticks;
-            m_data2 = (int)(ticks >> 32);
+            _objref = null;
+            _flags = CV_DATETIME;
+            _data = val.Ticks;
         }
 
         public Variant(decimal val)
         {
-            m_objref = (object)val;
-            m_flags = CV_DECIMAL;
-            m_data1 = 0;
-            m_data2 = 0;
+            _objref = (object)val;
+            _flags = CV_DECIMAL;
+            _data = 0;
         }
 
         public Variant(object? obj)
         {
-            m_data1 = 0;
-            m_data2 = 0;
+            _data = 0;
 
             VarEnum vt = VarEnum.VT_EMPTY;
 
             if (obj is DateTime)
             {
-                m_objref = null;
-                m_flags = CV_DATETIME;
-                ulong ticks = (ulong)((DateTime)obj).Ticks;
-                m_data1 = (int)ticks;
-                m_data2 = (int)(ticks >> 32);
+                _objref = null;
+                _flags = CV_DATETIME;
+                _data = ((DateTime)obj).Ticks;
                 return;
             }
 
             if (obj is string)
             {
-                m_flags = CV_STRING;
-                m_objref = obj;
+                _flags = CV_STRING;
+                _objref = obj;
                 return;
             }
 
@@ -307,14 +268,14 @@ public Variant(object? obj)
 
             if (obj is Array)
             {
-                m_flags = CV_OBJECT | ArrayBitMask;
-                m_objref = obj;
+                _flags = CV_OBJECT | ArrayBitMask;
+                _objref = obj;
                 return;
             }
 
             // Compiler appeasement
-            m_flags = CV_EMPTY;
-            m_objref = null;
+            _flags = CV_EMPTY;
+            _objref = null;
 
             // Check to see if the object passed in is a wrapper object.
             if (obj is UnknownWrapper)
@@ -352,7 +313,7 @@ public Variant(object? obj)
 
             // If the object passed in is one of the wrappers then set the VARIANT type.
             if (vt != VarEnum.VT_EMPTY)
-                m_flags |= ((int)vt << VTBitShift);
+                _flags |= ((int)vt << VTBitShift);
         }
 
         //This is a family-only accessor for the CVType.
@@ -361,7 +322,7 @@ internal int CVType
         {
             get
             {
-                return (m_flags & TypeCodeBitMask);
+                return (_flags & TypeCodeBitMask);
             }
         }
 
@@ -372,33 +333,33 @@ internal int CVType
                 case CV_EMPTY:
                     return null;
                 case CV_BOOLEAN:
-                    return (object)(m_data1 != 0);
+                    return (object)((int)_data != 0);
                 case CV_I1:
-                    return (object)((sbyte)m_data1);
+                    return (object)((sbyte)_data);
                 case CV_U1:
-                    return (object)((byte)m_data1);
+                    return (object)((byte)_data);
                 case CV_CHAR:
-                    return (object)((char)m_data1);
+                    return (object)((char)_data);
                 case CV_I2:
-                    return (object)((short)m_data1);
+                    return (object)((short)_data);
                 case CV_U2:
-                    return (object)((ushort)m_data1);
+                    return (object)((ushort)_data);
                 case CV_I4:
-                    return (object)(m_data1);
+                    return (object)((int)_data);
                 case CV_U4:
-                    return (object)((uint)m_data1);
+                    return (object)((uint)_data);
                 case CV_I8:
-                    return (object)(GetI8FromVar());
+                    return (object)((long)_data);
                 case CV_U8:
-                    return (object)((ulong)GetI8FromVar());
+                    return (object)((ulong)_data);
                 case CV_R4:
-                    return (object)(GetR4FromVar());
+                    return (object)(BitConverter.Int32BitsToSingle((int)_data));
                 case CV_R8:
-                    return (object)(GetR8FromVar());
+                    return (object)(BitConverter.Int64BitsToDouble(_data));
                 case CV_DATETIME:
-                    return new DateTime(GetI8FromVar());
+                    return new DateTime(_data);
                 case CV_TIMESPAN:
-                    return new TimeSpan(GetI8FromVar());
+                    return new TimeSpan(_data);
                 case CV_ENUM:
                     return BoxEnum();
                 case CV_MISSING:
@@ -409,7 +370,7 @@ internal int CVType
                 case CV_STRING:
                 case CV_OBJECT:
                 default:
-                    return m_objref;
+                    return _objref;
             }
         }
 
@@ -554,7 +515,7 @@ internal static void MarshalHelperCastVariant(object pValue, int vt, ref Variant
                         if (pValue == null)
                         {
                             v = new Variant(null);
-                            v.m_flags = CV_STRING;
+                            v._flags = CV_STRING;
                         }
                         else
                         {

src/coreclr/src/classlibnative/bcltype/variant.cpp

@@ -35,77 +35,6 @@
 #define EnumU8          0x800000
 #define EnumMask        0xF00000
 
-//
-// Current Conversions
-// 
-
-FCIMPL1(float, COMVariant::GetR4FromVar, VariantData* var)
-{
-    CONTRACTL
-    {
-        FCALL_CHECK;
-        PRECONDITION(CheckPointer(var));
-    }
-    CONTRACTL_END;
-    
-    INT32 val = var->GetDataAsInt32();
-    return (float&)val;
-}
-FCIMPLEND
-    
-FCIMPL1(double, COMVariant::GetR8FromVar, VariantData* var)
-{
-    CONTRACTL
-    {
-        FCALL_CHECK;
-        PRECONDITION(CheckPointer(var));
-    }
-    CONTRACTL_END;
-    
-    INT64 val = var->GetDataAsInt64();
-    return (double&)val;
-}
-FCIMPLEND
-
-
-/*=================================SetFieldsR4==================================
-**
-==============================================================================*/
-FCIMPL2_IV(void, COMVariant::SetFieldsR4, VariantData* var, float val)
-{
-    CONTRACTL
-    {
-        FCALL_CHECK;
-        PRECONDITION(CheckPointer(var));
-    }
-    CONTRACTL_END;
-    
-    INT64 tempData;
-
-    tempData = *((INT32 *)(&val));
-    var->SetData(&tempData);
-    var->SetType(CV_R4);
-}
-FCIMPLEND
-
-
-/*=================================SetFieldsR8==================================
-**
-==============================================================================*/
-FCIMPL2_IV(void, COMVariant::SetFieldsR8, VariantData* var, double val)
-{
-    CONTRACTL
-    {
-        FCALL_CHECK;
-        PRECONDITION(CheckPointer(var));
-    }
-    CONTRACTL_END;
-    
-    var->SetData((void *)(&val));
-    var->SetType(CV_R8);
-}
-FCIMPLEND
-
 
 /*===============================SetFieldsObject================================
 **

src/coreclr/src/classlibnative/bcltype/variant.h

@@ -31,17 +31,7 @@ public:
     // Helper Routines
     //
 
-    //
-    // Initialization Methods
-
-    static FCDECL2_IV(void, SetFieldsR4, VariantData* vThisRef, float val);
-    static FCDECL2_IV(void, SetFieldsR8, VariantData* vThisRef, double val);
     static FCDECL2(void, SetFieldsObject, VariantData* vThisRef, Object* vVal);
-    static FCDECL1(float, GetR4FromVar, VariantData* var);
-    static FCDECL1(double, GetR8FromVar, VariantData* var);
-
-    static FCDECL0(void, InitVariant);
-
     static FCDECL1(Object*, BoxEnum, VariantData* var);
 
 private:

src/coreclr/src/vm/binder.cpp

@@ -20,6 +20,7 @@
 #include "nativeoverlapped.h"
 #include "clrvarargs.h"
 #include "sigbuilder.h"
+#include "olevariant.h"
 
 #ifdef FEATURE_PREJIT
 #include "compile.h"

src/coreclr/src/vm/ecalllist.h

@@ -709,10 +709,6 @@ FCFuncEnd()
 #ifdef FEATURE_COMINTEROP
 FCFuncStart(gVariantFuncs)
     FCFuncElement("SetFieldsObject", COMVariant::SetFieldsObject)
-    FCFuncElement("SetFieldsR4", COMVariant::SetFieldsR4)
-    FCFuncElement("SetFieldsR8", COMVariant::SetFieldsR8)
-    FCFuncElement("GetR4FromVar", COMVariant::GetR4FromVar)
-    FCFuncElement("GetR8FromVar", COMVariant::GetR8FromVar)
     FCFuncElement("BoxEnum", COMVariant::BoxEnum)
 FCFuncEnd()
 #endif // FEATURE_COMINTEROP

src/coreclr/src/vm/mscorlib.h

@@ -432,6 +432,11 @@ DEFINE_CLASS(VARIANT,               System,                 Variant)
 DEFINE_METHOD(VARIANT,              CONVERT_OBJECT_TO_VARIANT,MarshalHelperConvertObjectToVariant,SM_Obj_RefVariant_RetVoid)
 DEFINE_METHOD(VARIANT,              CAST_VARIANT,           MarshalHelperCastVariant,   SM_Obj_Int_RefVariant_RetVoid)
 DEFINE_METHOD(VARIANT,              CONVERT_VARIANT_TO_OBJECT,MarshalHelperConvertVariantToObject,SM_RefVariant_RetObject)
+
+DEFINE_CLASS_U(System,              Variant,                VariantData)
+DEFINE_FIELD_U(_objref,             VariantData,            m_objref)
+DEFINE_FIELD_U(_data,               VariantData,            m_data)
+DEFINE_FIELD_U(_flags,              VariantData,            m_flags)
 #endif // FEATURE_COMINTEROP
 
 DEFINE_CLASS(IASYNCRESULT,          System,                 IAsyncResult)

src/coreclr/src/vm/olevariant.h

@@ -96,7 +96,7 @@ extern CVTypes CorElementTypeToCVTypes(CorElementType type);
       2)  Variant must contain an OBJECTREF field for Objects, etc.  Since we
           have no way of expressing a union between an OBJECTREF and an int, we
           always box Decimals in a Variant.
-      3)  The m_type field is not a CVType and will contain extra bits.  People
+      3)  The m_flags field is not a CVType and will contain extra bits.  People
           should use VariantData::GetType() to get the CVType.
       4)  You should use SetObjRef and GetObjRef to manipulate the OBJECTREF field.
           These will handle write barriers correctly, as well as CV_EMPTY.
@@ -118,6 +118,8 @@ extern CVTypes CorElementTypeToCVTypes(CorElementType type);
 
 struct VariantData
 {
+    friend class MscorlibBinder;
+
 public:        
     static void NewVariant(VariantData * const& dest, const CVTypes type, INT64 data
                                             DEBUG_ARG(BOOL bDestIsInterior = FALSE));
@@ -126,20 +128,20 @@ public:
     {
         LIMITED_METHOD_CONTRACT;
 
-        return (CVTypes)(m_type & VARIANT_TYPE_MASK);
+        return (CVTypes)(m_flags & VARIANT_TYPE_MASK);
     }
 
     FORCEINLINE void SetType(INT32 in)
     {
         LIMITED_METHOD_CONTRACT;
-        m_type = in;
+        m_flags = in;
     }
 
     FORCEINLINE VARTYPE GetVT() const
     {
         LIMITED_METHOD_CONTRACT;
 
-        VARTYPE vt = (m_type & VT_MASK) >> VT_BITSHIFT;
+        VARTYPE vt = (m_flags & VT_MASK) >> VT_BITSHIFT;
         if (vt & 0x80)
         {
             vt &= ~0x80;
@@ -165,7 +167,7 @@ public:
             vt &= ~VT_ARRAY;
             vt |= 0x80;
         }
-        m_type = (m_type & ~((INT32)VT_MASK)) | (vt << VT_BITSHIFT);
+        m_flags = (m_flags & ~((INT32)VT_MASK)) | (vt << VT_BITSHIFT);
     }
 
 
@@ -173,7 +175,7 @@ public:
     {
         WRAPPER_NO_CONTRACT;
         
-        return (OBJECTREF)m_or;
+        return (OBJECTREF)m_objref;
     }
 
     OBJECTREF* GetObjRefPtr()
@@ -187,7 +189,7 @@ public:
         }
         CONTRACT_END;
 
-        RETURN (OBJECTREF*)&m_or;
+        RETURN (OBJECTREF*)&m_objref;
     }
 
     void SetObjRef(OBJECTREF objRef)
@@ -202,13 +204,13 @@ public:
         
         if (objRef!=NULL)
         {
-            SetObjectReference((OBJECTREF*)&m_or, objRef);
+            SetObjectReference((OBJECTREF*)&m_objref, objRef);
         }
         else
         {
             // Casting trick to avoid going thru overloaded operator= (which
             // in this case would trigger a false write barrier violation assert.)
-            *(LPVOID*)(OBJECTREF*)&m_or=NULL;
+            *(LPVOID*)(OBJECTREF*)&m_objref=NULL;
         }
     }
 
@@ -324,9 +326,17 @@ public:
     }
 
 private:
-    Object*     m_or;
+    // Typeloader reorders fields of non-blitable types. This reordering differs between 32-bit and 64-bit platforms.
+#ifdef _TARGET_64BIT_
+    Object*     m_objref;
+    INT64       m_data;
+    INT32       m_flags;
+    INT32       m_padding;
+#else
     INT64       m_data;
-    INT32       m_type;
+    Object*     m_objref;
+    INT32       m_flags;
+#endif
 };
 
 #include <poppack.h>