Preserve last error in call-counting stubs

[jkotas]

Fixes #60819

[tannergooding]

Is this being considered for backport or servicing in .NET 6 so users can rely on Marshal.GetLastSystemError?

[jkotas]

Yes, I think we should backport it.

[jkotas]

/backport to release/6.0

[github-actions]

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1387213911

[kouvel]

Thanks!

[tannergooding]

LGTM. Is there anything additional we can or should do to help catch cases where BEGIN_PRESERVE_LAST_ERROR should be added to avoid accidentally breaking this, particularly in rare edge cases, in the future?

[jkotas]

We intentionally corrupt last error in checked builds of the runtime #60819 (comment) to increase chances of the problem being caught. It would take a lot longer to catch it without this instrumentation.

I am not sure what we can do better. These sort of bugs are a general problem with unpredictable runtime features like GC or tiered compilation.

It is also fairly common to corrupt the last error in managed code. I think the best fix for that would be to stop depending on thread-local storage for last error, and instead return it as a tuple with the actual result, similar to how it is done in e.g. golang. DllImport source generator may help with that - we would assume that the DllImport source generator can get it right and capture the last error before somebody gets a chance to corrupt it.

[danmoseley]

Why was seemingly only this test affected? There are many other places we test last error that I would expect to cause the wrong exception etc.

[jkotas]

The error corruption will only happen when we execute slow path of tiered compilation call counting stub. The call counting stub is installed some time after startup and the slow path of call counting stub is executed after it got executed certain number of times. This test was lucky enough to hit the slow path of the tiered compilation call counting stub, and be sensitive to last error corruption (the last error corruption won't cause real problem most of time).

[danmoseley]

Would there be value in running all our tests e.g. locally with the slow path forced?

[kouvel]

Yea it might be useful to lower the call count threshold for debug/checked builds, updated in #60945