[lldb-dap] Refactoring lldb-dap to support running in a server mode, allowing multiple connections. #114881
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nnection, or a single object when not in server mode. This is a fairly large refactor of the lldb-dap to allow multiple DAP sessions from a single lldb-dap server. The majority of the changes are around the removal of the g_dap instance and instead requiring all functions to either accept a DAP instance as a parameter or refactoring some code to not directly rely on the DAP object. When running without a port, this should behavior the same as it did before. When running with a port, the server will now accept each new connection and run the debug session to completion. In this mode, the server will continue until interrupted.
Member
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesThis is a fairly large refactor of the lldb-dap to allow multiple DAP sessions from a single The majority of the changes are around the removal of the g_dap instance and instead requiring all functions to either accept a DAP instance as a parameter or refactoring some code to not directly rely on the DAP object. When running without a port, this should behavior the same as it did before. When running with a port or unix socket, the server will now accept each new connection and run the debug session to completion. In this mode, the server will continue until interrupted. This allows you to start lldb-dap in a sever mode and reuse the server between debug sessions. This should improve load times by allowing better caching between debug sessions within lldb. Patch is 206.56 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/114881.diff 36 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index 8884ef5933ada8..c143b7ba37af22 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -37,6 +37,7 @@
import re
import shutil
import signal
+import socket
from subprocess import *
import sys
import time
@@ -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..31819328040aca 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,32 @@ class DebugAdaptorServer(DebugCommunication):
def __init__(
self,
executable=None,
- port=None,
+ launch=True,
+ connect=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, connect=connect, log_file=log_file, env=env
)
+
+ if connect:
+ if isinstance(connect, str) and connect.startswith("/"):
+ s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+ s.connect(connect)
+ else:
+ port = int(connect)
+ s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+ s.connect(("127.0.0.1", port))
DebugCommunication.__init__(
- self, self.process.stdout, self.process.stdin, init_commands, log_file
+ self, s.makefile("rb"), s.makefile("wb"), init_commands, log_file
)
- elif port is not None:
- s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
- s.connect(("127.0.0.1", port))
+ else:
DebugCommunication.__init__(
- self, s.makefile("r"), s.makefile("w"), init_commands
+ self, self.process.stdout, self.process.stdin, init_commands, log_file
)
def get_pid(self):
@@ -1191,11 +1189,45 @@ def get_pid(self):
def terminate(self):
super(DebugAdaptorServer, self).terminate()
- if self.process is not None:
+ if self.process:
self.process.terminate()
self.process.wait()
self.process = None
+ @classmethod
+ def launch(
+ cls, executable: str, /, connect=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 connect:
+ if isinstance(connect, str) and connect.startswith("/"):
+ args.append("--unix-socket")
+ args.append(connect)
+ else:
+ args.append("--port")
+ args.append(str(connect))
+
+ proc = subprocess.Popen(
+ args,
+ stdin=subprocess.PIPE,
+ stdout=subprocess.PIPE,
+ stderr=sys.stdout,
+ env=adaptor_env,
+ )
+
+ if connect:
+ # Wait for the server to startup.
+ time.sleep(0.1)
+
+ return proc
+
def attach_options_specified(options):
if options.pid is not None:
@@ -1271,9 +1303,9 @@ def main():
)
parser.add_option(
- "--vscode",
+ "--dap",
type="string",
- dest="vscode_path",
+ dest="dap",
help=(
"The path to the command line program that implements the "
"Visual Studio Code Debug Adaptor protocol."
@@ -1349,10 +1381,9 @@ def main():
)
parser.add_option(
- "--port",
- type="int",
- dest="port",
- help="Attach a socket to a port instead of using STDIN for VSCode",
+ "--connect",
+ dest="connect",
+ help="Attach a socket to the connection ('<port>' or '<path>') instead of using STDIN for VSCode",
default=None,
)
@@ -1498,15 +1529,15 @@ def main():
(options, args) = parser.parse_args(sys.argv[1:])
- if options.vscode_path is None and options.port is None:
+ if options.dap is None and options.connect is None:
print(
"error: must either specify a path to a Visual Studio Code "
- "Debug Adaptor vscode executable path using the --vscode "
+ "Debug Adaptor vscode executable path using the --dap "
"option, or a port to attach to for an existing lldb-dap "
- "using the --port option"
+ "using the --connect option"
)
return
- dbg = DebugAdaptorServer(executable=options.vscode_path, port=options.port)
+ dbg = DebugAdaptorServer(executable=options.dap, port=options.connect)
if options.debug:
raw_input('Waiting for debugger to attach pid "%i"' % (dbg.get_pid()))
if options.replay:
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..39d200a2ac566e 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, connect=None):
"""Create the Visual Studio Code debug adaptor"""
self.assertTrue(
is_exe(self.lldbDAPExec), "lldb-dap must exist and be executable"
@@ -21,14 +21,20 @@ 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,
+ connect=connect,
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, lldbDAPConnect=None
+ ):
self.build()
- self.create_debug_adaptor(lldbDAPEnv)
+ self.create_debug_adaptor(
+ lldbDAPEnv, launch=lldbDAPLaunch, connect=lldbDAPConnect
+ )
def set_source_breakpoints(self, source_path, lines, data=None):
"""Sets source breakpoints and returns an array of strings containing
@@ -475,11 +481,13 @@ def build_and_launch(
customThreadFormat=None,
launchCommands=None,
expectFailure=False,
+ lldbDAPConnect=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, lldbDAPLaunch, lldbDAPConnect)
self.assertTrue(os.path.exists(program), "executable must exist")
return self.launch(
diff --git a/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_logpoints.py b/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_logpoints.py
index 78ceb7971112e5..ad7e69463614be 100644
--- a/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_logpoints.py
+++ b/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_logpoints.py
@@ -3,11 +3,8 @@
"""
-import dap_server
-import shutil
from lldbsuite.test.decorators import *
from lldbsuite.test.lldbtest import *
-from lldbsuite.test import lldbutil
import lldbdap_testcase
import os
diff --git a/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py b/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py
index 7898d01457afc4..6135952bd176b9 100644
--- a/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py
+++ b/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py
@@ -2,12 +2,9 @@
Test lldb-dap setBreakpoints request
"""
-import dap_server
from lldbsuite.test.decorators import *
from lldbsuite.test.lldbtest import *
-from lldbsuite.test import lldbutil
import lldbdap_testcase
-import time
import os
import re
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..ae7094b096c4d7
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/server/TestDAP_server.py
@@ -0,0 +1,67 @@
+"""
+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, connect):
+ log_file_path = self.getBuildArtifact("dap.txt")
+ print("connect", connect)
+ server = dap_server.DebugAdaptorServer.launch(
+ self.lldbDAPExec, connect=connect, 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, connect=connect)
+ 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, connect=connect)
+ 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)
+
+ 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(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..c3599057621276
--- /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;
+}
diff --git a/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py b/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
index f4f30b6677e539..cfe61a8c0a5c8c 100644
--- a/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
+++ b/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
@@ -4,9 +4,7 @@
import os
-import dap_server
import lldbdap_testcase
-from lldbsuite.test import lldbutil
from lldbsuite.test.decorators import *
from lldbsuite.test.lldbtest import *
@@ -173,7 +171,7 @@ def do_test_scopes_variables_setVariable_evaluate(
"value": "1",
},
"declaration": {
- "equals": {"line": 12, "column": 14},
+ "equals": {"line": 15},
"contains": {"path": ["lldb-dap", "variables", "main.cpp"]},
},
},
@@ -194,7 +192,12 @@ def do_test_scopes_variables_setVariable_evaluate(
"buffer": {"children": buffer_children},
},
},
- "x": {"equals": {"type": "int"}},
+ "x": {
+ "equals": {
+ "type": "int",
+ "value": "0",
+ }
+ },
}
if enableAutoVariableSummaries:
verify_locals["pt"]["$__lldb_extensions"] = {
@@ -214,7 +217,9 @@ def do_test_scopes_variables_setVariable_evaluate(
verify_globals["::g_global"] = g_global
varref_dict = {}
+ print("locals", locals)
self.verify_variables(verify_locals, locals, varref_dict)
+ print("globals", globals)
self.verify_variables(verify_globals, globals, varref_dict)
# pprint.PrettyPrinter(indent=4).pprint(varref_dict)
# We need to test the functionality of the "variables" request as it
@@ -341,9 +346,9 @@ def do_test_scopes_variables_setVariable_evaluate(
verify_locals["argc"]["equals"]["value"] = "123"
verify_locals["pt"]["children"]["x"]["equals"]["value"] = "111"
- verify_locals["x @ main.cpp:17"] = {"equals": {"type": "int", "value": "89"}}
- verify_locals["x @ main.cpp:19"] = {"equals": {"type": "int", "value": "42"}}
- verify_locals["x @ main.cpp:21"] = {"equals": {"type": "int", "value": "72"}}
+ verify_locals["x @ main.cpp:20"] = {"equals": {"type": "int", "value": "89"}}
+ verify_locals["x @ main.cpp:22"] = {"equals": {"type": "int", "value": "42"}}
+ verify_locals["x @ main.cpp:24"] = {"equals": {"type": "int", "value": "72"}}
self.verify_variables(verify_locals, self.dap_server.get_local_variables())
@@ -354,13 +359,13 @@ def do_test_scopes_variables_setVariable_evaluate(
)
self.assertTrue(
- self.dap_server.request_setVariable(1, "x @ main.cpp:17", 17)["success"]
+ self.dap_server.request_setVariable(1, "x @ main.cpp:20", 17)["success"]
)
self.assertTrue(
- self.dap_server.request_setVariable(1, "x @ main.cpp:19", 19)["success"]
+ self.dap_server.request_setVariable(1, "x @ main.cpp:22", 19)["success"]
)
self.assertTrue(
- self.dap_server.request_setVariable(1, "x @ main.cpp:21", 21)["success"]
+ self.dap_server.request_setVariable(1, "x @ main.cpp:24", 21)["success"]
)
# The following should have no effect
@@ -370,15 +375,15 @@ def do_test_scopes_variables_setVariable_evaluate(
]
)
- verify_locals["x @ main.cpp:17"]["equals"]["value"] = "17"
- verify_locals["x @ main.cpp:19"]["equals"]["value"] = "19"
- verify_locals["x @ main.cpp:21"]["equals"]["value"] = "21"
+ verify_locals["x @ main.cpp:20"]["equals"]["value"] = "17"
+ verify_locals["x @ main.cpp:22"]["equals"]["value"] = "19"
+ verify_locals["x @ main.cpp:24"]["equals"]["value"] = "21"
self.verify_variables(verify_locals, self.dap_server.get_local_variables())
# The plain x variable shold refer to the innermost x
self.assertTrue(self.dap_server.request_setVariable(1, "x", 22)["success"])
- verify_locals["x @ main.cpp:21"]["equals"]["value"] = "22"
+ verify_locals["x @ main.cpp:24"]["equals"]["value"] = "22"
self.verify_variables(verify_locals, self.dap_server.get_local_variables())
@@ -395,9 +400,9 @@ def do_test_scopes_variables_setVariable_evaluate(
names = [var["name"] for var in locals]
# The first shadowed x shouldn't have a suffix anymore
verify_locals["x"] = {"equals": {"type": "int", "value": "17"}}
- self.assertNotIn("x @ main.cpp:17", names)
- self.assertNotIn("x @ main.cpp:19", names)
- self.assertNotIn("x @ main.cpp:21", names)
+ self.assertNotIn("x @ main.cpp:20", names)
+ self.assertNotIn("x @ main.cpp:22", names)
+ self.assertNotIn("x @ main.cpp:24", names)
self.verify_variables(verify_locals, locals)
diff --git a/lldb/test/API/tools/lldb-dap/variables/children/TestDAP_variables_children.py b/lldb/test/API/tools/lldb-dap/variables/children/TestDAP_variables_children.py
index 805e88ddf8f70b..9c18417d740e2b 100644
--- a/lldb/test/API/tools/lldb-dap/variables/children/TestDAP_variables_children.py
+++ b/lldb/test/API/tools/lldb-dap/variables/children/TestDAP_variables_children.py
@@ -1,8 +1,4 @@
-import os
-
-import dap_server
import lldbdap_testcase
-from lldbsuite.test import lldbutil
from lldbsuite.test.decorators import *
from lldbsuite.test.lldbtest import *
diff --git a/lldb/test/API/tools/lldb-dap/variables/main.cpp b/lldb/test/API/tools/lldb-dap/variables/main.cpp
index 3557067ed9ce13..77a240a6d64359 100644
--- a/lldb/test/API/tools/lldb-dap/variables/main.cpp
+++ b/lldb/test/API/tools/lldb-dap/variables/main.cpp
@@ -1,3 +1,4 @@
+#include <vector>
#define BUFFER_SIZE 16
struct PointType {
@@ -5,10 +6,12 @@ struct PointType {
int y;
int buffer[BUFFER_SIZE];
};
-#include <vector>
+
+int test_indexedVariables();
+
int g_global = 123;
static int s_global = 234;
-int test_indexedVariables();
+
int main(int argc, char const *argv[]) {
static float s_local = 2.25;
PointType pt = {11, 22, {0}};
diff --git a/lldb/tools/lldb-dap/Breakpoint.cpp b/lldb/tools/lldb-dap/Breakpoint.cpp
index 9ea7a42ca85a1e..aee2f87e2cc23e 100644
--- a/lldb/tools/lldb-dap/Breakpoint.cpp
+++ b/lldb/tools/lldb-dap/Breakpoint.cpp
@@ -7,11 +7,13 @@
//===----------------------------------------------------------------------===//
#include "Breakpoint.h"
-#include "DAP.h"
-#include "JSONUtils.h"
+
#include "lldb/API/SBBreakpointLocation.h"
#include "llvm/ADT/StringExtras.h"
+#include "DAP.h"
+#include "JSONUtils.h"
+
using namespace lldb_dap;
void Breakpoint::SetCondition() { bp.SetCondition(condition.c_str()); }
@@ -51,7 +53,7 @@ void Breakpoint::CreateJsonObject(llvm::json::Object &object) {
if (bp_addr.IsValid()) {
std::string formatted_addr =
- "0x" + llvm::utohexstr(bp_addr.GetLoadAddress(g_dap.target));
+ "0x" + llvm::utohexstr(bp_addr.GetLoadAddress(bp.GetTarget()));
object.try_emplace("instructionReference", formatted_addr);
auto line_entry = bp_addr.GetLineEntry();
const auto line = line_entry.GetLine();
diff --git a/lldb/tools/lldb-dap/Breakpoint.h b/lldb/tools/lldb-dap/Breakpoint.h
index ee9d3736d6190f..add4ff4da2a9bb 100644
--- a/lldb/tools/lldb-dap/Breakpoint.h
+++ b/lldb/tools/lldb-dap/Breakpoint.h
@@ -9,18 +9,19 @@
#ifndef LLDB_TOOLS_LLDB_DAP_BREAKPOINT_H
#define LLDB_TOOLS_LLDB_DAP_BREAKPOINT_H
-#include "BreakpointBase.h"
#include "lldb...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
vogelsgesang
reviewed
Nov 4, 2024
lldb/tools/lldb-dap/BreakpointBase.h
Outdated
Comment on lines
30
to
31
| BreakpointBase(DAP *d) : dap(d) {} | ||
| BreakpointBase(DAP *d, const llvm::json::Object &obj); |
Member
There was a problem hiding this comment.
afaik, those are not allowed to be nullptrs, are they?
Suggested change
| BreakpointBase(DAP *d) : dap(d) {} | |
| BreakpointBase(DAP *d, const llvm::json::Object &obj); | |
| BreakpointBase(DAP &d) : dap(d) {} | |
| BreakpointBase(DAP &d, const llvm::json::Object &obj); |
Contributor
Author
There was a problem hiding this comment.
I initially used DAP& but I couldn't copy the a Breakpoint as a result. I switched from the pointer to a reference and corrected usage of the breakpoint types to make sure we either construct entries in place or move them correctly.
| } | ||
|
|
||
| void SourceBreakpoint::SetBreakpoint(const llvm::StringRef source_path) { | ||
| if (!dap) |
Contributor
Author
There was a problem hiding this comment.
It would happen if a breakpoint outlived the dap or was incorrectly initialized. However, I replaced the DAP* with a DAP&, which means this shouldn't happen anymore.
lldb/tools/lldb-dap/BreakpointBase.h
Outdated
|
|
||
| struct BreakpointBase { | ||
| // Associated DAP session. | ||
| DAP *dap; |
Member
There was a problem hiding this comment.
is this allowed to be a nullptr?
Do we use std::reference_wrapper in LLVM / lldb?
Member
There was a problem hiding this comment.
A weak_ptr would be a great use case for this, as the DAP class is the owner of all these instances. A reference wrapper would also work
Collaborator
There was a problem hiding this comment.
Although I don't consider myself an lldb-dap maintainer, I want to speak out, as strongly as I can, against the usage of shared/weak_ptr here. I have a couple of reasons for that:
- it's an LLDB tradition that can't be found almost anywhere else in llvm
- it encourages "vague ownership" semantics, where noone can be really sure whether some object still exists or not (so it handles the "not" case "just in case"). This in turn leads to a lot of dead/untested code. If what you say is true and the DAP object is the owner of this object, then I would say this is a reason to not use weak (or shared) pointers, as that the owned object should be able to assume that its owner is always around (and not worry about what to do with a possibly empty result of
weak_ptr::lock). LLDB sort of has an excuse for using shared_ptrs (a python scripting API, which means code outside our influence can hold on to lldb objects), but I don't think anything like that applies to lldb-dap. - it's impossible to distinguish a "potentially empty" shared/weak pointer from one that is always set. This is kind of similar to the previous one, but without the temporal aspect. Even if you ignore the possibility of the object going away mid-flight, you still can't enforce that it is always there to start with. With raw pointers/reference, you can.
- Makes it harder to maintain const correctness. It still possible (
shared_ptr<const T>is a thing), but it seems people don't usually bother to do that. Const correctness is of particular importance for multithreaded code. - All of this copying, locking and atomic operations inherent in shared pointers make things slower.
Contributor
Author
There was a problem hiding this comment.
I updated this to use a DAP& everywhere. The DAP objects should live as long as the associated connection and will be released once the connection closes.
Member
walter-erquinigo
left a comment
walter-erquinigo
left a comment
There was a problem hiding this comment.
Something that is not very clear to me is the following:
- Do you intend to have multiple DAP servers at once, or to have only one at a time but leave the main SBDebugger around?
- Is the orchestration something that you can add to the typescript code of the extension?
| NO_DEBUG_INFO_TESTCASE = True | ||
|
|
||
| def create_debug_adaptor(self, lldbDAPEnv=None): | ||
| def create_debug_adaptor(self, env=None, launch=True, connect=None): |
Member
There was a problem hiding this comment.
can you document what connect means? It's not obvious just from reading the signature
Contributor
Author
There was a problem hiding this comment.
Changed this to port and unix_socket instead of using the same variable for both.
lldb/tools/lldb-dap/Breakpoint.h
Outdated
Comment on lines
22
to
24
| Breakpoint(DAP *d) : BreakpointBase(d) {} | ||
| Breakpoint(DAP *d, const llvm::json::Object &obj) : BreakpointBase(d, obj) {} | ||
| Breakpoint(DAP *d, lldb::SBBreakpoint bp) : BreakpointBase(d), bp(bp) {} |
Member
There was a problem hiding this comment.
I'd prefer if these APIs operated on a weak_ptr<DAP> or a reference wrapper.
Contributor
Author
There was a problem hiding this comment.
Updated to take a DAP& instead of a pointer. This should help us ensure the breakpoints don't outlive the DAP.
lldb/tools/lldb-dap/BreakpointBase.h
Outdated
|
|
||
| struct BreakpointBase { | ||
| // Associated DAP session. | ||
| DAP *dap; |
Member
There was a problem hiding this comment.
A weak_ptr would be a great use case for this, as the DAP class is the owner of all these instances. A reference wrapper would also work
Collaborator
The obvious question after an opening like this is whether the patch can be split into multiple smaller changes. I would expect that at least the "remove g_dap global" part can be removed from the addition new functionality (listen for multiple connections). https://llvm.org/docs/DeveloperPolicy.html#incremental-development |
…le handles for sockets with "b" to write `bytes` instead of `str`. Fix lldb-dap.cpp handling of stdout.
ashgti
force-pushed
the
lldb-dap-server-mode
branch
from
7e118c6 to
d710b9b
Compare
|
✅ With the latest revision this PR passed the Python code formatter. |
Contributor
Author
I made https://discourse.llvm.org/t/rfc-lldb-dap-update-server-mode-to-allow-multiple-connections/82770 to describe this, but I've been running a single server and allowing multiple sessions to run on the server at once. So far, things have been working correctly for me.
Yes, I was planning on updating the typescript code as well to have the option to start a server and keep it around until the extension is closed. This would allow for better caching between debug sessions as well. |
Contributor
Author
I can split this up into more granular PRs, I wasn't sure if I should have this as a single PR or not, but I can break this up if that is preferable. |
Member
|
Yeah, splitting it would be a nice idea |
Member
|
agree, splitting up would be great. I could review the |
Contributor
Author
|
I will work on splitting this up into smaller pieces |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a fairly large refactor of the lldb-dap to allow multiple DAP sessions from a single
lldb-dapserver.The majority of the changes are around the removal of the g_dap instance and instead requiring all functions to either accept a DAP instance as a parameter or refactoring some code to not directly rely on the DAP object.
When running without a port, this should behave the same as it did before.
When running with a port or unix socket, the server will now accept each new connection and run the debug session to completion. In this mode, the server will continue until interrupted.
This allows you to start lldb-dap in a server mode and reuse the server between debug sessions. This should improve load times by allowing better caching between debug sessions within lldb.