Support HasThis and ExplicitThis calling conventions in AssemblyBuilder and DynamicMethod

[steveharter]

Fixes #113626

[Copilot]

Pull Request Overview

This PR adds support for the HasThis calling convention in PersistedAssemblyBuilder by extending the signature calling convention and updating associated tests and module builder logic.

• Introduces an extended enum (SignatureCallingConventionEx) with a HasThis flag.
• Adds a new test (AssemblyWithInstanceBasedFunctionPointer) to verify the instance-based function pointer behavior.
• Updates ModuleBuilderImpl to check and set the HasThis flag based on the calling convention.

Reviewed Changes

Copilot reviewed 3 out of 5 changed files in this pull request and generated 1 comment.

File	Description
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/SignatureCallingConventionEx.cs	Adds an extended enum for calling convention flags.
src/libraries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveAssemblyBuilderTests.cs	Adds tests for instance-based function pointer calls.
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs	Updates GetSignatureConvention to incorporate the HasThis flag.

Files not reviewed (2)

• src/libraries/System.Reflection.Emit/src/System.Reflection.Emit.csproj: Language not supported
• src/libraries/System.Reflection.Emit/tests/System.Reflection.Emit.Tests.csproj: Language not supported

Comments suppressed due to low confidence (1)

src/libraries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveAssemblyBuilderTests.cs:351

• [nitpick] Consider adding a comment to clarify why a static method is being used, how it simulates an instance call with HasThis, and what the expected behavior is, to aid future maintainers in understanding the test.

il.EmitCalli(OpCodes.Calli, CallingConventions.HasThis, typeof(Guid), null, null);

[Copilot]

Ensure that the conversion from SignatureCallingConvention to SignatureCallingConventionEx preserves all intended flags without conflicts. Adding a short comment to document why the cast to the extended enum is safe would improve maintainability.

[AaronRobinsonMSFT]

Is there any benefit to adding an ExplicitThis test? I'm still not entirely clear on what it is used for.

[jkotas]

ExplicitThis means that the this type is explicitly encoded in the signature.

For example, MyClassWithGuidProperty.MyGuid property from the existing test would be called like this: il.EmitCalli(OpCodes.Calli, CallingConventions.HasThis | CallingConventions.ExplicitThis, typeof(Guid), [typeof(MyClassWithGuidProperty)], null); with ExplicitThis.

It makes sense on function pointers and calli signatures only. From ECMA spec: "The EXPLICITTHIS (0x40) bit can be set only in signatures for function pointers"

[steveharter]

I could add a test, but other than to verify the signature actually has the extra "this" parameter there isn't much to test since the IL stays the same: the caller still needs to push the "this" pointer before the calli, and the target instance method still needs to do a ldarg.0 as the first statement.

I did at least find one test that would verify the runtime doesn't choke on it:

runtime/src/tests/JIT/Directed/tailcall/more_tailcalls.cs

Line 434 in d34ef7e

IL.Emit.Calli(new StandAloneMethodSig(CallingConventions.Standard | CallingConventions.HasThis | CallingConventions.ExplicitThis,

.

If C# adds support for instance-based function pointers, I think it would want to use ExplicitThis since it would get the "this" parameter in the signature instead of some passing it some other way. We may also want to use it for intrinstic\UnsafeAccessor for #112994.

[jkotas]

I could add a test, but other than to verify the signature actually has the extra "this" parameter

Right, the test would verify that the extra "this" is not dropped on the floor somewhere along the way in the System.Reflection.Emit or System.Reflection.Metadata.

[AaronRobinsonMSFT]

Yep. I would like to see a test in this particular test suite even if only for completeness.

[steveharter]

I added a test for both the AssemblyBuilder and for the DynamicMethod case, and it is failing on the ExplicitThis tests.

I did verify that the signature is being created correctly with calli explicit instance, but the "this" pointer in the test's Guid-based property getter is not valid.

[AaronRobinsonMSFT]

How is it failing?

[steveharter]

I just pushed a change to the jitter - it needs to know that the first arg is "this" for ExplicitThis.

[jakobbotsch]

Yeah, looks like a pretty old bug. Without this we insert the retbuffer in the wrong place for callis with explicitthis that use return buffers.

[steveharter]

Wasm build failures due to #113685

[jkotas]

Suggested change

	il.EmitCalli(OpCodes.Calli, CallingConventions.HasThis, typeof(Guid), null, null);
	il.EmitCalli(OpCodes.Calli, CallingConventions.HasThis, typeof(Guid), null);
	il.Emit(OpCodes.Ldarg_0); // this
	il.Emit(OpCodes.Ldarg_1); // fn
	il.EmitCalli(OpCodes.Calli, CallingConventions.HasThis | CallingConventions.ExplicitThis, typeof(Guid), [typeof(MyClassWithGuidProperty)]);

This should be good-enough as a test for ExplicitThis

[AaronRobinsonMSFT]

/cc @dotnet/jit-contrib

[jkotas]

Use 'object' for type in tests

What was wrong with MyClassWithGuidProperty?