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

Marshal should be able to handle generic types #1685

Closed
masonwheeler opened this issue Oct 6, 2015 · 33 comments · Fixed by dotnet/runtime#103
Closed

Marshal should be able to handle generic types #1685

masonwheeler opened this issue Oct 6, 2015 · 33 comments · Fixed by dotnet/runtime#103
Labels
Milestone

Comments

@masonwheeler
Copy link

@masonwheeler masonwheeler commented Oct 6, 2015

I bet this was a big hassle back when generics were introduced, but now, it seems a bit ridiculous that Marshal can't handle generic types.

It's been considered good practice for years now to use Func<> and Action<> declarations rather than declaring named custom delegate types, but Marshal.GetDelegateForFunctionPointer doesn't allow you to follow this practice. And it should be simple to create a generic typed IntPtr-to-struct for working with native functions that take or return struct pointers, but the Marshal won't allow that either.

Is there any technical reason why Marshal can't handle generic types? Or was it just yet another case of "too much work to do in the limited time we had to get C# 2 out the door"? Because if it's the latter, ISTM it's about time we rectify that.

@jakesays-old

This comment has been minimized.

Copy link

@jakesays-old jakesays-old commented Oct 6, 2015

comment.

@yizhang82

This comment has been minimized.

Copy link

@yizhang82 yizhang82 commented Oct 29, 2015

@masonwheeler Thanks for bringing this up. At that point not supporting generics is mostly a decision to scope the work, until WinRT came along and we had to support generic interfaces and delegates in WinRT. I tend to agree that the limitations seems rather artificial (and in some cases, inconsistent), and as you've pointed out not supporting Func/Action case seems particularly inconvenient (I must admit I've run into them myself). The main work of implementing this in the runtime is hook up / manage the marshalling data structures to the generic MethodTable instances correctly, for all the scenarios that we support. I'm not an expert on type loader so I can't comment on how much work that is. I would think that a better time to implement this is when we have merged all the interop implementations in multiple runtimes (desktop CLR, CoreCLR, .NET Native) into one. It would've been much easier to implement this in code generators (MCG) and we only need to do it once :)

@masonwheeler

This comment has been minimized.

Copy link
Author

@masonwheeler masonwheeler commented Oct 29, 2015

@yizhang82 What is CoreCLR using right now? I sort of have the vague impression that it's using MCG but not everything is in place yet, but I could be wrong there...

@yizhang82

This comment has been minimized.

Copy link

@yizhang82 yizhang82 commented Oct 29, 2015

@masonwheeler CoreCLR's interop implementation is entirely in the runtime itself - COM interop (RCW/CCW), pinvoke, data marshalling, etc. The most interesting files are in runtimecallablewrapper.cpp (RCW), comcallablewrapper.cpp (CCW), mlinfo.cpp (signature parsing), ilmarshaler.cpp (marshaling code), dllimport.cpp (interop stubs), dllimportcallback.cpp (reverse pinvoke).

@xoofx

This comment has been minimized.

Copy link
Member

@xoofx xoofx commented Oct 12, 2018

Hey, bumping this old issue... running into a situation where we have quite some code in Unity that is actually passing delegate with generics to native and calling it back. As we can directly use Mono API for that, we are able to call this callback even if they have generics...
But in case we would like to use .NET CoreCLR, we would not be able to do this easily in a standard way using regular pinvoke.

So the question is: How much work is it to support generics for the various p/invoke scenarios?

cc: @jkotas

@masonwheeler

This comment has been minimized.

Copy link
Author

@masonwheeler masonwheeler commented Oct 12, 2018

@xoofx So Mono supports it, or it doesn't exactly but you're haxxoring up a workaround?

@xoofx

This comment has been minimized.

Copy link
Member

@xoofx xoofx commented Oct 12, 2018

@xoofx So Mono supports it, or it doesn't exactly but you're haxxoring up a workaround?

Yeah, mono p/invoke does not support it (actually I never tried, but I don't think), but as we are using the mono VM api from C++, we can call any C# code we want.

@xoofx

This comment has been minimized.

Copy link
Member

@xoofx xoofx commented Oct 12, 2018

Think of it as a manual reverse pinvoke where we are writing in C++ what would have to do an auto-generated reverse pinvoke callback

@masonwheeler

This comment has been minimized.

Copy link
Author

@masonwheeler masonwheeler commented Oct 12, 2018

Gah! That is something probably best not thought about! 😛

@xoofx

This comment has been minimized.

Copy link
Member

@xoofx xoofx commented Oct 12, 2018

Gah! That is something probably best not thought about! 😛

Yeah, that's why. I would like that we move out of this kind of hacks, but in order to do that, we should have a support from the regular VM. One way to solve this on our side would be to have a Roslyn compiler plugin to rewrite the code internally to non generic version... but I would prefer a standard support from the runtime for this rather than having an ugly workaround

@jkotas

This comment has been minimized.

Copy link
Member

@jkotas jkotas commented Oct 12, 2018

So the question is: How much work is it to support generics for the various p/invoke scenarios?

For blittable types, it is "just work" to support it for existing interop (not a small amount, but doable in principle). For non-blittable types, there is a lot of ambiguity in how it should even work.

@xoofx Does your case have blittable types only, or does it have arbitrary types?

@xoofx

This comment has been minimized.

Copy link
Member

@xoofx xoofx commented Oct 12, 2018

@xoofx Does your case have blittable types only, or does it have arbitrary types?

Blittable would be fine (we may not have fully this setup today, but we would have anyway)

@masonwheeler

This comment has been minimized.

Copy link
Author

@masonwheeler masonwheeler commented Oct 12, 2018

My original request was about generic delegate types, but I did have in mind using only blittable types as the arguments to the generic functions.

@jkotas

This comment has been minimized.

Copy link
Member

@jkotas jkotas commented Oct 12, 2018

BTW: For callbacks, a better solution may be NativeCallbableAttribute - #1566 . Enabling that for generic blitable types should be much easier. NativeCallbableAttribute is not really usable from C# directly today, but that's going to change with https://github.com/dotnet/csharplang/blob/master/proposals/intrinsics.md

@tannergooding

This comment has been minimized.

Copy link
Member

@tannergooding tannergooding commented Jan 27, 2019

Given that C# has now merged the unmanaged constructed types feature, would it be worth looking at enabling the same types of generic structs to be used in interop scenarios (this basically limits it to "blittable" generic structs)?

@jkotas

This comment has been minimized.

Copy link
Member

@jkotas jkotas commented Jan 27, 2019

C# "unmanaged constructed types" are not the same as interop blittable types. E.g. char is unmanaged type, but it is not blittable type.

@tannergooding

This comment has been minimized.

Copy link
Member

@tannergooding tannergooding commented Apr 10, 2019

@jkotas, is this a terribly complicated thing to support? I believe you've mentioned before that this is largely "just work", but I was wondering how much work that is...

Testing locally, it looks like changing one line lets things light up and function as expected (although I imagine I may be missing corner cases or other funcitonality). Namely, changing https://github.com/dotnet/coreclr/blob/master/src/vm/mlinfo.cpp#L2717 to if (m_pMT->HasInstantiation() && !m_pMT->IsBlittable()) allows blittable generics to be marshaled and continues throwing for non-blittable generics.

@jkotas

This comment has been minimized.

Copy link
Member

@jkotas jkotas commented Apr 10, 2019

It probably does not require too many changes; however there is very likely non-trivial bug tail associated with it. The build-in runtime interop always comes with lot of surprises.

@jkoritzinsky should be able to help you with this. He is pretty familiar with all the interesting corner cases of the built-in interop.

cc @AaronRobinsonMSFT @jeffschwMSFT

@tannergooding

This comment has been minimized.

Copy link
Member

@tannergooding tannergooding commented Apr 10, 2019

@jkoritzinsky, if you are interested; I started prototyping this here: https://github.com/tannergooding/coreclr/tree/marshal-generic

I also looked at all the places that currently use IDS_EE_BADMARSHAL_GENERICS_RESTRICTION and, other than the place I touched, they all seem to be around generic reference types (rather than value types).

@masonwheeler

This comment has been minimized.

Copy link
Author

@masonwheeler masonwheeler commented Apr 10, 2019

Great to see some real work being done on this! 😄

@jkoritzinsky

This comment has been minimized.

Copy link
Member

@jkoritzinsky jkoritzinsky commented Apr 10, 2019

@tannergooding from a quick glance I think that the prototype you have should work for most cases, but might cause unexpected behavior in the following scenario:

If the user creates a COM object with a method with the signature void Method(KeyValuePair<K, V> pair); then the way this is marshalled will depend on the type of T as well as if the type is a WinRT type or a non-WinRT type. If it's a WinRT type, then it would be marshaled as a Windows::Foundation::Collections::IKeyValuePair<K, V>*, but otherwise it would be marshalled by-value as the structure below:

struct KeyValuePair
{
K key;
V value;
};

System.Nullable<T> doesn't have this issue because its bool field isn't blittable, but if we were to fully enable generic interop then it would be a problem.

The bigger issue with marshalling generic types comes on the fields and arrays sides of interop. Those two stacks both are primarily implemented with native code instead of as IL stubs, which makes the work of actually doing the interop more complex.

The array marshalling code is actually all built on-top of the VT_* enum from COM along with some VTHACK_* extra members and is shared between all of the different types of arrays we support, including COM SAFEARRAYs. It should be possible to add support for with some work, but we would need to verify all of the code-paths that hit that code and make sure that we're doing the right thing. We might get lucky and it'll "just work" with a small change like your current prototype, but I don't know for sure off the top of my head.

The field marshaler code (see ParseNativeType) determines how each field is marshaled by parsing the metadata signatures directly, which means that adding support for generics there is going to be annoying since we'll have to parse the generic signature and assemble the MethodTable pointer ourselves from the metadata signature.

@tannergooding

This comment has been minimized.

Copy link
Member

@tannergooding tannergooding commented Apr 10, 2019

@jkoritzinsky. Thanks for the information.

I'll move forward by writing tests covering the various scenarios here and seeing what breaks where. I'll try to explicitly cover some of the cases you gave above as well.

@AaronRobinsonMSFT

This comment has been minimized.

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Apr 11, 2019

If the user creates a COM object with a method with the signature void Method(KeyValuePair<K, V> pair);

I would prefer if we didn't pollute the COM world with this. Properly expressing these kinds of things isn't possible in COM. Support should be relegated to P/Invokes and WinRT scenarios.

@tannergooding

This comment has been minimized.

Copy link
Member

@tannergooding tannergooding commented Apr 11, 2019

Properly expressing these kinds of things isn't possible in COM

@AaronRobinsonMSFT. I'm not sure what you mean by this in particular.

Generics, in general, aren't expressible in native code. The benefit is being able to define a single blittable value type on the managed side that can be used for multiple similar signatures on the native side.

Imagine, for example, you have struct Point2F { float x; float y } and struct Point2U { uint32_t x; uint32_t y}. These structs are identical other than the type of field they contain. You could define both separately, or you could define a single Point2<T> struct and then use Point2<float> and Point2<uint> in your interop signatures.

This type of signature would be perfectly valid for both regular P/Invoke and COM signatures. For example, Direct2D is exposed using COM and has signatures in their various interfaces that take D2D_POINT_2F and sometimes corresponding overloads that take D2D_POINT_2U.

This would ultimately allow you to do the following:

// GuidAttribute
// ComInterfaceTypeAttribute
// ComInteropAttribute
public interface ID2D1RenderTarget : ID2D1Resource
{
    // ID2D1Resource method redefinitions

    // Other ID2D1RenderTarget methods

    void DrawLine(
        D2D_POINT_2<float> point0,               // D2D_POINT_2F today
        D2D_POINT_2<float> point1,               // D2D_POINT_2F today
        ID2D1Brush brush,
        float strokeWidth = 1.0f,
        ID2D1StrokeStyle strokeStyle = null
    );

    // Other ID2D1RenderTarget methods
}
@tannergooding

This comment has been minimized.

Copy link
Member

@tannergooding tannergooding commented Apr 11, 2019

I'm personally fine if we do condition it; as users will have an escape hatch when C# function pointers come around (as they can then manually define the VTBLs and just not rely on the built-in COM support); but I also don't see a reason on why we should limit it.

@AaronRobinsonMSFT

This comment has been minimized.

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Apr 11, 2019

@tannergooding My comment was made due to a misunderstanding of the issue. I thought the request was for supporting generic member functions as in: void Foo<T>(Bar<T> r);

Permitting a complete type that is entirely blittable seems okay in general. This does however require updating the documentation which I would ask be done to at the same time as the work. I spoke with @jkoritzinsky about this and he filled me in on your specific scenario - which seems reasonable and should be permitted. One of my constant concern with adding support in COM marshalling interfaces are scenarios involving IDispatch which I don't see how will be possible to support in DISPPARAMS - this is similar as with my concerns here #23230.

I admittedly haven't fully thought through all the scenarios, but that dovetails with my feeling about COM features in general. There is such a lack of testing in the COM space already in .NET Core that adding new functionality simply increases that burden and makes the interop space less well-defined.

Basically, here would be my requirements for adding this support to COM:

  • Adequate testing for P/Invokes
  • Adequate testing for IUnknown and IDispatch scenarios
  • Update official documentation for describing what scenarios are being enabled

If all three of those are met, adding support in COM is fine. Note that I am supportive of adding support only for IUnknown and not IDispatch as long as we have some tests that validate that. Otherwise we should limit this entirely to P/Invokes.

@tannergooding

This comment has been minimized.

Copy link
Member

@tannergooding tannergooding commented Apr 11, 2019

I've put up a draft PR here #23899 which includes the basic support and tests.

I've also called out some additional areas that need explicit tests covering them.

@sharwell

This comment has been minimized.

Copy link
Member

@sharwell sharwell commented Nov 22, 2019

@tannergooding Can you set the Milestone for this issue to the first runtime version that will support this?

@AaronRobinsonMSFT AaronRobinsonMSFT modified the milestones: Future, 5.0 Nov 22, 2019
@AaronRobinsonMSFT

This comment has been minimized.

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Nov 22, 2019

@sharwell Good suggestion. This will be a .NET 5 feature.

@jkotas and @tannergooding Do we have any thoughts on Mono's support matrix for this?

@jkotas

This comment has been minimized.

Copy link
Member

@jkotas jkotas commented Nov 22, 2019

@jkotas and @tannergooding Do we have any thoughts on Mono's support matrix for this?

cc @marek-safar

@marek-safar

This comment has been minimized.

Copy link

@marek-safar marek-safar commented Nov 22, 2019

I think we can implement P/Invoke support as part of .NET5 work for MonoVM.

/cc @lambdageek

@jkotas

This comment has been minimized.

Copy link
Member

@jkotas jkotas commented Nov 22, 2019

Note that we are limiting this to blittable generics only.

@lambdageek

This comment has been minimized.

Copy link
Member

@lambdageek lambdageek commented Dec 16, 2019

I tried some examples with Mono, and things like this

  struct Point<T> {
     public T x;
     public T y;
   }

  [DllImport ("...")]
  public static extern void Foo (Point<double> p);

work in Mono already.

We will have some work to do for Mono, however: things like Point<object> also "work" instead of throwing an exception.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.