Skip to content

Commit 7a41761

Browse files
[lldb] Make SBProcess thread related actions listen to StopLocker (#134339)
# Summary This PR updates `SBProcess::GetNumThreads()` and `SBProcess::GetThreadAtIndex()` to listen to the stop locker. `SBProcess::GetNumThreads()` will return 0 if the process is running. ## Problem Description Recently upon debugging a program with thousands of threads in VS Code, lldb-dap would hang at a `threads` request sent right after receiving the `configurationDone` response. Soon after it will end the debug session with the following error ``` Process <pid> exited with status = -1 (0xffffffff) lost connection ``` This is because LLDB is still in the middle of resuming all the threads. And requesting threads will end up interrupt the process on Linux. From the gdb-remote log it ended up getting `lldb::StateType::eStateInvalid` and just exit with status -1. I don't think it's reasonable to allow getting threads from a running process. There are a few approaches to fix this: 1) Send the stopped event to IDE after `configurationDone`. This aligns with the CLI behavior. 2) However, the above approach will break the existing user facing behavior. The alternative will be reject the `threads` request if the process is not stopped. 3) Improve the run lock. This is a synchronize issue where process was in the middle of resuming while lldb-dap attempts to interrupt it. **This PR implements the option 3** ## HOWEVER This fixed the "lost connection" issue below but new issue has surfaced. From testing, and also from checking the [VSCode source code](https://github.com/microsoft/vscode/blob/174af221c9ea2ccdb64abe4aab8e1a805e77beae/src/vs/workbench/contrib/debug/browser/debugSession.ts#L791), it expects having threadID to perform `pause`. So after attaching, without any threads reported to the client, the user will not be able to pause the attached process. `setBreakpoint` will still work and once we make a stop at the bp (or any stop that will report threads, client can perform pause again. ## NEXT 1) Made an attempt to return initial thread list so that VSCode can pause (second commit in the PR) 2) Investigate why threads will trigger unwinding the second frame of a thread, which leads to sending the interrupt 3) Decided if we want to support `stopOnEntry` for attaching, given i. This is not an official specification ii. If enable stopOnEntry, we need to fix attaching on Linux, to send only one stopped event. Currently, all threads upon attaching will have stop reason `SIGSTOP` and lldb-dap will send `stopped` event for each one of them. Every `stopped` will trigger the client request for threads. iii. Alternatively, we can support auto continue correspond to `(lldb) process attach --continue`. This require the ii above. ### Additionally lldb-dap will not send a `continued` event after `configurationDone` because it checks `dap.focus_tid == LLDB_INVALID_THREAD_ID` (so that we don't send it for `launch` request). Notice `dap.focus_tid` will only get assigned when handling stop or stepping. According to DAP > Please note: a debug adapter is not expected to send this event in response to a request that implies that execution continues, e.g. launch or continue. It is only necessary to send a continued event if there was no previous request that implied this. So I guess we are not violating DAP if we don't send `continued` event. But I'd like to get some sense about this. ## Test Plan Used following program for testing: https://gist.github.com/kusmour/1729d2e07b7b1063897db77de194e47d **NOTE: Utilize stdin to get pid and attach AFTER hitting enter. Attach should happen when all the threads start running.** DAP messages before the change <img width="1165" alt="image" src="https://github.com/user-attachments/assets/a9ad85fb-81ce-419c-95e5-612639905c66" /> DAP message after the change - report zero threads after attaching <img width="1165" alt="image" src="https://github.com/user-attachments/assets/a1179e18-6844-437a-938c-0383702294cd" /> --------- Co-authored-by: Jonas Devlieghere <[email protected]>
1 parent 68383fc commit 7a41761

File tree

8 files changed

+55
-16
lines changed

8 files changed

+55
-16
lines changed

lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -649,6 +649,10 @@ def request_configurationDone(self):
649649
response = self.send_recv(command_dict)
650650
if response:
651651
self.configuration_done_sent = True
652+
# Client requests the baseline of currently existing threads after
653+
# a successful launch or attach.
654+
# Kick off the threads request that follows
655+
self.request_threads()
652656
return response
653657

654658
def _process_stopped(self):

lldb/source/API/SBProcess.cpp

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -193,10 +193,11 @@ uint32_t SBProcess::GetNumThreads() {
193193
if (process_sp) {
194194
Process::StopLocker stop_locker;
195195

196-
const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
197-
std::lock_guard<std::recursive_mutex> guard(
198-
process_sp->GetTarget().GetAPIMutex());
199-
num_threads = process_sp->GetThreadList().GetSize(can_update);
196+
if (stop_locker.TryLock(&process_sp->GetRunLock())) {
197+
std::lock_guard<std::recursive_mutex> guard(
198+
process_sp->GetTarget().GetAPIMutex());
199+
num_threads = process_sp->GetThreadList().GetSize();
200+
}
200201
}
201202

202203
return num_threads;
@@ -393,11 +394,12 @@ SBThread SBProcess::GetThreadAtIndex(size_t index) {
393394
ProcessSP process_sp(GetSP());
394395
if (process_sp) {
395396
Process::StopLocker stop_locker;
396-
const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
397-
std::lock_guard<std::recursive_mutex> guard(
398-
process_sp->GetTarget().GetAPIMutex());
399-
thread_sp = process_sp->GetThreadList().GetThreadAtIndex(index, can_update);
400-
sb_thread.SetThread(thread_sp);
397+
if (stop_locker.TryLock(&process_sp->GetRunLock())) {
398+
std::lock_guard<std::recursive_mutex> guard(
399+
process_sp->GetTarget().GetAPIMutex());
400+
thread_sp = process_sp->GetThreadList().GetThreadAtIndex(index, false);
401+
sb_thread.SetThread(thread_sp);
402+
}
401403
}
402404

403405
return sb_thread;

lldb/test/API/tools/lldb-dap/attach/TestDAP_attach.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
"""
2-
Test lldb-dap setBreakpoints request
2+
Test lldb-dap attach request
33
"""
44

55

lldb/tools/lldb-dap/DAP.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,9 @@ struct DAP {
211211
/// The set of features supported by the connected client.
212212
llvm::DenseSet<ClientFeature> clientFeatures;
213213

214+
/// The initial thread list upon attaching.
215+
std::optional<llvm::json::Array> initial_thread_list;
216+
214217
/// Creates a new DAP sessions.
215218
///
216219
/// \param[in] log

lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ namespace lldb_dap {
4444
// just an acknowledgement, so no body field is required."
4545
// }]
4646
// },
47+
4748
void ConfigurationDoneRequestHandler::operator()(
4849
const llvm::json::Object &request) const {
4950
llvm::json::Object response;
@@ -52,8 +53,15 @@ void ConfigurationDoneRequestHandler::operator()(
5253
dap.configuration_done_sent = true;
5354
if (dap.stop_at_entry)
5455
SendThreadStoppedEvent(dap);
55-
else
56+
else {
57+
// Client requests the baseline of currently existing threads after
58+
// a successful launch or attach by sending a 'threads' request
59+
// right after receiving the configurationDone response.
60+
// Obtain the list of threads before we resume the process
61+
dap.initial_thread_list =
62+
GetThreads(dap.target.GetProcess(), dap.thread_format);
5663
dap.target.GetProcess().Continue();
64+
}
5765
}
5866

5967
} // namespace lldb_dap

lldb/tools/lldb-dap/Handler/ThreadsRequestHandler.cpp

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,16 +50,23 @@ namespace lldb_dap {
5050
// }
5151
void ThreadsRequestHandler::operator()(
5252
const llvm::json::Object &request) const {
53-
lldb::SBProcess process = dap.target.GetProcess();
5453
llvm::json::Object response;
5554
FillResponse(request, response);
5655

57-
const uint32_t num_threads = process.GetNumThreads();
5856
llvm::json::Array threads;
59-
for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
60-
lldb::SBThread thread = process.GetThreadAtIndex(thread_idx);
61-
threads.emplace_back(CreateThread(thread, dap.thread_format));
57+
// Client requests the baseline of currently existing threads after
58+
// a successful launch or attach by sending a 'threads' request
59+
// right after receiving the configurationDone response.
60+
// If no thread has reported to the client, it prevents something
61+
// like the pause request from working in the running state.
62+
// Return the cache of initial threads as the process might have resumed
63+
if (dap.initial_thread_list) {
64+
threads = dap.initial_thread_list.value();
65+
dap.initial_thread_list.reset();
66+
} else {
67+
threads = GetThreads(dap.target.GetProcess(), dap.thread_format);
6268
}
69+
6370
if (threads.size() == 0) {
6471
response["success"] = llvm::json::Value(false);
6572
}

lldb/tools/lldb-dap/JSONUtils.cpp

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -870,6 +870,19 @@ llvm::json::Value CreateThread(lldb::SBThread &thread, lldb::SBFormat &format) {
870870
return llvm::json::Value(std::move(object));
871871
}
872872

873+
llvm::json::Array GetThreads(lldb::SBProcess process, lldb::SBFormat &format) {
874+
lldb::SBMutex lock = process.GetTarget().GetAPIMutex();
875+
std::lock_guard<lldb::SBMutex> guard(lock);
876+
877+
llvm::json::Array threads;
878+
const uint32_t num_threads = process.GetNumThreads();
879+
for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
880+
lldb::SBThread thread = process.GetThreadAtIndex(thread_idx);
881+
threads.emplace_back(CreateThread(thread, format));
882+
}
883+
return threads;
884+
}
885+
873886
// "StoppedEvent": {
874887
// "allOf": [ { "$ref": "#/definitions/Event" }, {
875888
// "type": "object",

lldb/tools/lldb-dap/JSONUtils.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -414,6 +414,8 @@ llvm::json::Value CreateExtendedStackFrameLabel(lldb::SBThread &thread,
414414
/// definition outlined by Microsoft.
415415
llvm::json::Value CreateThread(lldb::SBThread &thread, lldb::SBFormat &format);
416416

417+
llvm::json::Array GetThreads(lldb::SBProcess process, lldb::SBFormat &format);
418+
417419
/// Create a "StoppedEvent" object for a LLDB thread object.
418420
///
419421
/// This function will fill in the following keys in the returned

0 commit comments

Comments
 (0)