From c57822327a17408c44853ae1ce22d581b5047ab6 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 4 Jun 2019 15:38:47 -0700 Subject: Synchronize managed and unmanaged Variant fields (#24951) * Synchronize managed and unmanaged Variant fields Fixes #24948 --- src/System.Private.CoreLib/src/System/Variant.cs | 197 +++++++++-------------- src/classlibnative/bcltype/variant.cpp | 71 -------- src/classlibnative/bcltype/variant.h | 10 -- src/vm/binder.cpp | 1 + src/vm/ecalllist.h | 4 - src/vm/mscorlib.h | 5 + src/vm/olevariant.h | 32 ++-- 7 files changed, 106 insertions(+), 214 deletions(-) diff --git a/src/System.Private.CoreLib/src/System/Variant.cs b/src/System.Private.CoreLib/src/System/Variant.cs index b8e6d57f1c..0e8cc7d4f2 100644 --- a/src/System.Private.CoreLib/src/System/Variant.cs +++ b/src/System.Private.CoreLib/src/System/Variant.cs @@ -22,15 +22,13 @@ using System.Diagnostics; 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 @@ namespace System }; 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 @@ namespace System 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 @@ namespace System // 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 @@ namespace System { get { - return (m_flags & TypeCodeBitMask); + return (_flags & TypeCodeBitMask); } } @@ -372,33 +333,33 @@ namespace System 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 @@ namespace System case CV_STRING: case CV_OBJECT: default: - return m_objref; + return _objref; } } @@ -554,7 +515,7 @@ namespace System if (pValue == null) { v = new Variant(null); - v.m_flags = CV_STRING; + v._flags = CV_STRING; } else { diff --git a/src/classlibnative/bcltype/variant.cpp b/src/classlibnative/bcltype/variant.cpp index 892c5888a6..dfd6929a39 100644 --- a/src/classlibnative/bcltype/variant.cpp +++ b/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================================ ** diff --git a/src/classlibnative/bcltype/variant.h b/src/classlibnative/bcltype/variant.h index 0089693e23..6ec0fe4043 100644 --- a/src/classlibnative/bcltype/variant.h +++ b/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: diff --git a/src/vm/binder.cpp b/src/vm/binder.cpp index 5aaf976ae2..5da754af4c 100644 --- a/src/vm/binder.cpp +++ b/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" diff --git a/src/vm/ecalllist.h b/src/vm/ecalllist.h index 9f0abc3ce6..b63d216050 100644 --- a/src/vm/ecalllist.h +++ b/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 diff --git a/src/vm/mscorlib.h b/src/vm/mscorlib.h index 442a249e90..c986137b99 100644 --- a/src/vm/mscorlib.h +++ b/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) diff --git a/src/vm/olevariant.h b/src/vm/olevariant.h index d34124907a..fa5eb17ef1 100644 --- a/src/vm/olevariant.h +++ b/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 -- cgit v1.2.3