Port __sanitizer::StopTheWorld to Windows

[clemenswasser]

Summary

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

[vitalybuka]

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]

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.

[clemenswasser]

Remove ASSERT_NO_THROW from StopTheWorld tests

[clemenswasser]

Hopefully fixes test StopTheWorld.SuspendThreadsAdvanced

[clemenswasser]

In D115204#3175333, @vitalybuka wrote:

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?

[clemenswasser]

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

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

120

what if SuspendThread failed?

129

Why do we crash here?

132

VPrintf(1,

137

check return ResumeThread?

[clemenswasser]

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]

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp

129

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

129

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.

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.

[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!

compiler-rt/lib/sanitizer_common/CMakeLists.txt

set(SANITIZER_SOURCES_NOTERMINATION
   sanitizer_procmaps_bsd.cpp
   sanitizer_procmaps_fuchsia.cpp
   sanitizer_procmaps_linux.cpp
   sanitizer_procmaps_mac.cpp
   sanitizer_procmaps_solaris.cpp
   sanitizer_solaris.cpp
   sanitizer_stoptheworld_fuchsia.cpp
   sanitizer_stoptheworld_mac.cpp
+  sanitizer_stoptheworld_win.cpp
   sanitizer_suppressions.cpp
   sanitizer_tls_get_addr.cpp
   sanitizer_thread_registry.cpp
   sanitizer_type_traits.cpp
   sanitizer_win.cpp
   )
 
 set(SANITIZER_SOURCES

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp

+//===-- sanitizer_stoptheworld_win.cpp ------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// See sanitizer_stoptheworld.h for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "sanitizer_platform.h"
+
+#if SANITIZER_WINDOWS
+
+#  define WIN32_LEAN_AND_MEAN
+#  include <windows.h>
+// windows.h needs to be included before tlhelp32.h
+#  include <tlhelp32.h>
+
+#  include "sanitizer_stoptheworld.h"
+
+namespace __sanitizer {
+
+namespace {
+
+struct SuspendedThreadsListWindows final : public SuspendedThreadsList {
+  InternalMmapVector<HANDLE> threadHandles;
+  InternalMmapVector<DWORD> threadIds;
+
+  SuspendedThreadsListWindows() {
+    threadIds.reserve(1024);
+    threadHandles.reserve(1024);
+  }
+
+  PtraceRegistersStatus GetRegistersAndSP(uptr index,
+                                          InternalMmapVector<uptr> *buffer,
+                                          uptr *sp) const override;
+
+  tid_t GetThreadID(uptr index) const override;
+  uptr ThreadCount() const override;
+};
+
+// Stack Pointer register names on different architectures
+#  if SANITIZER_X64
+#    define SP_REG Rsp
+#  elif SANITIZER_I386
+#    define SP_REG Esp
+#  elif SANITIZER_ARM | SANITIZER_ARM64
+#    define SP_REG Sp
+#  else
+#    error Architecture not supported!
+#  endif
+
+PtraceRegistersStatus SuspendedThreadsListWindows::GetRegistersAndSP(
+    uptr index, InternalMmapVector<uptr> *buffer, uptr *sp) const {
+  CHECK_LT(index, threadHandles.size());
+
+  buffer->resize(RoundUpTo(sizeof(CONTEXT), sizeof(uptr)) / sizeof(uptr));
+  CONTEXT *thread_context = reinterpret_cast<CONTEXT *>(buffer->data());
+  thread_context->ContextFlags = CONTEXT_ALL;
+  CHECK(GetThreadContext(threadHandles[index], thread_context));
+  *sp = thread_context->SP_REG;
+
+  return REGISTERS_AVAILABLE;
+}
+
+tid_t SuspendedThreadsListWindows::GetThreadID(uptr index) const {
+  CHECK_LT(index, threadIds.size());
+  return threadIds[index];
+}
+
+uptr SuspendedThreadsListWindows::ThreadCount() const {
+  return threadIds.size();
+}
+
+struct RunThreadArgs {
+  StopTheWorldCallback callback;
+  void *argument;
+};
+
+DWORD WINAPI RunThread(void *argument) {
+  RunThreadArgs *run_args = (RunThreadArgs *)argument;
+
+  const DWORD this_thread = GetCurrentThreadId();
+  const DWORD this_process = GetCurrentProcessId();
+
+  SuspendedThreadsListWindows suspended_threads_list;
+  bool new_thread_found;
+
+  do {
+    // Take a snapshot of all Threads
+    const HANDLE threads = CreateToolhelp32Snapshot(TH32CS_SNAPTHREAD, 0);
+    CHECK(threads != INVALID_HANDLE_VALUE);
+
+    THREADENTRY32 thread_entry;
+    thread_entry.dwSize = sizeof(thread_entry);
+    new_thread_found = false;
+
+    if (!Thread32First(threads, &thread_entry))
+      break;
+
+    do {
+      if (thread_entry.th32ThreadID == this_thread ||
+          thread_entry.th32OwnerProcessID != this_process)
+        continue;
+
+      bool suspended_thread = false;
+      for (const auto thread_id : suspended_threads_list.threadIds) {
+        if (thread_id == thread_entry.th32ThreadID) {
+          suspended_thread = true;
+          break;
+        }
+      }
+
+      // Skip the Thread if it was already suspended
+      if (suspended_thread)
+        continue;
+
+      const HANDLE thread =
+          OpenThread(THREAD_ALL_ACCESS, FALSE, thread_entry.th32ThreadID);
+      CHECK(thread);
+
+      if (SuspendThread(thread) == -1) {
+        DWORD last_error = GetLastError();
+
+        VPrintf(1, "Could not suspend thread %lu (error %lu)",
+                thread_entry.th32ThreadID, last_error);
+        continue;
+      }
+
+      suspended_threads_list.threadIds.push_back(thread_entry.th32ThreadID);
+      suspended_threads_list.threadHandles.push_back(thread);
+      new_thread_found = true;
+    } while (Thread32Next(threads, &thread_entry));
+
+    CloseHandle(threads);
+
+    // Between the call to `CreateToolhelp32Snapshot` and suspending the
+    // relevant Threads, new Threads could have potentially been created. So
+    // continue to find and suspend new Threads until we don't find any.
+  } while (new_thread_found);
+
+  // Now all Threads of this Process except of this Thread should be suspended.
+  // Execute the callback function.
+  run_args->callback(suspended_threads_list, run_args->argument);
+
+  // Resume all Threads
+  for (const auto suspended_thread_handle :
+       suspended_threads_list.threadHandles) {
+    CHECK_NE(ResumeThread(suspended_thread_handle), -1);
+    CloseHandle(suspended_thread_handle);
+  }
+
+  return 0;
+}
+
+}  // namespace
+
+void StopTheWorld(StopTheWorldCallback callback, void *argument) {
+  struct RunThreadArgs arg = {callback, argument};
+  DWORD trace_thread_id;
+
+  auto trace_thread =
+      CreateThread(nullptr, 0, RunThread, &arg, 0, &trace_thread_id);
+  CHECK(trace_thread);
+
+  WaitForSingleObject(trace_thread, INFINITE);
+  CloseHandle(trace_thread);
+}
+
+}  // namespace __sanitizer
+
+#endif  // SANITIZER_WINDOWS

compiler-rt/lib/sanitizer_common/tests/sanitizer_stoptheworld_test.cpp

 //===-- sanitizer_stoptheworld_test.cpp -----------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // Tests for sanitizer_stoptheworld.h
 //
 //===----------------------------------------------------------------------===//
 
 #include "sanitizer_common/sanitizer_stoptheworld.h"
 
 #include "sanitizer_common/sanitizer_platform.h"
-#if SANITIZER_LINUX && defined(__x86_64__)
+#if (SANITIZER_LINUX || SANITIZER_WINDOWS) && defined(__x86_64__)
 
 #  include <atomic>
 #  include <mutex>
 #  include <thread>
 
 #  include "gtest/gtest.h"
 #  include "sanitizer_common/sanitizer_common.h"
 #  include "sanitizer_common/sanitizer_libc.h"
void AdvancedCallback(const SuspendedThreadsList &suspended_threads_list,
   }
   callback_argument->threads_stopped = true;
 }
 
 TEST(StopTheWorld, SuspendThreadsAdvanced) {
   AdvancedCallbackArgument argument;
 
   {
     std::lock_guard<std::mutex> lock(mutex);
     argument.threads[0] =
         std::thread(AdvancedIncrementerThread, std::ref(argument));
     // Wait for several threads to spawn before proceeding.
     while (argument.thread_index < kStopWorldAfter) std::this_thread::yield();
     StopTheWorld(&AdvancedCallback, &argument);
     EXPECT_TRUE(argument.callback_executed);
     EXPECT_TRUE(argument.threads_stopped);
 
     // Wait for all threads to spawn before we start terminating them.
     while (argument.thread_index < kThreadCount) std::this_thread::yield();
   }
   // Signal the threads to terminate.
   for (auto &t : argument.threads) t.join();
 }
 
 static void SegvCallback(const SuspendedThreadsList &suspended_threads_list,
                          void *argument) {
   *(volatile int *)0x1234 = 0;
 }
 
-TEST(StopTheWorld, SegvInCallback) {
+#  if SANITIZER_WINDOWS
+#    define MAYBE_SegvInCallback DISABLED_SegvInCallback
+#  else
+#    define MAYBE_SegvInCallback SegvInCallback
+#  endif
+
+TEST(StopTheWorld, MAYBE_SegvInCallback) {
   // Test that tracer thread catches SIGSEGV.
   StopTheWorld(&SegvCallback, NULL);
 }
 
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_LINUX && defined(__x86_64__)