From dd52e0b5c11fce4de311f4aa1ab88564da10f44a Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Wed, 22 Sep 2021 20:57:31 -0700 Subject: [PATCH 1/2] Rewrite Enum.HasFlags and Enum.Equals in C# --- .../src/System/Enum.CoreCLR.cs | 6 - src/coreclr/vm/commodule.cpp | 35 ----- src/coreclr/vm/commodule.h | 6 - src/coreclr/vm/ecalllist.h | 9 -- src/coreclr/vm/reflectioninvocation.cpp | 97 ------------- src/coreclr/vm/reflectioninvocation.h | 2 - .../System.Private.CoreLib/src/System/Enum.cs | 137 ++++++++++++++++-- .../src/System/Enum.Mono.cs | 3 - src/mono/mono/metadata/icall-def-netcore.h | 1 - src/mono/mono/metadata/icall.c | 12 -- 10 files changed, 123 insertions(+), 185 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Enum.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Enum.CoreCLR.cs index bdf3a7156d5d66..03a1e5079d8b65 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Enum.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Enum.CoreCLR.cs @@ -13,9 +13,6 @@ public abstract partial class Enum [DllImport(RuntimeHelpers.QCall, CharSet = CharSet.Unicode)] private static extern void GetEnumValuesAndNames(QCallTypeHandle enumType, ObjectHandleOnStack values, ObjectHandleOnStack names, Interop.BOOL getNames); - [MethodImpl(MethodImplOptions.InternalCall)] - public extern override bool Equals([NotNullWhen(true)] object? obj); - [MethodImpl(MethodImplOptions.InternalCall)] private static extern object InternalBoxEnum(RuntimeType enumType, long value); @@ -25,9 +22,6 @@ public abstract partial class Enum [MethodImpl(MethodImplOptions.InternalCall)] internal static extern RuntimeType InternalGetUnderlyingType(RuntimeType enumType); - [MethodImpl(MethodImplOptions.InternalCall)] - private extern bool InternalHasFlag(Enum flags); - private static EnumInfo GetEnumInfo(RuntimeType enumType, bool getNames = true) { EnumInfo? entry = enumType.GenericCache as EnumInfo; diff --git a/src/coreclr/vm/commodule.cpp b/src/coreclr/vm/commodule.cpp index 31bdeb649ef0bc..99b4982329f045 100644 --- a/src/coreclr/vm/commodule.cpp +++ b/src/coreclr/vm/commodule.cpp @@ -902,38 +902,3 @@ FCIMPL1(FC_BOOL_RET, COMModule::IsResource, ReflectModuleBaseObject* pModuleUNSA FC_RETURN_BOOL(pModuleUNSAFE->GetModule()->IsResource()); } FCIMPLEND - - -//--------------------------------------------------------------------- -// Helper code for PunkSafeHandle class. This does the Release in the -// safehandle's critical finalizer. -//--------------------------------------------------------------------- -static VOID __stdcall DReleaseTarget(IUnknown *punk) -{ - CONTRACTL - { - NOTHROW; - GC_TRIGGERS; - MODE_PREEMPTIVE; - } - CONTRACTL_END; - - if (punk) - { - punk->Release(); - } -} - - -//--------------------------------------------------------------------- -// Helper code for PunkSafeHandle class. This returns the function that performs -// the Release() for the safehandle's critical finalizer. -//--------------------------------------------------------------------- -FCIMPL0(void*, COMPunkSafeHandle::nGetDReleaseTarget) -{ - FCALL_CONTRACT; - - return (void*)DReleaseTarget; -} -FCIMPLEND - diff --git a/src/coreclr/vm/commodule.h b/src/coreclr/vm/commodule.h index 4935028f96069c..c995ec80dfd824 100644 --- a/src/coreclr/vm/commodule.h +++ b/src/coreclr/vm/commodule.h @@ -110,10 +110,4 @@ class COMModule }; -class COMPunkSafeHandle -{ - public: - static FCDECL0(void*, nGetDReleaseTarget); -}; - #endif diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index 6363fc766c209a..00b089878c44ba 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -79,16 +79,8 @@ FCFuncStart(gEnumFuncs) FCFuncElement("InternalGetCorElementType", ReflectionEnum::InternalGetCorElementType) QCFuncElement("GetEnumValuesAndNames", ReflectionEnum::GetEnumValuesAndNames) FCFuncElement("InternalBoxEnum", ReflectionEnum::InternalBoxEnum) - FCFuncElement("Equals", ReflectionEnum::InternalEquals) - FCFuncElement("InternalHasFlag", ReflectionEnum::InternalHasFlag) FCFuncEnd() - -FCFuncStart(gSymWrapperCodePunkSafeHandleFuncs) - FCFuncElement("nGetDReleaseTarget", COMPunkSafeHandle::nGetDReleaseTarget) -FCFuncEnd() - - FCFuncStart(gObjectFuncs) FCIntrinsic("GetType", ObjectNative::GetClass, CORINFO_INTRINSIC_Object_GetType) FCFuncEnd() @@ -1199,7 +1191,6 @@ FCClassElement("ObjectiveCMarshal", "System.Runtime.InteropServices.ObjectiveC", FCClassElement("OverlappedData", "System.Threading", gOverlappedFuncs) -FCClassElement("PunkSafeHandle", "System.Reflection.Emit", gSymWrapperCodePunkSafeHandleFuncs) FCClassElement("RegisteredWaitHandle", "System.Threading", gRegisteredWaitHandleFuncs) FCClassElement("RuntimeAssembly", "System.Reflection", gRuntimeAssemblyFuncs) diff --git a/src/coreclr/vm/reflectioninvocation.cpp b/src/coreclr/vm/reflectioninvocation.cpp index 8a92ce9d7d9336..08ad56c44c96a6 100644 --- a/src/coreclr/vm/reflectioninvocation.cpp +++ b/src/coreclr/vm/reflectioninvocation.cpp @@ -2492,100 +2492,3 @@ FCIMPL2_IV(Object*, ReflectionEnum::InternalBoxEnum, ReflectClassBaseObject* tar return OBJECTREFToObject(ret); } FCIMPLEND - -//************************************************************************************************* -//************************************************************************************************* -//************************************************************************************************* -// ReflectionEnum -//************************************************************************************************* -//************************************************************************************************* -//************************************************************************************************* - -FCIMPL2(FC_BOOL_RET, ReflectionEnum::InternalEquals, Object *pRefThis, Object* pRefTarget) -{ - FCALL_CONTRACT; - - VALIDATEOBJECT(pRefThis); - BOOL ret = false; - if (pRefTarget == NULL) { - FC_RETURN_BOOL(ret); - } - - if( pRefThis == pRefTarget) - FC_RETURN_BOOL(true); - - //Make sure we are comparing same type. - MethodTable* pMTThis = pRefThis->GetMethodTable(); - _ASSERTE(!pMTThis->IsArray()); // bunch of assumptions about arrays wrong. - if ( pMTThis != pRefTarget->GetMethodTable()) { - FC_RETURN_BOOL(ret); - } - - void * pThis = pRefThis->UnBox(); - void * pTarget = pRefTarget->UnBox(); - switch (pMTThis->GetNumInstanceFieldBytes()) { - case 1: - ret = (*(UINT8*)pThis == *(UINT8*)pTarget); - break; - case 2: - ret = (*(UINT16*)pThis == *(UINT16*)pTarget); - break; - case 4: - ret = (*(UINT32*)pThis == *(UINT32*)pTarget); - break; - case 8: - ret = (*(UINT64*)pThis == *(UINT64*)pTarget); - break; - default: - // should not reach here. - UNREACHABLE_MSG("Incorrect Enum Type size!"); - break; - } - - FC_RETURN_BOOL(ret); -} -FCIMPLEND - -// perform (this & flags) == flags -FCIMPL2(FC_BOOL_RET, ReflectionEnum::InternalHasFlag, Object *pRefThis, Object* pRefFlags) -{ - FCALL_CONTRACT; - - VALIDATEOBJECT(pRefThis); - - BOOL cmp = false; - - _ASSERTE(pRefFlags != NULL); // Enum.cs would have thrown ArgumentNullException before calling into InternalHasFlag - - VALIDATEOBJECT(pRefFlags); - - void * pThis = pRefThis->UnBox(); - void * pFlags = pRefFlags->UnBox(); - - MethodTable* pMTThis = pRefThis->GetMethodTable(); - - _ASSERTE(!pMTThis->IsArray()); // bunch of assumptions about arrays wrong. - _ASSERTE(pMTThis->GetNumInstanceFieldBytes() == pRefFlags->GetMethodTable()->GetNumInstanceFieldBytes()); // Enum.cs verifies that the types are Equivalent - - switch (pMTThis->GetNumInstanceFieldBytes()) { - case 1: - cmp = ((*(UINT8*)pThis & *(UINT8*)pFlags) == *(UINT8*)pFlags); - break; - case 2: - cmp = ((*(UINT16*)pThis & *(UINT16*)pFlags) == *(UINT16*)pFlags); - break; - case 4: - cmp = ((*(UINT32*)pThis & *(UINT32*)pFlags) == *(UINT32*)pFlags); - break; - case 8: - cmp = ((*(UINT64*)pThis & *(UINT64*)pFlags) == *(UINT64*)pFlags); - break; - default: - // should not reach here. - UNREACHABLE_MSG("Incorrect Enum Type size!"); - break; - } - - FC_RETURN_BOOL(cmp); -} -FCIMPLEND diff --git a/src/coreclr/vm/reflectioninvocation.h b/src/coreclr/vm/reflectioninvocation.h index 1bdce9ddcb3613..7fd56a2bc22571 100644 --- a/src/coreclr/vm/reflectioninvocation.h +++ b/src/coreclr/vm/reflectioninvocation.h @@ -91,8 +91,6 @@ class ReflectionEnum { void QCALLTYPE GetEnumValuesAndNames(QCall::TypeHandle pEnumType, QCall::ObjectHandleOnStack pReturnValues, QCall::ObjectHandleOnStack pReturnNames, BOOL fGetNames); static FCDECL2_IV(Object*, InternalBoxEnum, ReflectClassBaseObject* pEnumType, INT64 value); - static FCDECL2(FC_BOOL_RET, InternalEquals, Object *pRefThis, Object* pRefTarget); - static FCDECL2(FC_BOOL_RET, InternalHasFlag, Object *pRefThis, Object* pRefFlags); }; #endif // _REFLECTIONINVOCATION_H_ diff --git a/src/libraries/System.Private.CoreLib/src/System/Enum.cs b/src/libraries/System.Private.CoreLib/src/System/Enum.cs index 931353332a0628..44a791a93112ca 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Enum.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Enum.cs @@ -10,11 +10,6 @@ using System.Runtime.InteropServices; using Internal.Runtime.CompilerServices; -#if CORERT -using RuntimeType = System.Type; -using EnumInfo = Internal.Runtime.Augments.EnumInfo; -#endif - // The code below includes partial support for float/double and // pointer sized enums. // @@ -325,7 +320,15 @@ public static Type GetUnderlyingType(Type enumType) } public static TEnum[] GetValues() where TEnum : struct, Enum - => (TEnum[])GetValues(typeof(TEnum)); + { + RuntimeType enumType = (RuntimeType)typeof(TEnum); + ulong[] ulValues = Enum.InternalGetValues(enumType); + + TEnum[] ret = new TEnum[ulValues.Length]; + for (int i = 0; i < ulValues.Length; i++) + ret[i] = (TEnum)ToObject(enumType, ulValues[i]); + return ret; + } public static Array GetValues(Type enumType) { @@ -340,10 +343,54 @@ public bool HasFlag(Enum flag) { if (flag is null) throw new ArgumentNullException(nameof(flag)); - if (!GetType().IsEquivalentTo(flag.GetType())) + if (GetType() != flag.GetType() && !GetType().IsEquivalentTo(flag.GetType())) throw new ArgumentException(SR.Format(SR.Argument_EnumTypeDoesNotMatch, flag.GetType(), GetType())); - return InternalHasFlag(flag); + ref byte pThisValue = ref this.GetRawData(); + ref byte pFlagsValue = ref flag.GetRawData(); + + switch (InternalGetCorElementType()) + { + case CorElementType.ELEMENT_TYPE_I1: + case CorElementType.ELEMENT_TYPE_U1: + case CorElementType.ELEMENT_TYPE_BOOLEAN: + { + byte flagsValue = pFlagsValue; + return (pThisValue & flagsValue) == flagsValue; + } + case CorElementType.ELEMENT_TYPE_I2: + case CorElementType.ELEMENT_TYPE_U2: + case CorElementType.ELEMENT_TYPE_CHAR: + { + ushort flagsValue = Unsafe.As(ref pFlagsValue); + return (Unsafe.As(ref pThisValue) & flagsValue) == flagsValue; + } + case CorElementType.ELEMENT_TYPE_I4: + case CorElementType.ELEMENT_TYPE_U4: +#if TARGET_32BIT + case CorElementType.ELEMENT_TYPE_I: + case CorElementType.ELEMENT_TYPE_U: +#endif + case CorElementType.ELEMENT_TYPE_R4: + { + uint flagsValue = Unsafe.As(ref pFlagsValue); + return (Unsafe.As(ref pThisValue) & flagsValue) == flagsValue; + } + case CorElementType.ELEMENT_TYPE_I8: + case CorElementType.ELEMENT_TYPE_U8: +#if TARGET_64BIT + case CorElementType.ELEMENT_TYPE_I: + case CorElementType.ELEMENT_TYPE_U: +#endif + case CorElementType.ELEMENT_TYPE_R8: + { + ulong flagsValue = Unsafe.As(ref pFlagsValue); + return (Unsafe.As(ref pThisValue) & flagsValue) == flagsValue; + } + default: + Debug.Fail("Unknown enum underlying type"); + return false; + } } internal static ulong[] InternalGetValues(RuntimeType enumType) @@ -1103,21 +1150,30 @@ private ulong ToUInt64() case CorElementType.ELEMENT_TYPE_CHAR: return Unsafe.As(ref data); case CorElementType.ELEMENT_TYPE_I4: +#if TARGET_32BIT + case CorElementType.ELEMENT_TYPE_I: +#endif return (ulong)Unsafe.As(ref data); case CorElementType.ELEMENT_TYPE_U4: +#if TARGET_32BIT + case CorElementType.ELEMENT_TYPE_U: +#endif case CorElementType.ELEMENT_TYPE_R4: return Unsafe.As(ref data); case CorElementType.ELEMENT_TYPE_I8: +#if TARGET_64BIT + case CorElementType.ELEMENT_TYPE_I: +#endif return (ulong)Unsafe.As(ref data); case CorElementType.ELEMENT_TYPE_U8: +#if TARGET_64BIT + case CorElementType.ELEMENT_TYPE_U: +#endif case CorElementType.ELEMENT_TYPE_R8: return Unsafe.As(ref data); - case CorElementType.ELEMENT_TYPE_I: - return (ulong)Unsafe.As(ref data); - case CorElementType.ELEMENT_TYPE_U: - return (ulong)Unsafe.As(ref data); default: - throw new InvalidOperationException(SR.InvalidOperation_UnknownEnumType); + Debug.Fail("Unknown enum underlying type"); + return 0; } } @@ -1125,6 +1181,52 @@ private ulong ToUInt64() #region Object Overrides + public override bool Equals([NotNullWhen(true)] object? obj) + { + if (obj is null) + return false; + + if (this == obj) + return true; + + if (this.GetType() != obj.GetType()) + return false; + + ref byte pThisValue = ref this.GetRawData(); + ref byte pOtherValue = ref obj.GetRawData(); + + switch (InternalGetCorElementType()) + { + case CorElementType.ELEMENT_TYPE_I1: + case CorElementType.ELEMENT_TYPE_U1: + case CorElementType.ELEMENT_TYPE_BOOLEAN: + return pThisValue == pOtherValue; + case CorElementType.ELEMENT_TYPE_I2: + case CorElementType.ELEMENT_TYPE_U2: + case CorElementType.ELEMENT_TYPE_CHAR: + return Unsafe.As(ref pThisValue) == Unsafe.As(ref pOtherValue); + case CorElementType.ELEMENT_TYPE_I4: + case CorElementType.ELEMENT_TYPE_U4: +#if TARGET_32BIT + case CorElementType.ELEMENT_TYPE_I: + case CorElementType.ELEMENT_TYPE_U: +#endif + case CorElementType.ELEMENT_TYPE_R4: + return Unsafe.As(ref pThisValue) == Unsafe.As(ref pOtherValue); + case CorElementType.ELEMENT_TYPE_I8: + case CorElementType.ELEMENT_TYPE_U8: +#if TARGET_64BIT + case CorElementType.ELEMENT_TYPE_I: + case CorElementType.ELEMENT_TYPE_U: +#endif + case CorElementType.ELEMENT_TYPE_R8: + return Unsafe.As(ref pThisValue) == Unsafe.As(ref pOtherValue); + default: + Debug.Fail("Unknown enum underlying type"); + return false; + } + } + public override int GetHashCode() { // CONTRACT with the runtime: GetHashCode of enum types is implemented as GetHashCode of the underlying type. @@ -1214,7 +1316,8 @@ public int CompareTo(object? target) case CorElementType.ELEMENT_TYPE_R8: return Unsafe.As(ref pThisValue).CompareTo(Unsafe.As(ref pTargetValue)); default: - throw new InvalidOperationException(SR.InvalidOperation_UnknownEnumType); + Debug.Fail("Unknown enum underlying type"); + return 0; } } #endregion @@ -1408,6 +1511,12 @@ private static RuntimeType ValidateRuntimeType(Type enumType) throw new ArgumentException(SR.Arg_MustBeEnum, nameof(enumType)); if (enumType is not RuntimeType rtType) throw new ArgumentException(SR.Arg_MustBeType, nameof(enumType)); +#if CORERT + // Check for the unfortunate "typeof(Outer<>).InnerEnum" corner case. + // https://github.com/dotnet/runtime/issues/7976 + if (enumType.ContainsGenericParameters) + throw new InvalidOperationException(SR.Format(SR.Arg_OpenType, enumType.ToString())); +#endif return rtType; } } diff --git a/src/mono/System.Private.CoreLib/src/System/Enum.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Enum.Mono.cs index 3e342b73b6c984..6115ca3d99d8d3 100644 --- a/src/mono/System.Private.CoreLib/src/System/Enum.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Enum.Mono.cs @@ -20,9 +20,6 @@ public partial class Enum [MethodImpl(MethodImplOptions.InternalCall)] internal static extern RuntimeType InternalGetUnderlyingType(RuntimeType enumType); - [MethodImpl(MethodImplOptions.InternalCall)] - private extern bool InternalHasFlag(Enum flags); - private static EnumInfo GetEnumInfo(RuntimeType enumType, bool getNames = true) { EnumInfo? entry = enumType.Cache.EnumInfo; diff --git a/src/mono/mono/metadata/icall-def-netcore.h b/src/mono/mono/metadata/icall-def-netcore.h index 1b826c77371968..a4340404b82a07 100644 --- a/src/mono/mono/metadata/icall-def-netcore.h +++ b/src/mono/mono/metadata/icall-def-netcore.h @@ -76,7 +76,6 @@ HANDLES(ENUM_1, "GetEnumValuesAndNames", ves_icall_System_Enum_GetEnumValuesAndN HANDLES(ENUM_2, "InternalBoxEnum", ves_icall_System_Enum_ToObject, MonoObject, 2, (MonoReflectionType, guint64)) HANDLES(ENUM_3, "InternalGetCorElementType", ves_icall_System_Enum_InternalGetCorElementType, int, 1, (MonoObject)) HANDLES(ENUM_4, "InternalGetUnderlyingType", ves_icall_System_Enum_get_underlying_type, MonoReflectionType, 1, (MonoReflectionType)) -HANDLES(ENUM_5, "InternalHasFlag", ves_icall_System_Enum_InternalHasFlag, MonoBoolean, 2, (MonoObject, MonoObject)) ICALL_TYPE(ENV, "System.Environment", ENV_1) NOHANDLES(ICALL(ENV_1, "Exit", ves_icall_System_Environment_Exit)) diff --git a/src/mono/mono/metadata/icall.c b/src/mono/mono/metadata/icall.c index 2a756f9a51e9fb..c48f2b1199a0df 100644 --- a/src/mono/mono/metadata/icall.c +++ b/src/mono/mono/metadata/icall.c @@ -3613,18 +3613,6 @@ ves_icall_System_Enum_ToObject (MonoReflectionTypeHandle enumType, guint64 value return MONO_HANDLE_NEW (MonoObject, NULL); } -MonoBoolean -ves_icall_System_Enum_InternalHasFlag (MonoObjectHandle a, MonoObjectHandle b, MonoError *error) -{ - int size = mono_class_value_size (mono_handle_class (a), NULL); - guint64 a_val = 0, b_val = 0; - - memcpy (&a_val, mono_handle_unbox_unsafe (a), size); - memcpy (&b_val, mono_handle_unbox_unsafe (b), size); - - return (a_val & b_val) == b_val; -} - MonoReflectionTypeHandle ves_icall_System_Enum_get_underlying_type (MonoReflectionTypeHandle type, MonoError *error) { From a6a7ebd27a09b62e45190858402ca1937829b858 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Thu, 23 Sep 2021 20:58:07 -0700 Subject: [PATCH 2/2] Undo --- .../System.Private.CoreLib/src/System/Enum.cs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Enum.cs b/src/libraries/System.Private.CoreLib/src/System/Enum.cs index 44a791a93112ca..8e45829d2af52c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Enum.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Enum.cs @@ -320,15 +320,7 @@ public static Type GetUnderlyingType(Type enumType) } public static TEnum[] GetValues() where TEnum : struct, Enum - { - RuntimeType enumType = (RuntimeType)typeof(TEnum); - ulong[] ulValues = Enum.InternalGetValues(enumType); - - TEnum[] ret = new TEnum[ulValues.Length]; - for (int i = 0; i < ulValues.Length; i++) - ret[i] = (TEnum)ToObject(enumType, ulValues[i]); - return ret; - } + => (TEnum[])GetValues(typeof(TEnum)); public static Array GetValues(Type enumType) {