[lldb] Rewrite the 'settings set' command parser

[teemperor]

Summary

For convenience reasons the 'settings set' commands parsing logic is hand-written and doesn't strictly follow
the usual LLDB command rules. The help page gives the following syntax:

settings set [-f] [-g <filename>] -- <setting-variable-name> <value>
settings set <setting-variable-name> <value>

For the most part LLDB follows that syntax, but the current parser for the command syntax has some strange quirks
that are probably best demonstrated with some examples:

# Quotes break the parser
(lldb) settings set "prompt" foo
error: mismatched quotes

# Arguments starting with '-' behind the property name are recognized as command flags.
(lldb) settings set target.run-args -v
error: unknown or ambiguous option

# Even when working around via `--`, the settings write/read functionality doesn't support it.
(lldb) settings set -- target.run-args -v
(lldb) settings show target.run-args 
target.run-args (arguments) =
  [0]: "-v"
(lldb) settings write -f ~/set
(lldb) settings read -f ~/set
Executing commands in '/Users/teemperor/set'.
error: unknown or ambiguous option

# Also using the `-g` with a filename that contains the property name leads to some strange results.
(lldb) settings set -g ~/prompt-settings prompt foo
-settings prompt foo # <- this is the new prompt

We could just change the syntax to what the other 'raw' commands are using, but that would obviously
break everyone's script and all the settings files LLDB has generated for users.

So instead this is a partial rewrite of the old settings parser that tries to support the previous syntax
(with some minor exceptions) but also address the issues above.

In essence the patch does two things:

First we incrementally try to parse the argument list passed by the user until we find the property name.
Only the flags up until the property name are considered command flags as we promised in the help page.
This should fix issues related to passing values starting with - leading to error: unknown or ambiguous option.

Afterwards we explicitly hop over all the command flags we found in the first part instead
of just searching the input string for the property name. This way having the property name
occur in one of the arguments (e.g. -g path/to/property-name.settings) doesn't cause LLDB
to write set the property to whatever follows the first "property name" occurrence.

From what I can see the only use case this breaks is clearing a property value by doing settings set <name> -f
(as now we treat the -f as the value that <name> should be set to).

[jingham]

LGTM, a few trivial comment comments.

lldb/source/Commands/CommandObjectSettings.cpp

169

This is a bit of a weird example, since it doesn't in fact have a settings name. Did you mean to have some settings name between "my-command-args" and "-o"?

176

IIUC, the tricky bit of this routine is that you can't just look at the argument words one by one till you find a setting name, because you might have a setting name as an option value. That's why you have to do this progressive "truncate and parse" strategy. Be a good idea to mention that so somebody doesn't "simplify" your code for you later on.

You explain this below when you actually pull out the settings value. Maybe just refer to that comment?

178

Missing a verb.

219

This wasn't part of your changes, but it's a bit weird to call a property name a "variable" name. Could you fix this while you are here?

[jingham]

One confusion that could arise, however, is that since this is a CommandObjectRaw command, we print this at the end of the help:

This command takes options and free-form arguments.  If your arguments resemble option specifiers (i.e., they start with a - or --), you must use ' -- ' between the end of the command options and the beginning of
the arguments.

which isn't actually true. It would be good to override that automatic not-helpful help insertion. Overriding IsDashDashCommand on the CommandObjectSettingsSet command should turn this off, if I'm reading the code correctly.

lldb/include/lldb/Utility/Args.h

public:
   /// Sets the arguments to match those found in \a argv. All argument strings
   /// will be copied into an internal buffers.
   //
   //  FIXME: Handle the quote character somehow.
   void SetArguments(size_t argc, const char **argv);
 
   void SetArguments(const char **argv);
 
+  /// Shrinks the argument vector to the given size.
+  /// \param len The new size of the argument list. Has to be smaller or equal
+  ///            the current number of arguments.
+  void Shrink(size_t len);
+
   /// Shifts the first argument C string value of the array off the argument
   /// array.
   ///
   /// The string value will be freed, so a copy of the string should be made
   /// by calling Args::GetArgumentAtIndex (size_t) const first and copying the
   /// returned value before calling Args::Shift().
   ///
   /// \see Args::GetArgumentAtIndex (size_t) const

lldb/source/Commands/CommandObjectSettings.cpp

HandleArgumentCompletion(CompletionRequest &request,
     Status error;
     lldb::OptionValueSP value_sp(GetDebugger().GetPropertyValue(
         &m_exe_ctx, setting_var_name, false, error));
     if (!value_sp)
       return;
     value_sp->AutoComplete(m_interpreter, request);
   }
 
+  /// Takes a raw command and parses all arguments of 'settings set' until
+  /// it reaches the settings name. This is necessary to prevent that options
+  /// trailing the settings name are recognized as LLDB command options.
+  /// Example:
+  ///     settings set -f my-command-args -o file.txt
+  /// This function would return ["-f", "my-command-args"].
+  ///
+  /// Note: The option parser is used by this function and it will use the
+  ///       objects shared m_option state. You need to call ParseOptions after
+  ///       calling this function to have the right m_options values.
+  Args ParseArgsPrecedingOptionName(const Args &all_args) {
+    // The only way to find all args and the property name is by iterating
+    // over the args and check if the property parser.
+    for (size_t i = 0 ; i < all_args.size(); ++i) {
+      Args opt_args = all_args;
+      opt_args.Shrink(i);
+      CommandReturnObject result(/*colors=*/false);
+      Args to_parse = opt_args;
+      if (ParseOptions(to_parse, result) && to_parse.size() > 0)
+        return opt_args;
+    }
+    return all_args;
+  }
+
 protected:
   bool DoExecute(llvm::StringRef command,
                  CommandReturnObject &result) override {
-    Args cmd_args(command);
+    // Parse the whole command into an argument list.
+    const Args all_args(command);
 
-    // Process possible options.
-    if (!ParseOptions(cmd_args, result))
+    // Extract all the arguments that don't belong to the property value.
+    const Args opt_args = ParseArgsPrecedingOptionName(all_args);
+
+    // Parse the extracted arguments. ParseOptions will remove any flags/values,
+    // so the remaining option will only be the property name.
+    Args parsed_opt_args = opt_args;
+    if (!ParseOptions(parsed_opt_args, result))
       return false;
 
     const size_t min_argc = m_options.m_force ? 1 : 2;
-    const size_t argc = cmd_args.GetArgumentCount();
+    // The arguments without the value have to be a subset of all args.
+    assert(all_args.size() >= opt_args.size());
+    const size_t argc = all_args.size() - opt_args.size() + 1U;
 
     if ((argc < min_argc) && (!m_options.m_global)) {
       result.AppendError("'settings set' takes more arguments");
       result.SetStatus(eReturnStatusFailed);
       return false;
     }
 
-    const char *var_name = cmd_args.GetArgumentAtIndex(0);
-    if ((var_name == nullptr) || (var_name[0] == '\0')) {
+    llvm::StringRef var_name = parsed_opt_args.GetArgumentAtIndex(0);
+    if (var_name.empty()) {
       result.AppendError(
           "'settings set' command requires a valid variable name");
       result.SetStatus(eReturnStatusFailed);
       return false;
     }
 
     // A missing value corresponds to clearing the setting when "force" is
     // specified.
     if (argc == 1 && m_options.m_force) {
       Status error(GetDebugger().SetPropertyValue(
           &m_exe_ctx, eVarSetOperationClear, var_name, llvm::StringRef()));
       if (error.Fail()) {
         result.AppendError(error.AsCString());
         result.SetStatus(eReturnStatusFailed);
         return false;
       }
       return result.Succeeded();
     }
 
-    // Split the raw command into var_name and value pair.
+    // Extract the value from the command.
+    //
+    // Find the start of the value string in the command line. We can't
+    // just search the command because the option name might also occur in
+    // one of preceding option or option arguments (such as the -g file path
+    // argument). Instead skip over all options were parsed as part of the
+    // option parsing. This should skip until the ^ cursor in the example below:
+    //
+    //     settings set -g my-prompt.lldb prompt "prompt string"
+    //                                           ^
     llvm::StringRef var_value(command);
-    var_value = var_value.split(var_name).second.ltrim();
+    for (const Args::ArgEntry &a : opt_args) {
+      // Find the start of this argument.
+      size_t toDrop = var_value.find(a.ref());
+      lldbassert(toDrop != llvm::StringRef::npos);
+      // Jump to the end of the argument.
+      toDrop += a.ref().size();
+      // If the argument is quoted, jump over the trailing quote character.
+      if (a.IsQuoted())
+        ++toDrop;
+      // Remove this argument from the command string.
+      var_value = var_value.drop_front(toDrop);
+    }
+    var_value = var_value.ltrim();
 
     Status error;
     if (m_options.m_global)
       error = GetDebugger().SetPropertyValue(nullptr, eVarSetOperationAssign,
                                              var_name, var_value);
 
     if (error.Success()) {
       // FIXME this is the same issue as the one in commands script import

lldb/source/Utility/Args.cpp

for (size_t i = 0; i < args.size(); ++i) {
     m_argv[i] = m_entries[i].data();
   }
 }
 
 void Args::SetArguments(const char **argv) {
   SetArguments(ArgvToArgc(argv), argv);
 }
 
+void Args::Shrink(size_t len) {
+  assert(len <= GetArgumentCount() && "Shrink() can't grow Args list");
+  m_entries.resize(len);
+  m_argv.resize(len);
+  // Make sure that m_argv is still terminated by a nullptr.
+  m_argv.push_back(nullptr);
+}
+
 void Args::Clear() {
   m_entries.clear();
   m_argv.clear();
   m_argv.push_back(nullptr);
 }
 
 std::string Args::GetShellSafeArgument(const FileSpec &shell,
                                        llvm::StringRef unsafe_arg) {

lldb/test/API/commands/settings/TestSettings.py

def test_replace_target_run_args(self):
         self.runCmd('settings replace target.run-args 0 A')
         # Check it immediately!
         self.expect('settings show target.run-args',
                     substrs=['target.run-args (arguments) =',
                              '[0]: "A"',
                              '[1]: "b"',
                              '[2]: "c"'])
 
+    def test_set_parsing(self):
+        def expect_prompt_val(test_case, value):
+            self.expect("settings show prompt",
+                        startstr='prompt (string) = "' + value + '"')
+
+        # Test setting values that look like options.
+        self.runCmd("settings set prompt -v")
+        expect_prompt_val(self, "-v")
+
+        self.runCmd("settings set prompt \"-l\"")
+        expect_prompt_val(self, "-l")
+
+        self.runCmd("settings set prompt '-v'")
+        expect_prompt_val(self, "-v")
+
+        self.runCmd("settings set prompt -f")
+        expect_prompt_val(self, "-f")
+
+        self.runCmd("settings set prompt -g foo")
+        expect_prompt_val(self, '-g foo')
+
+        # Test parsing when a double dash is involved.
+        self.runCmd("settings set prompt -- -g foo1")
+        expect_prompt_val(self, '-- -g foo1')
+
+        self.runCmd("settings set -- prompt -- -g foo2")
+        expect_prompt_val(self, '-- -g foo2')
+
+        # Having a double dash before the property name is supported.
+        self.runCmd("settings set -- prompt -g foo3")
+        expect_prompt_val(self, '-g foo3')
+
+        self.runCmd("settings set -f -- prompt -g foo4")
+        expect_prompt_val(self, '-g foo4')
+
+        # A double dash indicates that the next arg is the property name.
+        self.expect("settings set -- -f prompt val", error=True,
+                    substrs=["invalid value path '-f'"])
+
+        # Test setting a variable name in quotes.
+        self.runCmd("settings set 'prompt' foo1")
+        expect_prompt_val(self, "foo1")
+
+        self.runCmd("settings set \"prompt\" foo2")
+        expect_prompt_val(self, "foo2")
+
+        # Test setting a value that also occurs in the preceding argument.
+        settings_file = self.getBuildArtifact("prompt.settings")
+        self.runCmd("settings set -g \"" + settings_file + "\" prompt foo")
+        expect_prompt_val(self, "foo")
+
     def test_set_prompt(self):
         """Test that 'set prompt' actually changes the prompt."""
 
         # Set prompt to 'lldb2'.
         self.runCmd("settings set prompt 'lldb2 '")
 
         # Immediately test the setting.
         self.expect("settings show prompt", SETTING_MSG("prompt"),

lldb/unittests/Utility/ArgsTest.cpp

 TEST(ArgsTest, ReplaceArgumentAtIndexFarOutOfRange) {
   Args args;
   args.SetCommandString("foo ba b");
   args.ReplaceArgumentAtIndex(4, "baar");
   EXPECT_EQ(3u, args.GetArgumentCount());
   EXPECT_STREQ(args.GetArgumentAtIndex(2), "b");
 }
 
+TEST(ArgsTest, Shrink) {
+  Args args;
+  args.SetCommandString("foo ba b");
+  args.Shrink(3);
+  EXPECT_STREQ("b", args.GetArgumentAtIndex(2));
+  EXPECT_STREQ("ba", args.GetArgumentAtIndex(1));
+  EXPECT_STREQ("foo", args.GetArgumentAtIndex(0));
+  EXPECT_EQ(3U, args.size());
+
+  args.Shrink(2);
+  EXPECT_STREQ("ba", args.GetArgumentAtIndex(1));
+  EXPECT_STREQ("foo", args.GetArgumentAtIndex(0));
+  EXPECT_EQ(2U, args.size());
+
+  args.Shrink(0);
+  EXPECT_EQ(0U, args.size());
+}
+
 TEST(ArgsTest, Yaml) {
   std::string buffer;
   llvm::raw_string_ostream os(buffer);
 
   // Serialize.
   Args args;
   args.SetCommandString("this 'has' \"multiple\" args");
   llvm::yaml::Output yout(os);