From 47007229997216749a6fea744e8c26465b0d36c3 Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Mon, 30 Nov 2020 10:41:11 -0800 Subject: [PATCH] Handle SetLastError=true (dotnet/runtimelab#360) Commit migrated from https://github.com/dotnet/runtimelab/commit/b36b0b8adc4d5c3e7f2927381ea800444f870f96 --- .../DllImportGenerator/DllImportGenerator.cs | 6 -- .../DllImportGenerator/StubCodeGenerator.cs | 53 ++++++++++- .../libraries/DllImportGenerator/Pipeline.md | 15 +++ .../Ancillary.Interop.csproj | 1 + .../tests/Ancillary.Interop/MarshalEx.cs | 74 +++++++++++++++ .../SetLastErrorTests.cs | 94 +++++++++++++++++++ .../CompileFails.cs | 3 +- .../DllImportGenerator.UnitTests/Compiles.cs | 28 +----- .../tests/TestAssets/NativeExports/Error.cs | 66 +++++++++++++ 9 files changed, 307 insertions(+), 33 deletions(-) create mode 100644 src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.Tests/SetLastErrorTests.cs create mode 100644 src/libraries/System.Runtime.InteropServices/tests/TestAssets/NativeExports/Error.cs diff --git a/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/DllImportGenerator.cs b/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/DllImportGenerator.cs index 993a7b358e3ee..86bb2467ab17c 100644 --- a/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/DllImportGenerator.cs +++ b/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/DllImportGenerator.cs @@ -107,12 +107,6 @@ public void Execute(GeneratorExecutionContext context) generatorDiagnostics.ReportConfigurationNotSupported(generatedDllImportAttr, nameof(DllImportStub.GeneratedDllImportData.ThrowOnUnmappableChar)); } - // [TODO] Remove once we support SetLastError=true - if (dllImportData.SetLastError) - { - generatorDiagnostics.ReportConfigurationNotSupported(generatedDllImportAttr, nameof(DllImportStub.GeneratedDllImportData.SetLastError), "true"); - } - if (lcidConversionAttr != null) { // Using LCIDConversion with GeneratedDllImport is not supported diff --git a/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/StubCodeGenerator.cs b/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/StubCodeGenerator.cs index 7b5dc53f92290..e11337b791e1e 100644 --- a/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/StubCodeGenerator.cs +++ b/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/StubCodeGenerator.cs @@ -30,6 +30,10 @@ internal sealed class StubCodeGenerator : StubCodeContext public string ReturnNativeIdentifier { get; private set; } = ReturnIdentifier; private const string InvokeReturnIdentifier = "__invokeRetVal"; + private const string LastErrorIdentifier = "__lastError"; + + // Error code representing success. This maps to S_OK for Windows HRESULT semantics and 0 for POSIX errno semantics. + private const int SuccessErrorCode = 0; private static readonly Stage[] Stages = new Stage[] { @@ -170,6 +174,14 @@ public override (string managed, string native) GetIdentifiers(TypePositionInfo AppendVariableDeclations(setupStatements, retMarshaller.TypeInfo, retMarshaller.Generator); } + if (this.dllImportData.SetLastError) + { + // Declare variable for last error + setupStatements.Add(MarshallerHelpers.DeclareWithDefault( + PredefinedType(Token(SyntaxKind.IntKeyword)), + LastErrorIdentifier)); + } + var tryStatements = new List(); var finallyStatements = new List(); var invoke = InvocationExpression(IdentifierName(dllImportName)); @@ -235,11 +247,37 @@ public override (string managed, string native) GetIdentifiers(TypePositionInfo invoke)); } + if (this.dllImportData.SetLastError) + { + // Marshal.SetLastSystemError(0); + var clearLastError = ExpressionStatement( + InvocationExpression( + MemberAccessExpression( + SyntaxKind.SimpleMemberAccessExpression, + ParseName(TypeNames.System_Runtime_InteropServices_MarshalEx), + IdentifierName("SetLastSystemError")), + ArgumentList(SingletonSeparatedList( + Argument(LiteralExpression(SyntaxKind.NumericLiteralExpression, Literal(SuccessErrorCode))))))); + + // = Marshal.GetLastSystemError(); + var getLastError = ExpressionStatement( + AssignmentExpression( + SyntaxKind.SimpleAssignmentExpression, + IdentifierName(LastErrorIdentifier), + InvocationExpression( + MemberAccessExpression( + SyntaxKind.SimpleMemberAccessExpression, + ParseName(TypeNames.System_Runtime_InteropServices_MarshalEx), + IdentifierName("GetLastSystemError"))))); + + invokeStatement = Block(clearLastError, invokeStatement, getLastError); + } + // Nest invocation in fixed statements if (fixedStatements.Any()) { fixedStatements.Reverse(); - invokeStatement = fixedStatements.First().WithStatement(Block(invokeStatement)); + invokeStatement = fixedStatements.First().WithStatement(invokeStatement); foreach (var fixedStatement in fixedStatements.Skip(1)) { invokeStatement = fixedStatement.WithStatement(Block(invokeStatement)); @@ -274,6 +312,19 @@ public override (string managed, string native) GetIdentifiers(TypePositionInfo allStatements.AddRange(tryStatements); } + if (this.dllImportData.SetLastError) + { + // Marshal.SetLastWin32Error(); + allStatements.Add(ExpressionStatement( + InvocationExpression( + MemberAccessExpression( + SyntaxKind.SimpleMemberAccessExpression, + ParseName(TypeNames.System_Runtime_InteropServices_MarshalEx), + IdentifierName("SetLastWin32Error")), + ArgumentList(SingletonSeparatedList( + Argument(IdentifierName(LastErrorIdentifier))))))); + } + // Return if (!stubReturnsVoid) allStatements.Add(ReturnStatement(IdentifierName(ReturnIdentifier))); diff --git a/src/libraries/System.Runtime.InteropServices/gen/docs/design/libraries/DllImportGenerator/Pipeline.md b/src/libraries/System.Runtime.InteropServices/gen/docs/design/libraries/DllImportGenerator/Pipeline.md index 6ff67b39ad69c..a751257bcee7f 100644 --- a/src/libraries/System.Runtime.InteropServices/gen/docs/design/libraries/DllImportGenerator/Pipeline.md +++ b/src/libraries/System.Runtime.InteropServices/gen/docs/design/libraries/DllImportGenerator/Pipeline.md @@ -139,6 +139,21 @@ These various scenarios have different levels of support for these three feature | non-blittable array marshalling in a P/Invoke | unsupported | unsupported (supportable with https://github.com/dotnet/runtime/issues/25423) | unuspported | | non-blittable array marshalling not in a P/Invoke | unsupported | unsupported | unuspported | +### `SetLastError=true` + +The stub code generation also handles [`SetLastError=true`][SetLastError] behaviour. This configuration indicates that system error code ([`errno`](https://en.wikipedia.org/wiki/Errno.h) on Unix, [`GetLastError`](https://docs.microsoft.com/windows/win32/api/errhandlingapi/nf-errhandlingapi-getlasterror) on Windows) should be stored after the native invocation, such that it can be retrieved using [`Marshal.GetLastWin32Error`](https://docs.microsoft.com/dotnet/api/system.runtime.interopservices.marshal.getlastwin32error). + +This means that, rather than simply invoke the native method, the generated stub will: + +1. Clear the system error by setting it to 0 +2. Invoke the native method +3. Get the system error +4. Set the stored error for the P/Invoke (accessible via `Marshal.GetLastWin32Error`) + +A core requirement of this functionality is that the P/Invoke called in (2) is blittable (the purpose of the P/Invoke source generator), such that there will be no additional operations (e.g unmarshalling) after the invocation that could change the system error that is retrieved in (3). Similarly, (3) must not involve any operations before getting the system error that could change the system error. This also relies on the runtime itself handling preserving the last error (see `BEGIN/END_PRESERVE_LAST_ERROR` macros) during JIT and P/Invoke resolution. + +Clearing the system error (1) is necessary because the native method may not set the error at all on success and the system error would retain its value from a previous operation. The developer should be able to check `Marshal.GetLastWin32Error` after a P/Inovke to determine success or failure, so the stub explicitly clears the error before the native invocation, such that the last error will indicate success if the native call does not change it. + ## P/Invoke The P/Invoke called by the stub is created based on the user's original declaration of the stub. The signature is generated using the syntax returned by `AsNativeType` and `AsParameter` of the marshalling generators for the return and parameters. Any marshalling attributes on the return and parameters of the managed method - [`MarshalAsAttribute`][MarshalAsAttribute], [`InAttribute`][InAttribute], [`OutAttribute`][OutAttribute] - are dropped. diff --git a/src/libraries/System.Runtime.InteropServices/tests/Ancillary.Interop/Ancillary.Interop.csproj b/src/libraries/System.Runtime.InteropServices/tests/Ancillary.Interop/Ancillary.Interop.csproj index e9063db196bec..642e04fe9e5d5 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/Ancillary.Interop/Ancillary.Interop.csproj +++ b/src/libraries/System.Runtime.InteropServices/tests/Ancillary.Interop/Ancillary.Interop.csproj @@ -5,6 +5,7 @@ 8.0 System.Runtime.InteropServices enable + true diff --git a/src/libraries/System.Runtime.InteropServices/tests/Ancillary.Interop/MarshalEx.cs b/src/libraries/System.Runtime.InteropServices/tests/Ancillary.Interop/MarshalEx.cs index 2633bd26e273d..a5ad55f72e687 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/Ancillary.Interop/MarshalEx.cs +++ b/src/libraries/System.Runtime.InteropServices/tests/Ancillary.Interop/MarshalEx.cs @@ -39,5 +39,79 @@ public static void SetHandle(SafeHandle safeHandle, IntPtr handle) { typeof(SafeHandle).GetMethod("SetHandle", BindingFlags.NonPublic | BindingFlags.Instance)!.Invoke(safeHandle, new object[] { handle }); } + + /// + /// Set the last platform invoke error on the thread + /// + public static void SetLastWin32Error(int error) + { + typeof(Marshal).GetMethod("SetLastWin32Error", BindingFlags.NonPublic | BindingFlags.Static)!.Invoke(null, new object[] { error }); + } + + /// + /// Get the last system error on the current thread (errno on Unix, GetLastError on Windows) + /// + public static unsafe int GetLastSystemError() + { + // Would be internal call that handles getting the last error for the thread using the PAL + + if (OperatingSystem.IsWindows()) + { + return Kernel32.GetLastError(); + } + else if (OperatingSystem.IsMacOS()) + { + return *libc.__error(); + } + else if (OperatingSystem.IsLinux()) + { + return *libc.__errno_location(); + } + + throw new NotImplementedException(); + } + + /// + /// Set the last system error on the current thread (errno on Unix, SetLastError on Windows) + /// + public static unsafe void SetLastSystemError(int error) + { + // Would be internal call that handles setting the last error for the thread using the PAL + + if (OperatingSystem.IsWindows()) + { + Kernel32.SetLastError(error); + } + else if (OperatingSystem.IsMacOS()) + { + *libc.__error() = error; + } + else if (OperatingSystem.IsLinux()) + { + *libc.__errno_location() = error; + } + else + { + throw new NotImplementedException(); + } + } + + private class Kernel32 + { + [DllImport(nameof(Kernel32))] + public static extern void SetLastError(int error); + + [DllImport(nameof(Kernel32))] + public static extern int GetLastError(); + } + + private class libc + { + [DllImport(nameof(libc))] + internal static unsafe extern int* __errno_location(); + + [DllImport(nameof(libc))] + internal static unsafe extern int* __error(); + } } } diff --git a/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.Tests/SetLastErrorTests.cs b/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.Tests/SetLastErrorTests.cs new file mode 100644 index 0000000000000..ba59f95803a4b --- /dev/null +++ b/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.Tests/SetLastErrorTests.cs @@ -0,0 +1,94 @@ +using System; +using System.Runtime.InteropServices; + +using Xunit; + +namespace DllImportGenerator.IntegrationTests +{ + [BlittableType] + public struct SetLastErrorMarshaller + { + public int val; + + public SetLastErrorMarshaller(int i) + { + val = i; + } + + public int ToManaged() + { + // Explicity set the last error to something else on unmarshalling + MarshalEx.SetLastWin32Error(val * 2); + return val; + } + } + + partial class NativeExportsNE + { + public partial class SetLastError + { + [GeneratedDllImport(nameof(NativeExportsNE), EntryPoint = "set_error", SetLastError = true)] + public static partial int SetError(int error, byte shouldSetError); + + [GeneratedDllImport(nameof(NativeExportsNE), EntryPoint = "set_error_return_string", SetLastError = true)] + [return: MarshalUsing(typeof(SetLastErrorMarshaller))] + public static partial int SetError_CustomMarshallingSetsError(int error, byte shouldSetError); + + [GeneratedDllImport(nameof(NativeExportsNE), EntryPoint = "set_error_return_string", SetLastError = true)] + [return: MarshalAs(UnmanagedType.LPWStr)] + public static partial string SetError_NonBlittableSignature(int error, [MarshalAs(UnmanagedType.U1)] bool shouldSetError, [MarshalAs(UnmanagedType.LPWStr)] string errorString); + } + } + + public class SetLastErrorTests + { + [Theory] + [InlineData(0)] + [InlineData(2)] + [InlineData(-5)] + public void LastWin32Error_HasExpectedValue(int error) + { + string errorString = error.ToString(); + string ret = NativeExportsNE.SetLastError.SetError_NonBlittableSignature(error, shouldSetError: true, errorString); + Assert.Equal(error, Marshal.GetLastWin32Error()); + Assert.Equal(errorString, ret); + + // Clear the last error + MarshalEx.SetLastWin32Error(0); + + NativeExportsNE.SetLastError.SetError(error, shouldSetError: 1); + Assert.Equal(error, Marshal.GetLastWin32Error()); + + MarshalEx.SetLastWin32Error(0); + + // Custom marshalling sets the last error on unmarshalling. + // Last error should reflect error from native call, not unmarshalling. + NativeExportsNE.SetLastError.SetError_CustomMarshallingSetsError(error, shouldSetError: 1); + Assert.Equal(error, Marshal.GetLastWin32Error()); + } + + [Fact] + public void ClearPreviousError() + { + int error = 100; + MarshalEx.SetLastWin32Error(error); + + // Don't actually set the error in the native call. SetLastError=true should clear any existing error. + string errorString = error.ToString(); + string ret = NativeExportsNE.SetLastError.SetError_NonBlittableSignature(error, shouldSetError: false, errorString); + Assert.Equal(0, Marshal.GetLastWin32Error()); + Assert.Equal(errorString, ret); + + MarshalEx.SetLastWin32Error(error); + + // Don't actually set the error in the native call. SetLastError=true should clear any existing error. + NativeExportsNE.SetLastError.SetError(error, shouldSetError: 0); + Assert.Equal(0, Marshal.GetLastWin32Error()); + + // Don't actually set the error in the native call. Custom marshalling still sets the last error. + // SetLastError=true should clear any existing error and ignore error set by custom marshalling. + NativeExportsNE.SetLastError.SetError_CustomMarshallingSetsError(error, shouldSetError: 0); + Assert.Equal(0, Marshal.GetLastWin32Error()); + } + } +} diff --git a/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/CompileFails.cs b/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/CompileFails.cs index 7500d50464ce3..d050b72617d9e 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/CompileFails.cs +++ b/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/CompileFails.cs @@ -41,8 +41,7 @@ public static IEnumerable CodeSnippetsToCompile() // Unsupported named arguments // * BestFitMapping, ThrowOnUnmappableChar - // [TODO]: Expected diagnostic count should be 2 once we support SetLastError - yield return new object[] { CodeSnippets.AllDllImportNamedArguments, 3, 0 }; + yield return new object[] { CodeSnippets.AllDllImportNamedArguments, 2, 0 }; // LCIDConversion yield return new object[] { CodeSnippets.LCIDConversionAttribute, 1, 0 }; diff --git a/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/Compiles.cs b/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/Compiles.cs index c51787baa1a69..07fef3b9500e1 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/Compiles.cs +++ b/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/Compiles.cs @@ -9,7 +9,7 @@ namespace DllImportGenerator.UnitTests { public class Compiles { - public static IEnumerable CodeSnippetsToCompile_NoDiagnostics() + public static IEnumerable CodeSnippetsToCompile() { yield return new[] { CodeSnippets.TrivialClassDeclarations }; yield return new[] { CodeSnippets.TrivialStructDeclarations }; @@ -17,7 +17,7 @@ public static IEnumerable CodeSnippetsToCompile_NoDiagnostics() yield return new[] { CodeSnippets.NestedNamespace }; yield return new[] { CodeSnippets.NestedTypes }; yield return new[] { CodeSnippets.UserDefinedEntryPoint }; - //yield return new[] { CodeSnippets.AllSupportedDllImportNamedArguments }; + yield return new[] { CodeSnippets.AllSupportedDllImportNamedArguments }; yield return new[] { CodeSnippets.DefaultParameters }; yield return new[] { CodeSnippets.UseCSharpFeaturesForConstants }; @@ -161,14 +161,9 @@ public static IEnumerable CodeSnippetsToCompile_NoDiagnostics() yield return new[] { CodeSnippets.CustomStructMarshallingMarshalUsingParametersAndModifiers }; } - public static IEnumerable CodeSnippetsToCompile_WithDiagnostics() - { - yield return new[] { CodeSnippets.AllSupportedDllImportNamedArguments }; - } - [Theory] - [MemberData(nameof(CodeSnippetsToCompile_NoDiagnostics))] - public async Task ValidateSnippets_NoDiagnostics(string source) + [MemberData(nameof(CodeSnippetsToCompile))] + public async Task ValidateSnippets(string source) { Compilation comp = await TestUtils.CreateCompilation(source); TestUtils.AssertPreSourceGeneratorCompilation(comp); @@ -179,20 +174,5 @@ public async Task ValidateSnippets_NoDiagnostics(string source) var newCompDiags = newComp.GetDiagnostics(); Assert.Empty(newCompDiags); } - - [Theory] - [MemberData(nameof(CodeSnippetsToCompile_WithDiagnostics))] - public async Task ValidateSnippets_WithDiagnostics(string source) - { - Compilation comp = await TestUtils.CreateCompilation(source); - TestUtils.AssertPreSourceGeneratorCompilation(comp); - - var newComp = TestUtils.RunGenerators(comp, out var generatorDiags, new Microsoft.Interop.DllImportGenerator()); - Assert.NotEmpty(generatorDiags); - Assert.All(generatorDiags, d => Assert.StartsWith(Microsoft.Interop.GeneratorDiagnostics.Ids.Prefix, d.Id)); - - var newCompDiags = newComp.GetDiagnostics(); - Assert.Empty(newCompDiags); - } } } diff --git a/src/libraries/System.Runtime.InteropServices/tests/TestAssets/NativeExports/Error.cs b/src/libraries/System.Runtime.InteropServices/tests/TestAssets/NativeExports/Error.cs new file mode 100644 index 0000000000000..8963c8138712d --- /dev/null +++ b/src/libraries/System.Runtime.InteropServices/tests/TestAssets/NativeExports/Error.cs @@ -0,0 +1,66 @@ +using System; +using System.Runtime.InteropServices; + +namespace NativeExports +{ + public static unsafe class Error + { + private class Kernel32 + { + [DllImport(nameof(Kernel32))] + public static extern void SetLastError(int error); + + [DllImport(nameof(Kernel32))] + public static extern int GetLastError(); + } + + private class libc + { + [DllImport(nameof(libc))] + internal static unsafe extern int* __errno_location(); + + [DllImport(nameof(libc))] + internal static unsafe extern int* __error(); + } + + [UnmanagedCallersOnly(EntryPoint = "set_error")] + public static int SetError(int error, byte shouldSetError) + { + if (shouldSetError != 0) + SetLastError(error); + + return error; + } + + [UnmanagedCallersOnly(EntryPoint = "set_error_return_string")] + public static ushort* SetErrorReturnString(int error, byte shouldSetError, ushort* errorString) + { + ushort* ret = (ushort*)Marshal.StringToCoTaskMemUni(new string((char*)errorString)); + + if (shouldSetError != 0) + SetLastError(error); + + return ret; + } + + private static void SetLastError(int error) + { + if (OperatingSystem.IsWindows()) + { + Kernel32.SetLastError(error); + } + else if (OperatingSystem.IsMacOS()) + { + *libc.__error() = error; + } + else if (OperatingSystem.IsLinux()) + { + *libc.__errno_location() = error; + } + else + { + throw new NotImplementedException(); + } + } + } +}