[lldb-vscode] Allow specifying a custom escape prefix for LLDB commands #69238
Conversation
|
@llvm/pr-subscribers-lldb Author: Walter Erquinigo (walter-erquinigo) ChangesWe've been using the backtick as our escape character, however that leads to a weird experience on VS Code, because on most hosts, as soon as you type the backtick on VS Code, the IDE will introduce another backtick. As changing the default escape character might be out of question because other plugins might rely on it, we can instead introduce an option to change this variable upon lldb-vscode initialization. Full diff: https://github.com/llvm/llvm-project/pull/69238.diff 7 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
index 8cd4e8454c89099..1869a5579f0a12e 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
@@ -8,7 +8,7 @@
class VSCodeTestCaseBase(TestBase):
NO_DEBUG_INFO_TESTCASE = True
- def create_debug_adaptor(self, lldbVSCodeEnv=None):
+ def create_debug_adaptor(self, lldbVSCodeEnv=None, commandEscapeChar='`'):
"""Create the Visual Studio Code debug adaptor"""
self.assertTrue(
is_exe(self.lldbVSCodeExec), "lldb-vscode must exist and be executable"
@@ -16,14 +16,15 @@ def create_debug_adaptor(self, lldbVSCodeEnv=None):
log_file_path = self.getBuildArtifact("vscode.txt")
self.vscode = vscode.DebugAdaptor(
executable=self.lldbVSCodeExec,
+ args=["--command-escape-character", commandEscapeChar],
init_commands=self.setUpCommands(),
log_file=log_file_path,
env=lldbVSCodeEnv,
)
- def build_and_create_debug_adaptor(self, lldbVSCodeEnv=None):
+ def build_and_create_debug_adaptor(self, lldbVSCodeEnv=None, commandEscapeChar='`'):
self.build()
- self.create_debug_adaptor(lldbVSCodeEnv)
+ self.create_debug_adaptor(lldbVSCodeEnv, commandEscapeChar)
def set_source_breakpoints(self, source_path, lines, data=None):
"""Sets source breakpoints and returns an array of strings containing
@@ -425,11 +426,12 @@ def build_and_launch(
lldbVSCodeEnv=None,
enableAutoVariableSummaries=False,
enableSyntheticChildDebugging=False,
+ commandEscapeChar='`',
):
"""Build the default Makefile target, create the VSCode debug adaptor,
and launch the process.
"""
- self.build_and_create_debug_adaptor(lldbVSCodeEnv)
+ self.build_and_create_debug_adaptor(lldbVSCodeEnv, commandEscapeChar=commandEscapeChar)
self.assertTrue(os.path.exists(program), "executable must exist")
return self.launch(
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
index 5ee0800b27a5699..f4b393e91c10c6e 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
@@ -1015,7 +1015,7 @@ def terminate(self):
class DebugAdaptor(DebugCommunication):
def __init__(
- self, executable=None, port=None, init_commands=[], log_file=None, env=None
+ self, executable=None, args=[], port=None, init_commands=[], log_file=None, env=None
):
self.process = None
if executable is not None:
@@ -1026,7 +1026,7 @@ def __init__(
if log_file:
adaptor_env["LLDBVSCODE_LOG"] = log_file
self.process = subprocess.Popen(
- [executable],
+ [executable, *args],
stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
diff --git a/lldb/test/API/tools/lldb-vscode/console/TestVSCode_console.py b/lldb/test/API/tools/lldb-vscode/console/TestVSCode_console.py
index d28e98b37c589dd..960b6c355fefc1d 100644
--- a/lldb/test/API/tools/lldb-vscode/console/TestVSCode_console.py
+++ b/lldb/test/API/tools/lldb-vscode/console/TestVSCode_console.py
@@ -2,16 +2,16 @@
Test lldb-vscode setBreakpoints request
"""
+import lldbvscode_testcase
import vscode
+from lldbsuite.test import lldbutil
from lldbsuite.test.decorators import *
from lldbsuite.test.lldbtest import *
-from lldbsuite.test import lldbutil
-import lldbvscode_testcase
class TestVSCode_console(lldbvscode_testcase.VSCodeTestCaseBase):
- def check_lldb_command(self, lldb_command, contains_string, assert_msg):
- response = self.vscode.request_evaluate("`%s" % (lldb_command), context="repl")
+ def check_lldb_command(self, lldb_command, contains_string, assert_msg, command_escape_char='`'):
+ response = self.vscode.request_evaluate(f"{command_escape_char}{lldb_command}", context="repl")
output = response["body"]["result"]
self.assertIn(
contains_string,
@@ -66,3 +66,16 @@ def test_scopes_variables_setVariable_evaluate(self):
# currently selected frame.
self.check_lldb_command("frame select", "frame #1", "frame 1 is selected")
+
+
+ @skipIfWindows
+ @skipIfRemote
+ def test_custom_escape_char(self):
+ program = self.getBuildArtifact("a.out")
+ self.build_and_launch(program, commandEscapeChar=":")
+ source = "main.cpp"
+ breakpoint1_line = line_number(source, "// breakpoint 1")
+ breakpoint_ids = self.set_source_breakpoints(source, [breakpoint1_line])
+ self.continue_to_breakpoints(breakpoint_ids)
+
+ self.check_lldb_command("help", "For more information on any command", "Help can be invoked", command_escape_char=':')
diff --git a/lldb/tools/lldb-vscode/Options.td b/lldb/tools/lldb-vscode/Options.td
index a6ba0a318f388a1..7e885abeb615326 100644
--- a/lldb/tools/lldb-vscode/Options.td
+++ b/lldb/tools/lldb-vscode/Options.td
@@ -43,3 +43,9 @@ def debugger_pid: S<"debugger-pid">,
def repl_mode: S<"repl-mode">,
MetaVarName<"<mode>">,
HelpText<"The mode for handling repl evaluation requests, supported modes: variable, command, auto.">;
+
+def command_escape_character: S<"command-escape-character">,
+ MetaVarName<"<character>">,
+ HelpText<"The escape prefix character to use for executing regular LLDB "
+ "commands in the Debug Console, instead of printing variables. "
+ "Defaults to a back-tick (`).">;
diff --git a/lldb/tools/lldb-vscode/VSCode.cpp b/lldb/tools/lldb-vscode/VSCode.cpp
index 1384604c48371b7..7ce5932f8691e80 100644
--- a/lldb/tools/lldb-vscode/VSCode.cpp
+++ b/lldb/tools/lldb-vscode/VSCode.cpp
@@ -384,8 +384,8 @@ llvm::json::Value VSCode::CreateTopLevelScopes() {
ExpressionContext VSCode::DetectExpressionContext(lldb::SBFrame &frame,
std::string &text) {
- // Include ` as an escape hatch.
- if (!text.empty() && text[0] == '`') {
+ // Include the escape hatch prefix.
+ if (!text.empty() && text[0] == g_vsc.command_escape_character) {
text = text.substr(1);
return ExpressionContext::Command;
}
@@ -418,7 +418,9 @@ ExpressionContext VSCode::DetectExpressionContext(lldb::SBFrame &frame,
if (!auto_repl_mode_collision_warning) {
llvm::errs() << "Variable expression '" << text
<< "' is hiding an lldb command, prefix an expression "
- "with ` to ensure it runs as a lldb command.\n";
+ "with "
+ << g_vsc.command_escape_character
+ << " to ensure it runs as a lldb command.\n";
auto_repl_mode_collision_warning = true;
}
return ExpressionContext::Variable;
diff --git a/lldb/tools/lldb-vscode/VSCode.h b/lldb/tools/lldb-vscode/VSCode.h
index 59bb11c71e67203..1802eeb821fd30d 100644
--- a/lldb/tools/lldb-vscode/VSCode.h
+++ b/lldb/tools/lldb-vscode/VSCode.h
@@ -188,6 +188,7 @@ struct VSCode {
ReplModeRequestHandler repl_mode_request_handler;
ReplMode repl_mode;
bool auto_repl_mode_collision_warning;
+ char command_escape_character = '`';
VSCode();
~VSCode();
diff --git a/lldb/tools/lldb-vscode/lldb-vscode.cpp b/lldb/tools/lldb-vscode/lldb-vscode.cpp
index 3904d430c49b4cd..06c7724fb66d25a 100644
--- a/lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ b/lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -3806,6 +3806,16 @@ int main(int argc, char *argv[]) {
g_vsc.output.descriptor = StreamDescriptor::from_file(new_stdout_fd, false);
}
+ if (llvm::opt::Arg *arg =
+ input_args.getLastArg(OPT_command_escape_character)) {
+ if (const char *escape_char = arg->getValue()) {
+ g_vsc.command_escape_character = *escape_char;
+ } else {
+ llvm::errs() << "Missing command escape character.\n";
+ return EXIT_FAILURE;
+ }
+ }
+
bool CleanExit = true;
if (auto Err = g_vsc.Loop()) {
if (g_vsc.log)
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
FWIW I like |
walter-erquinigo
force-pushed
the
walter/escape
branch
from
5b2da88
to
3d14b29
Compare
|
(Not related to this PR but I put up an RFC on Friday to rename it to |
Nice! I'll approve your PR if you make that change! |
There was a problem hiding this comment.
With "lldb-vscode" (which might be "lldb-dap" soon) being a native DAP plug-in, I believe this means we can't set settings in the IDE itself if we are not a typescript DAP plug-in.
How would anyone specify this extra option when running lldb-vscode? Is there a way to specify extra options that "lldb-vscode" should be launched with from the VS Code GUI? If so, how does this work? Or are you using this from a different IDE other than VS Code that allows you to specify extra options?
The optimal solution for this in my mind would be to have some global settings that we can set in the VS Code GUI. I know there can be plug-in specific setting set as we allow a path to be specified in a custom DAP plug-in that points to the path to the "lldb-vscode" binary to actaully run instead of the one in the extensions folder. But I don't know if this can be done with a native only DAP plug-in (our custom solution has a typescript wrapper that eventually spawns "lldb-vscode" as we need to intercept and fix some packets, so we actaully have a typescript as our main DAP plug-in and we forward packets to lldb-vscode).
The other way to fix this would be to add this as a launch config option. @walter-erquinigo this is how you did a previous fix for auto variable summaries being enabled.
So we should really figure out our solution to having global settings and do them all consistently.
There doesn't seem to be an actual standard 'LLDB' plugin in the VSCode plugin marketplace. Since they all could work differently, it makes sense to me that we add customization at the |
|
@clayborg , good point, I'll move it to launch and attach settings. |
Can you check real quick if any native plug-ins can have and or access global settings from the VS Code GUI? That would be the best way for all of these. Maybe there is a combination mode where we have some typescript in our extension, but still have a native DAP plug-in? |
|
Mainly asking since you just started adding things that fall into the "we need global plug-in settings for lldb-vscode" recently and it will be good to make sure we are doing things right from a VS code perspective. It would be nice to not have all of our settings in our launch config if possible. |
There's no way to do it natively. Typescript code is always needed, and that means adding a typescript shim to the code in LLVM. |
walter-erquinigo
force-pushed
the
walter/escape
branch
from
3d14b29
to
4595d09
Compare
|
@clayborg , I've updated this patch using launch and attach settings, which might be fine following what've been doing so far. Even though I agree that it'd be nice to read the GUI global configs, I think that all settings should be also configurable via launch and attach settings, as an overwriting mechanism. @JDevlieghere , @clayborg, what do you think of the idea of some of us in the LLDB community building a minimal typescript extension for lldb-vscode and upload it to the marketplace? Then companies building their custom debugging on top of lldb-vscode could just extend this minimal extension and contribute back to it. |
ok, then doing things as a launch config is fine for now. |
Sounds reasonable to me, thought I'm not sure that actually solves Greg's concern? I think clangd has a community plugin in the marketplace, I think it would be helpful to have something similar for |
We should go with the flow of what VS Code designs. I am fine if we get global settings eventually and allow them to be overridden with launch config stuff.
We should probably start shipping a real LLDB native marketplace solution at some point. But having typescript seems like a good idea so we can get settings, add new VS code commands, etc. Not sure if there is a performance impact to going through typescript first and forwarding to a native plug-in. If we do ship a native plug-in, it would be nice to have some settings:
Can we modify this patch so we can specify an empty prefix? This would allow users to just type in lldb command all of the time. We have an internal setting for this in our custom lldb solution at Meta. |
|
Sure, Greg, I'll update my patch with the empty prefix. |
|
Okay, to answer my own question: the difference is the typescript vs native plugin. I had missed that nuance. For the uninitiated, what most of us are currently using is the native version because it's just a |
walter-erquinigo
force-pushed
the
walter/escape
branch
from
4595d09
to
19295d5
Compare
walter-erquinigo
changed the title
|
@clayborg , friendly ping |
walter-erquinigo
force-pushed
the
walter/escape
branch
from
19295d5
to
adcf4b7
Compare
|
@clayborg , friendly ping :) |
…mands We've been using the backtick as our escape character, however that leads to a weird experience on VS Code, because on most hosts, as soon as you type the backtick on VS Code, the IDE will introduce another backtick. As changing the default escape character might be out of question because other plugins might rely on it, we can instead introduce an option to change this variable upon lldb-vscode initialization. FWIW, my users will be using : instead ot the backtick.
walter-erquinigo
force-pushed
the
walter/escape
branch
from
adcf4b7
to
09cd46a
Compare
#69238 caused breakage in VSCode debug console usage -- the user's input is always treated as commands instead of expressions (the same behavior as if empty command escape prefix is specified). The bug is in one overload of `GetString()` which did not respect the default value of "\`". But more important, I am puzzled to find out why the regression is not caught by lldb test (testdap_evaluate). Turns out #69238 specifies commandEscapePrefix default value in test framework to be "\`" while VSCode will default not specify any commandEscapePrefix at all. Changing it to None will fail `testdap_evaluate`. We should align the default behavior between DAP client and testcase. This patches fixes the bug in `GetString()` and changed the default value of commandEscapePrefix in testcases to be None (be consistent with IDE). Co-authored-by: jeffreytan81 <jeffreytan@fb.com>
…ds (llvm#69238) We've been using the backtick as our escape character, however that leads to a weird experience on VS Code, because on most hosts, as soon as you type the backtick on VS Code, the IDE will introduce another backtick. As changing the default escape character might be out of question because other plugins might rely on it, we can instead introduce an option to change this variable upon lldb-vscode initialization. FWIW, my users will be using : instead ot the backtick. (cherry picked from commit 1066481)
We've been using the backtick as our escape character, however that leads to a weird experience on VS Code, because on most hosts, as soon as you type the backtick on VS Code, the IDE will introduce another backtick. As changing the default escape character might be out of question because other plugins might rely on it, we can instead introduce an option to change this variable upon lldb-vscode initialization.
FWIW, my users will be using : instead ot the backtick.