Port __sanitizer::StopTheWorld to Windows

[clemenswasser]

Summary

This also makes the sanitizer_stoptheworld_test cross-platform by using the STL, rather than pthread.

[clemenswasser]

clang-format :(

[clemenswasser]

Cleanup of the StopTheWorld implementation.
Fix the following bug:

In D115204#3176168, @clemenswasser wrote:

@vitalybuka I can now run the tests on Windows. However the SuspendThreadsAdvanced test currently fails.
I think it fails because of an issue in my implementation of StopTheWorld.
The problem is that between the call to CreateToolhelp32Snapshot and suspending the Threads, there are still new threads being created, which are then not part of the snapshot.
Because of that, new threads can't being suspended.
Do you (or someone else) have any idea how I could fix this?

[clemenswasser]

Catch all SEH Exceptions like EXCEPTION_ACCESS_VIOLATION during execution of the callback in StopTheWorld and report them.
This makes the last failing test SegvInCallback pass on Windows 🎉

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp

119

what if SuspendThread failed?

128

Why do we crash here?

131

VPrintf(1,

136

check return ResumeThread?

[clemenswasser]

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp

128

What do you mean by that. I don't crash in the __except block? Did I miss something?

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp

128

Why w have exceptions here? What do we need to catch with __try?

[clemenswasser]

Add error handling

[clemenswasser]

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp

128

We catch for example the STATUS_ACCESS_VIOLATION hardware exception and more, which gets thrown in the SegvInCallback test.

[rnk]

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp

78

I reviewed this code, it looks reasonable to me, but I don't have a lot of time to do a careful review. In the future, we should identify a new Windows sanitizer code owner.

128

My reading of the comments suggests that we don't need to catch exceptions to make that test pass. On Linux, the suspender thread blocks most signals, which presumably interacts really badly with SIGSEGV: the suspender thread (or process? unclear) gets killed, but the user threads remain suspended. This ends up looking like a timeout, which is bad.

I think the test exists to ensure that crashes during StopTheWorld don't hang, not to show that the process recovers. Exiting the process with an error is fine. If that just happens naturally, I suggest disabling the SegvInCallback test on Windows.

[clemenswasser]

Disable SegvInCallback test on Windows

[vitalybuka]

rebase

[vitalybuka]

nits

[vitalybuka]

I didn't try on windows, can you please apply and test revision I've uploaded? If it's OK I'll land it later.

[vitalybuka]

Please update the summary, in particular remove:

So I couldn't test if this port actually works
Many other SanitizerUnitTests fail to compile (look in the attached log):

I assume you have resolve the issue in the summary?

[clemenswasser]

In D115204#3198568, @vitalybuka wrote:

Please update the summary, in particular remove:

So I couldn't test if this port actually works
Many other SanitizerUnitTests fail to compile (look in the attached log):

I assume you have resolve the issue in the summary?

Yes, everything works fine now.
Sorry for not updating it directly.

[vitalybuka]

Fix DISABLED_

[mstorsjo]

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp

23

This broke building on MinGW. When building in MinGW configurations, it's built with -nostdinc++ (to make sure that sanitizers don't end up depending on the C++ standard I presume?), which makes <algorithm> not be found.

Can you make the code not rely on that header?

[mstorsjo]

I'll go ahead and revert this one soon to restore the mingw build (and i386 msvc build too).

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp

60

This fails to build on i386 (where things build successfully before), as you'd have to use .Esp instead of .Rsp there.

106

This is the one thing that fails to build when leaving out <algorithm>.

[clemenswasser]

@mstorsjo sorry for the inconveniences.
I am quite busy right now. I will probably fix this either later today or tomorrow.

[mstorsjo]

In D115204#3200244, @clemenswasser wrote:

@mstorsjo sorry for the inconveniences.
I am quite busy right now. I will probably fix this either later today or tomorrow.

Ok, no problem. Reverting for now then.

[clemenswasser]

@mstorsjo I hope this fixes all of your Problems.
For me all sanitizer tests pass on X64 and I386.
Could someone test if the tests also pass on ARM and ARM64?

[vitalybuka]

@clemenswasser Please upload fixed version here and we will try again

[clemenswasser]

Fix typo

[clemenswasser]

In D115204#3200750, @vitalybuka wrote:

@clemenswasser Please upload fixed version here and we will try again

What do you mean by fixed version? Did I forget to fix something?

[vitalybuka]

In D115204#3200776, @clemenswasser wrote:

In D115204#3200750, @vitalybuka wrote:

@clemenswasser Please upload fixed version here and we will try again

What do you mean by fixed version? Did I forget to fix something?

Sorry, you didn't reopen review, so I didn't check if there is any new revision.

@mstorsjo Would you like to try this version?

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp

106

yep, we don't use STL in sanitizer runtimes

[mstorsjo]

In D115204#3200742, @clemenswasser wrote:

@mstorsjo I hope this fixes all of your Problems.

Yes, now building does succeed for both i386 and x86_64 mingw. Thanks!

For me all sanitizer tests pass on X64 and I386.
Could someone test if the tests also pass on ARM and ARM64?

The sanitizers aren't buildable for Windows on ARM in general (iirc there's a bunch of code in the common code that only is supported for x86, for things like intercepting/hotpatching stuff), so there's no need for this to work there I think, but the stack pointer register name seems correct anyway.

Building does produce this new warning though:

../lib/sanitizer_common/sanitizer_stoptheworld_win.cpp:56:52: warning: stack frame size (1304) exceeds limit (570) in '__sanitizer::(anonymous namespace)::SuspendedThreadsListWindows::GetRegistersAndSP(unsigned long long, __sanitizer::InternalMmapVector<unsigned long long>*, unsigned long long*) const' [-Wframe-larger-than]
PtraceRegistersStatus SuspendedThreadsListWindows::GetRegistersAndSP(
                                                   ^
1 warning generated.

See D91853 and D97726 for how such warnings were silenced elsewhere.

[clemenswasser]

@mstorsjo probably my only option is to silence the warning locally?

[vitalybuka]

In D115204#3200939, @clemenswasser wrote:

@mstorsjo probably my only option is to silence the warning locally?

In D115204#3200939, @clemenswasser wrote:

@mstorsjo probably my only option is to silence the warning locally?

you can resize InternalMmapVector to sizeof(CONTEXT) and GetThreadContext directly to the buffer

[clemenswasser]

Fix warning

[mstorsjo]

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp

113

This continue will only affect the current for loop, which probably isn’t what you want

[clemenswasser]

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp

113

Good catch, thanks!

[clemenswasser]

Fix continue in loop.
Thanks @mstorsjo!