[lldb-dap] Refactoring IOStream into Transport handler. #130026
Conversation
ashgti
force-pushed
the
transport
branch
2 times, most recently
from
a53fafd to
2283694
Compare
|
#130090 is a follow up to add improved type handling to |
I was going to wait for #129964 first so I didn't have to many PRs on top of PRs (github PRs don't stack well, at least IMO). |
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesInstead of having two discrete InputStream and OutputStream helpers, this merges the two into a unifed 'Transport' handler. This handler is responsible for reading the DAP message headers, parsing the resulting JSON and converting the messages into Patch is 23.05 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/130026.diff 12 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
index 9471594b66012..0fea3419d9725 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
@@ -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",
@@ -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",
diff --git a/lldb/test/API/tools/lldb-dap/terminated-event/TestDAP_terminatedEvent.py b/lldb/test/API/tools/lldb-dap/terminated-event/TestDAP_terminatedEvent.py
index 6d1c25e8e4534..b0abe2a38dac4 100644
--- a/lldb/test/API/tools/lldb-dap/terminated-event/TestDAP_terminatedEvent.py
+++ b/lldb/test/API/tools/lldb-dap/terminated-event/TestDAP_terminatedEvent.py
@@ -43,7 +43,7 @@ def test_terminated_event(self):
self.continue_to_breakpoints(breakpoint_ids)
self.continue_to_exit()
- statistics = self.dap_server.wait_for_terminated()["statistics"]
+ statistics = self.dap_server.wait_for_terminated()["body"]["$__lldb_statistics"]
self.assertGreater(statistics["totalDebugInfoByteSize"], 0)
self.assertGreater(statistics["totalDebugInfoEnabled"], 0)
self.assertGreater(statistics["totalModuleCountHasDebugInfo"], 0)
diff --git a/lldb/tools/lldb-dap/CMakeLists.txt b/lldb/tools/lldb-dap/CMakeLists.txt
index 9a2d604f4d573..8a76cb58dbcab 100644
--- a/lldb/tools/lldb-dap/CMakeLists.txt
+++ b/lldb/tools/lldb-dap/CMakeLists.txt
@@ -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
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 1f7b25e7c5bcc..36989ad532a2d 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -11,6 +11,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"
@@ -64,8 +65,8 @@ namespace lldb_dap {
DAP::DAP(std::string name, llvm::StringRef path, std::ofstream *log,
lldb::IOObjectSP input, lldb::IOObjectSP output, ReplMode repl_mode,
std::vector<std::string> pre_init_commands)
- : name(std::move(name)), debug_adapter_path(path), log(log),
- input(std::move(input)), output(std::move(output)),
+ : client_name(std::move(name)), debug_adapter_path(path), log(log),
+ transport(client_name, std::move(input), std::move(output)),
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),
@@ -219,65 +220,27 @@ 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);
-
- if (log) {
- auto now = std::chrono::duration<double>(
- std::chrono::system_clock::now().time_since_epoch());
- *log << llvm::formatv("{0:f9} {1} <-- ", now.count(), name).str()
- << std::endl
- << "Content-Length: " << json_str.size() << "\r\n\r\n"
- << llvm::formatv("{0:2}", json).str() << std::endl;
- }
-}
-
-// 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;
-
- if (log) {
- auto now = std::chrono::duration<double>(
- std::chrono::system_clock::now().time_since_epoch());
- *log << llvm::formatv("{0:f9} {1} --> ", now.count(), name).str()
- << std::endl
- << "Content-Length: " << length << "\r\n\r\n";
+ // FIXME: Instead of parsing the output message from JSON, pass the `Message`
+ // as parameter to `SendJSON`.
+ protocol::Message M;
+ llvm::json::Path::Root root;
+ if (!protocol::fromJSON(json, M, root)) {
+ if (log) {
+ std::string error;
+ llvm::raw_string_ostream OS(error);
+ root.printErrorContext(json, OS);
+ *log << "encoding failure: " << error << "\n";
+ }
+ return;
}
- return json_str;
+ auto status = transport.Write(log, M);
+ if (status.Fail() && log)
+ *log << llvm::formatv("failed to send {0}: {1}\n", llvm::json::Value(M),
+ status.AsCString())
+ .str();
}
// "OutputEvent": {
@@ -704,36 +667,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 (auto error = json_value.takeError()) {
- std::string error_str = llvm::toString(std::move(error));
- if (log)
- *log << "error: failed to parse JSON: " << error_str << std::endl
- << json << std::endl;
- return PacketStatus::JSONMalformed;
- }
-
- if (log) {
- *log << llvm::formatv("{0:2}", *json_value).str() << std::endl;
- }
-
- llvm::json::Object *object_ptr = json_value->getAsObject();
- if (!object_ptr) {
- if (log)
- *log << "error: json packet isn't a object" << std::endl;
- 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");
@@ -838,19 +775,16 @@ llvm::Error DAP::Loop() {
StopEventHandlers();
});
while (!disconnecting) {
- llvm::json::Object object;
- lldb_dap::PacketStatus status = GetNextObject(object);
-
- if (status == lldb_dap::PacketStatus::EndOfFile) {
- break;
- }
-
- if (status != lldb_dap::PacketStatus::Success) {
- return llvm::createStringError(llvm::inconvertibleErrorCode(),
- "failed to send packet");
+ auto next = transport.Read(log);
+ if (auto Err = next.takeError()) {
+ // On EOF, simply break out of the loop.
+ std::error_code ec = llvm::errorToErrorCode(std::move(Err));
+ if (ec == Transport::kEOF)
+ break;
+ return llvm::errorCodeToError(ec);
}
- if (!HandleObject(object)) {
+ if (!HandleObject(*next)) {
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"unhandled packet");
}
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index 8b2e498a28c95..f2f52c34af5ea 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -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"
@@ -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>
@@ -145,11 +145,10 @@ struct SendEventRequestHandler : public lldb::SBCommandPluginInterface {
};
struct DAP {
- std::string name;
+ std::string client_name;
llvm::StringRef debug_adapter_path;
std::ofstream *log;
- InputStream input;
- OutputStream output;
+ Transport transport;
lldb::SBFile in;
OutputRedirector out;
OutputRedirector err;
@@ -233,8 +232,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,
@@ -307,8 +304,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();
@@ -382,12 +378,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
diff --git a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
index 5bb73a7ec0d85..7b7d8d5cedaa6 100644
--- a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
@@ -111,7 +111,7 @@ void ProgressEventThreadFunction(DAP &dap) {
// them prevent multiple threads from writing simultaneously so no locking
// is required.
static void EventThreadFunction(DAP &dap) {
- llvm::set_thread_name(dap.name + ".event_handler");
+ llvm::set_thread_name(dap.client_name + ".event_handler");
lldb::SBEvent event;
lldb::SBListener listener = dap.debugger.GetListener();
dap.broadcaster.AddListener(listener, eBroadcastBitStopEventThread);
diff --git a/lldb/tools/lldb-dap/IOStream.cpp b/lldb/tools/lldb-dap/IOStream.cpp
deleted file mode 100644
index ee22a297ec248..0000000000000
--- a/lldb/tools/lldb-dap/IOStream.cpp
+++ /dev/null
@@ -1,73 +0,0 @@
-//===-- IOStream.cpp --------------------------------------------*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#include "IOStream.h"
-#include "lldb/Utility/IOObject.h"
-#include "lldb/Utility/Status.h"
-#include <fstream>
-#include <string>
-
-using namespace lldb_dap;
-
-bool OutputStream::write_full(llvm::StringRef str) {
- if (!descriptor)
- return false;
-
- size_t num_bytes = str.size();
- auto status = descriptor->Write(str.data(), num_bytes);
- return status.Success();
-}
-
-bool InputStream::read_full(std::ofstream *log, size_t length,
- std::string &text) {
- if (!descriptor)
- return false;
-
- std::string data;
- data.resize(length);
-
- auto status = descriptor->Read(data.data(), length);
- if (status.Fail())
- return false;
-
- text += data.substr(0, length);
- return true;
-}
-
-bool InputStream::read_line(std::ofstream *log, std::string &line) {
- line.clear();
- while (true) {
- std::string next;
- if (!read_full(log, 1, next))
- return false;
-
- // If EOF is encoutnered, '' is returned, break out of this loop.
- if (next.empty())
- return false;
-
- line += next;
-
- if (llvm::StringRef(line).ends_with("\r\n"))
- break;
- }
- line.erase(line.size() - 2);
- return true;
-}
-
-bool InputStream::read_expected(std::ofstream *log, llvm::StringRef expected) {
- std::string result;
- if (!read_full(log, expected.size(), result))
- return false;
- if (expected != result) {
- if (log)
- *log << "Warning: Expected '" << expected.str() << "', got '" << result
- << "\n";
- return false;
- }
- return true;
-}
diff --git a/lldb/tools/lldb-dap/IOStream.h b/lldb/tools/lldb-dap/IOStream.h
deleted file mode 100644
index e9fb8e11c92da..0000000000000
--- a/lldb/tools/lldb-dap/IOStream.h
+++ /dev/null
@@ -1,42 +0,0 @@
-//===-- IOStream.h ----------------------------------------------*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef LLDB_TOOLS_LLDB_DAP_IOSTREAM_H
-#define LLDB_TOOLS_LLDB_DAP_IOSTREAM_H
-
-#include "lldb/lldb-forward.h"
-#include "llvm/ADT/StringRef.h"
-#include <fstream>
-#include <string>
-
-namespace lldb_dap {
-
-struct InputStream {
- lldb::IOObjectSP descriptor;
-
- explicit InputStream(lldb::IOObjectSP descriptor)
- : descriptor(std::move(descriptor)) {}
-
- bool read_full(std::ofstream *log, size_t length, std::string &text);
-
- bool read_line(std::ofstream *log, std::string &line);
-
- bool read_expected(std::ofstream *log, llvm::StringRef expected);
-};
-
-struct OutputStream {
- lldb::IOObjectSP descriptor;
-
- explicit OutputStream(lldb::IOObjectSP descriptor)
- : descriptor(std::move(descriptor)) {}
-
- bool write_full(llvm::StringRef str);
-};
-} // namespace lldb_dap
-
-#endif
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index 7094bf60bfbc2..932145b1799bd 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -1526,7 +1526,8 @@ static void addStatistic(lldb::SBTarget &target, llvm::json::Object &event) {
const char *key = keys.GetStringAtIndex(i);
FilterAndGetValueForKey(statistics, key, stats_body);
}
- event.try_emplace("statistics", std::move(stats_body));
+ llvm::json::Object body{{"$__lldb_statistics", std::move(stats_body)}};
+ event.try_emplace("body", std::move(body));
}
llvm::json::Object CreateTerminatedEventObject(lldb::SBTarget &target) {
diff --git a/lldb/tools/lldb-dap/Transport.cpp b/lldb/tools/lldb-dap/Transport.cpp
new file mode 100644
index 0000000000000..f49bf373c4aff
--- /dev/null
+++ b/lldb/tools/lldb-dap/Transport.cpp
@@ -0,0 +1,146 @@
+//===-- Transport.cpp -----------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "Transport.h"
+#include "Protocol.h"
+#include "c++/v1/__system_error/error_code.h"
+#include "lldb/Utility/IOObject.h"
+#include "lldb/Utility/Status.h"
+#include "lldb/lldb-forward.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/raw_ostream.h"
+#include <string>
+#include <system_error>
+#include <utility>
+
+using namespace llvm;
+using namespace lldb;
+using namespace lldb_private;
+using namespace lldb_dap;
+using namespace lldb_dap::protocol;
+
+static Expected<std::string> ReadFull(IOObjectSP &descriptor, size_t length) {
+ if (!descriptor || !descriptor->IsValid())
+ return createStringError("transport input is closed");
+
+ std::string data;
+ data.resize(length);
+
+ auto status = descriptor->Read(data.data(), length);
+ if (status.Fail())
+ return status.takeError();
+
+ // If we got back zero then we have reached EOF.
+ if (length == 0)
+ return createStringError(Transport::kEOF, "end-of-file");
+
+ return data.substr(0, length);
+}
+
+static Expected<std::string> ReadUntil(IOObjectSP &descriptor,
+ StringRef delimiter) {
+ std::string buffer;
+ buffer.reserve(delimiter.size() + 1);
+ while (!llvm::StringRef(buffer).ends_with(delimiter)) {
+ auto next = ReadFull(descriptor, 1);
+ if (auto Err = next.takeError())
+ return std::move(Err);
+ buffer += *next;
+ }
+ return buffer.substr(0, buffer.size() - delimiter.size());
+}
+
+static Error ReadExpected(IOObjectSP &descriptor, StringRef want) {
+ auto got = ReadFull(descriptor, want.size());
+ if (auto Err = got.takeError())
+ return Err;
+ if (*got != want) {
+ return createStringError("want %s, got %s", want.str().c_str(),
+ got->c_str());
+ }
+ return Error::success();
+}
+
+namespace lldb_dap {
+
+const std::error_code Transport::kEOF =
+ std::error_code(0x1001, std::generic_category());
+
+Transport::Transport(StringRef client_name, IOObjectSP input, IOObjectSP output)
+ : m_client_name(client_name), m_input(std::move(input)),
+ m_output(std::move(output)) {}
+
+Expected<protocol::Message> Transport::Read(std::ofstream *log) {
+ // If we don't find the expected header we have reached EOF.
+ if (auto Err = ReadExpected(m_input, "Content-Length: "))
+ return std::move(Err);
+
+ auto rawLength = ReadUntil(m_input, "\r\n\r\n");
+ if (auto Err = rawLength.takeError())
+ return std::move(Err);
+
+ size_t length;
+ if (!to_integer(*rawLength, length))
+ return createStringError("invalid content length %s", rawLength->c_str());
+
+ auto rawJSON = ReadFull(m_input, length);
+ if (auto Err = rawJSON.takeError())
+ return std::move(Err);
+ if (rawJSON->length() != length)
+ return createStringError(
+ "malformed request, expected %ld bytes, got %ld bytes", length,
+ rawJSON->length());
+
+ if (log) {
+ auto now = std::chrono::duration<double>(
+ std::chrono::system_clock::now().time_since_epoch());
+ *log << formatv("{0:f9} <-- ({1}) {2}\n", now.count(), m_client_name,
+ *rawJSON)
+ .str();
+ }
+
+ auto JSON = json::parse(*rawJSON);
+ if (auto Err = JSON.takeError()) {
+ return createStringError("malformed JSON %s\n%s", rawJSON->c_str(),
+ llvm::toString(std::move(Err)).c_str());
+ }
+
+ protocol::Message M;
+ llvm::json::Path::Root Root;
+ if (!fromJSON(*JSON, M, Root)) {
+ std::string error;
+ raw_string_ostream OS(error);
+ Root.printErrorContext(*JSON, OS);
+ return createStringError("malformed request: %s", error.c_str());
+ }
+ return std::move(M);
+}
+
+lldb_private::Status Transport::Write(std::ofstream *log,
+ const protocol::Message &M) {
+ if (!m_output || !m_output->IsValid())
+ return Status("transport output is closed");
+
+ std::string JSON = fo...
[truncated]
|
lldb/tools/lldb-dap/Transport.cpp
Outdated
| return std::move(M); | ||
| } | ||
|
|
||
| lldb_private::Status Transport::Write(std::ofstream *log, |
There was a problem hiding this comment.
This should return an llvm::Error.
| lldb_private::Status Transport::Write(std::ofstream *log, | |
| llvm::Error Transport::Write(std::ofstream *log, |
lldb/tools/lldb-dap/DAP.cpp
Outdated
| if (status != lldb_dap::PacketStatus::Success) { | ||
| return llvm::createStringError(llvm::inconvertibleErrorCode(), | ||
| "failed to send packet"); | ||
| auto next = transport.Read(log); |
There was a problem hiding this comment.
I think this goes beyond LLVM's guidance of auto. It's not obvious from the right hand side what the type of next is.
lldb/tools/lldb-dap/DAP.cpp
Outdated
| // as parameter to `SendJSON`. | ||
| protocol::Message M; | ||
| llvm::json::Path::Root root; | ||
| if (!protocol::fromJSON(json, M, root)) { |
There was a problem hiding this comment.
Now that I see how this function is used, wouldn't it be better for protocol::fromJSON to return an std::optional<protocol::Message> instead of returning a bool and taking the message by reference?
lldb/tools/lldb-dap/DAP.cpp
Outdated
| << "Content-Length: " << length << "\r\n\r\n"; | ||
| // FIXME: Instead of parsing the output message from JSON, pass the `Message` | ||
| // as parameter to `SendJSON`. | ||
| protocol::Message M; |
There was a problem hiding this comment.
This patch is using a mix of LLVM and LLDB naming conventions. Let's be consistent and use lower_camel_case.
lldb/tools/lldb-dap/Transport.cpp
Outdated
| return std::move(Err); | ||
| if (rawJSON->length() != length) | ||
| return createStringError( | ||
| "malformed request, expected %ld bytes, got %ld bytes", length, |
There was a problem hiding this comment.
We have a few places in lldb which concatenate errors with : so I'm trying to be more consistent in using coons to separate error messages. You used a colon on line 120 so let's do the same here.
| "malformed request, expected %ld bytes, got %ld bytes", length, | |
| "malformed request: expected %ld bytes and got %ld bytes", length, |
There was a problem hiding this comment.
Also, %ld is not a correct way to print a variable of type size_t. It's %zd, but I think it'd be even better to avoid this question and format these strings with llvm::formatv, even though the result is a bit longer.
lldb/tools/lldb-dap/Transport.cpp
Outdated
| return createStringError("malformed JSON %s\n%s", rawJSON->c_str(), | ||
| llvm::toString(std::move(Err)).c_str()); |
There was a problem hiding this comment.
It's probably worth adding a colon here too:
| return createStringError("malformed JSON %s\n%s", rawJSON->c_str(), | |
| llvm::toString(std::move(Err)).c_str()); | |
| return createStringError("malformed JSON: %s\n%s", rawJSON->c_str(), | |
| llvm::toString(std::move(Err)).c_str()); |
Which makes me wonder: is it worth including the raw JSON in the error? How about returning just the error and logging the raw JSON?
lldb/tools/lldb-dap/Transport.cpp
Outdated
| const std::error_code Transport::kEOF = | ||
| std::error_code(0x1001, std::generic_category()); |
There was a problem hiding this comment.
Let's not add new members to scopes we don't control. The most llvm-y way to do this kind of thing would be to define a new ErrorInfo type (EOFError?).
There was a problem hiding this comment.
I ended up refactoring this to use an std::optional<protocol::Message> instead of a llvm::Expected<protocol::Message>.
I don't think returning the error was actually helpful and the error handling was getting complicated due to treating EOF as an error.
Instead, I tweaked the Transport class to log any errors it encounters and return std::nullopt whenever there is a problem, including EOF.
There was a problem hiding this comment.
Hmm... I can't say I like the repetitiveness of of the log statements in the Read function, nor the fact that Read and Write methods handle errors differently (one logs while the other returns them). What exactly was making that complicated?
Another option would be to return an Expected<optional<Message>> with a nullopt meaning "EOF". It's a bit of a mouthful, but we do have APIs like that, and it doesn't look like this function will be used from that many places.
lldb/tools/lldb-dap/DAP.cpp
Outdated
| if (auto Err = next.takeError()) { | ||
| // On EOF, simply break out of the loop. | ||
| std::error_code ec = llvm::errorToErrorCode(std::move(Err)); | ||
| if (ec == Transport::kEOF) | ||
| break; | ||
| return llvm::errorCodeToError(ec); | ||
| } |
There was a problem hiding this comment.
Then this would be something like:
| if (auto Err = next.takeError()) { | |
| // On EOF, simply break out of the loop. | |
| std::error_code ec = llvm::errorToErrorCode(std::move(Err)); | |
| if (ec == Transport::kEOF) | |
| break; | |
| return llvm::errorCodeToError(ec); | |
| } | |
| if (auto Err = next.takeError()) { | |
| // On EOF, simply break out of the loop. | |
| return handleErrors(std::move(Err), [](const EOFError &) {}); | |
| } |
There was a problem hiding this comment.
I tweaked the Transport class to log any errors it encounters and return std::nullopt whenever there is a problem, including EOF.
lldb/tools/lldb-dap/Transport.cpp
Outdated
|
|
||
| // If we got back zero then we have reached EOF. | ||
| if (length == 0) | ||
| return createStringError(Transport::kEOF, "end-of-file"); |
There was a problem hiding this comment.
| return createStringError(Transport::kEOF, "end-of-file"); | |
| return makeError<EOFError>(); |
There was a problem hiding this comment.
I tweaked the Transport class to log any errors it encounters and return std::nullopt whenever there is a problem, including EOF.
lldb/tools/lldb-dap/Transport.cpp
Outdated
| using namespace lldb_dap; | ||
| using namespace lldb_dap::protocol; | ||
|
|
||
| static Expected<std::string> ReadFull(IOObjectSP &descriptor, size_t length) { |
There was a problem hiding this comment.
A (non-const) reference to a shared pointer usually not what you want. Since this function doesn't really need the shared-pointerness. I'd go with a reference to the underlying object instead.
| static Expected<std::string> ReadFull(IOObjectSP &descriptor, size_t length) { | |
| static Expected<std::string> ReadFull(IOObject &descriptor, size_t length) { |
There was a problem hiding this comment.
Tweaked to use a pointer to the IOObject * (which is abstract so I can't use a reference to the base class).
There was a problem hiding this comment.
That probably fine, but I am pretty sure that being abstract doesn't prevent you from forming a reference to a class. You must have tried to copy the object somewhere. The main advantage of a reference is that it guarantees that the pointer is not null.
lldb/tools/lldb-dap/Transport.cpp
Outdated
| if (auto Err = ReadExpected(m_input, "Content-Length: ")) | ||
| return std::move(Err); | ||
|
|
||
| auto rawLength = ReadUntil(m_input, "\r\n\r\n"); |
There was a problem hiding this comment.
Is there any risk in this appearing in the middle of a single block returned by the read call (e.g. if the transport layer merges the \n terminating one request with the subsequent request)?
There was a problem hiding this comment.
Shouldn't break the output, even though it will produce something that looks like a message in the middle of a message.
We're not searching for the Content-Length: header, we expect it to be those exact bytes, per the spec. The messages should always be formatted like:
Content-Length: (\d+)\r\n\r\n(.{\1})
Or more generally:
<header><length><separator><body>
<header> = "Content-Length: "
<length> = \d+
<separator> = "\r\n\r\n"
<body> = length bytes
Otherwise the message is malformed.
Once we have a message header, if the Content-Length: string appears in the body of the message, that should be okay.
e.g.
int main() {
printf("Content-Length: 51\r\n\r\n{\"command\":\"disconnect\",\"seq\": 1,\"type\": \"request\"}");
return 0;
}
Works fine with VSCode and doesn't have any problems with lldb-dap.
There was a problem hiding this comment.
Okay, I see what you're doing now. ReadUntil is reading the string one character at a time, which means it can never read past the \r\n terminator. Not particularly efficient, but I suppose it will do given that this just needs to read a couple of bytes.
lldb/tools/lldb-dap/Transport.cpp
Outdated
| return std::move(Err); | ||
| if (rawJSON->length() != length) | ||
| return createStringError( | ||
| "malformed request, expected %ld bytes, got %ld bytes", length, |
There was a problem hiding this comment.
Also, %ld is not a correct way to print a variable of type size_t. It's %zd, but I think it'd be even better to avoid this question and format these strings with llvm::formatv, even though the result is a bit longer.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
lldb/tools/lldb-dap/Transport.cpp
Outdated
| using namespace lldb_dap; | ||
| using namespace lldb_dap::protocol; | ||
|
|
||
| static Expected<std::string> ReadFull(IOObjectSP &descriptor, size_t length) { |
There was a problem hiding this comment.
That probably fine, but I am pretty sure that being abstract doesn't prevent you from forming a reference to a class. You must have tried to copy the object somewhere. The main advantage of a reference is that it guarantees that the pointer is not null.
lldb/tools/lldb-dap/Transport.cpp
Outdated
| if (auto Err = ReadExpected(m_input, "Content-Length: ")) | ||
| return std::move(Err); | ||
|
|
||
| auto rawLength = ReadUntil(m_input, "\r\n\r\n"); |
There was a problem hiding this comment.
Okay, I see what you're doing now. ReadUntil is reading the string one character at a time, which means it can never read past the \r\n terminator. Not particularly efficient, but I suppose it will do given that this just needs to read a couple of bytes.
lldb/tools/lldb-dap/Transport.cpp
Outdated
| const std::error_code Transport::kEOF = | ||
| std::error_code(0x1001, std::generic_category()); |
There was a problem hiding this comment.
Hmm... I can't say I like the repetitiveness of of the log statements in the Read function, nor the fact that Read and Write methods handle errors differently (one logs while the other returns them). What exactly was making that complicated?
Another option would be to return an Expected<optional<Message>> with a nullopt meaning "EOF". It's a bit of a mouthful, but we do have APIs like that, and it doesn't look like this function will be used from that many places.
Instead of having two discrete InputStream and OutputStream helpers, this merges the two into a unifed 'Transport' handler. This handler is responsible for reading the DAP message headers, parsing the resulting JSON and converting the messages into `lldb_dap::protocol::Message`s for both input and output.
…g logging directly to the `Transport` class.
…ad of the shared_ptr.
…<protocol::Message>>` to try to refine the caller. An error reading or validating a message should now result in an `llvm::Error`. EOF is now only acceptable at the start of a message, otherwise we return an error indiciating we encountered a partial message.
|
Apparently rebasing my changes on a main caused GH to not let me reply to some comments directly (still learning GH's PR nuances...). From labath
Should be fixed now, I thought I had tried this but I get a compiler error I misinterpreted as not being able to use a reference.
There is a slight optimization in
Updated to use an |
lldb/tools/lldb-dap/DAP.h
Outdated
| /// \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'. |
There was a problem hiding this comment.
What's the reason for moving the path and client_name apart?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
What's the reason for moving the
pathandclient_nameapart?
I tried to reorganize the parameters into more constant between DAP sessions to more unique per DAP session. i.e.
pathis basically a constant (its the path tolldb-dapitself)logis also either setup or not at launchdefault_repl_modeandpre_init_commandsare CLI flags that affect the starting conditions of the DAP sessionclient_nameandtransportare the most unique values for the DAP session.
If the
client_nameis stored in theTransportclass, do we need to duplicate it in theDAPobject? Does it (not) make sense to get it from the transport instance?
I'll update this to access the client name from the transport.
Instead of having two discrete InputStream and OutputStream helpers, this merges the two into a unifed 'Transport' handler.
This handler is responsible for reading the DAP message headers, parsing the resulting JSON and converting the messages into
lldb_dap::protocol::Messages for both input and output.