First pass at addressing review comments

Author: jimingham

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}

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());

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()
 

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,32 +1304,30 @@ 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)
         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) {
+    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));
     }