Support HasThis and ExplicitThis calling conventions in AssemblyBuilder and DynamicMethod #113666
Conversation
There was a problem hiding this comment.
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);
| } | ||
|
|
||
| return convention; | ||
| if ((callingConvention & CallingConventions.HasThis) != 0) |
There was a problem hiding this comment.
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.
Copilot uses AI. Check for mistakes.
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/SignatureCallingConventionEx.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs
Outdated
Show resolved
Hide resolved
...es/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveAssemblyBuilderTests.cs
Outdated
Show resolved
Hide resolved
|
Wasm build failures due to #113685 |
...es/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveAssemblyBuilderTests.cs
Outdated
Show resolved
Hide resolved
steveharter
changed the title
|
/cc @dotnet/jit-contrib |
What was wrong with |
Nothing, just wanted to make sure I can use |
| if ((callingConvention & CallingConventions.HasThis) == CallingConventions.HasThis) | ||
|
|
||
| // Pop the this parameter if the method has an implicit this parameter. | ||
| if ((callingConvention & CallingConventions.HasThis) == CallingConventions.HasThis && |
There was a problem hiding this comment.
Could simplify the condition to (callingConvention & (CallingConventions.HasThis | CallingConventions.ExplicitThis)) == CallingConventions.HasThis
Fixes: - `BuildResumptionStubCalliSignature` was building a signature with `HASTHIS | EXPLICITTHIS` calling convention always, even for signatures without any 'this'. With dotnet/runtime#113666 this broke as we started reordering the first argument with ret buffers. - Update JIT interface `getEHinfo` to handle getting EH clause information when trying to inline methods with transient IL, like async thunks - Update for new refactored jitinterface classes - Update for removed `g_isNewExceptionHandlingEnabled`
Fixes issues around using
calliwithCallingConventions.HasThisandCallingConventions.ExplicitThisboth when generating the IL and when executing the IL.Fixes #113626