Skip to content

[lldb-dap] Refactoring IOStream into Transport handler. #130026

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 10 commits into from
Mar 12, 2025
Merged

[lldb-dap] Refactoring IOStream into Transport handler. #130026

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c340c38
[lldb-dap] Refactoring IOStream into Transport handler.
ashgti Mar 6, 2025
138d4ca
Adjusting `Transport::Read` to return an `optional<string>` and addin…
ashgti Mar 9, 2025
9fdcba9
Adjusting the reader helpers to take `lldb_private::IOObject *` inste…
ashgti Mar 10, 2025
9090a31
Aplying clang-format.
ashgti Mar 10, 2025
40d149d
Applying reviewer feedback.
ashgti Mar 11, 2025
7d9eeb1
Adjusting `Transport.Read` to return an `llvm::Expected<std::optional…
ashgti Mar 11, 2025
564cfc4
Apply suggestions from code review
ashgti Mar 12, 2025
025f802
Removing DAP::client_name and instead only storing the name in Transport
ashgti Mar 12, 2025
972028b
Reverting changes to `pre_init_commands` type in DAP::DAP.
ashgti Mar 12, 2025
2694f19
Applying clang-format.
ashgti Mar 12, 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: 2 additions & 2 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 @@ -337,7 +337,7 @@ def send_recv(self, command):
self.send_packet(
{
"type": "response",
"seq": -1,
"seq": 0,
"request_seq": response_or_request["seq"],
"success": True,
"command": "runInTerminal",
Expand All @@ -349,7 +349,7 @@ def send_recv(self, command):
self.send_packet(
{
"type": "response",
"seq": -1,
"seq": 0,
"request_seq": response_or_request["seq"],
"success": True,
"command": "startDebugging",
Expand Down
41 changes: 26 additions & 15 deletions lldb/test/API/tools/lldb-dap/io/TestDAP_io.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
Test lldb-dap IO handling.
"""

import sys

from lldbsuite.test.decorators import *
import lldbdap_testcase
import dap_server
Expand All @@ -19,18 +21,18 @@ def cleanup():
if process.poll() is None:
process.terminate()
process.wait()
stdout_data = process.stdout.read()
stderr_data = process.stderr.read()
print("========= STDOUT =========")
print(stdout_data)
print("========= END =========")
print("========= STDERR =========")
print(stderr_data)
print("========= END =========")
print("========= DEBUG ADAPTER PROTOCOL LOGS =========")
stdout_data = process.stdout.read().decode()
stderr_data = process.stderr.read().decode()
print("========= STDOUT =========", file=sys.stderr)
print(stdout_data, file=sys.stderr)
print("========= END =========", file=sys.stderr)
print("========= STDERR =========", file=sys.stderr)
print(stderr_data, file=sys.stderr)
print("========= END =========", file=sys.stderr)
print("========= DEBUG ADAPTER PROTOCOL LOGS =========", file=sys.stderr)
with open(log_file_path, "r") as file:
print(file.read())
print("========= END =========")
print(file.read(), file=sys.stderr)
print("========= END =========", file=sys.stderr)

# Execute the cleanup function during test case tear down.
self.addTearDownHook(cleanup)
Expand All @@ -45,14 +47,23 @@ def test_eof_immediately(self):
process.stdin.close()
self.assertEqual(process.wait(timeout=5.0), 0)

def test_invalid_header(self):
"""
lldb-dap handles invalid message headers.
"""
process = self.launch()
process.stdin.write(b"not the corret message header")
process.stdin.close()
self.assertEqual(process.wait(timeout=5.0), 1)

def test_partial_header(self):
"""
lldb-dap handles parital message headers.
"""
process = self.launch()
process.stdin.write(b"Content-Length: ")
process.stdin.close()
self.assertEqual(process.wait(timeout=5.0), 0)
self.assertEqual(process.wait(timeout=5.0), 1)

def test_incorrect_content_length(self):
"""
Expand All @@ -61,13 +72,13 @@ def test_incorrect_content_length(self):
process = self.launch()
process.stdin.write(b"Content-Length: abc")
process.stdin.close()
self.assertEqual(process.wait(timeout=5.0), 0)
self.assertEqual(process.wait(timeout=5.0), 1)

def test_partial_content_length(self):
"""
lldb-dap handles partial messages.
"""
process = self.launch()
process.stdin.write(b"Content-Length: 10{")
process.stdin.write(b"Content-Length: 10\r\n\r\n{")
process.stdin.close()
self.assertEqual(process.wait(timeout=5.0), 0)
self.assertEqual(process.wait(timeout=5.0), 1)
4 changes: 2 additions & 2 deletions lldb/tools/lldb-dap/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,14 @@ add_lldb_tool(lldb-dap
FifoFiles.cpp
FunctionBreakpoint.cpp
InstructionBreakpoint.cpp
IOStream.cpp
JSONUtils.cpp
LLDBUtils.cpp
OutputRedirector.cpp
ProgressEvent.cpp
Protocol.cpp
RunInTerminal.cpp
SourceBreakpoint.cpp
Protocol.cpp
Transport.cpp
Watchpoint.cpp

Handler/ResponseHandler.cpp
Expand Down
117 changes: 32 additions & 85 deletions lldb/tools/lldb-dap/DAP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "JSONUtils.h"
#include "LLDBUtils.h"
#include "OutputRedirector.h"
#include "Transport.h"
#include "lldb/API/SBBreakpoint.h"
#include "lldb/API/SBCommandInterpreter.h"
#include "lldb/API/SBCommandReturnObject.h"
Expand Down Expand Up @@ -63,12 +64,12 @@ const char DEV_NULL[] = "/dev/null";

namespace lldb_dap {

DAP::DAP(llvm::StringRef client_name, llvm::StringRef path, std::ofstream *log,
lldb::IOObjectSP input, lldb::IOObjectSP output, ReplMode repl_mode,
std::vector<std::string> pre_init_commands)
: client_name(client_name), debug_adapter_path(path), log(log),
input(std::move(input)), output(std::move(output)),
broadcaster("lldb-dap"), exception_breakpoints(),
DAP::DAP(llvm::StringRef path, std::ofstream *log,
const ReplMode default_repl_mode,
const std::vector<std::string> &pre_init_commands,
llvm::StringRef client_name, Transport &transport)
: debug_adapter_path(path), log(log), client_name(client_name),
transport(transport), broadcaster("lldb-dap"), exception_breakpoints(),
pre_init_commands(std::move(pre_init_commands)),
focus_tid(LLDB_INVALID_THREAD_ID), stop_at_entry(false), is_attach(false),
enable_auto_variable_summaries(false),
Expand All @@ -78,7 +79,7 @@ DAP::DAP(llvm::StringRef client_name, llvm::StringRef path, std::ofstream *log,
configuration_done_sent(false), waiting_for_run_in_terminal(false),
progress_event_reporter(
[&](const ProgressEvent &event) { SendJSON(event.ToJSON()); }),
reverse_request_seq(0), repl_mode(repl_mode) {}
reverse_request_seq(0), repl_mode(default_repl_mode) {}

DAP::~DAP() = default;

Expand Down Expand Up @@ -221,52 +222,22 @@ void DAP::StopEventHandlers() {
}
}

// Send the JSON in "json_str" to the "out" stream. Correctly send the
// "Content-Length:" field followed by the length, followed by the raw
// JSON bytes.
void DAP::SendJSON(const std::string &json_str) {
output.write_full("Content-Length: ");
output.write_full(llvm::utostr(json_str.size()));
output.write_full("\r\n\r\n");
output.write_full(json_str);
}

// Serialize the JSON value into a string and send the JSON packet to
// the "out" stream.
void DAP::SendJSON(const llvm::json::Value &json) {
std::string json_str;
llvm::raw_string_ostream strm(json_str);
strm << json;
static std::mutex mutex;
std::lock_guard<std::mutex> locker(mutex);
SendJSON(json_str);

DAP_LOG(log, "({0}) <-- {1}", client_name, json_str);
}

// Read a JSON packet from the "in" stream.
std::string DAP::ReadJSON() {
std::string length_str;
std::string json_str;
int length;

if (!input.read_expected(log, "Content-Length: "))
return json_str;

if (!input.read_line(log, length_str))
return json_str;

if (!llvm::to_integer(length_str, length))
return json_str;

if (!input.read_expected(log, "\r\n"))
return json_str;

if (!input.read_full(log, length, json_str))
return json_str;

DAP_LOG(log, "({0}) --> {1}", client_name, json_str);
return json_str;
// FIXME: Instead of parsing the output message from JSON, pass the `Message`
// as parameter to `SendJSON`.
protocol::Message message;
llvm::json::Path::Root root;
if (!fromJSON(json, message, root)) {
DAP_LOG_ERROR(log, root.getError(), "({1}) encoding failed: {0}",
client_name);
return;
}
auto err = transport.Write(message);
if (err) {
DAP_LOG_ERROR(log, std::move(err), "({1}) write failed: {0}", client_name);
}
}

// "OutputEvent": {
Expand Down Expand Up @@ -693,29 +664,10 @@ void DAP::SetTarget(const lldb::SBTarget target) {
}
}

PacketStatus DAP::GetNextObject(llvm::json::Object &object) {
std::string json = ReadJSON();
if (json.empty())
return PacketStatus::EndOfFile;

llvm::StringRef json_sref(json);
llvm::Expected<llvm::json::Value> json_value = llvm::json::parse(json_sref);
if (!json_value) {
DAP_LOG_ERROR(log, json_value.takeError(),
"({1}) failed to parse JSON: {0}", client_name);
return PacketStatus::JSONMalformed;
}

llvm::json::Object *object_ptr = json_value->getAsObject();
if (!object_ptr) {
DAP_LOG(log, "({0}) error: json packet isn't a object", client_name);
return PacketStatus::JSONNotObject;
}
object = *object_ptr;
return PacketStatus::Success;
}

bool DAP::HandleObject(const llvm::json::Object &object) {
bool DAP::HandleObject(const protocol::Message &M) {
// FIXME: Directly handle `Message` instead of serializing to JSON.
llvm::json::Value v = toJSON(M);
llvm::json::Object object = *v.getAsObject();
const auto packet_type = GetString(object, "type");
if (packet_type == "request") {
const auto command = GetString(object, "command");
Expand Down Expand Up @@ -749,9 +701,8 @@ bool DAP::HandleObject(const llvm::json::Object &object) {
// Result should be given, use null if not.
if (GetBoolean(object, "success").value_or(false)) {
llvm::json::Value Result = nullptr;
if (auto *B = object.get("body")) {
if (auto *B = object.get("body"))
Result = std::move(*B);
}
(*response_handler)(Result);
} else {
llvm::StringRef message = GetString(object, "message");
Expand Down Expand Up @@ -818,19 +769,15 @@ llvm::Error DAP::Loop() {
StopEventHandlers();
});
while (!disconnecting) {
llvm::json::Object object;
lldb_dap::PacketStatus status = GetNextObject(object);
llvm::Expected<std::optional<protocol::Message>> next = transport.Read();
if (!next)
return next.takeError();

if (status == lldb_dap::PacketStatus::EndOfFile) {
// nullopt on EOF
if (!*next)
break;
}

if (status != lldb_dap::PacketStatus::Success) {
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"failed to send packet");
}

if (!HandleObject(object)) {
if (!HandleObject(**next)) {
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"unhandled packet");
}
Expand Down
47 changes: 29 additions & 18 deletions lldb/tools/lldb-dap/DAP.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@
#include "FunctionBreakpoint.h"
#include "Handler/RequestHandler.h"
#include "Handler/ResponseHandler.h"
#include "IOStream.h"
#include "InstructionBreakpoint.h"
#include "OutputRedirector.h"
#include "ProgressEvent.h"
#include "Protocol.h"
#include "SourceBreakpoint.h"
#include "Transport.h"
#include "lldb/API/SBBroadcaster.h"
#include "lldb/API/SBCommandInterpreter.h"
#include "lldb/API/SBDebugger.h"
Expand All @@ -39,7 +40,6 @@
#include "llvm/Support/Error.h"
#include "llvm/Support/JSON.h"
#include "llvm/Support/Threading.h"
#include <map>
#include <memory>
#include <mutex>
#include <optional>
Expand Down Expand Up @@ -145,11 +145,10 @@ struct SendEventRequestHandler : public lldb::SBCommandPluginInterface {
};

struct DAP {
llvm::StringRef client_name;
llvm::StringRef debug_adapter_path;
std::ofstream *log;
InputStream input;
OutputStream output;
llvm::StringRef client_name;
Transport &transport;
lldb::SBFile in;
OutputRedirector out;
OutputRedirector err;
Expand Down Expand Up @@ -210,12 +209,33 @@ struct DAP {
// will contain that expression.
std::string last_nonempty_var_expression;

DAP(llvm::StringRef client_name, llvm::StringRef path, std::ofstream *log,
lldb::IOObjectSP input, lldb::IOObjectSP output, ReplMode repl_mode,
std::vector<std::string> pre_init_commands);
/// Creates a new DAP sessions.
///
/// \param[in] path
/// Path to the lldb-dap binary.
/// \param[in] log
/// Log file stream, if configured.
/// \param[in] default_repl_mode
/// Default repl mode behavior, as configured by the binary.
/// \param[in] pre_init_commands
/// LLDB commands to execute as soon as the debugger instance is allocaed.
/// \param[in] client_name
/// Debug session client name, for example 'stdin/stdout' or 'client_1'.
Copy link
Member

@JDevlieghere JDevlieghere Mar 12, 2025

Choose a reason for hiding this comment

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

What's the reason for moving the path and client_name apart?

Copy link
Member

@JDevlieghere JDevlieghere Mar 12, 2025

Choose a reason for hiding this comment

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

If the client_name is stored in the Transport class, do we need to duplicate it in the DAP object? Does it (not) make sense to get it from the transport instance?

Copy link
Contributor Author

@ashgti ashgti Mar 12, 2025

Choose a reason for hiding this comment

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

What's the reason for moving the path and client_name apart?

I tried to reorganize the parameters into more constant between DAP sessions to more unique per DAP session. i.e.

  • path is basically a constant (its the path to lldb-dap itself)
  • log is also either setup or not at launch
  • default_repl_mode and pre_init_commands are CLI flags that affect the starting conditions of the DAP session
  • client_name and transport are the most unique values for the DAP session.

If the client_name is stored in the Transport class, do we need to duplicate it in the DAP object? Does it (not) make sense to get it from the transport instance?

I'll update this to access the client name from the transport.

/// \param[in] transport
/// Transport for this debug session.
DAP(llvm::StringRef path, std::ofstream *log,
const ReplMode default_repl_mode,
const std::vector<std::string> &pre_init_commands,
llvm::StringRef client_name, Transport &transport);

~DAP();

/// DAP is not copyable.
/// @{
DAP(const DAP &rhs) = delete;
void operator=(const DAP &rhs) = delete;
/// @}

ExceptionBreakpoint *GetExceptionBreakpoint(const std::string &filter);
ExceptionBreakpoint *GetExceptionBreakpoint(const lldb::break_id_t bp_id);

Expand All @@ -233,8 +253,6 @@ struct DAP {
// the "out" stream.
void SendJSON(const llvm::json::Value &json);

std::string ReadJSON();

void SendOutput(OutputType o, const llvm::StringRef output);

void SendProgressEvent(uint64_t progress_id, const char *message,
Expand Down Expand Up @@ -307,8 +325,7 @@ struct DAP {
/// listeing for its breakpoint events.
void SetTarget(const lldb::SBTarget target);

PacketStatus GetNextObject(llvm::json::Object &object);
bool HandleObject(const llvm::json::Object &object);
bool HandleObject(const protocol::Message &M);

/// Disconnect the DAP session.
lldb::SBError Disconnect();
Expand Down Expand Up @@ -382,12 +399,6 @@ struct DAP {
InstructionBreakpoint *GetInstructionBreakpoint(const lldb::break_id_t bp_id);

InstructionBreakpoint *GetInstructionBPFromStopReason(lldb::SBThread &thread);

private:
// Send the JSON in "json_str" to the "out" stream. Correctly send the
// "Content-Length:" field followed by the length, followed by the raw
// JSON bytes.
void SendJSON(const std::string &json_str);
};

} // namespace lldb_dap
Expand Down
Loading