Break out the Python class & key/value options into a separate OptionGroup.
authorJim Ingham <jingham@apple.com>
Thu, 3 Oct 2019 22:18:51 +0000 (22:18 +0000)
committerJim Ingham <jingham@apple.com>
Thu, 3 Oct 2019 22:18:51 +0000 (22:18 +0000)
Use this in the scripted breakpoint command.  Added some tests for parsing
the key/value options.  This uncovered a bug in handling parsing errors mid-line.
I also fixed that bug.

Differential Revision: https://reviews.llvm.org/D68363

llvm-svn: 373673

lldb/include/lldb/Interpreter/OptionGroupPythonClassWithDict.h [new file with mode: 0644]
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/TestScriptedResolver.py
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py
lldb/source/Commands/CommandObjectBreakpoint.cpp
lldb/source/Commands/Options.td
lldb/source/Interpreter/CMakeLists.txt
lldb/source/Interpreter/OptionGroupPythonClassWithDict.cpp [new file with mode: 0644]
lldb/source/Interpreter/Options.cpp

diff --git a/lldb/include/lldb/Interpreter/OptionGroupPythonClassWithDict.h b/lldb/include/lldb/Interpreter/OptionGroupPythonClassWithDict.h
new file mode 100644 (file)
index 0000000..b92bb6f
--- /dev/null
@@ -0,0 +1,64 @@
+//===-- OptionGroupPythonClassWithDict.h -------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef liblldb_OptionGroupString_h_
+#define liblldb_OptionGroupString_h_
+
+#include "lldb/Interpreter/Options.h"
+#include "lldb/Utility/StructuredData.h"
+
+namespace lldb_private {
+
+// Use this Option group if you have a python class that implements some
+// Python extension point, and you pass a SBStructuredData to the class 
+// __init__ method.  
+// class_option specifies the class name
+// the key and value options are read in in pairs, and a 
+// StructuredData::Dictionary is constructed with those pairs.
+class OptionGroupPythonClassWithDict : public OptionGroup {
+public:
+  OptionGroupPythonClassWithDict(const char *class_use,
+                      int class_option = 'C',
+                      int key_option = 'k', 
+                      int value_option = 'v',
+                      char *class_long_option = "python-class",
+                      const char *key_long_option = "python-class-key",
+                      const char *value_long_option = "python-class-value",
+                      bool required = false);
+                      
+  ~OptionGroupPythonClassWithDict() override;
+
+  llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
+    return llvm::ArrayRef<OptionDefinition>(m_option_definition);
+  }
+
+  Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_value,
+                        ExecutionContext *execution_context) override;
+  Status SetOptionValue(uint32_t, const char *, ExecutionContext *) = delete;
+
+  void OptionParsingStarting(ExecutionContext *execution_context) override;
+  Status OptionParsingFinished(ExecutionContext *execution_context) override;
+  
+  const StructuredData::DictionarySP GetStructuredData() {
+    return m_dict_sp;
+  }
+  const std::string &GetClassName() {
+    return m_class_name;
+  }
+
+protected:
+  std::string m_class_name;
+  std::string m_current_key;
+  StructuredData::DictionarySP m_dict_sp;
+  std::string m_class_usage_text, m_key_usage_text, m_value_usage_text;
+  OptionDefinition m_option_definition[3];
+};
+
+} // namespace lldb_private
+
+#endif // liblldb_OptionGroupString_h_
index 59abad6..4842bc0 100644 (file)
@@ -38,6 +38,12 @@ class TestScriptedResolver(TestBase):
         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)
@@ -195,3 +201,15 @@ class TestScriptedResolver(TestBase):
         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'])
index 61f5f2d..f3af7c0 100644 (file)
@@ -7,6 +7,7 @@ class Resolver:
   def __init__(self, bkpt, extra_args, dict):
       self.bkpt = bkpt
       self.extra_args = extra_args
+        
       Resolver.func_list = []
       Resolver.got_files = 0
 
@@ -15,6 +16,8 @@ class Resolver:
       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
index e73c924..4700b62 100644 (file)
@@ -16,6 +16,7 @@
 #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"
@@ -241,12 +242,16 @@ public:
             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();
         }
@@ -352,14 +357,6 @@ public:
         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;
@@ -441,10 +438,6 @@ public:
         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;
@@ -458,16 +451,6 @@ public:
         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);
@@ -510,8 +493,6 @@ public:
       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();
     }
 
@@ -543,8 +524,6 @@ public:
     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;
   };
 
@@ -565,7 +544,7 @@ protected:
 
     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;
@@ -721,9 +700,9 @@ protected:
 
       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",
@@ -818,6 +797,7 @@ private:
 
   BreakpointOptionGroup m_bp_opts;
   BreakpointDummyOptionGroup m_dummy_options;
+  OptionGroupPythonClassWithDict m_python_class_options;
   CommandOptions m_options;
   OptionGroupOptions m_all_options;
 };
index 7791302..477b553 100644 (file)
@@ -163,17 +163,6 @@ let Command = "breakpoint set" in {
     "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 "
index 6486256..3900799 100644 (file)
@@ -20,6 +20,7 @@ add_lldb_library(lldbInterpreter
   OptionGroupBoolean.cpp
   OptionGroupFile.cpp
   OptionGroupFormat.cpp
+  OptionGroupPythonClassWithDict.cpp
   OptionGroupOutputFile.cpp
   OptionGroupPlatform.cpp
   OptionGroupString.cpp
diff --git a/lldb/source/Interpreter/OptionGroupPythonClassWithDict.cpp b/lldb/source/Interpreter/OptionGroupPythonClassWithDict.cpp
new file mode 100644 (file)
index 0000000..6b16786
--- /dev/null
@@ -0,0 +1,124 @@
+//===-- OptionGroupKeyValue.cpp ----------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Interpreter/OptionGroupPythonClassWithDict.h"
+
+#include "lldb/Host/OptionParser.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+OptionGroupPythonClassWithDict::OptionGroupPythonClassWithDict
+    (const char *class_use,
+     int class_option,
+     int key_option, 
+     int value_option,
+     char *class_long_option,
+     const char *key_long_option,
+     const char *value_long_option,
+     bool required) {
+  m_key_usage_text.assign("The key for a key/value pair passed to the class"
+                            " that implements a ");
+  m_key_usage_text.append(class_use);
+  m_key_usage_text.append(".  Pairs can be specified more than once.");
+  
+  m_value_usage_text.assign("The value for a previous key in the pair passed to"
+                            " the class that implements a ");
+  m_value_usage_text.append(class_use);
+  m_value_usage_text.append(".  Pairs can be specified more than once.");
+  
+  m_class_usage_text.assign("The name of the class that will manage a ");
+  m_class_usage_text.append(class_use);
+  m_class_usage_text.append(".");
+  
+  m_option_definition[0].usage_mask = LLDB_OPT_SET_1;
+  m_option_definition[0].required = required;
+  m_option_definition[0].long_option = class_long_option;
+  m_option_definition[0].short_option = class_option;
+  m_option_definition[0].validator = nullptr;
+  m_option_definition[0].option_has_arg = OptionParser::eRequiredArgument;
+  m_option_definition[0].enum_values = {};
+  m_option_definition[0].completion_type = 0;
+  m_option_definition[0].argument_type = eArgTypePythonClass;
+  m_option_definition[0].usage_text = m_class_usage_text.data();
+
+  m_option_definition[1].usage_mask = LLDB_OPT_SET_1;
+  m_option_definition[1].required = required;
+  m_option_definition[1].long_option = key_long_option;
+  m_option_definition[1].short_option = key_option;
+  m_option_definition[1].validator = nullptr;
+  m_option_definition[1].option_has_arg = OptionParser::eRequiredArgument;
+  m_option_definition[1].enum_values = {};
+  m_option_definition[1].completion_type = 0;
+  m_option_definition[1].argument_type = eArgTypeNone;
+  m_option_definition[1].usage_text = m_key_usage_text.data();
+
+  m_option_definition[2].usage_mask = LLDB_OPT_SET_1;
+  m_option_definition[2].required = required;
+  m_option_definition[2].long_option = value_long_option;
+  m_option_definition[2].short_option = value_option;
+  m_option_definition[2].validator = nullptr;
+  m_option_definition[2].option_has_arg = OptionParser::eRequiredArgument;
+  m_option_definition[2].enum_values = {};
+  m_option_definition[2].completion_type = 0;
+  m_option_definition[2].argument_type = eArgTypeNone;
+  m_option_definition[2].usage_text = m_value_usage_text.data();
+}
+
+OptionGroupPythonClassWithDict::~OptionGroupPythonClassWithDict() {}
+
+Status OptionGroupPythonClassWithDict::SetOptionValue(
+    uint32_t option_idx,
+    llvm::StringRef option_arg,
+    ExecutionContext *execution_context) {
+  Status error;
+  const int short_option = m_option_definition[option_idx].short_option;
+  switch (option_idx) {
+  case 0: {
+    m_class_name.assign(option_arg);
+  } break;
+  case 1: {
+      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 2: {
+      if (!m_current_key.empty()) {
+          m_dict_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;
+  default:
+    llvm_unreachable("Unimplemented option");
+  }
+  return error;
+}
+
+void OptionGroupPythonClassWithDict::OptionParsingStarting(
+  ExecutionContext *execution_context) {
+  m_current_key.erase();
+  m_dict_sp = std::make_shared<StructuredData::Dictionary>();
+}
+
+Status OptionGroupPythonClassWithDict::OptionParsingFinished(
+  ExecutionContext *execution_context) {
+  Status error;
+  // If we get here and there's contents in the m_current_key, somebody must
+  // have provided a key but no value.
+  if (!m_current_key.empty())
+      error.SetErrorStringWithFormat("Key: \"%s\" missing value.",
+                                     m_current_key.c_str());
+  return error;
+}
+
index 465271d..0bceea1 100644 (file)
@@ -1380,6 +1380,10 @@ llvm::Expected<Args> Options::Parse(const Args &args,
                                ? 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);
     }