Runtime: CallingConvention gap in ILGenerator.EmitCalli

[mellinoe]

Based on this stackoverflow submission.

It seems like there is a bit of a feature gap in the current interface for ILGenerator. .NET Core has a EmitCalli method, but the calling conventions it supports do not line up exactly with the ones exposed from the .NET Framework: https://msdn.microsoft.com/en-us/library/d81ee808(v=vs.110).aspx

We should understand if this is a meaningful gap, or if the use cases for the .NET Framework version can still be covered by the new version.

We may already be considering adding this functionality back as part of our bigger API compat push.

cc: @AtsushiKan

[ghost]

For future clarification:

ILGenerator on Core is missing the overload of EmitCalli that takes a System.Runtime.InteropServices.CallingConvention enum (the unmanaged calling convention enum that includes CDecl.)

[karelz]

@AtsushiKan @DnlHarvey please update top post with proposal if it is ready.

[ghost]

Does not need an api review as this proposal is simply about restoring an overload that already exists on the full framework:

public virtual void EmitCalli(
OpCode opcode,
CallingConvention unmanagedCallConv,
Type returnType,
Type[] parameterTypes
)

[karelz]

@AtsushiKan is it just matter of exposing existing functionality, or what is involved in the work? (the "next step & complexity" comment ;-))

[karelz]

Also, unassigning you as you don't plan to work on it now ...

[ghost]

The overload and its underlying support was physically removed from CoreClr so it'll have to be exhumed from the desktop source. Estimate: light to medium complexity.

[Cronan]

I'm interested in having a go at this, it's a major blocking issue for
https://github.com/pythonnet/pythonnet/issues/96

Any advice other than "read the contrib dox"?

[jkotas]

Needs to be exposed from the contracts: https://github.com/dotnet/corefx/tree/master/src/System.Reflection.Emit.ILGeneration

[Cronan]

I noticed this - I can do it as well, if you like, I’ve got the code open as it is

[jkotas]

@Cronan Yes, that would be great. Thanks!

[Cronan]

I'm slightly confused about where exactly the change should be made.
The screen grab below is from my corefx checkout of master

Also, this is in the interface:
coreclr\src\mscorlib\shared\System\Runtime\InteropServices\CallingConvention.cs

I'm not sure how to reference it correctly from corefx.

[jkotas]

1. Wait for https://github.com/dotnet/corefx/pull/25282 to get merged. This PR will propagate CoreCLR build with your change to CoreFX repo. (Alternatively, if you do not want to wait - you can use CoreCLROverridePath option for build.cmd to point it to your private CoreCLR build https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/developer-guide.md.)
2. Add the method to https://github.com/dotnet/corefx/blob/master/src/System.Reflection.Emit.ILGeneration/ref/System.Reflection.Emit.ILGeneration.cs
3. Find a some good spot to add a test for the new API under https://github.com/dotnet/corefx/tree/master/src/System.Reflection.Emit.ILGeneration/tests/ILGenerator .

Also, I have noticed that the fix in CoreCLR is incomplete - the method needs to be overriden in System\Reflection\Emit\DynamicILGenerator.cs with specialized implementation for dynamic methods. Could you please fix that one too (it is probably best to do it before above)?

[karelz]

@Cronan I sent you Collaborator invite. Let me know when you accept and we will be able to reassign this issue to you (assigning to myself temporarily).
Pro-tip: Disable repo notifications, there's a lot of them - switch only to notifications on issues you are mentioned.

[Cronan]

Accepted.

On 16 Nov 2017, at 18:34, Karel Zikmund [e-mail] wrote:

@Cronan I sent you Collaborator invite. Let me know when you accept and we will be able to reassign this issue to you (assigning to myself temporarily).
Pro-tip: Disable repo notifications, there's a lot of them - switch only to notifications on issues you are mentioned.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[Cronan]

I’m AFK until Monday, hope that’s not a problem

[jkotas]

No problem

[karelz]

@Cronan GH shows your invite as "not accepted". Can you please double check?

[Cronan]

Sorry, I didn’t have 2-factor enabled, and the mobile GUI wasn’t very clear. Should be accepted now.