[lldb] Fix (unintentional) recursion in CommandObjectRegexCommand

Jim noticed that the regex command is unintentionally recursive. Let's
use the following command regex as an example:

  (lldb) com regex humm 's/([^ ]+) ([^ ]+)/p %1 %2 %1 %2/'

If we call it with arguments foo bar, thing behave as expected:

  (lldb) humm foo bar
  (...)
  foo bar foo bar

However, if we include %2 in the arguments, things break down:

  (lldb) humm fo%2o bar
  (...)
  fobaro bar fobaro bar

The problem is that the implementation of the substitution is too naive.
It substitutes the %1 token into the target template in place, then does
the %2 substitution starting with the resultant string. So if the
previous substitution introduced a %2 token, it would get processed in
the second sweep, etc.

This patch addresses the issue by walking the command once and
substituting the % variables in place.

  (lldb) humm fo%2o bar
  (...)
  fo%2o bar fo%2o bar

Furthermore, this patch also reports an error if not enough variables
were provided and add support for substituting %0.

rdar://81236994

Differential revision: https://reviews.llvm.org/D120101

Author: JDevlieghere

lldb/include/lldb/Interpreter/CommandReturnObject.h

@@ -15,6 +15,7 @@
 #include "lldb/lldb-private.h"
 
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/WithColor.h"
 
@@ -132,6 +133,8 @@ class CommandReturnObject {
 
   void SetError(const Status &error, const char *fallback_error_cstr = nullptr);
 
+  void SetError(llvm::Error error);
+
   lldb::ReturnStatus GetStatus() const;
 
   void SetStatus(lldb::ReturnStatus status);

lldb/source/Commands/CommandObjectRegexCommand.cpp

@@ -10,6 +10,9 @@
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+
 using namespace lldb;
 using namespace lldb_private;
 
@@ -25,46 +28,64 @@ CommandObjectRegexCommand::CommandObjectRegexCommand(
 // Destructor
 CommandObjectRegexCommand::~CommandObjectRegexCommand() = default;
 
+llvm::Expected<std::string> CommandObjectRegexCommand::SubstituteVariables(
+    llvm::StringRef input,
+    const llvm::SmallVectorImpl<llvm::StringRef> &replacements) {
+  std::string buffer;
+  llvm::raw_string_ostream output(buffer);
+
+  llvm::SmallVector<llvm::StringRef, 4> parts;
+  input.split(parts, '%');
+
+  output << parts[0];
+  for (llvm::StringRef part : drop_begin(parts)) {
+    size_t idx = 0;
+    if (part.consumeInteger(10, idx))
+      output << '%';
+    else if (idx < replacements.size())
+      output << replacements[idx];
+    else
+      return llvm::make_error<llvm::StringError>(
+          llvm::formatv("%{0} is out of range: not enough arguments specified",
+                        idx),
+          llvm::errc::invalid_argument);
+    output << part;
+  }
+
+  return output.str();
+}
+
 bool CommandObjectRegexCommand::DoExecute(llvm::StringRef command,
                                           CommandReturnObject &result) {
   EntryCollection::const_iterator pos, end = m_entries.end();
   for (pos = m_entries.begin(); pos != end; ++pos) {
     llvm::SmallVector<llvm::StringRef, 4> matches;
     if (pos->regex.Execute(command, &matches)) {
-      std::string new_command(pos->command);
-      char percent_var[8];
-      size_t idx, percent_var_idx;
-      for (uint32_t match_idx = 1; match_idx <= m_max_matches; ++match_idx) {
-        if (match_idx < matches.size()) {
-          const std::string match_str = matches[match_idx].str();
-          const int percent_var_len =
-              ::snprintf(percent_var, sizeof(percent_var), "%%%u", match_idx);
-          for (idx = 0; (percent_var_idx = new_command.find(
-                             percent_var, idx)) != std::string::npos;) {
-            new_command.erase(percent_var_idx, percent_var_len);
-            new_command.insert(percent_var_idx, match_str);
-            idx = percent_var_idx + match_str.size();
-          }
-        }
+      llvm::Expected<std::string> new_command =
+          SubstituteVariables(pos->command, matches);
+      if (!new_command) {
+        result.SetError(new_command.takeError());
+        return false;
       }
+
       // Interpret the new command and return this as the result!
       if (m_interpreter.GetExpandRegexAliases())
-        result.GetOutputStream().Printf("%s\n", new_command.c_str());
+        result.GetOutputStream().Printf("%s\n", new_command->c_str());
       // Pass in true for "no context switching".  The command that called us
       // should have set up the context appropriately, we shouldn't have to
       // redo that.
-      return m_interpreter.HandleCommand(new_command.c_str(),
+      return m_interpreter.HandleCommand(new_command->c_str(),
                                          eLazyBoolCalculate, result);
     }
   }
   result.SetStatus(eReturnStatusFailed);
   if (!GetSyntax().empty())
     result.AppendError(GetSyntax());
   else
-    result.GetOutputStream() << "Command contents '" << command
-                             << "' failed to match any "
-                                "regular expression in the '"
-                             << m_cmd_name << "' regex ";
+    result.GetErrorStream() << "Command contents '" << command
+                            << "' failed to match any "
+                               "regular expression in the '"
+                            << m_cmd_name << "' regex ";
   return false;
 }
 

lldb/source/Commands/CommandObjectRegexCommand.h

@@ -39,6 +39,11 @@ class CommandObjectRegexCommand : public CommandObjectRaw {
 protected:
   bool DoExecute(llvm::StringRef command, CommandReturnObject &result) override;
 
+  /// Substitute variables of the format %\d+ in the input string.
+  static llvm::Expected<std::string> SubstituteVariables(
+      llvm::StringRef input,
+      const llvm::SmallVectorImpl<llvm::StringRef> &replacements);
+
   struct Entry {
     RegularExpression regex;
     std::string command;

lldb/source/Interpreter/CommandReturnObject.cpp

@@ -109,6 +109,11 @@ void CommandReturnObject::SetError(const Status &error,
   AppendError(error.AsCString(fallback_error_cstr));
 }
 
+void CommandReturnObject::SetError(llvm::Error error) {
+  if (error)
+    AppendError(llvm::toString(std::move(error)));
+}
+
 // Similar to AppendError, but do not prepend 'Status: ' to message, and don't
 // append "\n" to the end of it.
 

lldb/unittests/Interpreter/CMakeLists.txt

@@ -4,6 +4,7 @@ add_lldb_unittest(InterpreterTests
   TestOptionArgParser.cpp
   TestOptionValue.cpp
   TestOptionValueFileColonLine.cpp
+  TestRegexCommand.cpp
 
   LINK_LIBS
       lldbCore
@@ -14,4 +15,5 @@ add_lldb_unittest(InterpreterTests
       lldbUtilityHelpers
       lldbInterpreter
       lldbPluginPlatformMacOSX
+      LLVMTestingSupport
 )

lldb/unittests/Interpreter/TestRegexCommand.cpp

@@ -0,0 +1,68 @@
+//===-- TestRegexCommand.cpp ----------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "Commands/CommandObjectRegexCommand.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+namespace {
+class TestRegexCommand : public CommandObjectRegexCommand {
+public:
+  using CommandObjectRegexCommand::SubstituteVariables;
+
+  static std::string
+  Substitute(llvm::StringRef input,
+             const llvm::SmallVectorImpl<llvm::StringRef> &replacements) {
+    llvm::Expected<std::string> str = SubstituteVariables(input, replacements);
+    if (!str)
+      return llvm::toString(str.takeError());
+    return *str;
+  }
+};
+} // namespace
+
+TEST(RegexCommandTest, SubstituteVariablesSuccess) {
+  const llvm::SmallVector<llvm::StringRef, 4> substitutions = {"all", "foo",
+                                                               "bar", "baz"};
+
+  EXPECT_EQ(TestRegexCommand::Substitute("%0", substitutions), "all");
+  EXPECT_EQ(TestRegexCommand::Substitute("%1", substitutions), "foo");
+  EXPECT_EQ(TestRegexCommand::Substitute("%2", substitutions), "bar");
+  EXPECT_EQ(TestRegexCommand::Substitute("%3", substitutions), "baz");
+  EXPECT_EQ(TestRegexCommand::Substitute("%1%2%3", substitutions), "foobarbaz");
+  EXPECT_EQ(TestRegexCommand::Substitute("#%1#%2#%3#", substitutions),
+            "#foo#bar#baz#");
+}
+
+TEST(RegexCommandTest, SubstituteVariablesFailed) {
+  const llvm::SmallVector<llvm::StringRef, 4> substitutions = {"all", "foo",
+                                                               "bar", "baz"};
+
+  ASSERT_THAT_EXPECTED(
+      TestRegexCommand::SubstituteVariables("%1%2%3%4", substitutions),
+      llvm::Failed());
+  ASSERT_THAT_EXPECTED(
+      TestRegexCommand::SubstituteVariables("%5", substitutions),
+      llvm::Failed());
+  ASSERT_THAT_EXPECTED(
+      TestRegexCommand::SubstituteVariables("%11", substitutions),
+      llvm::Failed());
+}
+
+TEST(RegexCommandTest, SubstituteVariablesNoRecursion) {
+  const llvm::SmallVector<llvm::StringRef, 4> substitutions = {"all", "%2",
+                                                               "%3", "%4"};
+  EXPECT_EQ(TestRegexCommand::Substitute("%0", substitutions), "all");
+  EXPECT_EQ(TestRegexCommand::Substitute("%1", substitutions), "%2");
+  EXPECT_EQ(TestRegexCommand::Substitute("%2", substitutions), "%3");
+  EXPECT_EQ(TestRegexCommand::Substitute("%3", substitutions), "%4");
+  EXPECT_EQ(TestRegexCommand::Substitute("%1%2%3", substitutions), "%2%3%4");
+}