Generate an 'out' local variable for unmanaged to managed stubs when the value needs to be marshalled out to unmanaged #89139

jtschuster · 2023-07-18T21:36:27Z

Generates an 'out' local variable that will hold the marshalled value that will be passed back on a successful return. Right now it just generates the variable and discards the value immediately. The next step is to actually use that variable name in marshalling and copy the value to the parameter right before a successful return.

Using the variable just requires changing the MarshalToLocalContext to use the commented version of GetIdentifiers.

Copying from the local to the parameter is a little more complicated because of the ByValue [Out] case.

First step of #88438

…lingInterfaceCleanup

…ld 20230716.1 (dotnet#89077)" This reverts commit 09e724f.

…slyn build 20230716.1 (dotnet#89077)"" This reverts commit 0a532ad.

…lingInterfaceCleanup

ghost · 2023-07-18T21:36:48Z

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

Generates an 'out' local variable that will hold the marshalled value that will be passed back on a successful return. Right now it just generates the variable and discards the value immediately. The next step is to actually use that variable name in marshalling and copy the value to the parameter right before a successful return.

Using the variable just requires changing the MarshalToLocalContext to use the commented version of GetIdentifiers.

Copying from the local to the parameter is a little more complicated because of the ByValue [Out] case.

First step of #88438

Author:	jtschuster
Assignees:	-
Labels:	`area-System.Runtime.InteropServices`
Milestone:	-

...time.InteropServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/MarshallerHelpers.cs

...System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/GeneratedStatements.cs

...ies/System.Runtime.InteropServices/tests/TestAssets/SharedTypes/ComInterfaces/IRefStrings.cs

...ystem.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/VariableDeclarations.cs

…lingInterfaceCleanup

AaronRobinsonMSFT

Is this needed for a specific scenario or are we trying to be precise? I want to make sure we are weighting priorities correctly here.

AaronRobinsonMSFT · 2023-07-26T18:34:38Z

.../System.Runtime.InteropServices/gen/ComInterfaceGenerator/UnmanagedToManagedStubGenerator.cs

@@ -110,6 +109,9 @@ public BlockSyntax GenerateStubBody(ExpressionSyntax methodToInvoke)
                allStatements.AddRange(tryStatements);
            }

+            allStatements.AddRange(statements.AssignOut);
+            allStatements.AddRange(statements.Cleanup);


This order concerns me. We added the finally staements above after we added the cleanup. Inverting this seems like a profound change.

AaronRobinsonMSFT · 2023-07-26T18:35:39Z

...System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/GeneratedStatements.cs

@@ -22,6 +22,8 @@ public struct GeneratedStatements
        public ImmutableArray<StatementSyntax> NotifyForSuccessfulInvoke { get; init; }
        public ImmutableArray<StatementSyntax> GuaranteedUnmarshal { get; init; }
        public ImmutableArray<StatementSyntax> Cleanup { get; init; }
+        //public ImmutableArray<StatementSyntax> CleanupNativeOut { get; init; }


What are the next steps for commented out code? At a minimum we need to have that captured here so we know how to uncomment or delete it.

AaronRobinsonMSFT · 2023-07-26T18:35:54Z

...System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/GeneratedStatements.cs

@@ -39,7 +41,9 @@ public static GeneratedStatements Create(BoundGenerators marshallers, StubCodeCo
                NotifyForSuccessfulInvoke = GenerateStatementsForStubContext(marshallers, context with { CurrentStage = StubCodeContext.Stage.NotifyForSuccessfulInvoke }),
                GuaranteedUnmarshal = GenerateStatementsForStubContext(marshallers, context with { CurrentStage = StubCodeContext.Stage.GuaranteedUnmarshal }),
                Cleanup = GenerateStatementsForStubContext(marshallers, context with { CurrentStage = StubCodeContext.Stage.Cleanup }),
-                ManagedExceptionCatchClauses = GenerateCatchClauseForManagedException(marshallers, context)
+                //CleanupNativeOut = GenerateStatementsForStubContext(marshallers, new MarshalToLocalContext(context with { CurrentStage = StubCodeContext.Stage.Cleanup })),


Again with commented out code.

AaronRobinsonMSFT · 2023-07-26T18:36:51Z

...System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/GeneratedStatements.cs

+                    if (!MarshallerHelpers.MarshalsOutToLocal(marshaller.TypeInfo, context))
+                        continue;
+                    else
+                        localContext = new AssignOutContext(context, marshaller.Generator.AsParameter(marshaller.TypeInfo, context).Identifier.ToString());


Suggested change

if (!MarshallerHelpers.MarshalsOutToLocal(marshaller.TypeInfo, context))

continue;

else

localContext = new AssignOutContext(context, marshaller.Generator.AsParameter(marshaller.TypeInfo, context).Identifier.ToString());

if (!MarshallerHelpers.MarshalsOutToLocal(marshaller.TypeInfo, context))

{

continue;

}

localContext = new AssignOutContext(context, marshaller.Generator.AsParameter(marshaller.TypeInfo, context).Identifier.ToString());

AaronRobinsonMSFT · 2023-07-26T18:38:53Z

...Microsoft.Interop.SourceGeneration/Marshalling/AttributedMarshallingModelGeneratorFactory.cs

+                //if (freeStrategy == FreeStrategy.FreeOriginal)
+                //{
+                //    marshallingStrategy = new UnmanagedToManagedOwnershipTrackingStrategy(marshallingStrategy);
+                //}


I'll stop commenting, but undescribed commented out code is a red-flag in general.

AaronRobinsonMSFT · 2023-07-26T18:39:50Z

...ervices/gen/Microsoft.Interop.SourceGeneration/Marshalling/CustomTypeMarshallingGenerator.cs

+                    if (context is MarshalToLocalContext
+                        && (info.ByValueContentsMarshalKind.HasFlag(ByValueContentsMarshalKind.Out)
+                            || info.RefKind is RefKind.Out))
+                        break;
+                    else if (context.Direction is MarshalDirection.UnmanagedToManaged && info.RefKind is RefKind.Out)
+                        break;
+                    else
+                        return _nativeTypeMarshaller.GenerateCleanupStatements(info, context);


If/else chains require braces.

AaronRobinsonMSFT · 2023-07-26T18:40:06Z

....InteropServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/MarshalToLocalContext.cs

+        public override bool AdditionalTemporaryStateLivesAcrossStages => InnerContext.AdditionalTemporaryStateLivesAcrossStages;
+
+        public override (string managed, string native) GetIdentifiers(TypePositionInfo info)
+        //=> InnerContext.GetIdentifiers(info);


jkoritzinsky · 2023-07-26T18:32:32Z

...Microsoft.Interop.SourceGeneration/Marshalling/AttributedMarshallingModelGeneratorFactory.cs

+                //if (freeStrategy == FreeStrategy.FreeOriginal)
+                //{
+                //    marshallingStrategy = new UnmanagedToManagedOwnershipTrackingStrategy(marshallingStrategy);
+                //}


Please remove the commented-out code before merging

jkoritzinsky · 2023-07-26T18:33:00Z

...Microsoft.Interop.SourceGeneration/Marshalling/AttributedMarshallingModelGeneratorFactory.cs

@@ -467,7 +467,8 @@ private enum FreeStrategy
            /// <summary>
            /// Do not free the unmanaged value, we don't own it.
            /// </summary>
-            NoFree
+            NoFree,
+            FreeTemp


Can you add a comment here about what this strategy means?

jkoritzinsky · 2023-07-26T18:34:02Z

...ervices/gen/Microsoft.Interop.SourceGeneration/Marshalling/CustomTypeMarshallingGenerator.cs

+                    // TODO: Correctly clean up the allocated contents that aren't transferred back to the caller
+                    // We don't correctly clean up the [out] parameters


Is this TODO still accurate?

jkoritzinsky · 2023-07-26T18:38:54Z

...me.InteropServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/ElementsMarshalling.cs

+            statements.Add(numElementsAssignment);
+            if (MarshallerHelpers.MarshalsOutToLocal(info, context))
+            {
+                // #pragma warning disable CS9081


We shouldn't be suppressing this rule. This suppression likely covers up a use-out-of-scope bug. We should either declare native_out as scoped, or (if we can't), revisit how we do this. Can you share some example code that goes down this route?

jkoritzinsky · 2023-07-26T18:40:34Z

...me.InteropServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/ElementsMarshalling.cs

+            if (MarshallerHelpers.MarshalsOutToLocal(info, context) && _isStateful)
+            {
+                var elementType = _parameterPointedToType;//info.IsByRef ? PointerType(_unmanagedElementType) : _unmanagedElementType;
+                // <value_native_out> = (<nativeType>*)Unsafe.AsPointer(ref <nativeSpan>.GetPinnableReference());


Similar question here around using a "pinnable reference" without pinning. Are we sure that this works? If this is only in the "stateful element" case, we shouldn't add code that will do bad things and instead we should gracefully fail earlier.

jkoritzinsky · 2023-07-26T18:43:39Z

...ystem.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/VariableDeclarations.cs

                            // To simplify propogating back the value to the "byref" parameter,
                            // we'll just declare the native identifier as a ref to its type.
                            // The rest of the code we generate will work as expected, and we don't need
                            // to manually propogate back the updated values after the call.


Looks like this comment is out of date now. We should remove it if it's no longer accurate.

jkoritzinsky · 2023-07-26T18:44:42Z

...raries/System.Runtime.InteropServices/tests/ComInterfaceGenerator.Tests/RcwAroundCcwTests.cs

+            // https://github.com/dotnet/runtime/issues/89265
+            //obj.Double(data, data.Length);


Can we split this case into a separate Xunit test and mark it with ActiveIssue instead of just commenting it out? Using ActiveIssue makes it easier to track/find the tests when we fix them.

jkoritzinsky · 2023-07-26T18:45:29Z

...nteropServices/tests/ComInterfaceGenerator.Tests/UnmanagedToManagedCustomMarshallingTests.cs

@@ -281,6 +278,7 @@ public unsafe void ValidateArrayElementsByRefFreed_Stateful()
        }

        [Fact]
+        [ActiveIssue("Make issue: ByValueContents out do not get freed")]


Is this a new issue introduced by this PR or is this an existing issue?

jkoritzinsky · 2023-07-26T18:46:53Z

...raries/System.Runtime.InteropServices/tests/ComInterfaceGenerator.Unit.Tests/CompileFails.cs

+            // https://github.com/dotnet/runtime/issues/89265
+            //yield return new object[] {
+            //    ID(),
+            //    codeSnippets.ByValueMarshallingOfType(inAttribute + outAttribute + constElementCount, "int[]", paramNameWithLocation),
+            //    new DiagnosticResult[] { inOutAttributeIsDefaultDiagnostic }
+            //};


It looks like this test is passing in main. We're really late in the release cycle, so we shouldn't be disabling failing tests to get PRs in.

jkoritzinsky · 2023-07-26T18:47:21Z

...libraries/System.Runtime.InteropServices/tests/TestAssets/NativeExports/NativeExports.csproj

@@ -18,6 +18,13 @@
      '$(TargetOS)' == 'tvossimulator'">true</_TargetsAppleOS>
  </PropertyGroup>

+<Target Name="DisableComInterfaceGenerator"


Let's add a comment here about this target if we're going to merge it into main.

jtschuster · 2023-07-27T17:36:55Z

Is this needed for a specific scenario or are we trying to be precise? I want to make sure we are weighting priorities correctly here.

This is mainly to be precise and avoid memory leaks and potential double frees if marshalling fails.

jtschuster · 2023-07-27T17:38:17Z

This PR requires a few test regressions that aren't palatable and isn't that much smaller than the full change, so I'll close this in favor of the complete change.

jtschuster and others added 22 commits June 15, 2023 16:34

Fully qualify return type on UnreachableException methods

dc6356f

Merge branch 'main' of https://github.com/dotnet/runtime

0cc937e

Merge branch 'main' of https://github.com/dotnet/runtime

a16a7d6

Merge branch 'main' of https://github.com/dotnet/runtime

cca57d3

wip

ed7cd71

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

744a39b

…lingInterfaceCleanup

wip

788c862

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

d4aaad5

…lingInterfaceCleanup

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

7363299

…lingInterfaceCleanup

wip

1e1345b

builds the interop sln. repo build failing

5c0ba6a

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

203807e

…lingInterfaceCleanup

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

f2d7921

…lingInterfaceCleanup

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

2b99032

…lingInterfaceCleanup

Revert "Update dependencies from https://github.com/dotnet/roslyn bui…

0a532ad

…ld 20230716.1 (dotnet#89077)" This reverts commit 09e724f.

Revert "Revert "Update dependencies from https://github.com/dotnet/ro…

8128579

…slyn build 20230716.1 (dotnet#89077)"" This reverts commit 0a532ad.

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

acf29cc

…lingInterfaceCleanup

Fix changes to other files

f8da5a1

fix changes within interop sln

8a8f573

Tests pass

86099a2

Remove trailing space

8b14784

Remove commented code

ee7de7f

jtschuster requested review from jkoritzinsky and AaronRobinsonMSFT July 18, 2023 21:36

dotnet-issue-labeler bot added the area-System.Runtime.InteropServices label Jul 18, 2023

ghost assigned jtschuster Jul 18, 2023

jkoritzinsky reviewed Jul 18, 2023

View reviewed changes

...time.InteropServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/MarshallerHelpers.cs Outdated Show resolved Hide resolved

AaronRobinsonMSFT reviewed Jul 18, 2023

View reviewed changes

Refactor com interfaces and rename isMidlOut

cf38596

jtschuster added 5 commits July 18, 2023 15:49

PR feedback: rename _guid, be consistent in local vs property use

29a6bad

Remove temp test project

1627cc9

wip

c25b5d7

wip

31d5f1b

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

5370aa4

…lingInterfaceCleanup

build-analysis bot mentioned this pull request Jul 20, 2023

Helix work item failures due to failure to find xharness-event-processor.py dotnet/dnceng#400

Closed

3 tasks

jtschuster added 2 commits July 20, 2023 17:07

Almost Passing tests, skipping [out] collections cleanup

c615eb2

Passing tests

525f58c

jtschuster mentioned this pull request Jul 21, 2023

Ownership tracking #89329

Closed

Remove extraneous changes

0fccde6

AaronRobinsonMSFT reviewed Jul 26, 2023

View reviewed changes

jkoritzinsky reviewed Jul 26, 2023

View reviewed changes

build-analysis bot mentioned this pull request Jul 26, 2023

Tracking issue for CI build timeouts #76454

Closed

jtschuster closed this Jul 27, 2023

ghost locked as resolved and limited conversation to collaborators Aug 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate an 'out' local variable for unmanaged to managed stubs when the value needs to be marshalled out to unmanaged #89139

Generate an 'out' local variable for unmanaged to managed stubs when the value needs to be marshalled out to unmanaged #89139

jtschuster commented Jul 18, 2023

ghost commented Jul 18, 2023

AaronRobinsonMSFT left a comment

AaronRobinsonMSFT Jul 26, 2023

AaronRobinsonMSFT Jul 26, 2023

AaronRobinsonMSFT Jul 26, 2023

AaronRobinsonMSFT Jul 26, 2023

AaronRobinsonMSFT Jul 26, 2023

AaronRobinsonMSFT Jul 26, 2023

AaronRobinsonMSFT Jul 26, 2023

jkoritzinsky Jul 26, 2023

jkoritzinsky Jul 26, 2023

jkoritzinsky Jul 26, 2023

jkoritzinsky Jul 26, 2023

jkoritzinsky Jul 26, 2023

jkoritzinsky Jul 26, 2023

jkoritzinsky Jul 26, 2023

jkoritzinsky Jul 26, 2023

jkoritzinsky Jul 26, 2023

jkoritzinsky Jul 26, 2023

jtschuster commented Jul 27, 2023

jtschuster commented Jul 27, 2023

		// TODO: Correctly clean up the allocated contents that aren't transferred back to the caller
		// We don't correctly clean up the [out] parameters

		// https://github.com/dotnet/runtime/issues/89265
		//obj.Double(data, data.Length);

Generate an 'out' local variable for unmanaged to managed stubs when the value needs to be marshalled out to unmanaged #89139

Generate an 'out' local variable for unmanaged to managed stubs when the value needs to be marshalled out to unmanaged #89139

Conversation

jtschuster commented Jul 18, 2023

ghost commented Jul 18, 2023

AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtschuster commented Jul 27, 2023

jtschuster commented Jul 27, 2023