Page MenuHomePhabricator
Differential D115204 Diff 392377 compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp

Changeset View

Standalone View

View Options

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp

  • This file was added.
//===-- 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_common.h"
#include "sanitizer_internal_defs.h"
#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>
mstorsjoUnsubmitted
Done
ReplyInline Actions

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: This broke building on MinGW. When building in MinGW configurations, it's built with `…
# include <algorithm>
# include "sanitizer_stoptheworld.h"
namespace __sanitizer {
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;
};
PtraceRegistersStatus SuspendedThreadsListWindows::GetRegistersAndSP(
uptr index, InternalMmapVector<uptr> *buffer, uptr *sp) const {
CHECK_LT(index, threadHandles.size());
CONTEXT thread_context;
thread_context.ContextFlags = CONTEXT_ALL;
CHECK(GetThreadContext(threadHandles[index], &thread_context));
buffer->resize(RoundUpTo(sizeof(thread_context), sizeof(uptr)) /
sizeof(uptr));
internal_memcpy(buffer->data(), &thread_context, sizeof(thread_context));
*sp = thread_context.Rsp;
mstorsjoUnsubmitted
Done
ReplyInline Actions

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

mstorsjo: This fails to build on i386 (where things build successfully before), as you'd have to use `.
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;
};
rnkUnsubmitted
Done
ReplyInline Actions

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.

rnk: I reviewed this code, it looks reasonable to me, but I don't have a lot of time to do a careful…
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 of this Process
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;
const bool contains_thread =
std::find(suspended_threads_list.threadIds.begin(),
mstorsjoUnsubmitted
Done
ReplyInline Actions

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

mstorsjo: This is the one thing that fails to build when leaving out `<algorithm>`.
vitalybukaUnsubmitted
Done
ReplyInline Actions

yep, we don't use STL in sanitizer runtimes

vitalybuka: yep, we don't use STL in sanitizer runtimes
suspended_threads_list.threadIds.end(),
thread_entry.th32ThreadID) !=
suspended_threads_list.threadIds.end();
if (contains_thread)
continue;
mstorsjoUnsubmitted
Done
ReplyInline Actions

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

mstorsjo: This continue will only affect the current for loop, which probably isn’t what you want
clemenswasserAuthorUnsubmitted
Done
ReplyInline Actions

Good catch, thanks!

clemenswasser: Good catch, thanks!
const HANDLE thread =
OpenThread(THREAD_ALL_ACCESS, FALSE, thread_entry.th32ThreadID);
CHECK(thread);
suspended_threads_list.threadIds.push_back(thread_entry.th32ThreadID);
suspended_threads_list.threadHandles.push_back(thread);
SuspendThread(thread);
vitalybukaUnsubmitted
Done
ReplyInline Actions

what if SuspendThread failed?

vitalybuka: what if SuspendThread failed?
new_thread_found = true;
} while (Thread32Next(threads, &thread_entry));
CloseHandle(threads);
} while (new_thread_found);
__try {
run_args->callback(suspended_threads_list, run_args->argument);
} __except (EXCEPTION_EXECUTE_HANDLER) {
vitalybukaUnsubmitted
Done
ReplyInline Actions

Why do we crash here?

vitalybuka: Why do we crash here?
clemenswasserAuthorUnsubmitted
Done
ReplyInline Actions

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

clemenswasser: What do you mean by that. I don't crash in the `__except` block? Did I miss something?
vitalybukaUnsubmitted
Done
ReplyInline Actions

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

vitalybuka: Why w have exceptions here? What do we need to catch with __try?
clemenswasserAuthorUnsubmitted
Done
ReplyInline Actions

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

clemenswasser: We catch for example the `STATUS_ACCESS_VIOLATION` hardware exception and [more](https://docs.
rnkUnsubmitted
Done
ReplyInline Actions

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.

rnk: My reading of the comments suggests that we don't need to catch exceptions to make that test…
DWORD code = GetExceptionCode();
Printf("Tracer caught exception %lu\n", code);
vitalybukaUnsubmitted
Done
ReplyInline Actions

VPrintf(1,

vitalybuka: VPrintf(1,
}
for (const auto suspended_thread_handle :
suspended_threads_list.threadHandles) {
ResumeThread(suspended_thread_handle);
vitalybukaUnsubmitted
Done
ReplyInline Actions

check return ResumeThread?

vitalybuka: check return ResumeThread?
CloseHandle(suspended_thread_handle);
}
return 0;
}
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
No newline at end of file