Fix handling of backticks in CommandObjectParsed commands

[jingham]

Summary

A while back I reworked the way that backticks are handled to make backticks in aliases work, but I made two bad assumptions and broke the handling for parsed commands. The bad assumptions were:

1. There was testing for backticks in regular commands (turns out the extant tests were all for raw commands)
2. That the extant function that was supposed to handle backtick options did the thing you do with backtick options

2 is me not reading closely enough. The function called was m_interpreter.ProcessEmbeddedScriptCommands so I really should have seen this wasn't doing the right thing. Apparently at some point we were going to add the ability to invoke a script command and insert its text in place of the option. That's not a bad idea, but it was never actually implemented, and anyway, you can't use backticks as the demarcation - that's already taken for expression substitution.

This patch repairs that breakage, and adds some more tests for backticks in raw commands, in the arguments of parsed commands and in the option values for parsed commands.

[JDevlieghere]

LGTM

lldb/source/Interpreter/CommandInterpreter.cpp

1754–1760

You could avoid a level of indentation and a continue if you invert the condition.

lldb/source/Interpreter/CommandObject.cpp

729–738

There's a lot of entry.value() repetition here, might be worth extracting a temporary value here.

[jingham]

I fixed those two nits on submission.

[JDevlieghere]

https://reviews.llvm.org/rG75ca15fcbb2e1b3671e41f971a000c6d59f5e5ae

lldb/include/lldb/Interpreter/CommandInterpreter.h

bool SaveTranscript(CommandReturnObject &result,
                       std::optional<std::string> output_file = std::nullopt);
 
   FileSpec GetCurrentSourceDir();
 
   bool IsInteractive();
 
   bool IOHandlerInterrupt(IOHandler &io_handler) override;
 
+  Status PreprocessCommand(std::string &command);
+  Status PreprocessToken(std::string &token);
+
 protected:
   friend class Debugger;
 
   // This checks just the RunCommandInterpreter interruption state.  It is only
   // meant to be used in Debugger::InterruptRequested
   bool WasInterrupted() const;
 
   // IOHandlerDelegate functions
lldb::CommandObjectSP GetCommandSP(llvm::StringRef cmd,
                                      StringList *matches = nullptr,
                                      StringList *descriptions = nullptr) const;
 
 private:
   void OverrideExecutionContext(const ExecutionContext &override_context);
 
   void RestoreExecutionContext();
 
-  Status PreprocessCommand(std::string &command);
-
   void SourceInitFile(FileSpec file, CommandReturnObject &result);
 
   // Completely resolves aliases and abbreviations, returning a pointer to the
   // final command object and updating command_line to the fully substituted
   // and translated command.
   CommandObject *ResolveCommandImpl(std::string &command_line,
                                     CommandReturnObject &result);
 

lldb/source/Interpreter/CommandInterpreter.cpp

while ((start_backtick = command.find('`', pos)) != std::string::npos) {
     if (end_backtick == expr_content_start) {
       // Skip over empty expression. (two backticks in a row)
       command.erase(start_backtick, 2);
       continue;
     }
 
     std::string expr_str(command, expr_content_start,
                          end_backtick - expr_content_start);
+    error = PreprocessToken(expr_str);
+    if (error.Success()) {
+      command.erase(start_backtick, end_backtick - start_backtick + 1);
+      command.insert(start_backtick, std::string(expr_str));
+      pos = start_backtick + expr_str.size();
+      continue;
+    } else
+      break;
+  }
+  return error;                        
+}
 
-    ExecutionContext exe_ctx(GetExecutionContext());
+Status
+CommandInterpreter::PreprocessToken(std::string &expr_str) {
+  Status error;
 
-    // Get a dummy target to allow for calculator mode while processing
-    // backticks. This also helps break the infinite loop caused when target is
-    // null.
-    Target *exe_target = exe_ctx.GetTargetPtr();
-    Target &target = exe_target ? *exe_target : m_debugger.GetDummyTarget();
 
-    ValueObjectSP expr_result_valobj_sp;
 
-    EvaluateExpressionOptions options;
-    options.SetCoerceToId(false);
-    options.SetUnwindOnError(true);
-    options.SetIgnoreBreakpoints(true);
-    options.SetKeepInMemory(false);
-    options.SetTryAllThreads(true);
-    options.SetTimeout(std::nullopt);
 
-    ExpressionResults expr_result =
-        target.EvaluateExpression(expr_str.c_str(), exe_ctx.GetFramePtr(),
-                                  expr_result_valobj_sp, options);
 
-    if (expr_result == eExpressionCompleted) {
-      Scalar scalar;
-      if (expr_result_valobj_sp)
-        expr_result_valobj_sp =
-            expr_result_valobj_sp->GetQualifiedRepresentationIfAvailable(
-                expr_result_valobj_sp->GetDynamicValueType(), true);
-      if (expr_result_valobj_sp->ResolveValue(scalar)) {
-        command.erase(start_backtick, end_backtick - start_backtick + 1);
-        StreamString value_strm;
-        const bool show_type = false;
-        scalar.GetValue(&value_strm, show_type);
-        size_t value_string_size = value_strm.GetSize();
-        if (value_string_size) {
-          command.insert(start_backtick, std::string(value_strm.GetString()));
-          pos = start_backtick + value_string_size;
-          continue;
-        } else {
-          error.SetErrorStringWithFormat("expression value didn't result "
-                                         "in a scalar value for the "
-                                         "expression '%s'",
-                                         expr_str.c_str());
-          break;
-        }
-      } else {
-        error.SetErrorStringWithFormat("expression value didn't result "
-                                       "in a scalar value for the "
-                                       "expression '%s'",
-                                       expr_str.c_str());
-        break;
-      }
-
-      continue;
-    }
+      
+        expr_str = value_strm.GetData();
+    return error;
 
-    if (expr_result_valobj_sp)
-      error = expr_result_valobj_sp->GetError();
+  // If we have an error from the expression evaluation it will be in the
+  // ValueObject error, which won't be success and we will just report it.
+  // But if for some reason we didn't get a value object at all, then we will
+  // make up some helpful errors from the expression result.
 
-    if (error.Success()) {
-      switch (expr_result) {
-      case eExpressionSetupError:
-        error.SetErrorStringWithFormat(
-            "expression setup error for the expression '%s'", expr_str.c_str());
-        break;
-      case eExpressionParseError:
-        error.SetErrorStringWithFormat(
-            "expression parse error for the expression '%s'", expr_str.c_str());
-        break;
-      case eExpressionResultUnavailable:
-        error.SetErrorStringWithFormat(
-            "expression error fetching result for the expression '%s'",
-            expr_str.c_str());
-        break;
-      case eExpressionCompleted:
-        break;
-      case eExpressionDiscarded:
-        error.SetErrorStringWithFormat(
-            "expression discarded for the expression '%s'", expr_str.c_str());
-        break;
-      case eExpressionInterrupted:
-        error.SetErrorStringWithFormat(
-            "expression interrupted for the expression '%s'", expr_str.c_str());
-        break;
-      case eExpressionHitBreakpoint:
-        error.SetErrorStringWithFormat(
-            "expression hit breakpoint for the expression '%s'",
-            expr_str.c_str());
-        break;
-      case eExpressionTimedOut:
-        error.SetErrorStringWithFormat(
-            "expression timed out for the expression '%s'", expr_str.c_str());
-        break;
-      case eExpressionStoppedForDebug:
-        error.SetErrorStringWithFormat("expression stop at entry point "
-                                       "for debugging for the "
-                                       "expression '%s'",
-                                       expr_str.c_str());
-        break;
-      case eExpressionThreadVanished:
-        error.SetErrorStringWithFormat(
-            "expression thread vanished for the expression '%s'",
-            expr_str.c_str());
-        break;
-      }
-    }
-  }
   return error;
 }
 
 bool CommandInterpreter::HandleCommand(const char *command_line,
                                        LazyBool lazy_add_to_history,
                                        const ExecutionContext &override_context,
                                        CommandReturnObject &result) {
 

lldb/source/Interpreter/CommandObject.cpp

bool CommandObjectParsed::Execute(const char *args_string,
   Args cmd_args(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().GetQuoteChar() == '`') {
-        cmd_args.ReplaceArgumentAtIndex(
-            entry.index(),
-            m_interpreter.ProcessEmbeddedScriptCommands(entry.value().c_str()));
+        // We have to put the backtick back in place for PreprocessCommand.
+        std::string opt_string = entry.value().c_str();
+        Status error;
+        error = m_interpreter.PreprocessToken(opt_string);
+        if (error.Success())
+          cmd_args.ReplaceArgumentAtIndex(entry.index(), opt_string);
       }
     }
 
     if (CheckRequirements(result)) {
       if (ParseOptions(cmd_args, result)) {
         // Call the command-specific version of 'Execute', passing it the
         // already processed arguments.
         if (cmd_args.GetArgumentCount() != 0 && m_arguments.empty()) {
           result.AppendErrorWithFormatv("'{0}' doesn't take any arguments.",
                                         GetCommandName());

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

def test_backticks_in_alias(self):
         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")
 
+    def test_backticks_in_parsed_cmd_argument(self):
+        """ break list is a parsed command, use a variable for the breakpoint number
+            and make sure that and the direct use of the ID get the same result. """
+        self.build()
+        target, process, thread, bkpt = lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.c"))
+        # Make a second breakpoint so that if the backtick part -> nothing we'll print too much:
+        # It doesn't need to resolve to anything.
+        dummy_bkpt = target.BreakpointCreateByName("dont_really_care_if_this_exists")
+        
+        bkpt_id = bkpt.GetID()
+        self.runCmd(f"expr int $number = {bkpt_id}")
+        direct_result = lldb.SBCommandReturnObject()
+        backtick_result = lldb.SBCommandReturnObject()
+        interp = self.dbg.GetCommandInterpreter()
+        interp.HandleCommand(f"break list {bkpt_id}", direct_result)
+        self.assertTrue(direct_result.Succeeded(), "Break list with id works")
+        interp.HandleCommand("break list `$number`", backtick_result)
+        self.assertTrue(direct_result.Succeeded(), "Break list with backtick works")
+        self.assertEqual(direct_result.GetOutput(), backtick_result.GetOutput(), "Output is the same")
+
+    def test_backticks_in_parsed_cmd_option(self):
+        # The script interpreter is a raw command, so try that one:
+        self.build()
+        target, process, thread, bkpt = lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.c"))
+
+        self.runCmd(f"expr int $number = 2")
+        direct_result = lldb.SBCommandReturnObject()
+        backtick_result = lldb.SBCommandReturnObject()
+        interp = self.dbg.GetCommandInterpreter()
+        interp.HandleCommand(f"memory read --count 2 argv", direct_result)
+        self.assertTrue(direct_result.Succeeded(), "memory read with direct count works")
+        interp.HandleCommand("memory read --count `$number` argv", backtick_result)
+        self.assertTrue(direct_result.Succeeded(), "memory read with backtick works")
+        self.assertEqual(direct_result.GetOutput(), backtick_result.GetOutput(), "Output is the same")
+
+    def test_backticks_in_raw_cmd(self):
+        # The script interpreter is a raw command, so try that one:
+        self.build()
+        target, process, thread, bkpt = lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.c"))
+        argc_valobj = thread.frames[0].FindVariable("argc")
+        self.assertTrue(argc_valobj.GetError().Success(), "Made argc valobj")
+        argc_value = argc_valobj.GetValueAsUnsigned(0)
+        self.assertNotEqual(argc_value, 0, "Got a value for argc")
+        result = lldb.SBCommandReturnObject()
+        interp = self.dbg.GetCommandInterpreter()
+        interp.HandleCommand(f"script {argc_value} - `argc`", result)
+        self.assertTrue(result.Succeeded(), "Command succeeded")
+        self.assertEqual("0\n", result.GetOutput(), "Substitution worked")
+