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

Introduce CallConvMemberFunction to represent the member-function variants of Cdecl, Stdcall, etc. on Windows #46775

Closed
jkoritzinsky opened this issue Jan 9, 2021 · 18 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices
Milestone

Comments

@jkoritzinsky
Copy link
Member

jkoritzinsky commented Jan 9, 2021

Background and Motivation

For interop developers of managed APIs wrapping native API layers, sometimes they need to wrap C++ instance member functions with non-default calling conventions like cdecl and stdcall. However, on Windows, the C++ instance member function calling convention for cdecl and stdcall is not the same as the non-member function calling convention. I propose that we add a new calling convention type that can be used in the extensible calling convention syntax to denote that the function pointer should use the instance member calling convention variant of the specified calling convention instead of the non-member function calling convention.

Proposed API

namespace System.Runtime.CompilerServices
{
+    public class CallConvMemberFunction {
+    }
}

Usage Examples

This API would be used in the function pointer syntax as follows:

S Foo(void* thisPtr, int i, int j, delegate* unmanaged[Stdcall, MemberFunction]<void*, int, int, S> f)
{
     return f(thisPtr, i, j);
}

The MemberFunction modifier would modify the provided calling convention, or modify the default calling convention if MemberFunction or another calling convention modifier like SuppressGCTransition were provided.

If the MemberFunction is used on a platform where there is no "member function calling convention variant" of the other provided calling conventions, then it will be ignored.

Alternative Designs

We considered instead of introducing a new API, allowing function pointers of the style delegate* unmanaged[Stdcall, Thiscall]<void*, int, int, S> instead of introducing a new type. The CoreCLR Interop team decided that we did not like this design because it is not straightforward which type is the calling convention and which is a modifier. Additionally, allowing the above form would also require us to allow delegate* unmanaged[Thiscall, Stdcall]<void*, int, int, S> since they are equivalent, which we'd highly prefer not to do. We prefer a model where a function pointer can have at most one of the CallConv*call types provided to specify the "base" calling convention and any number of other CallConv* types that don't end in "call", such as this one or CallConvSuppressGCTransition, that modify the "base" calling convention.

We also considered alternative names like CallConvInstanceMethod or CallConvInstanceMemberFunction.

After more discussion, we've also come up with another alternative name that doesn't use C++ terminology so as to avoid implying more fully-featured C++ interop support: CallConvStructReturnBuffer. This name specifies the exact modification in the calling convention (structs go into return buffers) that happens on Windows in the cases this API proposal is meant to cover. After even further discussion, this was also determined undesirable due to lack of clarity and since the weirdness that calling conventions account for is inherently something specific to (Windows) C++.

cc: @AaronRobinsonMSFT @elinor-fung @tannergooding

@jkoritzinsky jkoritzinsky added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.InteropServices labels Jan 9, 2021
@jkoritzinsky jkoritzinsky added this to the 6.0.0 milestone Jan 9, 2021
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jan 9, 2021
@jkoritzinsky jkoritzinsky removed the untriaged New issue has not been triaged by the area owner label Jan 9, 2021
@AaronRobinsonMSFT
Copy link
Member

@jkoritzinsky Let's see an example of what needs to happen presently in C# in order to accomplish this scenario. Understanding what we are mitigating or making better is helpful to weight costs.

@tannergooding
Copy link
Member

@AaronRobinsonMSFT could you clarify? The C# compiler shouldn't require any changes here, it is already setup to allow any number of modifiers matching System.Runtime.CompilerServices.CallConv* in unmanaged[*].

Anything else, such as exposing a new CallConv kind should be entirely up to the runtime/libraries to expose and support. This was intentionally done so that things like CallConvSuppressGCTransition could be added: #46343
and likewise any future possible calling conventions like CallConvVectorcall, or combinations like the one proposed above.

As for what functionality it enables, it allows COM bindings (or other C++ instance member code) to be done without workarounds such as: https://source.terrafx.dev/#TerraFX.Interop.Windows/um/d3d12/ID3D12Device.cs,193
That is, the native signature is: D3D12_RESOURCE_ALLOCATION_INFO GetResourceAllocationInfo(ID3D12Device* pThis, UINT visibleMask, UINT numResourceDescs, const D3D12_RESOURCE_DESC * pResourceDescs), but due to this being a stdcall instance member, the maintainer of the interop bindings currently has to bind it as D3D12_RESOURCE_ALLOCATION_INFO* GetResourceAllocationInfo(ID3D12Device* pThis, D3D12_RESOURCE_ALLOCATIOn_INFO* pResult, UINT visibleMask, UINT numResourceDescs, const D3D12_RESOURCE_DESC * pResourceDescs) to ensure that everything lines up and works.

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Jan 9, 2021

could you clarify? The C# compiler shouldn't require any changes here, it is already setup to allow any number of modifiers matching System.Runtime.CompilerServices.CallConv* in unmanaged[*].

Yep.

As for what functionality it enables, it allows COM bindings (or other C++ instance member code) to be done without workarounds such as: https://source.terrafx.dev/#TerraFX.Interop.Windows/um/d3d12/ID3D12Device.cs,193

This is what I am talking about. The suggestion is about providing a new API that enables something. However, this appears already possible so the question is about what is improved that is already possible. The above indicates a COM based problem that is now able to be simplified - yay. What else? Solving a problem that is about COM or a legacy framework that is potentially not evolving and static may not rise to the level to warrant a new API. My question is about what is improved or enabled and if this doesn't go forward what are we left with.

I am personally not convinced that enabling this because it helps with a COM based scenario exclusively warrants the work. However, if it also enables something on macOS or Linux systems that is presently broken or blocked then that changes my calculus.

@jkoritzinsky
Copy link
Member Author

jkoritzinsky commented Jan 9, 2021

I'll give an example from SharpGenTools. SharpGenTools enables projects to use a COM-style API across all platforms. This is used by JetBrains and AvalonStudio to interface with the .NET debuggers and formerly by Avalonia to work around the lack of Objective-C binding support in .NET Core 2.0+. Avalonia still uses a COM-style API to wrap native windowing support on Mac, but they use their own generator now.

SharpGenTools supports generating code once for all platforms, but as a result has to handle a number of corner cases around this particular issue. Let's take a signature of the following in native code:

struct S { int i; };

class C
{
    virtual S __stdcall Foo(int i, int j);
};

Today, SharpGenTools generates something like the following to support this and heavily relies on fragile JIT optimizations to make PlatformSupport.IsWindows a JIT time constant get reasonable performance. As a result, this also makes this style of code-gen possibly incompatible with AOT (depending on which APIs for determining platform are available), which means that a library that's generated with this model cannot be consumed by an app developer who wants to use R2R or NativeAOT technology on all of their referenced libraries.

This method also causes a ton of binary bloat because every instance method signature that's emitted needs to be in the binary twice, once in each form. Additionally, this puts the onus on each source generator owner to correctly add support for opting intrinsic types like CLong, CULong, NFloat, or any relevant SIMD types out of this particular optimization, which is something I feel that should be owned singluarly by the runtime/JIT compiler, not pushed off to every source generator owner who touches this API model.

public S Foo(int i, int j)
{
     S __retval;
     if (PlatformSupport.IsWindows)
     {
            (*((delegate* unmanaged[Stdcall]<IntPtr, void*, int, int, void*>**)this.ThisPtr))[0](this.ThisPtr, &__retval, i, j);
     }
     else
     {
            __retval = (*((delegate* unmanaged[Stdcall]<IntPtr, int, int, S>**)this.ThisPtr))[0](this.ThisPtr, i, j);
     }
     return  __retval;
}

There is an option of not introducing a new API and instead using delegate* unmanaged[Stdcall, Thiscall]<> to represent this type of signature, but I thought we had already decided that going down that route was undesirable.

To reference other talks we've had internally, adding this calling convention could allow the debugger team to create a code generator to replace MCG that uses [Stdcall, MemberFunction] to call the COM debugger APIs for their internal testing and allow them to use whatever signatures they want in their COM contracts (and not be bound to always returning HRESULTs) without having to worry about whether the debugger is running on Windows or non-Windows.

This API isn't just for the simplification on Windows. It's to allow users who use a COM-style API off Windows (see everyone who's asked for COM Interop support on non-Windows #7968, #10572) to write their own source generators for COM and not have to worry about getting this one difference correct between Windows and non-Windows. Instead of having to use multiple signatures like above or just getting it wrong, they'd be able to use the [Stdcall, MemberFunction] calling convention to accurately model COM-style APIs on both Windows and non-Windows platforms simultaneously.

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Jan 9, 2021

Full disclosure, I am playing devil's advocate here and not trying to be difficult without purpose - remember I am huge fan of COM style APIs.

this also makes this style of code-gen possibly incompatible with AOT

That is a fair argument. I would say the pattern in question has been established so I think this might be less of a concern. Perhaps on the mono side this is an issue but AOT is a big focus and with our continual efforts on source generation I'm relatively confident we wouldn't grow without being attuned to this issue. However, the point is important to consider.

There is an option of not introducing a new API and instead using delegate* unmanaged[Stdcall, Thiscall]<> to represent this type of signature, but I thought we had already decided that going down that route was undesirable.

Yes, that is undesirable in my opinion.

could allow the debugger team to create a code generator [...] to call the COM debugger APIs
who use a COM-style API off Windows
The two above statements along with the 4 mentions of "COM-style".

This begs the question then why not call it CallConvCOMMethod - or something similar? It seems like the underlying desire is to support the COM style on all platforms, what is the argument to not just be explicit about what is desired?

Additionally, is the COM-style something that is growing or is that a legacy model that is receding from use? Mentions of the DirectX API (Windows only), Debugger API (exclusively .NET that we could special case or provide a bespoke tool), model for Avalonia, are there other forward looking products that would benefit to justify enabling this support?

Enabling this support opens up a large support matrix that we will own and will warrant a decent amount of testing. I want to make sure what we are enabling is useful going forward for interop. Consider if the argument was for this work or first class support for Rust, Swift, or Python interop. Would we enable this functionality or are those other areas worth investing in more fully rather than providing functionality for a platform that may lose market share over the next 5 to 10 years? I am hypothesizing that COM-style will continue to lose market share, but I don't see that API style growing much.

@tannergooding
Copy link
Member

This begs the question then why not call it CallConvCOMMethod - or something similar? It seems like the underlying desire is to support the COM style on all platforms, what is the argument to not just be explicit about what is desired?

The fact that it mirrors COM is really anecdotal I think. In practice, this is just binding against a C++ instance method which COM happens to mirror on most platforms due to how COM works.
I can explicitly annotate any C++ instance method with an additional calling convention and it will get respected and modify how it is used.

On Windows at least, it's not uncommon to do this to get __stdcall (x86) or __vectorcall (x86 and x64) semantics for key functions due to improved codegen.
It doesn't, AFAIK, apply to any Unix or non-x86 platforms today, namely due to them only having a single calling convention/ABI, but that doesn't mean it will always be the case.

@tannergooding
Copy link
Member

Additionally, is the COM-style something that is growing or is that a legacy model that is receding from use?

Fundamentally, all of the platform ABIs are based around C/C++ today and that likely isn't going to change due to back-compat requirements. All exports, even from other languages, are largely going to be via C/C++ compatible entry points, because they need to interop with the underlying ABI and system. In the cases where they aren't using a C/C++ compatible ABI, they typically are not directly exported and you must go through a separate export to get a compatible entry point instead. This is, AFAIK, true of .NET itself, Java, ObjC, Rust, Swift, and others.

This support is exposing one of the "core" calling convention semantics that we don't support today; which is an instance method (by default Thiscall) that has been modified with an alternative calling convention (typically __stdcall due to COM) and so I believe it will be around for a while and used, not only by users interoping with COM, but also with NanoCOM (which is what DX and others use), and with proper C++ exports.

@jkoritzinsky
Copy link
Member Author

could allow the debugger team to create a code generator [...] to call the COM debugger APIs
who use a COM-style API off Windows
The two above statements along with the 4 mentions of "COM-style".

This begs the question then why not call it CallConvCOMMethod - or something similar? It seems like the underlying desire is to support the COM style on all platforms, what is the argument to not just be explicit about what is desired?

As stated previously offline, I don't like using CallConvCOMMethod since COM implies additional semantics. Also, it falls into the same issue we have around the naming of ComWrappers, where we feel like it implies more than ABI semantics. And I don't like CallConvIUnknownMethod either since this calling convention doesn't require IUnknown to be anywhere in the type hierarcy.

Additionally, is the COM-style something that is growing or is that a legacy model that is receding from use? Mentions of the DirectX API (Windows only), Debugger API (exclusively .NET that we could special case or provide a bespoke tool), model for Avalonia, are there other forward looking products that would benefit to justify enabling this support?

So interestingly, DirectX is now available on WSL2, so it's not entirely Windows only any more. And Microsoft doesn't provide any tools for a .NET API for interacting with the debugger due to licensing reasons (the managed debugger API is only licensed for VS, VS4Mac, and VSCode), so external developers will have to write their own tools and end up having to account for these issues.

Enabling this support opens up a large support matrix that we will own and will warrant a decent amount of testing. I want to make sure what we are enabling is useful going forward for interop. Consider if the argument was for this work or first class support for Rust, Swift, or Python interop. Would we enable this functionality or are those other areas worth investing in more fully rather than providing functionality for a platform that may lose market share over the next 5 to 10 years? I am hypothesizing that COM-style will continue to lose market share, but I don't see that API style growing much.

I think the comparison between this issue and first class support for another language interop is a poor comparison. Once my current outstanding PRs are merged, the amount of work to implement and test this support is on the scale of #46343 or #46401 excluding the libraries tests, both of which I got the majority of the work done combined within less than a single week. All of the work I've done around calling conventions, return buffers, and ThisCall covers most of the work required to easily implement these calling conventions. And the testing could be entirely covered by a copy of the ThisCallTest test suite using each of these calling conventions in place of __thiscall.

@jkoritzinsky
Copy link
Member Author

After some offline discussions, here's what @AaronRobinsonMSFT and I discussed:

The argument around how each source generator would have to actively handle the new CLong, CULong, NFloat, SIMD, etc. types and any new types that need to opt out of this calling convention modification if we don't introduce CallConvMemberFunction is a compelling argument. Requiring each source generator to know every type we special case and to special case those types themselves in their source generator the same way is undesirable and a poor user experience.

There's concern around the terms "instance method" or "member function" possibly implying more support for C++ interop than just this calling convention modification. We discussed another possible alternative name (which I will add to the proposal at the top) that instead of using C++ terminology, it uses very specific terminology of the mechanical calling convention change.

@jkoritzinsky jkoritzinsky added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 11, 2021
@AaronRobinsonMSFT
Copy link
Member

doesn't use C++ terminology so as to avoid implying more fully-featured C++ interop support: CallConvStructReturnBuffer

I am supportive of this proposed modifier name. Thank you for the clarifying the intrinsic type issue (e.g. CLong) and the new name option.

@tannergooding
Copy link
Member

tannergooding commented Jan 11, 2021

I think I've got two more questions.

  1. This only covers function pointers. However, it is possible to bind against a C++ export using DllImport. Is the current plan that you would be required to use function pointers to bind against such functions and get the right default behavior? (I think doing so is 100% ok, just want to confirm).
  2. Users can currently specify calling conventions as part of UnmanagedCallersOnly. What is the behavior for such a function annotated with this member?

Also, I know I said earlier there shouldn't need to be any changes from the language side. But rethinking, it might be worth a sanity check on whether this would need any considerations around the compiler supporting explicit this and thiscall in the future and how they might handle it.

I'd hope that any such considerations would largely only impact managed function pointers, but it's probably worth double-checking and possibly having an IL test on how this works if the user has something with the explicit this modifiers.

@jkotas
Copy link
Member

jkotas commented Jan 11, 2021

CallConvStructReturnBuffer

I do not think that this is a good name. It talks about what this calling convention does at very low-level, on Windows. This name does not reflect what this calling convention does on Unix.

Whether you like it or not, this calling convention is specific to (Windows) C++. I do not see a problem with implying that it is about C++. I would be even fine with calling it CallConvCppMemberFunction.

@AaronRobinsonMSFT
Copy link
Member

this calling convention is specific to (Windows) C++

Okay. I would then prefer CallConvInstanceMethod or CallConvMemberFunction.

@jkoritzinsky
Copy link
Member Author

I think I've got two more questions.

  1. This only covers function pointers. However, it is possible to bind against a C++ export using DllImport. Is the current plan that you would be required to use function pointers to bind against such functions and get the right default behavior? (I think doing so is 100% ok, just want to confirm).

Yes, this would require you to use function pointers to use calling conventions with this modifier.

  1. Users can currently specify calling conventions as part of UnmanagedCallersOnly. What is the behavior for such a function annotated with this member?

A function annotated with this member will have its signature treated similarly as if the signature was on a function pointer. Meaning that calling it would be just like calling a native method with the same calling convention (as per the design of UnmanagedCallersOnly).

Also, I know I said earlier there shouldn't need to be any changes from the language side. But rethinking, it might be worth a sanity check on whether this would need any considerations around the compiler supporting explicit this and thiscall in the future and how they might handle it.

I'd hope that any such considerations would largely only impact managed function pointers, but it's probably worth double-checking and possibly having an IL test on how this works if the user has something with the explicit this modifiers.

In any case, the JIT would do something like the following (which matches the current behavior for thiscall)

Managed this pointer, Native "thisPtr" user arg, Return Buffer, remaining user args

However, since the mod-opt calling conventions are only supported on unmanaged function pointers and explicit this isn't applied except with a managed function pointer, this is a non-issue. And since UnmanagedCallersOnly needs to be a static method, it's not relevant there either.

@lambdageek
Copy link
Member

lambdageek commented Jan 12, 2021

Should we have a RuntimeFeature boolean property to indicate that the current runtime supports this feature?

@jkotas
Copy link
Member

jkotas commented Jan 12, 2021

The support is indicated by existence of the CallConvMemberFunction type.

@bartonjs
Copy link
Member

bartonjs commented Jan 15, 2021

Video

Looks good as proposed.

namespace System.Runtime.CompilerServices
{
    public class CallConvMemberFunction
    {
    }
}

@jkoritzinsky
Copy link
Member Author

Closing this issue as the Mono work is tracked by #50440

@ghost ghost locked as resolved and limited conversation to collaborators May 30, 2021
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

No branches or pull requests

7 participants