Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a new unmanaged calling convention bit for use with function pointers #38133

Closed
tannergooding opened this issue Jun 19, 2020 · 10 comments · Fixed by #39030
Closed

Add a new unmanaged calling convention bit for use with function pointers #38133

tannergooding opened this issue Jun 19, 2020 · 10 comments · Fixed by #39030
Labels
api-approved API was approved in API review, it can be implemented area-Interop-coreclr
Milestone

Comments

@tannergooding
Copy link
Member

tannergooding commented Jun 19, 2020

We need a new unmanaged calling convention bit for use with function pointers in .NET 5.

This bit will represent the "platform" default calling convention (essentially equivalent to Winapi for P/Invoke).

Based on the conversations, this would be bit 9 in the calling kind encoding.

namespace System.Reflection.Metadata
{
    public enum SignatureCallingConvention : byte
    {
        // existing
        Default = 0x0,
        CDecl = 0x1,
        StdCall = 0x2,
        ThisCall = 0x3,
        FastCall = 0x4,

        // 0x5 - 0x8 are taken by other types of signatures (fields, properties, etc.)

        // new
        Unmanaged = 0x9,
    }
}
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jun 19, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.

@tannergooding
Copy link
Member Author

tannergooding commented Jun 19, 2020

@tannergooding tannergooding added this to the 5.0.0 milestone Jun 19, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT added area-Interop-coreclr and removed untriaged New issue has not been triaged by the area owner labels Jun 19, 2020
@AaronRobinsonMSFT
Copy link
Member

Related #34805

@jkotas
Copy link
Member

jkotas commented Jun 19, 2020

This should include the new API proposal for System.Reflection.Metadata. I have edited the top post.

@jkotas jkotas added api-ready-for-review blocking Marks issues that we want to fast track in order to unblock other important work labels Jun 19, 2020
@AaronRobinsonMSFT
Copy link
Member

Seems like we are going to also have to update System.Runtime.InteropServices.CallingConvention to support the ILGenerator.EmitCalli() scenario.

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Jun 24, 2020

I have updated the above proposal. We could also not support it since the API doesn't seem to have a place to add modopts?

/cc @davidwrighton

Edit: Removed the updated enum CallingConvention from proposal since it isn't worth trying to reconcile with all the places the enum is used.

@jkotas
Copy link
Member

jkotas commented Jun 24, 2020

enum CallingConvention is used in other more common contexts like DllImport where this new value would be confusing.

The use of enum CallingConvention in the Reflection.Emit APIs is broken, e.g. CallingConvention.WinApi is mapped to StdCall here. I think we should leave the use of this enum in Reflection.Emit alone and not try to fix it or improve it.

If we wanted to make this work with existing reflection APIs, I think it may be better to add a new Unmanaged value to enum CallingConventions. This enum can be used in combination with SignatureHelper to produce signature with modopts and then emitted using Emit(OpCode opcode, SignatureHelper signature).

@AaronRobinsonMSFT
Copy link
Member

The use of enum CallingConvention in the Reflection.Emit APIs is broken

That is what I was thinking as well. Perhaps it would be worth marking that API as Obsolete?

@davidwrighton
Copy link
Member

I think we should hold off on adding new support to reflection emit for this in this release. We're very close to ship, and I'm not aware of any scenarios where we need calli support for this in the .NET 5 timeframe. Support in System.Reflection.Metadata is different though, as that is the api surface used by the compilers to generate code, we should fix that so that the Roslyn team doesn't need to maintain their own definition of the enum value.

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review blocking Marks issues that we want to fast track in order to unblock other important work labels Jul 2, 2020
@terrajobst
Copy link
Member

terrajobst commented Jul 2, 2020

Video

Technically, the actual calling convention is expressed as modopt on the return type but we felt a vague name is good enough.

namespace System.Reflection.Metadata
{
    public enum SignatureCallingConvention : byte
    {
        Unmanaged = 0x9
    }
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-Interop-coreclr
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants