This is an archive of the discontinued LLVM Phabricator instance.

Port __sanitizer::StopTheWorld to Windows
ClosedPublic

Authored by clemenswasser on Dec 6 2021, 4:27 PM.

Details

Summary

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

Diff Detail

Event Timeline

clemenswasser created this revision.Dec 6 2021, 4:27 PM
clemenswasser requested review of this revision.Dec 6 2021, 4:27 PM
vitalybuka added inline comments.Dec 6 2021, 6:56 PM
compiler-rt/lib/sanitizer_common/tests/sanitizer_stoptheworld_test.cpp
137

why do we need NO_THROW? Isn't the test fails if it throws anyway?

And I see the following on linux:

[ RUN      ] StopTheWorld.SuspendThreadsAdvanced
llvm-project/compiler-rt/lib/sanitizer_common/tests/sanitizer_stoptheworld_test.cpp:174: Failure
Expected: argument.threads[i].join() doesn't throw an exception.
  Actual: it throws.
terminate called without an active exception
vitalybuka edited the summary of this revision. (Show Details)Dec 6 2021, 7:24 PM
vitalybuka edited the summary of this revision. (Show Details)

Many other SanitizerUnitTests fail to compile (look in the attached log):

Something wrong with the compiler. Can you try different revisions and compare results HEAD, HEAD~10000 etc.?
If it's broken on HEAD then you can file a bug.

Remove ASSERT_NO_THROW from StopTheWorld tests

Hopefully fixes test StopTheWorld.SuspendThreadsAdvanced

Many other SanitizerUnitTests fail to compile (look in the attached log):

Something wrong with the compiler. Can you try different revisions and compare results HEAD, HEAD~10000 etc.?
If it's broken on HEAD then you can file a bug.

@vitalybuka the tests compile on HEAD^10000 = 83457d398df15a2688bab72a9b658779e51efbe2
Where should I file a bug report for this? Also the test objects compile forever, is this normal?

@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?

clang-format :(

Cleanup of the StopTheWorld implementation.
Fix the following bug:

@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?

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 ๐ŸŽ‰

clemenswasser marked an inline comment as done.Dec 7 2021, 7:44 AM
vitalybuka added inline comments.Dec 7 2021, 6:48 PM
compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp
120

what if SuspendThread failed?

129

Why do we crash here?

132

VPrintf(1,

137

check return ResumeThread?

clemenswasser added inline comments.Dec 7 2021, 11:57 PM
compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp
129

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

vitalybuka added inline comments.Dec 8 2021, 11:41 AM
compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp
129

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

Add error handling

clemenswasser added inline comments.Dec 10 2021, 12:46 AM
compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp
129

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

rnk added inline comments.Dec 13 2021, 10:47 AM
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.

129

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.

Disable SegvInCallback test on Windows

vitalybuka edited the summary of this revision. (Show Details)

rebase

Herald added a project: Restricted Project. ยท View Herald TranscriptDec 16 2021, 12:55 PM
Herald added a subscriber: Restricted Project. ยท View Herald Transcript
vitalybuka accepted this revision.Dec 16 2021, 1:03 PM

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.

This revision is now accepted and ready to land.Dec 16 2021, 1:03 PM

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 edited the summary of this revision. (Show Details)Dec 16 2021, 1:26 PM

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.

Fix DISABLED_

This revision was landed with ongoing or failed builds.Dec 16 2021, 11:29 PM
This revision was automatically updated to reflect the committed changes.
mstorsjo added inline comments.
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?

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>.

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

@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.

@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?

clemenswasser marked 10 inline comments as done.Dec 17 2021, 1:26 PM
vitalybuka reopened this revision.Dec 17 2021, 1:30 PM

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

This revision is now accepted and ready to land.Dec 17 2021, 1:30 PM
vitalybuka requested changes to this revision.Dec 17 2021, 1:30 PM
This revision now requires changes to proceed.Dec 17 2021, 1:30 PM

@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 accepted this revision.EditedDec 17 2021, 1:50 PM

@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

This revision is now accepted and ready to land.Dec 17 2021, 1:50 PM

@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.

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

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

@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 marked an inline comment as done.Dec 18 2021, 3:16 AM
mstorsjo added inline comments.Dec 18 2021, 3:28 AM
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 added inline comments.Dec 18 2021, 3:35 AM
compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp
113

Good catch, thanks!

Fix continue in loop.
Thanks @mstorsjo!

clemenswasser marked an inline comment as done.Dec 18 2021, 3:38 AM
This revision was landed with ongoing or failed builds.Dec 20 2021, 6:29 PM
This revision was automatically updated to reflect the committed changes.