Make delegates immutable

src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs

@@ -17,20 +17,16 @@ public abstract partial class Delegate : ICloneable, ISerializable
         // _target is the object we will invoke on
         internal object? _target; // Initialized by VM as needed; null if static delegate
 
-        // MethodBase, either cached after first request or assigned from a DynamicMethod
-        // For open delegates to collectible types, this may be a LoaderAllocator object
-        internal object? _methodBase; // Initialized by VM as needed
-
-        // _methodPtr is a pointer to the method we will invoke
-        // It could be a small thunk if this is a static or UM call
-        internal IntPtr _methodPtr;
-
         // In the case of a static method passed to a delegate, this field stores
         // whatever _methodPtr would have stored: and _methodPtr points to a
         // small thunk which removes the "this" pointer before going on
         // to _methodPtrAux.
         internal IntPtr _methodPtrAux;
 
+        // _methodPtr is a pointer to the method we will invoke
+        // It could be a small thunk if this is a static or UM call
+        internal IntPtr _methodPtr;
+
         // This constructor is called from the class generated by the
         //  compiler generated code
         [RequiresUnreferencedCode("The target method might be removed")]
@@ -130,10 +126,10 @@ public override bool Equals([NotNullWhen(true)] object? obj)
 
             // method ptrs don't match, go down long path
             //
-            if (_methodBase == null || d._methodBase == null || !(_methodBase is MethodInfo) || !(d._methodBase is MethodInfo))
-                return InternalEqualMethodHandles(this, d);
-            else
-                return _methodBase.Equals(d._methodBase);
+            if (Cache.s_methodCache.TryGetValue(this, out object? thisCache) && thisCache is MethodInfo thisMethod &&
+                Cache.s_methodCache.TryGetValue(d, out object? otherCache) && otherCache is MethodInfo otherMethod)
+                return thisMethod.Equals(otherMethod);
+            return InternalEqualMethodHandles(this, d);
         }
 
         public override int GetHashCode()
@@ -156,57 +152,71 @@ public override int GetHashCode()
 
         protected virtual MethodInfo GetMethodImpl()
         {
-            if ((_methodBase == null) || !(_methodBase is MethodInfo))
+            if (Cache.s_methodCache.TryGetValue(this, out object? cachedValue) && cachedValue is MethodInfo methodInfo)
+            {
+                return methodInfo;
+            }
+
+            return GetMethodImplUncached();
+        }
+
+        internal MethodInfo GetMethodImplUncached()
+        {
+            IRuntimeMethodInfo method = FindMethodHandle();
+            RuntimeType? declaringType = RuntimeMethodHandle.GetDeclaringType(method);
+            // need a proper declaring type instance method on a generic type
+            if (declaringType.IsGenericType)
             {
-                IRuntimeMethodInfo method = FindMethodHandle();
-                RuntimeType? declaringType = RuntimeMethodHandle.GetDeclaringType(method);
-                // need a proper declaring type instance method on a generic type
-                if (declaringType.IsGenericType)
+                bool isStatic = (RuntimeMethodHandle.GetAttributes(method) & MethodAttributes.Static) != (MethodAttributes)0;
+                if (!isStatic)
                 {
-                    bool isStatic = (RuntimeMethodHandle.GetAttributes(method) & MethodAttributes.Static) != (MethodAttributes)0;
-                    if (!isStatic)
+                    if (_methodPtrAux == IntPtr.Zero)
                     {
-                        if (_methodPtrAux == IntPtr.Zero)
+                        // The target may be of a derived type that doesn't have visibility onto the
+                        // target method. We don't want to call RuntimeType.GetMethodBase below with that
+                        // or reflection can end up generating a MethodInfo where the ReflectedType cannot
+                        // see the MethodInfo itself and that breaks an important invariant. But the
+                        // target type could include important generic type information we need in order
+                        // to work out what the exact instantiation of the method's declaring type is. So
+                        // we'll walk up the inheritance chain (which will yield exactly instantiated
+                        // types at each step) until we find the declaring type. Since the declaring type
+                        // we get from the method is probably shared and those in the hierarchy we're
+                        // walking won't be we compare using the generic type definition forms instead.
+                        Type? currentType = _target!.GetType();
+                        Type targetType = declaringType.GetGenericTypeDefinition();
+                        while (currentType != null)
                         {
-                            // The target may be of a derived type that doesn't have visibility onto the
-                            // target method. We don't want to call RuntimeType.GetMethodBase below with that
-                            // or reflection can end up generating a MethodInfo where the ReflectedType cannot
-                            // see the MethodInfo itself and that breaks an important invariant. But the
-                            // target type could include important generic type information we need in order
-                            // to work out what the exact instantiation of the method's declaring type is. So
-                            // we'll walk up the inheritance chain (which will yield exactly instantiated
-                            // types at each step) until we find the declaring type. Since the declaring type
-                            // we get from the method is probably shared and those in the hierarchy we're
-                            // walking won't be we compare using the generic type definition forms instead.
-                            Type? currentType = _target!.GetType();
-                            Type targetType = declaringType.GetGenericTypeDefinition();
-                            while (currentType != null)
+                            if (currentType.IsGenericType &&
+                                currentType.GetGenericTypeDefinition() == targetType)
                             {
-                                if (currentType.IsGenericType &&
-                                    currentType.GetGenericTypeDefinition() == targetType)
-                                {
-                                    declaringType = currentType as RuntimeType;
-                                    break;
-                                }
-                                currentType = currentType.BaseType;
+                                declaringType = currentType as RuntimeType;
+                                break;
                             }
-
-                            // RCWs don't need to be "strongly-typed" in which case we don't find a base type
-                            // that matches the declaring type of the method. This is fine because interop needs
-                            // to work with exact methods anyway so declaringType is never shared at this point.
-                            Debug.Assert(currentType != null || _target.GetType().IsCOMObject, "The class hierarchy should declare the method");
-                        }
-                        else
-                        {
-                            // it's an open one, need to fetch the first arg of the instantiation
-                            MethodInfo invoke = this.GetType().GetMethod("Invoke")!;
-                            declaringType = (RuntimeType)invoke.GetParametersAsSpan()[0].ParameterType;
+                            currentType = currentType.BaseType;
                         }
+
+                        // RCWs don't need to be "strongly-typed" in which case we don't find a base type
+                        // that matches the declaring type of the method. This is fine because interop needs
+                        // to work with exact methods anyway so declaringType is never shared at this point.
+                        Debug.Assert(currentType != null || _target.GetType().IsCOMObject, "The class hierarchy should declare the method");
+                    }
+                    else
+                    {
+                        // it's an open one, need to fetch the first arg of the instantiation
+                        MethodInfo invoke = GetType().GetMethod("Invoke")!;
+                        declaringType = (RuntimeType)invoke.GetParametersAsSpan()[0].ParameterType;
                     }
                 }
-                _methodBase = (MethodInfo)RuntimeType.GetMethodBase(declaringType, method)!;
             }
-            return (MethodInfo)_methodBase;
+            MethodInfo methodInfo = (MethodInfo)RuntimeType.GetMethodBase(declaringType, method)!;
+            SetCachedMethod(methodInfo);
+            return methodInfo;
+        }
+
+        // this is called by the VM in addition to calls from managed code
+        internal void SetCachedMethod(object? value)
+        {
+            Cache.s_methodCache.AddOrUpdate(this, value);
         }
 
         public object? Target => GetTarget();
@@ -519,6 +529,14 @@ internal void InitializeVirtualCallStub(IntPtr methodPtr)
         {
             return (_methodPtrAux == IntPtr.Zero) ? _target : null;
         }
+
+        // Caches MethodInfos, added either after first request or assigned from a DynamicMethod
+        // For open delegates to collectible types, this may contain a LoaderAllocator object
+        // that prevents the type from being unloaded.
+        internal static class Cache
+        {
+            public static readonly ConditionalWeakTable<Delegate, object?> s_methodCache = new();
+        }
     }
 
     // These flags effect the way BindToMethodInfo and BindToMethodName are allowed to bind a delegate to a target method. Their

src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs

@@ -200,10 +200,10 @@ internal void StoreDynamicMethod(MethodInfo dynamicMethod)
                 Debug.Assert(!IsUnmanagedFunctionPtr(), "dynamic method and unmanaged fntptr delegate combined");
                 // must be a secure/wrapper one, unwrap and save
                 MulticastDelegate d = ((MulticastDelegate?)_invocationList)!;
-                d._methodBase = dynamicMethod;
+                d.SetCachedMethod(dynamicMethod);
             }
             else
-                _methodBase = dynamicMethod;
+                SetCachedMethod(dynamicMethod);
         }
 
         // This method will combine this delegate with the passed delegate
@@ -495,6 +495,16 @@ public sealed override int GetHashCode()
         }
 
         protected override MethodInfo GetMethodImpl()
+        {
+            if (Cache.s_methodCache.TryGetValue(this, out object? cachedValue) && cachedValue is MethodInfo methodInfo)
+            {
+                return methodInfo;
+            }
+
+            return GetMulticastMethodImplUncached();
+        }
+
+        internal MethodInfo GetMulticastMethodImplUncached()
         {
             if (_invocationCount != 0 && _invocationList != null)
             {
@@ -515,25 +525,24 @@ protected override MethodInfo GetMethodImpl()
             {
                 // we handle unmanaged function pointers here because the generic ones (used for WinRT) would otherwise
                 // be treated as open delegates by the base implementation, resulting in failure to get the MethodInfo
-                if ((_methodBase == null) || !(_methodBase is MethodInfo))
-                {
-                    IRuntimeMethodInfo method = FindMethodHandle();
-                    RuntimeType declaringType = RuntimeMethodHandle.GetDeclaringType(method);
 
-                    // need a proper declaring type instance method on a generic type
-                    if (declaringType.IsGenericType)
-                    {
-                        // we are returning the 'Invoke' method of this delegate so use this.GetType() for the exact type
-                        RuntimeType reflectedType = (RuntimeType)GetType();
-                        declaringType = reflectedType;
-                    }
-                    _methodBase = (MethodInfo)RuntimeType.GetMethodBase(declaringType, method)!;
+                IRuntimeMethodInfo method = FindMethodHandle();
+                RuntimeType declaringType = RuntimeMethodHandle.GetDeclaringType(method);
+
+                // need a proper declaring type instance method on a generic type
+                if (declaringType.IsGenericType)
+                {
+                    // we are returning the 'Invoke' method of this delegate so use this.GetType() for the exact type
+                    RuntimeType reflectedType = (RuntimeType)GetType();
+                    declaringType = reflectedType;
                 }
-                return (MethodInfo)_methodBase;
+                MethodInfo methodInfo = (MethodInfo)RuntimeType.GetMethodBase(declaringType, method)!;
+                SetCachedMethod(methodInfo);
+                return methodInfo;
             }
 
             // Otherwise, must be an inner delegate of a wrapper delegate of an open virtual method. In that case, call base implementation
-            return base.GetMethodImpl();
+            return GetMethodImplUncached();
         }
 
         // this should help inlining
@@ -595,7 +604,7 @@ private void CtorCollectibleClosedStatic(object target, IntPtr methodPtr, IntPtr
         {
             this._target = target;
             this._methodPtr = methodPtr;
-            this._methodBase = GCHandle.InternalGet(gchandle);
+            this.SetCachedMethod(GCHandle.InternalGet(gchandle));
         }
 
         [DebuggerNonUserCode]
@@ -605,7 +614,7 @@ private void CtorCollectibleOpened(object target, IntPtr methodPtr, IntPtr shuff
             this._target = this;
             this._methodPtr = shuffleThunk;
             this._methodPtrAux = methodPtr;
-            this._methodBase = GCHandle.InternalGet(gchandle);
+            this.SetCachedMethod(GCHandle.InternalGet(gchandle));
         }
 
         [DebuggerNonUserCode]
@@ -614,7 +623,7 @@ private void CtorCollectibleVirtualDispatch(object target, IntPtr methodPtr, Int
         {
             this._target = this;
             this._methodPtr = shuffleThunk;
-            this._methodBase = GCHandle.InternalGet(gchandle);
+            this.SetCachedMethod(GCHandle.InternalGet(gchandle));
             this.InitializeVirtualCallStub(methodPtr);
         }
 #pragma warning restore IDE0060

src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs

@@ -2114,7 +2114,7 @@ private void ceeInfoGetCallInfo(
                 // of shared generic code calling a shared generic implementation method, which should be rare.
                 //
                 // An alternative design would be to add a new generic dictionary entry kind to hold the MethodDesc
-                // of the constrained target instead, and use that in some circumstances; however, implementation of 
+                // of the constrained target instead, and use that in some circumstances; however, implementation of
                 // that design requires refactoring variuos parts of the JIT interface as well as
                 // TryResolveConstraintMethodApprox. In particular we would need to be abled to embed a constrained lookup
                 // via EmbedGenericHandle, as well as decide in TryResolveConstraintMethodApprox if the call can be made

src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp

@@ -3631,10 +3631,10 @@ void MethodContext::recGetThreadLocalStaticBlocksInfo(CORINFO_THREAD_STATIC_BLOC
 void MethodContext::dmpGetThreadLocalStaticBlocksInfo(DWORD key, const Agnostic_GetThreadLocalStaticBlocksInfo& value)
 {
     printf("GetThreadLocalStaticBlocksInfo key %u, tlsIndex-%s, "
-           ", tlsGetAddrFtnPtr-%016" PRIX64 ", tlsIndexObject - %016" PRIX64 
+           ", tlsGetAddrFtnPtr-%016" PRIX64 ", tlsIndexObject - %016" PRIX64
            ", threadVarsSection - %016" PRIX64
            ", offsetOfThreadLocalStoragePointer-%u, offsetOfMaxThreadStaticBlocks-%u"
-           ", offsetOfThreadStaticBlocks-%u, offsetOfGCDataPointer-%u",           
+           ", offsetOfThreadStaticBlocks-%u, offsetOfGCDataPointer-%u",
            key, SpmiDumpHelper::DumpAgnostic_CORINFO_CONST_LOOKUP(value.tlsIndex).c_str(), value.tlsGetAddrFtnPtr,
            value.tlsIndexObject, value.threadVarsSection, value.offsetOfThreadLocalStoragePointer,
            value.offsetOfMaxThreadStaticBlocks, value.offsetOfThreadStaticBlocks, value.offsetOfGCDataPointer);
@@ -3652,7 +3652,7 @@ void MethodContext::repGetThreadLocalStaticBlocksInfo(CORINFO_THREAD_STATIC_BLOC
     pInfo->tlsIndexObject                       = (void*)value.tlsIndexObject;
     pInfo->threadVarsSection                    = (void*)value.threadVarsSection;
     pInfo->offsetOfThreadLocalStoragePointer    = value.offsetOfThreadLocalStoragePointer;
-    pInfo->offsetOfMaxThreadStaticBlocks        = value.offsetOfMaxThreadStaticBlocks;    
+    pInfo->offsetOfMaxThreadStaticBlocks        = value.offsetOfMaxThreadStaticBlocks;
     pInfo->offsetOfThreadStaticBlocks           = value.offsetOfThreadStaticBlocks;
     pInfo->offsetOfGCDataPointer                = value.offsetOfGCDataPointer;
 }

src/coreclr/vm/amd64/asmconstants.h

@@ -680,13 +680,13 @@ template<size_t N>
 class FindCompileTimeConstant
 {
 private:
-	FindCompileTimeConstant();
+    FindCompileTimeConstant();
 };
 
 void BogusFunction()
 {
-	// Sample usage to generate the error
-	FindCompileTimeConstant<offsetof(Thread, m_pDomain)> bogus_variable;
-	FindCompileTimeConstant<offsetof(Thread, m_ExceptionState)> bogus_variable2;
+    // Sample usage to generate the error
+    FindCompileTimeConstant<offsetof(Thread, m_pDomain)> bogus_variable;
+    FindCompileTimeConstant<offsetof(Thread, m_ExceptionState)> bogus_variable2;
 }
 #endif // defined(__cplusplus) && defined(USE_COMPILE_TIME_CONSTANT_FINDER)

src/coreclr/vm/comdelegate.cpp

@@ -2808,8 +2808,8 @@ MethodDesc* COMDelegate::GetDelegateCtor(TypeHandle delegateType, MethodDesc *pT
     LoaderAllocator *pTargetMethodLoaderAllocator = pTargetMethod->GetLoaderAllocator();
     BOOL isCollectible = pTargetMethodLoaderAllocator->IsCollectible();
     // A method that may be instantiated over a collectible type, and is static will require a delegate
-    // that has the _methodBase field filled in with the LoaderAllocator of the collectible assembly
-    // associated with the instantiation.
+    // that has the LoaderAllocator of the collectible assembly associated with the instantiation
+    // stored in the MethodInfo cache.
     BOOL fMaybeCollectibleAndStatic = FALSE;
 
     // Do not allow static methods with [UnmanagedCallersOnlyAttribute] to be a delegate target.