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

Source generation of ILStub for DllImport #43060

Closed
50 tasks done
AaronRobinsonMSFT opened this issue Oct 5, 2020 · 23 comments
Closed
50 tasks done

Source generation of ILStub for DllImport #43060

AaronRobinsonMSFT opened this issue Oct 5, 2020 · 23 comments
Labels
area-System.Runtime.InteropServices tracking This issue is tracking the completion of other related issues.
Milestone

Comments

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Oct 5, 2020

The Roslyn source generator feature has created an opportunity for the Interop team to consider the generation of the P/Invoke ILStub at compile time instead of during run time. A proposal for this work can be found here.

Prototyping is occurring in the dotnet/runtimelab.

Goals

  • Remove run time overhead of ILStub generation.
  • No observable degradation in performance of replaced P/Invoke calls.
  • Generated code is semantically equivalent to build-in ILStub code gen.
  • Support >= 80 % of P/Invoke patterns in NetCoreApp.
    • Stretch goal is to support 100 % in NetCoreApp.
  • Cross-platform support.
  • Ability to innovate in support of modern .NET types (e.g., ReadOnlySpan<T>/Span<T>).
  • Analyzer to ensure consumption success.
  • Have a scenario agnostic code generation (Roslyn APIs) library for marshalling.
  • Testable
    • Ability to create comprehensive test matrix.
    • High confidence in generated code through quantifiable unit testing metrics not "most common scenario" testing.

Non-Goals

  • Support P/Invoke signatures not found in NetCoreApp.
  • Alter built-in ILStub generation.
  • Support COM interop.

Prototype exit criteria

  • Proposal for new runtime APIs - see list below.
  • Source generator and Analyzer in NuGet package that can be consumed by NetCoreApp in a local build.
    • 80 % of P/Invokes in NetCoreApp can be updated with confidence.
    • Accounting of potential tenet deficiencies (e.g., performance, functional failures, semantic inconsistencies with built-in system).
  • Add one high-value marshaller that does not exist today (e.g., Span<T>). Implement marshallers for Span and ReadOnlySpan runtimelab#1222
  • Tooling to examine .NET assembly and indicate issues relying on source generator.
  • Comprehensive unit test and integration matrix for all supported scenarios.
  • Design documents contained within designs/ folder in repository.
  • Determine the size impact of the generated code on NetCoreApp
    • ~550kB (SPCL is not entirely converted)
  • Validate performance impact to IDE scenarios for projects in NetCoreApp. See Disable EventSource generator in design-time builds #50741 for concern.
  • Create a plan for transitioning from the prototype to a shipping release - Plan for .NET 7 integration runtimelab#1378.
  • Share a local build of NetCoreApp using the source generator prototype with stakeholders.

Source generator prototype

All work is current taking place in dotnet/runtimelab.

New .NET APIs

New APIs that will need to be introduced - remember to consider Linker semantics:

All above APIs have inefficient but semantically correct implementations in the Ancillary.Interop project.

Once prototyping has completed the above APIs will go through an official API review in dotnet/runtime.

@AaronRobinsonMSFT AaronRobinsonMSFT added Epic Groups multiple user stories. Can be grouped under a theme. area-Interop-coreclr labels Oct 5, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 6.0.0 milestone Oct 5, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Oct 5, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Oct 5, 2020
@AaronRobinsonMSFT
Copy link
Member Author

/cc @lambdageek @marek-safar @vargaz

@jkotas
Copy link
Member

jkotas commented Oct 5, 2020

Would it make sense to add "Enable evolution of PInvoke marshalling" to the list of goals?

If yes, it would be nice to have "Add one high-value marshaller that does not exist today" in the exit criteria list. My pick would be marshaller for ReadOnlySpan/Span<T>. It would simplify quite a bit of code under src/libraries in this repo.

@AaronRobinsonMSFT
Copy link
Member Author

Would it make sense to add "Enable evolution of PInvoke marshalling" to the list of goals?

Doh! That was the section I couldn't think of but knew I was missing something - innovation. Yes, that should be added.

@lambdageek
Copy link
Member

Interesting!

Thanks for the nice example in the design doc. To make sure I followed completely - the runtime will still generate a P/Invoke wrapper, it's just that we're guaranteed to only see blittable types? @marek-safar this dovetails nicely with the discussion we had about partitioning marshal.c to remove fancy marshalling --- on some targets that won't need it --- in order to get the runtime size down.

From the FAQ:

Can the above API be used to provide a reverse P/Invoke stub?
No. Reverse P/Invoke invocation is performed via a delegate and integrating this proposal would prove difficult.

😞 so we can only get rid of half of marshal-ilgen.c

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented Oct 6, 2020

the runtime will still generate a P/Invoke wrapper, it's just that we're guaranteed to only see blittable types?

Right. Presently there is a level of indirection between the managed invocation and the native call - typically called an IL Stub. The IL Stub marshals the input types to appropriate blittable types and then makes the native call via calli. When an IL Stub isn't needed the CoreCLR JIT optimizes that scenario and inlines the IL Stub and passes the arguments straight through to the native function call.

In the proposed model the IL Stub is generated at compile time and a P/Invoke with all blittable types is created. We are relying on the JIT's optimization of inlining P/Invoke calls with all blittable types. This inlining optimization is typically only available on Release builds.

There are several nice things about this approach, but one of the ones I accidentally discovered was that this model permits inlining in more cases. The following signature void(int, ref int) can't be inlined with the built-in approach but in this model it can be inlined - yay!

this dovetails nicely with the discussion we had about partitioning marshal.c to remove fancy marshalling --- on some targets that won't need it --- in order to get the runtime size down.

Nice to hear. CoreCLR has no plans on changing any aspect of the built-in system in response to this work. This model is interesting and being explored primarily for AOT and innovation possibilities.

😞 so we can only get rid of half of marshal-ilgen.c

Generating a Reverse P/Invoke stub is possible using this methodology in some scenarios - consider callbacks being passed to native - one could imagine a solution there. The other type of Reverse P/Invoke would be the native export of a manage function - this is probably the one being alluded to here. Utilizing the code generation approach it should be possible to extend the DNNE project to insert the generated marshalling. That avenue will need to be explored at a later date though.

@jkotas
Copy link
Member

jkotas commented Oct 6, 2020

We should discourage use of delegates for reverse P/Invoke and encourage use of function pointer instead Maybe the analyzer that comes with this should be emitting warnings for delegate marshaling?

@AaronRobinsonMSFT
Copy link
Member Author

Maybe the analyzer that comes with this should be emitting warnings for delegate marshaling?

@jkoritzinsky We should consider this after we ensure we have symmetry with the current system. Perhaps we could defer creating a Delegate marshaller entirely for now?

@marek-safar
Copy link
Contributor

Nice to see this moving forward! A few questions

  • I'm not sure I understand "Support >= 80 % of P/Invoke patterns in NetCoreApp" goal? What is NetCoreApp in this context?
  • What is the size impact of this change?
  • How is it going to work with [DllImport("__Internal")]? /cc @lambdageek
  • Is this approach going to respect attributes like MarshalAs ?
  • Nit: the label should perhaps be area-Interop because it's not CoreCLR specific, right?

@lambdageek I think on platforms we need to cut the size we can rely on the analyzer to block reverse P/Invoke.

@HurricanKai
Copy link
Member

I've done this with the Silk.NET Marshalling system, see SilkTouch could be of use if this is eventually implemented (we don't use P/Invoke under the hood, on .net core 3.0+ we instead use NativeLibrary and function pointers for the native calls).
I'd be interested what the estimated impact on IL size of this is. We've seen some impact, so if there are planned strategies on how to mitigate this, I'd be very interested (note: work on this hasn't completed, we are looking into minifying names where possible, and will look how critical IL size is after that)
What's the idea regarding IDE experience? Source generators are unfortunately forced to participate in the IDE experience (right now, I've heard this will change in the future). Which is one of our largest current problems, it's not really ideal that marshalling code is rebuilt every keypress.
How will this work regarding being valid C#? will a method be generated, which will be called by the runtime for marshalling (not sure how this would work, since generally there is both setup and cleanup code)?

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented Oct 6, 2020

I'm not sure I understand "Support >= 80 % of P/Invoke patterns in NetCoreApp" goal? What is NetCoreApp in this context?

NetCoreApp is the base framework.

What is the size impact of this change?

That is mentioned in the exit criteria as something we need to track.

How is it going to work with [DllImport("__Internal")]? /cc @lambdageek

This should have no impact on that as defined. If this changes to GeneratedDllImport then it would probably "just work" since the IL Stub would be source generated and then dispatched to a [DllImport("__Internal")] version that contained only blittable types.

Is this approach going to respect attributes like MarshalAs ?

Yes. This is intended as a drop in replacement for DllImport for any signatures contained in the NetCoreApp framework. Going forward as we handle more scenarios in the wild some features of MarshalAs may not be supported however. None of that is defined, but one can imagine that the IUnknown option wouldn't be supported and may never be supported. That will be driven by community feedback though.

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented Oct 6, 2020

Which is one of our largest current problems, it's not really ideal that marshalling code is rebuilt every keypress.

Great feedback on source generators - @chsienki any thoughts here?

How will this work regarding being valid C#? will a method be generated, which will be called by the runtime for marshalling (not sure how this would work, since generally there is both setup and cleanup code)?

All generated code is going to be valid C#. The only issue with previous tooling solutions not being able to have pure C# was calli and ldftn support. Both of those can now be expressed in C# so the entire IL Stub can be generated in C# and passed directly to the Roslyn compiler.

@tannergooding
Copy link
Member

I think, somewhat touching on what @HurricanKai was discussing, there are scenarios where you can't use DllImport such as with vkGetInstanceProcAddr which is used to resolve various optional extensions.

Does this also cover generating wrappers for function pointers? Would it alternatively be possible to allow hooking into export resolution much as we all hooking into library resolution via NativeLibrary.SetDllImportResolver?

@AaronRobinsonMSFT
Copy link
Member Author

Does this also cover generating wrappers for function pointers?

This sounds like having the ability to create an IL Stub that can be used outside of DllImport? If so that would be the symmetric part of the Reverse P/Invoke scenario I mentioned at #43060 (comment). Something to consider for the future but orthogonal to this work to handle the DllImport side of things. We are designing the code generation side of this to be agnostic of DllImport scenarios - think of it like a marshalling code gen library. A new source generator for these scenarios would be able to reuse the code generator for the marshalling code.

Would it alternatively be possible to allow hooking into export resolution much as we all hooking into library resolution via NativeLibrary.SetDllImportResolver?

This will continue to work with this current model. We are using the existing model of export discovery so all these callbacks will continue to be respected.

@Perksey
Copy link
Member

Perksey commented Oct 6, 2020

+1 on Tanner's comment - At Silk.NET we have to maintain and swap out different native function tables for each Vulkan instance and device. OpenXR has similar restrictions, not to mention OpenGL's platform-specific loading strategies.

NativeLibrary has helped with this in that we can control what function pointers P/Invoke is fed and we use NativeLibrary where available, but until .NET 6 Silk.NET is restricted to .NET Standard because of mobile support, so we have to manually load in using libdl or kernel32 on non-.NET Core platforms.

Today's challenges is P/Invoke simply doesn't offer the level of freedom advanced scenarios such as these require, where the underlying native function addresses are context-dependent (this is why Silk.NET exposes its native functions through instances instead of static functions) and need to be directly accessed in some cases (a la function pointers)

Massively thankful to Kai for championing SilkTouch and thankful to the C# LDM and Roslyn teams for making SilkTouch possible - our solution before this was a much slower System.Reflection.Emit-based approach (called SuperInvoke) in order to wield calli and do the marshalling we needed.

@tannergooding
Copy link
Member

This will continue to work with this current model. We are using the existing model of export discovery so all these callbacks will continue to be respected.

I was asking if it would be possible to expose a hook for resolving a DllImport function (as I believe the current API only works for resolving the library). This would allow, for example:

[DllImport("vulkan", ExactSpelling = true)]
public static extern VkResult vkCreateDebugReportCallbackEXT(IntPtr instance, VkDebugReportCallbackCreateInfoEXT* pCreateInfo, VkAllocationCallbacks* pAllocator, ulong* pCallback);

Where vkCreateDebugReportCallbackEXT isn't part of the standard library exports (it can't be resolved via GetProcAddress) and must be resolved via vkGetInstanceProcAddr instead.

This would, for example, allow the general DllImport logic to work with the above scenario and would only exclude things like callbacks.

@jkotas
Copy link
Member

jkotas commented Oct 6, 2020

I was asking if it would be possible to expose a hook for resolving a DllImport function

You can do it using function pointers today. It is a niche case - it was discussed before. I do not think it is worth the complexity.

@tannergooding
Copy link
Member

Fair enough, that just relegates it to potentially future source generator support around function pointers.

@AaronRobinsonMSFT
Copy link
Member Author

Fair enough, that just relegates it to potentially future source generator support around function pointers.

Yes. That is one of the reasons we will be ensuring a final marshalling code library is DllImport agnostic.

@jkoritzinsky
Copy link
Member

Maybe the analyzer that comes with this should be emitting warnings for delegate marshaling?

@jkoritzinsky We should consider this after we ensure we have symmetry with the current system. Perhaps we could defer creating a Delegate marshaller entirely for now?

Since a very large portion of P/Invokes in Microsoft.NETCore.App use delegate callbacks and we've already added the support for Delegate marshalling, I suggest we keep it, at least for v1 where we're trying to do a drop-in replacement for DllImport.

I wouldn't be against emitting a warning, although I think we should hold off on that until we have some sort of reverse P/Invoke generation.

@jkotas
Copy link
Member

jkotas commented Oct 6, 2020

Since a very large portion of P/Invokes in Microsoft.NETCore.App use delegate callbacks

FWIW, the number of P/Invokes in Microsoft.NETCore.App that use delegate callbacks is relatively small, and I plan to convert majority of them to use function pointers now that we have the Roslyn with the final syntax.

@AaronRobinsonMSFT AaronRobinsonMSFT removed the Epic Groups multiple user stories. Can be grouped under a theme. label Oct 19, 2020
@benaadams
Copy link
Member

Will this cover the ownership transfer for SafeHandles

// Release on failure
using (safeHandle)
{
    if (nativeCall(safeHandle))
    {
        // The handle ownership has been transferred; don't release.
        safeHandle.SetHandleAsInvalid();
    }
}

As used in

using (SafeX509Handle certHandle = Interop.Crypto.X509UpRef(certPtr))
{
if (!Interop.Crypto.PushX509StackField(publicCerts, certHandle))
{
throw Interop.Crypto.CreateOpenSslCryptographicException();
}
// The handle ownership has been transferred into the STACK_OF(X509).
certHandle.SetHandleAsInvalid();
}

using (SafeX509Handle upref = Interop.Crypto.X509UpRef(_leafHandle))
{
Interop.Crypto.PushX509StackField(untrusted, upref);
// Ownership moved to the stack
upref.SetHandleAsInvalid();
}

if (!SslAddExtraChainCert(sslContext, dupCertHandle))
{
Crypto.ErrClearError();
dupCertHandle.Dispose(); // we still own the safe handle; clean it up
return false;
}
dupCertHandle.SetHandleAsInvalid(); // ownership has been transferred to sslHandle; do not free via this safe handle

using (SafeX509Handle tmp = Interop.Crypto.X509UpRef(cert))
{
if (!Interop.Crypto.PushX509StackField(stack, tmp))
{
throw Interop.Crypto.CreateOpenSslCryptographicException();
}
// Ownership was transferred to the cert stack.
tmp.SetHandleAsInvalid();
}

using (SafeX509Handle tmp = Interop.Crypto.X509UpRef(pal.Handle))
{
if (!Interop.Crypto.PushX509StackField(intermedStore, tmp))
{
throw Interop.Crypto.CreateOpenSslCryptographicException();
}
// The ownership has been transferred to the stack
tmp.SetHandleAsInvalid();
}

etc

@jkoritzinsky
Copy link
Member

@benaadams I've taken another look and I believe that a marshaler that handles that case (SafeHandle either transferred ownership or released) would be possible for a developer to write with the source generator's custom marshalling model.

@GavinRay97
Copy link

GavinRay97 commented Apr 15, 2021

Based on this comment:

Utilizing the code generation approach it should be possible to extend the DNNE project to insert the generated marshalling. That avenue will need to be explored at a later date though.

This would mean that code like this, would be able to be simplified to what look like managed types on the surface, and the marshalling and forced conversion/check for blittability (is this a word? blittableness? 😅) would happen under the hood?

https://gist.github.com/GavinRay97/b3e92ca3245319eb358e4a882ab5c942

Though this would be more of a [DllExport] than a [DllImport] scenario

.NET 6.0 automation moved this from Proposed to Completed Aug 6, 2021
Interop-CoreCLR 6.0 automation moved this from Epics to Done Aug 6, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.InteropServices tracking This issue is tracking the completion of other related issues.
Projects
.NET 6.0
  
Completed
Development

No branches or pull requests