Implement System.Runtime.CompilerServices.DisabledRuntimeMarshallingAttribute on CoreCLR-family of runtimes/type systems by jkoritzinsky · Pull Request #63320 · dotnet/runtime

jkoritzinsky · 2022-01-04T00:38:41Z

Implement support for the new System.Runtime.CompilerServices.DisabledRuntimeMarshallingAttribute in the CoreCLR, R2R, and NativeAOT type systems/runtimes.

This feature (as discussed in API review), disables all runtime marshalling for P/Invokes, interop delegate types, and function pointer invocations defined within the assembly where the attribute is applied.

This also disables usage of SetLastError=true, PreserveSig=false and LCIDConversionAttribute on P/Invokes. Usage of any of these settings while using [assembly:DisabledRuntimeMarshalling] will result in a MarshalDirectiveException.

This PR also has a few minor fixes for NativeAOT:

Bump the R2R version of NativeAOT to 6. This was missed in Change all "unmanaged" (no GC fields) sequential types to have sequential layout. #61759 as NativeAOT wasn't merged into dotnet/runtime yet.
Change NativeAOT to throw a MarshalDirectiveException instead of a System.Exception when a parameter/field/return-value that cannot be marshalled is encountered to be consistent with CoreCLR.

This PR includes basic support in Mono. Further integration with Mono's componentization is tracked in #61685.

Fixes #60639

…feature.

…ribute is applied.

…n bugs are present. Fix a bug that was uncovered when the tests were refactored.

…arshalling

ghost · 2022-01-04T00:38:45Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

jkoritzinsky · 2022-01-04T00:44:38Z

I had a few open questions for reviewers:

Do we want to support CLR-integrated varargs (__arglist) when DisableRuntimeMarshallingAttribute is applied?

As there is no current alternative for varargs support today, I wanted to raise the question so we can decide whether or not we need to explicitly block it.

Do we want to support marshalling System.ArgIterator to the C va_list when DisableRuntimeMarshallingAttribute is applied?

Since va_list is a platform-specific type that has different layouts on each platform, it's not really practical for developers to provide their own implementations; however, we are also planning on moving away from the CLR-integrated varargs support, so I don't know if we want to explicitly enable support for System.ArgIterator when the attribute is applied.

When DisableRuntimeMarshallingAttribute is applied, we aren't blocking unmanaged value types that explicitly have auto-layout. Right now, we're supporting passing them as-is but with unspecified behavior (as the layout itself is unspecified). Do we want to block value types that have LayoutKind.Auto members in P/Invokes when DisableRuntimeMarshallingAttribute is applied?

stephentoub · 2022-01-04T01:24:11Z

Fixes #60639

Is the plan to eventually apply this on all of our assemblies produced from dotnet/runtime? If so, is that tracked separately?

jkoritzinsky · 2022-01-04T01:53:33Z

As we will be updating the spec for user-defined value type marshalling with DllImportGenerator to require this attribute, we will update the assemblies defined in dotnet/runtime to have this attribute applied as part of that change.

AaronRobinsonMSFT

This is a great start. Now that we are moving toward reconciling blittable and unmanaged, we should also update the BOTR.

AaronRobinsonMSFT · 2022-01-04T02:19:12Z

Do we want to support CLR-integrated varargs (__arglist) when DisableRuntimeMarshallingAttribute is applied?

Great question. I consider it part of the marshalling logic. I think it should apply. I would however like to see how C++/CLI plays with this attribute. I'm not sure how far one would get with C++/CLI if it were applied in general.

Do we want to support marshalling System.ArgIterator to the C va_list when DisableRuntimeMarshallingAttribute is applied?

I'd prefer this to be disabled.

When DisableRuntimeMarshallingAttribute is applied, we aren't blocking unmanaged value types that explicitly have auto-layout. Right now, we're supporting passing them as-is but with unspecified behavior (as the layout itself is unspecified). Do we want to block value types that have LayoutKind.Auto members in P/Invokes when DisableRuntimeMarshallingAttribute is applied?

I think disabling this is appropriate.

MichalStrehovsky · 2022-01-04T01:39:49Z


        private MethodIL EmitIL()
        {
+            if (_targetMethod.HasCustomAttribute("System.Runtime.InteropServices", "LCIDConversionAttribute"))


Fixes dotnet/runtimelab#171

marek-safar · 2022-01-04T09:46:04Z

This also disables usage of SetLastError=true, PreserveSig=false and LCIDConversionAttribute on P/Invokes. Usage of any of these settings while using [assembly:DisabledRuntimeMarshalling] will result in a MarshalDirectiveException.

Will the analyzer be available for a better developer experience?

lambdageek · 2022-01-04T16:33:37Z

This PR does not include Mono support. That is tracked in #61685.

@jkoritzinsky Could you add the new tests to issues.targets to disable them on Mono for now.

elinor-fung

I would be pro blocking/disabling ArgIterator/varargs/auto-layout when runtime marshalling is disabled.

MichalStrehovsky

Crossgen2/NativeAOT changes look good! Thank you!

marek-safar · 2022-01-13T08:06:23Z

+
+	if (m_type_is_byref(t)) {
+		if (emit_exception)
+			get_marshal_cb ()->emit_marshal_directive_exception (m, argnum, "Cannot marshal managed references when the built-in marshalling system is disabled.");	


Should such error messages include the assembly name?

Since the P/Invoke method will be included in the stack trace, I don't think we need to include the assembly name here.

marek-safar · 2022-01-13T08:09:19Z

@vargaz could you review mono changes?

jkotas · 2022-01-14T05:34:29Z

+    /// - LCIDConversionAttribute support is disabled.
+    /// </remarks>
+    [AttributeUsage(AttributeTargets.Assembly, Inherited = false, AllowMultiple = false)]
+    public sealed class DisableRuntimeMarshallingAttribute : Attribute


Have you tried to mark CoreLib with this and see what breaks? Should we add it to CoreLib as part to this PR?

I haven't tried applying this attribute to CoreLib yet. It looks like we have a few regular DllImports that are waiting on the next phase of work after this attribute gets in (described in #63765) to handle cases where we pass System.Guids to and from native code. Once we put this PR in, we can update the DllImportGenerator based on #63765, convert the rest of the DllImports over to the generated ones, and apply this attribute.

I might throw up another PR next week based on this one that applies the attribute just to see all that breaks.

jkotas

Thank you!

jkoritzinsky · 2022-01-18T17:49:18Z

Marking as blocking as a lot of the planned work for the Interop team (the rest of DllImportGenerator productization) requires this PR.

elinor-fung · 2022-01-19T19:17:44Z

+using System.Runtime.InteropServices;
+using Xunit;
+
+public unsafe class DisabledRuntimeMarshallingNative


For after this PR: thoughts on pulling out the common parts between this and the one under Native_Default or having one file with ifdefs?

Maybe... Depends on how much would actually be common between the two.

vargaz · 2022-01-20T20:33:19Z

The mono changes look ok, it could use a bit of cleanup later but nothing that would hold up this PR imho.

…en deciding if runtime marshalling is enabled for a varargs P/Invoke call.

jkoritzinsky added 11 commits December 15, 2021 14:24

Add the DisableRuntimeMarshallingAttribute to the build.

95c08cf

Add initial test suite

225dea7

Implement support in IL stubs for the "disabled runtime marshalling" …

7c85f02

…feature.

Add testing for inlining IL stubs.

132dc67

Block SetLastError and LCID support when DisableRuntimeMarshallingAtt…

70f2fab

…ribute is applied.

Bump NativeAOT-only R2R version header (missed previously)

ae4f309

Implement support in crossgen2 and NativeAOT

8681adf

Clean up the test tree and update the tests to fail more reliably whe…

7700998

…n bugs are present. Fix a bug that was uncovered when the tests were refactored.

Fix NativeAOT and clean up crossgen2

20094a2

Add a test for NoPreserveSig with DisableRuntimeMarshalling

f9f070f

Merge branch 'main' of github.com:dotnet/runtime into disableruntimem…

e0ee72e

…arshalling

jkoritzinsky added the area-Interop-coreclr label Jan 4, 2022

jkoritzinsky added this to the 7.0.0 milestone Jan 4, 2022

jkoritzinsky requested review from AaronRobinsonMSFT, MichalStrehovsky and elinor-fung January 4, 2022 00:38

ghost added the new-api-needs-documentation label Jan 4, 2022

ghost assigned jkoritzinsky Jan 4, 2022

Assign hr in SUCCEEDED macro.

694f5fb

AaronRobinsonMSFT reviewed Jan 4, 2022

View reviewed changes

Comment thread ...em.Private.CoreLib/src/System/Runtime/CompilerServices/DisableRuntimeMarshallingAttribute.cs

MichalStrehovsky reviewed Jan 4, 2022

View reviewed changes

jkotas reviewed Jan 4, 2022

View reviewed changes

Comment thread src/coreclr/vm/mlinfo.cpp

elinor-fung reviewed Jan 4, 2022

View reviewed changes

Comment thread src/coreclr/dlls/mscorrc/mscorrc.rc Outdated

jkoritzinsky requested review from elinor-fung and jkotas January 12, 2022 17:50

jkoritzinsky mentioned this pull request Jan 12, 2022

Add conceptual documentation for "disabled runtime marshalling" dotnet/docs#27820

Merged

MichalStrehovsky approved these changes Jan 13, 2022

View reviewed changes

Comment thread src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/Stubs/PInvokeILEmitter.cs Outdated

Comment thread src/coreclr/tools/aot/ILCompiler.ReadyToRun/Interop/IL/Marshaller.ReadyToRun.cs Outdated

marek-safar reviewed Jan 13, 2022

View reviewed changes

jkoritzinsky added 2 commits January 13, 2022 10:53

Unify on "runtime marshalling" terminology

3122f76

Clean up unused usings.

0d1f54e

jkotas reviewed Jan 14, 2022

View reviewed changes

Address Jan's feedback except for applying the attribute to CoreLib.

6a08e87

jkotas approved these changes Jan 15, 2022

View reviewed changes

jkoritzinsky added the blocking Marks issues that we want to fast track in order to unblock other important work label Jan 18, 2022

AaronRobinsonMSFT approved these changes Jan 19, 2022

View reviewed changes

elinor-fung approved these changes Jan 19, 2022

View reviewed changes

jkoritzinsky added 2 commits January 20, 2022 12:05

PR feedback.

9e0fc41

Mono throws an InvalidProgramException for varargs

0896e44

vargaz approved these changes Jan 20, 2022

View reviewed changes

jkoritzinsky and others added 3 commits January 20, 2022 13:53

Fix copy-paste issue.

3609c82

Make sure we use the P/Invoke's Module and not the caller's module wh…

74b0f61

…en deciding if runtime marshalling is enabled for a varargs P/Invoke call.

Handle how LLVM AOT reports the failure to handle varargs (EEE)

f496d66

jkoritzinsky merged commit 358ee3c into dotnet:main Jan 21, 2022

jkoritzinsky deleted the disableruntimemarshalling branch January 21, 2022 08:10

This was referenced Jan 21, 2022

system.reflection.metadata.applyupdatetest.testaddstaticfield failing on OSX with checked coreclr #64117

Closed

AF: g_TblIndex[ixTbl].m_Token == type in System.Reflection.Metadata.ApplyUpdate tests #64070

Closed

radical mentioned this pull request Jan 21, 2022

[wasm] Missing support for building native libraries as part of tests #64127

Open

lambdageek mentioned this pull request Feb 7, 2022

[Mono] Add support for DisableRuntimeMarshallingAttribute #61685

Open

5 tasks

ghost locked as resolved and limited conversation to collaborators Feb 20, 2022

Uh oh!

Conversation

jkoritzinsky commented Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jan 4, 2022

Uh oh!

jkoritzinsky commented Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentoub commented Jan 4, 2022

Uh oh!

jkoritzinsky commented Jan 4, 2022

Uh oh!

AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AaronRobinsonMSFT commented Jan 4, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichalStrehovsky Jan 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marek-safar commented Jan 4, 2022

Uh oh!

lambdageek commented Jan 4, 2022

Uh oh!

elinor-fung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MichalStrehovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marek-safar Jan 13, 2022

Choose a reason for hiding this comment

Uh oh!

jkoritzinsky Jan 13, 2022

Choose a reason for hiding this comment

Uh oh!

marek-safar commented Jan 13, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkotas Jan 14, 2022

Choose a reason for hiding this comment

Uh oh!

jkoritzinsky Jan 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

jkoritzinsky commented Jan 18, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elinor-fung Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

jkoritzinsky Jan 20, 2022

jkoritzinsky commented Jan 4, 2022 •

edited

Loading

jkoritzinsky commented Jan 4, 2022 •

edited

Loading