SetLastError=true missing on some extern declarations

[danmoseley]

There are quite a lot of extern declarations for Win32 API in the winforms repo that don't use SetLastError=true even though the docs indicate the API sets an error retrievable with GetLastError. (I attached an approximate list of some).

Of course in many, possibly all of those cases the code never calls Marshal.GetLastWin32Error() nor throws Win32Exception so it doesn't matter. I am not sure whether to interpret our guidance  to mean "apply SetLastError=true whenever the API is documented to set last error" or "apply SetLastError=true whenever the API sets last error and you also intend to retrieve it". The latter saves the infrastructure calling GetLastError() for you unnecessarily, but that is presumably very fast - and avoids the possibility of reading some previous API's error because you hadn't updated the extern declaration.

@AaronRobinsonMSFT @JeremyKuhne ?

list.txt

[AaronRobinsonMSFT]

I am not sure whether to interpret our guidance to mean "apply SetLastError=true whenever the API is documented to set last error" or "apply SetLastError=true whenever the API sets last error and you also intend to retrieve it"

@danmosemsft It is typically the latter. If the caller isn't concerned about the real underlying error then there isn't a real need to capture or provide it. That being said, what wonderful bugs might we find if we do start checking it?

[RussKie]

We have been uplifting our interops for the past year, we verify signature correctness (e.g. uint for DOWRD, InPtr instead of int, BOOL instead of int or bool etc), and verify correct decorations are applies. Most, if not all, interops in System.Windows.Forms.Primitives project have been reviewed and corrected. Those interops that aren't in that project are on the TODO list.

I haven't personally observed many Marshal.GetLastWin32Error() or throws Win32Exception, so it could be attributed to original omissions from years ago. We can certainly look into rectifying these gaps, if there is a substantial value in this work.

That being said, what wonderful bugs might we find if we do start checking it?

Do we really want to know? :D

[JeremyKuhne]

In general the guideline should be to set it (when it is documented that the API does so) and not ignore error states. For perf reasons, however, it can be skipped if you don't intend to check it. Note that the perf there is very small, and measurable on only the simplest of APIs. I would never skip it for something that does a ton of work or isn't called in tight loops.

For compat reasons (not wanting to introduce a new exception) we may not want to introduce error checking where we weren't before, but I'd recommend that we try to at least add debug asserts to flush out where we could otherwise be doing better (or worst case document the error states we're seeing). WinForms, for example, is very interop heavy and undoubtedly is throwing itself under the bus in a number of places without realizing it as it wasn't thorough about error checking. I'd be surprised if we didn't improve the overall robustness of WinForms substantively if we were more thorough. :)

[danmoseley]

Speaking generally, this all seems very prone to error

1. One must declare them correctly. There is no indication you have gotten it wrong - one must check each against MSDN. (To generate the list of 75 above, I compared against https://github.com/AArnott/pinvoke with some regexes. This assumes that repo is correct, of course.)
2. One must remember to GetLastWin32Error directly or indirectly - or decide not to.

In both cases, there is no compile or runtime error if you get it wrong. Just subtle issues like reading the error from a previous API call.

Perhaps someone has written an analyzer to catch at least (1) using a database of Win32 API.

I noticed that WPF segregates API into NativeMethods and NativeMethodsSetLastError possibly to help with this.

[weltkante]

One must declare them correctly.

I don't think you have to. As far as I understand it this flag exists to tell the CLR that it should not go stomping over the win32 error flag by saving it away in case you need it later. Technically you can always set it true at a small performance cost of having to save/restore the error flag.

Leaving it false is just a performance optimization when you definitely know you aren't going to check the flag. So you could treat it as any performance optimization: first measure, and if you know it helps disable it.

As far as analyzers are concerned, any call to Marshal.GetLastWin32Error() or new Win32Exception() should be preceeded unconditionally in the control flow by an interop method having the flag set. Having reverse control flow paths leading to interop calls without that flag, or performing unrelated method calls in between (risks jit stomping over the flag) should raise a warning.

In other words I'd take the reverse point of view, instead of painstakingly annotate each method

• default to SetLastError=true unless its performance sensitive and known to not needing to do error handling
• have an analyzer which ensures reading the error flag is preceeded by a correctly annotated interop call

For performance sensitive code it may be useful to have both variants available, so that only the performance sensitive code calls the version with SetLastError=false

(Just throwing that in as my opinion on the matter.)

[AaronRobinsonMSFT]

Perhaps someone has written an analyzer to catch at least (1) using a database of Win32 API.

@elinor-fung https://github.com/dotnet/runtime/issues/34831

[JeremyKuhne]

To generate the list of 75 above

I'd be wary about your results. The first one I looked at is wrong. IsChild does not set the last error. While the docs are correct in this case I've also found when going through the docs that there are some that are incorrectly documented. For my sanity I check the sources when I'm skeptical. Unfortunately we're the only ones who can. (Yes, the academic source release is floating around, but not legally where anyone can get at it.)

Throwing off of GetLastError when the API doesn't set it is another level of hell sometimes. That said, you usually end up with ERROR_SUCCESS as your error.

I don't think there is a P/Invoke resource we can use as an authoritative reference- I think we're building it. We can check some things via the headers and we can probably write analyzers to catch things that are technically correct that we don't like.

[JeremyKuhne]

On a related note, the last error behavior was just recently changed to clear the last error before making the call.

[danmoseley]

Looks like IsChild is mis-annotated in AArnott/pinvoke but correctly annotated in JeremyKuhne/WInterop - maybe I should have used that. 😄

Stepping back, this is just one way to annotate stuff wrong. It feels like we shouldn't all be solving this problem individually (and painstakingly in some cases)

We have been uplifting our interops for the past year, we verify signature correctness ...and verify correct decorations are applies.

I could imagine two analyzers

1. An analyzer checking the signature and annotations against a huge database of native API (akin to the repos above, or pinvoke.net)
2. The general P/Invoke analyzer proposed in https://github.com/dotnet/runtime/issues/34831. This could possibly flag if GLE is ignored, although as mentioned that may be intentional.

These seem separate problems to me because the second one is relatively straightforward to write and ship, but the first one needs a community around it to maintain the lists -- and may have more "policy" built in as well.

If there's prior art for these it's evidently not established enough that our repos have used them?

[RussKie]

/cc: @AArnott

[RussKie]

It would be great to have something like https://www.pinvoke.net/ that has correct signatures and that align to our interop guidelines. And we are the ones who is in the best position to do so.

[AArnott]

It would be great to have something like https://www.pinvoke.net/ that has correct signatures and that align to our interop guidelines.

I like to think that that is exactly the charter of https://github.com/aarnott/pinvoke. We have been meticulous to write signatures that are exactly correct, written to be alloc free, using blittable types so no marshaling is necessary, with test infrastructure, etc. I would hope you can contribute to it, and perhaps if you don't want to depend on it as an assembly reference you could share source in another way so that customers can continue to reference the pinvoke repo as the authoritative source.

I'll be happy to respond quickly to your group with any issues or PRs.

[AArnott]

And FWIW, I've applied for this repo to move into the .NET Foundation: https://github.com/dotnet-foundation/projects/issues/66

[AArnott]

Oh, and we write all our p/invoke signatures and structs to be safe for use in 32-bit and 64-bit processes, while still targeting AnyCPU. As for not having access to source, I guess I have it as a MSFT employee, but I've never had to look there. The header files are public and that's all we've needed so far. The docs may be wrong, or simply very inadequately written, so I refer to the .h files a lot when writing the p/invoke signatures.

[AaronRobinsonMSFT]

I would also like to second @AArnott's P/Invoke library. I have used it and recommend it often to people look for ready made P/Invoke signatures.

As far as pinvoke.net, the owner used to be a Microsoft employee but I am unsure if they are still here. Now that we have github I much prefer that.

[JeremyKuhne]

As far as pinvoke.net, the owner used to be a Microsoft employee but I am unsure if they are still here. Now that we have github I much prefer that.

A repo with tests is the best thing for confidence. https://www.pinvoke.net/ is a great idea and it has been helpful, but it is full of errors- some of which are not very obvious. While they could be fixed a wiki isn't nearly as robust as libraries in a gated public repo, particularly one that has governance like the .NET Foundation provides. I don't use the wiki and I don't contribute to it as I don't have the time to validate that my contributions don't get undone or the confidence that anyone else will keep it from degrading.

As to my own repo, my goals were somewhat different. My largest priority was getting work that I did out in the open so it could be a reference. I experimented quite heavily on its composition, trying to (at one point) handle all usages, including producing libraries that contained only "blessed" Windows Store APIs. I gave up on that and have been pushing on creating wraps that allow writing Win32 apps in a C# style that still feels Win32 like. I don't really have any known consumers, and I've leveraged that to throw different approaches against the wall to see what will stick.

I've done several hundred signature wraps and I'm totally fine with people using the code to help contribute to Andrew's library. I grant full usage without code attribution in this case, or for any .NET Foundation hosted repo. I haven't done it myself just because I don't have the need to consume his library. If his moves into the .NET Foundation I might allocate some of my free time to fill in some holes. I might even transition to using it for my base layer.