[lldb-dap] Refactoring lldb-dap port listening mode to allow multiple connections. #116392
Conversation
ashgti
force-pushed
the
lldb-dap-server-mode
branch
from
43b8fc2 to
acfdb2d
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
vogelsgesang
changed the title
lldb/tools/lldb-dap/lldb-dap.cpp
Outdated
| /// | ||
| /// \return | ||
| /// A fd pointing to the original stdout. | ||
| void SetupRedirection(DAP &dap, int stdoutfd = -1, int stderrfd = -1) { |
There was a problem hiding this comment.
what is the intended behavior here if we have multiple DAP connections running in parallel?
Should the stdout & stderr be redirected to all of them? Only the latest one? Only the first one?
There was a problem hiding this comment.
Each lldb::SBDebugger instance has its own file for stdout/stderr.
When running in server mode we don't actually redirect stdout/stderr to the DAP connection directly.
When running lldb commands the output is directed to the lldb::SBDebugger::GetOutputFileHandle and lldb::SBDebugger::GetErrorFileHandle.
the SBDebugger defaults to stdout/stderr, but we can change those since we have one debugger per client connection.
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesThis refactors the port listening mode to allocate a new DAP object for each connection, allowing multiple connections to run concurrently. Patch is 32.80 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/116392.diff 12 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index 8884ef5933ada8..a899854bb5ae14 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -39,6 +39,7 @@
import signal
from subprocess import *
import sys
+import socket
import time
import traceback
@@ -250,6 +251,13 @@ def which(program):
return None
+def pickrandomport():
+ """Returns a random open port."""
+ with socket.socket() as sock:
+ sock.bind(("", 0))
+ return sock.getsockname()[1]
+
+
class ValueCheck:
def __init__(
self,
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 c29992ce9c7848..e4a53fe0d45907 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
@@ -903,7 +903,7 @@ def request_setBreakpoints(self, file_path, line_array, data=None):
"sourceModified": False,
}
if line_array is not None:
- args_dict["lines"] = "%s" % line_array
+ args_dict["lines"] = line_array
breakpoints = []
for i, line in enumerate(line_array):
breakpoint_data = None
@@ -1154,34 +1154,38 @@ class DebugAdaptorServer(DebugCommunication):
def __init__(
self,
executable=None,
+ launch=True,
port=None,
+ unix_socket=None,
init_commands=[],
log_file=None,
env=None,
):
self.process = None
- if executable is not None:
- adaptor_env = os.environ.copy()
- if env is not None:
- adaptor_env.update(env)
-
- if log_file:
- adaptor_env["LLDBDAP_LOG"] = log_file
- self.process = subprocess.Popen(
- [executable],
- stdin=subprocess.PIPE,
- stdout=subprocess.PIPE,
- stderr=subprocess.PIPE,
- env=adaptor_env,
+ if launch:
+ self.process = DebugAdaptorServer.launch(
+ executable,
+ port=port,
+ unix_socket=unix_socket,
+ log_file=log_file,
+ env=env,
)
- DebugCommunication.__init__(
- self, self.process.stdout, self.process.stdin, init_commands, log_file
- )
- elif port is not None:
+
+ if port:
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.connect(("127.0.0.1", port))
DebugCommunication.__init__(
- self, s.makefile("r"), s.makefile("w"), init_commands
+ self, s.makefile("rb"), s.makefile("wb"), init_commands, log_file
+ )
+ elif unix_socket:
+ s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+ s.connect(unix_socket)
+ DebugCommunication.__init__(
+ self, s.makefile("rb"), s.makefile("wb"), init_commands, log_file
+ )
+ else:
+ DebugCommunication.__init__(
+ self, self.process.stdout, self.process.stdin, init_commands, log_file
)
def get_pid(self):
@@ -1196,6 +1200,39 @@ def terminate(self):
self.process.wait()
self.process = None
+ @classmethod
+ def launch(
+ cls, executable: str, /, port=None, unix_socket=None, log_file=None, env=None
+ ) -> subprocess.Popen:
+ adaptor_env = os.environ.copy()
+ if env:
+ adaptor_env.update(env)
+
+ if log_file:
+ adaptor_env["LLDBDAP_LOG"] = log_file
+
+ args = [executable]
+ if port:
+ args.append("--port")
+ args.append(str(port))
+ elif unix_socket:
+ args.append("--unix-socket")
+ args.append(unix_socket)
+
+ proc = subprocess.Popen(
+ args,
+ stdin=subprocess.PIPE,
+ stdout=subprocess.PIPE,
+ stderr=sys.stdout,
+ env=adaptor_env,
+ )
+
+ if port or unix_socket:
+ # Wait for the server to startup.
+ time.sleep(0.1)
+
+ return proc
+
def attach_options_specified(options):
if options.pid is not None:
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
index a25466f07fa557..3fcc08e9ff55cb 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
@@ -13,7 +13,7 @@ class DAPTestCaseBase(TestBase):
timeoutval = 10 * (10 if ('ASAN_OPTIONS' in os.environ) else 1)
NO_DEBUG_INFO_TESTCASE = True
- def create_debug_adaptor(self, lldbDAPEnv=None):
+ def create_debug_adaptor(self, env=None, launch=True, port=None, unix_socket=None):
"""Create the Visual Studio Code debug adaptor"""
self.assertTrue(
is_exe(self.lldbDAPExec), "lldb-dap must exist and be executable"
@@ -21,14 +21,28 @@ def create_debug_adaptor(self, lldbDAPEnv=None):
log_file_path = self.getBuildArtifact("dap.txt")
self.dap_server = dap_server.DebugAdaptorServer(
executable=self.lldbDAPExec,
+ launch=launch,
+ port=port,
+ unix_socket=unix_socket,
init_commands=self.setUpCommands(),
log_file=log_file_path,
- env=lldbDAPEnv,
+ env=env,
)
- def build_and_create_debug_adaptor(self, lldbDAPEnv=None):
+ def build_and_create_debug_adaptor(
+ self,
+ lldbDAPEnv=None,
+ lldbDAPLaunch=True,
+ lldbDAPPort=None,
+ lldbDAPUnixSocket=None,
+ ):
self.build()
- self.create_debug_adaptor(lldbDAPEnv)
+ self.create_debug_adaptor(
+ env=lldbDAPEnv,
+ launch=lldbDAPLaunch,
+ port=lldbDAPPort,
+ unix_socket=lldbDAPUnixSocket,
+ )
def set_source_breakpoints(self, source_path, lines, data=None):
"""Sets source breakpoints and returns an array of strings containing
@@ -475,11 +489,19 @@ def build_and_launch(
customThreadFormat=None,
launchCommands=None,
expectFailure=False,
+ lldbDAPPort=None,
+ lldbDAPUnixSocket=None,
+ lldbDAPLaunch=True,
):
"""Build the default Makefile target, create the DAP debug adaptor,
and launch the process.
"""
- self.build_and_create_debug_adaptor(lldbDAPEnv)
+ self.build_and_create_debug_adaptor(
+ lldbDAPEnv=lldbDAPEnv,
+ lldbDAPLaunch=lldbDAPLaunch,
+ lldbDAPPort=lldbDAPPort,
+ lldbDAPUnixSocket=lldbDAPUnixSocket,
+ )
self.assertTrue(os.path.exists(program), "executable must exist")
return self.launch(
diff --git a/lldb/test/API/tools/lldb-dap/server/Makefile b/lldb/test/API/tools/lldb-dap/server/Makefile
new file mode 100644
index 00000000000000..10495940055b63
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/server/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
diff --git a/lldb/test/API/tools/lldb-dap/server/TestDAP_server.py b/lldb/test/API/tools/lldb-dap/server/TestDAP_server.py
new file mode 100644
index 00000000000000..46b992a77a4815
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/server/TestDAP_server.py
@@ -0,0 +1,66 @@
+"""
+Test lldb-dap server integration.
+"""
+
+import os
+import tempfile
+
+import dap_server
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+import lldbdap_testcase
+
+
+class TestDAP_server(lldbdap_testcase.DAPTestCaseBase):
+ def do_test_server(self, port=None, unix_socket=None):
+ log_file_path = self.getBuildArtifact("dap.txt")
+ server = dap_server.DebugAdaptorServer.launch(
+ self.lldbDAPExec, port=port, unix_socket=unix_socket, log_file=log_file_path
+ )
+
+ def cleanup():
+ server.terminate()
+ server.wait()
+
+ self.addTearDownHook(cleanup)
+
+ self.build()
+ program = self.getBuildArtifact("a.out")
+ source = "main.c"
+ breakpoint_line = line_number(source, "// breakpoint")
+
+ # Initial connection over the port.
+ self.create_debug_adaptor(launch=False, port=port, unix_socket=unix_socket)
+ self.launch(
+ program,
+ disconnectAutomatically=False,
+ )
+ self.set_source_breakpoints(source, [breakpoint_line])
+ self.continue_to_next_stop()
+ self.continue_to_exit()
+ output = self.get_stdout()
+ self.assertEquals(output, "hello world!\r\n")
+ self.dap_server.request_disconnect()
+
+ # Second connection over the port.
+ self.create_debug_adaptor(launch=False, port=port, unix_socket=unix_socket)
+ self.launch(program)
+ self.set_source_breakpoints(source, [breakpoint_line])
+ self.continue_to_next_stop()
+ self.continue_to_exit()
+ output = self.get_stdout()
+ self.assertEquals(output, "hello world!\r\n")
+
+ def test_server_port(self):
+ """
+ Test launching a binary with a lldb-dap in server mode on a specific port.
+ """
+ port = pickrandomport()
+ self.do_test_server(port=port)
+
+ def test_server_unix_socket(self):
+ """
+ Test launching a binary with a lldb-dap in server mode on a unix socket.
+ """
+ dir = tempfile.gettempdir()
+ self.do_test_server(unix_socket=dir + "/dap-connection-" + str(os.getpid()))
diff --git a/lldb/test/API/tools/lldb-dap/server/main.c b/lldb/test/API/tools/lldb-dap/server/main.c
new file mode 100644
index 00000000000000..9a6326f3b57d45
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/server/main.c
@@ -0,0 +1,6 @@
+#include <stdio.h>
+
+int main(int argc, char const *argv[]) {
+ printf("hello world!\n"); // breakpoint 1
+ return 0;
+}
\ No newline at end of file
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 35250d9eef608a..8998036fbedf6b 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -32,10 +32,11 @@ using namespace lldb_dap;
namespace lldb_dap {
-DAP::DAP(llvm::StringRef path, ReplMode repl_mode)
+DAP::DAP(llvm::StringRef path, std::shared_ptr<std::ofstream> log,
+ ReplMode repl_mode, std::vector<std::string> pre_init_commands)
: debug_adaptor_path(path), broadcaster("lldb-dap"),
- exception_breakpoints(), focus_tid(LLDB_INVALID_THREAD_ID),
- stop_at_entry(false), is_attach(false),
+ log(log), exception_breakpoints(), pre_init_commands(pre_init_commands),
+ focus_tid(LLDB_INVALID_THREAD_ID), stop_at_entry(false), is_attach(false),
enable_auto_variable_summaries(false),
enable_synthetic_child_debugging(false),
display_extended_backtrace(false),
@@ -43,21 +44,7 @@ DAP::DAP(llvm::StringRef path, ReplMode repl_mode)
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) {
- const char *log_file_path = getenv("LLDBDAP_LOG");
-#if defined(_WIN32)
- // Windows opens stdout and stdin in text mode which converts \n to 13,10
- // while the value is just 10 on Darwin/Linux. Setting the file mode to binary
- // fixes this.
- int result = _setmode(fileno(stdout), _O_BINARY);
- assert(result);
- result = _setmode(fileno(stdin), _O_BINARY);
- UNUSED_IF_ASSERT_DISABLED(result);
- assert(result);
-#endif
- if (log_file_path)
- log.reset(new std::ofstream(log_file_path));
-}
+ reverse_request_seq(0), repl_mode(repl_mode) {}
DAP::~DAP() = default;
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index ae496236f13369..ec70da67edac14 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -9,36 +9,33 @@
#ifndef LLDB_TOOLS_LLDB_DAP_DAP_H
#define LLDB_TOOLS_LLDB_DAP_DAP_H
-#include <cstdio>
-#include <iosfwd>
-#include <map>
-#include <optional>
-#include <thread>
-
-#include "llvm/ADT/DenseMap.h"
-#include "llvm/ADT/DenseSet.h"
-#include "llvm/ADT/StringMap.h"
-#include "llvm/ADT/StringRef.h"
-#include "llvm/Support/JSON.h"
-#include "llvm/Support/Threading.h"
-#include "llvm/Support/raw_ostream.h"
-
+#include "ExceptionBreakpoint.h"
+#include "FunctionBreakpoint.h"
+#include "IOStream.h"
+#include "InstructionBreakpoint.h"
+#include "ProgressEvent.h"
+#include "SourceBreakpoint.h"
#include "lldb/API/SBAttachInfo.h"
#include "lldb/API/SBCommandInterpreter.h"
#include "lldb/API/SBCommandReturnObject.h"
#include "lldb/API/SBDebugger.h"
#include "lldb/API/SBEvent.h"
+#include "lldb/API/SBFile.h"
#include "lldb/API/SBFormat.h"
#include "lldb/API/SBLaunchInfo.h"
#include "lldb/API/SBTarget.h"
#include "lldb/API/SBThread.h"
-
-#include "ExceptionBreakpoint.h"
-#include "FunctionBreakpoint.h"
-#include "IOStream.h"
-#include "InstructionBreakpoint.h"
-#include "ProgressEvent.h"
-#include "SourceBreakpoint.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/Threading.h"
+#include "llvm/Support/raw_ostream.h"
+#include <iosfwd>
+#include <map>
+#include <optional>
+#include <thread>
#define VARREF_LOCALS (int64_t)1
#define VARREF_GLOBALS (int64_t)2
@@ -140,13 +137,16 @@ struct DAP {
llvm::StringRef debug_adaptor_path;
InputStream input;
OutputStream output;
+ lldb::SBFile in;
+ lldb::SBFile out;
+ lldb::SBFile err;
lldb::SBDebugger debugger;
lldb::SBTarget target;
Variables variables;
lldb::SBBroadcaster broadcaster;
std::thread event_thread;
std::thread progress_event_thread;
- std::unique_ptr<std::ofstream> log;
+ std::shared_ptr<std::ofstream> log;
llvm::StringMap<SourceBreakpointMap> source_breakpoints;
FunctionBreakpointMap function_breakpoints;
InstructionBreakpointMap instruction_breakpoints;
@@ -198,10 +198,14 @@ struct DAP {
// will contain that expression.
std::string last_nonempty_var_expression;
- DAP(llvm::StringRef path, ReplMode repl_mode);
+ DAP(llvm::StringRef path, std::shared_ptr<std::ofstream> log,
+ ReplMode repl_mode, std::vector<std::string> pre_init_commands);
~DAP();
+
+ DAP() = delete;
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);
diff --git a/lldb/tools/lldb-dap/Options.td b/lldb/tools/lldb-dap/Options.td
index d7b4a065abec01..dfca79b2884ac8 100644
--- a/lldb/tools/lldb-dap/Options.td
+++ b/lldb/tools/lldb-dap/Options.td
@@ -22,8 +22,17 @@ def port: S<"port">,
HelpText<"Communicate with the lldb-dap tool over the defined port.">;
def: Separate<["-"], "p">,
Alias<port>,
+ MetaVarName<"<port>">,
HelpText<"Alias for --port">;
+def unix_socket: S<"unix-socket">,
+ MetaVarName<"<path>">,
+ HelpText<"Communicate with the lldb-dap tool over the unix socket or named pipe.">;
+def: Separate<["-"], "u">,
+ Alias<unix_socket>,
+ MetaVarName<"<path>">,
+ HelpText<"Alias for --unix_socket">;
+
def launch_target: S<"launch-target">,
MetaVarName<"<target>">,
HelpText<"Launch a target for the launchInTerminal request. Any argument "
diff --git a/lldb/tools/lldb-dap/OutputRedirector.cpp b/lldb/tools/lldb-dap/OutputRedirector.cpp
index 2c2f49569869b4..0b725e1901b9fd 100644
--- a/lldb/tools/lldb-dap/OutputRedirector.cpp
+++ b/lldb/tools/lldb-dap/OutputRedirector.cpp
@@ -21,7 +21,8 @@ using namespace llvm;
namespace lldb_dap {
-Error RedirectFd(int fd, std::function<void(llvm::StringRef)> callback) {
+Expected<int> RedirectFd(int fd,
+ std::function<void(llvm::StringRef)> callback) {
int new_fd[2];
#if defined(_WIN32)
if (_pipe(new_fd, 4096, O_TEXT) == -1) {
@@ -34,7 +35,7 @@ Error RedirectFd(int fd, std::function<void(llvm::StringRef)> callback) {
strerror(error));
}
- if (dup2(new_fd[1], fd) == -1) {
+ if (fd != -1 && dup2(new_fd[1], fd) == -1) {
int error = errno;
return createStringError(inconvertibleErrorCode(),
"Couldn't override the fd %d. %s", fd,
@@ -57,7 +58,7 @@ Error RedirectFd(int fd, std::function<void(llvm::StringRef)> callback) {
}
});
t.detach();
- return Error::success();
+ return new_fd[1];
}
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/OutputRedirector.h b/lldb/tools/lldb-dap/OutputRedirector.h
index e26d1648b104f9..418b8bac102c7f 100644
--- a/lldb/tools/lldb-dap/OutputRedirector.h
+++ b/lldb/tools/lldb-dap/OutputRedirector.h
@@ -16,10 +16,16 @@ namespace lldb_dap {
/// Redirects the output of a given file descriptor to a callback.
///
+/// \param[in] fd
+/// Either -1 or the fd duplicate into the new handle.
+///
+/// \param[in] callback
+/// A callback invoked each time the file is written.
+///
/// \return
-/// \a Error::success if the redirection was set up correctly, or an error
-/// otherwise.
-llvm::Error RedirectFd(int fd, std::function<void(llvm::StringRef)> callback);
+/// A new file handle for the output.
+llvm::Expected<int> RedirectFd(int fd,
+ std::function<void(llvm::StringRef)> callback);
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index 3bfc578806021e..51197587ffccec 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -14,6 +14,7 @@
#include "RunInTerminal.h"
#include "Watchpoint.h"
#include "lldb/API/SBDeclaration.h"
+#include "lldb/API/SBFile.h"
#include "lldb/API/SBInstruction.h"
#include "lldb/API/SBListener.h"
#include "lldb/API/SBMemoryRegionInfo.h"
@@ -66,6 +67,7 @@
#else
#include <netinet/in.h>
#include <sys/socket.h>
+#include <sys/un.h>
#include <unistd.h>
#endif
@@ -116,6 +118,8 @@ enum LaunchMethod { Launch, Attach, AttachForSuspendedLaunch };
/// Page size used for reporting addtional frames in the 'stackTrace' request.
constexpr int StackPageSize = 20;
+void RegisterRequestCallbacks(DAP &dap);
+
/// Prints a welcome message on the editor if the preprocessor variable
/// LLDB_DAP_WELCOME_MESSAGE is defined.
static void PrintWelcomeMessage(DAP &dap) {
@@ -137,42 +141,232 @@ lldb::SBValueList *GetTopLevelScope(DAP &dap, int64_t variablesReference) {
}
}
-SOCKET AcceptConnection(DAP &dap, int portno) {
- // Accept a socket connection from any host on "portno".
- SOCKET newsockfd = -1;
- struct sockaddr_in serv_addr, cli_addr;
+/// Redirect stdout and stderr fo the IDE's console output.
+///
+/// Errors in this operation will be printed to the log file and the IDE's
+/// console output as well.
+///
+/// \return
+/// A fd pointing to the original stdout.
+void SetupRedirection(DAP &dap, int stdoutfd = -1, int stderrfd = -1) {
+ auto output_callback_stderr = [&dap](llvm::StringRef data) {
+ dap.SendOutput(OutputType::Stderr, data);
+ };
+ auto output_callback_stdout = [&dap](llvm::StringRef data) {
+ dap.SendOutput(OutputType::Stdout, data);
+ };
+
+ llvm::Expected<int> new_stdout_fd =
+ RedirectFd(stdoutfd, output_callback_stdout);
+ if (auto err = new_stdout_fd.takeError()) {
+ std::string error_message = llvm::toString(std::move(err));
+ if (dap.log)
+ *dap.log << error_message << std::endl;
+ output_callback_stderr(error_message);
+ }
+ dap.out = lldb::SBFile(new_stdout_fd.get(), "w", false);
+
+ llvm::Expected<int> new_stderr_fd =
+ RedirectFd(stderrfd, output_callback_stderr);
+ if (auto err = new_stderr_fd.takeError()) {
+ std::string error_message = llvm::toString(std::move(err));
+ if (dap.log)
+ *dap.log << error_message << std::endl;
+ output_callback_stderr(error_message);
+ }
+ dap.err = lldb::SBFile(new_stderr_fd.get(), "w", false);
+}
+
+void HandleClien...
[truncated]
|
lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
Outdated
Show resolved
Hide resolved
| elif unix_socket: | ||
| s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) | ||
| s.connect(unix_socket) | ||
| DebugCommunication.__init__( | ||
| self, s.makefile("rb"), s.makefile("wb"), init_commands, log_file | ||
| ) |
There was a problem hiding this comment.
What's the advantage in supporting both kinds of sockets. Could we just do with one?
There was a problem hiding this comment.
VS Code has helpers for running over stdout/stderr (https://code.visualstudio.com/api/references/vscode-api#DebugAdapterExecutable), a unix socket / named pipe (https://code.visualstudio.com/api/references/vscode-api#DebugAdapterNamedPipeServer) or tcp host:port (https://code.visualstudio.com/api/references/vscode-api#DebugAdapterServer).
Since lldb-dap had support for a port mode already, I didn't want to remove it. I added support for both protocols since it gives flexibility on how lldb-dap is used.
There was a problem hiding this comment.
Supporting unix sockets only would not be good, as then this wouldn't work on windows. However, TCP/IP works everywhere, so it might be sufficient to only support those? I don't think its my place to tell you you shouldn't implement that, but I also don't want you to think that's a prerequisite for this feature (making sure it possible to add the functionality later is nice, but I think we could forego the actual implementation until someone actually needs it).
There was a problem hiding this comment.
This should work with both tcp/unix sockets by parsing the --connection flag.
There was a problem hiding this comment.
The patch is still a bit larger than I'd like. I've tried to highlight a few things that could be split off into separate PRs, but I think the biggest question now (not really for you, but for lldb-dap maintainers) is whether this could be using the socket abstractions already present in lldb (instead of building your own). If the answer to that is "yes", then switching to those should probably be a separate patch as well.
lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
Outdated
Show resolved
Hide resolved
lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
Outdated
Show resolved
Hide resolved
lldb/tools/lldb-dap/DAP.cpp
Outdated
| RedirectFd(out_fd, [this](llvm::StringRef data) { | ||
| SendOutput(OutputType::Stdout, data); | ||
| }); |
There was a problem hiding this comment.
AFAICT, nothing ensures these callbacks don't run after the DAP object was terminated. This wasn't a big deal when the process was serving only one connection (although making the object stack-allocated in #116272 is kinda problematic), but I think this needs to be sorted out (ideally as a separate patch) for the multi-connection version.
It also doesn't look like the pipes created here get cleaned up anywhere.
There was a problem hiding this comment.
Yeah, this is maybe the trickiest part of all. The redirection was introduced to prevent LLDB proper from printing messages to stderr and unexpectedly causing the DAP client to abort. And indeed, there are many parts of LLDB that print directly to stderr upon unusual situations (dwarf parsing is one of them IIRC). The ideal solution is to create an actual SB API that can handle these unusual messages and print to stderr is simply the default behavior. You could also do that.
If you don't do such refactor, something that you should consider when using multiple connections simultaneously, is that it might be tricky to know which of those stderr messages correspond to which DAP client. You might decide to multicast them, which is better than sending the output to the wrong client. Silencing the messages altogether might not be a good idea.
In any case, you can think of the redirection as something that needs to be set up only once per process.
There was a problem hiding this comment.
Adjusted the RedirectFD to clean up once the DAP object is not used anymore. This includes making a best effort to use dup2 to revert the FD redirects.
As for stderr handling, in server mode it no longer setups a redirect on stderr to the current connection. When I allocate the SBDebugger I also configure the OutputFile and ErrorFile handles to pipes that forward to that specific connection.
This does mean that if we are running in a server mode and some part of lldb (or llvm/clang/etc.) print directly to stderr then it wouldn't be associated with the debug session, but I think that would be an issue for other users of the SBDebugger API.
I know that Xcode also has a helper lldb-rpc-server that is a similar configuration to the lldb-dap server mode. When a debug session starts in Xcode it starts an lldb-rpc-server that hosts all the SBDebugger instances. So a single version of Xcode will have 1 instance of lldb-rpc-server running at a time and that instance is used to run each debug session for each Xcode window. But my overall point with this is that if there are parts of the stack that are not respecting the SBDebugger's OutputFile and ErrorFile it would also show up in Xcode.
There was a problem hiding this comment.
once the DAP object is not used anymore
I don't think you can really guarantee that while there are detached forwarding threads floating around. I think we need to shut those down first. The tricky part is doing that in cross-platform way, but since we're already all-in on lldbHost, I think we could/should use lldb's Pipe::ReadWithTimeout to implement polled reads (which check for a "should terminate flag" in between). Not the most elegant solution, but I it's probably the best we can do with what we have right now. One of these days I'm going to implement a MainLoop<->Pipe integration that can multiplex reads (and other things), but it's not very easy due to very different Pipe APIs on windows vs. unix.
I also believe this parts should be a separate patch, as there's a lot of subtleness involved.
There was a problem hiding this comment.
I've refactored the IO handling in #122783 to ensure the DAP instance + SBDebugger has its own dedicated file handles for the DAP session.
| elif unix_socket: | ||
| s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) | ||
| s.connect(unix_socket) | ||
| DebugCommunication.__init__( | ||
| self, s.makefile("rb"), s.makefile("wb"), init_commands, log_file | ||
| ) |
There was a problem hiding this comment.
Supporting unix sockets only would not be good, as then this wouldn't work on windows. However, TCP/IP works everywhere, so it might be sufficient to only support those? I don't think its my place to tell you you shouldn't implement that, but I also don't want you to think that's a prerequisite for this feature (making sure it possible to add the functionality later is nice, but I think we could forego the actual implementation until someone actually needs it).
I can split this into smaller patches. As far as the socket abstraction, at the moment lldb-dap doesn't use much (if anything) from lldb outside of the API folder. I saw the |
Thanks.
I am aware of that, but I don't think that situation is sustainable in the long run. When lldb-dap was first added, I believe it only supported talking over stdio, so it had like ~zero amount of socket code, and so this kind of purist approach did not hurt (much). However, its getting quite complex now, and I think copying/reimplementing every abstraction it needs is going to come back and bite us. FWIW, the MainLoop class is self-contained, and might actually be useful if you wanted to support things like listening on IPv4 and v6 sockets simultaneously. And IOObject is just a abstract base, so it also doesn't shouldn't pull in much. We're also making sure (it took a lot of effort to reach that point) that the Host library as a whole does not depend on the rest of lldb. |
ashgti
force-pushed
the
lldb-dap-server-mode
branch
3 times, most recently
from
14080f6 to
35035cd
Compare
|
✅ With the latest revision this PR passed the Python code formatter. |
ashgti
force-pushed
the
lldb-dap-server-mode
branch
3 times, most recently
from
d1ff196 to
43aa4d9
Compare
Please, do that. Multithreading and servers and stuff are very tricky to get right, and it'd be a lot easier to review this if it was in smaller chunks. I think there's a lot of prepwork so that the code is prepared to handle multiple connections. That stuff can land without actually supporting multiple connections. |
| @@ -118,6 +124,15 @@ std::string TCPSocket::GetRemoteConnectionURI() const { | |||
| return ""; | |||
| } | |||
|
|
|||
| std::string TCPSocket::GetListeningConnectionURI() const { | |||
There was a problem hiding this comment.
Let's put these into a separate patch (with a test case). I also believe these should return a vector<string> as the listening socket can have more than one address. You can ignore the additional addresses in the caller if you don't need them.
There was a problem hiding this comment.
Split this into its own PR #118330
| std::vector<SocketAddress> addresses = | ||
| SocketAddress::GetAddressInfo(host_port->hostname.c_str(), nullptr, | ||
| AF_UNSPEC, SOCK_STREAM, IPPROTO_TCP); |
There was a problem hiding this comment.
It looks like you reformated the whole file.That's usually not a good idea as it produces spurious diffs like this.
lldb/tools/lldb-dap/DAP.cpp
Outdated
| @@ -904,11 +928,14 @@ bool StartDebuggingRequestHandler::DoExecute( | |||
| "startDebugging", | |||
| llvm::json::Object{{"request", request}, | |||
| {"configuration", std::move(*configuration)}}, | |||
| [](llvm::Expected<llvm::json::Value> value) { | |||
| [](auto dap, auto value) { | |||
There was a problem hiding this comment.
llvm policy is approximately to use auto only when the type is obvious from the context. That definitely isn't the case here.
I'm pretty sure this file was written by someone unaware of that policy, but let's not make it worse. And this is definitely making it worse. llvm::Expected can look very similar to std::optional, but it has a very important difference (the error must be checked), so it's very important to make the type obvious.
There was a problem hiding this comment.
Added the explicit types here instead of auto.
lldb/tools/lldb-dap/lldb-dap.cpp
Outdated
| if (log) | ||
| *log << "configureIO failed: " << llvm::toStringWithoutConsuming(Err) | ||
| << "\n"; | ||
| std::cerr << "failed to configureIO: " << llvm::toString(std::move(Err)) |
There was a problem hiding this comment.
let's choose whether to use llvm::errs() or std::cerr.
cerr is effectively banned, so I'd prefer the former, but I could be convinced to stick to it if everything else uses that.
There was a problem hiding this comment.
I am using llvm::errs() instead for reporting errors when running in server mode.
There was a problem hiding this comment.
That's good but beware os << error does not "consume" (set the "checked" bit), on the llvm::Error object (which means it will trigger an assert in debug mode). You need to consume it somehow, e.g., via toString, or logAllUnhandledErrors (the latter works best when the entire "main" function is wrapped in something returning an error object
lldb/tools/lldb-dap/DAP.cpp
Outdated
| RedirectFd(out_fd, [this](llvm::StringRef data) { | ||
| SendOutput(OutputType::Stdout, data); | ||
| }); |
There was a problem hiding this comment.
once the DAP object is not used anymore
I don't think you can really guarantee that while there are detached forwarding threads floating around. I think we need to shut those down first. The tricky part is doing that in cross-platform way, but since we're already all-in on lldbHost, I think we could/should use lldb's Pipe::ReadWithTimeout to implement polled reads (which check for a "should terminate flag" in between). Not the most elegant solution, but I it's probably the best we can do with what we have right now. One of these days I'm going to implement a MainLoop<->Pipe integration that can multiplex reads (and other things), but it's not very easy due to very different Pipe APIs on windows vs. unix.
I also believe this parts should be a separate patch, as there's a lot of subtleness involved.
ashgti
force-pushed
the
lldb-dap-server-mode
branch
from
d08ab90 to
31ead1b
Compare
ashgti
force-pushed
the
lldb-dap-server-mode
branch
2 times, most recently
from
61fefbe to
f946fd7
Compare
There was a problem hiding this comment.
Apart from the comment, this looks good to me. @vogelsgesang, @walter-erquinigo, any thoughts from you?
|
@walter-erquinigo Any updates on allowing multiple connections in server mode? |
|
@JDevlieghere Any thoughts on the server mode? |
| "'connect://[host]:port', got '%s'.", | ||
| conn.str().c_str()); | ||
| } | ||
|
|
||
| int main(int argc, char *argv[]) { |
There was a problem hiding this comment.
the newly added logic seems correct, but would it be possible to extract out some of code into other functions? I'm afraid main is starting to get too big to be easily readable
There was a problem hiding this comment.
I moved the main connection handling loop to its own function, we can break things up further as well if you'd like
… connections. This adjusts the lldb-dap listening mode to accept multiple clients. Each client initializes a new instance of DAP and an associated `lldb::SBDebugger` instance. The listening mode is configured with the `--connection` option and supports listening on a port or a unix socket on supported platforms.
…errupt handler to disconnect active sessions.
…AP sessions have disconnected to coordinate shutdowns.
…nection on startup of the binary.
…lying reviewer suggestions.
ashgti
force-pushed
the
lldb-dap-server-mode
branch
from
625083b to
43097eb
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/6506 Here is the relevant piece of the build log for the reference |
|
#128278 Should mark the test as skipped on windows to unblock the CI. |
… connections. (llvm#116392) This adjusts the lldb-dap listening mode to accept multiple clients. Each client initializes a new instance of DAP and an associated `lldb::SBDebugger` instance. The listening mode is configured with the `--connection` option and supports listening on a port or a unix socket on supported platforms. When running in server mode launch and attach performance should be improved by lldb sharing symbols for core libraries between debug sessions.
… connections. (llvm#116392) This adjusts the lldb-dap listening mode to accept multiple clients. Each client initializes a new instance of DAP and an associated `lldb::SBDebugger` instance. The listening mode is configured with the `--connection` option and supports listening on a port or a unix socket on supported platforms. When running in server mode launch and attach performance should be improved by lldb sharing symbols for core libraries between debug sessions.
… connections. (llvm#116392) This adjusts the lldb-dap listening mode to accept multiple clients. Each client initializes a new instance of DAP and an associated `lldb::SBDebugger` instance. The listening mode is configured with the `--connection` option and supports listening on a port or a unix socket on supported platforms. When running in server mode launch and attach performance should be improved by lldb sharing symbols for core libraries between debug sessions.
… connections. (llvm#116392) This adjusts the lldb-dap listening mode to accept multiple clients. Each client initializes a new instance of DAP and an associated `lldb::SBDebugger` instance. The listening mode is configured with the `--connection` option and supports listening on a port or a unix socket on supported platforms. When running in server mode launch and attach performance should be improved by lldb sharing symbols for core libraries between debug sessions.
… connections. (llvm#116392) This adjusts the lldb-dap listening mode to accept multiple clients. Each client initializes a new instance of DAP and an associated `lldb::SBDebugger` instance. The listening mode is configured with the `--connection` option and supports listening on a port or a unix socket on supported platforms. When running in server mode launch and attach performance should be improved by lldb sharing symbols for core libraries between debug sessions.
… connections. (llvm#116392) This adjusts the lldb-dap listening mode to accept multiple clients. Each client initializes a new instance of DAP and an associated `lldb::SBDebugger` instance. The listening mode is configured with the `--connection` option and supports listening on a port or a unix socket on supported platforms. When running in server mode launch and attach performance should be improved by lldb sharing symbols for core libraries between debug sessions.
… connections. (llvm#116392) This adjusts the lldb-dap listening mode to accept multiple clients. Each client initializes a new instance of DAP and an associated `lldb::SBDebugger` instance. The listening mode is configured with the `--connection` option and supports listening on a port or a unix socket on supported platforms. When running in server mode launch and attach performance should be improved by lldb sharing symbols for core libraries between debug sessions.
… connections. (llvm#116392) This adjusts the lldb-dap listening mode to accept multiple clients. Each client initializes a new instance of DAP and an associated `lldb::SBDebugger` instance. The listening mode is configured with the `--connection` option and supports listening on a port or a unix socket on supported platforms. When running in server mode launch and attach performance should be improved by lldb sharing symbols for core libraries between debug sessions.
This adjusts the lldb-dap listening mode to accept multiple clients.
Each client initializes a new instance of DAP and an associated
lldb::SBDebuggerinstance.The listening mode is configured with the
--connectionoption and supports listening on a port or a unix socket on supported platforms.