dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.96k stars 4.65k forks source link

[API Proposal]: `[LibraryExport]` analogous to `[JSExport]` #79786

Open am11 opened 1 year ago

am11 commented 1 year ago

Background and motivation

In .NET 7 we have:

From this outlook, [UnmanagedCallersOnly] name is unsymmetrical and the odd one out.

LibraryExport as attribute name would have been much cleaner, especially for AOT scenarios. Aside from the naming:

There is a room to add a dedicated LibraryExportAttribute with a single intent, similar to JSExportAttribute.

This may also give a chance for runtimes to perform targeted optimizations (reduce # of frames around these calls in coreclr) with a slight ease than it is today.

API Proposal

namespace System.Runtime.InteropServices
{
    [AttributeUsage(AttributeTargets.Method, Inherited = false)]
    public class LibraryExportAttribute : Attribute
    {
        public LibraryExportAttribute() { }
        public Type[]? CallConvs; // optional; defaults to current platform's calling convention
        public string? EntryPoint; // unlike UnmanagedCallersOnly, this is optional for native export
    }
}

API Usage

    [LibraryExport]
    public static int GetDotnetMajorVersion() => Environment.Version.Major;

test.c

#include<stdio.h>

extern int GetDotnetMajorVersion(void);

int main()
{
  printf(".NET major version is: %d\n", GetDotnetMajorVersion());
  return GetDotnetMajorVersion();
}
$ dotnet publish -r linux-x64 -p:PublishAot=true -p:NativeLib=Shared
$ cc test.c bin/Debug/net7.0/linux-x64/publish/mynativelib1.so

$ ./a.out
.NET major version is: 7

$ echo $?
7

Alternative Designs

No response

Risks

Confuse the consumers who have made their peace with UnmanagedCallersOnly(EntryPoint="mandatory_name").

ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/interop-contrib See info in area-owners.md if you want to be subscribed.

Issue Details
### Background and motivation In .NET 7 we have: * `[JSImport]` and `[JSExport]` to import and export JavaScript entrypoints * `[LibraryImport]` and `[UnmanagedCallersOnly]` to import and export native entrypoints with specified calling convention From this outlook, `[UnmanagedCallersOnly]` name is unsymmetrical and the odd one out. `LibraryExport` as attribute name would have been much cleaner, especially for AOT scenarios. Aside from the naming: * "exporting native entrypiont" is not the only use-case of `UnmanagedCallersOnlyAttribute`; it was originally added for roslyn to support `unmanaged` semantics. * it has a non-intuitive restriction for "exporting native entrypoint" that it doesn't work without the explicit `EntryPoint` value. There is a room to add a dedicated `LibraryExportAttribute` with a single intent, similar to `JSExportAttribute`. This may also give a chance for runtimes to perform targeted optimizations (reduce # of frames around these calls in coreclr) with a slight ease than it is today. ### API Proposal ```c# namespace System.Runtime.InteropServices { [AttributeUsage(AttributeTargets.Method, Inherited = false)] public class LibraryExportAttribute : Attribute { public LibraryExportAttribute() { } public Type[]? CallConvs; // optional; defaults to current platform's calling convention public string? EntryPoint; // unlike UnmanagedCallersOnly, this is optional for native export } } ``` ### API Usage ```c# [LibraryExport()] public static int GetDotnetMajorVersion() => Environment.Version.Major; ``` test.c ```c #include extern int GetDotnetMajorVersion(void); int main() { printf(".NET major version is: %d\n", GetDotnetMajorVersion()); return GetDotnetMajorVersion(); } ``` ```sh $ dotnet publish -r linux-x64 -p:PublishAot=true -p:NativeLib=Shared $ cc test.c bin/Debug/net7.0/linux-x64/publish/mynativelib1.so $ ./a.out .NET major version is: 7 $ echo $? 7 ``` ### Alternative Designs _No response_ ### Risks Confuse the consumers who have made their peace of with `UnmanagedCallersOnly(EntryPoint="mandatory_name")`.
Author: am11
Assignees: -
Labels: `api-suggestion`, `area-System.Runtime.InteropServices`, `untriaged`
Milestone: -
teo-tsirpanis commented 1 year ago

For symmetry with LibraryImport, LibraryExport should not have a CallConvs property and instead rely on the UnmanagedCallConvAttribute if exists.

Another thing to consider is to incorporate marshalling in the design of LibraryExport, and support exposing the generated blittable UnmanagedCallersOnly function to the user, so that we would be able to write something like this:

public unsafe partial class MyClass
{
    // GenerateNativeThunk to true would cause the source generator to emit a Callback_native
    // method with UnmanagedCallersOnly inside the declaring class, instead of a file class
    // to be seen by just NativeAOT.
    [LibraryExport(StringMarshalling = StringMarshalling.Utf16, GenerateNativeThunk = true)]
    private static void Callback(string s)
    {
        Console.WriteLine(s);
    }

    [LibraryImport("mylibrary")]
    private static void MyPInvoke(delegate* unmanaged<char*, void> callback);

    static void Main()
    {
        MyPInvoke(&Callback_native);
    }
}
am11 commented 1 year ago

LibraryExport should not have a CallConvs property and instead rely on the UnmanagedCallConvAttribute if exists.

Sounds reasonable. I haven't put much thought into CallConvs and source generator, but if we can seamlessly resolve the user intent in source generator without CallConvs property, that would be a preferred shape for this attribute.

Though I'm not sure if lowering LibraryExportAttribute to UnmanagedCallConvAttribute in source generator is what we would ultimately want in a long run, as these models are relatively new and runtimes can recognize LibraryExport directly (e.g. its support in Mono AOT is under development https://github.com/dotnet/runtime/issues/79377).

teo-tsirpanis commented 1 year ago

As I'm imagining it, [LibraryExport] would be the high-level equivalent of [UnmanagedCallersOnly] just like [LibraryImport] is the high-level equivalent of [DllImport]. The former would support source-generated marshalling and the latter wouldn't.

AaronRobinsonMSFT commented 1 year ago

I appreciate the symmetrically argument but I don't think this is appropriate. This is unfortunately an area where there isn't a clean separation and we would like to not introduce yet more confusion. The history here is tangled up in NativeAOT as well where we had an attribute that was similar - that is why we added the EntryPoint to UnmanagedCallersOnly. The UnmanagedCallersOnly already handles all needed scenarios, both internal and exporting and I don't see a lot of value in adding another just for symmetry sake. I will keep this on 8 for consideration but will likely close this toward the end of the release without a reason more compelling than "symmetry".

jkoritzinsky commented 1 year ago

I think this would be a good feature to add and I would put this in a similar category as #63590.

Today, we don't have a good mechanism for both providing unmanaged-callable functions and allowing marshalling without using the built-in marshalling system. This feature could be used as a counterpart to #63590 to enable implementing a fully-source-generated interop story for "delegate marshalling"-style scenarios.

Additionally, I think this feature actually fits well with our COM source-generation direction work, although possibly not the work that will make it into .NET 8 in particular. With the COM generator, we'll have support for both managed->unmanaged and unmanaged->managed stubs for implementing source-generated CCW/RCW implementations that support all of the goodness of the new source-generation marshalling model we produced in .NET 7. However, to implement a COM server (a la comhost but for source-generated COM interop) entirely with source-generated code, a developer would still have to manually write out the marshalling for any parameters in the API the COM server must implement. This would be a weird corner where we'd have a hole in our abstraction model.

AaronRobinsonMSFT commented 1 year ago

As I'm imagining it, [LibraryExport] would be the high-level equivalent of [UnmanagedCallersOnly] just like [LibraryImport] is the high-level equivalent of [DllImport]. The former would support source-generated marshalling and the latter wouldn't.

I missed this statement. Using LibraryExport as a marshalling supported entry that would then be entered into via an exported UnmanagedCallersOnly does make sense. I was missing the notion of having the marshalling code generated as the original example:

    [LibraryExport]
    public static int GetDotnetMajorVersion() => Environment.Version.Major;

would/could just be exported directly since there is no marshalling. This is similar to how when we detect a LibraryImport is fully blittable and create a forwarder DllImport that can be inlined instead of source generating anything.

    [UnmanagedCallersOnly(EntryPoint = "GetDotnetMajorVersion")]
    public static int GetDotnetMajorVersion() => GetDotnetMajorVersion();
AaronRobinsonMSFT commented 1 year ago

GenerateNativeThunk = true

@teo-tsirpanis I don't get this scenario. For the "export" scenario, under what circumstances would we not generate the UnmanagedCallersOnly?

jkoritzinsky commented 1 year ago

GenerateNativeThunk = true

@teo-tsirpanis I don't get this scenario. For the "export" scenario, under what circumstances would we not generate the UnmanagedCallersOnly?

The idea is when GenerateNativeThunk=false, the generated method is in a file-scoped class and is only exposed via a mechanism that supports projecting an UnmanagedCallersOnlyAttribute with the EntryPoint property set as a native export (i.e. DNNE, NativeAOT, MonoAOT starting in .NET 8). In this case, the user wouldn't be able to access the stub directly, but it would be exposed to native callers.

When GenerateNativeThunk=true, the generated method would be in a type that is accessible from user-written code, so the user could write &ManagedMethod_UnmanagedWrapper in their C# and get the unmanaged function pointer to the method.

I don't know if this scenario is compelling enough to provide the two different options, and I think the GenerateNativeThunk=true scenario is more compelling at first glance.

am11 commented 1 year ago

UnmanagedCallersOnly to me looks like a multi-purpose attribute; started off as a compiler support attribute and ended up supporting other scenarios like NativeAOT exports. I was particularly looking at the poor codegen around the exported entrypoints (which are probably hard to optimize with the current UnmanagedCallersOnly machinery). With LibraryExport new surface area, I was hoping if it could be designed in a way that the final codegen would shave off some of the instructions in blittable / primitive scenarios (pertaining to GC, P/Invoke transitions etc.).

I don't know if this scenario is compelling enough to provide the two different options, and I think the GenerateNativeThunk=true scenario is more compelling at first glance.

Provided generator will emit linker-friendly code, linker will drop the unused code from wrapping type for export-only usages. With that in mind, I agree that an additional switch might not be necessary.

jkotas commented 1 year ago

Where do you see inefficiencies in the existing UnmanagedCallersOnly that can be optimized? UnmanagedCallersOnly optimization should be independent on this proposal.

am11 commented 1 year ago

@jkotas, agreed. This discussion has evolved to source generator which will emit UnmanagedCallersOnly under the hood and no longer an independent / parallel API to UnmanagedCallersOnly.

Where do you see inefficiencies in the existing UnmanagedCallersOnly that can be optimized?

I was initially looking at simple exports (involving primitive constructs and constants), e.g. with:

using System.Runtime.InteropServices;
public class C
{
    [UnmanagedCallersOnly(EntryPoint = nameof(M))]
    public static int M() => 42;
}

on linux-arm64, we get:

// function con42_C__M

stp x29, x30, [sp, #-48]!
str x19, [sp, #40]
mov x29, sp
add x0, x29, #0x18
bl  0x15d40 <RhpReversePInvoke(ReversePInvokeFrame*)>
mov w19, #0x2a                      // #42
add x0, x29, #0x18
bl  0x15e14 <RhpReversePInvokeReturn(ReversePInvokeFrame*)>
mov w0, w19
ldr x19, [sp, #40]
ldp x29, x30, [sp], #48
ret

I was thinking perhaps for simple cases like this one, it is possible to skip ReversePInvokeFrame and optimize it to inlineable return 42.

AaronRobinsonMSFT commented 1 year ago

When GenerateNativeThunk=true, the generated method would be in a type that is accessible from user-written code, so the user could write &ManagedMethod_UnmanagedWrapper in their C# and get the unmanaged function pointer to the method.

I see. That seems like we are getting into more and more bespoke scenarios. However, the mechanism I believe would be simply to not set the EntryPoint field, right? If so, the cost would be quite small to provide the mechanism and avoid creating an "export". I could be mistaken but I though creating an "export" required setting the EntryPoint, so creating a file-scoped declaration wouldn't be needed.

I don't know if this scenario is compelling enough to provide the two different options, and I think the GenerateNativeThunk=true scenario is more compelling at first glance.

Agree.

I was thinking perhaps for simple cases like this one, it is possible to skip ReversePInvokeFrame and optimize it to inlineable return 42.

I think that is minor overhead all things considered. I say that because it is why the example with a blittable signature was so distracting. When factoring in the cost of marshalling non-trivial types, the transition and frames would become far less interesting relatively speaking. If we can see it under a profiler though that would be worth understanding.

The overhead you mention though is related to https://github.com/dotnet/runtime/issues/54107, in the sense of having a way to indicate an UnmanagedCallersOnly can skip boilerplate transition with something like SuppressGCTransition. This would only make sense in cases where it was a purely blittable API and didn't reference any managed memory. Perhaps there is a better way to express this though or the JIT could compute those cases.

jkotas commented 1 year ago

I agree with @AaronRobinsonMSFT that the optimizations of the reverse PInvoke transitions is same problem as #54107. It is something for JIT to do when it is safe. I do not think we would want to make it expressible in public surface.

This would only make sense in cases where it was a purely blittable API and didn't reference any managed memory.

This also includes calls to any methods, including helpers that the JIT may choose to insert or that may run as side-effect of code execution.

Also, the code where this optimization kicked in would be impossible to debug with .NET managed debuggers. .NET managed debuggers expect that the thread transitions to cooperative mode before any code runs.

jkoritzinsky commented 1 year ago

We aren't going to get to this in .NET 8, moving to .NET 9.