Segregate the Python class + key/value dictionary into a separate OptionGroup

[jingham]

Summary

I want to reuse these options (break set's -P -k -v) in another command (thread step-scripted) in a future commit, so I broke them off into their own OptionGroup. I made the short options settable because you can never tell whether there's going to be the same three open slots in every command. For instance, "thread step-scripted" uses -C not -P, which will be more correct if/when we have another script interpreter. But -C was already taken for "break set"...

I added some tests for the parsing of these options. In the course of adding those tests I discovered that if an OptionGroup returns an error, and the option is not the last option in the line, the error is ignored. I fixed that as well - that's the change to Options.cpp

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/TestScriptedResolver.py

class TestScriptedResolver(TestBase):
 
     @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24528")
     def test_command_line(self):
         """ Make sure we are called at the right depths depending on what we return
             from __get_depth__"""
         self.build()
         self.do_test_cli()
 
+    def test_bad_command_lines(self):
+        """Make sure we get appropriate errors when we give invalid key/value
+           options"""
+        self.build()
+        self.do_test_bad_options()        
+
     def setUp(self):
         # Call super's setUp().
         TestBase.setUp(self)
 
     def make_target_and_import(self):
         target = lldbutil.run_to_breakpoint_make_target(self)
         interp = self.dbg.GetCommandInterpreter()
         error = lldb.SBError()
def do_test_depths(self):
         self.assertTrue(bkpt.GetNumLocations() > 0, "ResolverFuncDepth got no locations.")
         self.expect("script print(resolver.Resolver.got_files)", substrs=["3"], msg="Was only passed modules")
         self.expect("script print(resolver.Resolver.func_list)", substrs=["break_on_me", "main", "test_func"], msg="Saw all the functions")
 
     def do_test_cli(self):
         target = self.make_target_and_import()
 
         lldbutil.run_break_set_by_script(self, "resolver.Resolver", extra_options="-k symbol -v break_on_me")
+
+    def do_test_bad_options(self):
+        target = self.make_target_and_import()
+
+        self.expect("break set -P resolver.Resolver -k a_key", error = True, msg="Missing value at end", 
+           substrs=['Key: "a_key" missing value'])
+        self.expect("break set -P resolver.Resolver -v a_value", error = True, msg="Missing key at end", 
+           substrs=['Value: "a_value" missing matching key'])
+        self.expect("break set -P resolver.Resolver -v a_value -k a_key -v another_value", error = True, msg="Missing key among args", 
+           substrs=['Value: "a_value" missing matching key'])
+        self.expect("break set -P resolver.Resolver -k a_key -k a_key -v another_value", error = True, msg="Missing value among args", 
+           substrs=['Key: "a_key" missing value'])

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py

 import lldb
 
 class Resolver:
   got_files = 0
   func_list = []
 
   def __init__(self, bkpt, extra_args, dict):
       self.bkpt = bkpt
       self.extra_args = extra_args
+        
       Resolver.func_list = []
       Resolver.got_files = 0
 
   def __callback__(self, sym_ctx):
       sym_name = "not_a_real_function_name"
       sym_item = self.extra_args.GetValueForKey("symbol")
       if sym_item.IsValid():
           sym_name = sym_item.GetStringValue(1000)
+      else:
+          print("Didn't have a value for key 'symbol'")
 
       if sym_ctx.compile_unit.IsValid():
           Resolver.got_files = 1
       else:
           Resolver.got_files = 2
       
       if sym_ctx.function.IsValid():
         Resolver.got_files = 3

lldb/source/Commands/CommandObjectBreakpoint.cpp

 #include "lldb/Breakpoint/Breakpoint.h"
 #include "lldb/Breakpoint/BreakpointIDList.h"
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Host/OptionParser.h"
 #include "lldb/Interpreter/CommandCompletions.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Interpreter/OptionArgParser.h"
+#include "lldb/Interpreter/OptionGroupPythonClassWithDict.h"
 #include "lldb/Interpreter/OptionValueBoolean.h"
 #include "lldb/Interpreter/OptionValueString.h"
 #include "lldb/Interpreter/OptionValueUInt64.h"
 #include "lldb/Interpreter/Options.h"
 #include "lldb/Target/Language.h"
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/Thread.h"
enum BreakpointSetType {
     eSetTypeScripted,
   };
 
   CommandObjectBreakpointSet(CommandInterpreter &interpreter)
       : CommandObjectParsed(
             interpreter, "breakpoint set",
             "Sets a breakpoint or set of breakpoints in the executable.",
             "breakpoint set <cmd-options>"),
+        m_python_class_options("scripted breakpoint", 'P'),
         m_bp_opts(), m_options() {
           // We're picking up all the normal options, commands and disable.
+          m_all_options.Append(&m_python_class_options, LLDB_OPT_SET_1,
+                               LLDB_OPT_SET_11);
           m_all_options.Append(&m_bp_opts, 
                                LLDB_OPT_SET_1 | LLDB_OPT_SET_3 | LLDB_OPT_SET_4, 
                                LLDB_OPT_SET_ALL);
-          m_all_options.Append(&m_dummy_options, LLDB_OPT_SET_1, LLDB_OPT_SET_ALL);
+          m_all_options.Append(&m_dummy_options, LLDB_OPT_SET_1, 
+                               LLDB_OPT_SET_ALL);
           m_all_options.Append(&m_options);
           m_all_options.Finalize();
         }
 
   ~CommandObjectBreakpointSet() override = default;
 
   Options *GetOptions() override { return &m_all_options; }
 
Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
               "Invalid boolean value for on-catch option: '%s'",
               option_arg.str().c_str());
       } break;
 
       case 'H':
         m_hardware = true;
         break;
         
-      case 'k': {
-          if (m_current_key.empty())
-            m_current_key.assign(option_arg);
-          else
-            error.SetErrorStringWithFormat("Key: %s missing value.",
-                                            m_current_key.c_str());
-        
-      } break;
       case 'K': {
         bool success;
         bool value;
         value = OptionArgParser::ToBoolean(option_arg, true, &success);
         if (value)
           m_skip_prologue = eLazyBoolYes;
         else
           m_skip_prologue = eLazyBoolNo;
Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
         m_exception_extra_args.AppendArgument("-O");
         m_exception_extra_args.AppendArgument(option_arg);
         break;
 
       case 'p':
         m_source_text_regexp.assign(option_arg);
         break;
         
-      case 'P':
-        m_python_class.assign(option_arg);
-        break;
-
       case 'r':
         m_func_regexp.assign(option_arg);
         break;
 
       case 's':
         m_modules.AppendIfUnique(FileSpec(option_arg));
         break;
 
       case 'S':
         m_func_names.push_back(option_arg);
         m_func_name_type_mask |= eFunctionNameTypeSelector;
         break;
 
-      case 'v': {
-          if (!m_current_key.empty()) {
-              m_extra_args_sp->AddStringItem(m_current_key, option_arg);
-              m_current_key.clear();
-          }
-          else
-            error.SetErrorStringWithFormat("Value \"%s\" missing matching key.",
-                                           option_arg.str().c_str());
-      } break;
-        
       case 'w': {
         bool success;
         m_throw_bp = OptionArgParser::ToBoolean(option_arg, true, &success);
         if (!success)
           error.SetErrorStringWithFormat(
               "Invalid boolean value for on-throw option: '%s'",
               option_arg.str().c_str());
       } break;
void OptionParsingStarting(ExecutionContext *execution_context) override {
       m_exception_language = eLanguageTypeUnknown;
       m_language = lldb::eLanguageTypeUnknown;
       m_skip_prologue = eLazyBoolCalculate;
       m_breakpoint_names.clear();
       m_all_files = false;
       m_exception_extra_args.Clear();
       m_move_to_nearest_code = eLazyBoolCalculate;
       m_source_regex_func_names.clear();
-      m_python_class.clear();
-      m_extra_args_sp = std::make_shared<StructuredData::Dictionary>();
       m_current_key.clear();
     }
 
     llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
       return llvm::makeArrayRef(g_breakpoint_set_options);
     }
 
     // Instance variables to hold the values for command options.
public:
     bool m_hardware; // Request to use hardware breakpoints
     lldb::LanguageType m_exception_language;
     lldb::LanguageType m_language;
     LazyBool m_skip_prologue;
     bool m_all_files;
     Args m_exception_extra_args;
     LazyBool m_move_to_nearest_code;
     std::unordered_set<std::string> m_source_regex_func_names;
-    std::string m_python_class;
-    StructuredData::DictionarySP m_extra_args_sp;
     std::string m_current_key;
   };
 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     Target &target = GetSelectedOrDummyTarget(m_dummy_options.m_use_dummy);
 
     // The following are the various types of breakpoints that could be set:
     //   1).  -f -l -p  [-s -g]   (setting breakpoint by source location)
     //   2).  -a  [-s -g]         (setting breakpoint by address)
     //   3).  -n  [-s -g]         (setting breakpoint by function name)
     //   4).  -r  [-s -g]         (setting breakpoint by function name regular
     //   expression)
     //   5).  -p -f               (setting a breakpoint by comparing a reg-exp
     //   to source text)
     //   6).  -E [-w -h]          (setting a breakpoint for exceptions for a
     //   given language.)
 
     BreakpointSetType break_type = eSetTypeInvalid;
 
-    if (!m_options.m_python_class.empty())
+    if (!m_python_class_options.GetClassName().empty())
       break_type = eSetTypeScripted;
     else if (m_options.m_line_num != 0)
       break_type = eSetTypeFileAndLine;
     else if (m_options.m_load_addr != LLDB_INVALID_ADDRESS)
       break_type = eSetTypeAddress;
     else if (!m_options.m_func_names.empty())
       break_type = eSetTypeFunctionName;
     else if (!m_options.m_func_regexp.empty())
case eSetTypeException: {
         result.SetStatus(eReturnStatusFailed);
         return false;
       }
     } break;
     case eSetTypeScripted: {
 
       Status error;
       bp_sp = target.CreateScriptedBreakpoint(
-          m_options.m_python_class, &(m_options.m_modules),
+          m_python_class_options.GetClassName().c_str(), &(m_options.m_modules),
           &(m_options.m_filenames), false, m_options.m_hardware,
-          m_options.m_extra_args_sp, &error);
+          m_python_class_options.GetStructuredData(), &error);
       if (error.Fail()) {
         result.AppendErrorWithFormat(
             "Error setting extra exception arguments: %s",
             error.AsCString());
         target.RemoveBreakpointByID(bp_sp->GetID());
         result.SetStatus(eReturnStatusFailed);
         return false;
       }
if (!target.GetSourceManager().GetDefaultFileAndLine(file, default_line)) {
         }
       }
     }
     return true;
   }
 
   BreakpointOptionGroup m_bp_opts;
   BreakpointDummyOptionGroup m_dummy_options;
+  OptionGroupPythonClassWithDict m_python_class_options;
   CommandOptions m_options;
   OptionGroupOptions m_all_options;
 };
 
 // CommandObjectBreakpointModify
 #pragma mark Modify
 
 class CommandObjectBreakpointModify : public CommandObjectParsed {

lldb/source/Commands/Options.td

def breakpoint_set_source_pattern_regexp :
     Option<"source-pattern-regexp", "p">, Group<9>, Arg<"RegularExpression">,
     Required, Desc<"Set the breakpoint by specifying a regular expression which"
     " is matched against the source text in a source file or files specified "
     "with the -f can be specified more than once.  If no source files "
     "are specified, uses the current \"default source file\".  If you want to "
     "match against all source files, pass the \"--all-files\" option.">;
   def breakpoint_set_all_files : Option<"all-files", "A">, Group<9>,
     Desc<"All files are searched for source pattern matches.">;
-  def breakpoint_set_python_class : Option<"python-class", "P">, Group<11>,
-    Arg<"PythonClass">, Required, Desc<"The name of the class that implement "
-    "a scripted breakpoint.">;
-  def breakpoint_set_python_class_key : Option<"python-class-key", "k">,
-    Group<11>, Arg<"None">,
-    Desc<"The key for a key/value pair passed to the class that implements a "
-    "scripted breakpoint.  Can be specified more than once.">;
-  def breakpoint_set_python_class_value : Option<"python-class-value", "v">,
-    Group<11>, Arg<"None">, Desc<"The value for the previous key in the pair "
-    "passed to the class that implements a scripted breakpoint. "
-    "Can be specified more than once.">;
   def breakpoint_set_language_exception : Option<"language-exception", "E">,
     Group<10>, Arg<"Language">, Required,
     Desc<"Set the breakpoint on exceptions thrown by the specified language "
     "(without options, on throw but not catch.)">;
   def breakpoint_set_on_throw : Option<"on-throw", "w">, Group<10>,
     Arg<"Boolean">, Desc<"Set the breakpoint on exception throW.">;
   def breakpoint_set_on_catch : Option<"on-catch", "h">, Group<10>,
     Arg<"Boolean">, Desc<"Set the breakpoint on exception catcH.">;

lldb/source/Interpreter/CMakeLists.txt

add_lldb_library(lldbInterpreter
   CommandObjectScript.cpp
   CommandOptionValidators.cpp
   CommandReturnObject.cpp
   OptionArgParser.cpp
   OptionGroupArchitecture.cpp
   OptionGroupBoolean.cpp
   OptionGroupFile.cpp
   OptionGroupFormat.cpp
+  OptionGroupPythonClassWithDict.cpp
   OptionGroupOutputFile.cpp
   OptionGroupPlatform.cpp
   OptionGroupString.cpp
   OptionGroupUInt64.cpp
   OptionGroupUUID.cpp
   OptionGroupValueObjectDisplay.cpp
   OptionValue.cpp
   OptionValueArch.cpp

lldb/source/Interpreter/Options.cpp

if (long_options_index >= 0 &&
       // As long as validation didn't fail, we set the option value.
       if (!validation_failed)
         error =
             SetOptionValue(long_options_index,
                            (def->option_has_arg == OptionParser::eNoArgument)
                                ? nullptr
                                : OptionParser::GetOptionArgument(),
                            execution_context);
+      // If the Option setting returned an error, we should stop parsing
+      // and return the error.
+      if (error.Fail())
+        break;      
     } else {
       error.SetErrorStringWithFormat("invalid option with value '%i'", val);
     }
   }
 
   if (error.Fail())
     return error.ToError();
 
   argv.pop_back();
   argv.erase(argv.begin(), argv.begin() + OptionParser::GetOptionIndex());
   return ReconstituteArgsAfterParsing(argv, args);
 }