Implement SetLastError ourselves when allowMarshaling is false

[AArnott]

It's worth noting that CsWin32's support for disabling marshalling won't actually be usable with DisableRuntimeMarshallingAttribute yet because of SetLastError: https://docs.microsoft.com/en-us/dotnet/standard/native-interop/disabled-marshalling#disabled-features

Originally posted by Alex Rønne Petersen (@alexrp) in #593 (comment)

[elachlan]

Helpful links on GetLastError()
https://www.pinvoke.net/default.aspx/kernel32.getlasterror
https://docs.microsoft.com/en-us/previous-versions/dotnet/netframework-3.0/ms182199(v=vs.80)?redirectedfrom=MSDN

[elachlan]

Loni Tra (@lonitra) is this important for the winforms cswin32 project?

[lonitra]

Loni Tra (@lonitra) is this important for the winforms cswin32 project?

I had not seen this attribute before, and it is not present in our codebase, so to my knowledge, I don't think so.

[elachlan]

Loni Tra (@lonitra) is this important for the winforms cswin32 project?

I had not seen this attribute before, and it is not present in our codebase, so to my knowledge, I don't think so.

Does winforms want to use generated code from cswin32 without marshalling?

[AArnott]

Loni Tra (@lonitra) WinForms uses SetLastError. CsWin32 does too. But as WinForms was moving from [DllImport] to [LibraryImport] which implements SetLastError in a trim-friendly way, CsWin32 will need to do the same.

[AArnott]

Lachlan Ennis (@elachlan) Would you be interested in implementing this? We'd need to do something fancy like LibraryImport does to ensure that we get to call GetLastError before the runtime has a chance on that same thread to call anything else that might call SetLastError.

[elachlan]

I can give it a go. Do you know where I can find the .net implementation of it when using library import?

I figure I'd just check the setting and after the pinvoke call, add a call to GetLastError, then throw a win32exception if there is an error code.
https://docs.microsoft.com/en-us/dotnet/api/system.componentmodel.win32exception?view=net-6.0

Am I missing something?

[AArnott]

Thanks!
SetLastError=true does not lead to an exception being thrown, and in this code change we do not propose to add that behavior.

DllImportAttribute.SetLastError indicates that the native method may call SetLastError. Some of these methods only call this in error conditions, and calling this method overwrites any previous value. But this value is in TLS (thread local storage) so each thread has its own copy. If you want to read the value set by a prior Win32 call, you must read it immediately after the call to ensure it wasn't overwritten by later code.

When the CLR emits the interop code for a DllImport method, if the SetLastError property is set to true, the CLR calls SetLastError(0) first, then invokes the method, and then calls GetLastError and stores the result in a CLR-specific TLS memory location so that even if the CLR internally makes other native calls, the user call to Marshal.GetLastWin32Error will always return the value from the last user-p/invoke on that thread.

I can't find it in the documentation now, but I believe when the CLR does do marshaling, it isn't exactly guaranteed that after an interop call completes that the CLR hasn't injected other interop calls for its own purposes, in which case the 'last error' may be overwritten by the time the first interop call returns. This means that if CsWin32 generates a method that tries to imitate the CLR's marshaling behavior, there isn't a guarantee that our own call to GetLastError will actually return the value from our own native call -- it might be from a CLR engine's call into Win32. So we have to be careful.

Now, by targeting net7.0 and using the [LibraryImport(SetLastError=true)] attribute, we can see what kind of code it generates:

internal static partial int GetTickCount()
{
    int __lastError;
    int __retVal;
    {
        System.Runtime.InteropServices.Marshal.SetLastSystemError(0);
        __retVal = __PInvoke();
        __lastError = System.Runtime.InteropServices.Marshal.GetLastSystemError();
    }

    System.Runtime.InteropServices.Marshal.SetLastPInvokeError(__lastError);
    return __retVal;
    // Local P/Invoke
    [System.Runtime.InteropServices.DllImportAttribute("Kernel32", EntryPoint = "GetTickCount", ExactSpelling = true)]
    static extern unsafe int __PInvoke();
}

Notice how the [DllImport] attribute in its generated code is on a local function, and the attribute lacks the SetLastError=true argument. So the extern API is no longer a method that others can call. The method is now a wrapper of the extern method. And that wrapper is equivalent to what the CLR would have produced if it were marshaling the call. This is what CsWin32 must do.

But here's the rub: while net7.0 evidently guarantees the correctness of this code, I don't think the CLR (e.g. net472) makes the same guarantee. Remember how I said the call into the extern method may inadvertently trigger the CLR to make other native calls before it returns? But I guess with Core CLR it's safe. Well, that and perhaps because LibraryImport always generates enough code to ensure that marshaling never occurs.

So... I think CsWin32 should emit this same kind of wrapper, but only when these conditions are true:

1. The metadata indicates this is a SetLastError = true method.
2. allowMarshaling has been set to false
3. we're targeting net7.0 or later.

Still want to tackle it, Lachlan Ennis (@elachlan)?

[elachlan]

Cool! Thanks for the write up, that is super helpful. Don't assign it to me just yet. I might focus on a couple of other issues first. But It seems mostly straight forward. I will see if I get time this week.

[jlaanstra]

Would it be possible to at least not generate the SetLastError when allowMarshalling is false? I can write the surrounding code myself but SetLastError is making CsWIn32 unusable in project with marshalling disabled.