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

Consider LibraryImport for PInvoke generation #6217

Closed
1 task done
kant2002 opened this issue Nov 24, 2021 · 13 comments
Closed
1 task done

Consider LibraryImport for PInvoke generation #6217

kant2002 opened this issue Nov 24, 2021 · 13 comments
Assignees
Labels
api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation area-COM area-ILLinker/AOT ⛔ blocked Blocked by external dependencies

Comments

@kant2002
Copy link
Contributor

kant2002 commented Nov 24, 2021

The LibraryImportGenerator experiment lands .NET 7 and provide slightly better performance with high compatibility bar. Also LibraryImportGenerator probably would be productized for .NET 7 dotnet/runtime#60595

.NET Team itself embrace it in core .NET - See https://github.com/dotnet/runtime/issues?q=is%3Aissue+is%3Aopen+LibraryImport

Unfortunately COM marshalling does not supported, but given that ComWrappers should be done here, it's probably would not be a problem.

Open issues:

@kant2002 kant2002 added the api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation label Nov 24, 2021
@kant2002 kant2002 changed the title Consider GeneratedDllImport for PInvoke generation Consider LibraryImport for PInvoke generation Apr 27, 2022
@AraHaan
Copy link
Member

AraHaan commented Apr 27, 2022

I think it should wait until it actually gets shipped and if it can add support for com marshalling then use it to avoid possibly breaking changes.

@kant2002
Copy link
Contributor Author

kant2002 commented Apr 27, 2022

I create issue for exploration. but I want to note, that Com Marshalling does not applicable. .NET 7 should work with NativeAOT/disabled built-in COM without bandaid required for previous versions.

Given that Preview 5 would have LibraryImportGenerator I think I can start playing with it here.

@RussKie RussKie added area-COM area-ILLinker/AOT ⛔ blocked Blocked by external dependencies labels May 3, 2022
@RussKie RussKie added this to the .NET 7.0 milestone May 3, 2022
@kant2002
Copy link
Contributor Author

First work is started in #7172

@AraHaan
Copy link
Member

AraHaan commented May 14, 2022

I think Winforms will still need a way to use COM then when this happens. Perhaps one should ask the maintainers of the generator to add COM marshaling support so that way there won't be any possible regressions in winforms.

@RussKie
Copy link
Member

RussKie commented May 15, 2022

/cc: @AaronRobinsonMSFT for visibility

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented May 15, 2022

I think Winforms will still need a way to use COM then when this happens.

WinForms can start to consume LibraryImport now and that would be much appreciated. We started doing this in dotnet/aspnetcore#41573 and found some deficiencies so it is possible WinForms will also find issues we should address.

Perhaps one should ask the maintainers of the generator to add COM marshaling support so that way there won't be any possible regressions in winforms.

This is on deck - see dotnet/runtime#66674.

/cc @dotnet/interop-contrib

@kant2002
Copy link
Contributor Author

For the record LibraryImport work started #7172

based on my previous attempts we need a lot of structure marshalers. What’s recommendations for dealing with structures? There a lot of them in WinForms and writing all of marshalers would be boring. Can simple marshalers be generated for us for this case?

@AaronRobinsonMSFT
Copy link
Member

Can simple marshalers be generated for us for this case?

That is something we have discussed. We are still looking for the justification of cost though. Knowing how many marshallers would need to be written manually in WinForms coupled with a few examples would be helpful in understanding the benefit.

@kant2002
Copy link
Contributor Author

Okay. I wrote a small tool which analyze System.Windows.Forms.Primitives where most DllImport lives.

Methods analysis:

=== Total DllImport methods: 447 ===
=== Methods with unmanaged parameters : 252 ===
=== Print marshal with batteries methods (String,HandleRef) : 18 ===

=== Additional code required for methods below  ===

=== Print unclassified methods : 0 ===
=== Print string builder methods : 6 ===
=== Print class methods : 17 ===
=== Print com interface methods : 23 ===
=== Print managed struct methods : 10 ===
=== Print unmanaged struct methods : 137 ===

And types analysis

======== Type marshal reasons =========
ComInterface Interop+Ole32+IClassFactory2
ComInterface Interop+Ole32+ILockBytes
ComInterface System.Runtime.InteropServices.ComTypes.IDataObject
ComInterface Interop+Ole32+IDropTarget
ComInterface Interop+Ole32+IStorage
ComInterface System.Runtime.InteropServices.ComTypes.ITypeLib
ComInterface Interop+Ole32+IFont
ComInterface Interop+Ole32+IFontDisp
ComInterface Interop+Oleaut32+IRecordInfo
ComInterface Interop+UiaCore+IRawElementProviderSimple
ComInterface Interop+Ole32+IStream
Class System.Windows.Forms.NativeMethods+PRINTDLGEX
Class System.Windows.Forms.NativeMethods+OPENFILENAME_I
Class System.Object
Class System.Byte[]
Class Microsoft.Win32.SafeHandles.CoTaskMemSafeHandle
Class System.Int32[]
UnmanagedStruct System.Guid
UnmanagedStruct Interop+Gdi32+HDC
UnmanagedStruct System.Drawing.Point
UnmanagedStruct System.Numerics.Matrix3x2
UnmanagedStruct Interop+RECT
UnmanagedStruct Interop+Gdi32+HRGN
UnmanagedStruct Interop+Gdi32+HGDIOBJ
UnmanagedStruct Interop+Gdi32+HPALETTE
UnmanagedStruct Interop+COLORREF
UnmanagedStruct Interop+Gdi32+HBITMAP
UnmanagedStruct System.Drawing.Size
UnmanagedStruct Interop+Gdi32+HENHMETAFILE
UnmanagedStruct Interop+Gdi32+HBRUSH
UnmanagedStruct Interop+Gdi32+HFONT
UnmanagedStruct Interop+Gdi32+HPEN
UnmanagedStruct Interop+Kernel32+LCID
UnmanagedStruct Interop+Atom
ManagedStruct Interop+Comdlg32+CHOOSECOLORW
ManagedStruct Interop+Comdlg32+CHOOSEFONTW
ManagedStruct Interop+Comdlg32+PAGESETUPDLGW
ManagedStruct Interop+Gdi32+BITMAPINFO
ManagedStruct System.Runtime.InteropServices.ComTypes.STGMEDIUM
ManagedStruct Interop+Oleaut32+FONTDESC
ManagedStruct Interop+Shell32+BROWSEINFO
ManagedStruct Interop+User32+DEVMODEW
ManagedDelegate Interop+Gdi32+Enhmfenumproc
ManagedDelegate Interop+User32+HOOKPROC
ManagedDelegate Interop+User32+MONITORENUMPROC

I would not claim that I copy all rules for LibraryImportGenerator, but seems to be I'm pretty close.

Not sure about which scenario runtime team think to implement, but I think it was what I call UnmanagedStruct (in the docs they are called Transparent Structures. In WinForms codebase there only 16 types like this, and even this is mundane, it seems manageable to implement. I overestimate complexity initially. Anyway, would be good do nothing in this cases.

Also not sure should DisableRuntimeMarshallingAttribute be applied for marshaling Transparent Structures.

Applying DisableRuntimeMarshallingAttribute should be last step, as it is require ComWrappers to be fully implemented across codebase. It is not clear from documentation, would it be possible to introduce changes piece meal or not.

It is not clear from documentation what is considered not strictly blittable. I do not get what's the difference between blittable, except all strictly blittable are should originate from same assembly where source generator runs. Please correct me.

Strictly blittable types seems to be a way how to postpone applying DisableRuntimeMarshallingAttribute at least in some parts. Is this correct?

/cc @gpetrou because you are driving changes in that direction now.

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented May 15, 2022

Applying DisableRuntimeMarshallingAttribute should be last step

Right.

I do not get what's the difference between blittable, except all strictly blittable

The idea is basically the old definition but limited to value types, classes marked sequential are not blittable, and must be made up of types that are either all defined in the same assembly or of always blittable primitives.

Strictly blittable types seems to be a way how to postpone applying DisableRuntimeMarshallingAttribute at least in some parts. Is this correct?

Sort of. There are a number of cases where DisableRuntimeMarshallingAttribute will cause a lot of complexity to change and although it might be worth it in the long run, it isn't needed for the majority of cases. One could think of it as a transition tool, but disabling runtime marshalling across the board because one needs to marshal something with two short fields shouldn't be the forcing function.

In WinForms codebase there only 16 types like this, and even this is mundane, it seems manageable to implement. I overestimate complexity initially. Anyway, would be good do nothing in this cases.

Right. This was our assumption. Generating marshallers for these types is expected to be rather simple and helps owners to be more active in the interop space rather than keeping it a mystery. If someone wants to perform some non-trivial interop they should be expected to participate at least a little.

@merriemcgaw merriemcgaw modified the milestones: .NET 7.0, .NET 8.0 Aug 10, 2022
@elachlan
Copy link
Contributor

@merriemcgaw is this still valid with the move to using CsWin32?

@AaronRobinsonMSFT
Copy link
Member

/cc @JeremyKuhne

@JeremyKuhne
Copy link
Member

Yes, this is no longer relevant. Thanks for pointing it out, @elachlan.

@ghost ghost removed this from the .NET 8.0 milestone Jan 18, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation area-COM area-ILLinker/AOT ⛔ blocked Blocked by external dependencies
Projects
None yet
Development

No branches or pull requests

7 participants