Skip to content

[lldb-dap] Refactoring lldb-dap 'launch' request to use typed RequestHandler<>. #133624

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

Merged
merged 19 commits into from
Apr 25, 2025
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -860,7 +860,8 @@ def request_launch(
args_dict["enableAutoVariableSummaries"] = enableAutoVariableSummaries
args_dict["enableSyntheticChildDebugging"] = enableSyntheticChildDebugging
args_dict["displayExtendedBacktrace"] = displayExtendedBacktrace
args_dict["commandEscapePrefix"] = commandEscapePrefix
if commandEscapePrefix:
args_dict["commandEscapePrefix"] = commandEscapePrefix
command_dict = {"command": "launch", "type": "request", "arguments": args_dict}
response = self.send_recv(command_dict)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,8 @@ def cleanup():

if not (response and response["success"]):
self.assertTrue(
response["success"], "launch failed (%s)" % (response["message"])
response["success"],
"launch failed (%s)" % (response["body"]["error"]["format"]),
)
return response

Expand Down
6 changes: 4 additions & 2 deletions lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ def test_termination(self):
self.dap_server.request_disconnect()

# Wait until the underlying lldb-dap process dies.
self.dap_server.process.wait(timeout=lldbdap_testcase.DAPTestCaseBase.timeoutval)
self.dap_server.process.wait(
timeout=lldbdap_testcase.DAPTestCaseBase.timeoutval
)

# Check the return code
self.assertEqual(self.dap_server.process.poll(), 0)
Expand Down Expand Up @@ -460,7 +462,7 @@ def test_failing_launch_commands(self):

self.assertFalse(response["success"])
self.assertRegex(
response["message"],
response["body"]["error"]["format"],
r"Failed to run launch commands\. See the Debug Console for more details",
)

Expand Down
54 changes: 39 additions & 15 deletions lldb/tools/lldb-dap/DAP.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ DAP::DAP(Log *log, const ReplMode default_repl_mode,
std::vector<std::string> pre_init_commands, Transport &transport)
: log(log), transport(transport), broadcaster("lldb-dap"),
exception_breakpoints(), focus_tid(LLDB_INVALID_THREAD_ID),
stop_at_entry(false), is_attach(false),
is_attach(false), stop_at_entry(false),
restarting_process_id(LLDB_INVALID_PROCESS_ID),
configuration_done_sent(false), waiting_for_run_in_terminal(false),
progress_event_reporter(
Expand Down Expand Up @@ -659,9 +659,7 @@ void DAP::RunTerminateCommands() {
configuration.terminateCommands);
}

lldb::SBTarget
DAP::CreateTargetFromArguments(const llvm::json::Object &arguments,
lldb::SBError &error) {
lldb::SBTarget DAP::CreateTarget(lldb::SBError &error) {
// Grab the name of the program we need to debug and create a target using
// the given program as an argument. Executable file can be a source of target
// architecture and platform, if they differ from the host. Setting exe path
Expand All @@ -672,21 +670,17 @@ DAP::CreateTargetFromArguments(const llvm::json::Object &arguments,
// enough information to determine correct arch and platform (or ELF can be
// omitted at all), so it is good to leave the user an apportunity to specify
// those. Any of those three can be left empty.
const llvm::StringRef target_triple =
GetString(arguments, "targetTriple").value_or("");
const llvm::StringRef platform_name =
GetString(arguments, "platformName").value_or("");
const llvm::StringRef program = GetString(arguments, "program").value_or("");
auto target = this->debugger.CreateTarget(
program.data(), target_triple.data(), platform_name.data(),
configuration.program.value_or("").data(),
configuration.targetTriple.value_or("").data(),
configuration.platformName.value_or("").data(),
true, // Add dependent modules.
error);

if (error.Fail()) {
// Update message if there was an error.
error.SetErrorStringWithFormat(
"Could not create a target for a program '%s': %s.", program.data(),
error.GetCString());
"Could not create a target for a program: %s.", error.GetCString());
Copy link
Member

Choose a reason for hiding this comment

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

Since you're touching this: error messages should start with a lowercase and not end with periods so they can be concatenated. I realize we're not very consistent in that aspect but since you're here I figured I'd at least mention it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously the error message would print something along the lines of:

Could not create a target for a program 'a.out': 'a.out' does not exist.

I'm not sure the 'Could not create target' part adds anything, removing it the error message becomes:

'a.out' does not exist

Here is an example using:

        {
            "type": "lldb-dap",
            "request": "launch",
            "name": "Debug bad path",
            "program": "~/Projects/baz-bad-path"
        },
Screenshot 2025-04-24 at 5 25 44 PM

Copy link
Member

Choose a reason for hiding this comment

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

Agreed

}

return target;
Expand Down Expand Up @@ -944,7 +938,7 @@ llvm::Error DAP::Loop() {
return queue_reader.get();
}

lldb::SBError DAP::WaitForProcessToStop(uint32_t seconds) {
lldb::SBError DAP::WaitForProcessToStop(std::chrono::seconds seconds) {
lldb::SBError error;
lldb::SBProcess process = target.GetProcess();
if (!process.IsValid()) {
Expand Down Expand Up @@ -979,8 +973,8 @@ lldb::SBError DAP::WaitForProcessToStop(uint32_t seconds) {
}
std::this_thread::sleep_for(std::chrono::microseconds(250));
}
error.SetErrorStringWithFormat("process failed to stop within %u seconds",
seconds);
error.SetErrorStringWithFormat("process failed to stop within %lld seconds",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Warning on AArch64 Linux compiling with clang:

[665/779] Building CXX object tools/lldb/tools/lldb-dap/CMakeFiles/lldb-dap.dir/DAP.cpp.o
/home/david.spickett/llvm-project/lldb/tools/lldb-dap/DAP.cpp:972:34: warning: format specifies type 'long long' but the argument has type 'rep' (aka 'long') [-Wformat]
  971 |   error.SetErrorStringWithFormat("process failed to stop within %lld seconds",
      |                                                                 ~~~~
      |                                                                 %ld
  972 |                                  seconds.count());
      |                                  ^~~~~~~~~~~~~~~
1 warning generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sent #137371 to address the warning.

seconds.count());
return error;
}

Expand Down Expand Up @@ -1177,6 +1171,36 @@ bool SendEventRequestHandler::DoExecute(lldb::SBDebugger debugger,
return true;
}

void DAP::ConfigureSourceMaps() {
if (configuration.sourceMap.empty() && !configuration.sourcePath)
return;

std::string sourceMapCommand;
llvm::raw_string_ostream strm(sourceMapCommand);
strm << "settings set target.source-map ";

if (!configuration.sourceMap.empty()) {
for (const auto &kv : configuration.sourceMap) {
strm << "\"" << kv.first << "\" \"" << kv.second << "\" ";
}
} else if (configuration.sourcePath) {
strm << "\".\" \"" << *configuration.sourcePath << "\"";
}

RunLLDBCommands("Setting source map:", {sourceMapCommand});
}

void DAP::SetConfiguration(const protocol::Configuration &config,
bool is_attach) {
configuration = config;
this->is_attach = is_attach;

if (configuration.customFrameFormat)
SetFrameFormat(*configuration.customFrameFormat);
if (configuration.customThreadFormat)
SetThreadFormat(*configuration.customThreadFormat);
}

void DAP::SetFrameFormat(llvm::StringRef format) {
if (format.empty())
return;
Expand Down
23 changes: 13 additions & 10 deletions lldb/tools/lldb-dap/DAP.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#include "lldb/lldb-types.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/FunctionExtras.h"
#include "llvm/ADT/SmallSet.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
Expand Down Expand Up @@ -175,14 +176,14 @@ struct DAP {
llvm::once_flag init_exception_breakpoints_flag;
// Map step in target id to list of function targets that user can choose.
llvm::DenseMap<lldb::addr_t, std::string> step_in_targets;
// A copy of the last LaunchRequest or AttachRequest so we can reuse its
// arguments if we get a RestartRequest.
std::optional<llvm::json::Object> last_launch_or_attach_request;
// A copy of the last LaunchRequest so we can reuse its arguments if we get a
// RestartRequest. Restarting an AttachRequest is not supported.
std::optional<protocol::LaunchRequestArguments> last_launch_request;
lldb::tid_t focus_tid;
bool disconnecting = false;
llvm::once_flag terminated_event_flag;
bool stop_at_entry;
bool is_attach;
bool stop_at_entry;
// The process event thread normally responds to process exited events by
// shutting down the entire adapter. When we're restarting, we keep the id of
// the old process here so we can detect this case and keep running.
Expand All @@ -199,7 +200,6 @@ struct DAP {
llvm::SmallDenseMap<int64_t, std::unique_ptr<ResponseHandler>>
inflight_reverse_requests;
ReplMode repl_mode;

lldb::SBFormat frame_format;
lldb::SBFormat thread_format;
// This is used to allow request_evaluate to handle empty expressions
Expand Down Expand Up @@ -248,6 +248,12 @@ struct DAP {
/// Stop event handler threads.
void StopEventHandlers();

/// Configures the debug adapter for launching/attaching.
void SetConfiguration(const protocol::Configuration &confing, bool is_attach);

/// Configure source maps based on the current `DAPConfiguration`.
void ConfigureSourceMaps();

/// Serialize the JSON value into a string and send the JSON packet to the
/// "out" stream.
void SendJSON(const llvm::json::Value &json);
Expand Down Expand Up @@ -311,17 +317,14 @@ struct DAP {
void RunTerminateCommands();

/// Create a new SBTarget object from the given request arguments.
/// \param[in] arguments
/// Launch configuration arguments.
///
/// \param[out] error
/// An SBError object that will contain an error description if
/// function failed to create the target.
///
/// \return
/// An SBTarget object.
lldb::SBTarget CreateTargetFromArguments(const llvm::json::Object &arguments,
lldb::SBError &error);
lldb::SBTarget CreateTarget(lldb::SBError &error);

/// Set given target object as a current target for lldb-dap and start
/// listeing for its breakpoint events.
Expand Down Expand Up @@ -395,7 +398,7 @@ struct DAP {
/// The number of seconds to poll the process to wait until it is stopped.
///
/// \return Error if waiting for the process fails, no error if succeeds.
lldb::SBError WaitForProcessToStop(uint32_t seconds);
lldb::SBError WaitForProcessToStop(std::chrono::seconds seconds);

void SetFrameFormat(llvm::StringRef format);

Expand Down
11 changes: 6 additions & 5 deletions lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,8 @@ namespace lldb_dap {
// acknowledgement, so no body field is required."
// }]
// }

void AttachRequestHandler::operator()(const llvm::json::Object &request) const {
dap.is_attach = true;
dap.last_launch_or_attach_request = request;
llvm::json::Object response;
lldb::SBError error;
FillResponse(request, response);
Expand All @@ -65,6 +63,7 @@ void AttachRequestHandler::operator()(const llvm::json::Object &request) const {
attach_info.SetWaitForLaunch(wait_for, false /*async*/);
dap.configuration.initCommands = GetStrings(arguments, "initCommands");
dap.configuration.preRunCommands = GetStrings(arguments, "preRunCommands");
dap.configuration.postRunCommands = GetStrings(arguments, "postRunCommands");
dap.configuration.stopCommands = GetStrings(arguments, "stopCommands");
dap.configuration.exitCommands = GetStrings(arguments, "exitCommands");
dap.configuration.terminateCommands =
Expand All @@ -76,7 +75,6 @@ void AttachRequestHandler::operator()(const llvm::json::Object &request) const {
dap.stop_at_entry = core_file.empty()
? GetBoolean(arguments, "stopOnEntry").value_or(false)
: true;
dap.configuration.postRunCommands = GetStrings(arguments, "postRunCommands");
const llvm::StringRef debuggerRoot =
GetString(arguments, "debuggerRoot").value_or("");
dap.configuration.enableAutoVariableSummaries =
Expand All @@ -87,6 +85,9 @@ void AttachRequestHandler::operator()(const llvm::json::Object &request) const {
GetBoolean(arguments, "displayExtendedBacktrace").value_or(false);
dap.configuration.commandEscapePrefix =
GetString(arguments, "commandEscapePrefix").value_or("`");
dap.configuration.program = GetString(arguments, "program");
dap.configuration.targetTriple = GetString(arguments, "targetTriple");
dap.configuration.platformName = GetString(arguments, "platformName");
dap.SetFrameFormat(GetString(arguments, "customFrameFormat").value_or(""));
dap.SetThreadFormat(GetString(arguments, "customThreadFormat").value_or(""));

Expand All @@ -110,7 +111,7 @@ void AttachRequestHandler::operator()(const llvm::json::Object &request) const {
SetSourceMapFromArguments(*arguments);

lldb::SBError status;
dap.SetTarget(dap.CreateTargetFromArguments(*arguments, status));
dap.SetTarget(dap.CreateTarget(status));
if (status.Fail()) {
response["success"] = llvm::json::Value(false);
EmplaceSafeString(response, "message", status.GetCString());
Expand Down Expand Up @@ -180,7 +181,7 @@ void AttachRequestHandler::operator()(const llvm::json::Object &request) const {

// Make sure the process is attached and stopped before proceeding as the
// the launch commands are not run using the synchronous mode.
error = dap.WaitForProcessToStop(timeout_seconds);
error = dap.WaitForProcessToStop(std::chrono::seconds(timeout_seconds));
}

if (error.Success() && core_file.empty()) {
Expand Down
Loading
Loading