From 081f6b31acb71589de02dc582ddbea946f3e8ab6 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Thu, 8 Feb 2024 18:09:03 -0600 Subject: [PATCH 01/12] Fast FieldInfo reflection --- .../src/System/Reflection/RtFieldInfo.cs | 135 +---- .../src/System/Reflection/RuntimeFieldInfo.cs | 2 +- .../src/System/RuntimeHandles.cs | 12 +- .../src/System/RuntimeType.CoreCLR.cs | 13 - src/coreclr/vm/corelib.h | 8 +- src/coreclr/vm/ecalllist.h | 3 + src/coreclr/vm/field.cpp | 12 +- src/coreclr/vm/field.h | 8 + src/coreclr/vm/invokeutil.cpp | 8 +- src/coreclr/vm/object.h | 1 + src/coreclr/vm/reflectioninvocation.cpp | 106 +++- src/coreclr/vm/runtimehandles.h | 5 +- .../src/Resources/Strings.resx | 3 + .../System.Private.CoreLib.Shared.projitems | 1 + .../src/System/Reflection/FieldAccessor.cs | 534 ++++++++++++++++++ .../System.Reflection.Tests/FieldInfoTests.cs | 230 +++++++- 16 files changed, 879 insertions(+), 202 deletions(-) create mode 100644 src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RtFieldInfo.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RtFieldInfo.cs index 6a8aaf6898b834..9bd2298eb9357a 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RtFieldInfo.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RtFieldInfo.cs @@ -18,46 +18,18 @@ internal sealed unsafe class RtFieldInfo : RuntimeFieldInfo, IRuntimeFieldInfo // lazy caching private string? m_name; private RuntimeType? m_fieldType; - private InvocationFlags m_invocationFlags; - internal InvocationFlags InvocationFlags - { - [MethodImpl(MethodImplOptions.AggressiveInlining)] - get => (m_invocationFlags & InvocationFlags.Initialized) != 0 ? - m_invocationFlags : InitializeInvocationFlags(); - } + private FieldAccessor? m_fieldAccessor; - [MethodImpl(MethodImplOptions.NoInlining)] - private InvocationFlags InitializeInvocationFlags() + internal FieldAccessor FieldAccessor { - Type? declaringType = DeclaringType; - - InvocationFlags invocationFlags = 0; - - // first take care of all the NO_INVOKE cases - if (declaringType != null && declaringType.ContainsGenericParameters) - { - invocationFlags |= InvocationFlags.NoInvoke; - } - - // If the invocationFlags are still 0, then - // this should be an usable field, determine the other flags - if (invocationFlags == 0) + [MethodImpl(MethodImplOptions.AggressiveInlining)] + get { - if ((m_fieldAttributes & FieldAttributes.InitOnly) != 0) - invocationFlags |= InvocationFlags.SpecialField; - - if ((m_fieldAttributes & FieldAttributes.HasFieldRVA) != 0) - invocationFlags |= InvocationFlags.SpecialField; - - // find out if the field type is one of the following: Primitive, Enum or Pointer - Type fieldType = FieldType; - if (fieldType.IsPointer || fieldType.IsEnum || fieldType.IsPrimitive) - invocationFlags |= InvocationFlags.FieldSpecialCast; + m_fieldAccessor ??= new FieldAccessor(this); + return m_fieldAccessor; } - - // must be last to avoid threading problems - return m_invocationFlags = invocationFlags | InvocationFlags.Initialized; } + #endregion #region Constructor @@ -75,28 +47,6 @@ internal RtFieldInfo( #endregion #region Internal Members - [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal void CheckConsistency(object? target) - { - // only test instance fields - if ((m_fieldAttributes & FieldAttributes.Static) != FieldAttributes.Static) - { - if (!m_declaringType.IsInstanceOfType(target)) - { - if (target == null) - { - throw new TargetException(SR.RFLCT_Targ_StatFldReqTarg); - } - else - { - throw new ArgumentException( - SR.Format(SR.Arg_FieldDeclTarget, - Name, m_declaringType, target.GetType())); - } - } - } - } - internal override bool CacheEquals(object? o) { return o is RtFieldInfo m && m.m_fieldHandle == m_fieldHandle; @@ -131,36 +81,7 @@ public override int GetHashCode() => #region FieldInfo Overrides [DebuggerStepThrough] [DebuggerHidden] - public override object? GetValue(object? obj) - { - InvocationFlags invocationFlags = InvocationFlags; - RuntimeType? declaringType = DeclaringType as RuntimeType; - - if ((invocationFlags & InvocationFlags.NoInvoke) != 0) - { - if (declaringType != null && DeclaringType!.ContainsGenericParameters) - throw new InvalidOperationException(SR.Arg_UnboundGenField); - - throw new FieldAccessException(); - } - - CheckConsistency(obj); - - RuntimeType fieldType = (RuntimeType)FieldType; - - bool domainInitialized = false; - if (declaringType == null) - { - return RuntimeFieldHandle.GetValue(this, obj, fieldType, null, ref domainInitialized); - } - else - { - domainInitialized = declaringType.DomainInitialized; - object? retVal = RuntimeFieldHandle.GetValue(this, obj, fieldType, declaringType, ref domainInitialized); - declaringType.DomainInitialized = domainInitialized; - return retVal; - } - } + public override object? GetValue(object? obj) => FieldAccessor.GetValue(obj); public override object GetRawConstantValue() { throw new InvalidOperationException(); } @@ -180,45 +101,7 @@ public override int GetHashCode() => [DebuggerStepThrough] [DebuggerHidden] public override void SetValue(object? obj, object? value, BindingFlags invokeAttr, Binder? binder, CultureInfo? culture) - { - InvocationFlags invocationFlags = InvocationFlags; - RuntimeType? declaringType = DeclaringType as RuntimeType; - - if ((invocationFlags & InvocationFlags.NoInvoke) != 0) - { - if (declaringType != null && declaringType.ContainsGenericParameters) - throw new InvalidOperationException(SR.Arg_UnboundGenField); - - throw new FieldAccessException(); - } - - CheckConsistency(obj); - - RuntimeType fieldType = (RuntimeType)FieldType; - if (value is null) - { - if (fieldType.IsActualValueType) - { - fieldType.CheckValue(ref value, binder, culture, invokeAttr); - } - } - else if (!ReferenceEquals(value.GetType(), fieldType)) - { - fieldType.CheckValue(ref value, binder, culture, invokeAttr); - } - - bool domainInitialized = false; - if (declaringType is null) - { - RuntimeFieldHandle.SetValue(this, obj, value, fieldType, m_fieldAttributes, null, ref domainInitialized); - } - else - { - domainInitialized = declaringType.DomainInitialized; - RuntimeFieldHandle.SetValue(this, obj, value, fieldType, m_fieldAttributes, declaringType, ref domainInitialized); - declaringType.DomainInitialized = domainInitialized; - } - } + => FieldAccessor.SetValue(obj, value, invokeAttr, binder, culture); [DebuggerStepThrough] [DebuggerHidden] diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs index dab1d07145dbd9..a314edaab2a15f 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs @@ -11,7 +11,7 @@ internal abstract class RuntimeFieldInfo : FieldInfo #region Private Data Members private readonly BindingFlags m_bindingFlags; protected readonly RuntimeTypeCache m_reflectedTypeCache; - protected readonly RuntimeType m_declaringType; + protected internal readonly RuntimeType m_declaringType; #endregion #region Constructor diff --git a/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs b/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs index 73b9bb167f7a7f..21228998515ac4 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs @@ -1087,6 +1087,7 @@ public RuntimeFieldInfoStub(RuntimeFieldHandleInternal fieldHandle, object keepa private object? m_d; private int m_b; private object? m_e; + private object? m_f; private RuntimeFieldHandleInternal m_fieldHandle; #pragma warning restore 414, 169, IDE0044 @@ -1189,6 +1190,15 @@ internal static RuntimeType GetApproxDeclaringType(IRuntimeFieldInfo field) return type; } + [MethodImpl(MethodImplOptions.InternalCall)] + internal static extern bool IsFastPathSupported(RtFieldInfo field); + + [MethodImpl(MethodImplOptions.InternalCall)] + internal static extern int GetInstanceFieldOffset(RtFieldInfo field); + + [MethodImpl(MethodImplOptions.InternalCall)] + internal static extern IntPtr GetStaticFieldAddress(RtFieldInfo field); + [MethodImpl(MethodImplOptions.InternalCall)] internal static extern int GetToken(RtFieldInfo field); @@ -1199,7 +1209,7 @@ internal static RuntimeType GetApproxDeclaringType(IRuntimeFieldInfo field) internal static extern object? GetValueDirect(RtFieldInfo field, RuntimeType fieldType, void* pTypedRef, RuntimeType? contextType); [MethodImpl(MethodImplOptions.InternalCall)] - internal static extern void SetValue(RtFieldInfo field, object? obj, object? value, RuntimeType fieldType, FieldAttributes fieldAttr, RuntimeType? declaringType, ref bool domainInitialized); + internal static extern void SetValue(RtFieldInfo field, object? obj, object? value, RuntimeType fieldType, RuntimeType? declaringType, ref bool domainInitialized); [MethodImpl(MethodImplOptions.InternalCall)] internal static extern void SetValueDirect(RtFieldInfo field, RuntimeType fieldType, void* pTypedRef, object? value, RuntimeType? contextType); diff --git a/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs index 1329f1f7140b43..c74d76388b91a9 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs @@ -1438,7 +1438,6 @@ internal T[] GetMemberList(MemberListType listType, string? name, CacheType cach private string? m_toString; private string? m_namespace; private readonly bool m_isGlobal; - private bool m_bIsDomainInitialized; private MemberInfoCache? m_methodInfoCache; private MemberInfoCache? m_constructorInfoCache; private MemberInfoCache? m_fieldInfoCache; @@ -1523,12 +1522,6 @@ internal Type[] FunctionPointerReturnAndParameterTypes } } - internal bool DomainInitialized - { - get => m_bIsDomainInitialized; - set => m_bIsDomainInitialized = value; - } - internal string? GetName(TypeNameKind kind) { switch (kind) @@ -1935,12 +1928,6 @@ internal object? GenericCache set => Cache.GenericCache = value; } - internal bool DomainInitialized - { - get => Cache.DomainInitialized; - set => Cache.DomainInitialized = value; - } - internal static FieldInfo GetFieldInfo(IRuntimeFieldInfo fieldHandle) { return GetFieldInfo(RuntimeFieldHandle.GetApproxDeclaringType(fieldHandle), fieldHandle); diff --git a/src/coreclr/vm/corelib.h b/src/coreclr/vm/corelib.h index 491265645b8e3a..0cb59f82fc05d0 100644 --- a/src/coreclr/vm/corelib.h +++ b/src/coreclr/vm/corelib.h @@ -337,13 +337,13 @@ DEFINE_FIELD(RT_TYPE_HANDLE, M_TYPE, m_type) DEFINE_CLASS(TYPE_NAME_PARSER, Reflection, TypeNameParser) DEFINE_METHOD(TYPE_NAME_PARSER, GET_TYPE_HELPER, GetTypeHelper, SM_Type_CharPtr_RuntimeAssembly_Bool_Bool_RetRuntimeType) -DEFINE_CLASS_U(Reflection, RtFieldInfo, NoClass) -DEFINE_FIELD_U(m_fieldHandle, ReflectFieldObject, m_pFD) +DEFINE_CLASS_U(Reflection, RtFieldInfo, NoClass) +DEFINE_FIELD_U(m_fieldHandle, ReflectFieldObject, m_pFD) DEFINE_CLASS(RT_FIELD_INFO, Reflection, RtFieldInfo) DEFINE_FIELD(RT_FIELD_INFO, HANDLE, m_fieldHandle) -DEFINE_CLASS_U(System, RuntimeFieldInfoStub, ReflectFieldObject) -DEFINE_FIELD_U(m_fieldHandle, ReflectFieldObject, m_pFD) +DEFINE_CLASS_U(System, RuntimeFieldInfoStub, ReflectFieldObject) +DEFINE_FIELD_U(m_fieldHandle, ReflectFieldObject, m_pFD) DEFINE_CLASS(STUBFIELDINFO, System, RuntimeFieldInfoStub) #if FOR_ILLINK DEFINE_METHOD(STUBFIELDINFO, CTOR, .ctor, IM_RetVoid) diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index 64142b1fb69d13..cbee36613dcccf 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -242,6 +242,9 @@ FCFuncStart(gCOMFieldHandleNewFuncs) FCFuncElement("GetStaticFieldForGenericType", RuntimeFieldHandle::GetStaticFieldForGenericType) FCFuncElement("AcquiresContextFromThis", RuntimeFieldHandle::AcquiresContextFromThis) FCFuncElement("GetLoaderAllocator", RuntimeFieldHandle::GetLoaderAllocator) + FCFuncElement("IsFastPathSupported", RuntimeFieldHandle::IsFastPathSupported) + FCFuncElement("GetInstanceFieldOffset", RuntimeFieldHandle::GetInstanceFieldOffset) + FCFuncElement("GetStaticFieldAddress", RuntimeFieldHandle::GetStaticFieldAddress) FCFuncEnd() FCFuncStart(gCOMModuleHandleFuncs) diff --git a/src/coreclr/vm/field.cpp b/src/coreclr/vm/field.cpp index b2973d8b4c66fd..c2eab291cc42b0 100644 --- a/src/coreclr/vm/field.cpp +++ b/src/coreclr/vm/field.cpp @@ -180,8 +180,8 @@ void* FieldDesc::GetStaticAddress(void *base) void* ret = GetStaticAddressHandle(base); // Get the handle - // For value classes, the handle points at an OBJECTREF - // which holds the boxed value class, so dereference and unbox. + // For value classes, the handle points at an OBJECTREF + // which holds the boxed value class, so dereference and unbox. if (GetFieldType() == ELEMENT_TYPE_VALUETYPE && !IsRVA()) { OBJECTREF obj = ObjectToOBJECTREF(*(Object**) ret); @@ -211,11 +211,10 @@ MethodTable * FieldDesc::GetExactDeclaringType(MethodTable * ownerOrSubType) #endif // #ifndef DACCESS_COMPILE - // static value classes are actually stored in their boxed form. - // this means that their address moves. +// Static value classes are actually stored in their boxed form. +// This means that their address moves. PTR_VOID FieldDesc::GetStaticAddressHandle(PTR_VOID base) { - CONTRACTL { INSTANCE_CHECK; @@ -255,7 +254,6 @@ PTR_VOID FieldDesc::GetStaticAddressHandle(PTR_VOID base) } #endif // FEATURE_METADATA_UPDATER - if (IsRVA()) { Module* pModule = GetModule(); @@ -270,12 +268,10 @@ PTR_VOID FieldDesc::GetStaticAddressHandle(PTR_VOID base) PTR_VOID ret = PTR_VOID(dac_cast(base) + GetOffset()); - return ret; } - // These routines encapsulate the operation of getting and setting // fields. void FieldDesc::GetInstanceField(OBJECTREF o, VOID * pOutVal) diff --git a/src/coreclr/vm/field.h b/src/coreclr/vm/field.h index e2324787febaf5..1bc8885c9faabf 100644 --- a/src/coreclr/vm/field.h +++ b/src/coreclr/vm/field.h @@ -285,6 +285,14 @@ class FieldDesc SetOffset(FIELD_OFFSET_NEW_ENC); } + BOOL IsCollectible() + { + LIMITED_METHOD_DAC_CONTRACT; + + LoaderAllocator *pLoaderAllocatorOfMethod = GetApproxEnclosingMethodTable()->GetLoaderAllocator(); + return pLoaderAllocatorOfMethod->IsCollectible(); + } + // Was this field added by EnC? // If this is true, then this object is an instance of EnCFieldDesc BOOL IsEnCNew() diff --git a/src/coreclr/vm/invokeutil.cpp b/src/coreclr/vm/invokeutil.cpp index eb8462ed16f245..0d7994a330ca75 100644 --- a/src/coreclr/vm/invokeutil.cpp +++ b/src/coreclr/vm/invokeutil.cpp @@ -741,7 +741,7 @@ void InvokeUtil::ValidateObjectTarget(FieldDesc *pField, TypeHandle enclosingTyp // SetValidField // Given an target object, a value object and a field this method will set the field -// on the target object. The field must be validate before calling this. +// on the target object. The field must be validated before calling this. void InvokeUtil::SetValidField(CorElementType fldType, TypeHandle fldTH, FieldDesc *pField, @@ -971,8 +971,6 @@ void InvokeUtil::SetValidField(CorElementType fldType, } } -// GetFieldValue -// This method will return an ARG_SLOT containing the value of the field. // GetFieldValue // This method will return an ARG_SLOT containing the value of the field. OBJECTREF InvokeUtil::GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJECTREF* target, TypeHandle declaringType, CLR_BOOL *pDomainInitialized) { @@ -999,7 +997,7 @@ OBJECTREF InvokeUtil::GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJ { pDeclMT = declaringType.GetMethodTable(); - // We don't allow getting the field just so we don't have more specical + // We don't allow getting the field just so we don't have more special // cases than we need to. Then we need at least the throw check to ensure // we don't allow data corruption. if (Nullable::IsNullableType(pDeclMT)) @@ -1084,7 +1082,7 @@ OBJECTREF InvokeUtil::GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJ case ELEMENT_TYPE_VALUETYPE: { - // Value classes require createing a boxed version of the field and then + // Value classes require creating a boxed version of the field and then // copying from the source... // Allocate an object to return... _ASSERTE(!fieldType.IsTypeDesc()); diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index 91bbc95304ce1e..915e45deca0636 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -1130,6 +1130,7 @@ class ReflectFieldObject : public BaseObjectWithCachedData INT32 m_empty2; OBJECTREF m_empty3; OBJECTREF m_empty4; + OBJECTREF m_empty5; FieldDesc * m_pFD; public: diff --git a/src/coreclr/vm/reflectioninvocation.cpp b/src/coreclr/vm/reflectioninvocation.cpp index e12ade403c6b5c..788c31b1ff4335 100644 --- a/src/coreclr/vm/reflectioninvocation.cpp +++ b/src/coreclr/vm/reflectioninvocation.cpp @@ -50,17 +50,6 @@ FCIMPL5(Object*, RuntimeFieldHandle::GetValue, ReflectFieldObject *pFieldUNSAFE, TypeHandle fieldType = gc.pFieldType->GetType(); TypeHandle declaringType = (gc.pDeclaringType != NULL) ? gc.pDeclaringType->GetType() : TypeHandle(); - Assembly *pAssem; - if (declaringType.IsNull()) - { - // global field - pAssem = gc.refField->GetField()->GetModule()->GetAssembly(); - } - else - { - pAssem = declaringType.GetAssembly(); - } - OBJECTREF rv = NULL; // not protected HELPER_METHOD_FRAME_BEGIN_RET_PROTECT(gc); @@ -169,7 +158,7 @@ FCIMPL2(Object*, ReflectionInvocation::AllocateValueType, ReflectClassBaseObject } FCIMPLEND -FCIMPL7(void, RuntimeFieldHandle::SetValue, ReflectFieldObject *pFieldUNSAFE, Object *targetUNSAFE, Object *valueUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE, DWORD attr, ReflectClassBaseObject *pDeclaringTypeUNSAFE, CLR_BOOL *pDomainInitialized) { +FCIMPL6(void, RuntimeFieldHandle::SetValue, ReflectFieldObject *pFieldUNSAFE, Object *targetUNSAFE, Object *valueUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE, ReflectClassBaseObject *pDeclaringTypeUNSAFE, CLR_BOOL *pDomainInitialized) { CONTRACTL { FCALL_CHECK; } @@ -195,17 +184,6 @@ FCIMPL7(void, RuntimeFieldHandle::SetValue, ReflectFieldObject *pFieldUNSAFE, Ob TypeHandle fieldType = gc.fieldType->GetType(); TypeHandle declaringType = gc.declaringType != NULL ? gc.declaringType->GetType() : TypeHandle(); - Assembly *pAssem; - if (declaringType.IsNull()) - { - // global field - pAssem = gc.refField->GetField()->GetModule()->GetAssembly(); - } - else - { - pAssem = declaringType.GetAssembly(); - } - FC_GC_POLL_NOT_NEEDED(); FieldDesc* pFieldDesc = gc.refField->GetField(); @@ -1243,6 +1221,88 @@ lExit: ; } FCIMPLEND +static FC_BOOL_RET IsFastPathSupportedHelper(FieldDesc* pFieldDesc) +{ + CONTRACTL { + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; + PRECONDITION(CheckPointer(pFieldDesc)); + } + CONTRACTL_END; + + return pFieldDesc->IsThreadStatic() || + pFieldDesc->IsEnCNew() || + pFieldDesc->IsCollectible() ? FALSE : TRUE; +} + +FCIMPL1(FC_BOOL_RET, RuntimeFieldHandle::IsFastPathSupported, ReflectFieldObject *pFieldUNSAFE) +{ + CONTRACTL { + FCALL_CHECK; + } + CONTRACTL_END; + + REFLECTFIELDREF refField = (REFLECTFIELDREF)ObjectToOBJECTREF(pFieldUNSAFE); + if (refField == NULL) + FCThrowRes(kArgumentNullException, W("Arg_InvalidHandle")); + + FieldDesc* pFieldDesc = refField->GetField(); + return IsFastPathSupportedHelper(pFieldDesc); +} +FCIMPLEND + +FCIMPL1(INT32, RuntimeFieldHandle::GetInstanceFieldOffset, ReflectFieldObject *pFieldUNSAFE) +{ + CONTRACTL { + FCALL_CHECK; + PRECONDITION(CheckPointer(pFieldUNSAFE)); + } + CONTRACTL_END; + + REFLECTFIELDREF refField = (REFLECTFIELDREF)ObjectToOBJECTREF(pFieldUNSAFE); + if (refField == NULL) + FCThrowRes(kArgumentNullException, W("Arg_InvalidHandle")); + + FieldDesc* pFieldDesc = refField->GetField(); + _ASSERTE(!pFieldDesc->IsStatic()); + + // IsFastPathSupported needs to checked before calling this method. + _ASSERTE(IsFastPathSupportedHelper(pFieldDesc)); + + return pFieldDesc->GetOffset(); +} +FCIMPLEND + +FCIMPL1(void*, RuntimeFieldHandle::GetStaticFieldAddress, ReflectFieldObject *pFieldUNSAFE) +{ + CONTRACTL { + FCALL_CHECK; + PRECONDITION(CheckPointer(pFieldUNSAFE)); + } + CONTRACTL_END; + + REFLECTFIELDREF refField = (REFLECTFIELDREF)ObjectToOBJECTREF(pFieldUNSAFE); + if (refField == NULL) + FCThrowRes(kArgumentNullException, W("Arg_InvalidHandle")); + + FieldDesc* pFieldDesc = refField->GetField(); + _ASSERTE(pFieldDesc->IsStatic()); + + // IsFastPathSupported needs to checked before calling this method. + _ASSERTE(IsFastPathSupportedHelper(pFieldDesc)); + + PTR_BYTE base = 0; + if (!pFieldDesc->IsRVA()) + { + // For RVA the base is ignored and offset is used. + base = pFieldDesc->GetBase(); + } + + return PTR_VOID(base + pFieldDesc->GetOffset()); +} +FCIMPLEND + extern "C" void QCALLTYPE ReflectionInvocation_CompileMethod(MethodDesc * pMD) { QCALL_CONTRACT; diff --git a/src/coreclr/vm/runtimehandles.h b/src/coreclr/vm/runtimehandles.h index 694a25a30623da..122224e27dc011 100644 --- a/src/coreclr/vm/runtimehandles.h +++ b/src/coreclr/vm/runtimehandles.h @@ -293,9 +293,12 @@ class RuntimeFieldHandle { public: static FCDECL5(Object*, GetValue, ReflectFieldObject *pFieldUNSAFE, Object *instanceUNSAFE, ReflectClassBaseObject *pFieldType, ReflectClassBaseObject *pDeclaringType, CLR_BOOL *pDomainInitialized); - static FCDECL7(void, SetValue, ReflectFieldObject *pFieldUNSAFE, Object *targetUNSAFE, Object *valueUNSAFE, ReflectClassBaseObject *pFieldType, DWORD attr, ReflectClassBaseObject *pDeclaringType, CLR_BOOL *pDomainInitialized); + static FCDECL6(void, SetValue, ReflectFieldObject *pFieldUNSAFE, Object *targetUNSAFE, Object *valueUNSAFE, ReflectClassBaseObject *pFieldType, ReflectClassBaseObject *pDeclaringType, CLR_BOOL *pDomainInitialized); static FCDECL4(Object*, GetValueDirect, ReflectFieldObject *pFieldUNSAFE, ReflectClassBaseObject *pFieldType, TypedByRef *pTarget, ReflectClassBaseObject *pDeclaringType); static FCDECL5(void, SetValueDirect, ReflectFieldObject *pFieldUNSAFE, ReflectClassBaseObject *pFieldType, TypedByRef *pTarget, Object *valueUNSAFE, ReflectClassBaseObject *pContextType); + static FCDECL1(FC_BOOL_RET, IsFastPathSupported, ReflectFieldObject *pField); + static FCDECL1(INT32, GetInstanceFieldOffset, ReflectFieldObject *pField); + static FCDECL1(void*, GetStaticFieldAddress, ReflectFieldObject *pField); static FCDECL1(StringObject*, GetName, ReflectFieldObject *pFieldUNSAFE); static FCDECL1(LPCUTF8, GetUtf8Name, FieldDesc *pField); diff --git a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx index b721414a669627..89a2de3695e8fd 100644 --- a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx +++ b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx @@ -4295,6 +4295,9 @@ This operation is not available because the reflection support was disabled at compile time. + + Cannot set initonly static field '{0}' after type '{1}' is initialized. + This AssemblyBuilder instance doesn't support saving. Use AssemblyBuilder.DefinePersistedAssembly to create an AssemblyBuilder instance that supports saving. diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index 261968ae7f8f3d..f2b203cbcfaaab 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -702,6 +702,7 @@ + diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs new file mode 100644 index 00000000000000..d95c55ae0c4da4 --- /dev/null +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs @@ -0,0 +1,534 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; +using System.Globalization; +using System.Runtime.CompilerServices; +using System.Threading; + +namespace System.Reflection +{ + internal sealed class FieldAccessor + { + private readonly RtFieldInfo _fieldInfo; + private readonly IntPtr _addressOrOffset; + private readonly unsafe MethodTable* _methodTable; + private FieldAccessorType _fieldAccessType; + + internal FieldAccessor(FieldInfo fieldInfo) + { + _fieldInfo = (RtFieldInfo)fieldInfo; + + InitializeClass(); + + Debug.Assert(_fieldInfo.m_declaringType != null); + if (_fieldInfo.m_declaringType.ContainsGenericParameters || + _fieldInfo.m_declaringType.IsNullableOfT) + { + _fieldAccessType = FieldAccessorType.NoInvoke; + } + else + { + _fieldAccessType = FieldAccessorType.SlowPathUntilClassInitialized; + + if (RuntimeFieldHandle.IsFastPathSupported(_fieldInfo)) + { + // Initialize the readonly fields. + _addressOrOffset = _fieldInfo.IsStatic ? + RuntimeFieldHandle.GetStaticFieldAddress(_fieldInfo) : + RuntimeFieldHandle.GetInstanceFieldOffset(_fieldInfo); + + unsafe + { + _methodTable = _fieldInfo.FieldType.IsFunctionPointer ? + (MethodTable*) typeof(IntPtr).TypeHandle.Value : + (MethodTable*) _fieldInfo.FieldType.TypeHandle.Value; + } + } + } + } + + private void Initialize() + { + RuntimeType fieldType = (RuntimeType)_fieldInfo.FieldType; + + if (!RuntimeFieldHandle.IsFastPathSupported(_fieldInfo)) + { + // Currently this is true for [ThreadStatic] cases, for fields added from EnC, and for fields on unloadable types. + _fieldAccessType = FieldAccessorType.SlowPath; + } + else if (_fieldInfo.IsStatic) + { + if (fieldType.IsValueType) + { + if (fieldType.IsEnum) + { + _fieldAccessType = GetPrimitiveAccessorTypeForStatic(fieldType.GetEnumUnderlyingType()); + } + else if (RuntimeTypeHandle.GetCorElementType(fieldType) == CorElementType.ELEMENT_TYPE_VALUETYPE) + { + // The runtime stores non-primitive value types as a boxed value. + _fieldAccessType = FieldAccessorType.StaticValueTypeBoxed; + } + else + { + _fieldAccessType = GetPrimitiveAccessorTypeForStatic(fieldType); + } + } + else if (fieldType.IsPointer) + { + _fieldAccessType = FieldAccessorType.StaticPointerType; + } + else if (fieldType.IsFunctionPointer) + { + _fieldAccessType = GetIntPtrAccessorTypeForStatic(); + } + else + { + _fieldAccessType = FieldAccessorType.StaticReferenceType; + } + } + else + { + if (fieldType.IsEnum) + { + _fieldAccessType = GetPrimitiveAccessorTypeForInstance(fieldType.GetEnumUnderlyingType()); + } + else if (fieldType.IsValueType) + { + _fieldAccessType = GetPrimitiveAccessorTypeForInstance(fieldType); + } + else if (fieldType.IsPointer) + { + _fieldAccessType = FieldAccessorType.InstancePointerType; + } + else if (fieldType.IsFunctionPointer) + { + _fieldAccessType = GetIntPtrAccessorTypeForInstance(); + } + else + { + _fieldAccessType = FieldAccessorType.InstanceReferenceType; + } + } + } + + public object? GetValue(object? obj) + { + bool domainInitialized; + + unsafe + { + switch (_fieldAccessType) + { + case FieldAccessorType.InstanceReferenceType: + VerifyTarget(obj); + Debug.Assert(obj != null); + return Volatile.Read(ref Unsafe.As(ref Unsafe.AddByteOffset(ref obj.GetRawData(), _addressOrOffset))); + + case FieldAccessorType.InstanceValueType: + case FieldAccessorType.InstanceValueTypeSize1: + case FieldAccessorType.InstanceValueTypeSize2: + case FieldAccessorType.InstanceValueTypeSize4: + case FieldAccessorType.InstanceValueTypeSize8: + VerifyTarget(obj); + Debug.Assert(obj != null); + return RuntimeHelpers.Box( + _methodTable, + ref Unsafe.AddByteOffset(ref obj.GetRawData(), _addressOrOffset)); + + case FieldAccessorType.InstancePointerType: + VerifyTarget(obj); + Debug.Assert(obj != null); + return Pointer.Box( + (void*)Unsafe.As(ref Unsafe.AddByteOffset(ref obj.GetRawData(), _addressOrOffset)), + _fieldInfo.FieldType); + + case FieldAccessorType.StaticReferenceType: + return Volatile.Read(ref Unsafe.As(ref *(IntPtr*)_addressOrOffset)); + + case FieldAccessorType.StaticValueType: + case FieldAccessorType.StaticValueTypeSize1: + case FieldAccessorType.StaticValueTypeSize2: + case FieldAccessorType.StaticValueTypeSize4: + case FieldAccessorType.StaticValueTypeSize8: + return RuntimeHelpers.Box(_methodTable, ref Unsafe.AsRef(_addressOrOffset.ToPointer())); + + case FieldAccessorType.StaticValueTypeBoxed: + // Re-box the value. + return RuntimeHelpers.Box( + _methodTable, + ref Unsafe.As(ref *(IntPtr*)_addressOrOffset).GetRawData()); + + case FieldAccessorType.StaticPointerType: + return Pointer.Box((void*)Unsafe.As( + ref Unsafe.AsRef(_addressOrOffset.ToPointer())), _fieldInfo.FieldType); + + case FieldAccessorType.SlowPathUntilClassInitialized: + if (!IsStatic()) + { + VerifyTarget(obj); + } + + domainInitialized = false; + object? ret = RuntimeFieldHandle.GetValue(_fieldInfo, obj, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref domainInitialized); + if (domainInitialized) + { + Initialize(); + } + + return ret; + + case FieldAccessorType.SlowPath: + if (!IsStatic()) + { + VerifyTarget(obj); + } + + domainInitialized = true; + return RuntimeFieldHandle.GetValue(_fieldInfo, obj, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref domainInitialized); + + case FieldAccessorType.NoInvoke: + if (_fieldInfo.DeclaringType is not null && _fieldInfo.DeclaringType.ContainsGenericParameters) + throw new InvalidOperationException(SR.Arg_UnboundGenField); + + if (_fieldInfo.DeclaringType is not null && ((RuntimeType)_fieldInfo.FieldType).IsNullableOfT) + throw new NotSupportedException(); + + throw new FieldAccessException(); + + default: + Debug.Assert(false, "Unknown enum value"); + return null; + } + } + } + + public void SetValue(object? obj, object? value, BindingFlags invokeAttr, Binder? binder, CultureInfo? culture) + { + bool domainInitialized; + + unsafe + { + switch (_fieldAccessType) + { + case FieldAccessorType.InstanceReferenceType: + VerifyInstanceField(obj, ref value, invokeAttr, binder, culture); + Debug.Assert(obj != null); + Volatile.Write(ref Unsafe.As(ref Unsafe.AddByteOffset(ref obj.GetRawData(), _addressOrOffset)), value); + return; + + case FieldAccessorType.InstanceValueTypeSize1: + VerifyInstanceField(obj, ref value, invokeAttr, binder, culture); + Debug.Assert(obj != null); + Volatile.Write( + ref Unsafe.AddByteOffset(ref obj.GetRawData(), _addressOrOffset), + value!.GetRawData()); + return; + + case FieldAccessorType.InstanceValueTypeSize2: + VerifyInstanceField(obj, ref value, invokeAttr, binder, culture); + Debug.Assert(obj != null); + Volatile.Write( + ref Unsafe.As(ref Unsafe.AddByteOffset(ref obj.GetRawData(), _addressOrOffset)), + Unsafe.As(ref value!.GetRawData())); + return; + + case FieldAccessorType.InstanceValueTypeSize4: + VerifyInstanceField(obj, ref value, invokeAttr, binder, culture); + Debug.Assert(obj != null); + Volatile.Write( + ref Unsafe.As(ref Unsafe.AddByteOffset(ref obj.GetRawData(), _addressOrOffset)), + Unsafe.As(ref value!.GetRawData())); + return; + + case FieldAccessorType.InstanceValueTypeSize8: + VerifyInstanceField(obj, ref value, invokeAttr, binder, culture); + Debug.Assert(obj != null); + Volatile.Write( + ref Unsafe.As(ref Unsafe.AddByteOffset(ref obj.GetRawData(), _addressOrOffset)), + Unsafe.As(ref value!.GetRawData())); + return; + + case FieldAccessorType.StaticReferenceType: + VerifyStaticField(ref value, invokeAttr, binder, culture); + Volatile.Write(ref Unsafe.As(ref *(IntPtr*)_addressOrOffset), value); + return; + + case FieldAccessorType.StaticValueTypeSize1: + VerifyStaticField(ref value, invokeAttr, binder, culture); + Volatile.Write( + ref Unsafe.AsRef(_addressOrOffset.ToPointer()), + value!.GetRawData()); + return; + + case FieldAccessorType.StaticValueTypeSize2: + VerifyStaticField(ref value, invokeAttr, binder, culture); + Volatile.Write( + ref Unsafe.AsRef(_addressOrOffset.ToPointer()), + Unsafe.As(ref value!.GetRawData())); + return; + + case FieldAccessorType.StaticValueTypeSize4: + VerifyStaticField(ref value, invokeAttr, binder, culture); + Volatile.Write( + ref Unsafe.AsRef(_addressOrOffset.ToPointer()), + Unsafe.As(ref value!.GetRawData())); + return; + + case FieldAccessorType.StaticValueTypeSize8: + VerifyStaticField(ref value, invokeAttr, binder, culture); + Volatile.Write( + ref Unsafe.AsRef(_addressOrOffset.ToPointer()), + Unsafe.As(ref value!.GetRawData())); + return; + + case FieldAccessorType.SlowPathUntilClassInitialized: + { + if (IsStatic()) + { + VerifyStaticField(ref value, invokeAttr, binder, culture); + } + else + { + VerifyInstanceField(obj, ref value, invokeAttr, binder, culture); + } + + domainInitialized = false; + RuntimeFieldHandle.SetValue(_fieldInfo, obj, value, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref domainInitialized); + if (domainInitialized) + { + Initialize(); + } + + return; + } + + case FieldAccessorType.NoInvoke: + if (_fieldInfo.DeclaringType is not null && _fieldInfo.DeclaringType.ContainsGenericParameters) + throw new InvalidOperationException(SR.Arg_UnboundGenField); + + throw new FieldAccessException(); + } + } + + // Slow path + if (IsStatic()) + { + VerifyStaticField(ref value, invokeAttr, binder, culture); + } + else + { + VerifyInstanceField(obj, ref value, invokeAttr, binder, culture); + } + + domainInitialized = true; + RuntimeFieldHandle.SetValue(_fieldInfo, obj, value, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref domainInitialized); + } + + private void InitializeClass() + { + if (_fieldInfo.DeclaringType is null) + { + RunModuleConstructor(_fieldInfo.Module); + } + else + { + RunClassConstructor(_fieldInfo); + } + + [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2059:RunClassConstructor", + Justification = "This represents the static constructor, so if this object was created, the static constructor exists.")] + static void RunClassConstructor(FieldInfo fieldInfo) + { + RuntimeHelpers.RunClassConstructor(fieldInfo.DeclaringType!.TypeHandle); + } + + static void RunModuleConstructor(Module module) + { + RuntimeHelpers.RunModuleConstructor(module.ModuleHandle); + } + } + + private bool IsStatic() => (_fieldInfo.Attributes & FieldAttributes.Static) == FieldAttributes.Static; + + private void VerifyStaticField(ref object? value, BindingFlags invokeAttr, Binder? binder, CultureInfo? culture) + { + VerifyInitOnly(); + CheckValue(ref value, invokeAttr, binder, culture); + } + + private void VerifyInstanceField(object? obj, ref object? value, BindingFlags invokeAttr, Binder? binder, CultureInfo? culture) + { + VerifyTarget(obj); + CheckValue(ref value, invokeAttr, binder, culture); + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal void VerifyTarget(object? target) + { + Debug.Assert(!IsStatic()); + + if (!_fieldInfo.m_declaringType.IsInstanceOfType(target)) + { + if (target == null) + { + ThrowHelperTargetException(); + } + else + { + ThrowHelperArgumentException(target, _fieldInfo); + } + } + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private void CheckValue(ref object? value, BindingFlags invokeAttr, Binder? binder, CultureInfo? culture) + { + if (value is null) + { + if (((RuntimeType)_fieldInfo.FieldType).IsActualValueType) + { + ((RuntimeType)_fieldInfo.FieldType).CheckValue(ref value, binder, culture, invokeAttr); + } + } + else if (!ReferenceEquals(value.GetType(), _fieldInfo.FieldType)) + { + ((RuntimeType)_fieldInfo.FieldType).CheckValue(ref value, binder, culture, invokeAttr); + } + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal void VerifyInitOnly() + { + Debug.Assert(IsStatic()); + + if ((_fieldInfo.Attributes & FieldAttributes.InitOnly) == FieldAttributes.InitOnly && + _fieldAccessType != FieldAccessorType.SlowPathUntilClassInitialized) + { + ThrowHelperFieldAccessException(_fieldInfo.Name, _fieldInfo.DeclaringType?.FullName); + } + } + + /// + /// Currently we only optimize for primitive types and not all value types. Primitive types support atomic write operations, are + /// not boxed by the runtime when stored as a static field, and don't need special nullable, GC or alignment checks. + /// + private static FieldAccessorType GetPrimitiveAccessorTypeForInstance(Type fieldType) + { + FieldAccessorType accessorType = FieldAccessorType.InstanceValueType; + + if (fieldType == typeof(byte) || + fieldType == typeof(sbyte) || + fieldType == typeof(bool)) + accessorType = FieldAccessorType.InstanceValueTypeSize1; + else if (fieldType == typeof(short) || + fieldType == typeof(ushort) || + fieldType == typeof(char)) + accessorType = FieldAccessorType.InstanceValueTypeSize2; + else if (fieldType == typeof(int) || + fieldType == typeof(uint) || + fieldType == typeof(float)) + accessorType = FieldAccessorType.InstanceValueTypeSize4; + else if (fieldType == typeof(long) || + fieldType == typeof(ulong) || + fieldType == typeof(double)) + accessorType = FieldAccessorType.InstanceValueTypeSize8; + else if (fieldType == typeof(IntPtr) || + fieldType == typeof(UIntPtr)) + accessorType = GetIntPtrAccessorTypeForInstance(); + + return accessorType; + } + + private static FieldAccessorType GetPrimitiveAccessorTypeForStatic(Type fieldType) + { + FieldAccessorType accessorType = FieldAccessorType.StaticValueType; + + if (fieldType == typeof(byte) || + fieldType == typeof(sbyte) || + fieldType == typeof(bool)) + accessorType = FieldAccessorType.StaticValueTypeSize1; + else if (fieldType == typeof(short) || + fieldType == typeof(ushort) || + fieldType == typeof(char)) + accessorType = FieldAccessorType.StaticValueTypeSize2; + else if (fieldType == typeof(int) || + fieldType == typeof(uint) || + fieldType == typeof(float)) + accessorType = FieldAccessorType.StaticValueTypeSize4; + else if (fieldType == typeof(long) || + fieldType == typeof(ulong) || + fieldType == typeof(double)) + accessorType = FieldAccessorType.StaticValueTypeSize8; + else if (fieldType == typeof(IntPtr) || + fieldType == typeof(UIntPtr)) + accessorType = GetIntPtrAccessorTypeForStatic(); + + return accessorType; + } + + private static FieldAccessorType GetIntPtrAccessorTypeForInstance() + { + FieldAccessorType accessorType = FieldAccessorType.InstanceValueType; + + if (IntPtr.Size == 4) + { + accessorType = FieldAccessorType.InstanceValueTypeSize4; + } + else if (IntPtr.Size == 8) + { + accessorType = FieldAccessorType.InstanceValueTypeSize8; + } + + return accessorType; + } + + private static FieldAccessorType GetIntPtrAccessorTypeForStatic() + { + FieldAccessorType accessorType = FieldAccessorType.StaticValueType; + + if (IntPtr.Size == 4) + { + accessorType = FieldAccessorType.StaticValueTypeSize4; + } + else if (IntPtr.Size == 8) + { + accessorType = FieldAccessorType.StaticValueTypeSize8; + } + + return accessorType; + } + + private static void ThrowHelperTargetException() => throw new TargetException(SR.RFLCT_Targ_StatFldReqTarg); + + private static void ThrowHelperArgumentException(object target, FieldInfo fieldInfo) => + throw new ArgumentException(SR.Format(SR.Arg_FieldDeclTarget, fieldInfo.Name, fieldInfo.DeclaringType, target.GetType())); + + private static void ThrowHelperFieldAccessException(string fieldName, string? declaringTypeName) => + throw new FieldAccessException(SR.Format(SR.RFLCT_CannotSetInitonlyStaticField, fieldName, declaringTypeName)); + + private enum FieldAccessorType + { + InstanceReferenceType, + InstanceValueType, + InstanceValueTypeSize1, + InstanceValueTypeSize2, + InstanceValueTypeSize4, + InstanceValueTypeSize8, + InstancePointerType, + StaticReferenceType, + StaticValueType, + StaticValueTypeSize1, + StaticValueTypeSize2, + StaticValueTypeSize4, + StaticValueTypeSize8, + StaticValueTypeBoxed, + StaticPointerType, + NoInvoke, + SlowPathUntilClassInitialized, + SlowPath, + } + } +} diff --git a/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs b/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs index 5f107d9b44e585..ff91da882d4ad5 100644 --- a/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs +++ b/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs @@ -2,6 +2,8 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Collections.Generic; +using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Linq; using Xunit; @@ -29,7 +31,7 @@ public void SetValue_ConstantField_ThrowsFieldAccessException(string field, obje [Fact] public void SetValue_ReadonlyField() { - FieldInfo fieldInfo = typeof(FieldInfoTests).GetTypeInfo().GetDeclaredField("readonlyIntField"); + FieldInfo fieldInfo = typeof(FieldInfoTests).GetTypeInfo().GetDeclaredField(nameof(readonlyIntField)); FieldInfoTests myInstance = new FieldInfoTests(); object current = fieldInfo.GetValue(myInstance); @@ -55,11 +57,39 @@ public static void CustomAttributes(Type type, string expectedToString) public static IEnumerable GetValue_TestData() { - yield return new object[] { typeof(FieldInfoTests), nameof(FieldInfoTests.s_intField), new FieldInfoTests(), 100 }; - yield return new object[] { typeof(FieldInfoTests), nameof(FieldInfoTests.s_intField), null, 100 }; - yield return new object[] { typeof(FieldInfoTests), nameof(FieldInfoTests.intField), new FieldInfoTests(), 101 }; - yield return new object[] { typeof(FieldInfoTests), nameof(FieldInfoTests.s_stringField), new FieldInfoTests(), "static" }; - yield return new object[] { typeof(FieldInfoTests), nameof(FieldInfoTests.stringField), new FieldInfoTests(), "non static" }; + yield return new object[] { typeof(FieldInfoTests), nameof(s_boolField), null, true }; + yield return new object[] { typeof(FieldInfoTests), nameof(s_intField), new FieldInfoTests(), 100 }; // Non-null 'obj' ignored. + yield return new object[] { typeof(FieldInfoTests), nameof(s_intField), null, 100 }; + yield return new object[] { typeof(FieldInfoTests), nameof(s_stringField), null, "static" }; + yield return new object[] { typeof(FieldInfoTests), nameof(s_myStruct), null, new MyStruct() }; + + yield return new object[] { typeof(FieldInfoTests), nameof(boolField), new FieldInfoTests(), true }; + yield return new object[] { typeof(FieldInfoTests), nameof(intField), new FieldInfoTests(), 101 }; + yield return new object[] { typeof(FieldInfoTests), nameof(stringField), new FieldInfoTests(), "non static" }; + yield return new object[] { typeof(FieldInfoTests), nameof(_myStruct), new FieldInfoTests(), new MyStruct() }; + + yield return new object[] { typeof(FieldInfoTests), nameof(shortEnumField), new FieldInfoTests(), default(ShortEnum) }; + yield return new object[] { typeof(FieldInfoTests), nameof(intEnumField), new FieldInfoTests(), default(IntEnum) }; + yield return new object[] { typeof(FieldInfoTests), nameof(longEnumField), new FieldInfoTests(), default(LongEnum) }; + yield return new object[] { typeof(FieldInfoTests), nameof(s_shortEnumField), null, default(ShortEnum) }; + yield return new object[] { typeof(FieldInfoTests), nameof(s_intEnumField), null, default(IntEnum) }; + yield return new object[] { typeof(FieldInfoTests), nameof(s_longEnumField), null, default(LongEnum) }; + + yield return new object[] { typeof(MyStruct), nameof(MyStruct.s_boolField), null, true }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.s_intField), null, 100 }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.s_intField), null, 100 }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.s_constIntField), null, 102 }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.s_stringField), null, "static" }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.s_readonlyStringField), null, "readonlyStatic" }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.s_objectField), null, MyStruct.s_objectField }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.s_intPtr), null, MyStruct.s_intPtrForComparison }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.s_rvaIntField), null, new int[] { 1, 2, 3 } }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.threadStatic_intField), null, 100 }; + + yield return new object[] { typeof(MyStruct), nameof(MyStruct.stringField), new MyStruct(), "non static" }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.intField), new MyStruct(), 101 }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.intPtr), new MyStruct(), MyStruct.intPtrForComparison }; + yield return new object[] { typeof(MyStruct_OnlyPrimitiveTypes), nameof(MyStruct_OnlyPrimitiveTypes.intField), new MyStruct_OnlyPrimitiveTypes(), 101 }; } [Theory] @@ -68,6 +98,43 @@ public void GetValue(Type type, string name, object obj, object expected) { FieldInfo fieldInfo = GetField(type, name); Assert.Equal(expected, fieldInfo.GetValue(obj)); + + // Perform a second time to rule out cases of slow-path vs. fast-path. + Assert.Equal(expected, fieldInfo.GetValue(obj)); + } + + public static IEnumerable GetValue_TestData_WithFunctionPointers() + { + yield return new object[] { typeof(MyStructWithFunctionPointers), nameof(MyStructWithFunctionPointers.s_fcnPtr), null, (IntPtr)45 }; + yield return new object[] { typeof(MyStructWithFunctionPointers), nameof(MyStructWithFunctionPointers.fcnPtr), new MyStructWithFunctionPointers(), (IntPtr)44 }; + } + + [Theory] + [ActiveIssue("https://github.com/dotnet/runtime/issues/97833", typeof(PlatformDetection), nameof(PlatformDetection.IsNativeAot))] + [MemberData(nameof(GetValue_TestData_WithFunctionPointers))] + public void GetValueWithFunctionPointers(Type type, string name, object obj, object expected) + { + FieldInfo fieldInfo = GetField(type, name); + Assert.Equal(expected, fieldInfo.GetValue(obj)); + + // Perform a second time to rule out cases of slow-path vs. fast-path. + Assert.Equal(expected, fieldInfo.GetValue(obj)); + } + + [Fact] + public void GetAndSetValueTypeFromStatic() + { + FieldInfo fieldInfo = GetField(typeof(FieldInfoTests), nameof(s_myStruct_GetAndSet)); + s_myStruct_GetAndSet.intField = 10; + object obj = fieldInfo.GetValue(null); + Assert.Equal(10, ((MyStruct)obj).intField); + s_myStruct_GetAndSet.intField = 11; + + // Make sure the previously boxed value didn't change. The runtime boxes non-primitive value types internally. + Assert.Equal(10, ((MyStruct)obj).intField); + + obj = fieldInfo.GetValue(null); + Assert.Equal(11, ((MyStruct)obj).intField); } public static IEnumerable GetValue_Invalid_TestData() @@ -82,23 +149,75 @@ public void GetValue_Invalid(Type type, string name, object obj, Type exceptionT { FieldInfo fieldInfo = GetField(type, name); Assert.Throws(exceptionType, () => fieldInfo.GetValue(obj)); + + // Perform a second time to rule out cases of slow-path vs. fast-path. + Assert.Throws(exceptionType, () => fieldInfo.GetValue(obj)); } public static IEnumerable SetValue_TestData() { - yield return new object[] { typeof(FieldInfoTests), nameof(FieldInfoTests.s_intField), new FieldInfoTests(), 1000, 1000 }; - yield return new object[] { typeof(FieldInfoTests), nameof(FieldInfoTests.s_intField), null, 1000, 1000 }; - yield return new object[] { typeof(FieldInfoTests), nameof(FieldInfoTests.intField), new FieldInfoTests(), 1000, 1000 }; - yield return new object[] { typeof(FieldInfoTests), nameof(FieldInfoTests.s_stringField), new FieldInfoTests(), "new", "new" }; - yield return new object[] { typeof(FieldInfoTests), nameof(FieldInfoTests.stringField), new FieldInfoTests(), "new", "new" }; - yield return new object[] { typeof(FieldInfoTests), nameof(FieldInfoTests.shortEnumField), new FieldInfoTests(), (byte)1, (ShortEnum)1 }; - yield return new object[] { typeof(FieldInfoTests), nameof(FieldInfoTests.intEnumField), new FieldInfoTests(), (short)2, (IntEnum)2 }; - yield return new object[] { typeof(FieldInfoTests), nameof(FieldInfoTests.longEnumField), new FieldInfoTests(), (int)3, (LongEnum)3 }; + yield return new object[] { typeof(FieldInfoTests), nameof(s_boolField_Set), null, true, true }; + yield return new object[] { typeof(FieldInfoTests), nameof(s_boolField_Set), null, false, false }; + yield return new object[] { typeof(FieldInfoTests), nameof(s_intField_Set), new FieldInfoTests(), 1000, 1000 }; // Non-null 'obj' ignored. + yield return new object[] { typeof(FieldInfoTests), nameof(s_intField_Set), null, 1001, 1001 }; + yield return new object[] { typeof(FieldInfoTests), nameof(s_stringField_Set), null, "new", "new" }; + yield return new object[] { typeof(FieldInfoTests), nameof(s_myStruct_Set), null, s_myStruct_Set, s_myStruct_Set }; + + yield return new object[] { typeof(FieldInfoTests), nameof(boolField), new FieldInfoTests(), true, true }; + yield return new object[] { typeof(FieldInfoTests), nameof(boolField), new FieldInfoTests(), false, false }; + yield return new object[] { typeof(FieldInfoTests), nameof(stringField), new FieldInfoTests(), "new", "new" }; + yield return new object[] { typeof(FieldInfoTests), nameof(shortEnumField), new FieldInfoTests(), (byte)1, (ShortEnum)1 }; + yield return new object[] { typeof(FieldInfoTests), nameof(intEnumField), new FieldInfoTests(), (short)2, (IntEnum)2 }; + yield return new object[] { typeof(FieldInfoTests), nameof(longEnumField), new FieldInfoTests(), (int)3, (LongEnum)3 }; + yield return new object[] { typeof(FieldInfoTests), nameof(_myStruct), new FieldInfoTests(), s_myStruct_Set, s_myStruct_Set }; + + yield return new object[] { typeof(MyStruct), nameof(MyStruct.s_boolField_Set), null, true, true }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.s_boolField_Set), null, false, false }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.s_intField_Set), null, 1001, 1001 }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.s_stringField_Set), null, "new", "new" }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.s_objectField_Set), null, MyStruct.s_objectField, MyStruct.s_objectField }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.s_intPtr_Set), null, MyStruct.s_intPtrForComparison, MyStruct.s_intPtrForComparison }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.threadStatic_intField_Set), null, 100, 100 }; + + yield return new object[] { typeof(MyStruct), nameof(MyStruct.boolField), new MyStruct(), true, true }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.boolField), new MyStruct(), false, false }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.intField), new MyStruct(), 1002, 1002 }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.stringField), new MyStruct(), "new", "new" }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.objectField), new MyStruct(), MyStruct.s_objectField, MyStruct.s_objectField }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.intPtr), new MyStruct(), MyStruct.s_intPtrForComparison, MyStruct.s_intPtrForComparison }; } [Theory] [MemberData(nameof(SetValue_TestData))] public void SetValue(Type type, string name, object obj, object value, object expected) + { + FieldInfo fieldInfo = GetField(type, name); + object original = fieldInfo.GetValue(obj); + try + { + fieldInfo.SetValue(obj, value); + Assert.Equal(expected, fieldInfo.GetValue(obj)); + + // Perform a second time to rule out cases of slow-path vs. fast-path. + fieldInfo.SetValue(obj, value); + Assert.Equal(expected, fieldInfo.GetValue(obj)); + } + finally + { + fieldInfo.SetValue(obj, original); + } + } + + public static IEnumerable SetValue_TestData_FunctionPointers() + { + yield return new object[] { typeof(MyStructWithFunctionPointers), nameof(MyStructWithFunctionPointers.s_fcnPtr_Set), null, (IntPtr)201, (IntPtr)201 }; + yield return new object[] { typeof(MyStructWithFunctionPointers), nameof(MyStructWithFunctionPointers.fcnPtr), new MyStructWithFunctionPointers(), (IntPtr)200, (IntPtr)200 }; + } + + [Theory] + [ActiveIssue("https://github.com/dotnet/runtime/issues/97833", typeof(PlatformDetection), nameof(PlatformDetection.IsNativeAot))] + [MemberData(nameof(SetValue_TestData_FunctionPointers))] + public void SetValueWithFunctionPointers(Type type, string name, object obj, object value, object expected) { FieldInfo fieldInfo = GetField(type, name); object original = fieldInfo.GetValue(obj); @@ -115,17 +234,23 @@ public void SetValue(Type type, string name, object obj, object value, object ex public static IEnumerable SetValue_Invalid_TestData() { - yield return new object[] { typeof(FieldInfoTests), nameof(FieldInfoTests.stringField), null, "new", typeof(TargetException) }; - yield return new object[] { typeof(FieldInfoTests), nameof(FieldInfoTests.stringField), new object(), "new", typeof(ArgumentException) }; - yield return new object[] { typeof(FieldInfoTests), nameof(FieldInfoTests.stringField), new FieldInfoTests(), 100, typeof(ArgumentException) }; + yield return new object[] { typeof(FieldInfoTests), nameof(stringField), null, "new", typeof(TargetException) }; + yield return new object[] { typeof(FieldInfoTests), nameof(stringField), new object(), "new", typeof(ArgumentException) }; + yield return new object[] { typeof(FieldInfoTests), nameof(stringField), new FieldInfoTests(), 100, typeof(ArgumentException) }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.s_constIntField), null, 100, typeof(FieldAccessException) }; + yield return new object[] { typeof(MyStruct), nameof(MyStruct.s_rvaIntField), null, new int[] { 3, 4, 5 }, typeof(FieldAccessException) }; } [Theory] + [ActiveIssue("https://github.com/dotnet/runtime/issues/97829", TestRuntimes.Mono)] [MemberData(nameof(SetValue_Invalid_TestData))] public void SetValue_Invalid(Type type, string name, object obj, object value, Type exceptionType) { FieldInfo fieldInfo = GetField(type, name); Assert.Throws(exceptionType, () => fieldInfo.SetValue(obj, value)); + + // Perform a second time to rule out cases of slow-path vs. fast-path. + Assert.Throws(exceptionType, () => fieldInfo.SetValue(obj, value)); } [Theory] @@ -239,6 +364,7 @@ public void IsPrivate(Type type, string name, bool expected) [Theory] [InlineData(typeof(FieldInfoTests), nameof(FieldInfoTests.readonlyIntField), true)] + [InlineData(typeof(FieldInfoTests), nameof(FieldInfoTests.s_readonlyIntField), true)] [InlineData(typeof(FieldInfoTests), nameof(FieldInfoTests.intField), false)] public void IsInitOnly(Type type, string name, bool expected) { @@ -462,18 +588,32 @@ private static FieldInfo GetField(Type type, string name) public const long ConstInt64Field = 1000; public const byte ConstByteField = 0; + public static bool s_boolField = true; + public static bool s_boolField_Set = false; public static int s_intField = 100; + public static int s_intField_Set = 0; public static string s_stringField = "static"; + public static string s_stringField_Set = "static"; + public static readonly int s_readonlyIntField = 100; + public bool boolField = true; public int intField = 101; public string stringField = "non static"; - public enum ShortEnum : short {} - public enum IntEnum {} - public enum LongEnum : long {} + public MyStruct _myStruct = new MyStruct(); + public static MyStruct s_myStruct = new MyStruct(); + public static MyStruct s_myStruct_Set = new MyStruct(); + public static MyStruct s_myStruct_GetAndSet = new MyStruct(); + + public enum ShortEnum : short { } + public enum IntEnum { } + public enum LongEnum : long { } public ShortEnum shortEnumField; public IntEnum intEnumField; public LongEnum longEnumField; + public static ShortEnum s_shortEnumField; + public static IntEnum s_intEnumField; + public static LongEnum s_longEnumField; private int privateIntField = 1; private string privateStringField = "privateStringField"; @@ -586,5 +726,55 @@ public static void SetValueDirect_GetValueDirectRoundDataTest(object value) Assert.Equal(value, result); } + + + public struct MyStruct_OnlyPrimitiveTypes + { + public int intField = 101; + + public MyStruct_OnlyPrimitiveTypes() + { + } + } + + public struct MyStruct + { + public static bool s_boolField = true; + public static bool s_boolField_Set = false; + public static int s_intField = 100; + public static int s_intField_Set = 0; + [ThreadStatic] public static int threadStatic_intField = 100; + [ThreadStatic] public static int threadStatic_intField_Set = 0; + public static string s_stringField = "static"; + public static readonly string s_readonlyStringField = "readonlyStatic"; + public static string s_stringField_Set = null; + public static object s_objectField = new MyClass1(); + public static object s_objectField_Set = null; + + // This does not report FieldAttributes.HasFieldRVA since Roslyn wraps the .data with generated helper class. + public static readonly int[] s_rvaIntField = [1, 2, 3]; + + public unsafe static object intPtrForComparison = Pointer.Box((void*)42, typeof(int*)); + public unsafe static int* s_intPtr = (int*)43; + public unsafe static int* s_intPtr_Set = (int*)0; + public unsafe static object s_intPtrForComparison = Pointer.Box((void*)43, typeof(int*)); + public bool boolField = true; + public int intField = 101; + public object objectField = null; + public string stringField = "non static"; + public const int s_constIntField = 102; + public unsafe int* intPtr = (int*)42; + + public MyStruct() { } + } + + public struct MyStructWithFunctionPointers + { + public unsafe static delegate* s_fcnPtr = (delegate*)45; + public unsafe static delegate* s_fcnPtr_Set = (delegate*)0; + public unsafe delegate* fcnPtr = (delegate*)44; + + public MyStructWithFunctionPointers() { } + } } } From 2ae69b20cfc0560a971c7cc7d301e196d1eb02fb Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Fri, 9 Feb 2024 16:23:16 -0600 Subject: [PATCH 02/12] Apply feedback --- src/coreclr/vm/field.h | 4 +- src/coreclr/vm/reflectioninvocation.cpp | 84 +++++++++++++++---------- 2 files changed, 52 insertions(+), 36 deletions(-) diff --git a/src/coreclr/vm/field.h b/src/coreclr/vm/field.h index 1bc8885c9faabf..0d2efde5b5bf65 100644 --- a/src/coreclr/vm/field.h +++ b/src/coreclr/vm/field.h @@ -289,8 +289,8 @@ class FieldDesc { LIMITED_METHOD_DAC_CONTRACT; - LoaderAllocator *pLoaderAllocatorOfMethod = GetApproxEnclosingMethodTable()->GetLoaderAllocator(); - return pLoaderAllocatorOfMethod->IsCollectible(); + LoaderAllocator *pLoaderAllocator = GetApproxEnclosingMethodTable()->GetLoaderAllocator(); + return pLoaderAllocator->IsCollectible(); } // Was this field added by EnC? diff --git a/src/coreclr/vm/reflectioninvocation.cpp b/src/coreclr/vm/reflectioninvocation.cpp index 788c31b1ff4335..ec5ad2871591d4 100644 --- a/src/coreclr/vm/reflectioninvocation.cpp +++ b/src/coreclr/vm/reflectioninvocation.cpp @@ -26,7 +26,8 @@ #include "argdestination.h" FCIMPL5(Object*, RuntimeFieldHandle::GetValue, ReflectFieldObject *pFieldUNSAFE, Object *instanceUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE, ReflectClassBaseObject *pDeclaringTypeUNSAFE, CLR_BOOL *pDomainInitialized) { - CONTRACTL { + CONTRACTL + { FCALL_CHECK; } CONTRACTL_END; @@ -62,7 +63,8 @@ FCIMPL5(Object*, RuntimeFieldHandle::GetValue, ReflectFieldObject *pFieldUNSAFE, FCIMPLEND FCIMPL2(FC_BOOL_RET, ReflectionInvocation::CanValueSpecialCast, ReflectClassBaseObject *pValueTypeUNSAFE, ReflectClassBaseObject *pTargetTypeUNSAFE) { - CONTRACTL { + CONTRACTL + { FCALL_CHECK; PRECONDITION(CheckPointer(pValueTypeUNSAFE)); PRECONDITION(CheckPointer(pTargetTypeUNSAFE)); @@ -115,7 +117,8 @@ FCIMPLEND /// Allocate the value type and copy the optional value into it. /// FCIMPL2(Object*, ReflectionInvocation::AllocateValueType, ReflectClassBaseObject *pTargetTypeUNSAFE, Object *valueUNSAFE) { - CONTRACTL { + CONTRACTL + { FCALL_CHECK; PRECONDITION(CheckPointer(pTargetTypeUNSAFE)); PRECONDITION(CheckPointer(valueUNSAFE, NULL_OK)); @@ -159,7 +162,8 @@ FCIMPL2(Object*, ReflectionInvocation::AllocateValueType, ReflectClassBaseObject FCIMPLEND FCIMPL6(void, RuntimeFieldHandle::SetValue, ReflectFieldObject *pFieldUNSAFE, Object *targetUNSAFE, Object *valueUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE, ReflectClassBaseObject *pDeclaringTypeUNSAFE, CLR_BOOL *pDomainInitialized) { - CONTRACTL { + CONTRACTL + { FCALL_CHECK; } CONTRACTL_END; @@ -203,7 +207,8 @@ extern "C" void QCALLTYPE RuntimeTypeHandle_CreateInstanceForAnotherGenericParam QCall::ObjectHandleOnStack pInstantiatedObject ) { - CONTRACTL{ + CONTRACTL + { QCALL_CHECK; PRECONDITION(!pTypeHandle.AsTypeHandle().IsNull()); PRECONDITION(cInstArray >= 0); @@ -288,7 +293,8 @@ FCIMPLEND static OBJECTREF InvokeArrayConstructor(TypeHandle th, PVOID* args, int argCnt) { - CONTRACTL { + CONTRACTL + { THROWS; GC_TRIGGERS; MODE_COOPERATIVE; @@ -322,7 +328,8 @@ static OBJECTREF InvokeArrayConstructor(TypeHandle th, PVOID* args, int argCnt) static BOOL IsActivationNeededForMethodInvoke(MethodDesc * pMD) { - CONTRACTL { + CONTRACTL + { THROWS; GC_TRIGGERS; MODE_COOPERATIVE; @@ -853,7 +860,8 @@ struct SkipStruct { // This method is called by the GetMethod function and will crawl backward // up the stack for integer methods. static StackWalkAction SkipMethods(CrawlFrame* frame, VOID* data) { - CONTRACTL { + CONTRACTL + { NOTHROW; GC_NOTRIGGER; MODE_ANY; @@ -915,7 +923,8 @@ FCIMPL1(ReflectMethodObject*, RuntimeMethodHandle::GetCurrentMethod, StackCrawlM FCIMPLEND static OBJECTREF DirectObjectFieldGet(FieldDesc *pField, TypeHandle fieldType, TypeHandle enclosingType, TypedByRef *pTarget, CLR_BOOL *pDomainInitialized) { - CONTRACTL { + CONTRACTL + { THROWS; GC_TRIGGERS; MODE_COOPERATIVE; @@ -938,7 +947,8 @@ static OBJECTREF DirectObjectFieldGet(FieldDesc *pField, TypeHandle fieldType, T } FCIMPL4(Object*, RuntimeFieldHandle::GetValueDirect, ReflectFieldObject *pFieldUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE, TypedByRef *pTarget, ReflectClassBaseObject *pDeclaringTypeUNSAFE) { - CONTRACTL { + CONTRACTL + { FCALL_CHECK; } CONTRACTL_END; @@ -1038,7 +1048,8 @@ lExit: ; FCIMPLEND static void DirectObjectFieldSet(FieldDesc *pField, TypeHandle fieldType, TypeHandle enclosingType, TypedByRef *pTarget, OBJECTREF *pValue, CLR_BOOL *pDomainInitialized) { - CONTRACTL { + CONTRACTL + { THROWS; GC_TRIGGERS; MODE_COOPERATIVE; @@ -1061,7 +1072,8 @@ static void DirectObjectFieldSet(FieldDesc *pField, TypeHandle fieldType, TypeHa } FCIMPL5(void, RuntimeFieldHandle::SetValueDirect, ReflectFieldObject *pFieldUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE, TypedByRef *pTarget, Object *valueUNSAFE, ReflectClassBaseObject *pContextTypeUNSAFE) { - CONTRACTL { + CONTRACTL + { FCALL_CHECK; } CONTRACTL_END; @@ -1221,9 +1233,10 @@ lExit: ; } FCIMPLEND -static FC_BOOL_RET IsFastPathSupportedHelper(FieldDesc* pFieldDesc) +static bool IsFastPathSupportedHelper(FieldDesc* pFieldDesc) { - CONTRACTL { + CONTRACTL + { NOTHROW; GC_NOTRIGGER; MODE_ANY; @@ -1231,38 +1244,34 @@ static FC_BOOL_RET IsFastPathSupportedHelper(FieldDesc* pFieldDesc) } CONTRACTL_END; - return pFieldDesc->IsThreadStatic() || - pFieldDesc->IsEnCNew() || - pFieldDesc->IsCollectible() ? FALSE : TRUE; + return !pFieldDesc->IsThreadStatic() && + !pFieldDesc->IsEnCNew() && + !pFieldDesc->IsCollectible(); } FCIMPL1(FC_BOOL_RET, RuntimeFieldHandle::IsFastPathSupported, ReflectFieldObject *pFieldUNSAFE) { - CONTRACTL { - FCALL_CHECK; - } - CONTRACTL_END; + FCALL_CONTRACT; REFLECTFIELDREF refField = (REFLECTFIELDREF)ObjectToOBJECTREF(pFieldUNSAFE); - if (refField == NULL) - FCThrowRes(kArgumentNullException, W("Arg_InvalidHandle")); + _ASSERTE(refField != NULL); FieldDesc* pFieldDesc = refField->GetField(); - return IsFastPathSupportedHelper(pFieldDesc); + return IsFastPathSupportedHelper(pFieldDesc) ? TRUE : FALSE; } FCIMPLEND FCIMPL1(INT32, RuntimeFieldHandle::GetInstanceFieldOffset, ReflectFieldObject *pFieldUNSAFE) { - CONTRACTL { + CONTRACTL + { FCALL_CHECK; PRECONDITION(CheckPointer(pFieldUNSAFE)); } CONTRACTL_END; REFLECTFIELDREF refField = (REFLECTFIELDREF)ObjectToOBJECTREF(pFieldUNSAFE); - if (refField == NULL) - FCThrowRes(kArgumentNullException, W("Arg_InvalidHandle")); + _ASSERTE(refField != NULL); FieldDesc* pFieldDesc = refField->GetField(); _ASSERTE(!pFieldDesc->IsStatic()); @@ -1276,7 +1285,8 @@ FCIMPLEND FCIMPL1(void*, RuntimeFieldHandle::GetStaticFieldAddress, ReflectFieldObject *pFieldUNSAFE) { - CONTRACTL { + CONTRACTL + { FCALL_CHECK; PRECONDITION(CheckPointer(pFieldUNSAFE)); } @@ -1373,7 +1383,8 @@ static void PrepareMethodHelper(MethodDesc * pMD) // It does not walk a subset of callgraph to provide CER guarantees. extern "C" void QCALLTYPE ReflectionInvocation_PrepareMethod(MethodDesc *pMD, TypeHandle *pInstantiation, UINT32 cInstantiation) { - CONTRACTL { + CONTRACTL + { QCALL_CHECK; PRECONDITION(pMD != NULL); PRECONDITION(CheckPointer(pInstantiation, NULL_OK)); @@ -1426,7 +1437,8 @@ extern "C" void QCALLTYPE ReflectionInvocation_PrepareMethod(MethodDesc *pMD, Ty // was prepared prior to the Combine. FCIMPL1(void, ReflectionInvocation::PrepareDelegate, Object* delegateUNSAFE) { - CONTRACTL { + CONTRACTL + { FCALL_CHECK; PRECONDITION(CheckPointer(delegateUNSAFE, NULL_OK)); } @@ -1487,7 +1499,8 @@ FCIMPLEND FCIMPL4(void, ReflectionInvocation::MakeTypedReference, TypedByRef * value, Object* targetUNSAFE, ArrayBase* fldsUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE) { - CONTRACTL { + CONTRACTL + { FCALL_CHECK; PRECONDITION(CheckPointer(targetUNSAFE)); PRECONDITION(CheckPointer(fldsUNSAFE)); @@ -1759,7 +1772,8 @@ extern "C" void QCALLTYPE RuntimeTypeHandle_GetActivationInfo( BOOL* pfCtorIsPublic ) { - CONTRACTL{ + CONTRACTL + { QCALL_CHECK; PRECONDITION(CheckPointer(ppfnAllocator)); PRECONDITION(CheckPointer(pvAllocatorFirstArg)); @@ -1878,7 +1892,8 @@ extern "C" void QCALLTYPE RuntimeTypeHandle_GetActivationInfo( FCIMPL1(Object*, RuntimeTypeHandle::AllocateComObject, void* pClassFactory) { - CONTRACTL{ + CONTRACTL + { FCALL_CHECK; PRECONDITION(CheckPointer(pClassFactory)); } @@ -1927,7 +1942,8 @@ extern "C" void QCALLTYPE ReflectionSerialization_GetCreateUninitializedObjectIn PCODE* ppfnAllocator, void** pvAllocatorFirstArg) { - CONTRACTL{ + CONTRACTL + { QCALL_CHECK; PRECONDITION(CheckPointer(ppfnAllocator)); PRECONDITION(CheckPointer(pvAllocatorFirstArg)); From e15a9ffbdd95de9a935e44a65a2c4aef7f823207 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Mon, 12 Feb 2024 09:08:38 -0600 Subject: [PATCH 03/12] Change a throw to an assert --- src/coreclr/vm/reflectioninvocation.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/vm/reflectioninvocation.cpp b/src/coreclr/vm/reflectioninvocation.cpp index ec5ad2871591d4..b7680eb4581ef0 100644 --- a/src/coreclr/vm/reflectioninvocation.cpp +++ b/src/coreclr/vm/reflectioninvocation.cpp @@ -1293,8 +1293,7 @@ FCIMPL1(void*, RuntimeFieldHandle::GetStaticFieldAddress, ReflectFieldObject *pF CONTRACTL_END; REFLECTFIELDREF refField = (REFLECTFIELDREF)ObjectToOBJECTREF(pFieldUNSAFE); - if (refField == NULL) - FCThrowRes(kArgumentNullException, W("Arg_InvalidHandle")); + _ASSERTE(refField != NULL); FieldDesc* pFieldDesc = refField->GetField(); _ASSERTE(pFieldDesc->IsStatic()); From a32cb6e6a1c356eb759b8059205d9c33d1f1db42 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Mon, 12 Feb 2024 17:34:50 -0600 Subject: [PATCH 04/12] Change 'ref bool domainInitialized' to 'bool isInitialized' --- .../src/System/RuntimeHandles.cs | 4 +-- src/coreclr/vm/invokeutil.cpp | 17 ++++------- src/coreclr/vm/invokeutil.h | 4 +-- src/coreclr/vm/reflectioninvocation.cpp | 22 +++++++-------- src/coreclr/vm/runtimehandles.h | 4 +-- .../src/System/Reflection/FieldAccessor.cs | 28 ++++--------------- .../src/System/Reflection/RuntimeFieldInfo.cs | 3 +- .../src/System/RuntimeFieldHandle.cs | 4 ++- 8 files changed, 32 insertions(+), 54 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs b/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs index 21228998515ac4..a7404d9dd8328e 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs @@ -1203,13 +1203,13 @@ internal static RuntimeType GetApproxDeclaringType(IRuntimeFieldInfo field) internal static extern int GetToken(RtFieldInfo field); [MethodImpl(MethodImplOptions.InternalCall)] - internal static extern object? GetValue(RtFieldInfo field, object? instance, RuntimeType fieldType, RuntimeType? declaringType, ref bool domainInitialized); + internal static extern object? GetValue(RtFieldInfo field, object? instance, RuntimeType fieldType, RuntimeType? declaringType, bool isInitialized); [MethodImpl(MethodImplOptions.InternalCall)] internal static extern object? GetValueDirect(RtFieldInfo field, RuntimeType fieldType, void* pTypedRef, RuntimeType? contextType); [MethodImpl(MethodImplOptions.InternalCall)] - internal static extern void SetValue(RtFieldInfo field, object? obj, object? value, RuntimeType fieldType, RuntimeType? declaringType, ref bool domainInitialized); + internal static extern void SetValue(RtFieldInfo field, object? obj, object? value, RuntimeType fieldType, RuntimeType? declaringType, bool isInitialized); [MethodImpl(MethodImplOptions.InternalCall)] internal static extern void SetValueDirect(RtFieldInfo field, RuntimeType fieldType, void* pTypedRef, object? value, RuntimeType? contextType); diff --git a/src/coreclr/vm/invokeutil.cpp b/src/coreclr/vm/invokeutil.cpp index 0d7994a330ca75..07b3fcfdedbafe 100644 --- a/src/coreclr/vm/invokeutil.cpp +++ b/src/coreclr/vm/invokeutil.cpp @@ -748,7 +748,7 @@ void InvokeUtil::SetValidField(CorElementType fldType, OBJECTREF *target, OBJECTREF *valueObj, TypeHandle declaringType, - CLR_BOOL *pDomainInitialized) { + CLR_BOOL isInitialized) { CONTRACTL { THROWS; GC_TRIGGERS; @@ -786,19 +786,17 @@ void InvokeUtil::SetValidField(CorElementType fldType, pDeclMT = pField->GetModule()->GetGlobalMethodTable(); } - if (*pDomainInitialized == FALSE) + if (isInitialized == FALSE) { EX_TRY { pDeclMT->EnsureInstanceActive(); pDeclMT->CheckRunClassInitThrowing(); - - *pDomainInitialized = TRUE; } EX_CATCH_THROWABLE(&Throwable); } #ifdef _DEBUG - else if (*pDomainInitialized == TRUE && !declaringType.IsNull()) + else if (isInitialized == TRUE && !declaringType.IsNull()) CONSISTENCY_CHECK(declaringType.GetMethodTable()->CheckActivated()); #endif @@ -973,7 +971,7 @@ void InvokeUtil::SetValidField(CorElementType fldType, // GetFieldValue // This method will return an ARG_SLOT containing the value of the field. -OBJECTREF InvokeUtil::GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJECTREF* target, TypeHandle declaringType, CLR_BOOL *pDomainInitialized) { +OBJECTREF InvokeUtil::GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJECTREF* target, TypeHandle declaringType, CLR_BOOL isInitialized) { CONTRACTL { THROWS; GC_TRIGGERS; @@ -1011,23 +1009,20 @@ OBJECTREF InvokeUtil::GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJ pDeclMT = pField->GetModule()->GetGlobalMethodTable(); } - if (*pDomainInitialized == FALSE) + if (isInitialized == FALSE) { EX_TRY { pDeclMT->EnsureInstanceActive(); pDeclMT->CheckRunClassInitThrowing(); - - *pDomainInitialized = TRUE; } EX_CATCH_THROWABLE(&Throwable); } #ifdef _DEBUG - else if (*pDomainInitialized == TRUE && !declaringType.IsNull()) + else if (isInitialized == TRUE && !declaringType.IsNull()) CONSISTENCY_CHECK(declaringType.GetMethodTable()->CheckActivated()); #endif - if(Throwable != NULL) { GCPROTECT_BEGIN(Throwable); diff --git a/src/coreclr/vm/invokeutil.h b/src/coreclr/vm/invokeutil.h index 0bd1577c7a19d3..aaa9bd0a88cf3b 100644 --- a/src/coreclr/vm/invokeutil.h +++ b/src/coreclr/vm/invokeutil.h @@ -138,9 +138,9 @@ class InvokeUtil // SetValidField // Given an target object, a value object and a field this method will set the field // on the target object. The field must be validate before calling this. - static void SetValidField(CorElementType fldType, TypeHandle fldTH, FieldDesc* pField, OBJECTREF* target, OBJECTREF* value, TypeHandle declaringType, CLR_BOOL *pDomainInitialized); + static void SetValidField(CorElementType fldType, TypeHandle fldTH, FieldDesc* pField, OBJECTREF* target, OBJECTREF* value, TypeHandle declaringType, CLR_BOOL isInitialized); - static OBJECTREF GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJECTREF* target, TypeHandle declaringType, CLR_BOOL *pDomainInitialized); + static OBJECTREF GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJECTREF* target, TypeHandle declaringType, CLR_BOOL isInitialized); // ValidateObjectTarget // This method will validate the Object/Target relationship diff --git a/src/coreclr/vm/reflectioninvocation.cpp b/src/coreclr/vm/reflectioninvocation.cpp index b7680eb4581ef0..f03fa190720a51 100644 --- a/src/coreclr/vm/reflectioninvocation.cpp +++ b/src/coreclr/vm/reflectioninvocation.cpp @@ -25,7 +25,7 @@ #include "dbginterface.h" #include "argdestination.h" -FCIMPL5(Object*, RuntimeFieldHandle::GetValue, ReflectFieldObject *pFieldUNSAFE, Object *instanceUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE, ReflectClassBaseObject *pDeclaringTypeUNSAFE, CLR_BOOL *pDomainInitialized) { +FCIMPL5(Object*, RuntimeFieldHandle::GetValue, ReflectFieldObject *pFieldUNSAFE, Object *instanceUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE, ReflectClassBaseObject *pDeclaringTypeUNSAFE, CLR_BOOL isInitialized) { CONTRACTL { FCALL_CHECK; @@ -55,7 +55,7 @@ FCIMPL5(Object*, RuntimeFieldHandle::GetValue, ReflectFieldObject *pFieldUNSAFE, HELPER_METHOD_FRAME_BEGIN_RET_PROTECT(gc); // There can be no GC after this until the Object is returned. - rv = InvokeUtil::GetFieldValue(gc.refField->GetField(), fieldType, &gc.target, declaringType, pDomainInitialized); + rv = InvokeUtil::GetFieldValue(gc.refField->GetField(), fieldType, &gc.target, declaringType, isInitialized); HELPER_METHOD_FRAME_END(); return OBJECTREFToObject(rv); @@ -161,7 +161,7 @@ FCIMPL2(Object*, ReflectionInvocation::AllocateValueType, ReflectClassBaseObject } FCIMPLEND -FCIMPL6(void, RuntimeFieldHandle::SetValue, ReflectFieldObject *pFieldUNSAFE, Object *targetUNSAFE, Object *valueUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE, ReflectClassBaseObject *pDeclaringTypeUNSAFE, CLR_BOOL *pDomainInitialized) { +FCIMPL6(void, RuntimeFieldHandle::SetValue, ReflectFieldObject *pFieldUNSAFE, Object *targetUNSAFE, Object *valueUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE, ReflectClassBaseObject *pDeclaringTypeUNSAFE, CLR_BOOL isInitialized) { CONTRACTL { FCALL_CHECK; @@ -194,7 +194,7 @@ FCIMPL6(void, RuntimeFieldHandle::SetValue, ReflectFieldObject *pFieldUNSAFE, Ob HELPER_METHOD_FRAME_BEGIN_PROTECT(gc); - InvokeUtil::SetValidField(fieldType.GetVerifierCorElementType(), fieldType, pFieldDesc, &gc.target, &gc.value, declaringType, pDomainInitialized); + InvokeUtil::SetValidField(fieldType.GetVerifierCorElementType(), fieldType, pFieldDesc, &gc.target, &gc.value, declaringType, isInitialized); HELPER_METHOD_FRAME_END(); } @@ -922,7 +922,7 @@ FCIMPL1(ReflectMethodObject*, RuntimeMethodHandle::GetCurrentMethod, StackCrawlM } FCIMPLEND -static OBJECTREF DirectObjectFieldGet(FieldDesc *pField, TypeHandle fieldType, TypeHandle enclosingType, TypedByRef *pTarget, CLR_BOOL *pDomainInitialized) { +static OBJECTREF DirectObjectFieldGet(FieldDesc *pField, TypeHandle fieldType, TypeHandle enclosingType, TypedByRef *pTarget, CLR_BOOL isInitialized) { CONTRACTL { THROWS; @@ -941,7 +941,7 @@ static OBJECTREF DirectObjectFieldGet(FieldDesc *pField, TypeHandle fieldType, T } InvokeUtil::ValidateObjectTarget(pField, enclosingType, &objref); - refRet = InvokeUtil::GetFieldValue(pField, fieldType, &objref, enclosingType, pDomainInitialized); + refRet = InvokeUtil::GetFieldValue(pField, fieldType, &objref, enclosingType, isInitialized); GCPROTECT_END(); return refRet; } @@ -982,9 +982,8 @@ FCIMPL4(Object*, RuntimeFieldHandle::GetValueDirect, ReflectFieldObject *pFieldU _ASSERTE(gc.refDeclaringType == NULL || !gc.refDeclaringType->GetType().IsTypeDesc()); MethodTable *pEnclosingMT = (gc.refDeclaringType != NULL ? gc.refDeclaringType->GetType() : TypeHandle()).AsMethodTable(); - CLR_BOOL domainInitialized = FALSE; if (pField->IsStatic() || !targetType.IsValueType()) { - refRet = DirectObjectFieldGet(pField, fieldType, TypeHandle(pEnclosingMT), pTarget, &domainInitialized); + refRet = DirectObjectFieldGet(pField, fieldType, TypeHandle(pEnclosingMT), pTarget, FALSE); goto lExit; } @@ -1047,7 +1046,7 @@ lExit: ; } FCIMPLEND -static void DirectObjectFieldSet(FieldDesc *pField, TypeHandle fieldType, TypeHandle enclosingType, TypedByRef *pTarget, OBJECTREF *pValue, CLR_BOOL *pDomainInitialized) { +static void DirectObjectFieldSet(FieldDesc *pField, TypeHandle fieldType, TypeHandle enclosingType, TypedByRef *pTarget, OBJECTREF *pValue, CLR_BOOL isInitialized) { CONTRACTL { THROWS; @@ -1067,7 +1066,7 @@ static void DirectObjectFieldSet(FieldDesc *pField, TypeHandle fieldType, TypeHa // Validate the target/fld type relationship InvokeUtil::ValidateObjectTarget(pField, enclosingType, &objref); - InvokeUtil::SetValidField(pField->GetFieldType(), fieldType, pField, &objref, pValue, enclosingType, pDomainInitialized); + InvokeUtil::SetValidField(pField->GetFieldType(), fieldType, pField, &objref, pValue, enclosingType, isInitialized); GCPROTECT_END(); } @@ -1114,9 +1113,8 @@ FCIMPL5(void, RuntimeFieldHandle::SetValueDirect, ReflectFieldObject *pFieldUNSA // Verify that the value passed can be widened into the target InvokeUtil::ValidField(fieldType, &gc.oValue); - CLR_BOOL domainInitialized = FALSE; if (pField->IsStatic() || !targetType.IsValueType()) { - DirectObjectFieldSet(pField, fieldType, TypeHandle(pEnclosingMT), pTarget, &gc.oValue, &domainInitialized); + DirectObjectFieldSet(pField, fieldType, TypeHandle(pEnclosingMT), pTarget, &gc.oValue, FALSE); goto lExit; } diff --git a/src/coreclr/vm/runtimehandles.h b/src/coreclr/vm/runtimehandles.h index 122224e27dc011..b2029bf14a18f5 100644 --- a/src/coreclr/vm/runtimehandles.h +++ b/src/coreclr/vm/runtimehandles.h @@ -292,8 +292,8 @@ extern "C" void QCALLTYPE RuntimeMethodHandle_Destroy(MethodDesc * pMethod); class RuntimeFieldHandle { public: - static FCDECL5(Object*, GetValue, ReflectFieldObject *pFieldUNSAFE, Object *instanceUNSAFE, ReflectClassBaseObject *pFieldType, ReflectClassBaseObject *pDeclaringType, CLR_BOOL *pDomainInitialized); - static FCDECL6(void, SetValue, ReflectFieldObject *pFieldUNSAFE, Object *targetUNSAFE, Object *valueUNSAFE, ReflectClassBaseObject *pFieldType, ReflectClassBaseObject *pDeclaringType, CLR_BOOL *pDomainInitialized); + static FCDECL5(Object*, GetValue, ReflectFieldObject *pFieldUNSAFE, Object *instanceUNSAFE, ReflectClassBaseObject *pFieldType, ReflectClassBaseObject *pDeclaringType, CLR_BOOL isInitialized); + static FCDECL6(void, SetValue, ReflectFieldObject *pFieldUNSAFE, Object *targetUNSAFE, Object *valueUNSAFE, ReflectClassBaseObject *pFieldType, ReflectClassBaseObject *pDeclaringType, CLR_BOOL isInitialized); static FCDECL4(Object*, GetValueDirect, ReflectFieldObject *pFieldUNSAFE, ReflectClassBaseObject *pFieldType, TypedByRef *pTarget, ReflectClassBaseObject *pDeclaringType); static FCDECL5(void, SetValueDirect, ReflectFieldObject *pFieldUNSAFE, ReflectClassBaseObject *pFieldType, TypedByRef *pTarget, Object *valueUNSAFE, ReflectClassBaseObject *pContextType); static FCDECL1(FC_BOOL_RET, IsFastPathSupported, ReflectFieldObject *pField); diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs index d95c55ae0c4da4..1afcac46fd6f16 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs @@ -116,8 +116,6 @@ private void Initialize() public object? GetValue(object? obj) { - bool domainInitialized; - unsafe { switch (_fieldAccessType) @@ -171,13 +169,8 @@ private void Initialize() VerifyTarget(obj); } - domainInitialized = false; - object? ret = RuntimeFieldHandle.GetValue(_fieldInfo, obj, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref domainInitialized); - if (domainInitialized) - { - Initialize(); - } - + object? ret = RuntimeFieldHandle.GetValue(_fieldInfo, obj, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, isInitialized: false); + Initialize(); return ret; case FieldAccessorType.SlowPath: @@ -186,8 +179,7 @@ private void Initialize() VerifyTarget(obj); } - domainInitialized = true; - return RuntimeFieldHandle.GetValue(_fieldInfo, obj, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref domainInitialized); + return RuntimeFieldHandle.GetValue(_fieldInfo, obj, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, isInitialized: true); case FieldAccessorType.NoInvoke: if (_fieldInfo.DeclaringType is not null && _fieldInfo.DeclaringType.ContainsGenericParameters) @@ -207,8 +199,6 @@ private void Initialize() public void SetValue(object? obj, object? value, BindingFlags invokeAttr, Binder? binder, CultureInfo? culture) { - bool domainInitialized; - unsafe { switch (_fieldAccessType) @@ -295,13 +285,8 @@ ref Unsafe.AsRef(_addressOrOffset.ToPointer()), VerifyInstanceField(obj, ref value, invokeAttr, binder, culture); } - domainInitialized = false; - RuntimeFieldHandle.SetValue(_fieldInfo, obj, value, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref domainInitialized); - if (domainInitialized) - { - Initialize(); - } - + RuntimeFieldHandle.SetValue(_fieldInfo, obj, value, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, isInitialized: false); + Initialize(); return; } @@ -323,8 +308,7 @@ ref Unsafe.AsRef(_addressOrOffset.ToPointer()), VerifyInstanceField(obj, ref value, invokeAttr, binder, culture); } - domainInitialized = true; - RuntimeFieldHandle.SetValue(_fieldInfo, obj, value, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref domainInitialized); + RuntimeFieldHandle.SetValue(_fieldInfo, obj, value, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, isInitialized: true); } private void InitializeClass() diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs index 33162acb597819..0a60801b5d3a99 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs @@ -97,8 +97,7 @@ internal override void CheckConsistency(object target) [DebuggerHidden] internal override void UnsafeSetValue(object? obj, object? value, BindingFlags invokeAttr, Binder? binder, CultureInfo? culture) { - bool domainInitialized = false; - RuntimeFieldHandle.SetValue(this, obj, value, null, Attributes, null, ref domainInitialized); + RuntimeFieldHandle.SetValue(this, obj, value, null, Attributes, null, isInitialized: false); } [DebuggerStepThrough] diff --git a/src/mono/System.Private.CoreLib/src/System/RuntimeFieldHandle.cs b/src/mono/System.Private.CoreLib/src/System/RuntimeFieldHandle.cs index b28bcb0cf7d916..94a0187a8930ed 100644 --- a/src/mono/System.Private.CoreLib/src/System/RuntimeFieldHandle.cs +++ b/src/mono/System.Private.CoreLib/src/System/RuntimeFieldHandle.cs @@ -72,7 +72,9 @@ public override int GetHashCode() [MethodImplAttribute(MethodImplOptions.InternalCall)] private static extern void SetValueInternal(FieldInfo fi, object? obj, object? value); - internal static void SetValue(RuntimeFieldInfo field, object? obj, object? value, RuntimeType? _, FieldAttributes _1, RuntimeType? _2, ref bool _3) +#pragma warning disable IDE0060 // Remove unused parameter (isInitialized) + internal static void SetValue(RuntimeFieldInfo field, object? obj, object? value, RuntimeType? _, FieldAttributes _1, RuntimeType? _2, bool isInitialized) +#pragma warning restore IDE0060 { SetValueInternal(field, obj, value); } From b92218791922778294d763d0f12ea5e6ad6937e7 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Mon, 12 Feb 2024 17:38:49 -0600 Subject: [PATCH 05/12] Add second call to test per feedback --- .../tests/System.Reflection.Tests/FieldInfoTests.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs b/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs index ff91da882d4ad5..aefc6c18a42e05 100644 --- a/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs +++ b/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs @@ -225,6 +225,10 @@ public void SetValueWithFunctionPointers(Type type, string name, object obj, obj { fieldInfo.SetValue(obj, value); Assert.Equal(expected, fieldInfo.GetValue(obj)); + + // Perform a second time to rule out cases of slow-path vs. fast-path. + fieldInfo.SetValue(obj, value); + Assert.Equal(expected, fieldInfo.GetValue(obj)); } finally { From 14e0b2d6e366348112ce1be550f86329850bab2e Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Tue, 13 Feb 2024 08:47:47 -0600 Subject: [PATCH 06/12] Revert "Change 'ref bool domainInitialized' to 'bool isInitialized'" This reverts commit a32cb6e6a1c356eb759b8059205d9c33d1f1db42. --- .../src/System/RuntimeHandles.cs | 4 +-- src/coreclr/vm/invokeutil.cpp | 17 +++++++---- src/coreclr/vm/invokeutil.h | 4 +-- src/coreclr/vm/reflectioninvocation.cpp | 22 ++++++++------- src/coreclr/vm/runtimehandles.h | 4 +-- .../src/System/Reflection/FieldAccessor.cs | 28 +++++++++++++++---- .../src/System/Reflection/RuntimeFieldInfo.cs | 3 +- .../src/System/RuntimeFieldHandle.cs | 4 +-- 8 files changed, 54 insertions(+), 32 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs b/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs index a7404d9dd8328e..21228998515ac4 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs @@ -1203,13 +1203,13 @@ internal static RuntimeType GetApproxDeclaringType(IRuntimeFieldInfo field) internal static extern int GetToken(RtFieldInfo field); [MethodImpl(MethodImplOptions.InternalCall)] - internal static extern object? GetValue(RtFieldInfo field, object? instance, RuntimeType fieldType, RuntimeType? declaringType, bool isInitialized); + internal static extern object? GetValue(RtFieldInfo field, object? instance, RuntimeType fieldType, RuntimeType? declaringType, ref bool domainInitialized); [MethodImpl(MethodImplOptions.InternalCall)] internal static extern object? GetValueDirect(RtFieldInfo field, RuntimeType fieldType, void* pTypedRef, RuntimeType? contextType); [MethodImpl(MethodImplOptions.InternalCall)] - internal static extern void SetValue(RtFieldInfo field, object? obj, object? value, RuntimeType fieldType, RuntimeType? declaringType, bool isInitialized); + internal static extern void SetValue(RtFieldInfo field, object? obj, object? value, RuntimeType fieldType, RuntimeType? declaringType, ref bool domainInitialized); [MethodImpl(MethodImplOptions.InternalCall)] internal static extern void SetValueDirect(RtFieldInfo field, RuntimeType fieldType, void* pTypedRef, object? value, RuntimeType? contextType); diff --git a/src/coreclr/vm/invokeutil.cpp b/src/coreclr/vm/invokeutil.cpp index 07b3fcfdedbafe..0d7994a330ca75 100644 --- a/src/coreclr/vm/invokeutil.cpp +++ b/src/coreclr/vm/invokeutil.cpp @@ -748,7 +748,7 @@ void InvokeUtil::SetValidField(CorElementType fldType, OBJECTREF *target, OBJECTREF *valueObj, TypeHandle declaringType, - CLR_BOOL isInitialized) { + CLR_BOOL *pDomainInitialized) { CONTRACTL { THROWS; GC_TRIGGERS; @@ -786,17 +786,19 @@ void InvokeUtil::SetValidField(CorElementType fldType, pDeclMT = pField->GetModule()->GetGlobalMethodTable(); } - if (isInitialized == FALSE) + if (*pDomainInitialized == FALSE) { EX_TRY { pDeclMT->EnsureInstanceActive(); pDeclMT->CheckRunClassInitThrowing(); + + *pDomainInitialized = TRUE; } EX_CATCH_THROWABLE(&Throwable); } #ifdef _DEBUG - else if (isInitialized == TRUE && !declaringType.IsNull()) + else if (*pDomainInitialized == TRUE && !declaringType.IsNull()) CONSISTENCY_CHECK(declaringType.GetMethodTable()->CheckActivated()); #endif @@ -971,7 +973,7 @@ void InvokeUtil::SetValidField(CorElementType fldType, // GetFieldValue // This method will return an ARG_SLOT containing the value of the field. -OBJECTREF InvokeUtil::GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJECTREF* target, TypeHandle declaringType, CLR_BOOL isInitialized) { +OBJECTREF InvokeUtil::GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJECTREF* target, TypeHandle declaringType, CLR_BOOL *pDomainInitialized) { CONTRACTL { THROWS; GC_TRIGGERS; @@ -1009,20 +1011,23 @@ OBJECTREF InvokeUtil::GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJ pDeclMT = pField->GetModule()->GetGlobalMethodTable(); } - if (isInitialized == FALSE) + if (*pDomainInitialized == FALSE) { EX_TRY { pDeclMT->EnsureInstanceActive(); pDeclMT->CheckRunClassInitThrowing(); + + *pDomainInitialized = TRUE; } EX_CATCH_THROWABLE(&Throwable); } #ifdef _DEBUG - else if (isInitialized == TRUE && !declaringType.IsNull()) + else if (*pDomainInitialized == TRUE && !declaringType.IsNull()) CONSISTENCY_CHECK(declaringType.GetMethodTable()->CheckActivated()); #endif + if(Throwable != NULL) { GCPROTECT_BEGIN(Throwable); diff --git a/src/coreclr/vm/invokeutil.h b/src/coreclr/vm/invokeutil.h index aaa9bd0a88cf3b..0bd1577c7a19d3 100644 --- a/src/coreclr/vm/invokeutil.h +++ b/src/coreclr/vm/invokeutil.h @@ -138,9 +138,9 @@ class InvokeUtil // SetValidField // Given an target object, a value object and a field this method will set the field // on the target object. The field must be validate before calling this. - static void SetValidField(CorElementType fldType, TypeHandle fldTH, FieldDesc* pField, OBJECTREF* target, OBJECTREF* value, TypeHandle declaringType, CLR_BOOL isInitialized); + static void SetValidField(CorElementType fldType, TypeHandle fldTH, FieldDesc* pField, OBJECTREF* target, OBJECTREF* value, TypeHandle declaringType, CLR_BOOL *pDomainInitialized); - static OBJECTREF GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJECTREF* target, TypeHandle declaringType, CLR_BOOL isInitialized); + static OBJECTREF GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJECTREF* target, TypeHandle declaringType, CLR_BOOL *pDomainInitialized); // ValidateObjectTarget // This method will validate the Object/Target relationship diff --git a/src/coreclr/vm/reflectioninvocation.cpp b/src/coreclr/vm/reflectioninvocation.cpp index f03fa190720a51..b7680eb4581ef0 100644 --- a/src/coreclr/vm/reflectioninvocation.cpp +++ b/src/coreclr/vm/reflectioninvocation.cpp @@ -25,7 +25,7 @@ #include "dbginterface.h" #include "argdestination.h" -FCIMPL5(Object*, RuntimeFieldHandle::GetValue, ReflectFieldObject *pFieldUNSAFE, Object *instanceUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE, ReflectClassBaseObject *pDeclaringTypeUNSAFE, CLR_BOOL isInitialized) { +FCIMPL5(Object*, RuntimeFieldHandle::GetValue, ReflectFieldObject *pFieldUNSAFE, Object *instanceUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE, ReflectClassBaseObject *pDeclaringTypeUNSAFE, CLR_BOOL *pDomainInitialized) { CONTRACTL { FCALL_CHECK; @@ -55,7 +55,7 @@ FCIMPL5(Object*, RuntimeFieldHandle::GetValue, ReflectFieldObject *pFieldUNSAFE, HELPER_METHOD_FRAME_BEGIN_RET_PROTECT(gc); // There can be no GC after this until the Object is returned. - rv = InvokeUtil::GetFieldValue(gc.refField->GetField(), fieldType, &gc.target, declaringType, isInitialized); + rv = InvokeUtil::GetFieldValue(gc.refField->GetField(), fieldType, &gc.target, declaringType, pDomainInitialized); HELPER_METHOD_FRAME_END(); return OBJECTREFToObject(rv); @@ -161,7 +161,7 @@ FCIMPL2(Object*, ReflectionInvocation::AllocateValueType, ReflectClassBaseObject } FCIMPLEND -FCIMPL6(void, RuntimeFieldHandle::SetValue, ReflectFieldObject *pFieldUNSAFE, Object *targetUNSAFE, Object *valueUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE, ReflectClassBaseObject *pDeclaringTypeUNSAFE, CLR_BOOL isInitialized) { +FCIMPL6(void, RuntimeFieldHandle::SetValue, ReflectFieldObject *pFieldUNSAFE, Object *targetUNSAFE, Object *valueUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE, ReflectClassBaseObject *pDeclaringTypeUNSAFE, CLR_BOOL *pDomainInitialized) { CONTRACTL { FCALL_CHECK; @@ -194,7 +194,7 @@ FCIMPL6(void, RuntimeFieldHandle::SetValue, ReflectFieldObject *pFieldUNSAFE, Ob HELPER_METHOD_FRAME_BEGIN_PROTECT(gc); - InvokeUtil::SetValidField(fieldType.GetVerifierCorElementType(), fieldType, pFieldDesc, &gc.target, &gc.value, declaringType, isInitialized); + InvokeUtil::SetValidField(fieldType.GetVerifierCorElementType(), fieldType, pFieldDesc, &gc.target, &gc.value, declaringType, pDomainInitialized); HELPER_METHOD_FRAME_END(); } @@ -922,7 +922,7 @@ FCIMPL1(ReflectMethodObject*, RuntimeMethodHandle::GetCurrentMethod, StackCrawlM } FCIMPLEND -static OBJECTREF DirectObjectFieldGet(FieldDesc *pField, TypeHandle fieldType, TypeHandle enclosingType, TypedByRef *pTarget, CLR_BOOL isInitialized) { +static OBJECTREF DirectObjectFieldGet(FieldDesc *pField, TypeHandle fieldType, TypeHandle enclosingType, TypedByRef *pTarget, CLR_BOOL *pDomainInitialized) { CONTRACTL { THROWS; @@ -941,7 +941,7 @@ static OBJECTREF DirectObjectFieldGet(FieldDesc *pField, TypeHandle fieldType, T } InvokeUtil::ValidateObjectTarget(pField, enclosingType, &objref); - refRet = InvokeUtil::GetFieldValue(pField, fieldType, &objref, enclosingType, isInitialized); + refRet = InvokeUtil::GetFieldValue(pField, fieldType, &objref, enclosingType, pDomainInitialized); GCPROTECT_END(); return refRet; } @@ -982,8 +982,9 @@ FCIMPL4(Object*, RuntimeFieldHandle::GetValueDirect, ReflectFieldObject *pFieldU _ASSERTE(gc.refDeclaringType == NULL || !gc.refDeclaringType->GetType().IsTypeDesc()); MethodTable *pEnclosingMT = (gc.refDeclaringType != NULL ? gc.refDeclaringType->GetType() : TypeHandle()).AsMethodTable(); + CLR_BOOL domainInitialized = FALSE; if (pField->IsStatic() || !targetType.IsValueType()) { - refRet = DirectObjectFieldGet(pField, fieldType, TypeHandle(pEnclosingMT), pTarget, FALSE); + refRet = DirectObjectFieldGet(pField, fieldType, TypeHandle(pEnclosingMT), pTarget, &domainInitialized); goto lExit; } @@ -1046,7 +1047,7 @@ lExit: ; } FCIMPLEND -static void DirectObjectFieldSet(FieldDesc *pField, TypeHandle fieldType, TypeHandle enclosingType, TypedByRef *pTarget, OBJECTREF *pValue, CLR_BOOL isInitialized) { +static void DirectObjectFieldSet(FieldDesc *pField, TypeHandle fieldType, TypeHandle enclosingType, TypedByRef *pTarget, OBJECTREF *pValue, CLR_BOOL *pDomainInitialized) { CONTRACTL { THROWS; @@ -1066,7 +1067,7 @@ static void DirectObjectFieldSet(FieldDesc *pField, TypeHandle fieldType, TypeHa // Validate the target/fld type relationship InvokeUtil::ValidateObjectTarget(pField, enclosingType, &objref); - InvokeUtil::SetValidField(pField->GetFieldType(), fieldType, pField, &objref, pValue, enclosingType, isInitialized); + InvokeUtil::SetValidField(pField->GetFieldType(), fieldType, pField, &objref, pValue, enclosingType, pDomainInitialized); GCPROTECT_END(); } @@ -1113,8 +1114,9 @@ FCIMPL5(void, RuntimeFieldHandle::SetValueDirect, ReflectFieldObject *pFieldUNSA // Verify that the value passed can be widened into the target InvokeUtil::ValidField(fieldType, &gc.oValue); + CLR_BOOL domainInitialized = FALSE; if (pField->IsStatic() || !targetType.IsValueType()) { - DirectObjectFieldSet(pField, fieldType, TypeHandle(pEnclosingMT), pTarget, &gc.oValue, FALSE); + DirectObjectFieldSet(pField, fieldType, TypeHandle(pEnclosingMT), pTarget, &gc.oValue, &domainInitialized); goto lExit; } diff --git a/src/coreclr/vm/runtimehandles.h b/src/coreclr/vm/runtimehandles.h index b2029bf14a18f5..122224e27dc011 100644 --- a/src/coreclr/vm/runtimehandles.h +++ b/src/coreclr/vm/runtimehandles.h @@ -292,8 +292,8 @@ extern "C" void QCALLTYPE RuntimeMethodHandle_Destroy(MethodDesc * pMethod); class RuntimeFieldHandle { public: - static FCDECL5(Object*, GetValue, ReflectFieldObject *pFieldUNSAFE, Object *instanceUNSAFE, ReflectClassBaseObject *pFieldType, ReflectClassBaseObject *pDeclaringType, CLR_BOOL isInitialized); - static FCDECL6(void, SetValue, ReflectFieldObject *pFieldUNSAFE, Object *targetUNSAFE, Object *valueUNSAFE, ReflectClassBaseObject *pFieldType, ReflectClassBaseObject *pDeclaringType, CLR_BOOL isInitialized); + static FCDECL5(Object*, GetValue, ReflectFieldObject *pFieldUNSAFE, Object *instanceUNSAFE, ReflectClassBaseObject *pFieldType, ReflectClassBaseObject *pDeclaringType, CLR_BOOL *pDomainInitialized); + static FCDECL6(void, SetValue, ReflectFieldObject *pFieldUNSAFE, Object *targetUNSAFE, Object *valueUNSAFE, ReflectClassBaseObject *pFieldType, ReflectClassBaseObject *pDeclaringType, CLR_BOOL *pDomainInitialized); static FCDECL4(Object*, GetValueDirect, ReflectFieldObject *pFieldUNSAFE, ReflectClassBaseObject *pFieldType, TypedByRef *pTarget, ReflectClassBaseObject *pDeclaringType); static FCDECL5(void, SetValueDirect, ReflectFieldObject *pFieldUNSAFE, ReflectClassBaseObject *pFieldType, TypedByRef *pTarget, Object *valueUNSAFE, ReflectClassBaseObject *pContextType); static FCDECL1(FC_BOOL_RET, IsFastPathSupported, ReflectFieldObject *pField); diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs index 1afcac46fd6f16..d95c55ae0c4da4 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs @@ -116,6 +116,8 @@ private void Initialize() public object? GetValue(object? obj) { + bool domainInitialized; + unsafe { switch (_fieldAccessType) @@ -169,8 +171,13 @@ private void Initialize() VerifyTarget(obj); } - object? ret = RuntimeFieldHandle.GetValue(_fieldInfo, obj, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, isInitialized: false); - Initialize(); + domainInitialized = false; + object? ret = RuntimeFieldHandle.GetValue(_fieldInfo, obj, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref domainInitialized); + if (domainInitialized) + { + Initialize(); + } + return ret; case FieldAccessorType.SlowPath: @@ -179,7 +186,8 @@ private void Initialize() VerifyTarget(obj); } - return RuntimeFieldHandle.GetValue(_fieldInfo, obj, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, isInitialized: true); + domainInitialized = true; + return RuntimeFieldHandle.GetValue(_fieldInfo, obj, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref domainInitialized); case FieldAccessorType.NoInvoke: if (_fieldInfo.DeclaringType is not null && _fieldInfo.DeclaringType.ContainsGenericParameters) @@ -199,6 +207,8 @@ private void Initialize() public void SetValue(object? obj, object? value, BindingFlags invokeAttr, Binder? binder, CultureInfo? culture) { + bool domainInitialized; + unsafe { switch (_fieldAccessType) @@ -285,8 +295,13 @@ ref Unsafe.AsRef(_addressOrOffset.ToPointer()), VerifyInstanceField(obj, ref value, invokeAttr, binder, culture); } - RuntimeFieldHandle.SetValue(_fieldInfo, obj, value, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, isInitialized: false); - Initialize(); + domainInitialized = false; + RuntimeFieldHandle.SetValue(_fieldInfo, obj, value, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref domainInitialized); + if (domainInitialized) + { + Initialize(); + } + return; } @@ -308,7 +323,8 @@ ref Unsafe.AsRef(_addressOrOffset.ToPointer()), VerifyInstanceField(obj, ref value, invokeAttr, binder, culture); } - RuntimeFieldHandle.SetValue(_fieldInfo, obj, value, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, isInitialized: true); + domainInitialized = true; + RuntimeFieldHandle.SetValue(_fieldInfo, obj, value, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref domainInitialized); } private void InitializeClass() diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs index 0a60801b5d3a99..33162acb597819 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs @@ -97,7 +97,8 @@ internal override void CheckConsistency(object target) [DebuggerHidden] internal override void UnsafeSetValue(object? obj, object? value, BindingFlags invokeAttr, Binder? binder, CultureInfo? culture) { - RuntimeFieldHandle.SetValue(this, obj, value, null, Attributes, null, isInitialized: false); + bool domainInitialized = false; + RuntimeFieldHandle.SetValue(this, obj, value, null, Attributes, null, ref domainInitialized); } [DebuggerStepThrough] diff --git a/src/mono/System.Private.CoreLib/src/System/RuntimeFieldHandle.cs b/src/mono/System.Private.CoreLib/src/System/RuntimeFieldHandle.cs index 94a0187a8930ed..b28bcb0cf7d916 100644 --- a/src/mono/System.Private.CoreLib/src/System/RuntimeFieldHandle.cs +++ b/src/mono/System.Private.CoreLib/src/System/RuntimeFieldHandle.cs @@ -72,9 +72,7 @@ public override int GetHashCode() [MethodImplAttribute(MethodImplOptions.InternalCall)] private static extern void SetValueInternal(FieldInfo fi, object? obj, object? value); -#pragma warning disable IDE0060 // Remove unused parameter (isInitialized) - internal static void SetValue(RuntimeFieldInfo field, object? obj, object? value, RuntimeType? _, FieldAttributes _1, RuntimeType? _2, bool isInitialized) -#pragma warning restore IDE0060 + internal static void SetValue(RuntimeFieldInfo field, object? obj, object? value, RuntimeType? _, FieldAttributes _1, RuntimeType? _2, ref bool _3) { SetValueInternal(field, obj, value); } From 694bd5fa1ee9ad71a85dcddc548c1e68c7b549b2 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Tue, 13 Feb 2024 11:25:43 -0600 Subject: [PATCH 07/12] Don't move to fast path until cctor is finished --- .../src/System/RuntimeHandles.cs | 4 +- src/coreclr/vm/field.h | 4 +- src/coreclr/vm/invokeutil.cpp | 21 +++----- src/coreclr/vm/invokeutil.h | 4 +- src/coreclr/vm/methodtable.cpp | 14 +++-- src/coreclr/vm/methodtable.h | 2 +- src/coreclr/vm/reflectioninvocation.cpp | 24 ++++----- src/coreclr/vm/runtimehandles.h | 4 +- .../src/System/Reflection/FieldAccessor.cs | 39 ++++++++------ .../System.Reflection.Tests/FieldInfoTests.cs | 53 +++++++++++++++++++ .../src/System/Reflection/RuntimeFieldInfo.cs | 4 +- 11 files changed, 116 insertions(+), 57 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs b/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs index 21228998515ac4..244312cf59809c 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs @@ -1203,13 +1203,13 @@ internal static RuntimeType GetApproxDeclaringType(IRuntimeFieldInfo field) internal static extern int GetToken(RtFieldInfo field); [MethodImpl(MethodImplOptions.InternalCall)] - internal static extern object? GetValue(RtFieldInfo field, object? instance, RuntimeType fieldType, RuntimeType? declaringType, ref bool domainInitialized); + internal static extern object? GetValue(RtFieldInfo field, object? instance, RuntimeType fieldType, RuntimeType? declaringType, ref bool isClassInitialized); [MethodImpl(MethodImplOptions.InternalCall)] internal static extern object? GetValueDirect(RtFieldInfo field, RuntimeType fieldType, void* pTypedRef, RuntimeType? contextType); [MethodImpl(MethodImplOptions.InternalCall)] - internal static extern void SetValue(RtFieldInfo field, object? obj, object? value, RuntimeType fieldType, RuntimeType? declaringType, ref bool domainInitialized); + internal static extern void SetValue(RtFieldInfo field, object? obj, object? value, RuntimeType fieldType, RuntimeType? declaringType, ref bool isClassInitialized); [MethodImpl(MethodImplOptions.InternalCall)] internal static extern void SetValueDirect(RtFieldInfo field, RuntimeType fieldType, void* pTypedRef, object? value, RuntimeType? contextType); diff --git a/src/coreclr/vm/field.h b/src/coreclr/vm/field.h index 0d2efde5b5bf65..bcaafd427db697 100644 --- a/src/coreclr/vm/field.h +++ b/src/coreclr/vm/field.h @@ -526,7 +526,7 @@ class FieldDesc } } - VOID CheckRunClassInitThrowing() + BOOL CheckRunClassInitThrowing() { CONTRACTL { @@ -536,7 +536,7 @@ class FieldDesc } CONTRACTL_END; - GetEnclosingMethodTable()->CheckRunClassInitThrowing(); + return GetEnclosingMethodTable()->CheckRunClassInitThrowing(); } #endif //DACCESS_COMPILE diff --git a/src/coreclr/vm/invokeutil.cpp b/src/coreclr/vm/invokeutil.cpp index 0d7994a330ca75..5c529f7766d5f4 100644 --- a/src/coreclr/vm/invokeutil.cpp +++ b/src/coreclr/vm/invokeutil.cpp @@ -748,7 +748,7 @@ void InvokeUtil::SetValidField(CorElementType fldType, OBJECTREF *target, OBJECTREF *valueObj, TypeHandle declaringType, - CLR_BOOL *pDomainInitialized) { + CLR_BOOL *pIsClassInitialized) { CONTRACTL { THROWS; GC_TRIGGERS; @@ -786,19 +786,17 @@ void InvokeUtil::SetValidField(CorElementType fldType, pDeclMT = pField->GetModule()->GetGlobalMethodTable(); } - if (*pDomainInitialized == FALSE) + if (*pIsClassInitialized == FALSE) { EX_TRY { pDeclMT->EnsureInstanceActive(); - pDeclMT->CheckRunClassInitThrowing(); - - *pDomainInitialized = TRUE; + *pIsClassInitialized = pDeclMT->CheckRunClassInitThrowing(); } EX_CATCH_THROWABLE(&Throwable); } #ifdef _DEBUG - else if (*pDomainInitialized == TRUE && !declaringType.IsNull()) + else if (*pIsClassInitialized == TRUE && !declaringType.IsNull()) CONSISTENCY_CHECK(declaringType.GetMethodTable()->CheckActivated()); #endif @@ -973,7 +971,7 @@ void InvokeUtil::SetValidField(CorElementType fldType, // GetFieldValue // This method will return an ARG_SLOT containing the value of the field. -OBJECTREF InvokeUtil::GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJECTREF* target, TypeHandle declaringType, CLR_BOOL *pDomainInitialized) { +OBJECTREF InvokeUtil::GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJECTREF* target, TypeHandle declaringType, CLR_BOOL *pIsClassInitialized) { CONTRACTL { THROWS; GC_TRIGGERS; @@ -1011,23 +1009,20 @@ OBJECTREF InvokeUtil::GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJ pDeclMT = pField->GetModule()->GetGlobalMethodTable(); } - if (*pDomainInitialized == FALSE) + if (*pIsClassInitialized == FALSE) { EX_TRY { pDeclMT->EnsureInstanceActive(); - pDeclMT->CheckRunClassInitThrowing(); - - *pDomainInitialized = TRUE; + *pIsClassInitialized = pDeclMT->CheckRunClassInitThrowing(); } EX_CATCH_THROWABLE(&Throwable); } #ifdef _DEBUG - else if (*pDomainInitialized == TRUE && !declaringType.IsNull()) + else if (*pIsClassInitialized == TRUE && !declaringType.IsNull()) CONSISTENCY_CHECK(declaringType.GetMethodTable()->CheckActivated()); #endif - if(Throwable != NULL) { GCPROTECT_BEGIN(Throwable); diff --git a/src/coreclr/vm/invokeutil.h b/src/coreclr/vm/invokeutil.h index 0bd1577c7a19d3..b288c475aae07e 100644 --- a/src/coreclr/vm/invokeutil.h +++ b/src/coreclr/vm/invokeutil.h @@ -138,9 +138,9 @@ class InvokeUtil // SetValidField // Given an target object, a value object and a field this method will set the field // on the target object. The field must be validate before calling this. - static void SetValidField(CorElementType fldType, TypeHandle fldTH, FieldDesc* pField, OBJECTREF* target, OBJECTREF* value, TypeHandle declaringType, CLR_BOOL *pDomainInitialized); + static void SetValidField(CorElementType fldType, TypeHandle fldTH, FieldDesc* pField, OBJECTREF* target, OBJECTREF* value, TypeHandle declaringType, CLR_BOOL *pIsClassInitialized); - static OBJECTREF GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJECTREF* target, TypeHandle declaringType, CLR_BOOL *pDomainInitialized); + static OBJECTREF GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJECTREF* target, TypeHandle declaringType, CLR_BOOL *pIsClassInitialized); // ValidateObjectTarget // This method will validate the Object/Target relationship diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index 7d16ed26fbbf05..0ac3a566752ee7 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -4487,7 +4487,8 @@ void MethodTable::DoRunClassInitThrowing() } //========================================================================================== -void MethodTable::CheckRunClassInitThrowing() +// Returns true if the class is initialized. +BOOL MethodTable::CheckRunClassInitThrowing() { CONTRACTL { @@ -4506,11 +4507,11 @@ void MethodTable::CheckRunClassInitThrowing() TRIGGERSGC(); if (IsClassPreInited()) - return; + return TRUE; // Don't initialize shared generic instantiations (e.g. MyClass<__Canon>) if (IsSharedByGenericInstantiations()) - return; + return TRUE; DomainLocalModule *pLocalModule = GetDomainLocalModule(); _ASSERTE(pLocalModule); @@ -4521,8 +4522,11 @@ void MethodTable::CheckRunClassInitThrowing() if (!pLocalModule->IsClassAllocated(this, iClassIndex)) pLocalModule->PopulateClass(this); - if (!pLocalModule->IsClassInitialized(this, iClassIndex)) - DoRunClassInitThrowing(); + if (pLocalModule->IsClassInitialized(this, iClassIndex)) + return TRUE; + + DoRunClassInitThrowing(); + return pLocalModule->IsClassInitialized(this, iClassIndex); } //========================================================================================== diff --git a/src/coreclr/vm/methodtable.h b/src/coreclr/vm/methodtable.h index 4a7cfb2232061b..9262ca51fd8151 100644 --- a/src/coreclr/vm/methodtable.h +++ b/src/coreclr/vm/methodtable.h @@ -813,7 +813,7 @@ class MethodTable public: // checks whether the class initialiser should be run on this class, and runs it if necessary - void CheckRunClassInitThrowing(); + BOOL CheckRunClassInitThrowing(); // checks whether or not the non-beforefieldinit class initializers have been run for all types in this type's // inheritance hierarchy, and runs them if necessary. This simulates the behavior of running class constructors diff --git a/src/coreclr/vm/reflectioninvocation.cpp b/src/coreclr/vm/reflectioninvocation.cpp index b7680eb4581ef0..c4390aff2fdc19 100644 --- a/src/coreclr/vm/reflectioninvocation.cpp +++ b/src/coreclr/vm/reflectioninvocation.cpp @@ -25,7 +25,7 @@ #include "dbginterface.h" #include "argdestination.h" -FCIMPL5(Object*, RuntimeFieldHandle::GetValue, ReflectFieldObject *pFieldUNSAFE, Object *instanceUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE, ReflectClassBaseObject *pDeclaringTypeUNSAFE, CLR_BOOL *pDomainInitialized) { +FCIMPL5(Object*, RuntimeFieldHandle::GetValue, ReflectFieldObject *pFieldUNSAFE, Object *instanceUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE, ReflectClassBaseObject *pDeclaringTypeUNSAFE, CLR_BOOL *pIsClassInitialized) { CONTRACTL { FCALL_CHECK; @@ -55,7 +55,7 @@ FCIMPL5(Object*, RuntimeFieldHandle::GetValue, ReflectFieldObject *pFieldUNSAFE, HELPER_METHOD_FRAME_BEGIN_RET_PROTECT(gc); // There can be no GC after this until the Object is returned. - rv = InvokeUtil::GetFieldValue(gc.refField->GetField(), fieldType, &gc.target, declaringType, pDomainInitialized); + rv = InvokeUtil::GetFieldValue(gc.refField->GetField(), fieldType, &gc.target, declaringType, pIsClassInitialized); HELPER_METHOD_FRAME_END(); return OBJECTREFToObject(rv); @@ -161,7 +161,7 @@ FCIMPL2(Object*, ReflectionInvocation::AllocateValueType, ReflectClassBaseObject } FCIMPLEND -FCIMPL6(void, RuntimeFieldHandle::SetValue, ReflectFieldObject *pFieldUNSAFE, Object *targetUNSAFE, Object *valueUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE, ReflectClassBaseObject *pDeclaringTypeUNSAFE, CLR_BOOL *pDomainInitialized) { +FCIMPL6(void, RuntimeFieldHandle::SetValue, ReflectFieldObject *pFieldUNSAFE, Object *targetUNSAFE, Object *valueUNSAFE, ReflectClassBaseObject *pFieldTypeUNSAFE, ReflectClassBaseObject *pDeclaringTypeUNSAFE, CLR_BOOL *pIsClassInitialized) { CONTRACTL { FCALL_CHECK; @@ -194,7 +194,7 @@ FCIMPL6(void, RuntimeFieldHandle::SetValue, ReflectFieldObject *pFieldUNSAFE, Ob HELPER_METHOD_FRAME_BEGIN_PROTECT(gc); - InvokeUtil::SetValidField(fieldType.GetVerifierCorElementType(), fieldType, pFieldDesc, &gc.target, &gc.value, declaringType, pDomainInitialized); + InvokeUtil::SetValidField(fieldType.GetVerifierCorElementType(), fieldType, pFieldDesc, &gc.target, &gc.value, declaringType, pIsClassInitialized); HELPER_METHOD_FRAME_END(); } @@ -922,7 +922,7 @@ FCIMPL1(ReflectMethodObject*, RuntimeMethodHandle::GetCurrentMethod, StackCrawlM } FCIMPLEND -static OBJECTREF DirectObjectFieldGet(FieldDesc *pField, TypeHandle fieldType, TypeHandle enclosingType, TypedByRef *pTarget, CLR_BOOL *pDomainInitialized) { +static OBJECTREF DirectObjectFieldGet(FieldDesc *pField, TypeHandle fieldType, TypeHandle enclosingType, TypedByRef *pTarget, CLR_BOOL *pIsClassInitialized) { CONTRACTL { THROWS; @@ -941,7 +941,7 @@ static OBJECTREF DirectObjectFieldGet(FieldDesc *pField, TypeHandle fieldType, T } InvokeUtil::ValidateObjectTarget(pField, enclosingType, &objref); - refRet = InvokeUtil::GetFieldValue(pField, fieldType, &objref, enclosingType, pDomainInitialized); + refRet = InvokeUtil::GetFieldValue(pField, fieldType, &objref, enclosingType, pIsClassInitialized); GCPROTECT_END(); return refRet; } @@ -982,9 +982,9 @@ FCIMPL4(Object*, RuntimeFieldHandle::GetValueDirect, ReflectFieldObject *pFieldU _ASSERTE(gc.refDeclaringType == NULL || !gc.refDeclaringType->GetType().IsTypeDesc()); MethodTable *pEnclosingMT = (gc.refDeclaringType != NULL ? gc.refDeclaringType->GetType() : TypeHandle()).AsMethodTable(); - CLR_BOOL domainInitialized = FALSE; + CLR_BOOL isClassInitialized = FALSE; if (pField->IsStatic() || !targetType.IsValueType()) { - refRet = DirectObjectFieldGet(pField, fieldType, TypeHandle(pEnclosingMT), pTarget, &domainInitialized); + refRet = DirectObjectFieldGet(pField, fieldType, TypeHandle(pEnclosingMT), pTarget, &isClassInitialized); goto lExit; } @@ -1047,7 +1047,7 @@ lExit: ; } FCIMPLEND -static void DirectObjectFieldSet(FieldDesc *pField, TypeHandle fieldType, TypeHandle enclosingType, TypedByRef *pTarget, OBJECTREF *pValue, CLR_BOOL *pDomainInitialized) { +static void DirectObjectFieldSet(FieldDesc *pField, TypeHandle fieldType, TypeHandle enclosingType, TypedByRef *pTarget, OBJECTREF *pValue, CLR_BOOL *pIsClassInitialized) { CONTRACTL { THROWS; @@ -1067,7 +1067,7 @@ static void DirectObjectFieldSet(FieldDesc *pField, TypeHandle fieldType, TypeHa // Validate the target/fld type relationship InvokeUtil::ValidateObjectTarget(pField, enclosingType, &objref); - InvokeUtil::SetValidField(pField->GetFieldType(), fieldType, pField, &objref, pValue, enclosingType, pDomainInitialized); + InvokeUtil::SetValidField(pField->GetFieldType(), fieldType, pField, &objref, pValue, enclosingType, pIsClassInitialized); GCPROTECT_END(); } @@ -1114,9 +1114,9 @@ FCIMPL5(void, RuntimeFieldHandle::SetValueDirect, ReflectFieldObject *pFieldUNSA // Verify that the value passed can be widened into the target InvokeUtil::ValidField(fieldType, &gc.oValue); - CLR_BOOL domainInitialized = FALSE; + CLR_BOOL isClassInitialized = FALSE; if (pField->IsStatic() || !targetType.IsValueType()) { - DirectObjectFieldSet(pField, fieldType, TypeHandle(pEnclosingMT), pTarget, &gc.oValue, &domainInitialized); + DirectObjectFieldSet(pField, fieldType, TypeHandle(pEnclosingMT), pTarget, &gc.oValue, &isClassInitialized); goto lExit; } diff --git a/src/coreclr/vm/runtimehandles.h b/src/coreclr/vm/runtimehandles.h index 122224e27dc011..ec7864ad2bc499 100644 --- a/src/coreclr/vm/runtimehandles.h +++ b/src/coreclr/vm/runtimehandles.h @@ -292,8 +292,8 @@ extern "C" void QCALLTYPE RuntimeMethodHandle_Destroy(MethodDesc * pMethod); class RuntimeFieldHandle { public: - static FCDECL5(Object*, GetValue, ReflectFieldObject *pFieldUNSAFE, Object *instanceUNSAFE, ReflectClassBaseObject *pFieldType, ReflectClassBaseObject *pDeclaringType, CLR_BOOL *pDomainInitialized); - static FCDECL6(void, SetValue, ReflectFieldObject *pFieldUNSAFE, Object *targetUNSAFE, Object *valueUNSAFE, ReflectClassBaseObject *pFieldType, ReflectClassBaseObject *pDeclaringType, CLR_BOOL *pDomainInitialized); + static FCDECL5(Object*, GetValue, ReflectFieldObject *pFieldUNSAFE, Object *instanceUNSAFE, ReflectClassBaseObject *pFieldType, ReflectClassBaseObject *pDeclaringType, CLR_BOOL *pIsClassInitialized); + static FCDECL6(void, SetValue, ReflectFieldObject *pFieldUNSAFE, Object *targetUNSAFE, Object *valueUNSAFE, ReflectClassBaseObject *pFieldType, ReflectClassBaseObject *pDeclaringType, CLR_BOOL *pIsClassInitialized); static FCDECL4(Object*, GetValueDirect, ReflectFieldObject *pFieldUNSAFE, ReflectClassBaseObject *pFieldType, TypedByRef *pTarget, ReflectClassBaseObject *pDeclaringType); static FCDECL5(void, SetValueDirect, ReflectFieldObject *pFieldUNSAFE, ReflectClassBaseObject *pFieldType, TypedByRef *pTarget, Object *valueUNSAFE, ReflectClassBaseObject *pContextType); static FCDECL1(FC_BOOL_RET, IsFastPathSupported, ReflectFieldObject *pField); diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs index d95c55ae0c4da4..5b2b9497d330b8 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs @@ -116,7 +116,7 @@ private void Initialize() public object? GetValue(object? obj) { - bool domainInitialized; + bool isClassInitialized; unsafe { @@ -171,9 +171,9 @@ private void Initialize() VerifyTarget(obj); } - domainInitialized = false; - object? ret = RuntimeFieldHandle.GetValue(_fieldInfo, obj, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref domainInitialized); - if (domainInitialized) + isClassInitialized = false; + object? ret = RuntimeFieldHandle.GetValue(_fieldInfo, obj, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref isClassInitialized); + if (isClassInitialized) { Initialize(); } @@ -186,8 +186,8 @@ private void Initialize() VerifyTarget(obj); } - domainInitialized = true; - return RuntimeFieldHandle.GetValue(_fieldInfo, obj, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref domainInitialized); + isClassInitialized = true; + return RuntimeFieldHandle.GetValue(_fieldInfo, obj, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref isClassInitialized); case FieldAccessorType.NoInvoke: if (_fieldInfo.DeclaringType is not null && _fieldInfo.DeclaringType.ContainsGenericParameters) @@ -207,7 +207,7 @@ private void Initialize() public void SetValue(object? obj, object? value, BindingFlags invokeAttr, Binder? binder, CultureInfo? culture) { - bool domainInitialized; + bool isClassInitialized; unsafe { @@ -295,9 +295,9 @@ ref Unsafe.AsRef(_addressOrOffset.ToPointer()), VerifyInstanceField(obj, ref value, invokeAttr, binder, culture); } - domainInitialized = false; - RuntimeFieldHandle.SetValue(_fieldInfo, obj, value, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref domainInitialized); - if (domainInitialized) + isClassInitialized = false; + RuntimeFieldHandle.SetValue(_fieldInfo, obj, value, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref isClassInitialized); + if (isClassInitialized) { Initialize(); } @@ -323,19 +323,26 @@ ref Unsafe.AsRef(_addressOrOffset.ToPointer()), VerifyInstanceField(obj, ref value, invokeAttr, binder, culture); } - domainInitialized = true; - RuntimeFieldHandle.SetValue(_fieldInfo, obj, value, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref domainInitialized); + isClassInitialized = true; + RuntimeFieldHandle.SetValue(_fieldInfo, obj, value, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref isClassInitialized); } private void InitializeClass() { - if (_fieldInfo.DeclaringType is null) + try { - RunModuleConstructor(_fieldInfo.Module); + if (_fieldInfo.DeclaringType is null) + { + RunModuleConstructor(_fieldInfo.Module); + } + else + { + RunClassConstructor(_fieldInfo); + } } - else + catch (TypeInitializationException ex) { - RunClassConstructor(_fieldInfo); + throw new TargetInvocationException(ex); } [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2059:RunClassConstructor", diff --git a/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs b/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs index aefc6c18a42e05..e6a0b9e2abd042 100644 --- a/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs +++ b/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs @@ -137,6 +137,37 @@ public void GetAndSetValueTypeFromStatic() Assert.Equal(11, ((MyStruct)obj).intField); } + [Fact] + public void ClassInitializerCalledOnceOnException() + { + TestThrows(); + for (int j = 0; j < 100; j++) GC.Collect(); // Encourage the type to unload. + TestThrows(); + + for (int j = 0; j < 100; j++) GC.Collect(); // Encourage the type to unload. + InitializerNotCalledAfterThrow(); + + static void TestThrows() + { + FieldInfo fi = typeof(MyTypeThatThrowsInClassInitializer).GetField(nameof(MyTypeThatThrowsInClassInitializer.s_field)); + for (int i = 0; i < 1; i++) + { + Assert.Throws(() => fi.GetValue(null)); + Assert.Throws(() => fi.SetValue(null, 100)); + } + } + + static void InitializerNotCalledAfterThrow() + { + // Setting this stops the class initializer's code from throwing, but the runtime caches the previous exception so it never runs. + SettingsForMyTypeThatThrowsInClassInitializer.s_shouldThrow = false; + + FieldInfo fi = typeof(MyTypeThatThrowsInClassInitializer).GetField(nameof(MyTypeThatThrowsInClassInitializer.s_field)); + Assert.Throws(() => fi.GetValue(null)); + Assert.Throws(() => fi.SetValue(null, 100)); + } + } + public static IEnumerable GetValue_Invalid_TestData() { yield return new object[] { typeof(FieldInfoTests), nameof(FieldInfoTests.stringField), null, typeof(TargetException) }; @@ -780,5 +811,27 @@ public struct MyStructWithFunctionPointers public MyStructWithFunctionPointers() { } } + + public class MyTypeThatThrowsInClassInitializer + { + public static int s_field; + + static MyTypeThatThrowsInClassInitializer() + { + FieldInfo fi = typeof(MyTypeThatThrowsInClassInitializer).GetField(nameof(s_field)); + //for (int i = 0; i < 3; i++) + //fi.GetValue(null); + + if (SettingsForMyTypeThatThrowsInClassInitializer.s_shouldThrow) + { + throw new Exception(); + } + } + } + + public static class SettingsForMyTypeThatThrowsInClassInitializer + { + public static bool s_shouldThrow = true; + } } } diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs index 33162acb597819..7811adcf631c05 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs @@ -97,8 +97,8 @@ internal override void CheckConsistency(object target) [DebuggerHidden] internal override void UnsafeSetValue(object? obj, object? value, BindingFlags invokeAttr, Binder? binder, CultureInfo? culture) { - bool domainInitialized = false; - RuntimeFieldHandle.SetValue(this, obj, value, null, Attributes, null, ref domainInitialized); + bool isClassInitialized = false; + RuntimeFieldHandle.SetValue(this, obj, value, null, Attributes, null, ref isClassInitialized); } [DebuggerStepThrough] From c13cbb96f73b3b77eed01246501487522d9ad3da Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Tue, 13 Feb 2024 11:28:03 -0600 Subject: [PATCH 08/12] Uncomment some test code --- .../tests/System.Reflection.Tests/FieldInfoTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs b/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs index e6a0b9e2abd042..91cec2bfa99fc8 100644 --- a/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs +++ b/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs @@ -819,8 +819,8 @@ public class MyTypeThatThrowsInClassInitializer static MyTypeThatThrowsInClassInitializer() { FieldInfo fi = typeof(MyTypeThatThrowsInClassInitializer).GetField(nameof(s_field)); - //for (int i = 0; i < 3; i++) - //fi.GetValue(null); + for (int i = 0; i < 3; i++) + fi.GetValue(null); if (SettingsForMyTypeThatThrowsInClassInitializer.s_shouldThrow) { From 8b266a16e96f9502010ab72856e4fac2ee73f1f8 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Tue, 13 Feb 2024 13:53:56 -0600 Subject: [PATCH 09/12] Avoid check exception Type in tests --- .../src/System/Reflection/FieldAccessor.cs | 15 ++++----------- .../System.Reflection.Tests/FieldInfoTests.cs | 16 +++++++++++----- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs index 5b2b9497d330b8..3acfe1f039b7e9 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs @@ -329,20 +329,13 @@ ref Unsafe.AsRef(_addressOrOffset.ToPointer()), private void InitializeClass() { - try + if (_fieldInfo.DeclaringType is null) { - if (_fieldInfo.DeclaringType is null) - { - RunModuleConstructor(_fieldInfo.Module); - } - else - { - RunClassConstructor(_fieldInfo); - } + RunModuleConstructor(_fieldInfo.Module); } - catch (TypeInitializationException ex) + else { - throw new TargetInvocationException(ex); + RunClassConstructor(_fieldInfo); } [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2059:RunClassConstructor", diff --git a/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs b/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs index 91cec2bfa99fc8..d854e19c71ade7 100644 --- a/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs +++ b/src/libraries/System.Runtime/tests/System.Reflection.Tests/FieldInfoTests.cs @@ -150,10 +150,12 @@ public void ClassInitializerCalledOnceOnException() static void TestThrows() { FieldInfo fi = typeof(MyTypeThatThrowsInClassInitializer).GetField(nameof(MyTypeThatThrowsInClassInitializer.s_field)); - for (int i = 0; i < 1; i++) + for (int i = 0; i < 3; i++) { - Assert.Throws(() => fi.GetValue(null)); - Assert.Throws(() => fi.SetValue(null, 100)); + // The actual exception may be TargetInvocationException or TypeInitializationException; there is no guarantee on when + // exactly the class initializer is called (e.g. it could happen before the GetValue\SetValue operation actually runs). + Assert.ThrowsAny(() => fi.GetValue(null)); + Assert.ThrowsAny(() => fi.SetValue(null, 100)); } } @@ -163,8 +165,8 @@ static void InitializerNotCalledAfterThrow() SettingsForMyTypeThatThrowsInClassInitializer.s_shouldThrow = false; FieldInfo fi = typeof(MyTypeThatThrowsInClassInitializer).GetField(nameof(MyTypeThatThrowsInClassInitializer.s_field)); - Assert.Throws(() => fi.GetValue(null)); - Assert.Throws(() => fi.SetValue(null, 100)); + Assert.ThrowsAny(() => fi.GetValue(null)); + Assert.ThrowsAny(() => fi.SetValue(null, 100)); } } @@ -819,8 +821,12 @@ public class MyTypeThatThrowsInClassInitializer static MyTypeThatThrowsInClassInitializer() { FieldInfo fi = typeof(MyTypeThatThrowsInClassInitializer).GetField(nameof(s_field)); + + // Ensure that the runtime doesn't treat this type has having been initialized due to successful GetValue(). for (int i = 0; i < 3; i++) + { fi.GetValue(null); + } if (SettingsForMyTypeThatThrowsInClassInitializer.s_shouldThrow) { From fe994593d6af88eaf9dce5ded8fe3ee06b0f9627 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Tue, 13 Feb 2024 16:11:05 -0600 Subject: [PATCH 10/12] Update to last commit for feedback --- src/coreclr/vm/field.h | 4 +- src/coreclr/vm/invokeutil.cpp | 6 +- src/coreclr/vm/methodtable.cpp | 13 +-- src/coreclr/vm/methodtable.h | 2 +- .../src/System/Reflection/FieldAccessor.cs | 110 +++++++----------- 5 files changed, 56 insertions(+), 79 deletions(-) diff --git a/src/coreclr/vm/field.h b/src/coreclr/vm/field.h index bcaafd427db697..c37fa4244dadf9 100644 --- a/src/coreclr/vm/field.h +++ b/src/coreclr/vm/field.h @@ -526,7 +526,7 @@ class FieldDesc } } - BOOL CheckRunClassInitThrowing() + void CheckRunClassInitThrowing() { CONTRACTL { @@ -536,7 +536,7 @@ class FieldDesc } CONTRACTL_END; - return GetEnclosingMethodTable()->CheckRunClassInitThrowing(); + GetEnclosingMethodTable()->CheckRunClassInitThrowing(); } #endif //DACCESS_COMPILE diff --git a/src/coreclr/vm/invokeutil.cpp b/src/coreclr/vm/invokeutil.cpp index 5c529f7766d5f4..7d0c8f80becdb7 100644 --- a/src/coreclr/vm/invokeutil.cpp +++ b/src/coreclr/vm/invokeutil.cpp @@ -791,7 +791,8 @@ void InvokeUtil::SetValidField(CorElementType fldType, EX_TRY { pDeclMT->EnsureInstanceActive(); - *pIsClassInitialized = pDeclMT->CheckRunClassInitThrowing(); + pDeclMT->CheckRunClassInitThrowing(); + *pIsClassInitialized = pDeclMT->IsClassInited(); } EX_CATCH_THROWABLE(&Throwable); } @@ -1014,7 +1015,8 @@ OBJECTREF InvokeUtil::GetFieldValue(FieldDesc* pField, TypeHandle fieldType, OBJ EX_TRY { pDeclMT->EnsureInstanceActive(); - *pIsClassInitialized = pDeclMT->CheckRunClassInitThrowing(); + pDeclMT->CheckRunClassInitThrowing(); + *pIsClassInitialized = pDeclMT->IsClassInited(); } EX_CATCH_THROWABLE(&Throwable); } diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index 0ac3a566752ee7..c2a7605f048a24 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -4488,7 +4488,7 @@ void MethodTable::DoRunClassInitThrowing() //========================================================================================== // Returns true if the class is initialized. -BOOL MethodTable::CheckRunClassInitThrowing() +void MethodTable::CheckRunClassInitThrowing() { CONTRACTL { @@ -4507,11 +4507,11 @@ BOOL MethodTable::CheckRunClassInitThrowing() TRIGGERSGC(); if (IsClassPreInited()) - return TRUE; + return; // Don't initialize shared generic instantiations (e.g. MyClass<__Canon>) if (IsSharedByGenericInstantiations()) - return TRUE; + return; DomainLocalModule *pLocalModule = GetDomainLocalModule(); _ASSERTE(pLocalModule); @@ -4522,11 +4522,8 @@ BOOL MethodTable::CheckRunClassInitThrowing() if (!pLocalModule->IsClassAllocated(this, iClassIndex)) pLocalModule->PopulateClass(this); - if (pLocalModule->IsClassInitialized(this, iClassIndex)) - return TRUE; - - DoRunClassInitThrowing(); - return pLocalModule->IsClassInitialized(this, iClassIndex); + if (!pLocalModule->IsClassInitialized(this, iClassIndex)) + DoRunClassInitThrowing(); } //========================================================================================== diff --git a/src/coreclr/vm/methodtable.h b/src/coreclr/vm/methodtable.h index 9262ca51fd8151..4a7cfb2232061b 100644 --- a/src/coreclr/vm/methodtable.h +++ b/src/coreclr/vm/methodtable.h @@ -813,7 +813,7 @@ class MethodTable public: // checks whether the class initialiser should be run on this class, and runs it if necessary - BOOL CheckRunClassInitThrowing(); + void CheckRunClassInitThrowing(); // checks whether or not the non-beforefieldinit class initializers have been run for all types in this type's // inheritance hierarchy, and runs them if necessary. This simulates the behavior of running class constructors diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs index 3acfe1f039b7e9..753a32ebb55fd7 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics; -using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.Runtime.CompilerServices; using System.Threading; @@ -12,17 +11,14 @@ namespace System.Reflection internal sealed class FieldAccessor { private readonly RtFieldInfo _fieldInfo; - private readonly IntPtr _addressOrOffset; - private readonly unsafe MethodTable* _methodTable; + private IntPtr _addressOrOffset; + private unsafe MethodTable* _methodTable; private FieldAccessorType _fieldAccessType; internal FieldAccessor(FieldInfo fieldInfo) { _fieldInfo = (RtFieldInfo)fieldInfo; - InitializeClass(); - - Debug.Assert(_fieldInfo.m_declaringType != null); if (_fieldInfo.m_declaringType.ContainsGenericParameters || _fieldInfo.m_declaringType.IsNullableOfT) { @@ -31,35 +27,31 @@ internal FieldAccessor(FieldInfo fieldInfo) else { _fieldAccessType = FieldAccessorType.SlowPathUntilClassInitialized; - - if (RuntimeFieldHandle.IsFastPathSupported(_fieldInfo)) - { - // Initialize the readonly fields. - _addressOrOffset = _fieldInfo.IsStatic ? - RuntimeFieldHandle.GetStaticFieldAddress(_fieldInfo) : - RuntimeFieldHandle.GetInstanceFieldOffset(_fieldInfo); - - unsafe - { - _methodTable = _fieldInfo.FieldType.IsFunctionPointer ? - (MethodTable*) typeof(IntPtr).TypeHandle.Value : - (MethodTable*) _fieldInfo.FieldType.TypeHandle.Value; - } - } } } private void Initialize() { - RuntimeType fieldType = (RuntimeType)_fieldInfo.FieldType; + Debug.Assert(_fieldInfo.m_declaringType != null); if (!RuntimeFieldHandle.IsFastPathSupported(_fieldInfo)) { // Currently this is true for [ThreadStatic] cases, for fields added from EnC, and for fields on unloadable types. _fieldAccessType = FieldAccessorType.SlowPath; + return; } - else if (_fieldInfo.IsStatic) + + unsafe { + _methodTable = (MethodTable*)_fieldInfo.FieldType.TypeHandle.Value; + } + + RuntimeType fieldType = (RuntimeType)_fieldInfo.FieldType; + + if (_fieldInfo.IsStatic) + { + _addressOrOffset = RuntimeFieldHandle.GetStaticFieldAddress(_fieldInfo); + if (fieldType.IsValueType) { if (fieldType.IsEnum) @@ -83,6 +75,11 @@ private void Initialize() else if (fieldType.IsFunctionPointer) { _fieldAccessType = GetIntPtrAccessorTypeForStatic(); + + unsafe + { + _methodTable = (MethodTable*)typeof(IntPtr).TypeHandle.Value; + } } else { @@ -91,6 +88,8 @@ private void Initialize() } else { + _addressOrOffset = RuntimeFieldHandle.GetInstanceFieldOffset(_fieldInfo); + if (fieldType.IsEnum) { _fieldAccessType = GetPrimitiveAccessorTypeForInstance(fieldType.GetEnumUnderlyingType()); @@ -106,6 +105,11 @@ private void Initialize() else if (fieldType.IsFunctionPointer) { _fieldAccessType = GetIntPtrAccessorTypeForInstance(); + + unsafe + { + _methodTable = (MethodTable*)typeof(IntPtr).TypeHandle.Value; + } } else { @@ -285,26 +289,24 @@ ref Unsafe.AsRef(_addressOrOffset.ToPointer()), return; case FieldAccessorType.SlowPathUntilClassInitialized: + if (IsStatic()) + { + VerifyStaticField(ref value, invokeAttr, binder, culture); + } + else + { + VerifyInstanceField(obj, ref value, invokeAttr, binder, culture); + } + + isClassInitialized = false; + RuntimeFieldHandle.SetValue(_fieldInfo, obj, value, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref isClassInitialized); + if (isClassInitialized) { - if (IsStatic()) - { - VerifyStaticField(ref value, invokeAttr, binder, culture); - } - else - { - VerifyInstanceField(obj, ref value, invokeAttr, binder, culture); - } - - isClassInitialized = false; - RuntimeFieldHandle.SetValue(_fieldInfo, obj, value, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref isClassInitialized); - if (isClassInitialized) - { - Initialize(); - } - - return; + Initialize(); } + return; + case FieldAccessorType.NoInvoke: if (_fieldInfo.DeclaringType is not null && _fieldInfo.DeclaringType.ContainsGenericParameters) throw new InvalidOperationException(SR.Arg_UnboundGenField); @@ -313,7 +315,7 @@ ref Unsafe.AsRef(_addressOrOffset.ToPointer()), } } - // Slow path + // All other cases use the slow path. if (IsStatic()) { VerifyStaticField(ref value, invokeAttr, binder, culture); @@ -327,30 +329,6 @@ ref Unsafe.AsRef(_addressOrOffset.ToPointer()), RuntimeFieldHandle.SetValue(_fieldInfo, obj, value, (RuntimeType)_fieldInfo.FieldType, _fieldInfo.m_declaringType, ref isClassInitialized); } - private void InitializeClass() - { - if (_fieldInfo.DeclaringType is null) - { - RunModuleConstructor(_fieldInfo.Module); - } - else - { - RunClassConstructor(_fieldInfo); - } - - [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2059:RunClassConstructor", - Justification = "This represents the static constructor, so if this object was created, the static constructor exists.")] - static void RunClassConstructor(FieldInfo fieldInfo) - { - RuntimeHelpers.RunClassConstructor(fieldInfo.DeclaringType!.TypeHandle); - } - - static void RunModuleConstructor(Module module) - { - RuntimeHelpers.RunModuleConstructor(module.ModuleHandle); - } - } - private bool IsStatic() => (_fieldInfo.Attributes & FieldAttributes.Static) == FieldAttributes.Static; private void VerifyStaticField(ref object? value, BindingFlags invokeAttr, Binder? binder, CultureInfo? culture) @@ -526,9 +504,9 @@ private enum FieldAccessorType StaticValueTypeSize8, StaticValueTypeBoxed, StaticPointerType, - NoInvoke, SlowPathUntilClassInitialized, SlowPath, + NoInvoke, } } } From 3ab82acb108b915508a836ded9ae6083ac592ba4 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Wed, 14 Feb 2024 10:00:08 -0600 Subject: [PATCH 11/12] Only set the _methodTable when it is used --- .../src/System/Reflection/FieldAccessor.cs | 110 +++++++++--------- 1 file changed, 53 insertions(+), 57 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs index 753a32ebb55fd7..2370d8fff263a7 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/FieldAccessor.cs @@ -18,6 +18,7 @@ internal sealed class FieldAccessor internal FieldAccessor(FieldInfo fieldInfo) { _fieldInfo = (RtFieldInfo)fieldInfo; + Debug.Assert(_fieldInfo.m_declaringType != null); if (_fieldInfo.m_declaringType.ContainsGenericParameters || _fieldInfo.m_declaringType.IsNullableOfT) @@ -32,8 +33,6 @@ internal FieldAccessor(FieldInfo fieldInfo) private void Initialize() { - Debug.Assert(_fieldInfo.m_declaringType != null); - if (!RuntimeFieldHandle.IsFastPathSupported(_fieldInfo)) { // Currently this is true for [ThreadStatic] cases, for fields added from EnC, and for fields on unloadable types. @@ -41,79 +40,74 @@ private void Initialize() return; } - unsafe - { - _methodTable = (MethodTable*)_fieldInfo.FieldType.TypeHandle.Value; - } - RuntimeType fieldType = (RuntimeType)_fieldInfo.FieldType; - if (_fieldInfo.IsStatic) + unsafe { - _addressOrOffset = RuntimeFieldHandle.GetStaticFieldAddress(_fieldInfo); - - if (fieldType.IsValueType) + if (_fieldInfo.IsStatic) { - if (fieldType.IsEnum) + _addressOrOffset = RuntimeFieldHandle.GetStaticFieldAddress(_fieldInfo); + + if (fieldType.IsValueType) { - _fieldAccessType = GetPrimitiveAccessorTypeForStatic(fieldType.GetEnumUnderlyingType()); + if (fieldType.IsEnum) + { + _fieldAccessType = GetPrimitiveAccessorTypeForStatic(fieldType.GetEnumUnderlyingType()); + _methodTable = (MethodTable*)fieldType.TypeHandle.Value; + } + else if (RuntimeTypeHandle.GetCorElementType(fieldType) == CorElementType.ELEMENT_TYPE_VALUETYPE) + { + // The runtime stores non-primitive value types as a boxed value. + _fieldAccessType = FieldAccessorType.StaticValueTypeBoxed; + _methodTable = (MethodTable*)fieldType.TypeHandle.Value; + } + else + { + _fieldAccessType = GetPrimitiveAccessorTypeForStatic(fieldType); + _methodTable = (MethodTable*)fieldType.TypeHandle.Value; + } } - else if (RuntimeTypeHandle.GetCorElementType(fieldType) == CorElementType.ELEMENT_TYPE_VALUETYPE) + else if (fieldType.IsPointer) { - // The runtime stores non-primitive value types as a boxed value. - _fieldAccessType = FieldAccessorType.StaticValueTypeBoxed; + _fieldAccessType = FieldAccessorType.StaticPointerType; } - else + else if (fieldType.IsFunctionPointer) { - _fieldAccessType = GetPrimitiveAccessorTypeForStatic(fieldType); + _fieldAccessType = GetIntPtrAccessorTypeForStatic(); + _methodTable = (MethodTable*)typeof(IntPtr).TypeHandle.Value; } - } - else if (fieldType.IsPointer) - { - _fieldAccessType = FieldAccessorType.StaticPointerType; - } - else if (fieldType.IsFunctionPointer) - { - _fieldAccessType = GetIntPtrAccessorTypeForStatic(); - - unsafe + else { - _methodTable = (MethodTable*)typeof(IntPtr).TypeHandle.Value; + _fieldAccessType = FieldAccessorType.StaticReferenceType; } } else { - _fieldAccessType = FieldAccessorType.StaticReferenceType; - } - } - else - { - _addressOrOffset = RuntimeFieldHandle.GetInstanceFieldOffset(_fieldInfo); + _addressOrOffset = RuntimeFieldHandle.GetInstanceFieldOffset(_fieldInfo); - if (fieldType.IsEnum) - { - _fieldAccessType = GetPrimitiveAccessorTypeForInstance(fieldType.GetEnumUnderlyingType()); - } - else if (fieldType.IsValueType) - { - _fieldAccessType = GetPrimitiveAccessorTypeForInstance(fieldType); - } - else if (fieldType.IsPointer) - { - _fieldAccessType = FieldAccessorType.InstancePointerType; - } - else if (fieldType.IsFunctionPointer) - { - _fieldAccessType = GetIntPtrAccessorTypeForInstance(); - - unsafe + if (fieldType.IsEnum) { + _fieldAccessType = GetPrimitiveAccessorTypeForInstance(fieldType.GetEnumUnderlyingType()); + _methodTable = (MethodTable*)fieldType.TypeHandle.Value; + } + else if (fieldType.IsValueType) + { + _fieldAccessType = GetPrimitiveAccessorTypeForInstance(fieldType); + _methodTable = (MethodTable*)fieldType.TypeHandle.Value; + } + else if (fieldType.IsPointer) + { + _fieldAccessType = FieldAccessorType.InstancePointerType; + } + else if (fieldType.IsFunctionPointer) + { + _fieldAccessType = GetIntPtrAccessorTypeForInstance(); _methodTable = (MethodTable*)typeof(IntPtr).TypeHandle.Value; } - } - else - { - _fieldAccessType = FieldAccessorType.InstanceReferenceType; + else + { + _fieldAccessType = FieldAccessorType.InstanceReferenceType; + } } } } @@ -220,7 +214,9 @@ public void SetValue(object? obj, object? value, BindingFlags invokeAttr, Binder case FieldAccessorType.InstanceReferenceType: VerifyInstanceField(obj, ref value, invokeAttr, binder, culture); Debug.Assert(obj != null); - Volatile.Write(ref Unsafe.As(ref Unsafe.AddByteOffset(ref obj.GetRawData(), _addressOrOffset)), value); + Volatile.Write( + ref Unsafe.As(ref Unsafe.AddByteOffset(ref obj.GetRawData(), _addressOrOffset)), + value); return; case FieldAccessorType.InstanceValueTypeSize1: From f2dc39fc2cf3da10c48bc7de57ff0eaffac97c02 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Wed, 14 Feb 2024 10:30:21 -0600 Subject: [PATCH 12/12] IsFastPathSupported should support unloadable+instance --- src/coreclr/vm/methodtable.cpp | 1 - src/coreclr/vm/reflectioninvocation.cpp | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index c2a7605f048a24..7d16ed26fbbf05 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -4487,7 +4487,6 @@ void MethodTable::DoRunClassInitThrowing() } //========================================================================================== -// Returns true if the class is initialized. void MethodTable::CheckRunClassInitThrowing() { CONTRACTL diff --git a/src/coreclr/vm/reflectioninvocation.cpp b/src/coreclr/vm/reflectioninvocation.cpp index c4390aff2fdc19..fb5cd978b85fdf 100644 --- a/src/coreclr/vm/reflectioninvocation.cpp +++ b/src/coreclr/vm/reflectioninvocation.cpp @@ -1246,7 +1246,7 @@ static bool IsFastPathSupportedHelper(FieldDesc* pFieldDesc) return !pFieldDesc->IsThreadStatic() && !pFieldDesc->IsEnCNew() && - !pFieldDesc->IsCollectible(); + !(pFieldDesc->IsCollectible() && pFieldDesc->IsStatic()); } FCIMPL1(FC_BOOL_RET, RuntimeFieldHandle::IsFastPathSupported, ReflectFieldObject *pFieldUNSAFE)