[lldb] add support for thread names on Windows #74731
Conversation
|
@llvm/pr-subscribers-lldb Author: oltolm (oltolm) ChangesThis PR adds support for thread names in lldb on Windows. Full diff: https://github.com/llvm/llvm-project/pull/74731.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.cpp
index 37dc8f6d6d14a..d9e0fbcdf7ffd 100644
--- a/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.cpp
+++ b/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.cpp
@@ -10,6 +10,7 @@
#include "lldb/Host/HostNativeThreadBase.h"
#include "lldb/Host/windows/HostThreadWindows.h"
#include "lldb/Host/windows/windows.h"
+#include "llvm/Support/ConvertUTF.h"
#include "lldb/Target/RegisterContext.h"
#include "lldb/Target/Unwind.h"
#include "lldb/Utility/LLDBLog.h"
@@ -33,6 +34,9 @@
using namespace lldb;
using namespace lldb_private;
+using GetThreadDescriptionFunctionPtr = HRESULT
+WINAPI (*)(HANDLE hThread, PWSTR *ppszThreadDescription);
+
TargetThreadWindows::TargetThreadWindows(ProcessWindows &process,
const HostThread &thread)
: Thread(process, thread.GetNativeThread().GetThreadId()),
@@ -175,3 +179,30 @@ Status TargetThreadWindows::DoResume() {
return Status();
}
+
+const char *TargetThreadWindows::GetName() {
+ Log *log = GetLog(LLDBLog::Thread);
+ HMODULE hModule = ::LoadLibraryW(L"Kernel32.dll");
+ if (hModule) {
+ auto GetThreadDescription =
+ reinterpret_cast<GetThreadDescriptionFunctionPtr>(
+ ::GetProcAddress(hModule, "GetThreadDescription"));
+ LLDB_LOGF(log, "GetProcAddress: %p",
+ reinterpret_cast<void *>(GetThreadDescription));
+ if (GetThreadDescription) {
+ PWSTR pszThreadName;
+ if (SUCCEEDED(GetThreadDescription(
+ m_host_thread.GetNativeThread().GetSystemHandle(),
+ &pszThreadName))) {
+ LLDB_LOGF(log, "GetThreadDescription: %ls", pszThreadName);
+ llvm::convertUTF16ToUTF8String(
+ llvm::ArrayRef(reinterpret_cast<char *>(pszThreadName),
+ wcslen(pszThreadName) * sizeof(wchar_t)),
+ m_name);
+ ::LocalFree(pszThreadName);
+ }
+ }
+ }
+
+ return m_name.c_str();
+}
diff --git a/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.h b/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.h
index 2845847738f60..07e1db464ad59 100644
--- a/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.h
+++ b/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.h
@@ -34,6 +34,7 @@ class TargetThreadWindows : public lldb_private::Thread {
lldb::RegisterContextSP
CreateRegisterContextForFrame(StackFrame *frame) override;
bool CalculateStopInfo() override;
+ const char *GetName() override;
Status DoResume();
@@ -42,6 +43,7 @@ class TargetThreadWindows : public lldb_private::Thread {
private:
lldb::RegisterContextSP m_thread_reg_ctx_sp;
HostThread m_host_thread;
+ std::string m_name;
};
} // namespace lldb_private
|
|
| HMODULE hModule = ::LoadLibraryW(L"Kernel32.dll"); | ||
| if (hModule) { | ||
| auto GetThreadDescription = | ||
| reinterpret_cast<GetThreadDescriptionFunctionPtr>( | ||
| ::GetProcAddress(hModule, "GetThreadDescription")); |
There was a problem hiding this comment.
This code is only compiled on windows, why do we need to get the library handle manually and get the function pointer. Can't we just #include <processthreadsapi.h> and then call the function directly?
There was a problem hiding this comment.
The function was added in Windows 10 1607, which was also the only build (excluding Server 2016) where it isn't available in the header:
There was a problem hiding this comment.
If there is compatibility that we are concerned about, I think that we should consider falling back to more ... esoteric solutions.
__try {
RaiseException(MS_VC_EXCEPTION, 0, sizeof(info) / sizeof(ULONG_PTR), (ULONG_PTR*)&info);
} __except (EXCEPTION_EXECUTE_HANDLER) {
} Should be far more portable and is what VS also uses. This is documented at https://learn.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2015/debugger/how-to-set-a-thread-name-in-native-code?view=vs-2015&redirectedfrom=MSDN.
I would be okay with also raising the requirements to a newer version of Windows as 1607 is RS1 which makes it more than 8 years old at this point.
There was a problem hiding this comment.
You are talking about setting thread names, but I implemented getting a thread name from Windows.
There was a problem hiding this comment.
There is the set name below though.
|
I have added a test. |
There was a problem hiding this comment.
One other way to test is to create a python test that makes a test windows binary that creates a thread using the Windows APIs and then we set a breakpoint in one of the thread functions and then verify the thread name is returned in the output of "thread list" and via the python lldb.SBThread.GetName() function. The unit test verifies things work on the native layer.
That being said, I am ok with these changes, but I don't do any windows debugging, so it might be best some someone with windows experience to give the final ok
|
@compnerd Perhaps you can take a look at this one? Not sure who does Windows work on LLDB these days. |
| HMODULE hModule = ::LoadLibraryW(L"Kernel32.dll"); | ||
| if (hModule) { | ||
| auto GetThreadDescription = | ||
| reinterpret_cast<GetThreadDescriptionFunctionPtr>( | ||
| ::GetProcAddress(hModule, "GetThreadDescription")); |
There was a problem hiding this comment.
If there is compatibility that we are concerned about, I think that we should consider falling back to more ... esoteric solutions.
__try {
RaiseException(MS_VC_EXCEPTION, 0, sizeof(info) / sizeof(ULONG_PTR), (ULONG_PTR*)&info);
} __except (EXCEPTION_EXECUTE_HANDLER) {
} Should be far more portable and is what VS also uses. This is documented at https://learn.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2015/debugger/how-to-set-a-thread-name-in-native-code?view=vs-2015&redirectedfrom=MSDN.
I would be okay with also raising the requirements to a newer version of Windows as 1607 is RS1 which makes it more than 8 years old at this point.
| ::GetProcAddress(hModule, "GetThreadDescription")); | ||
| LLDB_LOGF(log, "GetProcAddress: %p", | ||
| reinterpret_cast<void *>(GetThreadDescription)); | ||
| if (GetThreadDescription) { |
There was a problem hiding this comment.
An early out here would be nice to reduce indentation.
| Log *log = GetLog(LLDBLog::Thread); | ||
| HMODULE hModule = ::LoadLibraryW(L"Kernel32.dll"); | ||
| if (hModule) { | ||
| auto GetThreadDescription = |
There was a problem hiding this comment.
This should be pulled out into a lambda and store the lookup rather than re-evaluating it each time.
|
What happens next with my PR? |
DavidSpickett
changed the title
|
If you have permissions, once a PR is approved you can click "squash and merge" to land it. I've gone ahead and done that for you in this time. |
|
You'll have email from at least one Linux builder. I believe I've fixed that link error with: 513c215 |
|
Thank you David, I didn't see a button for merging the PR. |
|
Everything passed on Windows (https://lab.llvm.org/buildbot/#/builders/219/builds/7700) and the rest of the lldb specific bots are green. You may get a few more messages as other builders get to it (the Mac ones mainly), if it's a link error with If you do miss a failure for a while, someone like me will deal with it or ping you about it here. Thanks for the contribution! |
This PR adds support for thread names in lldb on Windows.