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

Add struct marshalling design doc. #45

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 127 additions & 0 deletions DllImportGenerator/designs/StructMarshalling.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
# Struct Marshalling

As part of the new source-generated direction for .NET Interop, we are looking at various options for supporting marshaling user-defined struct types.

These types pose an interesting problem for a number of reasons listed below. With a few constraints, I believe we can create a system that will enable users to use their own user-defined types and pass them by-value to native code.

## Problems

- Unmanaged vs Blittable
- The C# language (and Roslyn) do not have a concept of "blittable types". It only has the concept of "unmanaged types", which is similar to blittable, but differs for `bool`s and `char`s. `bool` and `char` types are "unmanaged", but are never (in the case of `bool`), or only sometimes (in the case of `char`) blittable. As a result, we cannot use the "is this type unmanaged" check in Roslyn for structures.
- Limited type information in ref assemblies.
- In the ref assemblies generated by dotnet/runtime, we save space and prevent users from relying on private implementation details of structures by emitting limited information about their fields. Structures that have at least one non-object field are given a private `int` field, and structures that have at least one field that transitively contains an object are given one private `object`-typed field. As a result, we do not have full type information at code-generation time for any structures defined in the BCL when compiling a library that uses the ref assemblies.
- Private reflection
- Even when we do have information about all of the fields, we can't emit code that references them if they are private, so we would have to emit unsafe code and calculate offsets manually to support marshaling them.

## Possible Solution A - Opt-In Structure Interop

We've been working around another problem for a while in the runtime-integrated interop design: The user can use any type that is non-auto layout and has fields that can be marshaled in interop. This has lead to various issues where types that were not intended for interop usage became usable and then we couldn't update their behavior to be special-cased since users may have been relying on the generic behavior (`Span<T>`, `Vector<T>` come to mind).

I propose an opt-in design where the owner of a struct has to explicitly opt-in to usage for interop. This enables our team to add special support as desired for various types such as `Span<T>` while also avoiding the private reflection and limited type information issues mentioned above.

This design would use three attributes:

```csharp

public class GeneratedMarshallingAttribute : Attribute {}

public class BlittableTypeAttribute : Attribute {}

public class NativeStructMarshallingAttribute : Attribute
{
public NativeStructMarshallingAttribute(Type nativeStructType, string managedToNativeMarshalMethod, string nativeToManagedMarshalMethod, string nativeFreeMethod) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jkotas jkotas Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

```

There are 2 usage mechanisms of these attributes.

- Usage 1, Source-generated interop:

The user can apply the `GeneratedMarshallingAttribute` to their structure `S`. The source generator will determine if the type is blittable. If it is blittable, the source generator will generate a partial definition and apply the `BlittableTypeAttribute` to the struct type `S`. Otherwise, it will generate a blittable representation of the struct and apply the `NativeStructMarshallingAttribute` and point it to the blittable representation, a method that marshals the managed value to the native value, a method that marshals a value of the native struct to a managed value, and a method that frees any native resources that were allocated for the native value. The blittable representation can either be generated as a separate top-level type or as a nested type on `S`.
jkoritzinsky marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


- Usage 2, Manual interop:

The user may want to manually mark their types as marshalable in this system due to specific restrictions in their code base around marshaling specific types that the source generator does not account for. We could also use this internally to support custom types in source instead of in the code generator. In this scenario, the user would apply either the `BlittableTypeAttribute` or the `NativeStructMarshallingAttribute` attribute to their struct type. An analyzer would validate that the struct is blittable if the `BlittableTypeAttribute` is applied or validate that the native struct type is blittable and that the methods provided in the attribute have the correct signatures when the `NativeStructMarshallingAttribute` is applied.

The P/Invoke source generator (as well as the struct source generator when nested struct types are used) would use the `BlittableTypeAttribute` and `NativeStructMarshallingAttribute` to determine how to marshal a value type parameter or field instead of looking at the fields of the struct directly.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jkotas jkotas Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


The largest problem of this design is the switch to an opt-in model for structure types. We could augment this design by providing a fallback to the built-in struct marshaling system via an attribute (either `MarshalAsAttribute` or another one) if we believe providing an explicit escape hatch is needed.
Copy link
Member

@jkotas jkotas Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#### Why do we need `BlittableTypeAttribute`?

Based on the design above, it seems that we wouldn't need `BlittableTypeAttribute`. However, due to the ref assembly issue above in combination with the desire to enable manual interop, we need to provide a way for users to signal that a given type should be blittable and that the source generator should not generate marshalling code.

I'll give a specific example for where we need the `BlittableTypeAttribute` below. Let's take a scenario where we don't have `BlittableTypeAttribute`.

In Foo.csproj, we have the following types:

```csharp
public struct Foo
{
private bool b;
}

public struct Bar
{
private short s;
}
```

We compile these types into an assembly Foo.dll and we want to publish a package. We decide to use infrastructure similar to dotnet/runtime and produce a ref assembly. The ref assembly will have the following types:

```csharp
struct Foo
{
private int dummy;
}
struct Bar
{
private int dummy;
}
```

We package up the ref and impl assemblies and ship them in a NuGet package.

Someone else pulls down this package and writes their own struct type Baz1 and Baz2:

```csharp
struct Baz1
{
private Foo f;
}
struct Baz2
{
private Bar b;
}
```

Since the source generator only sees ref assemblies, it would think that both `Baz1` and `Baz2` are blittable, when in reality only `Baz2` is blittable. This is the ref assembly issue mentioned above. The source generator cannot trust the shape of structures in other assemblies since those types may have private implementation details hidden.

Now let's take this scenario again with `BlittableTypeAttribute`:

```csharp
[BlittableType]
public struct Foo
{
private bool b;
}

[BlittableType]
public struct Bar
{
private short s;
}
```

This time, we produce an error since Foo is not blittable. We need to either apply the `GeneratedMarshalling` attribute (to generate marshalling code) or the `NativeStructMarshalling` attribute (so provide manually written marshalling code) to Foo. This is also why we require each type used in interop to have either a `[BlittableType]` attribute or a `[NativeStructMarshalling]` attribute; we can't validate the shape of a type not defined in the current assembly because its shape may be different between its reference assembly and the runtime type.

Now there's another question: Why we can't just say that a type with `[GeneratedMarshalling]` and not `[NativeStructMarshalling]` has been considered blittable?

We don't want to require usage of `[GeneratedMarshalling]` to mark that a type is blittable because then there is no way to enforce that the type is blittable. If we require usage of `[GeneratedMarshalling]`, then we will automatically generate marshalling code if the type is not blittable. By also having the `[BlittableType]` attribute, we enable users to mark types that they want to ensure are blittable and an analyzer will validate the blittability.

Basically, the design of this feature is as follows:

At build time, the user can apply either `[GeneratedMarshallling]`, `[BlittableType]`, or `[NativeStructMarshalling]`. If they apply `[GeneratedMarshalling]`, then the source generator will run and generate marshalling code as needed and apply either `[BlittableType]` or `[NativeStructMarshalling]`. If the user manually applies `[BlittableType]` or `[NativeStructMarshalling]` instead of `[GeneratedMarshalling]`, then an analyzer validates that the type is blittable (for `[BlitttableType]`) or that the marshalling methods and types have the required shapes (for `[NativeStructMarshalling]`).

When the source generator (either Struct, P/Invoke, Reverse P/Invoke, etc.) encounters a struct type, it will look for either the `[BlittableType]` or the `[NativeStructMarshalling]` attributes to determine how to marshal the structure. If neither of these attributes are applied, then the struct cannot be passed by value.
6 changes: 5 additions & 1 deletion DllImportGenerator/readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,8 @@ Below are additional work items that are not presently captured in this reposito

### Optional

* A tool to compare the resulting IL from the generated source to that generated by the built-in IL Marshaller system. This would help with validation of what is being generated.
* A tool to compare the resulting IL from the generated source to that generated by the built-in IL Marshaller system. This would help with validation of what is being generated.

## Designs

- [Struct Marshalling](./designs/StructMarshalling.md)