From 66db86e038e3e5d05877feb44fda6cbd7b743ecb Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Tue, 28 Sep 2021 00:26:42 -0700 Subject: [PATCH 1/5] Make Utf16CharMarshaller pin ref parameters --- .../Marshalling/CharMarshaller.cs | 65 ++++++++++++++++--- 1 file changed, 57 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/CharMarshaller.cs b/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/CharMarshaller.cs index bc61f5e39c7c53..8af4562537aa74 100644 --- a/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/CharMarshaller.cs +++ b/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/CharMarshaller.cs @@ -22,16 +22,29 @@ public Utf16CharMarshaller() public ArgumentSyntax AsArgument(TypePositionInfo info, StubCodeContext context) { - string identifier = context.GetIdentifiers(info).native; - if (info.IsByRef) + (string managedIdentifier, string nativeIdentifier) = context.GetIdentifiers(info); + if (!info.IsByRef) + { + // (ushort) + return Argument( + CastExpression( + AsNativeType(info), + IdentifierName(managedIdentifier))); + } + else if (IsPinningPathSupported(info, context)) { + // (ushort*) return Argument( - PrefixUnaryExpression( - SyntaxKind.AddressOfExpression, - IdentifierName(identifier))); + CastExpression( + PointerType(AsNativeType(info)), + IdentifierName(PinnedIdentifier(info.InstanceIdentifier)))); } - return Argument(IdentifierName(identifier)); + // & + return Argument( + PrefixUnaryExpression( + SyntaxKind.AddressOfExpression, + IdentifierName(nativeIdentifier))); } public TypeSyntax AsNativeType(TypePositionInfo info) @@ -52,12 +65,36 @@ public ParameterSyntax AsParameter(TypePositionInfo info) public IEnumerable Generate(TypePositionInfo info, StubCodeContext context) { (string managedIdentifier, string nativeIdentifier) = context.GetIdentifiers(info); + + if (IsPinningPathSupported(info, context)) + { + if (context.CurrentStage == StubCodeContext.Stage.Pin) + { + // fixed (char* = &) + yield return FixedStatement( + VariableDeclaration( + PointerType(PredefinedType(Token(SyntaxKind.CharKeyword))), + SingletonSeparatedList( + VariableDeclarator(Identifier(PinnedIdentifier(info.InstanceIdentifier))) + .WithInitializer(EqualsValueClause( + PrefixUnaryExpression( + SyntaxKind.AddressOfExpression, + IdentifierName(Identifier(managedIdentifier))) + )) + ) + ), + EmptyStatement() + ); + } + yield break; + } + switch (context.CurrentStage) { case StubCodeContext.Stage.Setup: break; case StubCodeContext.Stage.Marshal: - if (info.RefKind != RefKind.Out) + if (info.IsByRef && info.RefKind != RefKind.Out) { yield return ExpressionStatement( AssignmentExpression( @@ -86,8 +123,20 @@ public IEnumerable Generate(TypePositionInfo info, StubCodeCont } } - public bool UsesNativeIdentifier(TypePositionInfo info, StubCodeContext context) => true; + public bool UsesNativeIdentifier(TypePositionInfo info, StubCodeContext context) + { + return info.IsManagedReturnPosition || info.RefKind == RefKind.In || (info.IsByRef && !context.SingleFrameSpansNativeContext); + } public bool SupportsByValueMarshalKind(ByValueContentsMarshalKind marshalKind, StubCodeContext context) => false; + + private bool IsPinningPathSupported(TypePositionInfo info, StubCodeContext context) + { + return context.SingleFrameSpansNativeContext + && !info.IsManagedReturnPosition + && (info.IsByRef && info.RefKind != RefKind.In); + } + + private static string PinnedIdentifier(string identifier) => $"{identifier}__pinned"; } } From 5585626369aa01e07756bef3cdae14e530423676 Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Tue, 28 Sep 2021 00:29:47 -0700 Subject: [PATCH 2/5] Add test --- .../DllImportGenerator.Tests/CharacterTests.cs | 17 +++++++++++++++++ .../TestAssets/NativeExports/Characters.cs | 8 ++++++++ 2 files changed, 25 insertions(+) diff --git a/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.Tests/CharacterTests.cs b/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.Tests/CharacterTests.cs index 3443a5df2bb195..4f0608193e4ce5 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.Tests/CharacterTests.cs +++ b/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.Tests/CharacterTests.cs @@ -1,7 +1,9 @@ // 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.Collections.Generic; +using System.Linq; using System.Runtime.InteropServices; using Xunit; @@ -32,6 +34,9 @@ partial class NativeExportsNE [GeneratedDllImport(NativeExportsNE_Binary, EntryPoint = "char_return_as_uint", CharSet = CharSet.Ansi)] [return: MarshalAs(UnmanagedType.I2)] public static partial char ReturnI2AsI2IgnoreCharSet([MarshalAs(UnmanagedType.I2)] char input); + + [GeneratedDllImport(NativeExportsNE_Binary, EntryPoint = "char_reverse_buffer_ref", CharSet = CharSet.Unicode)] + public static partial void ReverseBuffer(ref char buffer, int len); } public class CharacterTests @@ -81,5 +86,17 @@ public void ValidateIgnoreCharSet(char value, uint expectedUInt) Assert.Equal(expected, NativeExportsNE.ReturnU2AsU2IgnoreCharSet(value)); Assert.Equal(expected, NativeExportsNE.ReturnI2AsI2IgnoreCharSet(value)); } + + [Fact] + public void ValidateRefCharAsBuffer() + { + char[] chars = CharacterMappings().Select(o => (char)o[0]).ToArray(); + char[] expected = new char[chars.Length]; + Array.Copy(chars, expected, chars.Length); + Array.Reverse(expected); + + NativeExportsNE.ReverseBuffer(ref MemoryMarshal.GetArrayDataReference(chars), chars.Length); + Assert.Equal(expected, chars); + } } } diff --git a/src/libraries/System.Runtime.InteropServices/tests/TestAssets/NativeExports/Characters.cs b/src/libraries/System.Runtime.InteropServices/tests/TestAssets/NativeExports/Characters.cs index 95c666eca4f60b..ed4e18e22e19d1 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/TestAssets/NativeExports/Characters.cs +++ b/src/libraries/System.Runtime.InteropServices/tests/TestAssets/NativeExports/Characters.cs @@ -1,6 +1,7 @@ // 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.Runtime.InteropServices; namespace NativeExports @@ -24,5 +25,12 @@ public static void ReturnUIntAsRefUInt(uint input, uint* res) { *res = input; } + + [UnmanagedCallersOnly(EntryPoint = "char_reverse_buffer_ref")] + public static void ReverseBuffer(ushort *buffer, int len) + { + var span = new Span(buffer, len); + span.Reverse(); + } } } From 832bf22d79da0b056c60c778504325364b83c574 Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Tue, 28 Sep 2021 11:04:13 -0700 Subject: [PATCH 3/5] Pin 'in' as well --- .../Marshalling/CharMarshaller.cs | 4 ++-- .../tests/DllImportGenerator.Tests/CharacterTests.cs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/CharMarshaller.cs b/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/CharMarshaller.cs index 8af4562537aa74..fe77d1aba92668 100644 --- a/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/CharMarshaller.cs +++ b/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/CharMarshaller.cs @@ -125,7 +125,7 @@ public IEnumerable Generate(TypePositionInfo info, StubCodeCont public bool UsesNativeIdentifier(TypePositionInfo info, StubCodeContext context) { - return info.IsManagedReturnPosition || info.RefKind == RefKind.In || (info.IsByRef && !context.SingleFrameSpansNativeContext); + return info.IsManagedReturnPosition || (info.IsByRef && !context.SingleFrameSpansNativeContext); } public bool SupportsByValueMarshalKind(ByValueContentsMarshalKind marshalKind, StubCodeContext context) => false; @@ -134,7 +134,7 @@ private bool IsPinningPathSupported(TypePositionInfo info, StubCodeContext conte { return context.SingleFrameSpansNativeContext && !info.IsManagedReturnPosition - && (info.IsByRef && info.RefKind != RefKind.In); + && info.IsByRef; } private static string PinnedIdentifier(string identifier) => $"{identifier}__pinned"; diff --git a/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.Tests/CharacterTests.cs b/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.Tests/CharacterTests.cs index 4f0608193e4ce5..d68aef5f91b883 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.Tests/CharacterTests.cs +++ b/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.Tests/CharacterTests.cs @@ -75,7 +75,7 @@ public void ValidateUnicodeReturns(char expected, uint value) result = initial; NativeExportsNE.ReturnUIntAsUnicode_In(value, in result); - Assert.Equal(initial, result); // Should not be updated when using 'in' + Assert.Equal(expected, result); // Value is updated even when passed with 'in' keyword (matches built-in system) } [Theory] From e7f175cbc98d2f1d454046793edcbef6143bb796 Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Tue, 28 Sep 2021 11:32:23 -0700 Subject: [PATCH 4/5] Add comment about 'in' keyword to compat doc --- docs/design/libraries/DllImportGenerator/Compatibility.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/design/libraries/DllImportGenerator/Compatibility.md b/docs/design/libraries/DllImportGenerator/Compatibility.md index e98b6a4ec5fa27..1cad9039830e3d 100644 --- a/docs/design/libraries/DllImportGenerator/Compatibility.md +++ b/docs/design/libraries/DllImportGenerator/Compatibility.md @@ -69,6 +69,10 @@ Jagged arrays (arrays of arrays) are technically unsupported as was the case in In the source-generated marshalling, arrays will be allocated on the stack (instead of through [`AllocCoTaskMem`](https://docs.microsoft.com/dotnet/api/system.runtime.interopservices.marshal.alloccotaskmem)) if they are passed by value or by read-only reference if they contain at most 256 bytes of data. The built-in system does not support this optimization for arrays. +### `in` keyword + +For some types - blittable or Unicode `char` - passed by read-only reference via the [`in` keyword](https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/in-parameter-modifier), the built-in system simply pins the parameter. The generated marshalling code does the same. A consequence of this behaviour is that any modifications made by the invoked function will be reflected in the caller. It is left to the user to avoid the situation in which `in` is used for a parameter that will actually be modified by the invoked function. + ### `LCIDConversion` support [`LCIDConversionAttribute`](`https://docs.microsoft.com/dotnet/api/system.runtime.interopservices.lcidconversionattribute`) will not be supported for methods marked with `GeneratedDllImportAttribute`. From 84f0b97667859466a79883e3c6d78c9037b1a7d1 Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Tue, 28 Sep 2021 11:38:57 -0700 Subject: [PATCH 5/5] Update docs/design/libraries/DllImportGenerator/Compatibility.md Co-authored-by: Aaron Robinson --- docs/design/libraries/DllImportGenerator/Compatibility.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/design/libraries/DllImportGenerator/Compatibility.md b/docs/design/libraries/DllImportGenerator/Compatibility.md index 1cad9039830e3d..3a6ab0e6c94d59 100644 --- a/docs/design/libraries/DllImportGenerator/Compatibility.md +++ b/docs/design/libraries/DllImportGenerator/Compatibility.md @@ -71,7 +71,7 @@ In the source-generated marshalling, arrays will be allocated on the stack (inst ### `in` keyword -For some types - blittable or Unicode `char` - passed by read-only reference via the [`in` keyword](https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/in-parameter-modifier), the built-in system simply pins the parameter. The generated marshalling code does the same. A consequence of this behaviour is that any modifications made by the invoked function will be reflected in the caller. It is left to the user to avoid the situation in which `in` is used for a parameter that will actually be modified by the invoked function. +For some types - blittable or Unicode `char` - passed by read-only reference via the [`in` keyword](https://docs.microsoft.com/dotnet/csharp/language-reference/keywords/in-parameter-modifier), the built-in system simply pins the parameter. The generated marshalling code does the same. A consequence of this behaviour is that any modifications made by the invoked function will be reflected in the caller. It is left to the user to avoid the situation in which `in` is used for a parameter that will actually be modified by the invoked function. ### `LCIDConversion` support