jingham created this revision.
jingham added reviewers: JDevlieghere, clayborg.
Herald added a subscriber: jeroen.dobbelaere.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
At present, backtick substitution is a "preprocessor" operation, which is not,
I think, what people expect. I'm pretty sure you didn't mean to run an
expression when you specified a file name that happened to have backticks in
it, etc. I think it makes a lot more sense that the substitution should be
done when it is encountered as a quote character. There was, in fact, some
code in CommandObjectParsed that attempted to treat it this way, but it was
defeated by the fact that we unconditionally do PreprocessCommand on the
command string before parsing. It was also not quite right since it assumed
the quote would still be present in the argument value, when in fact it was
just in the quote member of the ArgEntry...
I fixed that, and changed the CommandInterpreter code to only preprocess the
whole string for raw commands. I think really that Raw commands should handle
their own preprocessing, for instance I don't think doing backtick substitution
in the expression passed to `expr` is really what people expect...
When I did that a bunch of tests failed because we were not correctly
preserving the backticks around arguments when we parsed them into a temporary
argv array and then back out again, and because we didn't take them into
account when stripping elements out of the incoming command when handling
command aliases.
I also added a test that explicitly tests command aliases with embedded
backticks. The test has a succeeding case and a failing case. The latter
shows an extant bug this patch doesn't fix: the CommandAlias::Desugar doesn't
correctly handle the case where you alias an alias and the second alias fills
in slots of the first alias. It just pastes the second aliases options after
the first aliases and then tries to evaluate them, which ends up with unfilled
slots. As I say, that's an extant bug so the test for it fails now. I'll fix
that one next chance I have to mess with this.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D133045
Files:
lldb/include/lldb/Interpreter/CommandInterpreter.h
lldb/packages/Python/lldbsuite/test/lldbtest.py
lldb/source/Interpreter/CommandAlias.cpp
lldb/source/Interpreter/CommandInterpreter.cpp
lldb/source/Interpreter/CommandObject.cpp
lldb/source/Interpreter/Options.cpp
lldb/source/Utility/Args.cpp
lldb/test/API/commands/command/backticks/Makefile
lldb/test/API/commands/command/backticks/TestBackticksInAlias.py
lldb/test/API/commands/command/backticks/main.c
Index: lldb/test/API/commands/command/backticks/main.c
===================================================================
--- /dev/null
+++ lldb/test/API/commands/command/backticks/main.c
@@ -0,0 +1,5 @@
+int main (int argc, char const *argv[])
+{
+ return argc; // break here
+}
+
Index: lldb/test/API/commands/command/backticks/TestBackticksInAlias.py
===================================================================
--- /dev/null
+++ lldb/test/API/commands/command/backticks/TestBackticksInAlias.py
@@ -0,0 +1,32 @@
+"""
+Test that an alias can contain active backticks
+"""
+
+
+
+import lldb
+from lldbsuite.test.lldbtest import *
+import lldbsuite.test.lldbutil as lldbutil
+
+
+class TestBackticksInAlias(TestBase):
+ NO_DEBUG_INFO_TESTCASE = True
+
+ def test_backticks_in_alias(self):
+ """Test that an alias can contain active backtraces."""
+ self.build()
+ (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.c"))
+ interp = self.dbg.GetCommandInterpreter();
+ result = lldb.SBCommandReturnObject()
+ interp.HandleCommand('command alias _test-argv-cmd expression -Z \`argc\` -- argv', result)
+ self.assertCommandReturn(result, "Made the alias")
+ interp.HandleCommand("_test-argv-cmd", result)
+ self.assertCommandReturn(result, "The alias worked")
+
+ # Now try a harder case where we create this using an alias:
+ interp.HandleCommand('command alias _test-argv-parray-cmd parray \`argc\` argv', result)
+ self.assertCommandReturn(result, "Made the alias")
+ interp.HandleCommand("_test-argv-parray-cmd", result)
+ self.assertFalse(result.Succeeded(), "CommandAlias::Desugar currently fails if a alias substitutes %N arguments in another alias")
+
+
Index: lldb/test/API/commands/command/backticks/Makefile
===================================================================
--- /dev/null
+++ lldb/test/API/commands/command/backticks/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
Index: lldb/source/Utility/Args.cpp
===================================================================
--- lldb/source/Utility/Args.cpp
+++ lldb/source/Utility/Args.cpp
@@ -215,7 +215,12 @@
for (size_t i = 0; i < m_entries.size(); ++i) {
if (i > 0)
command += ' ';
+ char quote = m_entries[i].quote;
+ if (quote != '\0')
+ command += quote;
command += m_entries[i].ref();
+ if (quote != '\0')
+ command += quote;
}
return !m_entries.empty();
Index: lldb/source/Interpreter/Options.cpp
===================================================================
--- lldb/source/Interpreter/Options.cpp
+++ lldb/source/Interpreter/Options.cpp
@@ -1002,37 +1002,47 @@
.str(),
llvm::inconvertibleErrorCode());
}
- if (!option_arg)
- option_arg = "<no-argument>";
- option_arg_vector->emplace_back(std::string(option_str.GetString()),
- has_arg, std::string(option_arg));
-
// Find option in the argument list; also see if it was supposed to take an
// argument and if one was supplied. Remove option (and argument, if
// given) from the argument list. Also remove them from the
// raw_input_string, if one was passed in.
+ // Note: We also need to preserve any option argument values that were
+ // surrounded by backticks, as we lose track of them in the
+ // option_args_vector.
size_t idx =
FindArgumentIndexForOption(args_copy, long_options[long_options_index]);
+ std::string option_to_insert;
+ if (option_arg) {
+ if (idx != size_t(-1) && has_arg) {
+ bool arg_has_backtick = args_copy[idx + 1].GetQuoteChar() == '`';
+ if (arg_has_backtick)
+ option_to_insert = "`";
+ option_to_insert += option_arg;
+ if (arg_has_backtick)
+ option_to_insert += "`";
+ } else
+ option_to_insert = option_arg;
+ } else
+ option_to_insert = CommandInterpreter::g_no_argument;
+
+ option_arg_vector->emplace_back(std::string(option_str.GetString()),
+ has_arg, option_to_insert);
+
if (idx == size_t(-1))
continue;
if (!input_line.empty()) {
- auto tmp_arg = args_copy[idx].ref();
+ llvm::StringRef tmp_arg = args_copy[idx].ref();
size_t pos = input_line.find(std::string(tmp_arg));
if (pos != std::string::npos)
input_line.erase(pos, tmp_arg.size());
}
args_copy.DeleteArgumentAtIndex(idx);
- if ((long_options[long_options_index].definition->option_has_arg !=
- OptionParser::eNoArgument) &&
- (OptionParser::GetOptionArgument() != nullptr) &&
- (idx < args_copy.GetArgumentCount()) &&
- (args_copy[idx].ref() == OptionParser::GetOptionArgument())) {
+ if (option_to_insert != CommandInterpreter::g_no_argument) {
if (input_line.size() > 0) {
- auto tmp_arg = args_copy[idx].ref();
- size_t pos = input_line.find(std::string(tmp_arg));
+ size_t pos = input_line.find(option_to_insert);
if (pos != std::string::npos)
- input_line.erase(pos, tmp_arg.size());
+ input_line.erase(pos, option_to_insert.size());
}
args_copy.DeleteArgumentAtIndex(idx);
}
Index: lldb/source/Interpreter/CommandObject.cpp
===================================================================
--- lldb/source/Interpreter/CommandObject.cpp
+++ lldb/source/Interpreter/CommandObject.cpp
@@ -727,7 +727,7 @@
}
if (!handled) {
for (auto entry : llvm::enumerate(cmd_args.entries())) {
- if (!entry.value().ref().empty() && entry.value().ref().front() == '`') {
+ if (!entry.value().ref().empty() && entry.value().GetQuoteChar() == '`') {
cmd_args.ReplaceArgumentAtIndex(
entry.index(),
m_interpreter.ProcessEmbeddedScriptCommands(entry.value().c_str()));
Index: lldb/source/Interpreter/CommandInterpreter.cpp
===================================================================
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -105,6 +105,11 @@
"and\n"
"accept the security risk.";
+constexpr const char *CommandInterpreter::g_no_argument = "<no-argument>";
+constexpr const char *CommandInterpreter::g_need_argument = "<need-argument>";
+constexpr const char *CommandInterpreter::g_argument = "<argument>";
+
+
#define LLDB_PROPERTIES_interpreter
#include "InterpreterProperties.inc"
@@ -1634,7 +1639,7 @@
std::string value;
for (const auto &entry : *option_arg_vector) {
std::tie(option, value_type, value) = entry;
- if (option == "<argument>") {
+ if (option == g_argument) {
result_str.Printf(" %s", value.c_str());
continue;
}
@@ -1656,11 +1661,33 @@
index);
return nullptr;
} else {
- size_t strpos = raw_input_string.find(cmd_args.GetArgumentAtIndex(index));
- if (strpos != std::string::npos)
+ const Args::ArgEntry &entry = cmd_args[index];
+ size_t strpos = raw_input_string.find(entry.c_str());
+ const char quote_char = entry.GetQuoteChar();
+ if (strpos != std::string::npos) {
+ const size_t start_fudge = quote_char == '\0' ? 0 : 1;
+ const size_t len_fudge = quote_char == '\0' ? 0 : 2;
+
+ // Make sure we aren't going outside the bounds of the cmd string:
+ if (strpos < start_fudge) {
+ result.AppendError("Unmatched quote at command beginning.");
+ return nullptr;
+ }
+ llvm::StringRef arg_text = entry.ref();
+ if (strpos - start_fudge + arg_text.size() + len_fudge
+ > raw_input_string.size()) {
+ result.AppendError("Unmatched quote at command end.");
+ return nullptr;
+ }
raw_input_string = raw_input_string.erase(
- strpos, strlen(cmd_args.GetArgumentAtIndex(index)));
- result_str.Printf("%s", cmd_args.GetArgumentAtIndex(index));
+ strpos - start_fudge,
+ strlen(cmd_args.GetArgumentAtIndex(index)) + len_fudge);
+ }
+ if (quote_char == '\0')
+ result_str.Printf("%s", cmd_args.GetArgumentAtIndex(index));
+ else
+ result_str.Printf("%c%s%c", quote_char,
+ entry.c_str(), quote_char);
}
}
@@ -1911,13 +1938,6 @@
return true;
}
- Status error(PreprocessCommand(command_string));
-
- if (error.Fail()) {
- result.AppendError(error.AsCString());
- return false;
- }
-
// Phase 1.
// Before we do ANY kind of argument processing, we need to figure out what
@@ -1935,6 +1955,20 @@
CommandObject *cmd_obj = ResolveCommandImpl(command_string, result);
+ // We have to preprocess the whole command string for Raw commands, since we
+ // don't know the structure of the command. For parsed commands, we only
+ // treat backticks as quote characters specially.
+ // FIXME: We probably want to have raw commands do their own preprocessing.
+ // For instance, I don't think people expect substitution in expr expressions.
+ if (cmd_obj && cmd_obj->WantsRawCommandString()) {
+ Status error(PreprocessCommand(command_string));
+
+ if (error.Fail()) {
+ result.AppendError(error.AsCString());
+ return false;
+ }
+ }
+
// Although the user may have abbreviated the command, the command_string now
// has the command expanded to the full name. For example, if the input was
// "br s -n main", command_string is now "breakpoint set -n main".
@@ -2163,7 +2197,7 @@
std::string value;
for (const auto &option_entry : *option_arg_vector) {
std::tie(option, value_type, value) = option_entry;
- if (option == "<argument>") {
+ if (option == g_argument) {
if (!wants_raw_input || (value != "--")) {
// Since we inserted this above, make sure we don't insert it twice
new_args.AppendArgument(value);
@@ -2174,7 +2208,7 @@
if (value_type != OptionParser::eOptionalArgument)
new_args.AppendArgument(option);
- if (value == "<no-argument>")
+ if (value == g_no_argument)
continue;
int index = GetOptionArgumentPosition(value.c_str());
Index: lldb/source/Interpreter/CommandAlias.cpp
===================================================================
--- lldb/source/Interpreter/CommandAlias.cpp
+++ lldb/source/Interpreter/CommandAlias.cpp
@@ -60,11 +60,12 @@
if (!options_string.empty()) {
if (cmd_obj_sp->WantsRawCommandString())
- option_arg_vector->emplace_back("<argument>", -1, options_string);
+ option_arg_vector->emplace_back(CommandInterpreter::g_argument,
+ -1, options_string);
else {
for (auto &entry : args.entries()) {
if (!entry.ref().empty())
- option_arg_vector->emplace_back(std::string("<argument>"), -1,
+ option_arg_vector->emplace_back(std::string(CommandInterpreter::g_argument), -1,
std::string(entry.ref()));
}
}
@@ -153,11 +154,12 @@
for (const auto &opt_entry : *options) {
std::tie(opt, std::ignore, value) = opt_entry;
- if (opt == "<argument>") {
+ if (opt == CommandInterpreter::g_argument) {
help_string.Printf(" %s", value.c_str());
} else {
help_string.Printf(" %s", opt.c_str());
- if ((value != "<no-argument>") && (value != "<need-argument")) {
+ if ((value != CommandInterpreter::g_no_argument)
+ && (value != CommandInterpreter::g_need_argument)) {
help_string.Printf(" %s", value.c_str());
}
}
@@ -178,7 +180,7 @@
for (const auto &opt_entry : *GetOptionArguments()) {
std::tie(opt, std::ignore, value) = opt_entry;
- if (opt == "<argument>" && !value.empty() &&
+ if (opt == CommandInterpreter::g_argument && !value.empty() &&
llvm::StringRef(value).endswith("--")) {
m_is_dashdash_alias = eLazyBoolYes;
break;
@@ -206,6 +208,8 @@
return {nullptr, nullptr};
if (underlying->IsAlias()) {
+ // FIXME: This doesn't work if the original alias fills a slot in the
+ // underlying alias, since this just appends the two lists.
auto desugared = ((CommandAlias *)underlying.get())->Desugar();
auto options = GetOptionArguments();
options->insert(options->begin(), desugared.second->begin(),
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===================================================================
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -2471,6 +2471,13 @@
self.fail(self._formatMessage(msg,
"'{}' is not success".format(error)))
+ """Assert that a command return object is successful"""
+ def assertCommandReturn(self, obj, msg=None):
+ if not obj.Succeeded():
+ error = obj.GetError()
+ self.fail(self._formatMessage(msg,
+ "'{}' is not success".format(error)))
+
"""Assert two states are equal"""
def assertState(self, first, second, msg=None):
if first != second:
Index: lldb/include/lldb/Interpreter/CommandInterpreter.h
===================================================================
--- lldb/include/lldb/Interpreter/CommandInterpreter.h
+++ lldb/include/lldb/Interpreter/CommandInterpreter.h
@@ -239,6 +239,14 @@
eCommandTypesAllThem = 0xFFFF //< all commands
};
+ // The CommandAlias and CommandInterpreter both have a hand in
+ // substituting for alias commands. They work by writing special tokens
+ // in the template form of the Alias command, and then detecting them when the
+ // command is executed. These are the special tokens:
+ static const char * const g_no_argument;
+ static const char * const g_need_argument;
+ static const char * const g_argument;
+
CommandInterpreter(Debugger &debugger, bool synchronous_execution);
~CommandInterpreter() override = default;
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
