Partial fix for handling backticks in commands and aliases

[jingham]

Summary

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.

[jingham]

The other bit of this patch was a cleanup I made as I was understanding the alias parsing where we were using specific tokens in the parsed alias, but the tokens were ad hoc strings, so I replaced them with constants.

[hawkinsw]

Sorry if these comments are not helpful! Everything looks great!

lldb/source/Interpreter/Options.cpp

1026

Question: Could we drop the final else if we initialized option_to_insert to CommandInterpreter::g_no_argument? Just curious.

lldb/test/API/commands/command/backticks/TestBackticksInAlias.py

17

Nit: Typo. backtraces => backticks.

[jingham]

lldb/source/Interpreter/Options.cpp

1026

No, the correct default when there is an option_arg is the empty string. So I'd also have to set the string back to empty after the if (option_arg) { which would look weird.

lldb/test/API/commands/command/backticks/TestBackticksInAlias.py

17

Thanks for spotting that, I'll fix on commit.

[mstorsjo]

This seems to break the build with GCC (tested with the system compiler GCC 9 on Ubuntu 20.04):

ld.lld: error: undefined symbol: lldb_private::CommandInterpreter::g_argument
>>> referenced by CommandAlias.cpp
>>>               CommandAlias.cpp.o:(lldb_private::CommandAlias::GetAliasExpansion(lldb_private::StreamString&) const (.localalias)) in archive lib/liblldbInterpreter.a

(with lots more similar errors)

lldb/include/lldb/Interpreter/CommandInterpreter.h

enum CommandTypes {
     eCommandTypesBuiltin = 0x0001, //< native commands such as "frame"
     eCommandTypesUserDef = 0x0002, //< scripted commands
     eCommandTypesUserMW  = 0x0004, //< multiword commands (command containers)
     eCommandTypesAliases = 0x0008, //< aliases such as "po"
     eCommandTypesHidden  = 0x0010, //< commands prefixed with an underscore
     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;
 
   // These two functions fill out the Broadcaster interface:
 
   static ConstString &GetStaticBroadcasterClass();
 

lldb/packages/Python/lldbsuite/test/lldbtest.py

""".format(cmd_status, filecheck_cmd, output, cmd_stdout, cmd_stderr)
 
     """Assert that an lldb.SBError is in the "success" state."""
     def assertSuccess(self, obj, msg=None):
         if not obj.Success():
             error = obj.GetCString()
             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:
             error = "{} ({}) != {} ({})".format(
                 lldbutil.state_type_to_str(first), first,
                 lldbutil.state_type_to_str(second), second)
             self.fail(self._formatMessage(msg, error))
 

lldb/source/Interpreter/CommandAlias.cpp

if (!result.Succeeded() &&
         result.GetStatus() != lldb::eReturnStatusStarted) {
       result.AppendError("Unable to create requested alias.\n");
       return false;
     }
   }
 
   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()));
       }
     }
   }
 
   return success;
 }
 
void CommandAlias::GetAliasExpansion(StreamString &help_string) const {
   }
 
   OptionArgVector *options = m_option_args_sp.get();
   std::string opt;
   std::string value;
 
   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());
       }
     }
   }
 
   help_string.Printf("'");
 }
 
 bool CommandAlias::IsDashDashCommand() {
   if (m_is_dashdash_alias != eLazyBoolCalculate)
     return (m_is_dashdash_alias == eLazyBoolYes);
   m_is_dashdash_alias = eLazyBoolNo;
   if (!IsValid())
     return false;
 
   std::string opt;
   std::string value;
 
   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;
     }
   }
 
   // if this is a nested alias, it may be adding arguments on top of an already
   // dash-dash alias

 }
 
 std::pair<lldb::CommandObjectSP, OptionArgVectorSP> CommandAlias::Desugar() {
   auto underlying = GetUnderlyingCommand();
   if (!underlying)
     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(),
                     desugared.second->end());
     return {desugared.first, options};
   }
 
   return {underlying, GetOptionArguments()};

lldb/source/Interpreter/CommandInterpreter.cpp

static constexpr const char *InitFileWarning =
     "add the following to the lldbinit file in your home directory:\n"
     "    settings set target.load-cwd-lldbinit false\n"
     "To allow lldb to source .lldbinit files in the current working "
     "directory,\n"
     "set the value of this variable to true.  Only do so if you understand "
     "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"
 
 enum {
 #define LLDB_PROPERTIES_interpreter
 #include "InterpreterPropertiesEnum.inc"
 };
 
CommandObject *CommandInterpreter::BuildAliasResult(
   }
   OptionArgVector *option_arg_vector = option_arg_vector_sp.get();
 
   int value_type;
   std::string option;
   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;
     }
 
     result_str.Printf(" %s", option.c_str());
     if (value_type == OptionParser::eNoArgument)
       continue;
 
     if (value_type != OptionParser::eOptionalArgument)
       result_str.Printf(" ");
     int index = GetOptionArgumentPosition(value.c_str());
     if (index == 0)
       result_str.Printf("%s", value.c_str());
     else if (static_cast<size_t>(index) >= cmd_args.GetArgumentCount()) {
 
       result.AppendErrorWithFormat("Not enough arguments provided; you "
                                    "need at least %d arguments to use "
                                    "this alias.\n",
                                    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)));
+            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);
     }
   }
 
   alias_result = std::string(result_str.GetString());
   return alias_cmd_obj;
 }
 
 Status CommandInterpreter::PreprocessCommand(std::string &command) {
if (empty_command) {
     }
 
     add_to_history = false;
   } else if (comment_command) {
     result.SetStatus(eReturnStatusSuccessFinishNoResult);
     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
   // the real/final command object is for the specified command.  This gets
   // complicated by the fact that the user could have specified an alias, and,
   // in translating the alias, there may also be command options and/or even
   // data (including raw text strings) that need to be found and inserted into
   // the command line as part of the translation.  So this first step is plain
   // look-up and replacement, resulting in:
   //    1. the command object whose Execute method will actually be called
   //    2. a revised command string, with all substitutions and replacements
   //       taken care of
   // From 1 above, we can determine whether the Execute function wants raw
   // input or not.
 
   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".
   if (log) {
     llvm::StringRef command_name = cmd_obj ? cmd_obj->GetCommandName() : "<not found>";
     LLDB_LOGF(log, "HandleCommand, cmd_obj : '%s'", command_name.str().c_str());
     LLDB_LOGF(log, "HandleCommand, (revised) command_string: '%s'",
               command_string.c_str());
if (option_arg_vector_sp.get()) {
 
     used[0] = true;
 
     int value_type;
     std::string option;
     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);
         }
         continue;
       }
 
       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());
       if (index == 0) {
         // value was NOT a positional argument; must be a real value
         if (value_type != OptionParser::eOptionalArgument)
           new_args.AppendArgument(value);
         else {

lldb/source/Interpreter/CommandObject.cpp

bool CommandObjectParsed::Execute(const char *args_string,
   if (HasOverrideCallback()) {
     Args full_args(GetCommandName());
     full_args.AppendArguments(cmd_args);
     handled =
         InvokeOverrideCallback(full_args.GetConstArgumentVector(), result);
   }
   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()));
       }
     }
 
     if (CheckRequirements(result)) {
       if (ParseOptions(cmd_args, result)) {

lldb/source/Interpreter/Options.cpp

while (true) {
     default:
       return llvm::make_error<llvm::StringError>(
           llvm::formatv("error with options table; invalid value in has_arg "
                         "field for option '{0}'.",
                         char(val))
               .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);
     }
   }
 
   return std::move(args_copy);
 }
 

lldb/source/Utility/Args.cpp

 }
 
 bool Args::GetCommandString(std::string &command) const {
   command.clear();
 
   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();
 }
 
 bool Args::GetQuotedCommandString(std::string &command) const {
   command.clear();
 

lldb/test/API/commands/command/backticks/Makefile

+C_SOURCES := main.c
+
+include Makefile.rules

lldb/test/API/commands/command/backticks/TestBackticksInAlias.py

+"""
+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 backticks."""
+        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")
+
+        

lldb/test/API/commands/command/backticks/main.c

+int main (int argc, char const *argv[])
+{
+  return argc; // break here
+}
+