Skip to content

Commit

Permalink
Simplify Enum_GetValuesAndNames QCall, fix handling of floats and dou…
Browse files Browse the repository at this point in the history 
…bles

This change avoids allocation of the temporary ulong[] array during EnumInfo initialization

Fixes dotnet#29266
  • Loading branch information
@jkotas @stephentoub
jkotas authored and stephentoub committed Dec 2, 2022
1 parent 0afa5b3 commit 6a36c7c
Show file tree
Hide file tree
Showing 5 changed files with 66 additions and 139 deletions.
15 changes: 8 additions & 7 deletions src/coreclr/System.Private.CoreLib/src/System/Enum.CoreCLR.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ namespace System
{
public abstract partial class Enum
{
// This returns 0 for all values for float/double/nint/nuint.
[LibraryImport(RuntimeHelpers.QCall, EntryPoint = "Enum_GetValuesAndNames")]
private static partial void GetEnumValuesAndNames(QCallTypeHandle enumType, ObjectHandleOnStack values, ObjectHandleOnStack names, Interop.BOOL getNames);

Expand Down Expand Up @@ -84,17 +83,19 @@ private static EnumInfo<TUnderlyingValue> GetEnumInfo<TUnderlyingValue>(RuntimeT
[MethodImpl(MethodImplOptions.NoInlining)]
static EnumInfo<TUnderlyingValue> InitializeEnumInfo(RuntimeType enumType, bool getNames)
{
ulong[]? uint64Values = null;
TUnderlyingValue[]? values = null;
string[]? names = null;
RuntimeTypeHandle enumTypeHandle = enumType.TypeHandle;

GetEnumValuesAndNames(
new QCallTypeHandle(ref enumTypeHandle),
ObjectHandleOnStack.Create(ref uint64Values),
new QCallTypeHandle(ref enumType),
ObjectHandleOnStack.Create(ref values),
ObjectHandleOnStack.Create(ref names),
getNames ? Interop.BOOL.TRUE : Interop.BOOL.FALSE);
bool hasFlagsAttribute = enumType.IsDefined(typeof(FlagsAttribute), inherit: false);

TUnderlyingValue[] values = ToUnderlyingValues<TUnderlyingValue>(uint64Values!);
Debug.Assert(values!.GetType() == typeof(TUnderlyingValue[]));
Debug.Assert(!getNames || names!.GetType() == typeof(string[]));

bool hasFlagsAttribute = enumType.IsDefined(typeof(FlagsAttribute), inherit: false);

var entry = new EnumInfo<TUnderlyingValue>(hasFlagsAttribute, values, names!);
enumType.GenericCache = entry;
Expand Down
113 changes: 21 additions & 92 deletions src/coreclr/vm/reflectioninvocation.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1919,36 +1919,14 @@ struct TempEnumValue
UINT64 value;
};

//*******************************************************************************
class TempEnumValueSorter : public CQuickSort<TempEnumValue>
{
public:
TempEnumValueSorter(TempEnumValue *pArray, SSIZE_T iCount)
: CQuickSort<TempEnumValue>(pArray, iCount) { LIMITED_METHOD_CONTRACT; }

int Compare(TempEnumValue *pFirst, TempEnumValue *pSecond)
{
LIMITED_METHOD_CONTRACT;

if (pFirst->value == pSecond->value)
return 0;
if (pFirst->value > pSecond->value)
return 1;
else
return -1;
}
};

extern "C" void QCALLTYPE Enum_GetValuesAndNames(QCall::TypeHandle pEnumType, QCall::ObjectHandleOnStack pReturnValues, QCall::ObjectHandleOnStack pReturnNames, BOOL fGetNames)
{
QCALL_CONTRACT;

BEGIN_QCALL;

TypeHandle th = pEnumType.AsTypeHandle();

if (!th.IsEnum())
COMPlusThrow(kArgumentException, W("Arg_MustBeEnum"));
_ASSERTE(th.IsEnum());

MethodTable *pMT = th.AsMethodTable();

Expand All @@ -1959,81 +1937,30 @@ extern "C" void QCALLTYPE Enum_GetValuesAndNames(QCall::TypeHandle pEnumType, QC
HENUMInternalHolder fieldEnum(pImport);
fieldEnum.EnumInit(mdtFieldDef, pMT->GetCl());

//
// Note that we're fine treating signed types as unsigned, because all we really
// want to do is sort them based on a convenient strong ordering.
//

BOOL sorted = TRUE;

CorElementType type = pMT->GetInternalCorElementType();
CorElementType type = pMT->GetClass()->GetInternalCorElementType();

mdFieldDef field;
while (pImport->EnumNext(&fieldEnum, &field))
{
DWORD dwFlags;
IfFailThrow(pImport->GetFieldDefProps(field, &dwFlags));
if (IsFdStatic(dwFlags))
{
TempEnumValue temp;

if (fGetNames)
IfFailThrow(pImport->GetNameOfFieldDef(field, &temp.name));

UINT64 value = 0;
if (!IsFdStatic(dwFlags))
continue;

MDDefaultValue defaultValue;
IfFailThrow(pImport->GetDefaultValue(field, &defaultValue));
TempEnumValue temp;

// The following code assumes that the address of all union members is the same.
static_assert_no_msg(offsetof(MDDefaultValue, m_byteValue) == offsetof(MDDefaultValue, m_usValue));
static_assert_no_msg(offsetof(MDDefaultValue, m_ulValue) == offsetof(MDDefaultValue, m_ullValue));
PVOID pValue = &defaultValue.m_byteValue;

switch (type) {
case ELEMENT_TYPE_I1:
value = *((INT8 *)pValue);
break;

case ELEMENT_TYPE_U1:
case ELEMENT_TYPE_BOOLEAN:
value = *((UINT8 *)pValue);
break;

case ELEMENT_TYPE_I2:
value = *((INT16 *)pValue);
break;

case ELEMENT_TYPE_U2:
case ELEMENT_TYPE_CHAR:
value = *((UINT16 *)pValue);
break;

case ELEMENT_TYPE_I4:
IN_TARGET_32BIT(case ELEMENT_TYPE_I:)
value = *((INT32 *)pValue);
break;
if (fGetNames)
IfFailThrow(pImport->GetNameOfFieldDef(field, &temp.name));

case ELEMENT_TYPE_U4:
IN_TARGET_32BIT(case ELEMENT_TYPE_U:)
value = *((UINT32 *)pValue);
break;
MDDefaultValue defaultValue = { };
IfFailThrow(pImport->GetDefaultValue(field, &defaultValue));

case ELEMENT_TYPE_I8:
case ELEMENT_TYPE_U8:
IN_TARGET_64BIT(case ELEMENT_TYPE_I:)
IN_TARGET_64BIT(case ELEMENT_TYPE_U:)
value = *((INT64 *)pValue);
break;
// The following code assumes that the address of all union members is the same.
static_assert_no_msg(offsetof(MDDefaultValue, m_byteValue) == offsetof(MDDefaultValue, m_usValue));
static_assert_no_msg(offsetof(MDDefaultValue, m_ulValue) == offsetof(MDDefaultValue, m_ullValue));
temp.value = defaultValue.m_ullValue;

default:
break;
}

temp.value = value;

temps.Append(temp);
}
temps.Append(temp);
}

TempEnumValue * pTemps = &(temps[0]);
Expand All @@ -2043,7 +1970,7 @@ extern "C" void QCALLTYPE Enum_GetValuesAndNames(QCall::TypeHandle pEnumType, QC
GCX_COOP();

struct gc {
I8ARRAYREF values;
BASEARRAYREF values;
PTRARRAYREF names;
} gc;
gc.values = NULL;
Expand All @@ -2052,12 +1979,14 @@ extern "C" void QCALLTYPE Enum_GetValuesAndNames(QCall::TypeHandle pEnumType, QC
GCPROTECT_BEGIN(gc);

{
gc.values = (I8ARRAYREF) AllocatePrimitiveArray(ELEMENT_TYPE_U8, cFields);
// The managed side expects ELEMENT_TYPE_U1 as the underlying type for boolean
gc.values = (BASEARRAYREF) AllocatePrimitiveArray((type == ELEMENT_TYPE_BOOLEAN) ? ELEMENT_TYPE_U1 : type, cFields);

INT64 *pToValues = gc.values->GetDirectPointerToNonObjectElements();
BYTE* pToValues = gc.values->GetDataPtr();
size_t elementSize = gc.values->GetComponentSize();

for (DWORD i = 0; i < cFields; i++) {
pToValues[i] = pTemps[i].value;
for (DWORD i = 0; i < cFields; i++, pToValues += elementSize) {
memcpyNoGCRefs(pToValues, &pTemps[i].value, elementSize);
}

pReturnValues.Set(gc.values);
Expand Down
27 changes: 2 additions & 25 deletions src/libraries/System.Private.CoreLib/src/System/Enum.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -846,14 +846,14 @@ static bool HandleRareTypes(RuntimeType rt, ReadOnlySpan<char> value, bool ignor
if (underlyingType == typeof(float))
{
parsed = TryParseByValueOrName(rt, value, ignoreCase, throwOnFailure, out float localResult);
result = parsed ? InternalBoxEnum(rt, (long)localResult) : null;
result = parsed ? InternalBoxEnum(rt, BitConverter.SingleToInt32Bits(localResult)) : null;
return parsed;
}

if (underlyingType == typeof(double))
{
parsed = TryParseByValueOrName(rt, value, ignoreCase, throwOnFailure, out double localResult);
result = parsed ? InternalBoxEnum(rt, (long)localResult) : null;
result = parsed ? InternalBoxEnum(rt, BitConverter.DoubleToInt64Bits(localResult)) : null;
return parsed;
}

Expand Down Expand Up @@ -2193,29 +2193,6 @@ private static void WriteMultipleFoundFlagsNames(string[] names, ReadOnlySpan<in
names[foundItems[0]].CopyTo(destination);
}

/// <summary>Creates a new TUnderlyingValue[] from a ulong[] array of values.</summary>
private static TUnderlyingValue[] ToUnderlyingValues<TUnderlyingValue>(ulong[] uint64Values)
where TUnderlyingValue : struct, INumber<TUnderlyingValue>
{
TUnderlyingValue[] values;

if (typeof(TUnderlyingValue) == typeof(ulong))
{
values = (TUnderlyingValue[])(object)uint64Values;
}
else
{
values = new TUnderlyingValue[uint64Values.Length];

for (int i = 0; i < values.Length; i++)
{
values[i] = TUnderlyingValue.CreateTruncating(uint64Values[i]);
}
}

return values;
}

private static RuntimeType ValidateRuntimeType(Type enumType)
{
ArgumentNullException.ThrowIfNull(enumType);
Expand Down
27 changes: 12 additions & 15 deletions src/libraries/System.Runtime/tests/System/EnumTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,16 +88,13 @@ public static IEnumerable<object[]> Parse_TestData()
yield return new object[] { "Value1", false, Enum.ToObject(s_boolEnumType, true) };
yield return new object[] { "vaLue2", true, Enum.ToObject(s_boolEnumType, false) };

if (!PlatformDetection.IsMonoRuntime) // [ActiveIssue("https://github.com/dotnet/runtime/issues/29266")]
{
// Single - parses successfully, but doesn't properly represent the underlying value
yield return new object[] { "Value1", false, Enum.GetValues(s_floatEnumType).GetValue(0) };
yield return new object[] { "vaLue2", true, Enum.GetValues(s_floatEnumType).GetValue(0) };
// Single
yield return new object[] { "Value1", false, Enum.GetValues(s_floatEnumType).GetValue(1) };
yield return new object[] { "vaLue2", true, Enum.GetValues(s_floatEnumType).GetValue(2) };

// Double - parses successfully, but doesn't properly represent the underlying value
yield return new object[] { "Value1", false, Enum.GetValues(s_doubleEnumType).GetValue(0) };
yield return new object[] { "vaLue2", true, Enum.GetValues(s_doubleEnumType).GetValue(0) };
}
// Double
yield return new object[] { "Value1", false, Enum.GetValues(s_doubleEnumType).GetValue(1) };
yield return new object[] { "vaLue2", true, Enum.GetValues(s_doubleEnumType).GetValue(2) };
}

// SimpleEnum
Expand Down Expand Up @@ -1409,16 +1406,16 @@ public void GetNames_InvokeBoolEnum_ReturnsExpected()
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))]
public void GetNames_InvokeSingleEnum_ReturnsExpected()
{
var expected = new string[] { "Value1", "Value2", "Value0x3f06", "Value0x3000", "Value0x0f06", "Value0x1000", "Value0x0000", "Value0x0010", "Value0x3f16" };
Assert.Equal(new HashSet<string>(expected), new HashSet<string>(Enum.GetNames(s_floatEnumType))); // using sets because coreclr returns 0 for all float enum values, affecting sort order
var expected = new string[] { "Value0x0000", "Value1", "Value2", "Value0x0010", "Value0x0f06", "Value0x1000", "Value0x3000", "Value0x3f06", "Value0x3f16" };
Assert.Equal(expected, Enum.GetNames(s_floatEnumType));
Assert.NotSame(Enum.GetNames(s_floatEnumType), Enum.GetNames(s_floatEnumType));
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))]
public void GetNames_InvokeDoubleEnum_ReturnsExpected()
{
var expected = new string[] { "Value1", "Value2", "Value0x3f06", "Value0x3000", "Value0x0f06", "Value0x1000", "Value0x0000", "Value0x0010", "Value0x3f16" };
Assert.Equal(new HashSet<string>(expected), new HashSet<string>(Enum.GetNames(s_doubleEnumType))); // using sets because coreclr returns 0 for all double enum values, affecting sort order
var expected = new string[] { "Value0x0000", "Value1", "Value2", "Value0x0010", "Value0x0f06", "Value0x1000", "Value0x3000", "Value0x3f06", "Value0x3f16" };
Assert.Equal(expected, Enum.GetNames(s_doubleEnumType));
Assert.NotSame(Enum.GetNames(s_doubleEnumType), Enum.GetNames(s_doubleEnumType));
}

Expand Down Expand Up @@ -1554,15 +1551,15 @@ public void GetValues_InvokeBoolEnum_ReturnsExpected()
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))]
public void GetValues_InvokeSingleEnum_ReturnsExpected()
{
var expected = new object[] { Enum.Parse(s_floatEnumType, "Value1"), Enum.Parse(s_floatEnumType, "Value2"), Enum.Parse(s_floatEnumType, "Value0x3f06"), Enum.Parse(s_floatEnumType, "Value0x3000"), Enum.Parse(s_floatEnumType, "Value0x0f06"), Enum.Parse(s_floatEnumType, "Value0x1000"), Enum.Parse(s_floatEnumType, "Value0x0000"), Enum.Parse(s_floatEnumType, "Value0x0010"), Enum.Parse(s_floatEnumType, "Value0x3f16") };
var expected = new object[] { Enum.Parse(s_floatEnumType, "Value0x0000"), Enum.Parse(s_floatEnumType, "Value1"), Enum.Parse(s_floatEnumType, "Value2"), Enum.Parse(s_floatEnumType, "Value0x0010"), Enum.Parse(s_floatEnumType, "Value0x0f06"), Enum.Parse(s_floatEnumType, "Value0x1000"), Enum.Parse(s_floatEnumType, "Value0x3000"), Enum.Parse(s_floatEnumType, "Value0x3f06"), Enum.Parse(s_floatEnumType, "Value0x3f16") };
Assert.Equal(expected, Enum.GetValues(s_floatEnumType));
Assert.NotSame(Enum.GetValues(s_floatEnumType), Enum.GetValues(s_floatEnumType));
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))]
public void GetValues_InvokeDoubleEnum_ReturnsExpected()
{
var expected = new object[] { Enum.Parse(s_doubleEnumType, "Value1"), Enum.Parse(s_doubleEnumType, "Value2"), Enum.Parse(s_doubleEnumType, "Value0x3f06"), Enum.Parse(s_doubleEnumType, "Value0x3000"), Enum.Parse(s_doubleEnumType, "Value0x0f06"), Enum.Parse(s_doubleEnumType, "Value0x1000"), Enum.Parse(s_doubleEnumType, "Value0x0000"), Enum.Parse(s_doubleEnumType, "Value0x0010"), Enum.Parse(s_doubleEnumType, "Value0x3f16") };
var expected = new object[] { Enum.Parse(s_doubleEnumType, "Value0x0000"), Enum.Parse(s_doubleEnumType, "Value1"), Enum.Parse(s_doubleEnumType, "Value2"), Enum.Parse(s_doubleEnumType, "Value0x0010"), Enum.Parse(s_doubleEnumType, "Value0x0f06"), Enum.Parse(s_doubleEnumType, "Value0x1000"), Enum.Parse(s_doubleEnumType, "Value0x3000"), Enum.Parse(s_doubleEnumType, "Value0x3f06"), Enum.Parse(s_doubleEnumType, "Value0x3f16") };
Assert.Equal(expected, Enum.GetValues(s_doubleEnumType));
Assert.NotSame(Enum.GetValues(s_doubleEnumType), Enum.GetValues(s_doubleEnumType));
}
Expand Down
23 changes: 23 additions & 0 deletions src/mono/System.Private.CoreLib/src/System/Enum.Mono.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,29 @@ internal static RuntimeType InternalGetUnderlyingType(RuntimeType enumType)
return res!;
}

/// <summary>Creates a new TUnderlyingValue[] from a ulong[] array of values.</summary>
private static TUnderlyingValue[] ToUnderlyingValues<TUnderlyingValue>(ulong[] uint64Values)
where TUnderlyingValue : struct, INumber<TUnderlyingValue>
{
TUnderlyingValue[] values;

if (typeof(TUnderlyingValue) == typeof(ulong))
{
values = (TUnderlyingValue[])(object)uint64Values;
}
else
{
values = new TUnderlyingValue[uint64Values.Length];

for (int i = 0; i < values.Length; i++)
{
values[i] = TUnderlyingValue.CreateTruncating(uint64Values[i]);
}
}

return values;
}

private static EnumInfo<TUnderlyingValue> GetEnumInfo<TUnderlyingValue>(RuntimeType enumType, bool getNames = true)
where TUnderlyingValue : struct, INumber<TUnderlyingValue>
{
Expand Down

0 comments on commit 6a36c7c

Please sign in to comment.