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

Update UnmanagedCallersOnlyAttribute to align with C# function pointers #37612

Closed
AaronRobinsonMSFT opened this issue Jun 8, 2020 · 10 comments · Fixed by #37843
Closed

Update UnmanagedCallersOnlyAttribute to align with C# function pointers #37612

AaronRobinsonMSFT opened this issue Jun 8, 2020 · 10 comments · Fixed by #37843
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices
Milestone

Comments

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Jun 8, 2020

Background and Motivation

The C# function pointers proposal will be consuming new types as a means to indicate future calling conventions. See discussion about calling conventions with CallConvXXX referenced in dotnet/roslyn#39865.

Original API review: #32462

Proposed API

The existing API below uses the CallingConvention enumeration. This enumeration is tailored for a 32-bit Windows world. This severely limits the growth of this API.

[AttributeUsage(AttributeTargets.Method)]
public sealed class UnmanagedCallersOnlyAttribute : Attribute
{
    public UnmanagedCallersOnlyAttribute();
    public CallingConvention CallingConvention;
    public string? EntryPoint;
}

The updated API would change the CallingConvention field to be Type[] CallConvs which is more flexible as new calling conventions are adopted.

[AttributeUsage(AttributeTargets.Method)]
public sealed class UnmanagedCallersOnlyAttribute : Attribute
{
    public UnmanagedCallersOnlyAttribute();
    public Type[]? CallConvs;
    public string? EntryPoint;
}

Usage Examples

Old:

[UnmanagedCallersOnly(CallingConvention = CallingConvention.StdCall)]
public static int SquareNumber(int val)
{
    return val * val;
}

New:

using System.Runtime.CompilerServices;

[UnmanagedCallersOnly(CallConvs = new [] { typeof(CallConvStdcall) })]
public static int SquareNumber(int val)
{
    return val * val;
}

Note It would be requested that the Roslyn compiler apply the same validation algorithm applied in the function pointers specification to the types in the CallConvs array.

Risks

If this work is not done for .NET 5 updating the UnmanagedCallersOnlyAttribute will become substantially more difficult.

@AaronRobinsonMSFT AaronRobinsonMSFT added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.InteropServices blocking Marks issues that we want to fast track in order to unblock other important work labels Jun 8, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 5.0 milestone Jun 8, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jun 8, 2020
@AaronRobinsonMSFT
Copy link
Member Author

@AaronRobinsonMSFT AaronRobinsonMSFT added api-ready-for-review and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Jun 8, 2020
@tannergooding
Copy link
Member

No types corresponds to unmanaged (WinApi), is that correct?

We can't easily expose convenience constructors that take CallingConvention (and maybe bool for things like SupressGCTransition) because the compiler would have to understand that constructor, is that correct?

@333fred
Copy link
Member

333fred commented Jun 8, 2020

We can't easily expose convenience constructors that take CallingConvention (and maybe bool for things like SupressGCTransition) because the compiler would have to understand that constructor, is that correct?

That's correct. If we have to understand that enum, it defeats the purpose of having an extensible calling convention protocol in the first place :).

@AaronRobinsonMSFT
Copy link
Member Author

No types corresponds to unmanaged (WinApi), is that correct?

Correct.

SupressGCTransition

I realize that this is part of the function pointers discussion, but will call out that marking the function UnmanagedCallersOnly with SupressGCTransition would be a very unusual and bad combination.

@AaronRobinsonMSFT
Copy link
Member Author

@tannergooding and @333fred Thoughts on changing the CallingConventionTypes property to CallConv to match the targeted type prefixes?

@tannergooding
Copy link
Member

Maybe CallConvs? to make it clearer that it is a collection?

@333fred
Copy link
Member

333fred commented Jun 8, 2020

However many 🚲s you want to 🏠 (no :shed: 🙁) is fine with me. So long as there's an array of types that matches up to what we need to emit.

@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 Jun 12, 2020
@andrew-boyarshin
Copy link
Contributor

I know I'm a bit too late (API is already approved), but here's my 2 cents: I'd name it CallingConventions, rather than CallConvs, even though it contains types named CallConvXXX. Abbreviations obscure meaning.

@nathan-alden-sr
Copy link

FWIW, Supress is spelled wrong. Hopefully wherever that is can be corrected.

@AaronRobinsonMSFT
Copy link
Member Author

@nathan-alden-sr It is correct in the official name - https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/SuppressGCTransitionAttribute.cs. I am just poor at spelling in comments 😄

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
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-System.Runtime.InteropServices
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants