Skip to content
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

[compiler-rt][Fuzzer] SetThreadName windows implementation new try. #76761

Merged

Conversation

devnexen
Copy link
Member

@devnexen devnexen commented Jan 2, 2024

SetThreadDescription symbol needs to be dynamically loaded before usage. Then using a wide string buffer, since we re using a null terminated string, we can use MultiByteToWideChar -1 as 4th argument to finally set the thread name.

Previously SetThreadDescription was called directly causing crash.
It was reverted in dd3aa26

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 2, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: David CARLIER (devnexen)

Changes

SetThreadDescription symbol needs to be dynamically loaded before usage. Then using a wide string buffer, since we re using a null terminated string, we can use MultiByteToWideChar -1 as 4th argument to finally set the thread name.


Full diff: https://github.com/llvm/llvm-project/pull/76761.diff

1 Files Affected:

  • (modified) compiler-rt/lib/fuzzer/FuzzerUtilWindows.cpp (+15-2)
diff --git a/compiler-rt/lib/fuzzer/FuzzerUtilWindows.cpp b/compiler-rt/lib/fuzzer/FuzzerUtilWindows.cpp
index 71770166805f78..e47494060a0ff5 100644
--- a/compiler-rt/lib/fuzzer/FuzzerUtilWindows.cpp
+++ b/compiler-rt/lib/fuzzer/FuzzerUtilWindows.cpp
@@ -18,8 +18,10 @@
 #include <errno.h>
 #include <io.h>
 #include <iomanip>
+#include <libloaderapi.h>
 #include <signal.h>
 #include <stdio.h>
+#include <stringapiset.h>
 #include <sys/types.h>
 #include <windows.h>
 
@@ -234,8 +236,19 @@ size_t PageSize() {
 }
 
 void SetThreadName(std::thread &thread, const std::string &name) {
-  // TODO ?
-  // to UTF-8 then SetThreadDescription ?
+  typedef HRESULT(WINAPI *proc)(HANDLE, PCWSTR);
+  HMODULE kbase = GetModuleHandleA("KernelBase.dll");
+  proc ThreadNameProc = reinterpret_cast<proc>(GetProcAddress, "SetThreadDescription");
+  if (proc) {
+    std::wstring buf;
+    auto sz = MultiByteToWideChar(CP_UTF8, 0, name.data(), -1, nullptr, 0);
+    if (sz > 0) {
+      buf.resize(sz);
+      if (MultyByteToWideChar(CP_UTF8, 0, name.data(), -1, &buf[0], sz) > 0) {
+        (void)ThreadNameProc(thread.native_handle(), buf.c_str());
+      }
+    }
+  }
 }
 
 } // namespace fuzzer

Copy link

github-actions bot commented Jan 2, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@vitalybuka
Copy link
Collaborator

Can you please include into description # of the prev attempt?

@devnexen devnexen force-pushed the windows_fuzzer_set_thread_name_newtry branch from d99a0f9 to 9db1173 Compare January 2, 2024 23:02
@vitalybuka
Copy link
Collaborator

Note, PR github description, not commit, will be used, if PR is pushed web UI

@devnexen devnexen force-pushed the windows_fuzzer_set_thread_name_newtry branch from 9db1173 to b7f17aa Compare January 2, 2024 23:07
SetThreadDescription symbol needs to be dynamically loaded before usage.
Then using a wide string buffer, since we re using a null terminated string,
we can use MultiByteToWideChar -1 as 4th argument to finally set the thread name.
@devnexen devnexen force-pushed the windows_fuzzer_set_thread_name_newtry branch from b7f17aa to 5e6e63d Compare January 2, 2024 23:09
@vitalybuka
Copy link
Collaborator

Can you please include into description the PR# pr commit of the previous attempt?

@vitalybuka
Copy link
Collaborator

Can you please include into description the PR# pr commit of the previous attempt?

I added revert patch into description. Just to help to see improvement vs prev version.

@vitalybuka vitalybuka merged commit 2cdf611 into llvm:main Mar 1, 2024
4 checks passed
@glandium
Copy link
Contributor

glandium commented Mar 1, 2024

This interestingly broke our compiler-rt builds downstream:

[task 2024-03-01T02:17:28.933Z] /builds/worker/fetches/clang/bin/clang-cl --target=x86_64-pc-windows-msvc  /nologo -TP  -I/builds/worker/fetches/llvm-project/compiler-rt/lib/fuzzer/../../include -D_CRT_SECURE_NO_WARNINGS --target=x86_64-windows-msvc -fms-compatibility-version=19.27 -vctoolsversion 14.29.30133 -winsdkversion 10.0.19041.0 -winsysroot /builds/worker/fetches/vs -Xclang -ivfsoverlay -Xclang /builds/worker/build/winsdk_vfs_overlay.yaml /DWIN32 /D_WINDOWS /EHsc /W4 -Wno-unused-parameter /O2 /Ob2 /DNDEBUG -MT -fno-builtin -fno-sanitize=safe-stack -fno-lto /Oy- /GS- /Zc:threadSafeInit- -Wthread-safety -Wthread-safety-reference -Wthread-safety-beta /Z7 -Wno-gnu -Wno-variadic-macros -Wno-c99-extensions /wd4146 /wd4291 /wd4391 /wd4722 /wd4800 -D_HAS_EXCEPTIONS=0 -std:c++17 /showIncludes /Folib/fuzzer/CMakeFiles/RTfuzzer.x86_64.dir/FuzzerUtilWindows.cpp.obj /Fdlib/fuzzer/CMakeFiles/RTfuzzer.x86_64.dir/ -c -- /builds/worker/fetches/llvm-project/compiler-rt/lib/fuzzer/FuzzerUtilWindows.cpp
[task 2024-03-01T02:17:28.933Z] In file included from /builds/worker/fetches/llvm-project/compiler-rt/lib/fuzzer/FuzzerUtilWindows.cpp:21:
[task 2024-03-01T02:17:28.933Z] In file included from /builds/worker/fetches/vs/Windows Kits/10/Include/10.0.19041.0/um/libloaderapi.h:18:
[task 2024-03-01T02:17:28.933Z] In file included from /builds/worker/fetches/vs/Windows Kits/10/Include/10.0.19041.0/shared/minwindef.h:182:
[task 2024-03-01T02:17:28.933Z] /builds/worker/fetches/vs/Windows Kits/10/Include/10.0.19041.0/um/winnt.h(173,2): error: "No Target Architecture"
[task 2024-03-01T02:17:28.933Z]   173 | #error "No Target Architecture"
[task 2024-03-01T02:17:28.933Z]       |  ^

Interestingly, the contents of winnt.h around that line are:

#if defined(_AMD64_) || defined(_X86_)
#define PROBE_ALIGNMENT( _s ) TYPE_ALIGNMENT( DWORD )
#elif defined(_IA64_) || defined(_ARM_) || defined(_ARM64_)

//
// TODO: WOWXX - Unblock ARM. Make all alignment checks DWORD for now.
//

#define PROBE_ALIGNMENT( _s ) TYPE_ALIGNMENT( DWORD )
#elif !defined(RC_INVOKED)
#error "No Target Architecture"
#endif

which suggests _AMD64_ is not set, which only happens if atlconv.h is included, which I guess is not happening.

@devnexen
Copy link
Member Author

devnexen commented Mar 1, 2024

Then changing the order of the includes should solve it, windows.h before the new added entries ?

@glandium
Copy link
Contributor

glandium commented Mar 1, 2024

After the include order change, it fails with:

[task 2024-03-01T09:50:41.939Z] /builds/worker/fetches/clang/bin/clang-cl --target=x86_64-pc-windows-msvc  /nologo -TP  -I/builds/worker/fetches/llvm-project/compiler-rt/lib/fuzzer/../../include -D_CRT_SECURE_NO_WARNINGS --target=x86_64-windows-msvc -fms-compatibility-version=19.27 -vctoolsversion 14.29.30133 -winsdkversion 10.0.19041.0 -winsysroot /builds/worker/fetches/vs -Xclang -ivfsoverlay -Xclang /builds/worker/build/winsdk_vfs_overlay.yaml /DWIN32 /D_WINDOWS /EHsc /W4 -Wno-unused-parameter /O2 /Ob2 /DNDEBUG -MT -fno-builtin -fno-sanitize=safe-stack -fno-lto /Oy- /GS- /Zc:threadSafeInit- -Wthread-safety -Wthread-safety-reference -Wthread-safety-beta /Z7 -Wno-gnu -Wno-variadic-macros -Wno-c99-extensions /wd4146 /wd4291 /wd4391 /wd4722 /wd4800 -D_HAS_EXCEPTIONS=0 -std:c++17 /showIncludes /Folib/fuzzer/CMakeFiles/RTfuzzer.x86_64.dir/FuzzerUtilWindows.cpp.obj /Fdlib/fuzzer/CMakeFiles/RTfuzzer.x86_64.dir/ -c -- /builds/worker/fetches/llvm-project/compiler-rt/lib/fuzzer/FuzzerUtilWindows.cpp
[task 2024-03-01T09:50:41.939Z] /builds/worker/fetches/llvm-project/compiler-rt/lib/fuzzer/FuzzerUtilWindows.cpp(245,7): error: reinterpret_cast from 'const char *' to 'proc' (aka 'long (*)(void *, const wchar_t *)') casts away qualifiers
[task 2024-03-01T09:50:41.939Z]   245 |       reinterpret_cast<proc>(GetProcAddress, "SetThreadDescription");
[task 2024-03-01T09:50:41.939Z]       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2024-03-01T09:50:41.939Z] /builds/worker/fetches/llvm-project/compiler-rt/lib/fuzzer/FuzzerUtilWindows.cpp(246,11): error: expected unqualified-id
[task 2024-03-01T09:50:41.939Z]   246 |   if (proc) {
[task 2024-03-01T09:50:41.939Z]       |           ^
[task 2024-03-01T09:50:41.939Z] /builds/worker/fetches/llvm-project/compiler-rt/lib/fuzzer/FuzzerUtilWindows.cpp(251,11): error: use of undeclared identifier 'MultyByteToWideChar'; did you mean 'MultiByteToWideChar'?
[task 2024-03-01T09:50:41.939Z]   251 |       if (MultyByteToWideChar(CP_UTF8, 0, name.data(), -1, &buf[0], sz) > 0) {
[task 2024-03-01T09:50:41.939Z]       |           ^~~~~~~~~~~~~~~~~~~
[task 2024-03-01T09:50:41.939Z]       |           MultiByteToWideChar
[task 2024-03-01T09:50:41.939Z] /builds/worker/fetches/vs/Windows Kits/10/Include/10.0.19041.0/um/stringapiset.h(126,1): note: 'MultiByteToWideChar' declared here
[task 2024-03-01T09:50:41.939Z]   126 | MultiByteToWideChar(
[task 2024-03-01T09:50:41.939Z]       | ^
[task 2024-03-01T09:50:41.939Z] /builds/worker/fetches/llvm-project/compiler-rt/lib/fuzzer/FuzzerUtilWindows.cpp(245,30): warning: left operand of comma operator has no effect [-Wunused-value]
[task 2024-03-01T09:50:41.939Z]   245 |       reinterpret_cast<proc>(GetProcAddress, "SetThreadDescription");
[task 2024-03-01T09:50:41.939Z]       |                              ^~~~~~~~~~~~~~
[task 2024-03-01T09:50:41.940Z] 1 warning and 3 errors generated.

@devnexen
Copy link
Member Author

devnexen commented Mar 1, 2024

oh I see I ll revert and redo a PR, couple of typos here and there.

devnexen added a commit to devnexen/llvm-project that referenced this pull request Mar 1, 2024
devnexen added a commit to devnexen/llvm-project that referenced this pull request Mar 1, 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 this pull request may close these issues.

None yet

4 participants