Skip to content

libFuzzer does not build on mingw with pthreads #106871

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Zentrik opened this issue Aug 31, 2024 · 4 comments · Fixed by #109525
Closed

libFuzzer does not build on mingw with pthreads #106871

Zentrik opened this issue Aug 31, 2024 · 4 comments · Fixed by #109525

Comments

@Zentrik
Copy link
Contributor

Zentrik commented Aug 31, 2024

This code,

void SetThreadName(std::thread &thread, const std::string &name) {
typedef HRESULT(WINAPI * proc)(HANDLE, PCWSTR);
HMODULE kbase = GetModuleHandleA("KernelBase.dll");
proc ThreadNameProc =
reinterpret_cast<proc>(GetProcAddress(kbase, "SetThreadDescription"));
if (ThreadNameProc) {
std::wstring buf;
auto sz = MultiByteToWideChar(CP_UTF8, 0, name.data(), -1, nullptr, 0);
if (sz > 0) {
buf.resize(sz);
if (MultiByteToWideChar(CP_UTF8, 0, name.data(), -1, &buf[0], sz) > 0) {
(void)ThreadNameProc(thread.native_handle(), buf.c_str());
}
}
}
}
introduced in 8bf8d36 throws the following error

ninja: job failed: /opt/bin/x86_64-w64-mingw32-libgfortran5-cxx11/x86_64-w64-mingw32-clang++ --target=x86_64-w64-mingw32 --sysroot=/opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/ -D_FILE_OFFSET_BITS=64 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/workspace/build/projects/compiler-rt/lib/fuzzer -I/workspace/srcdir/llvm-project/compiler-rt/lib/fuzzer -I/workspace/build/include -I/workspace/srcdir/llvm-project/llvm/include -I/workspace/srcdir/llvm-project/compiler-rt/lib/fuzzer/../../include -remap -D__USING_SJLJ_EXCEPTIONS__ -D__CRT__NO_INLINE -pthread -DMLIR_CAPI_ENABLE_WINDOWS_DLL_DECLSPEC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -ffunction-sections -fdata-sections -Wall -O3 -DNDEBUG -m64 -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fno-sanitize=safe-stack -fvisibility=hidden -fno-lto -O3 -gline-tables-only -fms-extensions -fno-omit-frame-pointer -std=c++17 -MD -MT projects/compiler-rt/lib/fuzzer/CMakeFiles/RTfuzzer.x86_64.dir/FuzzerUtilWindows.cpp.obj -MF projects/compiler-rt/lib/fuzzer/CMakeFiles/RTfuzzer.x86_64.dir/FuzzerUtilWindows.cpp.obj.d -o projects/compiler-rt/lib/fuzzer/CMakeFiles/RTfuzzer.x86_64.dir/FuzzerUtilWindows.cpp.obj -c /workspace/srcdir/llvm-project/compiler-rt/lib/fuzzer/FuzzerUtilWindows.cpp
clang-16: warning: argument unused during compilation: '-remap' [-Wunused-command-line-argument]
/workspace/srcdir/llvm-project/compiler-rt/lib/fuzzer/FuzzerUtilWindows.cpp:252:30: error: cannot initialize a parameter of type 'HANDLE' (aka 'void *') with an rvalue of type 'native_handle_type' (aka 'unsigned long long')
        (void)ThreadNameProc(thread.native_handle(), buf.c_str());
                             ^~~~~~~~~~~~~~~~~~~~~~
1 error generated.

Using the pthreads implementation used for linux fixes this issue, i.e.

void SetThreadName(std::thread &thread, const std::string &name) {
  (void)pthread_setname_np(thread.native_handle(), name.c_str());
}

I'm not sure what exactly we should test to know which version of the function to use. The commit a8d15a9 has a message discussing some different ways to detect pthreads but doesn't seem to have a solution.

@Zentrik
Copy link
Contributor Author

Zentrik commented Sep 1, 2024

Maybe something like

# Checking threading requirements. Note that compiling to WebAssembly threads
# with either the Emscripten or wasi-threads flavor ends up using the pthreads
# interface in a WebAssembly-compiled libc; CMake does not yet know how to
# detect this.
if (NOT WASM)
find_package(Threads REQUIRED)
if(WIN32)
if(NOT CMAKE_USE_WIN32_THREADS_INIT)
libomp_error_say("Need Win32 thread interface on Windows.")
endif()
else()
if(NOT CMAKE_USE_PTHREADS_INIT)
libomp_error_say("Need pthread interface on Unix-like systems.")
endif()
endif()
endif()
can be used to detect if we're using pthreads.

@Zentrik
Copy link
Contributor Author

Zentrik commented Sep 1, 2024

Closed by #106902.

@mstorsjo
Copy link
Member

mstorsjo commented Sep 2, 2024

I reverted the fix, since that just traded one set of mingw builds being broken, for another part of the being broken - that's no good fix. Previously all builds on top of libc++ worked, now none of them work, while fixing it for libstdc++ instead. At https://github.com/mstorsjo/llvm-mingw/actions/workflows/build.yml I have nightly builds, which broke due to the attempted fix: https://github.com/mstorsjo/llvm-mingw/actions/runs/10658889158/job/29540641689

Indeed, if we're using std::thread::native_handle(), we need to know what kind of handle that is.

If the C++ standard library is MS STL, then it's a win32 thread handle.

If it's libc++, it is most probably always a win32 thread handle. (Since a8d15a9, libc++ with winpthreads is extremely rare.) If we want to introspect this, we can check if _LIBCPP_HAS_THREAD_API_PTHREAD is defined, then libc++ is using pthreads, otherwise not. (We can also check _LIBCPP_HAS_THREAD_API_WIN32.)

With libstdc++, it doesn't seem to be quite as easy. With older versions of libstdc++, you only had working std::thread if using pthreads, but in the latest versions of GCC, you can have working std::thread based on either native win32 threads, or pthreads, just like libc++. It seems like a header guard like _GLIBCXX_GCC_GTHR_POSIX_H or GCC_GTHR_WIN32_H can be used to disambiguate between these, but that's not a very clean thing to check for.

@mstorsjo
Copy link
Member

mstorsjo commented Sep 5, 2024

I'm not quite familiar with the context here, but if the thread name is more of a nice-to-have rather than a must-have, would the simplest and safest fix just be to ifdef out the whole SetThreadName implementation if we're on a mingw target, where we're unsure about the type of std::thread::native_handle(). It's not ideal, but would be a safer and more straightforward fix.

Alternative, if we'd make sure to set the thread description from within each thread itself (which requires refactoring the overall structure of the fuzzer library, across all OSes), we wouldn't need to query std::thread_native_handle() for the handle at all, we could just call GetCurrentThread().

Zentrik added a commit to Zentrik/llvm-project that referenced this issue Sep 21, 2024
Zentrik added a commit to Zentrik/llvm-project that referenced this issue Sep 23, 2024
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Oct 1, 2024
giordano pushed a commit to JuliaLang/llvm-project that referenced this issue Oct 8, 2024
giordano pushed a commit to JuliaLang/llvm-project that referenced this issue Oct 9, 2024
tru pushed a commit to llvmbot/llvm-project that referenced this issue Oct 11, 2024
tru pushed a commit to llvmbot/llvm-project that referenced this issue Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants