[lldb-dap] Treat empty thread names as unset

[Nerixyz]

When a target thread returned an empty but not nullptr string as its name, the thread would show up with an empty name in lldb-dap.

I don't know how this works on macOS and Linux, but on Windows, TargetThreadWindows::GetName returns a non-null pointer to an empty string, because on MSVC's STL, std::string{}.c_str() returns a pointer to inside the object (the SSO storage).

This changes the check in CreateThread, when no custom thread formatter is set, to check for the length of the thread and queue name instead of it being nullptr.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: nerix (Nerixyz)

Changes

When a target thread returned an empty but not nullptr string as its name, the thread would show up with an empty name in lldb-dap.

I don't know how this works on macOS and Linux, but on Windows, TargetThreadWindows::GetName returns a non-null pointer to an empty string, because on MSVC's STL, std::string{}.c_str() returns a pointer to inside the object (the SSO storage).

This changes the check in CreateThread, when no custom thread formatter is set, to check for the length of the thread and queue name instead of it being nullptr.

---

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

1 Files Affected:

• (modified) lldb/tools/lldb-dap/JSONUtils.cpp (+5-5)

diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index c9c6f4554c325..f0b3dfb595717 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -764,12 +764,12 @@ llvm::json::Value CreateThread(lldb::SBThread &thread, lldb::SBFormat &format) {
   if (format && thread.GetDescriptionWithFormat(format, stream).Success()) {
     thread_str = stream.GetData();
   } else {
-    const char *thread_name = thread.GetName();
-    const char *queue_name = thread.GetQueueName();
+    llvm::StringRef thread_name(thread.GetName());
+    llvm::StringRef queue_name(thread.GetQueueName());
 
-    if (thread_name) {
-      thread_str = std::string(thread_name);
-    } else if (queue_name) {
+    if (!thread_name.empty()) {
+      thread_str = thread_name.str();
+    } else if (!queue_name.empty()) {
       auto kind = thread.GetQueue().GetKind();
       std::string queue_kind_label = "";
       if (kind == lldb::eQueueKindSerial) {

[JDevlieghere]

The change looks reasonable. Can we test this (on Windows)?

[Nerixyz]

(on Windows)?

That's the hard part 😅. I tried to get https://github.com/llvm/llvm-project/blob/main/lldb/test/API/tools/lldb-dap/threads/TestDAP_threads.py to run on Windows, but it's too flaky. From my observation, the debug adapter is too fast at launching the program before the test can set the breakpoints.
If I understand DAP correctly, then the server can send breakpoint requests after the connection has been initialized, but that doesn't seem to be easily doable in the tests right now[?] as DAPTestCaseBase.launch already does an initialization request (and launch request). Ideally, one could set the breakpoints inbetween.

I don't know why, but for some reason, my VS Code sends the launch request before existing breakpoints with lldb-dap, but when using the MSVC debugger from the cpptools (vsdbg), the breakpoints are sent before the launch request (as documented in launch sequencing).

[JDevlieghere]

(on Windows)?

That's the hard part 😅. I tried to get https://github.com/llvm/llvm-project/blob/main/lldb/test/API/tools/lldb-dap/threads/TestDAP_threads.py to run on Windows, but it's too flaky. From my observation, the debug adapter is too fast at launching the program before the test can set the breakpoints. If I understand DAP correctly, then the server can send breakpoint requests after the connection has been initialized, but that doesn't seem to be easily doable in the tests right now[?] as DAPTestCaseBase.launch already does an initialization request (and launch request). Ideally, one could set the breakpoints inbetween.

Ack. One solution could be to hand-roll your launch sequence so you can send the requests in the order that reproduces the issue. The other alternative is to extend the helpers to make them support your use case. Without knowing what exactly you need I'm leaning towards the latter as that means everyone can benefit from it and if something changes in the launch sequence, we don't need to update multiple places.

I don't know why, but for some reason, my VS Code sends the launch request before existing breakpoints with lldb-dap, but when using the MSVC debugger from the cpptools (vsdbg), the breakpoints are sent before the launch request (as documented in launch sequencing).

The launch sequence is in parallel. Is it possible that the two debug adapter implements are handling them in different orders? It took me a while to realize this. I found this diagram enlightening: microsoft/vscode#4902 (comment)

[Nerixyz]

The launch sequence is in parallel. Is it possible that the two debug adapter implements are handling them in different orders? It took me a while to realize this. I found this diagram enlightening: microsoft/vscode#4902 (comment)

Ah, thank you! This cleared things up for me. I was a bit confused why I saw a third thread when debugging, but it turns out that the launch sequence is fine and it's Windows creating a thread pool in the background when calling WaitForSingleObject (according to this SO answer), which is called in thread.join().

I converted the tests to C++ now, as C's <threads.h> isn't supported everywhere. Furthermore, I only ignore the order of threads on Windows.

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.