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

[API Proposal]: System.Runtime.CompilerServices.RuntimeCompatibilityAttribute.DisableRuntimeMarshalling #60639

Closed
Tracked by #60595
jkoritzinsky opened this issue Oct 19, 2021 · 16 comments · Fixed by #63320
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices
Milestone

Comments

@jkoritzinsky
Copy link
Member

jkoritzinsky commented Oct 19, 2021

Background and motivation

As the interop team is working on a source-generated DllImport solution, we've decided to take an extra look at the built-in marshalling rules per feedback from @stephentoub.

In the built-in marshalling system today, the runtime looks at all member fields of a type at runtime and uses this runtime type information to determine if a type is blittable and how to marshal it. For compile-time source generators, this sort of design is untenable for performance and feasibility reasons (we need a static type in the final "native call" signature to get correct calling-convention behavior). See https://github.com/dotnet/runtime/blob/main/docs/design/libraries/DllImportGenerator/StructMarshalling.md for a more information.

A complicating factor in this scenario is that the default marshalling rules for bool and char are non-blittable. In particular, bool maps to a 4-byte Windows API BOOL, and char maps to an ANSI (based on the current code page) 1-byte character. As a result, the C# language concept of "unmanaged" is not the same as the runtime concept of "blittable". In the proposal above, we took the route of building a new attribute and analysis system to ensure that a type marked as blittable is actually blittable.

@stephentoub and @jkotas recommended that we could instead investigate changing the rules in source-generated P/Invokes to make the unmanaged and blittable concepts equivalent. To enable this mechanism at the interop boundary, we need to provide some mechanism to disable the built in marshalling rules. We've explored a few options in #59824, and I've selected one of the options to bring up for API review here (the other options are covered in Alternative Designs).

We propose adding a new property to RuntimeCompatibilityAttribute that disables the built-in marshalling system for all P/Invoke and Reverse P/Invoke scenarios defined in the assembly (all methods marked [DllImport], all delegates that are defined in the assembly when used in interop scenarios, and all UnmanagedCallersOnly-attributed methods). When this property is set to true, all unmanaged types are considered blittable, and all non-blittable types (in/ref/out parameters as well since they require pinning) are disallowed.

We like the idea of disabling across a whole assembly for a few reasons:

  1. We're looking at providing guidance to developers to move all of their P/Invokes to use source-generated stubs. By making one of the gestures for using source generated P/Invokes force this behavior change, we can better push/enforce this guidance.
  2. By making the decision at the assembly boundary, analyzing all assemblies to determine if any use the built-in marshalling system becomes much easier. This is useful for device scenarios where Mono would like to exclude the native-implemented built-in marshalling system entirely if it is unused.
  3. By requiring an explicit gesture for opt-in to this system, we can use the presence of the property assignment as a marker that the developer has acknowledged the change in the "what is blittable?" rules and not issue a diagnostic to notify the developer that source-generated P/Invokes have differing behavior.

API Proposal

namespace System.Runtime.CompilerServices
{
    public class RuntimeCompatibilityAttribute
    {
+         public bool DisableRuntimeMarshalling { get; set; }
    }
}

Additionally, we propose a behavior change for GCHandle.Alloc to support all unmanaged types for allocating a pinned GC handle. There is no API change required for this portion of the request, as this only changes an exceptional scenario (non-blittable unmanaged types throw today in this case, and all other unmanaged types are blittable and as such succeed).

API Usage

[assembly:RuntimeCompatibilityAttribute(WrapNonExceptionThrows = true, DisableRuntimeMarshalling = true)]

Alternative Designs

Alternative Design 1: System.Runtime.InteropServices.UnmanagedType.UnmanagedBlittable

namespace System.Runtime.InteropServices
{
    public enum UnmanagedType
    {
+         UnmanagedBlittable = /* TBD */
    }
}

In this design, we'd either explicitly require or implicitly emit a [MarshalAs(UnmanagedType.UnmanagedBlittable)] attribute on every unmanaged parameter or return type that doesn't have any marshalling attributes already applied.

Pros:

  • Parameter/return value-specific instead of assembly-wide

Cons:

  • Usage is either very noisy (in the explicit requirement design) or not discoverable (implicit emit design)
  • In the implicit design, no mechanism to help developers know about the breaking change without warning all of the time (no acknowledgement mechanism outside of disabling a diagnostic)
  • Analysis for determining if the built-in system is used is significantly more difficult, making componentization in constrained scenarios difficult/impossible.
  • Not compatible with function pointers or UnmanagedCallersOnly, which is the direction the interop team has been pushing.
  • Requires emitting a lot of additional metadata for libraries with many P/Invokes, bloating assembly size.
  • In the implicit design, not easily discoverable for other interop source-gen authors, possibly causing confusing behavior when using multiple different source generators for interop scenarios.

Alternative design 2: System.Runtime.CompilerServices.CallConvUnmanagedBlittable

namespace System.Runtime.CompilerServices
{
+    public sealed class CallConvUnmanagedBlittable {}
}

In this design, we introduce a new "calling convention" type that specifies the "unmanaged == blittable" behavior for all parameters and return values, and we'd implicitly apply it in source-generated scenarios.

Pros:

  • Call site-specific instead of assembly-wide

Cons:

  • Usage is not discoverable
  • No mechanism to help developers know about the breaking change without warning all of the time (no acknowledgement mechanism outside of disabling a diagnostic)
  • Analysis for determining if the built-in system is used is significantly more difficult, making componentization in constrained scenarios difficult/impossible, especially since this only changes behavior for some parameters, not all.
  • Requires emitting quite a bit of additional metadata for libraries with many P/Invokes, bloating assembly size.
  • Not easily discoverable for other interop source-gen authors, possibly causing confusing behavior when using multiple different source generators for interop scenarios.
  • This isn't really a calling convention, so feels weird to use the calling convention mechanism to specify it.

Alternative design 3: System.Runtime.CompilerServices.CallConvNoMarshalling

namespace System.Runtime.CompilerServices
{
+    public sealed class CallConvNoMarshalling {}
}

In this design, we introduce a new "calling convention" type that disables the built-in marshalling behavior for all parameters and return values, and we'd implicitly apply it in source-generated scenarios.

Pros:

  • Call site-specific instead of assembly-wide

Cons:

  • Usage is not discoverable
  • No mechanism to help developers know about the breaking change without warning all of the time (no acknowledgement mechanism outside of disabling a diagnostic)
  • Analysis for determining if the built-in system is used is significantly more difficult, making componentization in constrained scenarios difficult as every interop boundary in every assembly needs to be analyzed individually to ensure that this calling convention is specified.
  • Requires emitting quite a bit of additional metadata for libraries with many P/Invokes, bloating assembly size.
  • Not easily discoverable for other interop source-gen authors, possibly causing confusing behavior when using multiple different source generators for interop scenarios.
  • This isn't really a calling convention, so feels weird to use the calling convention mechanism to specify it.

Risks

Today, Roslyn emits the equivalent to [assembly:RuntimeCompatibilityAttribute(WrapNonExceptionThrows = true)] into every assembly if the RuntimeCompatibilityAttribute is not defined by the user. If the user manually provides the RuntimeCompatibilityAttribute attribution themselves, they may forget to set WrapNonExceptionThrows=true. As scenarios where WrapNonExceptionThrows=true actually kicks in (requires manually written IL or C++/CLI to my knowledge), this shouldn't be much of an issue. If we view this as a problem, we can do one or both of the following things:

  1. We could provide a code-fix that emits a RuntimeCompatibilityAttribute attribute application in scenarios where we'd recommend applying it (ie. with the DllImportGenerator or other interop source-generators).
  2. We could provide an MSBuild property that Roslyn will read in so it can emit the RuntimeCompatibilityAttribute with the DisableRuntimeMarshalling = true property itself.
@jkoritzinsky jkoritzinsky added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.InteropServices labels Oct 19, 2021
@jkoritzinsky jkoritzinsky added this to the 7.0.0 milestone Oct 19, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Oct 19, 2021
@jkoritzinsky
Copy link
Member Author

cc: @AaronRobinsonMSFT @elinor-fung @lambdageek

@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Oct 19, 2021
@AaronRobinsonMSFT
Copy link
Member

@jkoritzinsky I like the global assembly level indication. Two things comes to mind though.

  1. Will this global setting have an impact on consumers of the assembly if a P/Invoke is publicly accessible?

  2. The global setting seems like it will restrict dual use which might be desired to work around limitations or bugs. At present the only way someone could work around this would be to create yet another assembly or manually define the call. Would an "opt-out" mechanism be desirable?

@jkoritzinsky
Copy link
Member Author

  1. Will this global setting have an impact on consumers of the assembly if a P/Invoke is publicly accessible?

Yes, this will impact customers who are consuming a public P/Invoke defined in an assembly with this attribute. There is no plan to use some sort of call-site checking to determine behavior based on the caller's assembly having this attribute in case they differ.

  1. The global setting seems like it will restrict dual use which might be desired to work around limitations or bugs. At present the only way someone could work around this would be to create yet another assembly or manually define the call. Would an "opt-out" mechanism be desirable?

If we decide that an opt-out would be desirable, I'd suggest that we should reconsider the opt-in design options. An opt-out design (instead of a single all-or-nothing switch) makes the "can Mono not ship the built-in system" decision nearly as difficult as the opt-in mechanisms. Even if we decide to add an opt-out mechanism, I'd suggest that we use the alternative opt-in designs as a basis to design the opt-out system.

@tannergooding
Copy link
Member

tannergooding commented Oct 19, 2021

I much prefer the global level indiciation (RuntimeCompatibilityAttribute) as well. As a maintainer of explicitly blittable bindings, I would not appreciate having to annotate several thousand extern calls (even if source generated) with MarshalAs or additional CallConv annotations.

Blittable bindings are blittable and one benefit of that is that they are more readable. Forcing users who want to be blittable to add additional magic annotations everywhere is a major pain point; while opting an entire assembly out via an attribute would not be.


It would be great if this came with an analyzer (or even compiler level warning/error) if DisableRuntimeMarshalling was set so you get notified when DllImport methods are non-blittable. That would likewise fit in if Roslyn needs to coordinate with its own setting of WrapNonExceptionThrows.

@AaronRobinsonMSFT
Copy link
Member

If we decide that an opt-out would be desirable, I'd suggest that we should reconsider the opt-in design options.

That seems a bit extreme for cases that should ideally be few. The concern here is for consumers that have perhaps a couple of P/Invokes that would an impediment to adopt the source generator for the vast majority of their assembly. This is similar thinking to @tannergooding's ID3DInclude example discussed at #60143 (comment). A simple affordance that helps ease adoption. We could say this is a hard requirement too though which does force us to prioritize this impact.

@tannergooding
Copy link
Member

The concern here is for consumers that have perhaps a couple of P/Invokes that would an impediment to adopt the source generator for the vast majority of their assembly

@AaronRobinsonMSFT, are you saying they might have a situation where most of their DllImport are blittable, but they have a couple that aren't and adopting the source generator for those couple might not be possible?

@jkotas
Copy link
Member

jkotas commented Oct 19, 2021

The global setting seems like it will restrict dual use which might be desired to work around limitations or bugs.

You should be able to workaround limitations or bugs by writing the marshaler manually or tweaking the signature today. I do not think we should be promoting this attribute for this purpose.

It would be great if this came with an analyzer

Yes, I think the DllImportGenerator should produce hard errors (not warnings) for all non-blitable DllImports and unmanaged function pointer calls when it sees this attribute.

@AaronRobinsonMSFT
Copy link
Member

that aren't and adopting the source generator for those couple might not be possible?

@tannergooding Yes. Consider the assembly owner that has a library that exposes an API that requires StringBuilder. Rather than force that person to rev their assembly with a break change, we can permit them to use this and leave their one API alone with an "opt-out".

I do not think we should be promoting this attribute for this purpose.

That is fair.

@jkotas
Copy link
Member

jkotas commented Oct 19, 2021

There are a few places in the libraries where the blitability concept leaks out. GCHandle.Alloc for pinned handle is the example that I have mind, there may be others.

Should we consider relaxing the argument validation in GCHandle.Alloc to check for the unmanaged type instead of blittable types? It should be an acceptable breaking change (exception thrown changing to success). It can be part of this API proposal or be separate proposal.

@jkoritzinsky
Copy link
Member Author

I think removing the validation for GCHandle.Alloc would make sense. I'll add that to the proposal above.

@tannergooding
Copy link
Member

Just leaving a comment to track...

We should consider how this impacts SetLastError = true and potentially other DllImport flags that would become unavailable (or potentially unavailable) in this context.

@AaronRobinsonMSFT
Copy link
Member

I believe, we also need to consider the case where the type loader may reorder or pack the type if it is determined to be non-blittable. If I recall during type load the layout of the type is managed auto and not managed sequential. This means if we apply the proposed attribute to a P/Invoke argument but the type was loaded with auto the native side must adhere to how the type loader decided to lay out the type.

@jkoritzinsky Can likely provide a better example but I think the gist here is the type loader has the option of doing something that will make just passing through a type difficult in some cases.

@jkotas
Copy link
Member

jkotas commented Nov 4, 2021

I believe, we also need to consider the case where the type loader may reorder or pack the type if it is determined to be non-blittable

Yes, it is possible today and it makes the field layout algorithm very complex. We should replace the IsBlittable checks in field layout with is IsUnmanaged` (ie does not contain object references).

It was attempted in #51416 (comment) , but we run out of time to stabilize that change for .NET 6. The plan is to do that in .NET 7. We should make this proposal dependent on that. cc @trylek

@jkoritzinsky jkoritzinsky added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Nov 12, 2021
@bartonjs
Copy link
Member

bartonjs commented Nov 16, 2021

Video

  • We changed this to be a separate attribute because of how it composes with the compiler-emitted RuntimeCompatibilityAttribute
  • We had a philosophical discussion about generators emitting attributes like this, and concluded that they shouldn't because of the generator source-view model and composability. (When they're required that should be detected by an analyzer)
namespace System.Runtime.CompilerServices
{
    [AttributeUsage(System.AttributeTargets.Assembly, AllowMultiple = false, Inherited = false)]
    public sealed class DisableRuntimeMarshallingAttribute : Attribute
    {
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 16, 2021
@lambdageek
Copy link
Member

Made a tracking issue for Mono #61685

@PathogenDavid
Copy link
Contributor

This is something I've wanted for quite some time, very excited to see this scheduled for .NET 7!

One concern I do have though is how this could be confusing when function pointers are allowed to cross between assembly boundaries.

For example, imagine I have these two assemblies:

// MyCoolLibraryWrapper
[assembly: DisableRuntimeMarshalling]

public unsafe static class MyCoolLibrary
{
    // Returns an unmanaged function pointer which takes a 16 bit UTF16 character and returns a proper 8 bit bool.
    // (IE: No marshaling is necessary or desired to invoke the returned function pointer.)
    [DllImport("MyCoolLibrary.dll")]
    public static extern delegate* unmanaged<char, bool> GetFilter();

    public static bool PassesFilter(char c)
        => GetFilter()(c);
}
// MyCoolApp, depends on MyCoolLibraryWrapper
// Note that DisableRuntimeMarshalling is not applied.
const char c = 'a';
Console.WriteLine(MyCoolLibrary.PassesFilter(c));
Console.WriteLine(MyCoolLibrary.GetFilter()(c));

For the above:

  • PassesFilter will work as expected regardless of who calls it since marshaling is disabled in the context of the function pointer invocation.
  • MyCoolLibrary.GetFilter()(c) has undefined behavior in the context of MyCoolApp since the marshaler will mess with the function pointer invocation.
    • (This mistake is hard to notice because it can seem to work in some situations.)

Would it be reasonable to provide an in-box analyzer which warns if a non-DisableRuntimeMarshalling assembly accesses an API involving a function pointer which uses non-blittable types from a DisableRuntimeMarshalling assembly and vice-versa?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices
Projects
None yet
7 participants