-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Generate an 'out' local variable for unmanaged to managed stubs when the value needs to be marshalled out to unmanaged #89139
Changes from all commits
dc6356f
0cc937e
a16a7d6
cca57d3
ed7cd71
744a39b
788c862
d4aaad5
7363299
1e1345b
5c0ba6a
203807e
f2d7921
2b99032
0a532ad
8128579
acf29cc
f8da5a1
8a8f573
86099a2
8b14784
ee7de7f
cf38596
29a6bad
1627cc9
c25b5d7
31d5f1b
5370aa4
c615eb2
525f58c
0fccde6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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; } | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||||||||||||
public ImmutableArray<StatementSyntax> AssignOut { get; init; } | ||||||||||||||||||||
|
||||||||||||||||||||
public ImmutableArray<CatchClauseSyntax> ManagedExceptionCatchClauses { get; init; } | ||||||||||||||||||||
|
||||||||||||||||||||
|
@@ -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 })), | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again with commented out code. |
||||||||||||||||||||
ManagedExceptionCatchClauses = GenerateCatchClauseForManagedException(marshallers, context), | ||||||||||||||||||||
AssignOut = GenerateStatementsForStubContext(marshallers, context with { CurrentStage = StubCodeContext.Stage.AssignOut }) | ||||||||||||||||||||
}; | ||||||||||||||||||||
} | ||||||||||||||||||||
public static GeneratedStatements Create(BoundGenerators marshallers, StubCodeContext context, ExpressionSyntax expressionToInvoke) | ||||||||||||||||||||
|
@@ -71,7 +75,26 @@ private static ImmutableArray<StatementSyntax> GenerateStatementsForStubContext( | |||||||||||||||||||
ImmutableArray<StatementSyntax>.Builder statementsToUpdate = ImmutableArray.CreateBuilder<StatementSyntax>(); | ||||||||||||||||||||
foreach (BoundGenerator marshaller in marshallers.SignatureMarshallers) | ||||||||||||||||||||
{ | ||||||||||||||||||||
statementsToUpdate.AddRange(marshaller.Generator.Generate(marshaller.TypeInfo, context)); | ||||||||||||||||||||
StubCodeContext localContext = context; | ||||||||||||||||||||
if (context.CurrentStage is StubCodeContext.Stage.Marshal or StubCodeContext.Stage.AssignOut | ||||||||||||||||||||
&& MarshallerHelpers.MarshalsOutToLocal(marshaller.TypeInfo, context)) | ||||||||||||||||||||
{ | ||||||||||||||||||||
localContext = new MarshalToLocalContext(context); | ||||||||||||||||||||
} | ||||||||||||||||||||
// Right now, MarshalsOutToLocal is the only way we determine if we assign out, so we can return early here for perf | ||||||||||||||||||||
if (context.CurrentStage is StubCodeContext.Stage.AssignOut) | ||||||||||||||||||||
{ | ||||||||||||||||||||
if (!MarshallerHelpers.MarshalsOutToLocal(marshaller.TypeInfo, context)) | ||||||||||||||||||||
continue; | ||||||||||||||||||||
else | ||||||||||||||||||||
localContext = new AssignOutContext(context, marshaller.Generator.AsParameter(marshaller.TypeInfo, context).Identifier.ToString()); | ||||||||||||||||||||
Comment on lines
+87
to
+90
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||
} | ||||||||||||||||||||
if (context is MarshalToLocalContext && context.CurrentStage == StubCodeContext.Stage.Cleanup) | ||||||||||||||||||||
{ | ||||||||||||||||||||
if (!MarshallerHelpers.MarshalsOutToLocal(marshaller.TypeInfo, context)) | ||||||||||||||||||||
continue; | ||||||||||||||||||||
} | ||||||||||||||||||||
statementsToUpdate.AddRange(marshaller.Generator.Generate(marshaller.TypeInfo, localContext)); | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
if (statementsToUpdate.Count > 0) | ||||||||||||||||||||
|
@@ -164,6 +187,11 @@ private static ImmutableArray<CatchClauseSyntax> GenerateCatchClauseForManagedEx | |||||||||||||||||||
catchClauseBuilder.AddRange( | ||||||||||||||||||||
managedExceptionMarshaller.Generator.Generate( | ||||||||||||||||||||
managedExceptionMarshaller.TypeInfo, context with { CurrentStage = StubCodeContext.Stage.PinnedMarshal })); | ||||||||||||||||||||
catchClauseBuilder.AddRange(GenerateStatementsForStubContext(marshallers, new MarshalToLocalContext(context with { CurrentStage = StubCodeContext.Stage.Cleanup }))); | ||||||||||||||||||||
if (!marshallers.IsUnmanagedVoidReturn) | ||||||||||||||||||||
{ | ||||||||||||||||||||
catchClauseBuilder.Add(ReturnStatement(IdentifierName(context.GetIdentifiers(marshallers.NativeReturnMarshaller.TypeInfo).native))); | ||||||||||||||||||||
} | ||||||||||||||||||||
return ImmutableArray.Create( | ||||||||||||||||||||
CatchClause( | ||||||||||||||||||||
CatchDeclaration(ParseTypeName(TypeNames.System_Exception), Identifier(managed)), | ||||||||||||||||||||
|
@@ -185,6 +213,7 @@ private static SyntaxTriviaList GenerateStageTrivia(StubCodeContext.Stage stage) | |||||||||||||||||||
StubCodeContext.Stage.Cleanup => "Perform required cleanup.", | ||||||||||||||||||||
StubCodeContext.Stage.NotifyForSuccessfulInvoke => "Keep alive any managed objects that need to stay alive across the call.", | ||||||||||||||||||||
StubCodeContext.Stage.GuaranteedUnmarshal => "Convert native data to managed data even in the case of an exception during the non-cleanup phases.", | ||||||||||||||||||||
StubCodeContext.Stage.AssignOut => "Assign to parameters", | ||||||||||||||||||||
_ => throw new ArgumentOutOfRangeException(nameof(stage)) | ||||||||||||||||||||
}; | ||||||||||||||||||||
|
||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Collections.Generic; | ||
using System.Collections.Immutable; | ||
using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
|
||
namespace Microsoft.Interop | ||
{ | ||
internal static class ICustomTypeMarshallingStrategyExtensions | ||
{ | ||
public static IEnumerable<ExpressionStatementSyntax> GenerateDefaultAssignOutStatement(this ICustomTypeMarshallingStrategy _, TypePositionInfo info, AssignOutContext context) | ||
{ | ||
if (MarshallerHelpers.MarshalsOutToLocal(info, context)) | ||
{ | ||
var (_, native) = context.GetIdentifiers(info); | ||
return ImmutableArray.Create(MarshallerHelpers.GenerateAssignmentToPointerValue(context.ParameterIdentifier, native)); | ||
} | ||
return ImmutableArray<ExpressionStatementSyntax>.Empty; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System; | ||
using System.Diagnostics; | ||
|
||
namespace Microsoft.Interop | ||
{ | ||
public sealed record AssignOutContext : StubCodeContext | ||
{ | ||
internal StubCodeContext InnerContext { get; init; } | ||
public string ParameterIdentifier { get; init; } | ||
|
||
internal AssignOutContext(StubCodeContext inner, string parameterIdentifier) | ||
{ | ||
InnerContext = inner; | ||
Debug.Assert(inner.CurrentStage == Stage.AssignOut); | ||
CurrentStage = Stage.AssignOut; | ||
Direction = inner.Direction; | ||
ParentContext = inner.ParentContext; | ||
ParameterIdentifier = parameterIdentifier; | ||
} | ||
|
||
public override (TargetFramework framework, Version version) GetTargetFramework() => InnerContext.GetTargetFramework(); | ||
|
||
public override bool SingleFrameSpansNativeContext => InnerContext.SingleFrameSpansNativeContext; | ||
|
||
public override bool AdditionalTemporaryStateLivesAcrossStages => InnerContext.AdditionalTemporaryStateLivesAcrossStages; | ||
|
||
public override (string managed, string native) GetIdentifiers(TypePositionInfo info) | ||
=> (InnerContext.GetIdentifiers(info).managed, InnerContext.GetAdditionalIdentifier(info, "out")); | ||
|
||
public override string GetAdditionalIdentifier(TypePositionInfo info, string name) => InnerContext.GetAdditionalIdentifier(info, name); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -278,20 +278,20 @@ private ResolvedGenerator CreateCustomNativeTypeMarshaller(TypePositionInfo info | |
|
||
FreeStrategy freeStrategy = GetFreeStrategy(info, context); | ||
|
||
if (freeStrategy == FreeStrategy.FreeOriginal) | ||
{ | ||
marshallingStrategy = new UnmanagedToManagedOwnershipTrackingStrategy(marshallingStrategy); | ||
} | ||
//if (freeStrategy == FreeStrategy.FreeOriginal) | ||
//{ | ||
// marshallingStrategy = new UnmanagedToManagedOwnershipTrackingStrategy(marshallingStrategy); | ||
//} | ||
Comment on lines
+281
to
+284
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove the commented-out code before merging
Comment on lines
+281
to
+284
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll stop commenting, but undescribed commented out code is a red-flag in general. |
||
|
||
if (freeStrategy != FreeStrategy.NoFree && marshallerData.Shape.HasFlag(MarshallerShape.Free)) | ||
{ | ||
marshallingStrategy = new StatelessFreeMarshalling(marshallingStrategy, marshallerData.MarshallerType.Syntax); | ||
} | ||
|
||
if (freeStrategy == FreeStrategy.FreeOriginal) | ||
{ | ||
marshallingStrategy = new CleanupOwnedOriginalValueMarshalling(marshallingStrategy); | ||
} | ||
//if (freeStrategy == FreeStrategy.FreeOriginal) | ||
//{ | ||
// marshallingStrategy = new CleanupOwnedOriginalValueMarshalling(marshallingStrategy); | ||
//} | ||
} | ||
|
||
IMarshallingGenerator marshallingGenerator = new CustomTypeMarshallingGenerator(marshallingStrategy, ByValueMarshalKindSupportDescriptor.Default, marshallerData.Shape.HasFlag(MarshallerShape.StatelessPinnableReference)); | ||
|
@@ -371,19 +371,19 @@ private ResolvedGenerator CreateNativeCollectionMarshaller( | |
|
||
var freeStrategy = GetFreeStrategy(info, context); | ||
IElementsMarshallingCollectionSource collectionSource = new StatefulLinearCollectionSource(); | ||
ElementsMarshalling elementsMarshalling = CreateElementsMarshalling(marshallerData, elementInfo, elementMarshaller, unmanagedElementType, collectionSource); | ||
ElementsMarshalling elementsMarshalling = CreateElementsMarshalling(marshallerData, elementInfo, elementMarshaller, unmanagedElementType, collectionSource, nativeType.Syntax, true); | ||
|
||
if (freeStrategy == FreeStrategy.FreeOriginal) | ||
{ | ||
marshallingStrategy = new UnmanagedToManagedOwnershipTrackingStrategy(marshallingStrategy); | ||
} | ||
//if (freeStrategy == FreeStrategy.FreeOriginal) | ||
//{ | ||
// marshallingStrategy = new UnmanagedToManagedOwnershipTrackingStrategy(marshallingStrategy); | ||
//} | ||
|
||
marshallingStrategy = new StatefulLinearCollectionMarshalling(marshallingStrategy, marshallerData.Shape, numElementsExpression, elementsMarshalling, freeStrategy != FreeStrategy.NoFree); | ||
marshallingStrategy = new StatefulLinearCollectionMarshalling(marshallingStrategy, marshallerData.Shape, numElementsExpression, elementsMarshalling, freeStrategy is not FreeStrategy.NoFree or FreeStrategy.FreeTemp); | ||
|
||
if (freeStrategy == FreeStrategy.FreeOriginal) | ||
{ | ||
marshallingStrategy = new CleanupOwnedOriginalValueMarshalling(marshallingStrategy); | ||
} | ||
//if (freeStrategy == FreeStrategy.FreeOriginal) | ||
//{ | ||
// marshallingStrategy = new CleanupOwnedOriginalValueMarshalling(marshallingStrategy); | ||
//} | ||
|
||
if (marshallerData.Shape.HasFlag(MarshallerShape.Free)) | ||
{ | ||
|
@@ -397,14 +397,14 @@ private ResolvedGenerator CreateNativeCollectionMarshaller( | |
var freeStrategy = GetFreeStrategy(info, context); | ||
|
||
IElementsMarshallingCollectionSource collectionSource = new StatelessLinearCollectionSource(marshallerTypeSyntax); | ||
if (freeStrategy == FreeStrategy.FreeOriginal) | ||
{ | ||
marshallingStrategy = new UnmanagedToManagedOwnershipTrackingStrategy(marshallingStrategy); | ||
} | ||
//if (freeStrategy == FreeStrategy.FreeOriginal) | ||
//{ | ||
// marshallingStrategy = new UnmanagedToManagedOwnershipTrackingStrategy(marshallingStrategy); | ||
//} | ||
|
||
ElementsMarshalling elementsMarshalling = CreateElementsMarshalling(marshallerData, elementInfo, elementMarshaller, unmanagedElementType, collectionSource); | ||
ElementsMarshalling elementsMarshalling = CreateElementsMarshalling(marshallerData, elementInfo, elementMarshaller, unmanagedElementType, collectionSource, nativeType.Syntax, false); | ||
|
||
marshallingStrategy = new StatelessLinearCollectionMarshalling(marshallingStrategy, elementsMarshalling, nativeType, marshallerData.Shape, numElementsExpression, freeStrategy != FreeStrategy.NoFree); | ||
marshallingStrategy = new StatelessLinearCollectionMarshalling(marshallingStrategy, elementsMarshalling, nativeType, marshallerData.Shape, numElementsExpression, !(freeStrategy is FreeStrategy.NoFree or FreeStrategy.FreeTemp)); | ||
|
||
if (marshallerData.Shape.HasFlag(MarshallerShape.CallerAllocatedBuffer)) | ||
{ | ||
|
@@ -420,10 +420,10 @@ private ResolvedGenerator CreateNativeCollectionMarshaller( | |
marshallingStrategy = new StatelessFreeMarshalling(marshallingStrategy, marshallerTypeSyntax); | ||
} | ||
|
||
if (freeStrategy == FreeStrategy.FreeOriginal) | ||
{ | ||
marshallingStrategy = new CleanupOwnedOriginalValueMarshalling(marshallingStrategy); | ||
} | ||
//if (freeStrategy == FreeStrategy.FreeOriginal) | ||
//{ | ||
// marshallingStrategy = new CleanupOwnedOriginalValueMarshalling(marshallingStrategy); | ||
//} | ||
} | ||
|
||
ByValueMarshalKindSupportDescriptor byValueMarshalKindSupport; | ||
|
@@ -467,7 +467,8 @@ private enum FreeStrategy | |
/// <summary> | ||
/// Do not free the unmanaged value, we don't own it. | ||
/// </summary> | ||
NoFree | ||
NoFree, | ||
FreeTemp | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment here about what this strategy means? |
||
} | ||
|
||
private static FreeStrategy GetFreeStrategy(TypePositionInfo info, StubCodeContext context) | ||
|
@@ -491,12 +492,16 @@ private static FreeStrategy GetFreeStrategy(TypePositionInfo info, StubCodeConte | |
{ | ||
return FreeStrategy.FreeOriginal; | ||
} | ||
if (MarshallerHelpers.MarshalsOutToLocal(info, context)) | ||
{ | ||
return FreeStrategy.FreeTemp; | ||
} | ||
|
||
// In an unmanaged-to-managed stub, we don't take ownership of the value when it isn't passed by 'ref'. | ||
return FreeStrategy.NoFree; | ||
} | ||
|
||
private static ElementsMarshalling CreateElementsMarshalling(CustomTypeMarshallerData marshallerData, TypePositionInfo elementInfo, IMarshallingGenerator elementMarshaller, TypeSyntax unmanagedElementType, IElementsMarshallingCollectionSource collectionSource) | ||
private static ElementsMarshalling CreateElementsMarshalling(CustomTypeMarshallerData marshallerData, TypePositionInfo elementInfo, IMarshallingGenerator elementMarshaller, TypeSyntax unmanagedElementType, IElementsMarshallingCollectionSource collectionSource, TypeSyntax parameterPointedToType, bool isStateful) | ||
{ | ||
ElementsMarshalling elementsMarshalling; | ||
|
||
|
@@ -507,7 +512,7 @@ private static ElementsMarshalling CreateElementsMarshalling(CustomTypeMarshalle | |
} | ||
else | ||
{ | ||
elementsMarshalling = new NonBlittableElementsMarshalling(unmanagedElementType, elementMarshaller, elementInfo, collectionSource); | ||
elementsMarshalling = new NonBlittableElementsMarshalling(unmanagedElementType, elementMarshaller, elementInfo, collectionSource, parameterPointedToType, isStateful); | ||
} | ||
|
||
return elementsMarshalling; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This order concerns me. We added the finally staements above after we added the cleanup. Inverting this seems like a profound change.