This also makes the sanitizer_stoptheworld_test cross-platform by using the STL, rather than pthread.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
compiler-rt/lib/sanitizer_common/tests/sanitizer_stoptheworld_test.cpp | ||
---|---|---|
159 | 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 |
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?
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 ๐
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? |
compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp | ||
---|---|---|
129 | Why w have exceptions here? What do we need to catch with __try? |
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. |
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.
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?
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 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?
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.
you can resize InternalMmapVector to sizeof(CONTEXT) and GetThreadContext directly to the buffer
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 |
compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp | ||
---|---|---|
113 | Good catch, thanks! |
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?