Skip to content

[lldb] Make SBProcess thread related actions listen to StopLocker #134339

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 15, 2025
Merged

[lldb] Make SBProcess thread related actions listen to StopLocker #134339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4f0ac25
[lldb] Make SBProcess thread related actions listen to StopLocker
kusmour Apr 4, 2025
13f7249
[lldb-dap] Client expects initial threads request not empty
kusmour Apr 9, 2025
37340cb
Apply suggestions from code review
kusmour Apr 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,6 +649,10 @@ def request_configurationDone(self):
response = self.send_recv(command_dict)
if response:
self.configuration_done_sent = True
# Client requests the baseline of currently existing threads after
# a successful launch or attach.
# Kick off the threads request that follows
self.request_threads()
return response

def _process_stopped(self):
Expand Down
20 changes: 11 additions & 9 deletions lldb/source/API/SBProcess.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,10 +193,11 @@ uint32_t SBProcess::GetNumThreads() {
if (process_sp) {
Process::StopLocker stop_locker;

const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
std::lock_guard<std::recursive_mutex> guard(
process_sp->GetTarget().GetAPIMutex());
num_threads = process_sp->GetThreadList().GetSize(can_update);
if (stop_locker.TryLock(&process_sp->GetRunLock())) {
std::lock_guard<std::recursive_mutex> guard(
process_sp->GetTarget().GetAPIMutex());
num_threads = process_sp->GetThreadList().GetSize();
}
}

return num_threads;
Expand Down Expand Up @@ -393,11 +394,12 @@ SBThread SBProcess::GetThreadAtIndex(size_t index) {
ProcessSP process_sp(GetSP());
if (process_sp) {
Process::StopLocker stop_locker;
const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
std::lock_guard<std::recursive_mutex> guard(
process_sp->GetTarget().GetAPIMutex());
thread_sp = process_sp->GetThreadList().GetThreadAtIndex(index, can_update);
sb_thread.SetThread(thread_sp);
if (stop_locker.TryLock(&process_sp->GetRunLock())) {
std::lock_guard<std::recursive_mutex> guard(
process_sp->GetTarget().GetAPIMutex());
thread_sp = process_sp->GetThreadList().GetThreadAtIndex(index, false);
sb_thread.SetThread(thread_sp);
}
}

return sb_thread;
Expand Down
2 changes: 1 addition & 1 deletion lldb/test/API/tools/lldb-dap/attach/TestDAP_attach.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
"""
Test lldb-dap setBreakpoints request
Test lldb-dap attach request
"""


Expand Down
3 changes: 3 additions & 0 deletions lldb/tools/lldb-dap/DAP.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,9 @@ struct DAP {
/// The set of features supported by the connected client.
llvm::DenseSet<ClientFeature> clientFeatures;

/// The initial thread list upon attaching.
std::optional<llvm::json::Array> initial_thread_list;

/// Creates a new DAP sessions.
///
/// \param[in] log
Expand Down
10 changes: 9 additions & 1 deletion lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ namespace lldb_dap {
// just an acknowledgement, so no body field is required."
// }]
// },

void ConfigurationDoneRequestHandler::operator()(
const llvm::json::Object &request) const {
llvm::json::Object response;
Expand All @@ -52,8 +53,15 @@ void ConfigurationDoneRequestHandler::operator()(
dap.configuration_done_sent = true;
if (dap.stop_at_entry)
SendThreadStoppedEvent(dap);
else
else {
// Client requests the baseline of currently existing threads after
// a successful launch or attach by sending a 'threads' request
// right after receiving the configurationDone response.
// Obtain the list of threads before we resume the process
Copy link
Collaborator

@clayborg clayborg Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe expand on this a bit saying something like:

// If a 'threads' request returns no threads, then the interrupt 
// button won't be available in the GUI debugger controls, so
// we need to report valid threads back to the IDE. We also need
// a valid thread for the "continued" packet to be sent.

Copy link
Contributor Author

@kusmour kusmour Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pause button relies on having some threads informations and it always send the threadID in the pause request. I have also checked the VSCode source code about fetching threads. If no threads are returned in the threads response then VSCode won't update the know thread list.

Which means, we really just need to return the baseline of threads for the debug session to carry on.

Copy link
Contributor Author

@kusmour kusmour Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for the continued event, VSCode will ignore the threadID if allThreadsContinued: true. Looks like can send an invalid threadID if resuming all threads

Copy link
Collaborator

@clayborg clayborg Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, we just need to change our code to always send it even if the dap.focus_tid is not valid.

Copy link
Contributor Author

@kusmour kusmour Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

In this case, if the attach is expecting the execution continues then we should treat it the same way as launch. I am happy to discuss more about this as well as if we should support stopOnEntry for attaching :) going forward.

dap.initial_thread_list =
GetThreads(dap.target.GetProcess(), dap.thread_format);
dap.target.GetProcess().Continue();
}
}

} // namespace lldb_dap
17 changes: 12 additions & 5 deletions lldb/tools/lldb-dap/Handler/ThreadsRequestHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,16 +50,23 @@ namespace lldb_dap {
// }
void ThreadsRequestHandler::operator()(
const llvm::json::Object &request) const {
lldb::SBProcess process = dap.target.GetProcess();
llvm::json::Object response;
FillResponse(request, response);

const uint32_t num_threads = process.GetNumThreads();
llvm::json::Array threads;
for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
lldb::SBThread thread = process.GetThreadAtIndex(thread_idx);
threads.emplace_back(CreateThread(thread, dap.thread_format));
// Client requests the baseline of currently existing threads after
// a successful launch or attach by sending a 'threads' request
// right after receiving the configurationDone response.
// If no thread has reported to the client, it prevents something
// like the pause request from working in the running state.
// Return the cache of initial threads as the process might have resumed
if (dap.initial_thread_list) {
threads = dap.initial_thread_list.value();
dap.initial_thread_list.reset();
} else {
threads = GetThreads(dap.target.GetProcess(), dap.thread_format);
}

if (threads.size() == 0) {
response["success"] = llvm::json::Value(false);
}
Expand Down
13 changes: 13 additions & 0 deletions lldb/tools/lldb-dap/JSONUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -870,6 +870,19 @@ llvm::json::Value CreateThread(lldb::SBThread &thread, lldb::SBFormat &format) {
return llvm::json::Value(std::move(object));
}

llvm::json::Array GetThreads(lldb::SBProcess process, lldb::SBFormat &format) {
lldb::SBMutex lock = process.GetTarget().GetAPIMutex();
std::lock_guard<lldb::SBMutex> guard(lock);

llvm::json::Array threads;
const uint32_t num_threads = process.GetNumThreads();
for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
lldb::SBThread thread = process.GetThreadAtIndex(thread_idx);
threads.emplace_back(CreateThread(thread, format));
}
return threads;
}

// "StoppedEvent": {
// "allOf": [ { "$ref": "#/definitions/Event" }, {
// "type": "object",
Expand Down
2 changes: 2 additions & 0 deletions lldb/tools/lldb-dap/JSONUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,8 @@ llvm::json::Value CreateExtendedStackFrameLabel(lldb::SBThread &thread,
/// definition outlined by Microsoft.
llvm::json::Value CreateThread(lldb::SBThread &thread, lldb::SBFormat &format);

llvm::json::Array GetThreads(lldb::SBProcess process, lldb::SBFormat &format);

/// Create a "StoppedEvent" object for a LLDB thread object.
///
/// This function will fill in the following keys in the returned
Expand Down
Loading