Move option parsing out of the Args class

[labath]

Summary

The args class is used in plenty of places (a lot of them in the lower lldb
layers) for representing a list of arguments, and most of these places don't
care about option parsing. Moving the option parsing out of the class removes
the largest external dependency (there are a couple more, but these are in
static functions), and brings us closer to being able to move it to the
Utility module).

The new home for these functions is the Options class, which was already used
as an argument to the parse calls, so this just inverts the dependency between
the two.

The functions are themselves are mainly just copied -- the biggest functional
change I've made to them is to avoid modifying the input Args argument (getopt
likes to permute the argument vector), as it was weird to have another class
reorder the entries in Args class. So now the functions don't modify the input
arguments, and (for those where it makes sense) return a new Args vector
instead. I've also made the addition of a "fake arg0" (required for getopt
compatibility) an implementation detail rather than a part of interface.

While doing that I noticed that ParseForCompletion function was recording the
option indexes in the shuffled vector, but then the consumer was looking up the
entries in the unshuffled one. This manifested itself as us not being able to
complete "watchpoint set variable foo --" (because getopt would move "foo" to
the end). Surprisingly all other completions (e.g. "watchpoint set variable foo
--w") were not affected by this. However, I couldn't find a comprehensive test
for command argument completion, so I consolidated the existing tests and added
a bunch of new ones.

[jingham]

I don't have any objections to the contents of this patch per se. But I wonder if having to do all this work to separate the uses of Args from the options parser so we don't drag it in in some low level uses doesn't rather mean the Args class was not appropriate for use at that level, when what they really meant was just a collection of strings. For instance, we use the Args class to hold arguments to pass to running processes. That's convenient when we parse them from commands into run-args, but hardly essential to the function of passing a collection of strings to posix_spawnp or its like.

I haven't gone through and audited all the uses you are trying to separate out from the options part of Args, but if possible it would be cleaner to have the class that's supposed to be cheek to jowl with the command line parsing, and another to store a list of strings.

[labath]

In D43837#1022303, @jingham wrote:

I don't have any objections to the contents of this patch per se. But I wonder if having to do all this work to separate the uses of Args from the options parser so we don't drag it in in some low level uses doesn't rather mean the Args class was not appropriate for use at that level, when what they really meant was just a collection of strings. For instance, we use the Args class to hold arguments to pass to running processes. That's convenient when we parse them from commands into run-args, but hardly essential to the function of passing a collection of strings to posix_spawnp or its like.

I haven't gone through and audited all the uses you are trying to separate out from the options part of Args, but if possible it would be cleaner to have the class that's supposed to be cheek to jowl with the command line parsing, and another to store a list of strings.

I was thinking about that as well, but eventually I chose this approach. The reason for that is that there is already functionality in the Args class that is useful for posix_spawn and friends, and that's the ability to turn itself into an argv vector. So, the new class couldn't just be a vector<string>, but it would need some additional smartness. So after implementing that, I think I would need to find a way to rip that code out of the Args class (maybe by making the new class a member of Args). So the end result may end up being very similar, we would just reach in it a different way.

The other reason I want to move this out is the single responsibility principle. Right now, I can identify about 4 responsibilities of the Args class:

• being a representation of a list of arguments (I put it as a separate item because of the argv requirement)
• parsing a string into a list of arguments
• parsing a list of arguments into options
• parsing a string into random other objects (via various static functions)

Now we probably don't want to go all out and split this into 4 classes, but I figured the first two items are enough for a single class (one could even argue the two items are actually a single thing). I think parsing a string into args and parsing args into options are two sufficiently complicated algorithms that makes sense to keep them separate.

The thing I'm not 100% clear on is whether the Options class is the best home for these functions. The reason I chose this at the end (instead of e,g, putting it in a new class) was because I saw this pattern in the CommandObject:

options->NotifyOptionParsingStarting(&exe_ctx);
...
options->Parse(...);
...
options->NotifyOptionParsingFinished(&exe_ctx);

It seemed to be that having these functions in the Options class would open up possibilities for simplifying the Options interface by folding the Notify functions into the Parse call.

[jingham]

I still worry a bit because there's another unstated responsibility for Args which is that even though it is going to get used at a very high level in lldb it has to NOT depend on anything you don't want lldb-server to depend on. That seems like a more slippery responsibility, and one that's worth stating explicitly by making the part of Args that gets used in lldb-server its own class. But on the implementation wins principle, as long as this doesn't worry you, I'm content.

BTW, most the string -> int/address/whatever conversion functions don't belong in Args at all. It makes these fairly useful functions hard to find, and we should have some string conversion utility to hold all these convenience functions. There are a few (like picking an enum option string from a set of enums) that belong more properly in options. But most of them have no real relation to Args. But that's an orthogonal issue.

[zturner]

include/lldb/Interpreter/Options.h

123–126 ↗

(On Diff #136168)

It appears that all of these could be static functions. Can we do that?

[labath]

In D43837#1022430, @jingham wrote:

I still worry a bit because there's another unstated responsibility for Args which is that even though it is going to get used at a very high level in lldb it has to NOT depend on anything you don't want lldb-server to depend on.

This will be enforced by moving the class (once I get rid of the extra static functions like you mentioned) into the Utility module. Nothing in the Utility module can depend on anything outside of that module.

include/lldb/Interpreter/Options.h

123–126 ↗

(On Diff #136168)

They can't be. All of them access the this object. If you look at the original functions, they were taking an Options& as an argument and Args as this. These have that inverted.

[zturner]

It would be nice if we could eventually get rid of the need to pass in a platform and execution context here, but that's work for another day.

include/lldb/Interpreter/Options.h

123–126 ↗

(On Diff #136168)

I originally searched for m_ and didn't find anything so assumed they could be static. But it looks like they are calling member functions, which is why I didn't see it. It's too bad it can't even be const, given that it returns a copy of the args. Seems like an awkward interface, maybe future cleanup can try to tackle that though. Anyway, ignore my comment.

[labath]

include/lldb/Interpreter/Options.h

123–126 ↗

(On Diff #136168)

The returning of args is kind of a side effect of the function. The return value contains the arguments that could not be parsed into options (more precisely, the positional arguments). The main effect of the function is that it populates the Options object with the options that have been parsed. (I should probably add that to the comment).

[jingham]

Okay, that sounds good then. Will you enforce the rule about the Utilities directory socially or by some mechanism?

[zturner]

In D43837#1022554, @jingham wrote:

Okay, that sounds good then. Will you enforce the rule about the Utilities directory socially or by some mechanism?

If you mean the rule that Utility can't depend on anything else, I think it's enforced implicitly. because we have a UtilityTests unit test that only links against Utility and nothing else. If anything else gets brought in, this binary will fail to link.

That said, a while back I wrote a python script (checked in under lldb/scripts) which dumps the set of header dependencies for every project. It would perhaps be a useful exercise to write a lit test that enforces this by dumping the deps of Utility and ensuring that they're empty.

[labath]

In D43837#1022554, @jingham wrote:

Okay, that sounds good then. Will you enforce the rule about the Utilities directory socially or by some mechanism?

You will get a link error because the Utility unittest executable will have missing symbols. I'm not sure how xcode builds the unit test executables; if it goes through cmake you will definitely get it locally; if not, at the very latest you will get it from one of the bots.

[labath]

Add a slightly longer comment to the Parse function operation.

[jingham]

The separate gtest directories don't get build separately the way Todd set the Xcode project up. The tests get built into one combo .a file that links to liblldbcore.a, and then into the test binary. So the Xcode build wouldn't see that failure.

Since you are building .a files not dylibs from Utility, the UtilityTest won't show a failure for a class that has a dependency that the tests binary doesn't use, so this sort of thing could creep in. Another reason to be vigilant about the test coverage.

Once your whole project is done, lldb-server shouldn't build if something it uses from Utility uses something not in Utility (or in some other .a file that lldb-server depends on). That doesn't help altogether, since macOS only builds the platform part of lldb-server, so the macOS builds won't check usage in the other parts of lldb-server. But the bots should be good enough.

[labath]

Is there anything else you wanted me to do here?

[jingham]

Nope.

lldb/trunk/include/lldb/Interpreter/Args.h

 // Project includes
 #include "lldb/Utility/Environment.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/lldb-private-types.h"
 #include "lldb/lldb-types.h"
 
 namespace lldb_private {
 
-struct Option;
-
 typedef std::vector<std::tuple<std::string, int, std::string>> OptionArgVector;
 typedef std::shared_ptr<OptionArgVector> OptionArgVectorSP;
 
 struct OptionArgElement {
   enum { eUnrecognizedArg = -1, eBareDash = -2, eBareDoubleDash = -3 };
 
   OptionArgElement(int defs_index, int pos, int arg_pos)
       : opt_defs_index(defs_index), opt_pos(pos), opt_arg_pos(arg_pos) {}
public:
   ///     The NULL terminated C string argument pointer if \a idx is a
   ///     valid argument index, NULL otherwise.
   //------------------------------------------------------------------
   const char *GetArgumentAtIndex(size_t idx) const;
 
   llvm::ArrayRef<ArgEntry> entries() const { return m_entries; }
   char GetArgumentQuoteCharAtIndex(size_t idx) const;
 
-  std::vector<ArgEntry>::const_iterator begin() const {
-    return m_entries.begin();
-  }
-  std::vector<ArgEntry>::const_iterator end() const { return m_entries.end(); }
+  using const_iterator = std::vector<ArgEntry>::const_iterator;
+
+  const_iterator begin() const { return m_entries.begin(); }
+  const_iterator end() const { return m_entries.end(); }
 
   size_t size() const { return GetArgumentCount(); }
   const ArgEntry &operator[](size_t n) const { return m_entries[n]; }
 
   //------------------------------------------------------------------
   /// Gets the argument vector.
   ///
   /// The value returned by this function can be used by any function
public:
   ///     The argument to push on the front of the argument stack.
   ///
   /// @param[in] quote_char
   ///     If the argument was originally quoted, put in the quote char here.
   //------------------------------------------------------------------
   void Unshift(llvm::StringRef arg_str, char quote_char = '\0');
 
   //------------------------------------------------------------------
-  /// Parse the arguments in the contained arguments.
-  ///
-  /// The arguments that are consumed by the argument parsing process
-  /// will be removed from the argument vector. The arguments that
-  /// get processed start at the second argument. The first argument
-  /// is assumed to be the command and will not be touched.
-  ///
-  /// param[in] platform_sp
-  ///   The platform used for option validation.  This is necessary
-  ///   because an empty execution_context is not enough to get us
-  ///   to a reasonable platform.  If the platform isn't given,
-  ///   we'll try to get it from the execution context.  If we can't
-  ///   get it from the execution context, we'll skip validation.
-  ///
-  /// param[in] require_validation
-  ///   When true, it will fail option parsing if validation could
-  ///   not occur due to not having a platform.
-  ///
-  /// @see class Options
-  //------------------------------------------------------------------
-  Status ParseOptions(Options &options, ExecutionContext *execution_context,
-                      lldb::PlatformSP platform_sp, bool require_validation);
-
-  bool IsPositionalArgument(const char *arg);
-
-  // The following works almost identically to ParseOptions, except that no
-  // option is required to have arguments, and it builds up the
-  // option_arg_vector as it parses the options.
-
-  std::string ParseAliasOptions(Options &options, CommandReturnObject &result,
-                                OptionArgVector *option_arg_vector,
-                                llvm::StringRef raw_input_line);
-
-  void ParseArgsForCompletion(Options &options,
-                              OptionElementVector &option_element_vector,
-                              uint32_t cursor_index);
-
-  //------------------------------------------------------------------
   // Clear the arguments.
   //
   // For re-setting or blanking out the list of arguments.
   //------------------------------------------------------------------
   void Clear();
 
   static const char *StripSpaces(std::string &s, bool leading = true,
                                  bool trailing = true,
public:
   // help for EncodeEscapeSequences, then it will be encoded as octal. Printable
   // characters are left alone.
   static void ExpandEscapedCharacters(const char *src, std::string &dst);
 
   static std::string EscapeLLDBCommandArgument(const std::string &arg,
                                                char quote_char);
 
 private:
-  size_t FindArgumentIndexForOption(Option *long_options,
-                                    int long_options_index) const;
-
   std::vector<ArgEntry> m_entries;
   std::vector<char *> m_argv;
-
-  void UpdateArgsAfterOptionParsing();
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_Command_h_

lldb/trunk/include/lldb/Interpreter/Options.h

 #include "lldb/Interpreter/Args.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-private.h"
 
 #include "llvm/ADT/ArrayRef.h"
 
 namespace lldb_private {
 
+struct Option;
+
 static inline bool isprint8(int ch) {
   if (ch & 0xffffff00u)
     return false;
   return isprint(ch);
 }
 
 //----------------------------------------------------------------------
 /// @class Options Options.h "lldb/Interpreter/Options.h"
 /// @brief A command line option parsing protocol class.
 ///
 /// Options is designed to be subclassed to contain all needed
-/// options for a given command. The options can be parsed by calling:
-/// \code
-///     Status Args::ParseOptions (Options &);
-/// \endcode
+/// options for a given command. The options can be parsed by calling the Parse
+/// function.
 ///
 /// The options are specified using the format defined for the libc
 /// options parsing function getopt_long_only:
 /// \code
 ///     #include <getopt.h>
 ///     int getopt_long_only(int argc, char * const *argv, const char
 ///     *optstring, const struct option *longopts, int *longindex);
 /// \endcode
 ///
-/// Example code:
-/// \code
-///     #include <getopt.h>
-///     #include <string>
-///
-///     class CommandOptions : public Options
-///     {
-///     public:
-///         virtual struct option *
-///         GetLongOptions() {
-///             return g_options;
-///         }
-///
-///         virtual Status
-///         SetOptionValue (uint32_t option_idx, int option_val, const char
-///         *option_arg)
-///         {
-///             Status error;
-///             switch (option_val)
-///             {
-///             case 'g': debug = true; break;
-///             case 'v': verbose = true; break;
-///             case 'l': log_file = option_arg; break;
-///             case 'f': log_flags = strtoull(option_arg, nullptr, 0); break;
-///             default:
-///                 error.SetErrorStringWithFormat("unrecognized short option
-///                 %c", option_val);
-///                 break;
-///             }
-///
-///             return error;
-///         }
-///
-///         CommandOptions (CommandInterpreter &interpreter) : debug (true),
-///         verbose (false), log_file (), log_flags (0)
-///         {}
-///
-///         bool debug;
-///         bool verbose;
-///         std::string log_file;
-///         uint32_t log_flags;
-///
-///         static struct option g_options[];
-///
-///     };
-///
-///     struct option CommandOptions::g_options[] =
-///     {
-///         { "debug",              no_argument,        nullptr,   'g' },
-///         { "log-file",           required_argument,  nullptr,   'l' },
-///         { "log-flags",          required_argument,  nullptr,   'f' },
-///         { "verbose",            no_argument,        nullptr,   'v' },
-///         { nullptr,              0,                  nullptr,   0   }
-///     };
-///
-///     int main (int argc, const char **argv, const char **envp)
-///     {
-///         CommandOptions options;
-///         Args main_command;
-///         main_command.SetArguments(argc, argv, false);
-///         main_command.ParseOptions(options);
-///
-///         if (options.verbose)
-///         {
-///             std::cout << "verbose is on" << std::endl;
-///         }
-///     }
-/// \endcode
 //----------------------------------------------------------------------
 class Options {
 public:
   Options();
 
   virtual ~Options();
 
   void BuildGetoptTable();
public:
 
   // Call this prior to parsing any options. This call will call the
   // subclass OptionParsingStarting() and will avoid the need for all
   // OptionParsingStarting() function instances from having to call the
   // Option::OptionParsingStarting() like they did before. This was error
   // prone and subclasses shouldn't have to do it.
   void NotifyOptionParsingStarting(ExecutionContext *execution_context);
 
+  //------------------------------------------------------------------
+  /// Parse the provided arguments.
+  ///
+  /// The parsed options are set via calls to SetOptionValue. In case of a
+  /// successful parse, the function returns a copy of the input arguments with
+  /// the parsed options removed. Otherwise, it returns an error.
+  ///
+  /// param[in] platform_sp
+  ///   The platform used for option validation.  This is necessary
+  ///   because an empty execution_context is not enough to get us
+  ///   to a reasonable platform.  If the platform isn't given,
+  ///   we'll try to get it from the execution context.  If we can't
+  ///   get it from the execution context, we'll skip validation.
+  ///
+  /// param[in] require_validation
+  ///   When true, it will fail option parsing if validation could
+  ///   not occur due to not having a platform.
+  //------------------------------------------------------------------
+  llvm::Expected<Args> Parse(const Args &args,
+                             ExecutionContext *execution_context,
+                             lldb::PlatformSP platform_sp,
+                             bool require_validation);
+
+  llvm::Expected<Args> ParseAlias(const Args &args,
+                                  OptionArgVector *option_arg_vector,
+                                  std::string &input_line);
+
+  OptionElementVector ParseForCompletion(const Args &args,
+                                         uint32_t cursor_index);
+
   Status NotifyOptionParsingFinished(ExecutionContext *execution_context);
 
   //------------------------------------------------------------------
   /// Set the value of an option.
   ///
   /// @param[in] option_idx
   ///     The index into the "struct option" array that was returned
   ///     by Options::GetLongOptions().

lldb/trunk/packages/Python/lldbsuite/test/functionalities/completion/TestCompletion.py

def test_watchpoint_command_a(self):
         """Test that 'watchpoint command a' completes to 'watchpoint command add '."""
         self.complete_from_to(
             'watchpoint command a',
             'watchpoint command add ')
 
     @expectedFailureAll(hostoslist=["windows"], bugnumber="llvm.org/pr24679")
     @skipIfFreeBSD  # timing out on the FreeBSD buildbot
     @no_debug_info_test
-    def test_watchpoint_set_variable_dash_w(self):
-        """Test that 'watchpoint set variable -w' completes to 'watchpoint set variable -w '."""
-        self.complete_from_to(
-            'watchpoint set variable -w',
-            'watchpoint set variable -w ')
-
-    @expectedFailureAll(hostoslist=["windows"], bugnumber="llvm.org/pr24679")
-    @skipIfFreeBSD  # timing out on the FreeBSD buildbot
-    @no_debug_info_test
-    def test_watchpoint_set_variable_dash_w_space(self):
-        """Test that 'watchpoint set variable -w ' completes to ['read', 'write', 'read_write']."""
-        self.complete_from_to('watchpoint set variable -w ',
-                              ['read', 'write', 'read_write'])
-
-    @expectedFailureAll(hostoslist=["windows"], bugnumber="llvm.org/pr24679")
-    @skipIfFreeBSD  # timing out on the FreeBSD buildbot
-    @no_debug_info_test
     def test_watchpoint_set_ex(self):
         """Test that 'watchpoint set ex' completes to 'watchpoint set expression '."""
         self.complete_from_to(
             'watchpoint set ex',
             'watchpoint set expression ')
 
     @expectedFailureAll(hostoslist=["windows"], bugnumber="llvm.org/pr24679")
     @skipIfFreeBSD  # timing out on the FreeBSD buildbot
     @no_debug_info_test
     def test_watchpoint_set_var(self):
         """Test that 'watchpoint set var' completes to 'watchpoint set variable '."""
         self.complete_from_to('watchpoint set var', 'watchpoint set variable ')
 
     @expectedFailureAll(hostoslist=["windows"], bugnumber="llvm.org/pr24679")
     @skipIfFreeBSD  # timing out on the FreeBSD buildbot
     @no_debug_info_test
-    def test_watchpoint_set_variable_dash_w_read_underbar(self):
-        """Test that 'watchpoint set variable -w read_' completes to 'watchpoint set variable -w read_write'."""
-        self.complete_from_to(
-            'watchpoint set variable -w read_',
-            'watchpoint set variable -w read_write')
-
-    @expectedFailureAll(hostoslist=["windows"], bugnumber="llvm.org/pr24679")
-    @skipIfFreeBSD  # timing out on the FreeBSD buildbot
-    @no_debug_info_test
     def test_help_fi(self):
         """Test that 'help fi' completes to ['file', 'finish']."""
         self.complete_from_to(
             'help fi', [
                 'file', 'finish'])
 
     @expectedFailureAll(hostoslist=["windows"], bugnumber="llvm.org/pr24679")
     @skipIfFreeBSD  # timing out on the FreeBSD buildbot
class CommandLineCompletionTestCase(TestBase):
 
     @expectedFailureAll(hostoslist=["windows"], bugnumber="llvm.org/pr24679")
     @skipIfFreeBSD  # timing out on the FreeBSD buildbot
     @no_debug_info_test
     def test_target_va(self):
         """Test that 'target va' completes to 'target variable '."""
         self.complete_from_to('target va', 'target variable ')
 
+    def test_command_argument_completion(self):
+        """Test completion of command arguments"""
+        self.complete_from_to("watchpoint set variable -", ["-w", "-s"])
+        self.complete_from_to('watchpoint set variable -w', 'watchpoint set variable -w ')
+        self.complete_from_to("watchpoint set variable --", ["--watch", "--size"])
+        self.complete_from_to("watchpoint set variable --w", "watchpoint set variable --watch")
+        self.complete_from_to('watchpoint set variable -w ', ['read', 'write', 'read_write'])
+        self.complete_from_to("watchpoint set variable --watch ", ["read", "write", "read_write"])
+        self.complete_from_to("watchpoint set variable --watch w", "watchpoint set variable --watch write")
+        self.complete_from_to('watchpoint set variable -w read_', 'watchpoint set variable -w read_write')
+        # Now try the same thing with a variable name (non-option argument) to
+        # test that getopts arg reshuffling doesn't confuse us.
+        self.complete_from_to("watchpoint set variable foo -", ["-w", "-s"])
+        self.complete_from_to('watchpoint set variable foo -w', 'watchpoint set variable foo -w ')
+        self.complete_from_to("watchpoint set variable foo --", ["--watch", "--size"])
+        self.complete_from_to("watchpoint set variable foo --w", "watchpoint set variable foo --watch")
+        self.complete_from_to('watchpoint set variable foo -w ', ['read', 'write', 'read_write'])
+        self.complete_from_to("watchpoint set variable foo --watch ", ["read", "write", "read_write"])
+        self.complete_from_to("watchpoint set variable foo --watch w", "watchpoint set variable foo --watch write")
+        self.complete_from_to('watchpoint set variable foo -w read_', 'watchpoint set variable foo -w read_write')
+
     @expectedFailureAll(hostoslist=["windows"], bugnumber="llvm.org/pr24679")
     def test_symbol_name(self):
         self.build()
         self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
         self.complete_from_to('breakpoint set -n Fo',
                               'breakpoint set -n Foo::Bar(int,\\ int)',
                               turn_off_re_match=True)
 

lldb/trunk/source/Commands/CommandObjectSettings.cpp

int HandleArgumentCompletion(Args &input, int &cursor_index,
                                bool &word_complete,
                                StringList &matches) override {
     std::string completion_str(input.GetArgumentAtIndex(cursor_index),
                                cursor_char_position);
 
     const size_t argc = input.GetArgumentCount();
     const char *arg = nullptr;
     int setting_var_idx;
-    for (setting_var_idx = 1; setting_var_idx < static_cast<int>(argc);
+    for (setting_var_idx = 0; setting_var_idx < static_cast<int>(argc);
          ++setting_var_idx) {
       arg = input.GetArgumentAtIndex(setting_var_idx);
       if (arg && arg[0] != '-')
         break; // We found our setting variable name index
     }
     if (cursor_index == setting_var_idx) {
       // Attempting to complete setting variable name
       CommandCompletions::InvokeCommonCompletionCallbacks(

lldb/trunk/source/Interpreter/Args.cpp

 //===-- Args.cpp ------------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 // C Includes
 #include <cstdlib>
 // C++ Includes
 // Other libraries and framework includes
 // Project includes
 #include "lldb/DataFormatters/FormatManager.h"
-#include "lldb/Host/OptionParser.h"
 #include "lldb/Interpreter/Args.h"
-#include "lldb/Interpreter/CommandInterpreter.h"
-#include "lldb/Interpreter/CommandReturnObject.h"
-#include "lldb/Interpreter/Options.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/StreamString.h"
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 
while (!command.empty()) {
     std::tie(arg, quote, command) = ParseSingleArgument(command);
     m_entries.emplace_back(arg, quote);
     m_argv.push_back(m_entries.back().data());
     command = command.ltrim(k_space_separators);
   }
   m_argv.push_back(nullptr);
 }
 
-void Args::UpdateArgsAfterOptionParsing() {
-  assert(!m_argv.empty());
-  assert(m_argv.back() == nullptr);
-
-  // Now m_argv might be out of date with m_entries, so we need to fix that.
-  // This happens because getopt_long_only may permute the order of the
-  // arguments in argv, so we need to re-order the quotes and the refs array
-  // to match.
-  for (size_t i = 0; i < m_argv.size() - 1; ++i) {
-    const char *argv = m_argv[i];
-    auto pos =
-        std::find_if(m_entries.begin() + i, m_entries.end(),
-                     [argv](const ArgEntry &D) { return D.c_str() == argv; });
-    assert(pos != m_entries.end());
-    size_t distance = std::distance(m_entries.begin(), pos);
-    if (i == distance)
-      continue;
-
-    assert(distance > i);
-
-    std::swap(m_entries[i], m_entries[distance]);
-    assert(m_entries[i].ref.data() == m_argv[i]);
-  }
-  m_entries.resize(m_argv.size() - 1);
-}
-
 size_t Args::GetArgumentCount() const { return m_entries.size(); }
 
 const char *Args::GetArgumentAtIndex(size_t idx) const {
   if (idx < m_argv.size())
     return m_argv[idx];
   return nullptr;
 }
 
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);
 }
 
-Status Args::ParseOptions(Options &options, ExecutionContext *execution_context,
-                          PlatformSP platform_sp, bool require_validation) {
-  StreamString sstr;
-  Status error;
-  Option *long_options = options.GetLongOptions();
-  if (long_options == nullptr) {
-    error.SetErrorStringWithFormat("invalid long options");
-    return error;
-  }
-
-  for (int i = 0; long_options[i].definition != nullptr; ++i) {
-    if (long_options[i].flag == nullptr) {
-      if (isprint8(long_options[i].val)) {
-        sstr << (char)long_options[i].val;
-        switch (long_options[i].definition->option_has_arg) {
-        default:
-        case OptionParser::eNoArgument:
-          break;
-        case OptionParser::eRequiredArgument:
-          sstr << ':';
-          break;
-        case OptionParser::eOptionalArgument:
-          sstr << "::";
-          break;
-        }
-      }
-    }
-  }
-  std::unique_lock<std::mutex> lock;
-  OptionParser::Prepare(lock);
-  int val;
-  while (1) {
-    int long_options_index = -1;
-    val = OptionParser::Parse(GetArgumentCount(), GetArgumentVector(),
-                              sstr.GetString(), long_options,
-                              &long_options_index);
-    if (val == -1)
-      break;
-
-    // Did we get an error?
-    if (val == '?') {
-      error.SetErrorStringWithFormat("unknown or ambiguous option");
-      break;
-    }
-    // The option auto-set itself
-    if (val == 0)
-      continue;
-
-    ((Options *)&options)->OptionSeen(val);
-
-    // Lookup the long option index
-    if (long_options_index == -1) {
-      for (int i = 0; long_options[i].definition || long_options[i].flag ||
-                      long_options[i].val;
-           ++i) {
-        if (long_options[i].val == val) {
-          long_options_index = i;
-          break;
-        }
-      }
-    }
-    // Call the callback with the option
-    if (long_options_index >= 0 &&
-        long_options[long_options_index].definition) {
-      const OptionDefinition *def = long_options[long_options_index].definition;
-
-      if (!platform_sp) {
-        // User did not pass in an explicit platform.  Try to grab
-        // from the execution context.
-        TargetSP target_sp =
-            execution_context ? execution_context->GetTargetSP() : TargetSP();
-        platform_sp = target_sp ? target_sp->GetPlatform() : PlatformSP();
-      }
-      OptionValidator *validator = def->validator;
-
-      if (!platform_sp && require_validation) {
-        // Caller requires validation but we cannot validate as we
-        // don't have the mandatory platform against which to
-        // validate.
-        error.SetErrorString("cannot validate options: "
-                             "no platform available");
-        return error;
-      }
-
-      bool validation_failed = false;
-      if (platform_sp) {
-        // Ensure we have an execution context, empty or not.
-        ExecutionContext dummy_context;
-        ExecutionContext *exe_ctx_p =
-            execution_context ? execution_context : &dummy_context;
-        if (validator && !validator->IsValid(*platform_sp, *exe_ctx_p)) {
-          validation_failed = true;
-          error.SetErrorStringWithFormat("Option \"%s\" invalid.  %s",
-                                         def->long_option,
-                                         def->validator->LongConditionString());
-        }
-      }
-
-      // As long as validation didn't fail, we set the option value.
-      if (!validation_failed)
-        error = options.SetOptionValue(
-            long_options_index,
-            (def->option_has_arg == OptionParser::eNoArgument)
-                ? nullptr
-                : OptionParser::GetOptionArgument(),
-            execution_context);
-    } else {
-      error.SetErrorStringWithFormat("invalid option with value '%i'", val);
-    }
-    if (error.Fail())
-      break;
-  }
-
-  // Update our ARGV now that get options has consumed all the options
-  m_argv.erase(m_argv.begin(), m_argv.begin() + OptionParser::GetOptionIndex());
-  UpdateArgsAfterOptionParsing();
-  return error;
-}
-
 void Args::Clear() {
   m_entries.clear();
   m_argv.clear();
   m_argv.push_back(nullptr);
 }
 
 lldb::addr_t Args::StringToAddress(const ExecutionContext *exe_ctx,
                                    llvm::StringRef s, lldb::addr_t fail_value,
uint32_t result = llvm::StringSwitch<uint32_t>(s)
                         .Case("arg5", LLDB_REGNUM_GENERIC_ARG5)
                         .Case("arg6", LLDB_REGNUM_GENERIC_ARG6)
                         .Case("arg7", LLDB_REGNUM_GENERIC_ARG7)
                         .Case("arg8", LLDB_REGNUM_GENERIC_ARG8)
                         .Default(LLDB_INVALID_REGNUM);
   return result;
 }
 
-size_t Args::FindArgumentIndexForOption(Option *long_options,
-                                        int long_options_index) const {
-  char short_buffer[3];
-  char long_buffer[255];
-  ::snprintf(short_buffer, sizeof(short_buffer), "-%c",
-             long_options[long_options_index].val);
-  ::snprintf(long_buffer, sizeof(long_buffer), "--%s",
-             long_options[long_options_index].definition->long_option);
-
-  for (auto entry : llvm::enumerate(m_entries)) {
-    if (entry.value().ref.startswith(short_buffer) ||
-        entry.value().ref.startswith(long_buffer))
-      return entry.index();
-  }
-
-  return size_t(-1);
-}
-
-bool Args::IsPositionalArgument(const char *arg) {
-  if (arg == nullptr)
-    return false;
-
-  bool is_positional = true;
-  const char *cptr = arg;
-
-  if (cptr[0] == '%') {
-    ++cptr;
-    while (isdigit(cptr[0]))
-      ++cptr;
-    if (cptr[0] != '\0')
-      is_positional = false;
-  } else
-    is_positional = false;
-
-  return is_positional;
-}
-
-std::string Args::ParseAliasOptions(Options &options,
-                                    CommandReturnObject &result,
-                                    OptionArgVector *option_arg_vector,
-                                    llvm::StringRef raw_input_string) {
-  std::string result_string(raw_input_string);
-  StreamString sstr;
-  int i;
-  Option *long_options = options.GetLongOptions();
-
-  if (long_options == nullptr) {
-    result.AppendError("invalid long options");
-    result.SetStatus(eReturnStatusFailed);
-    return result_string;
-  }
-
-  for (i = 0; long_options[i].definition != nullptr; ++i) {
-    if (long_options[i].flag == nullptr) {
-      sstr << (char)long_options[i].val;
-      switch (long_options[i].definition->option_has_arg) {
-      default:
-      case OptionParser::eNoArgument:
-        break;
-      case OptionParser::eRequiredArgument:
-        sstr << ":";
-        break;
-      case OptionParser::eOptionalArgument:
-        sstr << "::";
-        break;
-      }
-    }
-  }
-
-  std::unique_lock<std::mutex> lock;
-  OptionParser::Prepare(lock);
-  result.SetStatus(eReturnStatusSuccessFinishNoResult);
-  int val;
-  while (1) {
-    int long_options_index = -1;
-    val = OptionParser::Parse(GetArgumentCount(), GetArgumentVector(),
-                              sstr.GetString(), long_options,
-                              &long_options_index);
-
-    if (val == -1)
-      break;
-
-    if (val == '?') {
-      result.AppendError("unknown or ambiguous option");
-      result.SetStatus(eReturnStatusFailed);
-      break;
-    }
-
-    if (val == 0)
-      continue;
-
-    options.OptionSeen(val);
-
-    // Look up the long option index
-    if (long_options_index == -1) {
-      for (int j = 0; long_options[j].definition || long_options[j].flag ||
-                      long_options[j].val;
-           ++j) {
-        if (long_options[j].val == val) {
-          long_options_index = j;
-          break;
-        }
-      }
-    }
-
-    // See if the option takes an argument, and see if one was supplied.
-    if (long_options_index == -1) {
-      result.AppendErrorWithFormat("Invalid option with value '%c'.\n", val);
-      result.SetStatus(eReturnStatusFailed);
-      return result_string;
-    }
-
-    StreamString option_str;
-    option_str.Printf("-%c", val);
-    const OptionDefinition *def = long_options[long_options_index].definition;
-    int has_arg =
-        (def == nullptr) ? OptionParser::eNoArgument : def->option_has_arg;
-
-    const char *option_arg = nullptr;
-    switch (has_arg) {
-    case OptionParser::eRequiredArgument:
-      if (OptionParser::GetOptionArgument() == nullptr) {
-        result.AppendErrorWithFormat(
-            "Option '%s' is missing argument specifier.\n",
-            option_str.GetData());
-        result.SetStatus(eReturnStatusFailed);
-        return result_string;
-      }
-      LLVM_FALLTHROUGH;
-    case OptionParser::eOptionalArgument:
-      option_arg = OptionParser::GetOptionArgument();
-      LLVM_FALLTHROUGH;
-    case OptionParser::eNoArgument:
-      break;
-    default:
-      result.AppendErrorWithFormat("error with options table; invalid value "
-                                   "in has_arg field for option '%c'.\n",
-                                   val);
-      result.SetStatus(eReturnStatusFailed);
-      return result_string;
-    }
-    if (!option_arg)
-      option_arg = "<no-argument>";
-    option_arg_vector->emplace_back(option_str.GetString(), has_arg,
-                                    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.
-    size_t idx = FindArgumentIndexForOption(long_options, long_options_index);
-    if (idx == size_t(-1))
-      continue;
-
-    if (!result_string.empty()) {
-      auto tmp_arg = m_entries[idx].ref;
-      size_t pos = result_string.find(tmp_arg);
-      if (pos != std::string::npos)
-        result_string.erase(pos, tmp_arg.size());
-    }
-    ReplaceArgumentAtIndex(idx, llvm::StringRef());
-    if ((long_options[long_options_index].definition->option_has_arg !=
-         OptionParser::eNoArgument) &&
-        (OptionParser::GetOptionArgument() != nullptr) &&
-        (idx + 1 < GetArgumentCount()) &&
-        (m_entries[idx + 1].ref == OptionParser::GetOptionArgument())) {
-      if (result_string.size() > 0) {
-        auto tmp_arg = m_entries[idx + 1].ref;
-        size_t pos = result_string.find(tmp_arg);
-        if (pos != std::string::npos)
-          result_string.erase(pos, tmp_arg.size());
-      }
-      ReplaceArgumentAtIndex(idx + 1, llvm::StringRef());
-    }
-  }
-  return result_string;
-}
-
-void Args::ParseArgsForCompletion(Options &options,
-                                  OptionElementVector &option_element_vector,
-                                  uint32_t cursor_index) {
-  StreamString sstr;
-  Option *long_options = options.GetLongOptions();
-  option_element_vector.clear();
-
-  if (long_options == nullptr) {
-    return;
-  }
-
-  // Leading : tells getopt to return a : for a missing option argument AND
-  // to suppress error messages.
-
-  sstr << ":";
-  for (int i = 0; long_options[i].definition != nullptr; ++i) {
-    if (long_options[i].flag == nullptr) {
-      sstr << (char)long_options[i].val;
-      switch (long_options[i].definition->option_has_arg) {
-      default:
-      case OptionParser::eNoArgument:
-        break;
-      case OptionParser::eRequiredArgument:
-        sstr << ":";
-        break;
-      case OptionParser::eOptionalArgument:
-        sstr << "::";
-        break;
-      }
-    }
-  }
-
-  std::unique_lock<std::mutex> lock;
-  OptionParser::Prepare(lock);
-  OptionParser::EnableError(false);
-
-  int val;
-  auto opt_defs = options.GetDefinitions();
-
-  // Fooey... OptionParser::Parse permutes the GetArgumentVector to move the
-  // options to the front. So we have to build another Arg and pass that to
-  // OptionParser::Parse so it doesn't change the one we have.
-
-  std::vector<char *> dummy_vec = m_argv;
-
-  bool failed_once = false;
-  uint32_t dash_dash_pos = -1;
-
-  while (1) {
-    bool missing_argument = false;
-    int long_options_index = -1;
-
-    val = OptionParser::Parse(dummy_vec.size() - 1, &dummy_vec[0],
-                              sstr.GetString(), long_options,
-                              &long_options_index);
-
-    if (val == -1) {
-      // When we're completing a "--" which is the last option on line,
-      if (failed_once)
-        break;
-
-      failed_once = true;
-
-      // If this is a bare  "--" we mark it as such so we can complete it
-      // successfully later.
-      // Handling the "--" is a little tricky, since that may mean end of
-      // options or arguments, or the
-      // user might want to complete options by long name.  I make this work by
-      // checking whether the
-      // cursor is in the "--" argument, and if so I assume we're completing the
-      // long option, otherwise
-      // I let it pass to OptionParser::Parse which will terminate the option
-      // parsing.
-      // Note, in either case we continue parsing the line so we can figure out
-      // what other options
-      // were passed.  This will be useful when we come to restricting
-      // completions based on what other
-      // options we've seen on the line.
-
-      if (static_cast<size_t>(OptionParser::GetOptionIndex()) <
-              dummy_vec.size() - 1 &&
-          (strcmp(dummy_vec[OptionParser::GetOptionIndex() - 1], "--") == 0)) {
-        dash_dash_pos = OptionParser::GetOptionIndex() - 1;
-        if (static_cast<size_t>(OptionParser::GetOptionIndex() - 1) ==
-            cursor_index) {
-          option_element_vector.push_back(
-              OptionArgElement(OptionArgElement::eBareDoubleDash,
-                               OptionParser::GetOptionIndex() - 1,
-                               OptionArgElement::eBareDoubleDash));
-          continue;
-        } else
-          break;
-      } else
-        break;
-    } else if (val == '?') {
-      option_element_vector.push_back(
-          OptionArgElement(OptionArgElement::eUnrecognizedArg,
-                           OptionParser::GetOptionIndex() - 1,
-                           OptionArgElement::eUnrecognizedArg));
-      continue;
-    } else if (val == 0) {
-      continue;
-    } else if (val == ':') {
-      // This is a missing argument.
-      val = OptionParser::GetOptionErrorCause();
-      missing_argument = true;
-    }
-
-    ((Options *)&options)->OptionSeen(val);
-
-    // Look up the long option index
-    if (long_options_index == -1) {
-      for (int j = 0; long_options[j].definition || long_options[j].flag ||
-                      long_options[j].val;
-           ++j) {
-        if (long_options[j].val == val) {
-          long_options_index = j;
-          break;
-        }
-      }
-    }
-
-    // See if the option takes an argument, and see if one was supplied.
-    if (long_options_index >= 0) {
-      int opt_defs_index = -1;
-      for (size_t i = 0; i < opt_defs.size(); i++) {
-        if (opt_defs[i].short_option != val)
-          continue;
-        opt_defs_index = i;
-        break;
-      }
-
-      const OptionDefinition *def = long_options[long_options_index].definition;
-      int has_arg =
-          (def == nullptr) ? OptionParser::eNoArgument : def->option_has_arg;
-      switch (has_arg) {
-      case OptionParser::eNoArgument:
-        option_element_vector.push_back(OptionArgElement(
-            opt_defs_index, OptionParser::GetOptionIndex() - 1, 0));
-        break;
-      case OptionParser::eRequiredArgument:
-        if (OptionParser::GetOptionArgument() != nullptr) {
-          int arg_index;
-          if (missing_argument)
-            arg_index = -1;
-          else
-            arg_index = OptionParser::GetOptionIndex() - 1;
-
-          option_element_vector.push_back(OptionArgElement(
-              opt_defs_index, OptionParser::GetOptionIndex() - 2, arg_index));
-        } else {
-          option_element_vector.push_back(OptionArgElement(
-              opt_defs_index, OptionParser::GetOptionIndex() - 1, -1));
-        }
-        break;
-      case OptionParser::eOptionalArgument:
-        if (OptionParser::GetOptionArgument() != nullptr) {
-          option_element_vector.push_back(OptionArgElement(
-              opt_defs_index, OptionParser::GetOptionIndex() - 2,
-              OptionParser::GetOptionIndex() - 1));
-        } else {
-          option_element_vector.push_back(OptionArgElement(
-              opt_defs_index, OptionParser::GetOptionIndex() - 2,
-              OptionParser::GetOptionIndex() - 1));
-        }
-        break;
-      default:
-        // The options table is messed up.  Here we'll just continue
-        option_element_vector.push_back(
-            OptionArgElement(OptionArgElement::eUnrecognizedArg,
-                             OptionParser::GetOptionIndex() - 1,
-                             OptionArgElement::eUnrecognizedArg));
-        break;
-      }
-    } else {
-      option_element_vector.push_back(
-          OptionArgElement(OptionArgElement::eUnrecognizedArg,
-                           OptionParser::GetOptionIndex() - 1,
-                           OptionArgElement::eUnrecognizedArg));
-    }
-  }
-
-  // Finally we have to handle the case where the cursor index points at a
-  // single "-".  We want to mark that in
-  // the option_element_vector, but only if it is not after the "--".  But it
-  // turns out that OptionParser::Parse just ignores
-  // an isolated "-".  So we have to look it up by hand here.  We only care if
-  // it is AT the cursor position.
-  // Note, a single quoted dash is not the same as a single dash...
-
-  const ArgEntry &cursor = m_entries[cursor_index];
-  if ((static_cast<int32_t>(dash_dash_pos) == -1 ||
-       cursor_index < dash_dash_pos) &&
-      cursor.quote == '\0' && cursor.ref == "-") {
-    option_element_vector.push_back(
-        OptionArgElement(OptionArgElement::eBareDash, cursor_index,
-                         OptionArgElement::eBareDash));
-  }
-}
-
 void Args::EncodeEscapeSequences(const char *src, std::string &dst) {
   dst.clear();
   if (src) {
     for (const char *p = src; *p != '\0'; ++p) {
       size_t non_special_chars = ::strcspn(p, "\\");
       if (non_special_chars > 0) {
         dst.append(p, non_special_chars);
         p += non_special_chars;

lldb/trunk/source/Interpreter/CommandAlias.cpp

static bool ProcessAliasOptionsArgs(lldb::CommandObjectSP &cmd_obj_sp,
   // Check to see if the command being aliased can take any command options.
   Options *options = cmd_obj_sp->GetOptions();
   if (options) {
     // See if any options were specified as part of the alias;  if so, handle
     // them appropriately.
     ExecutionContext exe_ctx =
         cmd_obj_sp->GetCommandInterpreter().GetExecutionContext();
     options->NotifyOptionParsingStarting(&exe_ctx);
-    args.Unshift(llvm::StringRef("dummy_arg"));
-    options_string = args.ParseAliasOptions(*options, result, option_arg_vector,
-                                            options_args);
-    args.Shift();
-    if (result.Succeeded())
-      options->VerifyPartialOptions(result);
+
+    llvm::Expected<Args> args_or =
+        options->ParseAlias(args, option_arg_vector, options_string);
+    if (!args_or) {
+      result.AppendError(toString(args_or.takeError()));
+      result.AppendError("Unable to create requested alias.\n");
+      result.SetStatus(eReturnStatusFailed);
+      return false;
+    }
+    args = std::move(*args_or);
     if (!result.Succeeded() &&
         result.GetStatus() != lldb::eReturnStatusStarted) {
       result.AppendError("Unable to create requested alias.\n");
       return false;
     }
   }
 
   if (!options_string.empty()) {

lldb/trunk/source/Interpreter/CommandObject.cpp

bool CommandObject::ParseOptions(Args &args, CommandReturnObject &result) {
   // See if the subclass has options?
   Options *options = GetOptions();
   if (options != nullptr) {
     Status error;
 
     auto exe_ctx = GetCommandInterpreter().GetExecutionContext();
     options->NotifyOptionParsingStarting(&exe_ctx);
 
-    // ParseOptions calls getopt_long_only, which always skips the zero'th item
-    // in the array and starts at position 1,
-    // so we need to push a dummy value into position zero.
-    args.Unshift(llvm::StringRef("dummy_string"));
     const bool require_validation = true;
-    error = args.ParseOptions(*options, &exe_ctx,
-                              GetCommandInterpreter().GetPlatform(true),
-                              require_validation);
+    llvm::Expected<Args> args_or = options->Parse(
+        args, &exe_ctx, GetCommandInterpreter().GetPlatform(true),
 
-    // The "dummy_string" will have already been removed by ParseOptions,
-    // so no need to remove it.
-
-    if (error.Success())
+    if (args_or) {
+      args = std::move(*args_or);
       error = options->NotifyOptionParsingFinished(&exe_ctx);
+    } else
+      error = args_or.takeError();
 
     if (error.Success()) {
       if (options->VerifyOptions(result))
         return true;
     } else {
       const char *error_cstr = error.AsCString();
       if (error_cstr) {
         // We got an error string, lets use that
if (WantsRawCommandString() && !WantsCompletion()) {
     return -1;
   } else {
     // Can we do anything generic with the options?
     Options *cur_options = GetOptions();
     CommandReturnObject result;
     OptionElementVector opt_element_vector;
 
     if (cur_options != nullptr) {
-      // Re-insert the dummy command name string which will have been
-      // stripped off:
-      input.Unshift(llvm::StringRef("dummy-string"));
-      cursor_index++;
-
-      // I stick an element on the end of the input, because if the last element
-      // is option that requires an argument, getopt_long_only will freak out.
-
-      input.AppendArgument(llvm::StringRef("<FAKE-VALUE>"));
-
-      input.ParseArgsForCompletion(*cur_options, opt_element_vector,
-                                   cursor_index);
-
-      input.DeleteArgumentAtIndex(input.GetArgumentCount() - 1);
+      opt_element_vector = cur_options->ParseForCompletion(input, cursor_index);
 
       bool handled_by_options;
       handled_by_options = cur_options->HandleOptionCompletion(
           input, opt_element_vector, cursor_index, cursor_char_position,
           match_start_point, max_return_elements, GetCommandInterpreter(),
           word_complete, matches);
       if (handled_by_options)
         return matches.GetSize();

lldb/trunk/source/Interpreter/Options.cpp

if (group_set.find(group) == group_set.end()) {
       error = group->OptionParsingFinished(execution_context);
       group_set.insert(group);
       if (error.Fail())
         return error;
     }
   }
   return error;
 }
+
+// OptionParser permutes the arguments while processing them, so we create a
+// temporary array holding to avoid modification of the input arguments. The
+// options themselves are never modified, but the API expects a char * anyway,
+// hence the const_cast.
+static std::vector<char *> GetArgvForParsing(const Args &args) {
+  std::vector<char *> result;
+  // OptionParser always skips the first argument as it is based on getopt().
+  result.push_back(const_cast<char *>("<FAKE-ARG0>"));
+  for (const Args::ArgEntry &entry : args)
+    result.push_back(const_cast<char *>(entry.c_str()));
+  return result;
+}
+
+// Given a permuted argument, find it's position in the original Args vector.
+static Args::const_iterator FindOriginalIter(const char *arg,
+                                             const Args &original) {
+  return llvm::find_if(
+      original, [arg](const Args::ArgEntry &D) { return D.c_str() == arg; });
+}
+
+// Given a permuted argument, find it's index in the original Args vector.
+static size_t FindOriginalIndex(const char *arg, const Args &original) {
+  return std::distance(original.begin(), FindOriginalIter(arg, original));
+}
+
+// Construct a new Args object, consisting of the entries from the original
+// arguments, but in the permuted order.
+static Args ReconstituteArgsAfterParsing(llvm::ArrayRef<char *> parsed,
+                                         const Args &original) {
+  Args result;
+  for (const char *arg : parsed) {
+    auto pos = FindOriginalIter(arg, original);
+    assert(pos != original.end());
+    result.AppendArgument(pos->ref, pos->quote);
+  }
+  return result;
+}
+
+static size_t FindArgumentIndexForOption(const Args &args,
+                                         const Option &long_option) {
+  std::string short_opt = llvm::formatv("-{0}", char(long_option.val)).str();
+  std::string long_opt =
+      llvm::formatv("--{0}", long_option.definition->long_option);
+  for (const auto &entry : llvm::enumerate(args)) {
+    if (entry.value().ref.startswith(short_opt) ||
+        entry.value().ref.startswith(long_opt))
+      return entry.index();
+  }
+
+  return size_t(-1);
+}
+
+llvm::Expected<Args> Options::ParseAlias(const Args &args,
+                                         OptionArgVector *option_arg_vector,
+                                         std::string &input_line) {
+  StreamString sstr;
+  int i;
+  Option *long_options = GetLongOptions();
+
+  if (long_options == nullptr) {
+    return llvm::make_error<llvm::StringError>("Invalid long options",
+                                               llvm::inconvertibleErrorCode());
+  }
+
+  for (i = 0; long_options[i].definition != nullptr; ++i) {
+    if (long_options[i].flag == nullptr) {
+      sstr << (char)long_options[i].val;
+      switch (long_options[i].definition->option_has_arg) {
+      default:
+      case OptionParser::eNoArgument:
+        break;
+      case OptionParser::eRequiredArgument:
+        sstr << ":";
+        break;
+      case OptionParser::eOptionalArgument:
+        sstr << "::";
+        break;
+      }
+    }
+  }
+
+  Args args_copy = args;
+  std::vector<char *> argv = GetArgvForParsing(args);
+
+  std::unique_lock<std::mutex> lock;
+  OptionParser::Prepare(lock);
+  int val;
+  while (1) {
+    int long_options_index = -1;
+    val = OptionParser::Parse(argv.size(), &*argv.begin(), sstr.GetString(),
+                              long_options, &long_options_index);
+
+    if (val == -1)
+      break;
+
+    if (val == '?') {
+      return llvm::make_error<llvm::StringError>(
+          "Unknown or ambiguous option", llvm::inconvertibleErrorCode());
+    }
+
+    if (val == 0)
+      continue;
+
+    OptionSeen(val);
+
+    // Look up the long option index
+    if (long_options_index == -1) {
+      for (int j = 0; long_options[j].definition || long_options[j].flag ||
+                      long_options[j].val;
+           ++j) {
+        if (long_options[j].val == val) {
+          long_options_index = j;
+          break;
+        }
+      }
+    }
+
+    // See if the option takes an argument, and see if one was supplied.
+    if (long_options_index == -1) {
+      return llvm::make_error<llvm::StringError>(
+          llvm::formatv("Invalid option with value '{0}'.", char(val)).str(),
+          llvm::inconvertibleErrorCode());
+    }
+
+    StreamString option_str;
+    option_str.Printf("-%c", val);
+    const OptionDefinition *def = long_options[long_options_index].definition;
+    int has_arg =
+        (def == nullptr) ? OptionParser::eNoArgument : def->option_has_arg;
+
+    const char *option_arg = nullptr;
+    switch (has_arg) {
+    case OptionParser::eRequiredArgument:
+      if (OptionParser::GetOptionArgument() == nullptr) {
+        return llvm::make_error<llvm::StringError>(
+            llvm::formatv("Option '{0}' is missing argument specifier.",
+                          option_str.GetString())
+                .str(),
+            llvm::inconvertibleErrorCode());
+      }
+      LLVM_FALLTHROUGH;
+    case OptionParser::eOptionalArgument:
+      option_arg = OptionParser::GetOptionArgument();
+      LLVM_FALLTHROUGH;
+    case OptionParser::eNoArgument:
+      break;
+    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(option_str.GetString(), has_arg,
+                                    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.
+    size_t idx =
+        FindArgumentIndexForOption(args_copy, long_options[long_options_index]);
+    if (idx == size_t(-1))
+      continue;
+
+    if (!input_line.empty()) {
+      auto tmp_arg = args_copy[idx].ref;
+      size_t pos = input_line.find(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 (input_line.size() > 0) {
+        auto tmp_arg = args_copy[idx].ref;
+        size_t pos = input_line.find(tmp_arg);
+        if (pos != std::string::npos)
+          input_line.erase(pos, tmp_arg.size());
+      }
+      args_copy.DeleteArgumentAtIndex(idx);
+    }
+  }
+
+  return std::move(args_copy);
+}
+
+OptionElementVector Options::ParseForCompletion(const Args &args,
+                                                uint32_t cursor_index) {
+  OptionElementVector option_element_vector;
+  StreamString sstr;
+  Option *long_options = GetLongOptions();
+  option_element_vector.clear();
+
+  if (long_options == nullptr)
+    return option_element_vector;
+
+  // Leading : tells getopt to return a : for a missing option argument AND
+  // to suppress error messages.
+
+  sstr << ":";
+  for (int i = 0; long_options[i].definition != nullptr; ++i) {
+    if (long_options[i].flag == nullptr) {
+      sstr << (char)long_options[i].val;
+      switch (long_options[i].definition->option_has_arg) {
+      default:
+      case OptionParser::eNoArgument:
+        break;
+      case OptionParser::eRequiredArgument:
+        sstr << ":";
+        break;
+      case OptionParser::eOptionalArgument:
+        sstr << "::";
+        break;
+      }
+    }
+  }
+
+  std::unique_lock<std::mutex> lock;
+  OptionParser::Prepare(lock);
+  OptionParser::EnableError(false);
+
+  int val;
+  auto opt_defs = GetDefinitions();
+
+  std::vector<char *> dummy_vec = GetArgvForParsing(args);
+
+  // I stick an element on the end of the input, because if the last element
+  // is option that requires an argument, getopt_long_only will freak out.
+  dummy_vec.push_back(const_cast<char *>("<FAKE-VALUE>"));
+
+  bool failed_once = false;
+  uint32_t dash_dash_pos = -1;
+
+  while (1) {
+    bool missing_argument = false;
+    int long_options_index = -1;
+
+    val = OptionParser::Parse(dummy_vec.size(), &dummy_vec[0], sstr.GetString(),
+                              long_options, &long_options_index);
+
+    if (val == -1) {
+      // When we're completing a "--" which is the last option on line,
+      if (failed_once)
+        break;
+
+      failed_once = true;
+
+      // If this is a bare  "--" we mark it as such so we can complete it
+      // successfully later.  Handling the "--" is a little tricky, since that
+      // may mean end of options or arguments, or the user might want to
+      // complete options by long name.  I make this work by checking whether
+      // the cursor is in the "--" argument, and if so I assume we're
+      // completing the long option, otherwise I let it pass to
+      // OptionParser::Parse which will terminate the option parsing.  Note, in
+      // either case we continue parsing the line so we can figure out what
+      // other options were passed.  This will be useful when we come to
+      // restricting completions based on what other options we've seen on the
+      // line.
+
+      if (static_cast<size_t>(OptionParser::GetOptionIndex()) <
+              dummy_vec.size() &&
+          (strcmp(dummy_vec[OptionParser::GetOptionIndex() - 1], "--") == 0)) {
+        dash_dash_pos = FindOriginalIndex(
+            dummy_vec[OptionParser::GetOptionIndex() - 1], args);
+        if (dash_dash_pos == cursor_index) {
+          option_element_vector.push_back(
+              OptionArgElement(OptionArgElement::eBareDoubleDash, dash_dash_pos,
+                               OptionArgElement::eBareDoubleDash));
+          continue;
+        } else
+          break;
+      } else
+        break;
+    } else if (val == '?') {
+      option_element_vector.push_back(OptionArgElement(
+          OptionArgElement::eUnrecognizedArg,
+          FindOriginalIndex(dummy_vec[OptionParser::GetOptionIndex() - 1],
+                            args),
+          OptionArgElement::eUnrecognizedArg));
+      continue;
+    } else if (val == 0) {
+      continue;
+    } else if (val == ':') {
+      // This is a missing argument.
+      val = OptionParser::GetOptionErrorCause();
+      missing_argument = true;
+    }
+
+    OptionSeen(val);
+
+    // Look up the long option index
+    if (long_options_index == -1) {
+      for (int j = 0; long_options[j].definition || long_options[j].flag ||
+                      long_options[j].val;
+           ++j) {
+        if (long_options[j].val == val) {
+          long_options_index = j;
+          break;
+        }
+      }
+    }
+
+    // See if the option takes an argument, and see if one was supplied.
+    if (long_options_index >= 0) {
+      int opt_defs_index = -1;
+      for (size_t i = 0; i < opt_defs.size(); i++) {
+        if (opt_defs[i].short_option != val)
+          continue;
+        opt_defs_index = i;
+        break;
+      }
+
+      const OptionDefinition *def = long_options[long_options_index].definition;
+      int has_arg =
+          (def == nullptr) ? OptionParser::eNoArgument : def->option_has_arg;
+      switch (has_arg) {
+      case OptionParser::eNoArgument:
+        option_element_vector.push_back(OptionArgElement(
+            opt_defs_index,
+            FindOriginalIndex(dummy_vec[OptionParser::GetOptionIndex() - 1],
+                              args),
+            0));
+        break;
+      case OptionParser::eRequiredArgument:
+        if (OptionParser::GetOptionArgument() != nullptr) {
+          int arg_index;
+          if (missing_argument)
+            arg_index = -1;
+          else
+            arg_index = OptionParser::GetOptionIndex() - 2;
+
+          option_element_vector.push_back(OptionArgElement(
+              opt_defs_index,
+              FindOriginalIndex(dummy_vec[OptionParser::GetOptionIndex() - 2],
+                                args),
+              arg_index));
+        } else {
+          option_element_vector.push_back(OptionArgElement(
+              opt_defs_index,
+              FindOriginalIndex(dummy_vec[OptionParser::GetOptionIndex() - 1],
+                                args),
+              -1));
+        }
+        break;
+      case OptionParser::eOptionalArgument:
+        if (OptionParser::GetOptionArgument() != nullptr) {
+          option_element_vector.push_back(OptionArgElement(
+              opt_defs_index,
+              FindOriginalIndex(dummy_vec[OptionParser::GetOptionIndex() - 2],
+                                args),
+              FindOriginalIndex(dummy_vec[OptionParser::GetOptionIndex() - 1],
+                                args)));
+        } else {
+          option_element_vector.push_back(OptionArgElement(
+              opt_defs_index,
+              FindOriginalIndex(dummy_vec[OptionParser::GetOptionIndex() - 2],
+                                args),
+              FindOriginalIndex(dummy_vec[OptionParser::GetOptionIndex() - 1],
+                                args)));
+        }
+        break;
+      default:
+        // The options table is messed up.  Here we'll just continue
+        option_element_vector.push_back(OptionArgElement(
+            OptionArgElement::eUnrecognizedArg,
+            FindOriginalIndex(dummy_vec[OptionParser::GetOptionIndex() - 1],
+                              args),
+            OptionArgElement::eUnrecognizedArg));
+        break;
+      }
+    } else {
+      option_element_vector.push_back(OptionArgElement(
+          OptionArgElement::eUnrecognizedArg,
+          FindOriginalIndex(dummy_vec[OptionParser::GetOptionIndex() - 1],
+                            args),
+          OptionArgElement::eUnrecognizedArg));
+    }
+  }
+
+  // Finally we have to handle the case where the cursor index points at a
+  // single "-".  We want to mark that in
+  // the option_element_vector, but only if it is not after the "--".  But it
+  // turns out that OptionParser::Parse just ignores
+  // an isolated "-".  So we have to look it up by hand here.  We only care if
+  // it is AT the cursor position.
+  // Note, a single quoted dash is not the same as a single dash...
+
+  const Args::ArgEntry &cursor = args[cursor_index];
+  if ((static_cast<int32_t>(dash_dash_pos) == -1 ||
+       cursor_index < dash_dash_pos) &&
+      cursor.quote == '\0' && cursor.ref == "-") {
+    option_element_vector.push_back(
+        OptionArgElement(OptionArgElement::eBareDash, cursor_index,
+                         OptionArgElement::eBareDash));
+  }
+  return option_element_vector;
+}
+
+llvm::Expected<Args> Options::Parse(const Args &args,
+                                    ExecutionContext *execution_context,
+                                    lldb::PlatformSP platform_sp,
+                                    bool require_validation) {
+  StreamString sstr;
+  Status error;
+  Option *long_options = GetLongOptions();
+  if (long_options == nullptr) {
+    return llvm::make_error<llvm::StringError>("Invalid long options.",
+                                               llvm::inconvertibleErrorCode());
+  }
+
+  for (int i = 0; long_options[i].definition != nullptr; ++i) {
+    if (long_options[i].flag == nullptr) {
+      if (isprint8(long_options[i].val)) {
+        sstr << (char)long_options[i].val;
+        switch (long_options[i].definition->option_has_arg) {
+        default:
+        case OptionParser::eNoArgument:
+          break;
+        case OptionParser::eRequiredArgument:
+          sstr << ':';
+          break;
+        case OptionParser::eOptionalArgument:
+          sstr << "::";
+          break;
+        }
+      }
+    }
+  }
+  std::vector<char *> argv = GetArgvForParsing(args);
+  std::unique_lock<std::mutex> lock;
+  OptionParser::Prepare(lock);
+  int val;
+  while (1) {
+    int long_options_index = -1;
+    val = OptionParser::Parse(argv.size(), &*argv.begin(), sstr.GetString(),
+                              long_options, &long_options_index);
+    if (val == -1)
+      break;
+
+    // Did we get an error?
+    if (val == '?') {
+      error.SetErrorStringWithFormat("unknown or ambiguous option");
+      break;
+    }
+    // The option auto-set itself
+    if (val == 0)
+      continue;
+
+    OptionSeen(val);
+
+    // Lookup the long option index
+    if (long_options_index == -1) {
+      for (int i = 0; long_options[i].definition || long_options[i].flag ||
+                      long_options[i].val;
+           ++i) {
+        if (long_options[i].val == val) {
+          long_options_index = i;
+          break;
+        }
+      }
+    }
+    // Call the callback with the option
+    if (long_options_index >= 0 &&
+        long_options[long_options_index].definition) {
+      const OptionDefinition *def = long_options[long_options_index].definition;
+
+      if (!platform_sp) {
+        // User did not pass in an explicit platform.  Try to grab
+        // from the execution context.
+        TargetSP target_sp =
+            execution_context ? execution_context->GetTargetSP() : TargetSP();
+        platform_sp = target_sp ? target_sp->GetPlatform() : PlatformSP();
+      }
+      OptionValidator *validator = def->validator;
+
+      if (!platform_sp && require_validation) {
+        // Caller requires validation but we cannot validate as we
+        // don't have the mandatory platform against which to
+        // validate.
+        return llvm::make_error<llvm::StringError>(
+            "cannot validate options: no platform available",
+            llvm::inconvertibleErrorCode());
+      }
+
+      bool validation_failed = false;
+      if (platform_sp) {
+        // Ensure we have an execution context, empty or not.
+        ExecutionContext dummy_context;
+        ExecutionContext *exe_ctx_p =
+            execution_context ? execution_context : &dummy_context;
+        if (validator && !validator->IsValid(*platform_sp, *exe_ctx_p)) {
+          validation_failed = true;
+          error.SetErrorStringWithFormat("Option \"%s\" invalid.  %s",
+                                         def->long_option,
+                                         def->validator->LongConditionString());
+        }
+      }
+
+      // As long as validation didn't fail, we set the option value.
+      if (!validation_failed)
+        error =
+            SetOptionValue(long_options_index,
+                           (def->option_has_arg == OptionParser::eNoArgument)
+                               ? nullptr
+                               : OptionParser::GetOptionArgument(),
+                           execution_context);
+    } else {
+      error.SetErrorStringWithFormat("invalid option with value '%i'", val);
+    }
+    if (error.Fail())
+      return error.ToError();
+  }
+
+  argv.erase(argv.begin(), argv.begin() + OptionParser::GetOptionIndex());
+  return ReconstituteArgsAfterParsing(argv, args);
+}

lldb/trunk/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp

BaseCommand(CommandInterpreter &interpreter)
 
     // status
     auto status_cmd_sp = CommandObjectSP(new StatusCommand(interpreter));
     LoadSubCommand("status", status_cmd_sp);
   }
 };
 
 EnableOptionsSP ParseAutoEnableOptions(Status &error, Debugger &debugger) {
+  Log *log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS);
   // We are abusing the options data model here so that we can parse
   // options without requiring the Debugger instance.
 
   // We have an empty execution context at this point.  We only want
   // to parse options, and we don't need any context to do this here.
   // In fact, we want to be able to parse the enable options before having
   // any context.
   ExecutionContext exe_ctx;
EnableOptionsSP ParseAutoEnableOptions(Status &error, Debugger &debugger) {
   if (args.GetArgumentCount() > 0) {
     // Eliminate the initial '--' that would be required to set the
     // settings that themselves include '-' and/or '--'.
     const char *first_arg = args.GetArgumentAtIndex(0);
     if (first_arg && (strcmp(first_arg, "--") == 0))
       args.Shift();
   }
 
-  // ParseOptions calls getopt_long_only, which always skips the zero'th item in
-  // the array and starts at position 1,
-  // so we need to push a dummy value into position zero.
-  args.Unshift(llvm::StringRef("dummy_string"));
   bool require_validation = false;
-  error = args.ParseOptions(*options_sp.get(), &exe_ctx, PlatformSP(),
-                            require_validation);
-  if (!error.Success())
+  llvm::Expected<Args> args_or =
+      options_sp->Parse(args, &exe_ctx, PlatformSP(), require_validation);
+  if (!args_or) {
+    LLDB_LOG_ERROR(
+        log, args_or.takeError(),
+        "Parsing plugin.structured-data.darwin-log.auto-enable-options value "
+        "failed: {0}");
     return EnableOptionsSP();
+  }
 
   if (!options_sp->VerifyOptions(result))
     return EnableOptionsSP();
 
   // We successfully parsed and validated the options.
   return options_sp;
 }