Add struct marshalling design doc.

[jkoritzinsky]

Add a design doc explaining the approach we've discussed for struct marshalling.

[jkotas]

Is it worth the complexity to make the method names configurable? Can the method names follow a convention instead?

[jkoritzinsky]

As long as we make the convention well documented so people writing manual marshalling code can easily opt in to this system (and validate that they have it set up correctly via an analyzer), I am totally fine making them follow a convention.

[AaronRobinsonMSFT]

I would prefer to have an interface that needs to be implemented here or perhaps some kind of registration of function pointer contract.

Is it worth the complexity to make the method names configurable?

What complexity do you foresee with this approach?

[jkotas]

I would prefer to have an interface that needs to be implemented here or perhaps some kind of registration of function pointer contract.

Both interfaces or function pointers are extra indirections. I think they would just get into way for generating efficient marshalers.

What complexity do you foresee with this approach?

The configurable names make the sources more verbose, but I do not see the value that they add.

[AaronRobinsonMSFT]

The extra indirections are a concern for me as well. I have measured the ICustomMarshaler overheard before and am concerned about how we could improve that. I am also conflicted by success of use. I know this is an advanced feature so that argument is fair. The other side of that is longevity that I want to ensure provides some intuitive sense to adapt and grow without becoming an incrementally designed and free form mess (i.e. support matrix of sadness).

Agreed upon convention is fine but not for naming in my experience because it embeds current thinking and doesn't typically allow much room for change. I don't mind the above in the sense the supplied string represents a method to call that has a defined signature and we insert that string into a format: "{ret} = {USER_SUPPLIED}({args});". If it compiles great, otherwise the supplied function needs to be "fixed".

[AaronRobinsonMSFT]

Alright I am becoming less concerned about this. Our support assembly could contain a collection of these "NativeType"s and this would allow interop library authors the flexibility to mix/compose how they want to marshal the managed type. This scales pretty well as we can fold popular "NativeType"s into the support assembly. Is that the path you are seeing here?

[jkotas]

Yes, this can be one of the paths that this enables.

[tannergooding]

I don't think simply matching by name alone is good enough. Depending on the exact scenario you may have helper methods or other members available on a type that aren't necessarily meant for use in marshaling.

IAMDevMemoryAllocator in particular has a method named Free(), as do several other types

[tannergooding]

I think it needs to be opt-in in some fashion, likely by matching some attribute + signature to ensure that the correct thing always happens.

[jkotas]

It is opt-in by having dedicated NativeType that contains the marshalling methods. This type is different from your mainstream type, and so there should not be risk of collisions.

[jkotas]

This design assumes that there is one and only right way to marshal everything.

Maybe we should allow specifying the native type to use for marshalling in the PInvoke signature. Something like:

[DllImport(...)]
static void foo([MarshalUsing(MyNativeType)] TypeThatIDoNotOwn a);

[AaronRobinsonMSFT]

This seems to me that it would make the associated functions disjoint from the alloc/free methods implied on the NativeStructMarshallingAttribute type above. Is that intentional?

[jkotas]

I think the best motivating scenario for this is to allow marshalling of non-blittable types that are owned by somebody else (I have edited my example to reflect it.)

I expect that associated functions would be found by convention on MyNativeType in this case.

[AaronRobinsonMSFT]

Hmmm. With this argument the BlittableTypeAttribute type becomes less interesting because if the native type is the managed type that could be interpreted as "Blittable". Right?

[jkotas]

Agree. BlittableTypeAttribute can be encoded as NativeStructMarshallingAttribute pointing to self.

[jkotas]

This may not be a good idea given the method name collisions that it can introduce (see the other comment).

[jkoritzinsky]

I'd be ok with encoding blittability as a NativeStructMarshallingAttribute pointing to self since in the blittable case we wouldn't need to use the marshalling methods (and we can't for cases where we pin).

[AaronRobinsonMSFT]

This may not be a good idea given the method name collisions that it can introduce (see the other comment).

Name collisions on what? The supplied "NativeType" would be a new "transport" type simply for the moving between managed/unmanaged.

[jkotas]

I was referring to #45 (comment).

For example, let's say that we want mark the existing GCHandle type as blittable.

If it is done via [BlittableType], it is clear what it means. No ambiguity. This type is always passed through without any marshaling.

If it is done via [NativeStructMarshalling(typeof(GCHandle)], it is less clear. For example, should GCHandle.Free participate in marshaling or not, assuming Free method is part of the convention.

[AaronRobinsonMSFT]

If it is done via [NativeStructMarshalling(typeof(GCHandle)], it is less clear. For example, should GCHandle.Free participate in marshaling or not, assuming Free method is part of the convention.

Agreed that is a concern - that was part of my push back when talking about convention. However, it presupposes the need to use existing types as "Transfer types" - I like this term because it describes their purpose. For example, GCHandle isn't a transfer type because it already has a purpose and isn't something we would be likely to express on the unmanaged side of the boundary - we can debate that. If it is what we want to reflect on both side, then it is blittable so the collision wouldnt come into play.

Alternatively we could expand the concept mentioned above by merging it with @jkoritzinsky's original proposal.

struct MyNameType
{
  // Pre-existing functions on the type
  public static MyNameType Alloc() { ... }
  public static long Free() { ... }

  // Support for DllImport source generator
  // N.B. Signatures are for illustrative purposes only.
  public static IntPtr MyAlloc() { ... }
  public static void MyFree(IntPtr a) { ... }
}

static void foo([MarshalUsing(MyNativeType, Alloc = nameof(MyNameType.MyAlloc), Free = nameof(MyNameType.MyFree))] TypeThatIDoNotOwn a);

This allows us by default to have a strongly typed explicit convention or in the cases where it doesn't work the user can say "Use this instead". I genuinely like the merging of the two ideas.

[tannergooding]

What would happen if the type was marked Blittable but the type contained a non-blittable field?

[tannergooding]

The two main concerns here are:

1. Users can write their own IL
2. Users can disable analyzer warnings

In both scenarios, you could have struct S { int _field; } in the reference assembly but struct S { bool _x; } in the implementation.
There are also odd scenarios where the user changes the backing field without a dependent assembly recompiling (which would be a breaking change, but which is still something that may need consideration).

[jkoritzinsky]

If the user explicitly marks it as blittable, then the analyzer would produce an error. Otherwise, if the type is marked with [GeneratedMarshalling], it will only have the BlittableTypeAttribute attribute when it is blittable.

If someone disables the analyzer and applies BlittableTypeAttribute manually (which is very much a "don't do this"/unsupported scenario), then we will not generate any marshalling code and behavior at native boundaries would fall into unspecified behavior. (The P/Invoke source generator would currently fall back to the built-in system based on it's implementation)

[AaronRobinsonMSFT]

The two main concerns here are:

1. Users can write their own IL
2. Users can disable analyzer warnings

I think these are fair concerns, but I don't think they change much.

Anyone writing their own IL is in a very advanced scenario and this isn't something I think we can solve or address. That being said, I am very much open for making an attempt additionally I could be misunderstanding the concern here.

Disabling analyzer warnings is perfectly acceptable and that is the prerogative of any developer. If the developer knows better then the compiler/analyzer they are okay going it on their own and if the runtime crashes in an interop call that is on them. Consider the case of "use prior to initialize" in C/C++; if the warning was on the error would have been flagged and could have been corrected. Disabling the warning is on the developer.

The rebuttal to that is the case where .NET should be designed to help users avoid those mistakes and in general I agree. However, this is already an advanced scenario so I would prefer to focus on making analyzers avoid false positives for incorrect use - not noisy - and enable experts to produce high quality AOT friendly interop libraries that users can consume so very few need to actually author P/Invokes - see dotnet/pinvoke.

[tannergooding]

What is the expected extensibility model, if any, for things which can't currently be surfaced to the runtime today?

For example, the "TransparentStruct" discussion we had previously, where you want to say "I have a struct, but you should pass it like the underlying T instead" because I really just want a convenience API.

or as we start bringing onboard Java (Android) and Objective-C (iOS) support; specific ABI concepts that can't be represented today such as differentiating a C++03 POD type which may have different behavior than a non POD type.

[jkotas]

The TransparentStruct can be implemented using optional Value property on NativeType. If the source generator detected presence of Value property on the NativeType, it would use the value of this property for the inner call instead of the NativeType itself. It only works if the type is constant (ie it would not work well for CLong).

The low level ABI concepts would have to be built into the runtime, or we would need to generate a helper C/C++/Objective-C library to implement them on the unmanaged side. I do not think that this proposal helps with that.

[tannergooding]

For the functions, would there be any "new APIs" for doing low overhead malloc/free like operations? We have AllocHGlobal today, but it is fairly expensive (comparatively) and it would be nice to be able to do a simple "allocate native mem", "marshal data", "free native mem"

[jkotas]

That sounds like an independent proposal / issue to me. AllocHGlobal has some unnecessary overhead (in particular on Unix). It is something to fix first.

[jkoritzinsky]

I've updated the proposal with a few attribute renames and removing the customizability of the native marshalling method names. I've also added a description and a few examples using the "transparent struct" support Jan suggested with the Value property, which actually ended up making this design extremely powerful and versatile while adding minimal complexity.

[jkotas]

Nit: constructor + ToManaged method are asymmetric style-wise. Maybe it should be constructor + conversion operator; or FromManaged + ToManaged methods.

We can easily support multiple styles if needed.

[jkoritzinsky]

I'm unsure if I like using a conversion operator because it makes the IDE experience slightly worse and is a little harder to follow. If we go the FromManaged/ToManaged route, we'd have to decide if we want to use mutable structs and instance methods or use static methods and don't use mutable structures.