Expose setting/getting of system error code

Background and Motivation

.NET developers can use SetLastError on a P/Invoke to specify whether or not the last error from the invoked function should be stored. The error is then retrievable through Marshal.GetLastWin32Error.

The runtime's built-in interop marshallers are currently for clearing the system error before invoking the function, getting the system error after invoking the function, and storing the error as the last P/Invoke error code.

With the proposed P/Invoke source generation, a tool (Roslyn source generator) outside of the runtime is responsible for handling the system error such that it can be used/checked by the caller of the P/Invoke.

The proposed APIs would support a source generator's ability to handle this through new APIs for getting/setting the system error and setting the last P/Invoke error.

Proposed API

New APIs to allow getting and setting the system error:

namespace System.Runtime.InteropServices
{
    public static class Marshal
    {
+        /// <summary>
+        /// Get the last system error on the current thread
+        /// </summary>
+        /// <remarks>
+        /// The error is that for the current operating system (e.g. errno on Unix, GetLastError on Windows)
+        /// </remarks>
+        public static int GetLastSystemError();

+        /// <summary>
+        /// Set the last system error on the current thread
+        /// </summary>
+        /// <remarks>
+        /// The error is that for the current operating system (e.g. errno on Unix, SetLastError on Windows)
+        /// </remarks>
+        public static void SetLastSystemError(int error);

+        /// <summary>
+        /// Set the last platform invoke error on the current thread 
+        /// </summary>
+        public static void SetLastWin32Error(int error);
    }
}

Usage Examples

Example possible usage from a source generator:

public static partial (int, bool) InvokeWithError(int i)
{
    unsafe
    {
        bool __retVal = default;
        int __retVal_gen_native = default;
        int __lastError = default;
        //
        // Invoke
        //
        {
            System.Runtime.InteropServices.Marshal.SetLastSystemError(0);
            __retVal_gen_native = InvokeWithError__PInvoke__(i);
            __lastError = System.Runtime.InteropServices.Marshal.GetLastSystemError();
        }
        //
        // Unmarshal
        //
        __retVal = __retVal_gen_native != 0;
        System.Runtime.InteropServices.Marshal.SetLastWin32Error(__lastError);
        return (__lastError, __retVal);
    }
}

[System.Runtime.InteropServices.DllImportAttribute("Microsoft.Interop.Tests.NativeExportsNE", EntryPoint = "invoke_with_error")]
extern private static unsafe int InvokeWithError__PInvoke__(int i);

Alternative Designs

Get/SetLastSystemError could be written outside of the runtime (e.g. by invoking into a native function for each platform to match how the runtime gets/sets the system error). Exposing a public API allows developers to get/set the error in a way that is consistent with the runtime on all platforms.

Source generators for P/Invokes technically only need to clear the last system error (set it to 0), not set it to any arbitrary value. Providing only a Get/Clear (instead of Get/Set) seems inconsistent with existing APIs. The SetLastSystemError would also give managed functions called from native code the option to explicitly set an error without handling different platforms themselves.

The name of SetLastWin32Error does not properly reflect the error it would actually set (the last P/Invoke error, not Win32 error), but it is named for consistency with the existing GetLastWin32Error. An alternative is to create a new pair that is more accurately named:

namespace System.Runtime.InteropServices
{
    public static class Marshal
    {
+        /// <summary>
+        /// Get the last platform invoke error on the current thread 
+        /// </summary>
+        public static int GetLastPInvokeError();

+        /// <summary>
+        /// Set the last platform invoke error on the current thread 
+        /// </summary>
+        public static void SetLastPInvokeError(int error);
    }
}

The proposed APIs will be needed for any handling of last system error by a third party. They will allow possible future additions such a specific types for propagating the system error to the caller - for example:

namespace System.Runtime.InteropServices
{
    /// <summary>
    /// Represents the return value and system error for a function
    /// </summary>
    /// <typeparam name="T">The type of the value</typeparam>
    public readonly struct SystemErrorAndValue<T>
    {
        public int Error { get; }
        public T Value { get; }
        public SystemErrorAndValue(int error, T value);

        [EditorBrowsable(EditorBrowsableState.Never)]
        public void Deconstruct(out int error, out T value);
    }

    /// <summary>
    /// Represents the system error for a function
    /// </summary>
    public readonly struct SystemError
    {
        public int Error { get; }
        public SystemError(int error);
    }
}

Additions like these can be handled by the third party / source generator, so are not part of this proposal.

Risks

These APIs expose system error code setting that was hidden within the runtime, allowing an easy way for a developer could set the system error. This is already achievable without an API by explicitly P/Invoking into a native function. It would also be possible for a developer to set the last P/Invoke error (call Marshal.SetLastWin32Error) outside of a P/Invoke.

@AaronRobinsonMSFT

/cc @AaronRobinsonMSFT @jkoritzinsky

@elinor-fung

@elinor-fung I don't think GetLastSystemError or GetLastSystemError need to be marked unsafe.

Oops, fixed.

The difference between XXXLastSystemError and XXXLastWin32Error is not indicated by their names, especially if not in Windows.

It seems to me that this is our opportunity to remove the Win32 part and create a new Get/Set pair with more explanatory names (Get/SetLastPinvokeError?) with GetLastWin32Error remaining for compatibility.

#26227 (comment)

remove the Win32 part and create a new Get/Set pair with more explanatory names (Get/SetLastPinvokeError?) with GetLastWin32Error remaining for compatibility.

The last error pattern is bug-prone. I have seen countless of bugs over the years where the last error is accidentally overwritten. To improve this with source generated interop, we may want to get away with the cached last error concept completely and instead teach the marshalers to include the last system error in the method result. It would address the Win32 name problem as side-effect.

[GeneratedDllImportAttribute("kernel32.dll")]
extern static ValueAndSystemError<bool> CreateFile(....);

...
    var result = CreateFile(...);
    if (!result.Value)
         throw ExceptionFromSystemError(result.SystemError);
...

Definitely like the idea of getting rid of the existing last error mechanism.

So it seems like it would be:

No SetLastError field for GeneratedDllImportAttribute
Source-generated P/Invokes don't participate in Marshal.GetLastWin32Error
New type that developers can use as an indication for the marshallers that the system error is desired (e.g. ValueAndSystemError<T> and SystemError)

Is ExceptionFromSystemError a proposed new type? Or did you mean a utility method like ThrowExceptionFromSystemError and if so would that be often simply throw new Win32Exception() ?

Would eliminating SetLastError (making it always true, same as VB default) cause a measurable perf impact (when calling random API that don't participate in setting the last error)? Although I certainly agree it would be nice to get rid of this error prone requirement.

Would eliminating SetLastError (making it always true, same as VB default) cause a measurable perf impact

Yes, it would have a measurable perf impact for cheap APIs.

Video

Looks good as proposed
The naming of SetLastWin32Error() is unfortunate, because it really sets the last P/Invoke error, regardless of OS but it matches the existing method name. Let's just obsolete the current concept and add a new pair. This also makes people aware of the conceptual difference between GetLastSystemError() and GetLastPInvokeError().

namespace System.Runtime.InteropServices
{
    public static class Marshal
    {
        public static int GetLastSystemError();
        public static void SetLastSystemError(int error);

        public static int GetLastPInvokeError();
        public static void SetLastPInvokeError(int error);

        [Obsolete("Use GetLastSystemError() or GetLastPInvokeError()", DiagnosticId="<NextID>", UrlFormat="<the proper URL>")]
        public static int GetLastWin32Error();
    }
}

[Obsolete("Use GetLastSystemError() or GetLastPInvokeError()"...

This will be very noisy warning. We have ~500 calls to of Marshal.GetLastWin32Error in dotnet/runtime libraries alone and we won't be able to really fix many of them since the libraries cross-compile for down-level targets. None of the obsoletions so far were as noisy as this.

Also, should the message just say Use GetLastPInvokeError()? One cannot safely replace calls to GetLastWin32Error with GetLastSystemError without auditing and changing other code. GetLastPInvokeError() is the right easy fix.

Looking at telemetry, you're right:

API Port Telemetry: 30%
NetFx compat lab: 48%
NuGet 0.5%

The assumption was that this is an advanced API. If we expect this API to be that widely used, obsoleting might be a bit harsh.

Also, should the message just say Use GetLastPInvokeError()? One cannot safely replace calls to GetLastWin32Error with GetLastSystemError without auditing and changing other code. GetLastPInvokeError() is the right easy fix.

I think the reasoning to recommend either was that people didn't necessarily understand that GetLastWin32Error() only returns the last trapped error, rather than calling straight to the Win32 GetLastError() function.

It would be fine to recommend GetLastSystemError as a potential replacement, but it would have to come with a longer explanation about other refactoring that one has to do do make it reliable. A simple TLDR suggestion is to replace it with GetLastPInvokeError that is safe without much thinking 100% of time. This discussion may be irrelevant if we drop obsoleting the existing method.

I got a better idea, for places that use the GetLastWin32Error have ifdefs where if .NET 6+ then use the new api, else use old api for the downlevel stuff that way everyone is happy and then also obsolete the old as well too without too much noise.

plus the runtime uses ifdefs anyway on the versions of the TFMS they target anyway so.

plus the runtime uses ifdefs anyway on the versions of the TFMS they target anyway so.

We do use ifdefs in C# as last resort. We strongly prefer code without ifdefs.

Why would we have to #if in dotnet/runtime? Why couldn't we just use the new API?

There are 200+ projects under src/libraries that target netstandard that cannot use the new API. It is not unusual for these projects to multi-target or share files with projects that target netcoreapp current. So some of the ~500 calls Marshal.GetLastWin32Error would have to stay intact and some of them would have to be under ifdef or disable the obsolete warning.

Implemented in #51505.

@elinor-fung

@elinor-fung would it make sense for you to add a little more to the doc pages for the new methods?

Gets the last system error on the current thread
Returns the error code returned by the last unmanaged function that was called using platform invoke that has the SetLastError flag set.
Get the last platform invoke error on the current thread

It isn't clear which to use when. We also do not have any note in GetLastWin32Error explaining to use other methods in preference when targeting >=.NET 6.0

Worst of all, two of these descriptions don't end with a period

dotnet/dotnet-api-docs#7334

Worst of all, two of these descriptions don't end with a period

This statement amused me more than is probably normal.

Navigation Menu

Expose setting/getting of system error code #46843

Background and Motivation

Proposed API

Usage Examples

Alternative Designs

Risks

16 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Expose setting/getting of system error code #46843

Description

Background and Motivation

Proposed API

Usage Examples

Alternative Designs

Risks

Activity

elinor-fung commented on Jan 12, 2021

AaronRobinsonMSFT commented on Jan 12, 2021

elinor-fung commented on Jan 12, 2021

danmoseley commented on Jan 12, 2021

danmoseley commented on Jan 12, 2021

jkotas commented on Jan 12, 2021

elinor-fung commented on Jan 12, 2021

danmoseley commented on Jan 12, 2021

danmoseley commented on Jan 12, 2021

jkotas commented on Jan 12, 2021

16 remaining items

terrajobst commented on Jan 26, 2021

jkotas commented on Jan 26, 2021

terrajobst commented on Jan 26, 2021

jkotas commented on Jan 26, 2021

AraHaan commented on Jan 26, 2021

jkotas commented on Jan 26, 2021

terrajobst commented on Jan 26, 2021

jkotas commented on Jan 27, 2021

elinor-fung commented on Apr 27, 2021

danmoseley commented on Oct 28, 2021

elinor-fung commented on Oct 29, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions