Remove DelegateInvokeClosedStaticThunk — use InitializeClosedInstance for closed static delegates

[Copilot]

Closed delegates to static methods (e.g. extension method with a bound first parameter) unnecessarily used a dedicated DelegateInvokeClosedStaticThunk + InitializeClosedStaticThunk. At the ABI level, staticMethod(capturedObj, arg1, arg2) and instanceMethod(this=capturedObj, arg1, arg2) have identical register layouts, so the thunk indirection is wasted work — one extra generated method per delegate type.

Description

• Delegate.cs — Remove InitializeClosedStaticThunk, remove ClosedStaticThunk = 1 constant, drop stale thunk check from Target, update the reflection CreateDelegate helper to use InitializeClosedInstanceWithoutNullCheck (handles fat/generic function pointers correctly).

• DelegateCreationInfo.cs — Restructure the static branch so the closed-static path calls InitializeClosedInstance with Thunk = null, exactly mirroring how closed instance delegates are constructed. For generic static methods where targetMethod != targetCanonMethod, use InitializeClosedInstanceSlow to correctly handle fat function pointers (function pointer + instantiation argument in a single pointer). Open-static path is unchanged.

• DelegateThunks.cs — Delete DelegateInvokeClosedStaticThunk class entirely.

• DelegateThunks.Sorting.cs — Delete corresponding sorting partial.

• DelegateInfo.cs — Remove _closedStaticThunk field, its construction, its switch arm, and ClosedStaticThunk = 1 from DelegateThunkKind. The GetThunk switch table retains 6 slots (0–5); slot 1 now falls through to returnNullLabel / IntPtr.Zero.

Note

AI-generated code changes.

Original prompt

Summary

Closed delegates to static methods (e.g. a delegate to a C# extension method with a pre-populated first parameter) currently use a dedicated DelegateInvokeClosedStaticThunk and InitializeClosedStaticThunk. This is unnecessary — at the ABI level, staticMethod(capturedObject, arg1, arg2) and instanceMethod(this=capturedObject, arg1, arg2) have identical register/stack layouts, so we can use InitializeClosedInstance / InitializeClosedInstanceSlow instead, just like closed delegates to instance methods. This eliminates one thunk per delegate type and simplifies the delegate infrastructure.

Changes required

1. src/coreclr/nativeaot/System.Private.CoreLib/src/System/Delegate.cs

• Remove InitializeClosedStaticThunk method entirely (lines ~200-207).
• Remove the ClosedStaticThunk = 1 constant. Since thunk kind values are used as indices into a switch table in the GetThunk override, we cannot renumber the remaining values. Instead, keep the constant value 1 as unused/reserved. The simplest approach: remove the ClosedStaticThunk constant and leave a comment that index 1 is unused/reserved, OR just remove it entirely since nothing should reference it anymore.
  • Actually, the comment says "WARNING: These constants are also declared in System.Private.TypeLoader". We should check there too. But since NativeAOT doesn't use the TypeLoader's CallConverterThunk, and we're removing all usages, we can remove the constant.
• Update Target property: Remove the _functionPointer == GetThunk(ClosedStaticThunk) check. After the change, closed static delegates will use _firstParameter to hold the captured object (just like closed instance delegates), so Target will naturally return _firstParameter via the final fallthrough path.
• Update Delegate.CreateDelegate(MethodTable*, IntPtr, object, bool, bool) (the reflection helper): The isStatic && !isOpen branch currently calls GetThunk(Delegate.ClosedStaticThunk) + InitializeClosedStaticThunk. Change it to call InitializeClosedInstanceWithoutNullCheck(thisObject, ldftnResult) instead (same as the !isStatic && !isOpen branch). This is correct because InitializeClosedInstanceWithoutNullCheck handles both normal and generic (fat) function pointers.

2. src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DelegateCreationInfo.cs

In DelegateCreationInfo.Create, the closed && targetMethod.Signature.IsStatic branch currently does:

invokeThunk = delegateInfo.Thunks[DelegateThunkKind.ClosedStaticThunk];
initMethod = systemDelegate.GetKnownMethod("InitializeClosedStaticThunk"u8, null);

Change this to use InitializeClosedInstance (no thunk needed):

initMethod = systemDelegate.GetKnownMethod("InitializeClosedInstance"u8, null);

And remove the invokeThunk / Thunk for this path — the delegate will be constructed without a thunk, same as a normal closed instance delegate. The DelegateCreationInfo returned should have Thunk = null for this case.

Important: The closed-static path currently always passes a thunk. After the change, there is no thunk. Make sure the DelegateCreationInfo constructor and the code that consumes it (in DelegateConstructorExpansion or similar) can handle a null Thunk for static methods. Look at how the closed-instance path works — it sets Thunk to null and the init method takes only 2 parameters (object + IntPtr). InitializeClosedInstance also takes 2 parameters. So the static-closed path should be restructured to match the instance-closed path.

3. src/coreclr/tools/Common/TypeSystem/IL/Stubs/DelegateThunks.cs

• Remove the entire DelegateInvokeClosedStaticThunk class (the sealed partial class DelegateInvokeClosedStaticThunk : DelegateThunk with its EmitIL() and Name property).

4. src/coreclr/tools/Common/TypeSystem/IL/Stubs/DelegateThunks.Sorting.cs

• Remove the DelegateInvokeClosedStaticThunk partial class with ClassCode => 28195375.

5. src/coreclr/tools/Common/TypeSystem/IL/DelegateInfo.cs

• Remove _closedStaticThunk field from DelegateThunkCollection.
• Remove the _closedStaticThunk = new DelegateInvokeClosedStaticThunk(owningDelegate) line from the constructor.
• In the indexer, the case DelegateThunkKind.ClosedStaticThunk: should return null.
• Remove ClosedStaticThunk = 1 from the DelegateThunkKind enum. Since the values are used as switch indices in the generated GetThunk override, we should keep the numbering stable. Either: (a) keep the enum value but always return null for it, or (b) remove it entirely if no code references it anymore. Since DelegateCreationInfo will no longer reference DelegateThunkKind.ClosedStaticThunk, we can remove the enum value entirely. But the GetThunkMethodOverride.EmitIL() generates a switch based on DelegateThunkCollection.MaxThunkKind, so removing a value in the middle is fine as long as the switch handle...

This pull request was created from Copilot chat.

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib
See info in area-owners.md if you want to be subscribed.

[jkotas]

Would it be better to delete this instead?

[MichalStrehovsky]

Yes, I think copilot was just holding on to the comments around this that warn against touching this.

This PR is just for a conversation I had with @MichalPetryka on Discord. He pointed out CoreCLR doesn't do a thunk for this and lives with the calling convention mismatch. So I'm just wondering if we can live with it too. Will see how the testing goes. Copilot might need to write extra tests too...

[Copilot]

Deleted the reserved-index comments from both Delegate.cs and DelegateInfo.cs in 446f57e.

[MichalStrehovsky]

@copilot a test is crashing, apparently ARM64 only:

Process terminated. Access Violation: Attempted to read or write protected memory. This is often an indication that other memory is corrupt. The application will be terminated since this platform does not support throwing an AccessViolationException.
   at System.RuntimeExceptionHelpers.FailFast(String, Exception, String, RhFailFastReason, IntPtr, IntPtr)
   at System.RuntimeExceptionHelpers.GetRuntimeException(ExceptionIDs)
   at System.Runtime.EH.GetClasslibException(ExceptionIDs, IntPtr)
   at Xunit.Sdk.CollectionTrackerExtensions.AsNonStringTracker(Object)
   at Xunit.Assert.Equal[T](T, T, IEqualityComparer`1)
   at System.Reflection.Tests.MethodInfoTests.CreateDelegate_GenericMethod()

[Copilot]

@copilot a test is crashing, apparently ARM64 only:

...

The crash was because the closed-static path unconditionally used InitializeClosedInstance, which doesn't handle fat function pointers (function pointer + instantiation argument in a single pointer). For generic methods where targetMethod != targetCanonMethod, it needs InitializeClosedInstanceSlow — same as the instance delegate path does at line 264-270. Fixed in 446f57e by adding the HasInstantiation / canon check to the closed-static branch in DelegateCreationInfo.Create.

[MichalStrehovsky]

@copilot the PR description says "At the ABI level, staticMethod(capturedObj, arg1, arg2) and instanceMethod(this=capturedObj, arg1, arg2) have identical register layouts, so the thunk indirection is wasted work — one extra generated method per delegate type.". Is this true if there is a return buffer?

[Copilot]

@copilot the PR description says "At the ABI level, staticMethod(capturedObj, arg1, arg2) and instanceMethod(this=capturedObj, arg1, arg2) have identical register layouts, so the t...

No, the claim is not true when there's a return buffer on x64.

On x64 (Windows and SysV), IsRetBuffPassedAsFirstArg is true, which means the return buffer position depends on whether the call uses instance or static calling convention:

• Instance call: this(RCX/RDI), retbuf(RDX/RSI), args...
• Static call: retbuf(RCX/RDI), args...

The delegate Invoke body (DelegateMethodILEmitter.EmitIL()) emits a calli using the original Invoke signature flags — which is instance (MethodSignatureFlags.None). So the JIT positions the return buffer after this. But the target static method was compiled expecting the return buffer as the first slot. The return buffer and the captured first argument get swapped.

The old DelegateInvokeClosedStaticThunk handled this correctly — it created a new signature with Signature.Flags | MethodSignatureFlags.Static for the calli, ensuring the return buffer landed in the right place.

On ARM64 this isn't an issue because IsRetBuffPassedAsFirstArg is false — the return buffer always goes in X8 (a dedicated register), not an argument slot.

CoreCLR handles the same problem with ThisPtrRetBufPrecode — it avoids the thunk for non-retbuf cases but inserts a precode stub that swaps registers when there IS a return buffer (comdelegate.cpp line 2932-2936).

So the optimization is valid for the no-return-buffer case and for ARM64, but we'd need special handling for static methods with return buffers on x64. Options would be to keep the thunk only for that case, or implement an equivalent of CoreCLR's ThisPtrRetBufPrecode.

[MichalStrehovsky]

Yeah, the point of this would be to remove complexity for an edge case delegate scenario, not add more. Closing.