[Lldb-commits] [lldb] Add the ability to define a Python based command that uses CommandObjectParsed (PR #70734)

via lldb-commits lldb-commits at lists.llvm.org
Mon Jan 29 17:04:33 PST 2024


https://github.com/jimingham updated https://github.com/llvm/llvm-project/pull/70734

>From a0c72c07861218fa27420045aecdd5cd072cc60a Mon Sep 17 00:00:00 2001
From: Jim Ingham <jingham at apple.com>
Date: Mon, 30 Oct 2023 14:48:37 -0700
Subject: [PATCH 1/4] Add the ability to define a Python based command that
 uses the CommandObjectParsed way to specify options and arguments so that
 these commands can be "first class citizens" with build-in commands.

---
 lldb/bindings/python/CMakeLists.txt           |   3 +-
 lldb/bindings/python/python-wrapper.swig      |  31 +
 lldb/examples/python/parsed_cmd.py            | 315 ++++++++
 lldb/include/lldb/Interpreter/CommandObject.h |   5 +-
 .../lldb/Interpreter/ScriptInterpreter.h      |  29 +
 .../source/Commands/CommandObjectCommands.cpp | 697 +++++++++++++++++-
 lldb/source/Commands/Options.td               |  22 +-
 lldb/source/Interpreter/CommandObject.cpp     |  16 +
 .../Python/PythonDataObjects.h                |   2 +
 .../Python/SWIGPythonBridge.h                 |   7 +
 .../Python/ScriptInterpreterPython.cpp        | 252 ++++++-
 .../Python/ScriptInterpreterPythonImpl.h      |  21 +
 .../script/add/TestAddParsedCommand.py        | 146 ++++
 .../command/script/add/test_commands.py       | 175 +++++
 .../Python/PythonTestSuite.cpp                |   8 +
 15 files changed, 1714 insertions(+), 15 deletions(-)
 create mode 100644 lldb/examples/python/parsed_cmd.py
 create mode 100644 lldb/test/API/commands/command/script/add/TestAddParsedCommand.py
 create mode 100644 lldb/test/API/commands/command/script/add/test_commands.py

diff --git a/lldb/bindings/python/CMakeLists.txt b/lldb/bindings/python/CMakeLists.txt
index c941f764dfc92..657fdd2c95900 100644
--- a/lldb/bindings/python/CMakeLists.txt
+++ b/lldb/bindings/python/CMakeLists.txt
@@ -96,7 +96,8 @@ function(finish_swig_python swig_target lldb_python_bindings_dir lldb_python_tar
     ${lldb_python_target_dir}
     "utils"
     FILES "${LLDB_SOURCE_DIR}/examples/python/in_call_stack.py"
-          "${LLDB_SOURCE_DIR}/examples/python/symbolication.py")
+          "${LLDB_SOURCE_DIR}/examples/python/symbolication.py"
+          "${LLDB_SOURCE_DIR}/examples/python/parsed_cmd.py")
 
   create_python_package(
     ${swig_target}
diff --git a/lldb/bindings/python/python-wrapper.swig b/lldb/bindings/python/python-wrapper.swig
index 17bc7b1f21987..47887491d0c55 100644
--- a/lldb/bindings/python/python-wrapper.swig
+++ b/lldb/bindings/python/python-wrapper.swig
@@ -831,6 +831,37 @@ bool lldb_private::python::SWIGBridge::LLDBSwigPythonCallCommandObject(
   return true;
 }
 
+bool lldb_private::python::SWIGBridge::LLDBSwigPythonCallParsedCommandObject(
+    PyObject *implementor, lldb::DebuggerSP debugger, lldb_private::StructuredDataImpl &args_impl,
+    lldb_private::CommandReturnObject &cmd_retobj,
+    lldb::ExecutionContextRefSP exe_ctx_ref_sp) {
+
+  PyErr_Cleaner py_err_cleaner(true);
+
+  PythonObject self(PyRefType::Borrowed, implementor);
+  auto pfunc = self.ResolveName<PythonCallable>("__call__");
+
+  if (!pfunc.IsAllocated())
+    return false;
+
+  auto cmd_retobj_arg = SWIGBridge::ToSWIGWrapper(cmd_retobj);
+
+  // FIXME:
+  // I wanted to do something like:
+  // size_t num_elem = args.size();
+  // PythonList my_list(num_elem);
+  // for (const char *elem : args)
+  //   my_list.append(PythonString(elem);
+  //
+  // and then pass my_list to the pfunc, but that crashes somewhere
+  // deep in Python for reasons that aren't clear to me.
+  
+  pfunc(SWIGBridge::ToSWIGWrapper(std::move(debugger)), SWIGBridge::ToSWIGWrapper(args_impl),
+        SWIGBridge::ToSWIGWrapper(exe_ctx_ref_sp), cmd_retobj_arg.obj());
+
+  return true;
+}
+
 PythonObject lldb_private::python::SWIGBridge::LLDBSWIGPythonCreateOSPlugin(
     const char *python_class_name, const char *session_dictionary_name,
     const lldb::ProcessSP &process_sp) {
diff --git a/lldb/examples/python/parsed_cmd.py b/lldb/examples/python/parsed_cmd.py
new file mode 100644
index 0000000000000..7ee9e077d49ab
--- /dev/null
+++ b/lldb/examples/python/parsed_cmd.py
@@ -0,0 +1,315 @@
+"""
+This module implements a couple of utility classes to make writing
+lldb parsed commands more Pythonic.
+The way to use it is to make a class for you command that inherits from ParsedCommandBase.
+That will make an LLDBOVParser which you will use for your
+option definition, and to fetch option values for the current invocation
+of your command.  Access to the OV parser is through:
+
+ParsedCommandBase.get_parser()
+
+Next, implement setup_command_definition in your new command class, and call:
+
+  self.get_parser().add_option
+
+to add all your options.  The order doesn't matter for options, lldb will sort them
+alphabetically for you when it prints help.
+
+Similarly you can define the arguments with:
+
+  self.get_parser.add_argument
+
+at present, lldb doesn't do as much work as it should verifying arguments, it pretty
+much only checks that commands that take no arguments don't get passed arguments.
+
+Then implement the execute function for your command as:
+
+    def __call__(self, debugger, args_array, exe_ctx, result):
+
+The arguments will be in a python array as strings.  
+
+You can access the option values using varname you passed in when defining the option.  
+If you need to know whether a given option was set by the user or not, you can retrieve 
+the option definition array with:
+
+  self.get_options_definition()
+
+look up your element by varname and check the "_value_set" element.
+
+There are example commands in the lldb testsuite at:
+
+llvm-project/lldb/test/API/commands/command/script/add/test_commands.py
+
+FIXME: I should make a convenient wrapper for that. 
+"""
+import inspect
+import lldb
+import sys
+
+class LLDBOVParser:
+    def __init__(self):
+        self.options_array = []
+        self.args_array = []
+
+    # Some methods to translate common value types.  Should return a
+    # tuple of the value and an error value (True => error) if the
+    # type can't be converted.
+    # FIXME: Need a way to push the conversion error string back to lldb.
+    @staticmethod
+    def to_bool(in_value):
+        error = True
+        value = False
+        low_in = in_value.lower()
+        if low_in == "yes" or low_in == "true" or low_in == "1":
+            value = True
+            error = False
+            
+        if not value and low_in == "no" or low_in == "false" or low_in == "0":
+            value = False
+            error = False
+
+        return (value, error)
+
+    @staticmethod
+    def to_int(in_value):
+        #FIXME: Not doing errors yet...
+        return (int(in_value), False)
+
+    def to_unsigned(in_value):
+        # FIXME: find an unsigned converter...
+        # And handle errors.
+        return (int(in_value), False)
+
+    translators = {
+        lldb.eArgTypeBoolean : to_bool,
+        lldb.eArgTypeBreakpointID : to_unsigned,
+        lldb.eArgTypeByteSize : to_unsigned,
+        lldb.eArgTypeCount : to_unsigned,
+        lldb.eArgTypeFrameIndex : to_unsigned,
+        lldb.eArgTypeIndex : to_unsigned,
+        lldb.eArgTypeLineNum : to_unsigned,
+        lldb.eArgTypeNumLines : to_unsigned,
+        lldb.eArgTypeNumberPerLine : to_unsigned,
+        lldb.eArgTypeOffset : to_int,
+        lldb.eArgTypeThreadIndex : to_unsigned,
+        lldb.eArgTypeUnsignedInteger : to_unsigned,
+        lldb.eArgTypeWatchpointID : to_unsigned,
+        lldb.eArgTypeColumnNum : to_unsigned,
+        lldb.eArgTypeRecognizerID : to_unsigned,
+        lldb.eArgTypeTargetID : to_unsigned,
+        lldb.eArgTypeStopHookID : to_unsigned
+    }
+
+    @classmethod
+    def translate_value(cls, value_type, value):
+        error = False
+        try:
+            return cls.translators[value_type](value)
+        except KeyError:
+            # If we don't have a translator, return the string value.
+            return (value, False)
+
+    # FIXME: would this be better done on the C++ side?
+    # The common completers are missing some useful ones.
+    # For instance there really should be a common Type completer
+    # And an "lldb command name" completer.
+    completion_table = {
+        lldb.eArgTypeAddressOrExpression : lldb.eVariablePathCompletion,
+        lldb.eArgTypeArchitecture : lldb.eArchitectureCompletion,
+        lldb.eArgTypeBreakpointID : lldb.eBreakpointCompletion,
+        lldb.eArgTypeBreakpointIDRange : lldb.eBreakpointCompletion,
+        lldb.eArgTypeBreakpointName : lldb.eBreakpointNameCompletion,
+        lldb.eArgTypeClassName : lldb.eSymbolCompletion,
+        lldb.eArgTypeDirectoryName : lldb.eDiskDirectoryCompletion,
+        lldb.eArgTypeExpression : lldb.eVariablePathCompletion,
+        lldb.eArgTypeExpressionPath : lldb.eVariablePathCompletion,
+        lldb.eArgTypeFilename : lldb.eDiskFileCompletion,
+        lldb.eArgTypeFrameIndex : lldb.eFrameIndexCompletion,
+        lldb.eArgTypeFunctionName : lldb.eSymbolCompletion,
+        lldb.eArgTypeFunctionOrSymbol : lldb.eSymbolCompletion,
+        lldb.eArgTypeLanguage : lldb.eTypeLanguageCompletion,
+        lldb.eArgTypePath : lldb.eDiskFileCompletion,
+        lldb.eArgTypePid : lldb.eProcessIDCompletion,
+        lldb.eArgTypeProcessName : lldb.eProcessNameCompletion,
+        lldb.eArgTypeRegisterName : lldb.eRegisterCompletion,
+        lldb.eArgTypeRunArgs : lldb.eDiskFileCompletion,
+        lldb.eArgTypeShlibName : lldb.eModuleCompletion,
+        lldb.eArgTypeSourceFile : lldb.eSourceFileCompletion,
+        lldb.eArgTypeSymbol : lldb.eSymbolCompletion,
+        lldb.eArgTypeThreadIndex : lldb.eThreadIndexCompletion,
+        lldb.eArgTypeVarName : lldb.eVariablePathCompletion,
+        lldb.eArgTypePlatform : lldb.ePlatformPluginCompletion,
+        lldb.eArgTypeWatchpointID : lldb.eWatchpointIDCompletion,
+        lldb.eArgTypeWatchpointIDRange : lldb.eWatchpointIDCompletion,
+        lldb.eArgTypeModuleUUID : lldb.eModuleUUIDCompletion,
+        lldb.eArgTypeStopHookID : lldb.eStopHookIDCompletion
+    }
+
+    @classmethod
+    def determine_completion(cls, arg_type):
+        try:
+            return cls.completion_table[arg_type]
+        except KeyError:
+            return lldb.eNoCompletion
+
+    def get_option_element(self, long_name):
+        # Fixme: Is it worth making a long_option dict holding the rest of
+        # the options dict so this lookup is faster?
+        for item in self.options_array:
+            if item["long_option"] == long_name:
+                return item
+
+        return None
+            
+    def option_parsing_started(self):
+        # This makes the ivars for all the varnames in the array and gives them
+        # their default values.
+        for elem in self.options_array:
+            elem['_value_set'] = False       
+            try:
+                object.__setattr__(self, elem["varname"], elem["default"])
+            except AttributeError:
+            # It isn't an error not to have a target, you'll just have to set and
+            # get this option value on your own.
+                continue
+
+    def set_enum_value(self, enum_values, input):
+        candidates = []
+        for candidate in enum_values:
+            # The enum_values are a duple of value & help string.
+            value = candidate[0]
+            if value.startswith(input):
+                candidates.append(value)
+
+        if len(candidates) == 1:
+            return (candidates[0], False)
+        else:
+            return (input, True)
+        
+    def set_option_value(self, exe_ctx, opt_name, opt_value):
+        elem = self.get_option_element(opt_name)
+        if not elem:
+            return False
+        
+        if "enum_values" in elem:
+            (value, error) = self.set_enum_value(elem["enum_values"], opt_value)
+        else:
+            (value, error)  = __class__.translate_value(elem["value_type"], opt_value)
+
+        if not error:
+            object.__setattr__(self, elem["varname"], value)
+            elem["_value_set"] = True
+            return True
+        return False
+
+    def was_set(self, opt_name):
+        elem = self.get_option_element(opt_name)
+        if not elem:
+            return False
+        try:
+            return elem["_value_set"]
+        except AttributeError:
+            return False
+
+    def is_enum_opt(self, opt_name):
+        elem = self.get_option_element(opt_name)
+        if not elem:
+            return False
+        return "enum_values" in elem
+
+    def add_option(self, short_option, long_option, usage, default,
+                   varname = None, required=False, groups = None,
+                   value_type=lldb.eArgTypeNone, completion_type=None,
+                   enum_values=None):
+        """
+        short_option: one character, must be unique, not required
+        long_option: no spaces, must be unique, required
+        usage: a usage string for this option, will print in the command help
+        default: the initial value for this option (if it has a value)
+        varname: the name of the property that gives you access to the value for
+                 this value.  Defaults to the long option if not provided.
+        required: if true, this option must be provided or the command will error out
+        groups: Which "option groups" does this option belong to
+        value_type: one of the lldb.eArgType enum values.  Some of the common arg
+                    types also have default completers, which will be applied automatically.
+        completion_type: currently these are values form the lldb.CompletionType enum, I
+                         haven't done custom completions yet.
+        enum_values: An array of duples: ["element_name", "element_help"].  If provided,
+                     only one of the enum elements is allowed.  The value will be the 
+                     element_name for the chosen enum element as a string. 
+        """
+        if not varname:
+            varname = long_option
+
+        if not completion_type:
+            completion_type = self.determine_completion(value_type)
+            
+        dict = {"short_option" : short_option,
+                "long_option" : long_option,
+                "required" : required,
+                "usage" : usage,
+                "value_type" : value_type,
+                "completion_type" : completion_type,
+                "varname" : varname,
+                "default" : default}
+
+        if enum_values:
+            dict["enum_values"] = enum_values
+        if groups:
+            dict["groups"] = groups
+
+        self.options_array.append(dict)
+
+    def make_argument_element(self, arg_type, repeat = "optional", groups = None):
+        element = {"arg_type" : arg_type, "repeat" : repeat}
+        if groups:
+            element["groups"] = groups
+        return element
+
+    def add_argument_set(self, arguments):
+        self.args_array.append(arguments)
+
+class ParsedCommandBase:
+    def __init__(self, debugger, unused):
+        self.debugger = debugger
+        self.ov_parser = LLDBOVParser()
+        self.setup_command_definition()
+        
+    def get_parser(self):
+        return self.ov_parser
+
+    def get_options_definition(self):
+        return self.get_parser().options_array
+
+    def get_flags(self):
+        return 0
+
+    def get_args_definition(self):
+        return self.get_parser().args_array
+
+    def option_parsing_started(self):
+        self.get_parser().option_parsing_started()
+
+    def set_option_value(self, exe_ctx, opt_name, opt_value):
+        return self.get_parser().set_option_value(exe_ctx, opt_name, opt_value)
+
+    # These are the two "pure virtual" methods:
+    def __call__(self, debugger, args_array, exe_ctx, result):
+        raise NotImplementedError()
+
+    def setup_command_definition(self):
+        raise NotImplementedError()
+
+    @staticmethod
+    def do_register_cmd(cls, debugger, module_name):
+        # Add any commands contained in this module to LLDB
+        command = "command script add -o -p -c %s.%s %s" % (
+            module_name,
+            cls.__name__,
+            cls.program,
+        )
+        debugger.HandleCommand(command)
+        print(
+            'The "{0}" command has been installed, type "help {0}"'
+            'for detailed help.'.format(cls.program)
+        )
diff --git a/lldb/include/lldb/Interpreter/CommandObject.h b/lldb/include/lldb/Interpreter/CommandObject.h
index 7b427de0264f7..b99de56f53446 100644
--- a/lldb/include/lldb/Interpreter/CommandObject.h
+++ b/lldb/include/lldb/Interpreter/CommandObject.h
@@ -224,7 +224,10 @@ class CommandObject : public std::enable_shared_from_this<CommandObject> {
   void GetFormattedCommandArguments(Stream &str,
                                     uint32_t opt_set_mask = LLDB_OPT_SET_ALL);
 
-  bool IsPairType(ArgumentRepetitionType arg_repeat_type);
+  static bool IsPairType(ArgumentRepetitionType arg_repeat_type);
+
+  static std::optional<ArgumentRepetitionType> 
+    ArgRepetitionFromString(llvm::StringRef string);
 
   bool ParseOptions(Args &args, CommandReturnObject &result);
 
diff --git a/lldb/include/lldb/Interpreter/ScriptInterpreter.h b/lldb/include/lldb/Interpreter/ScriptInterpreter.h
index b941f6012a117..932eaa8b8a4a2 100644
--- a/lldb/include/lldb/Interpreter/ScriptInterpreter.h
+++ b/lldb/include/lldb/Interpreter/ScriptInterpreter.h
@@ -473,6 +473,14 @@ class ScriptInterpreter : public PluginInterface {
     return false;
   }
 
+  virtual bool RunScriptBasedParsedCommand(
+      StructuredData::GenericSP impl_obj_sp, Args& args,
+      ScriptedCommandSynchronicity synchronicity,
+      lldb_private::CommandReturnObject &cmd_retobj, Status &error,
+      const lldb_private::ExecutionContext &exe_ctx) {
+    return false;
+  }
+
   virtual bool RunScriptFormatKeyword(const char *impl_function,
                                       Process *process, std::string &output,
                                       Status &error) {
@@ -517,6 +525,27 @@ class ScriptInterpreter : public PluginInterface {
     dest.clear();
     return false;
   }
+  
+  virtual StructuredData::ObjectSP
+  GetOptionsForCommandObject(StructuredData::GenericSP cmd_obj_sp) {
+    return {};
+  }
+
+  virtual StructuredData::ObjectSP
+  GetArgumentsForCommandObject(StructuredData::GenericSP cmd_obj_sp) {
+    return {};
+  }
+  
+  virtual bool SetOptionValueForCommandObject(
+      StructuredData::GenericSP cmd_obj_sp, ExecutionContext *exe_ctx, 
+      llvm::StringRef long_option, llvm::StringRef value) {
+    return false;
+  }
+
+  virtual void OptionParsingStartedForCommandObject(
+      StructuredData::GenericSP cmd_obj_sp) {
+    return;
+  }
 
   virtual uint32_t
   GetFlagsForCommandObject(StructuredData::GenericSP cmd_obj_sp) {
diff --git a/lldb/source/Commands/CommandObjectCommands.cpp b/lldb/source/Commands/CommandObjectCommands.cpp
index 5b9af4a3e1b88..68e3438a9b7ee 100644
--- a/lldb/source/Commands/CommandObjectCommands.cpp
+++ b/lldb/source/Commands/CommandObjectCommands.cpp
@@ -1149,6 +1149,9 @@ class CommandObjectPythonFunction : public CommandObjectRaw {
   CompletionType m_completion_type = eNoCompletion;
 };
 
+/// This class implements a "raw" scripted command.  lldb does no parsing of the
+/// command line, instead passing the line unaltered (except for backtick
+/// substitution).
 class CommandObjectScriptingObject : public CommandObjectRaw {
 public:
   CommandObjectScriptingObject(CommandInterpreter &interpreter,
@@ -1244,6 +1247,676 @@ class CommandObjectScriptingObject : public CommandObjectRaw {
   CompletionType m_completion_type = eNoCompletion;
 };
 
+
+/// This command implements a lldb parsed scripted command.  The command
+/// provides a definition of the options and arguments, and a option value
+/// setting callback, and then the command's execution function gets passed
+/// just the parsed arguments.
+/// Note, implementing a command in Python using these base interfaces is a bit
+/// of a pain, but it is much easier to export this low level interface, and
+/// then make it nicer on the Python side, than to try to do that in a
+/// script language neutral way.
+/// So I've also added a base class in Python that provides a table-driven
+/// way of defining the options and arguments, which automatically fills the
+/// option values, making them available as properties in Python.
+/// 
+class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
+private: 
+  class CommandOptions : public Options {
+  public:
+    CommandOptions(CommandInterpreter &interpreter, 
+        StructuredData::GenericSP cmd_obj_sp) : m_interpreter(interpreter), 
+            m_cmd_obj_sp(cmd_obj_sp) {}
+
+    ~CommandOptions() override = default;
+
+    Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
+                          ExecutionContext *execution_context) override {
+      Status error;
+      ScriptInterpreter *scripter = 
+        m_interpreter.GetDebugger().GetScriptInterpreter();
+      if (!scripter) {
+        error.SetErrorString("No script interpreter for SetOptionValue.");
+        return error;
+      }
+      if (!m_cmd_obj_sp) {
+        error.SetErrorString("SetOptionValue called with empty cmd_obj.");
+        return error;
+      }
+      if (!m_options_definition_up) {
+        error.SetErrorString("SetOptionValue called before options definitions "
+                             "were created.");
+        return error;
+      }
+      // Pass the long option, since you aren't actually required to have a
+      // short_option, and for those options the index or short option character
+      // aren't meaningful on the python side.
+      const char * long_option = 
+        m_options_definition_up.get()[option_idx].long_option;
+      bool success = scripter->SetOptionValueForCommandObject(m_cmd_obj_sp, 
+        execution_context, long_option, option_arg);
+      if (!success)
+        error.SetErrorStringWithFormatv("Error setting option: {0} to {1}",
+                                        long_option, option_arg);
+      return error;
+    }
+
+    void OptionParsingStarting(ExecutionContext *execution_context) override {
+      ScriptInterpreter *scripter = 
+        m_interpreter.GetDebugger().GetScriptInterpreter();
+      if (!scripter) {
+        return;
+      }
+      if (!m_cmd_obj_sp) {
+        return;
+      }
+      scripter->OptionParsingStartedForCommandObject(m_cmd_obj_sp);
+    };
+
+    llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
+      if (!m_options_definition_up)
+        return {};
+      return llvm::ArrayRef(m_options_definition_up.get(), m_num_options);
+    }
+    
+    static bool ParseUsageMaskFromArray(StructuredData::ObjectSP obj_sp, 
+        size_t counter, uint32_t &usage_mask, Status &error) {
+      // If the usage entry is not provided, we use LLDB_OPT_SET_ALL.
+      // If the usage mask is a UINT, the option belongs to that group.
+      // If the usage mask is a vector of UINT's, the option belongs to all the
+      // groups listed.
+      // If a subelement of the vector is a vector of two ints, then the option
+      // belongs to the inclusive range from the first to the second element.
+      if (!obj_sp) {
+        usage_mask = LLDB_OPT_SET_ALL;
+        return true;
+      }
+      
+      usage_mask = 0;
+      
+      StructuredData::UnsignedInteger *uint_val = 
+          obj_sp->GetAsUnsignedInteger();
+      if (uint_val) {
+        // If this is an integer, then this specifies a single group:
+        uint32_t value = uint_val->GetValue();
+        if (value == 0) {
+          error.SetErrorStringWithFormatv(
+              "0 is not a valid group for option {0}", counter);
+          return false;
+        }
+        usage_mask = (1 << (value - 1));
+        return true;
+      }
+      // Otherwise it has to be an array:
+      StructuredData::Array *array_val = obj_sp->GetAsArray();
+      if (!array_val) {
+        error.SetErrorStringWithFormatv(
+            "required field is not a array for option {0}", counter);
+        return false;
+      }
+      // This is the array ForEach for accumulating a group usage mask from
+      // an array of string descriptions of groups.
+      auto groups_accumulator 
+          = [counter, &usage_mask, &error] 
+            (StructuredData::Object *obj) -> bool {
+        StructuredData::UnsignedInteger *int_val = obj->GetAsUnsignedInteger();
+        if (int_val) {
+          uint32_t value = int_val->GetValue();
+          if (value == 0) {
+            error.SetErrorStringWithFormatv(
+                "0 is not a valid group for element {0}", counter);
+            return false;
+          }
+          usage_mask |= (1 << (value - 1));
+          return true;
+        }
+        StructuredData::Array *arr_val = obj->GetAsArray();
+        if (!arr_val) {
+          error.SetErrorStringWithFormatv(
+              "Group element not an int or array of integers for element {0}", 
+              counter);
+          return false; 
+        }
+        size_t num_range_elem = arr_val->GetSize();
+        if (num_range_elem != 2) {
+          error.SetErrorStringWithFormatv(
+              "Subranges of a group not a start and a stop for element {0}", 
+              counter);
+          return false; 
+        }
+        int_val = arr_val->GetItemAtIndex(0)->GetAsUnsignedInteger();
+        if (!int_val) {
+          error.SetErrorStringWithFormatv("Start element of a subrange of a "
+              "group not unsigned int for element {0}", counter);
+          return false; 
+        }
+        uint32_t start = int_val->GetValue();
+        int_val = arr_val->GetItemAtIndex(1)->GetAsUnsignedInteger();
+        if (!int_val) {
+          error.SetErrorStringWithFormatv("End element of a subrange of a group"
+              " not unsigned int for element {0}", counter);
+          return false; 
+        }
+        uint32_t end = int_val->GetValue();
+        if (start == 0 || end == 0 || start > end) {
+          error.SetErrorStringWithFormatv("Invalid subrange of a group: {0} - "
+              "{1} for element {2}", start, end, counter);
+          return false;
+        }
+        for (uint32_t i = start; i <= end; i++) {
+          usage_mask |= (1 << (i - 1));
+        }
+        return true;
+      };
+      return array_val->ForEach(groups_accumulator);
+    }
+    
+    
+    Status SetOptionsFromArray(StructuredData::Array &options, Status &error) {
+      m_num_options = options.GetSize();
+      m_options_definition_up.reset(new OptionDefinition[m_num_options]);
+      // We need to hand out pointers to contents of these vectors; we reserve
+      // as much as we'll need up front so they don't get freed on resize...
+      m_usage_container.reserve(m_num_options);
+      m_enum_storage.reserve(m_num_options);
+      m_enum_vector.reserve(m_num_options);
+      
+      size_t counter = 0;
+      size_t short_opt_counter = 0;
+      // This is the Array::ForEach function for adding option elements:
+      auto add_element = [this, &error, &counter, &short_opt_counter] 
+          (StructuredData::Object *object) -> bool {
+        StructuredData::Dictionary *opt_dict = object->GetAsDictionary();
+        if (!opt_dict) {
+          error.SetErrorString("Object in options array is not a dictionary");
+          return false;
+        }
+        OptionDefinition &option_def = m_options_definition_up.get()[counter];
+        
+        // We aren't exposing the validator yet, set it to null
+        option_def.validator = nullptr;
+        // We don't require usage masks, so set it to one group by default:
+        option_def.usage_mask = 1;
+        
+        // Now set the fields of the OptionDefinition Array from the dictionary:
+        //
+        // Note that I don't check for unknown fields in the option dictionaries
+        // so a scriptor can add extra elements that are helpful when they go to
+        // do "set_option_value"
+        
+        // Usage Mask:
+        StructuredData::ObjectSP obj_sp = opt_dict->GetValueForKey("groups");
+        if (obj_sp) {
+          ParseUsageMaskFromArray(obj_sp, counter, option_def.usage_mask, 
+              error);
+          if (error.Fail())
+            return false;
+        }
+
+        // Required:
+        option_def.required = false;
+        obj_sp = opt_dict->GetValueForKey("required");
+        if (obj_sp) {
+          StructuredData::Boolean *boolean_val = obj_sp->GetAsBoolean();
+          if (!boolean_val) {
+            error.SetErrorStringWithFormatv("'required' field is not a boolean "
+                "for option {0}", counter);
+            return false;
+          } 
+          option_def.required = boolean_val->GetValue();      
+        }
+        
+        // Short Option:
+        int short_option;
+        obj_sp = opt_dict->GetValueForKey("short_option");
+        if (obj_sp) {
+          // The value is a string, so pull the 
+          llvm::StringRef short_str = obj_sp->GetStringValue();
+          if (short_str.empty()) {
+            error.SetErrorStringWithFormatv("short_option field empty for "
+                "option {0}", counter);
+            return false;
+          } else if (short_str.size() != 1) {
+            error.SetErrorStringWithFormatv("short_option field has extra "
+                "characters for option {0}", counter);
+            return false;
+          }
+          short_option = (int) short_str[0];
+        } else {
+          // If the short option is not provided, then we need a unique value 
+          // less than the lowest printable ASCII character.
+          short_option = short_opt_counter++;
+        }
+        option_def.short_option = short_option;
+        
+        // Long Option:
+        std::string long_option;
+        obj_sp = opt_dict->GetValueForKey("long_option");
+        if (!obj_sp) {
+          error.SetErrorStringWithFormatv("required long_option missing from "
+          "option {0}", counter);
+          return false;
+        }
+        llvm::StringRef long_stref = obj_sp->GetStringValue();
+        if (long_stref.empty()) {
+          error.SetErrorStringWithFormatv("empty long_option for option {0}", 
+              counter);
+          return false;
+        }
+        auto inserted = g_string_storer.insert(long_stref.str());
+        option_def.long_option = ((*(inserted.first)).data());
+        
+        // Value Type:
+        obj_sp = opt_dict->GetValueForKey("value_type");
+        if (obj_sp) {
+          StructuredData::UnsignedInteger *uint_val 
+              = obj_sp->GetAsUnsignedInteger();
+          if (!uint_val) {
+            error.SetErrorStringWithFormatv("Value type must be an unsigned "
+                "integer");
+            return false;
+          }
+          uint64_t val_type = uint_val->GetValue();
+          if (val_type >= eArgTypeLastArg) {
+            error.SetErrorStringWithFormatv("Value type {0} beyond the "
+                "CommandArgumentType bounds", val_type);
+            return false;
+          }
+          option_def.argument_type = (CommandArgumentType) val_type;
+          option_def.option_has_arg = true;
+        } else {
+          option_def.argument_type = eArgTypeNone;
+          option_def.option_has_arg = false;
+        }
+        
+        // Completion Type:
+        obj_sp = opt_dict->GetValueForKey("completion_type");
+        if (obj_sp) {
+          StructuredData::UnsignedInteger *uint_val = obj_sp->GetAsUnsignedInteger();
+          if (!uint_val) {
+            error.SetErrorStringWithFormatv("Completion type must be an "
+                "unsigned integer for option {0}", counter);
+            return false;
+          }
+          uint64_t completion_type = uint_val->GetValue();
+          if (completion_type > eCustomCompletion) {
+            error.SetErrorStringWithFormatv("Completion type for option {0} "
+                "beyond the CompletionType bounds", completion_type);
+            return false;
+          }
+          option_def.completion_type = (CommandArgumentType) completion_type;
+        } else
+          option_def.completion_type = eNoCompletion;
+        
+        // Usage Text:
+        std::string usage_text;
+        obj_sp = opt_dict->GetValueForKey("usage");
+        if (!obj_sp) {
+          error.SetErrorStringWithFormatv("required usage missing from option "
+              "{0}", counter);
+          return false;
+        }
+        long_stref = obj_sp->GetStringValue();
+        if (long_stref.empty()) {
+          error.SetErrorStringWithFormatv("empty usage text for option {0}", 
+              counter);
+          return false;
+        }
+        m_usage_container[counter] = long_stref.str().c_str();
+        option_def.usage_text = m_usage_container[counter].data();
+
+        // Enum Values:
+        
+        obj_sp = opt_dict->GetValueForKey("enum_values");
+        if (obj_sp) {
+          StructuredData::Array *array = obj_sp->GetAsArray();
+          if (!array) {
+            error.SetErrorStringWithFormatv("enum values must be an array for "
+                "option {0}", counter);
+            return false;
+          }
+          size_t num_elem = array->GetSize();
+          size_t enum_ctr = 0;
+          m_enum_storage[counter] = std::vector<EnumValueStorer>(num_elem);
+          std::vector<EnumValueStorer> &curr_elem = m_enum_storage[counter];
+          
+          // This is the Array::ForEach function for adding enum elements:
+          // Since there are only two fields to specify the enum, use a simple
+          // two element array with value first, usage second.
+          // counter is only used for reporting so I pass it by value here.
+          auto add_enum = [&enum_ctr, &curr_elem, counter, &error] 
+              (StructuredData::Object *object) -> bool {
+            StructuredData::Array *enum_arr = object->GetAsArray();
+            if (!enum_arr) {
+              error.SetErrorStringWithFormatv("Enum values for option {0} not "
+                  "an array", counter);
+              return false;
+            }
+            size_t num_enum_elements = enum_arr->GetSize();
+            if (num_enum_elements != 2) {
+              error.SetErrorStringWithFormatv("Wrong number of elements: {0} "
+                  "for enum {1} in option {2}",
+                  num_enum_elements, enum_ctr, counter);
+              return false;
+            }
+            // Enum Value:
+            StructuredData::ObjectSP obj_sp = enum_arr->GetItemAtIndex(0);
+            llvm::StringRef val_stref = obj_sp->GetStringValue();
+            std::string value_cstr_str = val_stref.str().c_str();
+            
+            // Enum Usage:
+            obj_sp = enum_arr->GetItemAtIndex(1);
+            if (!obj_sp) {
+              error.SetErrorStringWithFormatv("No usage for enum {0} in option "
+                  "{1}",  enum_ctr, counter);
+              return false;
+            }
+            llvm::StringRef usage_stref = obj_sp->GetStringValue();
+            std::string usage_cstr_str = usage_stref.str().c_str();
+            curr_elem[enum_ctr] = EnumValueStorer(value_cstr_str, 
+                usage_cstr_str, enum_ctr);
+            
+            enum_ctr++;
+            return true;
+          }; // end of add_enum
+          
+          array->ForEach(add_enum);
+          if (!error.Success())
+            return false;
+          // We have to have a vector of elements to set in the options, make 
+          // that here:
+          for (auto &elem : curr_elem)
+            m_enum_vector[counter].emplace_back(elem.element);
+
+          option_def.enum_values = llvm::ArrayRef(m_enum_vector[counter]);
+        }
+        counter++;
+        return true;
+      }; // end of add_element
+      
+      options.ForEach(add_element);
+      return error;
+    }
+    
+  private:
+    struct EnumValueStorer {
+      EnumValueStorer() {
+        element.string_value = "value not set";
+        element.usage = "usage not set";
+        element.value = 0;
+      }
+      
+      EnumValueStorer(std::string &in_str_val, std::string &in_usage, 
+          size_t in_value) : value(in_str_val), usage(in_usage) {
+        SetElement(in_value);
+      }
+      
+      EnumValueStorer(const EnumValueStorer &in) : value(in.value), 
+          usage(in.usage) {
+        SetElement(in.element.value);
+      }
+      
+      EnumValueStorer &operator=(const EnumValueStorer &in) {
+        value = in.value;
+        usage = in.usage;
+        SetElement(in.element.value);
+        return *this;
+      }
+      
+      void SetElement(size_t in_value) {
+        element.value = in_value;
+        element.string_value = value.data();
+        element.usage = usage.data(); 
+      }
+      
+      std::string value;
+      std::string usage;
+      OptionEnumValueElement element;
+    };
+    // We have to provide char * values for the long option, usage and enum
+    // values, that's what the option definitions hold.
+    // The long option strings are quite likely to be reused in other added
+    // commands, so those are stored in a global set: g_string_storer.
+    // But the usages are much less likely to be reused, so those are stored in
+    // a vector in the command instance.  It gets resized to the correct size
+    // and then filled with null-terminated strings in the std::string, so the 
+    // are valid C-strings that won't move around.
+    // The enum values and descriptions are treated similarly - these aren't
+    // all that common so it's not worth the effort to dedup them.  
+    size_t m_num_options = 0;
+    std::unique_ptr<OptionDefinition> m_options_definition_up;
+    std::vector<std::vector<EnumValueStorer>> m_enum_storage;
+    std::vector<std::vector<OptionEnumValueElement>> m_enum_vector;
+    std::vector<std::string> m_usage_container;
+    CommandInterpreter &m_interpreter;
+    StructuredData::GenericSP m_cmd_obj_sp;
+    static std::unordered_set<std::string> g_string_storer;
+  };
+
+public:
+  CommandObjectScriptingObjectParsed(CommandInterpreter &interpreter,
+                               std::string name,
+                               StructuredData::GenericSP cmd_obj_sp,
+                               ScriptedCommandSynchronicity synch)
+      : CommandObjectParsed(interpreter, name.c_str()), 
+        m_cmd_obj_sp(cmd_obj_sp), m_synchro(synch), 
+        m_options(interpreter, cmd_obj_sp), m_fetched_help_short(false), 
+        m_fetched_help_long(false) {
+    StreamString stream;
+    ScriptInterpreter *scripter = GetDebugger().GetScriptInterpreter();
+    if (!scripter) {
+      m_options_error.SetErrorString("No script interpreter");
+      return;
+    }
+
+    // Set the flags:
+    GetFlags().Set(scripter->GetFlagsForCommandObject(cmd_obj_sp));
+
+    // Now set up the options definitions from the options:
+    StructuredData::ObjectSP options_object_sp 
+        = scripter->GetOptionsForCommandObject(cmd_obj_sp);
+    // It's okay not to have an options array.
+    if (options_object_sp) {
+      StructuredData::Array *options_array = options_object_sp->GetAsArray();
+      // but if it exists, it has to be an array.
+      if (options_array) {
+        m_options.SetOptionsFromArray(*(options_array), m_options_error);
+        // If we got an error don't bother with the arguments...
+        if (m_options_error.Fail())
+          return;
+      } else {
+        m_options_error.SetErrorString("Options array not an array");
+        return;
+      }
+    }
+    // Then fetch the args.  Since the arguments can have usage masks you need
+    // an array of arrays.
+    StructuredData::ObjectSP args_object_sp 
+      = scripter->GetArgumentsForCommandObject(cmd_obj_sp);
+    if (args_object_sp) {
+      StructuredData::Array *args_array = args_object_sp->GetAsArray();        
+      if (!args_array) {
+        m_args_error.SetErrorString("Argument specification is not an array");
+        return;
+      }
+      size_t counter = 0;
+      
+      // This is the Array::ForEach function that handles the
+      // CommandArgumentEntry arrays one by one:
+      auto arg_array_adder = [this, &counter] (StructuredData::Object *object) 
+          -> bool {
+        // This is the Array::ForEach function to add argument entries:
+        CommandArgumentEntry this_entry;
+        size_t elem_counter = 0;
+        auto args_adder = [this, counter, &elem_counter, &this_entry] 
+            (StructuredData::Object *object) -> bool {
+          // The arguments definition has three fields, the argument type, the
+          // repeat and the usage mask. 
+          CommandArgumentType arg_type = eArgTypeNone;
+          ArgumentRepetitionType arg_repetition = eArgRepeatOptional;
+          uint32_t arg_opt_set_association;
+          
+          auto report_error = [this, elem_counter, counter] 
+              (const char *err_txt) -> bool {
+            m_args_error.SetErrorStringWithFormatv("Element {0} of arguments "
+                "list element {1}: %s.", elem_counter, counter, err_txt);
+            return false;
+          };
+          
+          StructuredData::Dictionary *arg_dict = object->GetAsDictionary();
+          if (!arg_dict) {
+            report_error("is not a dictionary.");
+            return false;
+          }
+          // Argument Type:
+          StructuredData::ObjectSP obj_sp 
+              = arg_dict->GetValueForKey("arg_type");
+          if (obj_sp) {
+            StructuredData::UnsignedInteger *uint_val 
+                = obj_sp->GetAsUnsignedInteger();
+            if (!uint_val) {
+              report_error("value type must be an unsigned integer");
+              return false;
+            }
+            uint64_t arg_type_int = uint_val->GetValue();
+            if (arg_type_int >= eArgTypeLastArg) {
+              report_error("value type beyond ArgumentRepetitionType bounds");
+              return false;
+            }
+            arg_type = (CommandArgumentType) arg_type_int;
+          }
+          // Repeat Value:
+          obj_sp = arg_dict->GetValueForKey("repeat");
+          std::optional<ArgumentRepetitionType> repeat;
+          if (obj_sp) {
+            llvm::StringRef repeat_str = obj_sp->GetStringValue();
+            if (repeat_str.empty()) {
+              report_error("repeat value is empty");
+              return false;
+            }
+            repeat = ArgRepetitionFromString(repeat_str);
+            if (!repeat) {
+              report_error("invalid repeat value");
+              return false;
+            }
+            arg_repetition = *repeat;
+          } 
+          
+          // Usage Mask:
+          obj_sp = arg_dict->GetValueForKey("groups");
+          CommandOptions::ParseUsageMaskFromArray(obj_sp, counter, 
+              arg_opt_set_association, m_args_error);
+          this_entry.emplace_back(arg_type, arg_repetition, 
+              arg_opt_set_association);
+          elem_counter++;
+          return true;
+        };
+        StructuredData::Array *args_array = object->GetAsArray();
+        if (!args_array) {
+          m_args_error.SetErrorStringWithFormatv("Argument definition element "
+              "{0} is not an array", counter);
+        }
+        
+        args_array->ForEach(args_adder);
+        if (m_args_error.Fail())
+          return false;
+        if (this_entry.empty()) {
+          m_args_error.SetErrorStringWithFormatv("Argument definition element "
+              "{0} is empty", counter);
+          return false;
+        }
+        m_arguments.push_back(this_entry);
+        counter++;
+        return true;
+      }; // end of arg_array_adder
+      // Here we actually parse the args definition:
+      args_array->ForEach(arg_array_adder);
+    }
+  }
+
+  ~CommandObjectScriptingObjectParsed() override = default;
+
+  Status GetOptionsError() { return m_options_error; }
+  Status GetArgsError() { return m_args_error; }
+  bool WantsCompletion() override { return true; }
+
+  bool IsRemovable() const override { return true; }
+
+  ScriptedCommandSynchronicity GetSynchronicity() { return m_synchro; }
+
+  llvm::StringRef GetHelp() override {
+    if (m_fetched_help_short)
+      return CommandObjectParsed::GetHelp();
+    ScriptInterpreter *scripter = GetDebugger().GetScriptInterpreter();
+    if (!scripter)
+      return CommandObjectParsed::GetHelp();
+    std::string docstring;
+    m_fetched_help_short =
+        scripter->GetShortHelpForCommandObject(m_cmd_obj_sp, docstring);
+    if (!docstring.empty())
+      SetHelp(docstring);
+
+    return CommandObjectParsed::GetHelp();
+  }
+
+  llvm::StringRef GetHelpLong() override {
+    if (m_fetched_help_long)
+      return CommandObjectParsed::GetHelpLong();
+
+    ScriptInterpreter *scripter = GetDebugger().GetScriptInterpreter();
+    if (!scripter)
+      return CommandObjectParsed::GetHelpLong();
+
+    std::string docstring;
+    m_fetched_help_long =
+        scripter->GetLongHelpForCommandObject(m_cmd_obj_sp, docstring);
+    if (!docstring.empty())
+      SetHelpLong(docstring);
+    return CommandObjectParsed::GetHelpLong();
+  }
+  
+  Options *GetOptions() override { return &m_options; }
+
+
+protected:
+  bool DoExecute(Args &args,
+                 CommandReturnObject &result) override {
+    ScriptInterpreter *scripter = GetDebugger().GetScriptInterpreter();
+
+    Status error;
+
+    result.SetStatus(eReturnStatusInvalid);
+    
+    if (!scripter ||
+        !scripter->RunScriptBasedParsedCommand(m_cmd_obj_sp, args,
+                                         m_synchro, result, error, m_exe_ctx)) {
+      result.AppendError(error.AsCString());
+    } else {
+      // Don't change the status if the command already set it...
+      if (result.GetStatus() == eReturnStatusInvalid) {
+        if (result.GetOutputData().empty())
+          result.SetStatus(eReturnStatusSuccessFinishNoResult);
+        else
+          result.SetStatus(eReturnStatusSuccessFinishResult);
+      }
+    }
+
+    return result.Succeeded();
+  }
+
+private:
+  StructuredData::GenericSP m_cmd_obj_sp;
+  ScriptedCommandSynchronicity m_synchro;
+  CommandOptions m_options;
+  Status m_options_error;
+  Status m_args_error;
+  bool m_fetched_help_short : 1;
+  bool m_fetched_help_long : 1;
+};
+
+std::unordered_set<std::string>
+    CommandObjectScriptingObjectParsed::CommandOptions::g_string_storer;
+
 // CommandObjectCommandsScriptImport
 #define LLDB_OPTIONS_script_import
 #include "CommandOptions.inc"
@@ -1437,6 +2110,9 @@ class CommandObjectCommandsScriptAdd : public CommandObjectParsed,
       case 'o':
         m_overwrite_lazy = eLazyBoolYes;
         break;
+      case 'p':
+        m_parsed_command = true;
+        break;
       case 's':
         m_synchronicity =
             (ScriptedCommandSynchronicity)OptionArgParser::ToOptionEnum(
@@ -1472,6 +2148,7 @@ class CommandObjectCommandsScriptAdd : public CommandObjectParsed,
       m_completion_type = eNoCompletion;
       m_overwrite_lazy = eLazyBoolCalculate;
       m_synchronicity = eScriptedCommandSynchronicitySynchronous;
+      m_parsed_command = false;
     }
 
     llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
@@ -1487,6 +2164,7 @@ class CommandObjectCommandsScriptAdd : public CommandObjectParsed,
     ScriptedCommandSynchronicity m_synchronicity =
         eScriptedCommandSynchronicitySynchronous;
     CompletionType m_completion_type = eNoCompletion;
+    bool m_parsed_command = false;
   };
 
   void IOHandlerActivated(IOHandler &io_handler, bool interactive) override {
@@ -1626,10 +2304,21 @@ class CommandObjectCommandsScriptAdd : public CommandObjectParsed,
                                       "'{0}'", m_options.m_class_name);
         return;
       }
-
-      new_cmd_sp.reset(new CommandObjectScriptingObject(
-          m_interpreter, m_cmd_name, cmd_obj_sp, m_synchronicity,
-          m_completion_type));
+      
+      if (m_options.m_parsed_command) {
+        new_cmd_sp.reset(new CommandObjectScriptingObjectParsed(
+            m_interpreter, m_cmd_name, cmd_obj_sp, m_synchronicity));
+        Status options_error 
+            = static_cast<CommandObjectScriptingObjectParsed *>(new_cmd_sp.get())->GetOptionsError();
+        if (options_error.Fail()) {
+          result.AppendErrorWithFormat("failed to parse option definitions: %s",
+                             options_error.AsCString());
+          return false;
+        }
+      } else
+        new_cmd_sp.reset(new CommandObjectScriptingObject(
+            m_interpreter, m_cmd_name, cmd_obj_sp, m_synchronicity,
+            m_completion_type));
     }
     
     // Assume we're going to succeed...
diff --git a/lldb/source/Commands/Options.td b/lldb/source/Commands/Options.td
index ed3167727bcd3..165fccea868a5 100644
--- a/lldb/source/Commands/Options.td
+++ b/lldb/source/Commands/Options.td
@@ -805,19 +805,25 @@ let Command = "script add" in {
   def script_add_function : Option<"function", "f">, Group<1>,
     Arg<"PythonFunction">,
     Desc<"Name of the Python function to bind to this command name.">;
-  def script_add_class : Option<"class", "c">, Group<2>, Arg<"PythonClass">,
-  Desc<"Name of the Python class to bind to this command name.">;
+  def script_add_class : Option<"class", "c">, Groups<[2,3]>, 
+    Arg<"PythonClass">,
+    Desc<"Name of the Python class to bind to this command name.">;
   def script_add_help : Option<"help", "h">, Group<1>, Arg<"HelpText">,
-  Desc<"The help text to display for this command.">;
-  def script_add_overwrite : Option<"overwrite", "o">, Groups<[1,2]>,
-  Desc<"Overwrite an existing command at this node.">;
+    Desc<"The help text to display for this command.">;
+    def script_add_overwrite : Option<"overwrite", "o">,
+    Desc<"Overwrite an existing command at this node.">;
   def script_add_synchronicity : Option<"synchronicity", "s">,
     EnumArg<"ScriptedCommandSynchronicity">,
     Desc<"Set the synchronicity of this command's executions with regard to "
     "LLDB event system.">;
-  def completion_type : Option<"completion-type", "C">,
-  EnumArg<"CompletionType">,
-  Desc<"Specify which completion type the command should use - if none is specified, the command won't use auto-completion.">;
+  def script_add_completion_type : Option<"completion-type", "C">, 
+    Groups<[1,2]>, EnumArg<"CompletionType">,
+    Desc<"Specify which completion type the command should use - if none is "
+    "specified, the command won't use auto-completion.">;
+  def script_add_parsed_command : Option<"parsed", "p">, Group<3>,
+    Desc<"Make a parsed command. The command class will provide the command "
+    "definition by implementing get_options and get_arguments.">;
+
 }
 
 let Command = "container add" in {
diff --git a/lldb/source/Interpreter/CommandObject.cpp b/lldb/source/Interpreter/CommandObject.cpp
index 1ff9774a0da49..43b0b611a57c3 100644
--- a/lldb/source/Interpreter/CommandObject.cpp
+++ b/lldb/source/Interpreter/CommandObject.cpp
@@ -447,6 +447,22 @@ bool CommandObject::IsPairType(ArgumentRepetitionType arg_repeat_type) {
          (arg_repeat_type == eArgRepeatPairRangeOptional);
 }
 
+std::optional<ArgumentRepetitionType> 
+CommandObject::ArgRepetitionFromString(llvm::StringRef string) {
+  if (string == "plain") return eArgRepeatPlain ;   
+  if (string ==  "optional") return eArgRepeatOptional;
+  if (string ==  "plus") return eArgRepeatPlus;
+  if (string ==  "star") return eArgRepeatStar; 
+  if (string ==  "range") return eArgRepeatRange;
+  if (string ==  "pair-plain") return eArgRepeatPairPlain;
+  if (string ==  "pair-optional") return eArgRepeatPairOptional;
+  if (string ==  "pair-plus") return eArgRepeatPairPlus;
+  if (string ==  "pair-star") return eArgRepeatPairStar;
+  if (string ==  "pair-range") return eArgRepeatPairRange;
+  if (string ==  "pair-range-optional") return eArgRepeatPairRangeOptional;
+  return {};
+}
+
 static CommandObject::CommandArgumentEntry
 OptSetFiltered(uint32_t opt_set_mask,
                CommandObject::CommandArgumentEntry &cmd_arg_entry) {
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
index 82eee76e42b27..88c1bb7e729e7 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
@@ -194,6 +194,8 @@ template <typename T, char F> struct PassthroughFormat {
 };
 
 template <> struct PythonFormat<char *> : PassthroughFormat<char *, 's'> {};
+template <> struct PythonFormat<const char *> : 
+    PassthroughFormat<const char *, 's'> {};
 template <> struct PythonFormat<char> : PassthroughFormat<char, 'b'> {};
 template <>
 struct PythonFormat<unsigned char> : PassthroughFormat<unsigned char, 'B'> {};
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h b/lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h
index 7cdd5577919ba..c1a11b9134d62 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h
@@ -32,6 +32,7 @@ class SBStream;
 class SBStructuredData;
 class SBFileSpec;
 class SBModuleSpec;
+class SBStringList;
 } // namespace lldb
 
 namespace lldb_private {
@@ -212,6 +213,12 @@ class SWIGBridge {
                                   lldb::DebuggerSP debugger, const char *args,
                                   lldb_private::CommandReturnObject &cmd_retobj,
                                   lldb::ExecutionContextRefSP exe_ctx_ref_sp);
+  static bool
+  LLDBSwigPythonCallParsedCommandObject(PyObject *implementor,
+                                  lldb::DebuggerSP debugger,  
+                                  StructuredDataImpl &args_impl,
+                                  lldb_private::CommandReturnObject &cmd_retobj,
+                                  lldb::ExecutionContextRefSP exe_ctx_ref_sp);
 
   static bool LLDBSwigPythonCallModuleInit(const char *python_module_name,
                                            const char *session_dictionary_name,
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
index ef7a2c128a220..114a1b9a45cf2 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -24,6 +24,7 @@
 #include "ScriptInterpreterPythonImpl.h"
 
 #include "lldb/API/SBError.h"
+#include "lldb/API/SBExecutionContext.h"
 #include "lldb/API/SBFrame.h"
 #include "lldb/API/SBValue.h"
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
@@ -531,7 +532,6 @@ void ScriptInterpreterPythonImpl::IOHandlerInputComplete(IOHandler &io_handler,
         break;
       data_up->user_source.SplitIntoLines(data);
 
-      StructuredData::ObjectSP empty_args_sp;
       if (GenerateBreakpointCommandCallbackData(data_up->user_source,
                                                 data_up->script_source,
                                                 /*has_extra_args=*/false,
@@ -2766,6 +2766,58 @@ bool ScriptInterpreterPythonImpl::RunScriptBasedCommand(
   return ret_val;
 }
 
+bool ScriptInterpreterPythonImpl::RunScriptBasedParsedCommand(
+    StructuredData::GenericSP impl_obj_sp, Args &args,
+    ScriptedCommandSynchronicity synchronicity,
+    lldb_private::CommandReturnObject &cmd_retobj, Status &error,
+    const lldb_private::ExecutionContext &exe_ctx) {
+  if (!impl_obj_sp || !impl_obj_sp->IsValid()) {
+    error.SetErrorString("no function to execute");
+    return false;
+  }
+
+  lldb::DebuggerSP debugger_sp = m_debugger.shared_from_this();
+  lldb::ExecutionContextRefSP exe_ctx_ref_sp(new ExecutionContextRef(exe_ctx));
+
+  if (!debugger_sp.get()) {
+    error.SetErrorString("invalid Debugger pointer");
+    return false;
+  }
+
+  bool ret_val = false;
+
+  std::string err_msg;
+
+  {
+    Locker py_lock(this,
+                   Locker::AcquireLock | Locker::InitSession |
+                       (cmd_retobj.GetInteractive() ? 0 : Locker::NoSTDIN),
+                   Locker::FreeLock | Locker::TearDownSession);
+
+    SynchronicityHandler synch_handler(debugger_sp, synchronicity);
+
+    StructuredData::ArraySP args_arr_sp(new StructuredData::Array());
+
+    for (const Args::ArgEntry &entry : args) {
+      args_arr_sp->AddStringItem(entry.ref());
+    }
+    StructuredDataImpl args_impl(args_arr_sp);
+    
+    ret_val = SWIGBridge::LLDBSwigPythonCallParsedCommandObject(
+        static_cast<PyObject *>(impl_obj_sp->GetValue()), debugger_sp,
+        args_impl, cmd_retobj, exe_ctx_ref_sp);
+  }
+
+  if (!ret_val)
+    error.SetErrorString("unable to execute script function");
+  else if (cmd_retobj.GetStatus() == eReturnStatusFailed)
+    return false;
+
+  error.Clear();
+  return ret_val;
+}
+
+
 /// In Python, a special attribute __doc__ contains the docstring for an object
 /// (function, method, class, ...) if any is defined Otherwise, the attribute's
 /// value is None.
@@ -2884,6 +2936,204 @@ uint32_t ScriptInterpreterPythonImpl::GetFlagsForCommandObject(
   return result;
 }
 
+StructuredData::ObjectSP 
+ScriptInterpreterPythonImpl::GetOptionsForCommandObject(
+    StructuredData::GenericSP cmd_obj_sp) {
+  StructuredData::ObjectSP result = {};
+
+  Locker py_lock(this, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock);
+
+  static char callee_name[] = "get_options_definition";
+
+  if (!cmd_obj_sp)
+    return result;
+
+  PythonObject implementor(PyRefType::Borrowed,
+                           (PyObject *)cmd_obj_sp->GetValue());
+
+  if (!implementor.IsAllocated())
+    return result;
+
+  PythonObject pmeth(PyRefType::Owned,
+                     PyObject_GetAttrString(implementor.get(), callee_name));
+
+  if (PyErr_Occurred())
+    PyErr_Clear();
+
+  if (!pmeth.IsAllocated())
+    return result;
+
+  if (PyCallable_Check(pmeth.get()) == 0) {
+    if (PyErr_Occurred())
+      PyErr_Clear();
+    return result;
+  }
+
+  if (PyErr_Occurred())
+    PyErr_Clear();
+
+  PythonList py_return = unwrapOrSetPythonException(
+      As<PythonList>(implementor.CallMethod(callee_name)));
+
+  // if it fails, print the error but otherwise go on
+  if (PyErr_Occurred()) {
+    PyErr_Print();
+    PyErr_Clear();
+    return {};
+  }
+    return py_return.CreateStructuredObject();
+}
+
+StructuredData::ObjectSP 
+ScriptInterpreterPythonImpl::GetArgumentsForCommandObject(
+    StructuredData::GenericSP cmd_obj_sp) {
+  StructuredData::ObjectSP result = {};
+
+  Locker py_lock(this, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock);
+
+  static char callee_name[] = "get_args_definition";
+
+  if (!cmd_obj_sp)
+    return result;
+
+  PythonObject implementor(PyRefType::Borrowed,
+                           (PyObject *)cmd_obj_sp->GetValue());
+
+  if (!implementor.IsAllocated())
+    return result;
+
+  PythonObject pmeth(PyRefType::Owned,
+                     PyObject_GetAttrString(implementor.get(), callee_name));
+
+  if (PyErr_Occurred())
+    PyErr_Clear();
+
+  if (!pmeth.IsAllocated())
+    return result;
+
+  if (PyCallable_Check(pmeth.get()) == 0) {
+    if (PyErr_Occurred())
+      PyErr_Clear();
+    return result;
+  }
+
+  if (PyErr_Occurred())
+    PyErr_Clear();
+
+  PythonList py_return = unwrapOrSetPythonException(
+      As<PythonList>(implementor.CallMethod(callee_name)));
+
+  // if it fails, print the error but otherwise go on
+  if (PyErr_Occurred()) {
+    PyErr_Print();
+    PyErr_Clear();
+    return {};
+  }
+    return py_return.CreateStructuredObject();
+}
+
+void 
+ScriptInterpreterPythonImpl::OptionParsingStartedForCommandObject(
+    StructuredData::GenericSP cmd_obj_sp) {
+
+  Locker py_lock(this, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock);
+
+  static char callee_name[] = "option_parsing_started";
+
+  if (!cmd_obj_sp)
+    return ;
+
+  PythonObject implementor(PyRefType::Borrowed,
+                           (PyObject *)cmd_obj_sp->GetValue());
+
+  if (!implementor.IsAllocated())
+    return;
+
+  PythonObject pmeth(PyRefType::Owned,
+                     PyObject_GetAttrString(implementor.get(), callee_name));
+
+  if (PyErr_Occurred())
+    PyErr_Clear();
+
+  if (!pmeth.IsAllocated())
+    return;
+
+  if (PyCallable_Check(pmeth.get()) == 0) {
+    if (PyErr_Occurred())
+      PyErr_Clear();
+    return;
+  }
+
+  if (PyErr_Occurred())
+    PyErr_Clear();
+
+  // FIXME: this should really be a void function
+  bool py_return = unwrapOrSetPythonException(
+      As<bool>(implementor.CallMethod(callee_name)));
+
+  // if it fails, print the error but otherwise go on
+  if (PyErr_Occurred()) {
+    PyErr_Print();
+    PyErr_Clear();
+    return;
+  }
+}
+
+bool
+ScriptInterpreterPythonImpl::SetOptionValueForCommandObject(
+    StructuredData::GenericSP cmd_obj_sp, ExecutionContext *exe_ctx,
+    llvm::StringRef long_option, llvm::StringRef value) {
+  StructuredData::ObjectSP result = {};
+
+  Locker py_lock(this, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock);
+
+  static char callee_name[] = "set_option_value";
+
+  if (!cmd_obj_sp)
+    return false;
+
+  PythonObject implementor(PyRefType::Borrowed,
+                           (PyObject *)cmd_obj_sp->GetValue());
+
+  if (!implementor.IsAllocated())
+    return false;
+
+  PythonObject pmeth(PyRefType::Owned,
+                     PyObject_GetAttrString(implementor.get(), callee_name));
+
+  if (PyErr_Occurred())
+    PyErr_Clear();
+
+  if (!pmeth.IsAllocated())
+    return false;
+
+  if (PyCallable_Check(pmeth.get()) == 0) {
+    if (PyErr_Occurred())
+      PyErr_Clear();
+    return false;
+  }
+
+  if (PyErr_Occurred())
+    PyErr_Clear();
+    
+  lldb::ExecutionContextRefSP exe_ctx_ref_sp;
+  if (exe_ctx)
+    exe_ctx_ref_sp.reset(new ExecutionContextRef(exe_ctx));
+  PythonObject ctx_ref_obj = SWIGBridge::ToSWIGWrapper(exe_ctx_ref_sp);
+    
+  bool py_return = unwrapOrSetPythonException(
+      As<bool>(implementor.CallMethod(callee_name, ctx_ref_obj, long_option.str().c_str(), 
+                                      value.str().c_str())));
+
+  // if it fails, print the error but otherwise go on
+  if (PyErr_Occurred()) {
+    PyErr_Print();
+    PyErr_Clear();
+    return false;
+  }
+  return py_return;
+}
+
 bool ScriptInterpreterPythonImpl::GetLongHelpForCommandObject(
     StructuredData::GenericSP cmd_obj_sp, std::string &dest) {
   dest.clear();
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
index a33499816d8d3..fcd21dff612b1 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
@@ -182,6 +182,13 @@ class ScriptInterpreterPythonImpl : public ScriptInterpreterPython {
       lldb_private::CommandReturnObject &cmd_retobj, Status &error,
       const lldb_private::ExecutionContext &exe_ctx) override;
 
+    virtual bool RunScriptBasedParsedCommand(
+      StructuredData::GenericSP impl_obj_sp, Args& args,
+      ScriptedCommandSynchronicity synchronicity,
+      lldb_private::CommandReturnObject &cmd_retobj, Status &error,
+      const lldb_private::ExecutionContext &exe_ctx) override;
+
+  
   Status GenerateFunction(const char *signature, const StringList &input,
                           bool is_callback) override;
 
@@ -212,6 +219,20 @@ class ScriptInterpreterPythonImpl : public ScriptInterpreterPython {
 
   bool GetLongHelpForCommandObject(StructuredData::GenericSP cmd_obj_sp,
                                    std::string &dest) override;
+                                   
+  StructuredData::ObjectSP
+  GetOptionsForCommandObject(StructuredData::GenericSP cmd_obj_sp) override;
+
+  StructuredData::ObjectSP
+  GetArgumentsForCommandObject(StructuredData::GenericSP cmd_obj_sp) override;
+
+  bool SetOptionValueForCommandObject(StructuredData::GenericSP cmd_obj_sp,
+                                      ExecutionContext *exe_ctx,
+                                      llvm::StringRef long_option, 
+                                      llvm::StringRef value) override;
+
+  void OptionParsingStartedForCommandObject(
+      StructuredData::GenericSP cmd_obj_sp) override;
 
   bool CheckObjectExists(const char *name) override {
     if (!name || !name[0])
diff --git a/lldb/test/API/commands/command/script/add/TestAddParsedCommand.py b/lldb/test/API/commands/command/script/add/TestAddParsedCommand.py
new file mode 100644
index 0000000000000..7dba9c6937f21
--- /dev/null
+++ b/lldb/test/API/commands/command/script/add/TestAddParsedCommand.py
@@ -0,0 +1,146 @@
+"""
+Test option and argument definitions in parsed script commands
+"""
+
+
+import sys
+import os
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+
+class ParsedCommandTestCase(TestBase):
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def test(self):
+        self.pycmd_tests()
+
+    def check_help_options(self, cmd_name, opt_list, substrs = []):
+        """
+        Pass the command name in cmd_name and a vector of the short option, type & long option.
+        This will append the checks for all the options and test "help command".
+        Any strings already in substrs will also be checked.
+        Any element in opt list that begin with "+" will be added to the checked strings as is.
+        """
+        for elem in opt_list:
+            if elem[0] == "+":
+                substrs.append(elem[1:])
+            else:
+                (short_opt, type, long_opt) = elem
+                substrs.append(f"-{short_opt} <{type}> ( --{long_opt} <{type}> )")
+        print(f"Opt Vec\n{substrs}")
+        self.expect("help " + cmd_name, substrs = substrs)
+
+    def pycmd_tests(self):
+        source_dir = self.getSourceDir()
+        test_file_path = os.path.join(source_dir, "test_commands.py")
+        self.runCmd("command script import " + test_file_path)
+        self.expect("help", substrs = ["no-args", "one-arg-no-opt", "two-args"])
+
+        # Test that we did indeed add these commands as user commands:
+
+        # This is the function to remove the custom commands in order to have a
+        # clean slate for the next test case.
+        def cleanup():
+            self.runCmd("command script delete no-args one-arg-no-opt two-args", check=False)
+
+        # Execute the cleanup function during test case tear down.
+        self.addTearDownHook(cleanup)
+
+        # First test the no arguments command.  Make sure the help is right:
+        no_arg_opts = [["b", "boolean", "bool-arg"],
+                       "+a boolean arg, defaults to True",
+                       ["d", "filename", "disk-file-name"],
+                       "+An on disk filename",
+                       ["e", "none", "enum-option"],
+                       "+An enum, doesn't actually do anything",
+                       "+Values: foo | bar | baz",
+                       ["l", "linenum", "line-num"],
+                       "+A line number",
+                       ["s", "shlib-name", "shlib-name"],
+                       "+A shared library name"]
+        substrs = ["Example command for use in debugging",
+                   "Syntax: no-args <cmd-options>"]
+        
+        self.check_help_options("no-args", no_arg_opts, substrs)
+
+        # Make sure the command doesn't accept arguments:
+        self.expect("no-args an-arg", substrs=["'no-args' doesn't take any arguments."],
+                    error=True)
+
+        # Try setting the bool with the wrong value:
+        self.expect("no-args -b Something",
+                    substrs=["Error setting option: bool-arg to Something"],
+                    error=True)
+        # Try setting the enum to an illegal value as well:
+        self.expect("no-args --enum-option Something",
+                    substrs=["error: Error setting option: enum-option to Something"],
+                    error=True)
+        
+        # Check some of the command groups:
+        self.expect("no-args -b true -s Something -l 10",
+                    substrs=["error: invalid combination of options for the given command"],
+                    error=True)
+                    
+        # Now set the bool arg correctly, note only the first option was set:
+        self.expect("no-args -b true", substrs=["bool-arg (set: True): True",
+                                                "shlib-name (set: False):",
+                                                "disk-file-name (set: False):",
+                                                "line-num (set: False):",
+                                                "enum-option (set: False):"])
+
+        # Now set the enum arg correctly, note only the first option was set:
+        self.expect("no-args -e foo", substrs=["bool-arg (set: False):",
+                                                "shlib-name (set: False):",
+                                                "disk-file-name (set: False):",
+                                                "line-num (set: False):",
+                                                "enum-option (set: True): foo"])
+        # Try a pair together:
+        self.expect("no-args -b false -s Something", substrs=["bool-arg (set: True): False",
+                                                "shlib-name (set: True): Something",
+                                                "disk-file-name (set: False):",
+                                                "line-num (set: False):",
+                                                "enum-option (set: False):"])
+
+        # Next try some completion tests:
+
+        interp = self.dbg.GetCommandInterpreter()
+        matches = lldb.SBStringList()
+        descriptions = lldb.SBStringList()
+
+        # First try an enum completion: 
+        num_completions = interp.HandleCompletionWithDescriptions("no-args -e f", 12, 0,
+                                                                  1000, matches, descriptions)
+        self.assertEqual(num_completions, 1, "Only one completion for foo")
+        self.assertEqual(matches.GetSize(), 2, "The first element is the complete additional text")
+        self.assertEqual(matches.GetStringAtIndex(0), "oo ", "And we got the right extra characters")
+        self.assertEqual(matches.GetStringAtIndex(1), "foo", "And we got the right match")
+        self.assertEqual(descriptions.GetSize(), 2, "descriptions matche the return length")
+        # FIXME: we don't return descriptions for enum elements
+        #self.assertEqual(descriptions.GetStringAtIndex(1), "does foo things", "And we got the right description")
+
+        # Now try an internal completer, the on disk file one is handy:
+        partial_name = os.path.join(source_dir, "test_")
+        cmd_str = f"no-args -d '{partial_name}'"
+
+        matches.Clear()
+        descriptions.Clear()
+        num_completions = interp.HandleCompletionWithDescriptions(cmd_str, len(cmd_str) - 1, 0,
+                                                                  1000, matches, descriptions)
+        print(f"First: {matches.GetStringAtIndex(0)}\nSecond: {matches.GetStringAtIndex(1)}\nThird: {matches.GetStringAtIndex(2)}")
+        self.assertEqual(num_completions, 1, "Only one completion for source file")
+        self.assertEqual(matches.GetSize(), 2, "The first element is the complete line")
+        self.assertEqual(matches.GetStringAtIndex(0), "commands.py' ", "And we got the right extra characters")
+        self.assertEqual(matches.GetStringAtIndex(1), test_file_path, "And we got the right match")
+        self.assertEqual(descriptions.GetSize(), 2, "descriptions match the return length")
+        # FIXME: we don't return descriptions for enum elements
+        #self.assertEqual(descriptions.GetStringAtIndex(1), "does foo things", "And we got the right description")
+        
+        # Try a command with arguments.
+        # FIXME: It should be enough to define an argument and it's type to get the completer
+        # wired up for that argument type if it is a known type. But that isn't wired up in the
+        # command parser yet, so I don't have any tests for that.  We also don't currently check
+        # that the arguments passed match the argument specifications, so here I just pass a couple
+        # sets of arguments and make sure we get back what we put in:
+        self.expect("two-args 'First Argument' 'Second Argument'", substrs=["0: First Argument", "1: Second Argument"])
diff --git a/lldb/test/API/commands/command/script/add/test_commands.py b/lldb/test/API/commands/command/script/add/test_commands.py
new file mode 100644
index 0000000000000..b704b833ae7b8
--- /dev/null
+++ b/lldb/test/API/commands/command/script/add/test_commands.py
@@ -0,0 +1,175 @@
+"""
+Test defining commands using the lldb command definitions
+"""
+import inspect
+import sys
+import lldb
+from lldb.utils.parsed_cmd import ParsedCommandBase
+
+class ReportingCmd(ParsedCommandBase):
+    def __init__(self, debugger, unused):
+        super().__init__(debugger, unused)
+
+    def __call__(self, debugger, args_array, exe_ctx, result):
+        opt_def = self.get_options_definition()
+        if len(opt_def):
+            result.AppendMessage("Options:\n")
+            for elem in opt_def:
+                long_option = elem["long_option"]
+                varname = elem["varname"]
+                result.AppendMessage(f"{long_option} (set: {elem['_value_set']}): {object.__getattribute__(self.ov_parser, varname)}\n")
+        else:
+            result.AppendMessage("No options\n")
+
+        num_args = args_array.GetSize()
+        if num_args > 0:
+            result.AppendMessage(f"{num_args} arguments:")
+        for idx in range(0,num_args):
+          result.AppendMessage(f"{idx}: {args_array.GetItemAtIndex(idx).GetStringValue(10000)}\n")
+    
+class NoArgsCommand(ReportingCmd):
+    program = "no-args"
+
+    def __init__(self, debugger, unused):
+        super().__init__(debugger, unused)
+
+    @classmethod
+    def register_lldb_command(cls, debugger, module_name):
+        ParsedCommandBase.do_register_cmd(cls, debugger, module_name)
+
+    def setup_command_definition(self):
+        self.ov_parser.add_option(
+            "b",
+            "bool-arg",
+            "a boolean arg, defaults to True",
+            value_type = lldb.eArgTypeBoolean,
+            groups = [1,2],
+            varname = "bool_arg",
+            default = True
+        )
+
+        self.ov_parser.add_option(
+            "s",
+            "shlib-name",
+            "A shared library name.",
+            value_type=lldb.eArgTypeShlibName,
+            groups = [1, [3,4]],
+            varname = "shlib_name",
+            default = None
+        )
+
+        self.ov_parser.add_option(
+            "d",
+            "disk-file-name",
+            "An on disk filename",
+            value_type = lldb.eArgTypeFilename,
+            varname = "disk_file_name",
+            default = None
+        )
+
+        self.ov_parser.add_option(
+            "l",
+            "line-num",
+            "A line number",
+            value_type = lldb.eArgTypeLineNum,
+            groups = 3,
+            varname = "line_num",
+            default = 0
+        )
+        
+        self.ov_parser.add_option(
+            "e",
+            "enum-option",
+            "An enum, doesn't actually do anything",
+            enum_values = [["foo", "does foo things"],
+                           ["bar", "does bar things"],
+                           ["baz", "does baz things"]],
+            groups = 4,
+            varname = "enum_option",
+            default = "foo"
+        )
+        
+    def get_short_help(self):
+        return "Example command for use in debugging"
+
+    def get_long_help(self):
+        return self.help_string
+
+class OneArgCommandNoOptions(ReportingCmd):
+    program = "one-arg-no-opt"
+
+    def __init__(self, debugger, unused):
+        super().__init__(debugger, unused)
+
+    @classmethod
+    def register_lldb_command(cls, debugger, module_name):
+        ParsedCommandBase.do_register_cmd(cls, debugger, module_name)
+
+    def setup_command_definition(self):
+        self.ov_parser.add_argument_set([self.ov_parser.make_argument_element(lldb.eArgTypeSourceFile, "plain")])
+        
+    def get_short_help(self):
+        return "Example command for use in debugging"
+
+    def get_long_help(self):
+        return self.help_string
+
+class TwoArgGroupsCommand(ReportingCmd):
+    program = "two-args"
+
+    def __init__(self, debugger, unused):
+        super().__init__(debugger, unused)
+
+    @classmethod
+    def register_lldb_command(cls, debugger, module_name):
+        ParsedCommandBase.do_register_cmd(cls, debugger, module_name)
+
+    def setup_command_definition(self):
+        self.ov_parser.add_option(
+            "l",
+            "language",
+            "language defaults to None",
+            value_type = lldb.eArgTypeLanguage,
+            groups = [1,2],
+            varname = "language",
+            default = None
+        )
+
+        self.ov_parser.add_option(
+            "c",
+            "log-channel",
+            "log channel - defaults to lldb",
+            value_type=lldb.eArgTypeLogChannel,
+            groups = [1, 3],
+            varname = "log_channel",
+            default = "lldb"
+        )
+
+        self.ov_parser.add_option(
+            "p",
+            "process-name",
+            "A process name, defaults to None",
+            value_type = lldb.eArgTypeProcessName,
+            varname = "proc_name",
+            default = None
+        )
+
+        self.ov_parser.add_argument_set([self.ov_parser.make_argument_element(lldb.eArgTypeClassName, "plain", [1,2]),
+                         self.ov_parser.make_argument_element(lldb.eArgTypeOffset, "optional", [1,2])])
+
+        self.ov_parser.add_argument_set([self.ov_parser.make_argument_element(lldb.eArgTypePythonClass, "plain", [3,4]),
+                         self.ov_parser.make_argument_element(lldb.eArgTypePid, "optional", [3,4])])
+        
+    def get_short_help(self):
+        return "Example command for use in debugging"
+
+    def get_long_help(self):
+        return self.help_string
+
+def __lldb_init_module(debugger, dict):
+    # Register all classes that have a register_lldb_command method
+    for _name, cls in inspect.getmembers(sys.modules[__name__]):
+        if inspect.isclass(cls) and callable(
+            getattr(cls, "register_lldb_command", None)
+        ):
+            cls.register_lldb_command(debugger, __name__)
diff --git a/lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp b/lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
index 7f3359f6bf26b..5f0cc4c23db7b 100644
--- a/lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
+++ b/lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
@@ -218,6 +218,14 @@ bool lldb_private::python::SWIGBridge::LLDBSwigPythonCallCommandObject(
   return false;
 }
 
+bool lldb_private::python::SWIGBridge::LLDBSwigPythonCallParsedCommandObject(
+    PyObject *implementor, lldb::DebuggerSP debugger, 
+    StructuredDataImpl &args_impl,
+    lldb_private::CommandReturnObject &cmd_retobj,
+    lldb::ExecutionContextRefSP exe_ctx_ref_sp) {
+  return false;
+}
+
 bool lldb_private::python::SWIGBridge::LLDBSwigPythonCallModuleInit(
     const char *python_module_name, const char *session_dictionary_name,
     lldb::DebuggerSP debugger) {

>From 580f68d23995f8f9529d5e84944aa406c3036aca Mon Sep 17 00:00:00 2001
From: Jim Ingham <jingham at apple.com>
Date: Sun, 21 Jan 2024 08:52:22 -0800
Subject: [PATCH 2/4] First pass at addressing review comments

---
 lldb/bindings/python/CMakeLists.txt           |  3 +-
 lldb/bindings/python/python-wrapper.swig      | 18 +---
 lldb/examples/python/parsed_cmd.py            | 39 +++++---
 .../source/Commands/CommandObjectCommands.cpp | 99 +++++++++++--------
 lldb/source/Interpreter/CommandObject.cpp     | 25 ++---
 .../Python/ScriptInterpreterPython.cpp        |  5 +-
 6 files changed, 105 insertions(+), 84 deletions(-)

diff --git a/lldb/bindings/python/CMakeLists.txt b/lldb/bindings/python/CMakeLists.txt
index 657fdd2c95900..7e653ea04c2ed 100644
--- a/lldb/bindings/python/CMakeLists.txt
+++ b/lldb/bindings/python/CMakeLists.txt
@@ -96,8 +96,9 @@ function(finish_swig_python swig_target lldb_python_bindings_dir lldb_python_tar
     ${lldb_python_target_dir}
     "utils"
     FILES "${LLDB_SOURCE_DIR}/examples/python/in_call_stack.py"
+          "${LLDB_SOURCE_DIR}/examples/python/parsed_cmd.py"
           "${LLDB_SOURCE_DIR}/examples/python/symbolication.py"
-          "${LLDB_SOURCE_DIR}/examples/python/parsed_cmd.py")
+  )
 
   create_python_package(
     ${swig_target}
diff --git a/lldb/bindings/python/python-wrapper.swig b/lldb/bindings/python/python-wrapper.swig
index 47887491d0c55..bf31bef473128 100644
--- a/lldb/bindings/python/python-wrapper.swig
+++ b/lldb/bindings/python/python-wrapper.swig
@@ -287,12 +287,12 @@ PythonObject lldb_private::python::SWIGBridge::LLDBSwigPythonCreateScriptedThrea
 }
 
 bool lldb_private::python::SWIGBridge::LLDBSWIGPythonCallThreadPlan(
-    void *implementor, const char *method_name, lldb_private::Event *event,
+    void *implementer, const char *method_name, lldb_private::Event *event,
     bool &got_error) {
   got_error = false;
 
   PyErr_Cleaner py_err_cleaner(false);
-  PythonObject self(PyRefType::Borrowed, static_cast<PyObject *>(implementor));
+  PythonObject self(PyRefType::Borrowed, static_cast<PyObject *>(implementer));
   auto pfunc = self.ResolveName<PythonCallable>(method_name);
 
   if (!pfunc.IsAllocated())
@@ -325,12 +325,12 @@ bool lldb_private::python::SWIGBridge::LLDBSWIGPythonCallThreadPlan(
 }
 
 bool lldb_private::python::SWIGBridge::LLDBSWIGPythonCallThreadPlan(
-    void *implementor, const char *method_name, lldb_private::Stream *stream,
+    void *implementer, const char *method_name, lldb_private::Stream *stream,
     bool &got_error) {
   got_error = false;
 
   PyErr_Cleaner py_err_cleaner(false);
-  PythonObject self(PyRefType::Borrowed, static_cast<PyObject *>(implementor));
+  PythonObject self(PyRefType::Borrowed, static_cast<PyObject *>(implementer));
   auto pfunc = self.ResolveName<PythonCallable>(method_name);
 
   if (!pfunc.IsAllocated()) 
@@ -845,16 +845,6 @@ bool lldb_private::python::SWIGBridge::LLDBSwigPythonCallParsedCommandObject(
     return false;
 
   auto cmd_retobj_arg = SWIGBridge::ToSWIGWrapper(cmd_retobj);
-
-  // FIXME:
-  // I wanted to do something like:
-  // size_t num_elem = args.size();
-  // PythonList my_list(num_elem);
-  // for (const char *elem : args)
-  //   my_list.append(PythonString(elem);
-  //
-  // and then pass my_list to the pfunc, but that crashes somewhere
-  // deep in Python for reasons that aren't clear to me.
   
   pfunc(SWIGBridge::ToSWIGWrapper(std::move(debugger)), SWIGBridge::ToSWIGWrapper(args_impl),
         SWIGBridge::ToSWIGWrapper(exe_ctx_ref_sp), cmd_retobj_arg.obj());
diff --git a/lldb/examples/python/parsed_cmd.py b/lldb/examples/python/parsed_cmd.py
index 7ee9e077d49ab..c8f7e4e09f866 100644
--- a/lldb/examples/python/parsed_cmd.py
+++ b/lldb/examples/python/parsed_cmd.py
@@ -1,50 +1,51 @@
 """
 This module implements a couple of utility classes to make writing
 lldb parsed commands more Pythonic.
-The way to use it is to make a class for you command that inherits from ParsedCommandBase.
+The way to use it is to make a class for your command that inherits from ParsedCommandBase.
 That will make an LLDBOVParser which you will use for your
 option definition, and to fetch option values for the current invocation
 of your command.  Access to the OV parser is through:
 
 ParsedCommandBase.get_parser()
 
-Next, implement setup_command_definition in your new command class, and call:
+Next, implement setup_command_definition() in your new command class, and call:
 
-  self.get_parser().add_option
+  self.get_parser().add_option()
 
 to add all your options.  The order doesn't matter for options, lldb will sort them
 alphabetically for you when it prints help.
 
 Similarly you can define the arguments with:
 
-  self.get_parser.add_argument
+  self.get_parser().add_argument()
 
-at present, lldb doesn't do as much work as it should verifying arguments, it pretty
-much only checks that commands that take no arguments don't get passed arguments.
+At present, lldb doesn't do as much work as it should verifying arguments, it
+only checks that commands that take no arguments don't get passed arguments.
 
 Then implement the execute function for your command as:
 
-    def __call__(self, debugger, args_array, exe_ctx, result):
+    def __call__(self, debugger, args_list, exe_ctx, result):
+
+The arguments will be a list of strings.  
 
-The arguments will be in a python array as strings.  
+You can access the option values using the 'varname' string you passed in when defining the option.
 
-You can access the option values using varname you passed in when defining the option.  
 If you need to know whether a given option was set by the user or not, you can retrieve 
 the option definition array with:
 
   self.get_options_definition()
 
-look up your element by varname and check the "_value_set" element.
+then look up your element by the 'varname' field, and check the "_value_set" element.
+FIXME: I should add a convenience method to do this.
 
 There are example commands in the lldb testsuite at:
 
 llvm-project/lldb/test/API/commands/command/script/add/test_commands.py
-
-FIXME: I should make a convenient wrapper for that. 
 """
 import inspect
 import lldb
 import sys
+from abc import abstractmethod
 
 class LLDBOVParser:
     def __init__(self):
@@ -59,12 +60,16 @@ def __init__(self):
     def to_bool(in_value):
         error = True
         value = False
+        print(f"TYPE: {type(in_value)}")
+        if type(in_value) != str or len(in_value) == 0:
+            return (value, error)
+
         low_in = in_value.lower()
-        if low_in == "yes" or low_in == "true" or low_in == "1":
+        if low_in == "y" or low_in == "yes" or low_in == "t" or low_in == "true" or low_in == "1":
             value = True
             error = False
             
-        if not value and low_in == "no" or low_in == "false" or low_in == "0":
+        if not value and low_in == "n" or low_in == "no" or low_in == "f" or low_in == "false" or low_in == "0":
             value = False
             error = False
 
@@ -75,6 +80,7 @@ def to_int(in_value):
         #FIXME: Not doing errors yet...
         return (int(in_value), False)
 
+    @staticmethod
     def to_unsigned(in_value):
         # FIXME: find an unsigned converter...
         # And handle errors.
@@ -102,7 +108,6 @@ def to_unsigned(in_value):
 
     @classmethod
     def translate_value(cls, value_type, value):
-        error = False
         try:
             return cls.translators[value_type](value)
         except KeyError:
@@ -176,7 +181,7 @@ def option_parsing_started(self):
     def set_enum_value(self, enum_values, input):
         candidates = []
         for candidate in enum_values:
-            # The enum_values are a duple of value & help string.
+            # The enum_values are a two element list of value & help string.
             value = candidate[0]
             if value.startswith(input):
                 candidates.append(value)
@@ -294,9 +299,11 @@ def set_option_value(self, exe_ctx, opt_name, opt_value):
         return self.get_parser().set_option_value(exe_ctx, opt_name, opt_value)
 
     # These are the two "pure virtual" methods:
+    @abstractmethod
     def __call__(self, debugger, args_array, exe_ctx, result):
         raise NotImplementedError()
 
+    @abstractmethod
     def setup_command_definition(self):
         raise NotImplementedError()
 
diff --git a/lldb/source/Commands/CommandObjectCommands.cpp b/lldb/source/Commands/CommandObjectCommands.cpp
index 68e3438a9b7ee..e23751a247c18 100644
--- a/lldb/source/Commands/CommandObjectCommands.cpp
+++ b/lldb/source/Commands/CommandObjectCommands.cpp
@@ -1152,13 +1152,13 @@ class CommandObjectPythonFunction : public CommandObjectRaw {
 /// This class implements a "raw" scripted command.  lldb does no parsing of the
 /// command line, instead passing the line unaltered (except for backtick
 /// substitution).
-class CommandObjectScriptingObject : public CommandObjectRaw {
+class CommandObjectScriptingObjectRaw : public CommandObjectRaw {
 public:
-  CommandObjectScriptingObject(CommandInterpreter &interpreter,
-                               std::string name,
-                               StructuredData::GenericSP cmd_obj_sp,
-                               ScriptedCommandSynchronicity synch,
-                               CompletionType completion_type)
+  CommandObjectScriptingObjectRaw(CommandInterpreter &interpreter,
+                                  std::string name,
+                                  StructuredData::GenericSP cmd_obj_sp,
+                                  ScriptedCommandSynchronicity synch,
+                                  CompletionType completion_type)
       : CommandObjectRaw(interpreter, name), m_cmd_obj_sp(cmd_obj_sp),
         m_synchro(synch), m_fetched_help_short(false),
         m_fetched_help_long(false), m_completion_type(completion_type) {
@@ -1169,7 +1169,7 @@ class CommandObjectScriptingObject : public CommandObjectRaw {
       GetFlags().Set(scripter->GetFlagsForCommandObject(cmd_obj_sp));
   }
 
-  ~CommandObjectScriptingObject() override = default;
+  ~CommandObjectScriptingObjectRaw() override = default;
 
   void
   HandleArgumentCompletion(CompletionRequest &request,
@@ -1304,14 +1304,11 @@ class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
     void OptionParsingStarting(ExecutionContext *execution_context) override {
       ScriptInterpreter *scripter = 
         m_interpreter.GetDebugger().GetScriptInterpreter();
-      if (!scripter) {
+      if (!scripter || !m_cmd_obj_sp)
         return;
-      }
-      if (!m_cmd_obj_sp) {
-        return;
-      }
+
       scripter->OptionParsingStartedForCommandObject(m_cmd_obj_sp);
-    };
+    }
 
     llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
       if (!m_options_definition_up)
@@ -1319,17 +1316,18 @@ class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
       return llvm::ArrayRef(m_options_definition_up.get(), m_num_options);
     }
     
-    static bool ParseUsageMaskFromArray(StructuredData::ObjectSP obj_sp, 
-        size_t counter, uint32_t &usage_mask, Status &error) {
+    static Status ParseUsageMaskFromArray(StructuredData::ObjectSP obj_sp, 
+        size_t counter, uint32_t &usage_mask) {
       // If the usage entry is not provided, we use LLDB_OPT_SET_ALL.
       // If the usage mask is a UINT, the option belongs to that group.
       // If the usage mask is a vector of UINT's, the option belongs to all the
       // groups listed.
       // If a subelement of the vector is a vector of two ints, then the option
       // belongs to the inclusive range from the first to the second element.
+      Status error;
       if (!obj_sp) {
         usage_mask = LLDB_OPT_SET_ALL;
-        return true;
+        return error;
       }
       
       usage_mask = 0;
@@ -1342,17 +1340,17 @@ class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
         if (value == 0) {
           error.SetErrorStringWithFormatv(
               "0 is not a valid group for option {0}", counter);
-          return false;
+          return error;
         }
         usage_mask = (1 << (value - 1));
-        return true;
+        return error;
       }
       // Otherwise it has to be an array:
       StructuredData::Array *array_val = obj_sp->GetAsArray();
       if (!array_val) {
         error.SetErrorStringWithFormatv(
             "required field is not a array for option {0}", counter);
-        return false;
+        return error;
       }
       // This is the array ForEach for accumulating a group usage mask from
       // an array of string descriptions of groups.
@@ -1408,11 +1406,13 @@ class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
         }
         return true;
       };
-      return array_val->ForEach(groups_accumulator);
+      array_val->ForEach(groups_accumulator);
+      return error;
     }
     
     
-    Status SetOptionsFromArray(StructuredData::Array &options, Status &error) {
+    Status SetOptionsFromArray(StructuredData::Array &options) {
+      Status error;
       m_num_options = options.GetSize();
       m_options_definition_up.reset(new OptionDefinition[m_num_options]);
       // We need to hand out pointers to contents of these vectors; we reserve
@@ -1447,8 +1447,8 @@ class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
         // Usage Mask:
         StructuredData::ObjectSP obj_sp = opt_dict->GetValueForKey("groups");
         if (obj_sp) {
-          ParseUsageMaskFromArray(obj_sp, counter, option_def.usage_mask, 
-              error);
+          error = ParseUsageMaskFromArray(obj_sp, counter, 
+                                          option_def.usage_mask);
           if (error.Fail())
             return false;
         }
@@ -1694,6 +1694,34 @@ class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
   };
 
 public:
+  static CommandObjectSP Create(CommandInterpreter &interpreter, 
+                std::string name,
+                StructuredData::GenericSP cmd_obj_sp,
+                ScriptedCommandSynchronicity synch, 
+                CommandReturnObject &result) {
+    CommandObjectSP new_cmd_sp(new CommandObjectScriptingObjectParsed(
+        interpreter, name, cmd_obj_sp, synch));
+
+    CommandObjectScriptingObjectParsed *parsed_cmd 
+        = static_cast<CommandObjectScriptingObjectParsed *>(new_cmd_sp.get());
+    // Now check all the failure modes, and report if found.
+    Status opt_error = parsed_cmd->GetOptionsError();
+    Status arg_error = parsed_cmd->GetArgsError();
+
+    if (opt_error.Fail())
+      result.AppendErrorWithFormat("failed to parse option definitions: %s",
+                                   opt_error.AsCString());
+    if (arg_error.Fail())
+      result.AppendErrorWithFormat("%sfailed to parse argument definitions: %s",
+                                   opt_error.Fail() ? ", also " : "", 
+                                   arg_error.AsCString());
+
+    if (!result.Succeeded())
+      return {};
+
+    return new_cmd_sp;
+  }
+
   CommandObjectScriptingObjectParsed(CommandInterpreter &interpreter,
                                std::string name,
                                StructuredData::GenericSP cmd_obj_sp,
@@ -1720,7 +1748,7 @@ class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
       StructuredData::Array *options_array = options_object_sp->GetAsArray();
       // but if it exists, it has to be an array.
       if (options_array) {
-        m_options.SetOptionsFromArray(*(options_array), m_options_error);
+        m_options_error = m_options.SetOptionsFromArray(*(options_array));
         // If we got an error don't bother with the arguments...
         if (m_options_error.Fail())
           return;
@@ -1804,8 +1832,8 @@ class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
           
           // Usage Mask:
           obj_sp = arg_dict->GetValueForKey("groups");
-          CommandOptions::ParseUsageMaskFromArray(obj_sp, counter, 
-              arg_opt_set_association, m_args_error);
+          m_args_error = CommandOptions::ParseUsageMaskFromArray(obj_sp, 
+              counter, arg_opt_set_association);
           this_entry.emplace_back(arg_type, arg_repetition, 
               arg_opt_set_association);
           elem_counter++;
@@ -1879,7 +1907,7 @@ class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
 
 
 protected:
-  bool DoExecute(Args &args,
+  void DoExecute(Args &args,
                  CommandReturnObject &result) override {
     ScriptInterpreter *scripter = GetDebugger().GetScriptInterpreter();
 
@@ -1900,8 +1928,6 @@ class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
           result.SetStatus(eReturnStatusSuccessFinishResult);
       }
     }
-
-    return result.Succeeded();
   }
 
 private:
@@ -2306,17 +2332,12 @@ class CommandObjectCommandsScriptAdd : public CommandObjectParsed,
       }
       
       if (m_options.m_parsed_command) {
-        new_cmd_sp.reset(new CommandObjectScriptingObjectParsed(
-            m_interpreter, m_cmd_name, cmd_obj_sp, m_synchronicity));
-        Status options_error 
-            = static_cast<CommandObjectScriptingObjectParsed *>(new_cmd_sp.get())->GetOptionsError();
-        if (options_error.Fail()) {
-          result.AppendErrorWithFormat("failed to parse option definitions: %s",
-                             options_error.AsCString());
-          return false;
-        }
+        new_cmd_sp = CommandObjectScriptingObjectParsed::Create(m_interpreter, 
+            m_cmd_name, cmd_obj_sp, m_synchronicity, result);
+        if (!result.Succeeded())
+          return;
       } else
-        new_cmd_sp.reset(new CommandObjectScriptingObject(
+        new_cmd_sp.reset(new CommandObjectScriptingObjectRaw(
             m_interpreter, m_cmd_name, cmd_obj_sp, m_synchronicity,
             m_completion_type));
     }
diff --git a/lldb/source/Interpreter/CommandObject.cpp b/lldb/source/Interpreter/CommandObject.cpp
index 43b0b611a57c3..fd1d33d949f44 100644
--- a/lldb/source/Interpreter/CommandObject.cpp
+++ b/lldb/source/Interpreter/CommandObject.cpp
@@ -449,18 +449,19 @@ bool CommandObject::IsPairType(ArgumentRepetitionType arg_repeat_type) {
 
 std::optional<ArgumentRepetitionType> 
 CommandObject::ArgRepetitionFromString(llvm::StringRef string) {
-  if (string == "plain") return eArgRepeatPlain ;   
-  if (string ==  "optional") return eArgRepeatOptional;
-  if (string ==  "plus") return eArgRepeatPlus;
-  if (string ==  "star") return eArgRepeatStar; 
-  if (string ==  "range") return eArgRepeatRange;
-  if (string ==  "pair-plain") return eArgRepeatPairPlain;
-  if (string ==  "pair-optional") return eArgRepeatPairOptional;
-  if (string ==  "pair-plus") return eArgRepeatPairPlus;
-  if (string ==  "pair-star") return eArgRepeatPairStar;
-  if (string ==  "pair-range") return eArgRepeatPairRange;
-  if (string ==  "pair-range-optional") return eArgRepeatPairRangeOptional;
-  return {};
+  return llvm::StringSwitch<ArgumentRepetitionType>(string)
+  .Case("plain", eArgRepeatPlain)  
+  .Case("optional", eArgRepeatOptional)
+  .Case("plus", eArgRepeatPlus)
+  .Case("star", eArgRepeatStar) 
+  .Case("range", eArgRepeatRange)
+  .Case("pair-plain", eArgRepeatPairPlain)
+  .Case("pair-optional", eArgRepeatPairOptional)
+  .Case("pair-plus", eArgRepeatPairPlus)
+  .Case("pair-star", eArgRepeatPairStar)
+  .Case("pair-range", eArgRepeatPairRange)
+  .Case("pair-range-optional", eArgRepeatPairRangeOptional)
+  .Default({});
 }
 
 static CommandObject::CommandArgumentEntry
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
index 114a1b9a45cf2..a9fb02113c47d 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -3067,8 +3067,9 @@ ScriptInterpreterPythonImpl::OptionParsingStartedForCommandObject(
   if (PyErr_Occurred())
     PyErr_Clear();
 
-  // FIXME: this should really be a void function
-  bool py_return = unwrapOrSetPythonException(
+  // option_parsing_starting doesn't return anything, ignore anything but 
+  // python errors.
+  unwrapOrSetPythonException(
       As<bool>(implementor.CallMethod(callee_name)));
 
   // if it fails, print the error but otherwise go on

>From 48eaf17159ab3440fcde7e3dcc9f983118df306a Mon Sep 17 00:00:00 2001
From: Jim Ingham <jingham at apple.com>
Date: Fri, 26 Jan 2024 17:53:48 -0800
Subject: [PATCH 3/4] Address more review comments

---
 lldb/bindings/python/python-wrapper.swig      | 10 +++--
 lldb/examples/python/parsed_cmd.py            | 20 +++++-----
 .../source/Commands/CommandObjectCommands.cpp | 39 +++++++++----------
 .../Python/ScriptInterpreterPython.cpp        |  4 +-
 .../command/script/add/test_commands.py       |  3 +-
 5 files changed, 36 insertions(+), 40 deletions(-)

diff --git a/lldb/bindings/python/python-wrapper.swig b/lldb/bindings/python/python-wrapper.swig
index bf31bef473128..1370afc885d43 100644
--- a/lldb/bindings/python/python-wrapper.swig
+++ b/lldb/bindings/python/python-wrapper.swig
@@ -831,6 +831,8 @@ bool lldb_private::python::SWIGBridge::LLDBSwigPythonCallCommandObject(
   return true;
 }
 
+#include "lldb/Interpreter/CommandReturnObject.h"
+
 bool lldb_private::python::SWIGBridge::LLDBSwigPythonCallParsedCommandObject(
     PyObject *implementor, lldb::DebuggerSP debugger, lldb_private::StructuredDataImpl &args_impl,
     lldb_private::CommandReturnObject &cmd_retobj,
@@ -841,13 +843,13 @@ bool lldb_private::python::SWIGBridge::LLDBSwigPythonCallParsedCommandObject(
   PythonObject self(PyRefType::Borrowed, implementor);
   auto pfunc = self.ResolveName<PythonCallable>("__call__");
 
-  if (!pfunc.IsAllocated())
+  if (!pfunc.IsAllocated()) {
+    cmd_retobj.AppendError("Could not find '__call__' method in implementation class"); 
     return false;
+  }
 
-  auto cmd_retobj_arg = SWIGBridge::ToSWIGWrapper(cmd_retobj);
-  
   pfunc(SWIGBridge::ToSWIGWrapper(std::move(debugger)), SWIGBridge::ToSWIGWrapper(args_impl),
-        SWIGBridge::ToSWIGWrapper(exe_ctx_ref_sp), cmd_retobj_arg.obj());
+        SWIGBridge::ToSWIGWrapper(exe_ctx_ref_sp), SWIGBridge::ToSWIGWrapper(cmd_retobj).obj());
 
   return true;
 }
diff --git a/lldb/examples/python/parsed_cmd.py b/lldb/examples/python/parsed_cmd.py
index c8f7e4e09f866..1cd91734fe330 100644
--- a/lldb/examples/python/parsed_cmd.py
+++ b/lldb/examples/python/parsed_cmd.py
@@ -49,7 +49,9 @@ def __call__(self, debugger, args_list, exe_ctx, result):
 
 class LLDBOVParser:
     def __init__(self):
-        self.options_array = []
+        # This is a dictionary of dictionaries.  The key is the long option
+        # name, and the value is the rest of the definition.
+        self.options_dict = {}
         self.args_array = []
 
     # Some methods to translate common value types.  Should return a
@@ -160,17 +162,14 @@ def determine_completion(cls, arg_type):
     def get_option_element(self, long_name):
         # Fixme: Is it worth making a long_option dict holding the rest of
         # the options dict so this lookup is faster?
-        for item in self.options_array:
-            if item["long_option"] == long_name:
-                return item
-
-        return None
+        return self.options_dict.get(long_name, None)
             
     def option_parsing_started(self):
         # This makes the ivars for all the varnames in the array and gives them
         # their default values.
-        for elem in self.options_array:
-            elem['_value_set'] = False       
+        for key, elem in self.options_dict.items():
+            #breakpoint()
+            elem['_value_set'] = False
             try:
                 object.__setattr__(self, elem["varname"], elem["default"])
             except AttributeError:
@@ -250,7 +249,6 @@ def add_option(self, short_option, long_option, usage, default,
             completion_type = self.determine_completion(value_type)
             
         dict = {"short_option" : short_option,
-                "long_option" : long_option,
                 "required" : required,
                 "usage" : usage,
                 "value_type" : value_type,
@@ -263,7 +261,7 @@ def add_option(self, short_option, long_option, usage, default,
         if groups:
             dict["groups"] = groups
 
-        self.options_array.append(dict)
+        self.options_dict[long_option] = dict
 
     def make_argument_element(self, arg_type, repeat = "optional", groups = None):
         element = {"arg_type" : arg_type, "repeat" : repeat}
@@ -284,7 +282,7 @@ def get_parser(self):
         return self.ov_parser
 
     def get_options_definition(self):
-        return self.get_parser().options_array
+        return self.get_parser().options_dict
 
     def get_flags(self):
         return 0
diff --git a/lldb/source/Commands/CommandObjectCommands.cpp b/lldb/source/Commands/CommandObjectCommands.cpp
index e23751a247c18..57d016477aa88 100644
--- a/lldb/source/Commands/CommandObjectCommands.cpp
+++ b/lldb/source/Commands/CommandObjectCommands.cpp
@@ -1411,7 +1411,7 @@ class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
     }
     
     
-    Status SetOptionsFromArray(StructuredData::Array &options) {
+    Status SetOptionsFromArray(StructuredData::Dictionary &options) {
       Status error;
       m_num_options = options.GetSize();
       m_options_definition_up.reset(new OptionDefinition[m_num_options]);
@@ -1425,10 +1425,10 @@ class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
       size_t short_opt_counter = 0;
       // This is the Array::ForEach function for adding option elements:
       auto add_element = [this, &error, &counter, &short_opt_counter] 
-          (StructuredData::Object *object) -> bool {
+          (llvm::StringRef long_option, StructuredData::Object *object) -> bool {
         StructuredData::Dictionary *opt_dict = object->GetAsDictionary();
         if (!opt_dict) {
-          error.SetErrorString("Object in options array is not a dictionary");
+          error.SetErrorString("Value in options dictionary is not a dictionary");
           return false;
         }
         OptionDefinition &option_def = m_options_definition_up.get()[counter];
@@ -1489,21 +1489,13 @@ class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
         }
         option_def.short_option = short_option;
         
-        // Long Option:
-        std::string long_option;
-        obj_sp = opt_dict->GetValueForKey("long_option");
-        if (!obj_sp) {
-          error.SetErrorStringWithFormatv("required long_option missing from "
-          "option {0}", counter);
-          return false;
-        }
-        llvm::StringRef long_stref = obj_sp->GetStringValue();
-        if (long_stref.empty()) {
+        // Long Option is the key from the outer dict:
+        if (long_option.empty()) {
           error.SetErrorStringWithFormatv("empty long_option for option {0}", 
               counter);
           return false;
         }
-        auto inserted = g_string_storer.insert(long_stref.str());
+        auto inserted = g_string_storer.insert(long_option.str());
         option_def.long_option = ((*(inserted.first)).data());
         
         // Value Type:
@@ -1556,13 +1548,14 @@ class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
               "{0}", counter);
           return false;
         }
-        long_stref = obj_sp->GetStringValue();
-        if (long_stref.empty()) {
+        llvm::StringRef usage_stref;
+        usage_stref = obj_sp->GetStringValue();
+        if (usage_stref.empty()) {
           error.SetErrorStringWithFormatv("empty usage text for option {0}", 
               counter);
           return false;
         }
-        m_usage_container[counter] = long_stref.str().c_str();
+        m_usage_container[counter] = usage_stref.str().c_str();
         option_def.usage_text = m_usage_container[counter].data();
 
         // Enum Values:
@@ -1743,12 +1736,16 @@ class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
     // Now set up the options definitions from the options:
     StructuredData::ObjectSP options_object_sp 
         = scripter->GetOptionsForCommandObject(cmd_obj_sp);
-    // It's okay not to have an options array.
+    // It's okay not to have an options dict.
     if (options_object_sp) {
-      StructuredData::Array *options_array = options_object_sp->GetAsArray();
+      // The options come as a dictionary of dictionaries.  The key of the
+      // outer dict is the long option name (since that's required).  The
+      // value holds all the other option specification bits.
+      StructuredData::Dictionary *options_dict 
+          = options_object_sp->GetAsDictionary();
       // but if it exists, it has to be an array.
-      if (options_array) {
-        m_options_error = m_options.SetOptionsFromArray(*(options_array));
+      if (options_dict) {
+        m_options_error = m_options.SetOptionsFromArray(*(options_dict));
         // If we got an error don't bother with the arguments...
         if (m_options_error.Fail())
           return;
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
index a9fb02113c47d..dadcde612614b 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -2972,8 +2972,8 @@ ScriptInterpreterPythonImpl::GetOptionsForCommandObject(
   if (PyErr_Occurred())
     PyErr_Clear();
 
-  PythonList py_return = unwrapOrSetPythonException(
-      As<PythonList>(implementor.CallMethod(callee_name)));
+  PythonDictionary py_return = unwrapOrSetPythonException(
+      As<PythonDictionary>(implementor.CallMethod(callee_name)));
 
   // if it fails, print the error but otherwise go on
   if (PyErr_Occurred()) {
diff --git a/lldb/test/API/commands/command/script/add/test_commands.py b/lldb/test/API/commands/command/script/add/test_commands.py
index b704b833ae7b8..96551300173c4 100644
--- a/lldb/test/API/commands/command/script/add/test_commands.py
+++ b/lldb/test/API/commands/command/script/add/test_commands.py
@@ -14,8 +14,7 @@ def __call__(self, debugger, args_array, exe_ctx, result):
         opt_def = self.get_options_definition()
         if len(opt_def):
             result.AppendMessage("Options:\n")
-            for elem in opt_def:
-                long_option = elem["long_option"]
+            for long_option, elem in opt_def.items():
                 varname = elem["varname"]
                 result.AppendMessage(f"{long_option} (set: {elem['_value_set']}): {object.__getattribute__(self.ov_parser, varname)}\n")
         else:

>From 6c24cc69d1736f6de73b2e06fbe87076785c2f41 Mon Sep 17 00:00:00 2001
From: Jim Ingham <jingham at apple.com>
Date: Mon, 29 Jan 2024 17:03:35 -0800
Subject: [PATCH 4/4] Finishing up the current review comments in
 CommandObjectCommands.cpp.

---
 .../source/Commands/CommandObjectCommands.cpp | 20 +++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/lldb/source/Commands/CommandObjectCommands.cpp b/lldb/source/Commands/CommandObjectCommands.cpp
index 57d016477aa88..09377a3ec20da 100644
--- a/lldb/source/Commands/CommandObjectCommands.cpp
+++ b/lldb/source/Commands/CommandObjectCommands.cpp
@@ -1570,8 +1570,8 @@ class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
           }
           size_t num_elem = array->GetSize();
           size_t enum_ctr = 0;
-          m_enum_storage[counter] = std::vector<EnumValueStorer>(num_elem);
-          std::vector<EnumValueStorer> &curr_elem = m_enum_storage[counter];
+          m_enum_storage[counter] = std::vector<EnumValueStorage>(num_elem);
+          std::vector<EnumValueStorage> &curr_elem = m_enum_storage[counter];
           
           // This is the Array::ForEach function for adding enum elements:
           // Since there are only two fields to specify the enum, use a simple
@@ -1606,7 +1606,7 @@ class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
             }
             llvm::StringRef usage_stref = obj_sp->GetStringValue();
             std::string usage_cstr_str = usage_stref.str().c_str();
-            curr_elem[enum_ctr] = EnumValueStorer(value_cstr_str, 
+            curr_elem[enum_ctr] = EnumValueStorage(value_cstr_str, 
                 usage_cstr_str, enum_ctr);
             
             enum_ctr++;
@@ -1632,24 +1632,24 @@ class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
     }
     
   private:
-    struct EnumValueStorer {
-      EnumValueStorer() {
+    struct EnumValueStorage {
+      EnumValueStorage() {
         element.string_value = "value not set";
         element.usage = "usage not set";
         element.value = 0;
       }
       
-      EnumValueStorer(std::string &in_str_val, std::string &in_usage, 
-          size_t in_value) : value(in_str_val), usage(in_usage) {
+      EnumValueStorage(std::string in_str_val, std::string in_usage, 
+          size_t in_value) : value(std::move(in_str_val)), usage(std::move(in_usage)) {
         SetElement(in_value);
       }
       
-      EnumValueStorer(const EnumValueStorer &in) : value(in.value), 
+      EnumValueStorage(const EnumValueStorage &in) : value(in.value), 
           usage(in.usage) {
         SetElement(in.element.value);
       }
       
-      EnumValueStorer &operator=(const EnumValueStorer &in) {
+      EnumValueStorage &operator=(const EnumValueStorage &in) {
         value = in.value;
         usage = in.usage;
         SetElement(in.element.value);
@@ -1678,7 +1678,7 @@ class CommandObjectScriptingObjectParsed : public CommandObjectParsed {
     // all that common so it's not worth the effort to dedup them.  
     size_t m_num_options = 0;
     std::unique_ptr<OptionDefinition> m_options_definition_up;
-    std::vector<std::vector<EnumValueStorer>> m_enum_storage;
+    std::vector<std::vector<EnumValueStorage>> m_enum_storage;
     std::vector<std::vector<OptionEnumValueElement>> m_enum_vector;
     std::vector<std::string> m_usage_container;
     CommandInterpreter &m_interpreter;



More information about the lldb-commits mailing list