Add the ability to define a Python based command that uses CommandObjectParsed #70734
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: None (jimingham) ChangesThis allows you to specify options and arguments and their definitions and then have lldb handle the completions, help, etc. in the same way that lldb does for its parsed commands internally. This feature has some design considerations as well as the code, so I've also set up an RFC, but I did this one first and will put the RFC address in here once I've pushed it... Note, the lldb "ParsedCommand interface" doesn't actually do all the work that it should. For instance, saying the type of an option that has a completer doesn't automatically hook up the completer, and ditto for argument values. We also do almost no work to verify that the arguments match their definition, or do auto-completion for them. This patch allows you to make a command that's bug-for-bug compatible with built-in ones, but I didn't want to stall it on getting the auto-command checking to work all the way correctly. As an overall design note, my primary goal here was to make an interface that worked well in the script language. For that I needed, for instance, to have a property-based way to get all the option values that were specified. It was much more convenient to do that by making a fairly bare-bones C interface to define the options and arguments of a command, and set their values, and then wrap that in a Python class (installed along with the other bits of the lldb python module) which you can then derive from to make your new command. This approach will also make it easier to experiment. See the file test_commands.py in the test case for examples of how this works. Patch is 76.74 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/70734.diff 15 Files Affected:
diff --git a/lldb/bindings/python/CMakeLists.txt b/lldb/bindings/python/CMakeLists.txt
index c941f764dfc92ac..657fdd2c9590066 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 17bc7b1f2198709..47887491d0c552a 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 000000000000000..7ee9e077d49ab52
--- /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 004f5d42f1e44ee..c2f17f374fe5828 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 0146eeb86262003..4dce5f40328d8dd 100644
--- a/lldb/include/lldb/Interpreter/ScriptInterpreter.h
+++ b/lldb/include/lldb/Interpreter/ScriptInterpreter.h
@@ -476,6 +476,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) {
@@ -520,6 +528,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 656ace223b5f157..1e7227767eb982a 100644
--- a/lldb/source/Commands/CommandObjectCommands.cpp
+++ b/lldb/source/Commands/CommandObjectCommands.cpp
@@ -1158,6 +1158,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,
@@ -1255,6 +1258,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)...
[truncated]
|
You can test this locally with the following command:git-clang-format --diff 997ffce43c6d2d3f647eb091c732665049b1f47f ea2b242fdd67885a5ea5b78cedcce4f1cd7462b4 -- lldb/include/lldb/Interpreter/CommandObject.h lldb/include/lldb/Interpreter/ScriptInterpreter.h lldb/source/Commands/CommandObjectCommands.cpp lldb/source/Interpreter/CommandObject.cpp lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cppView the diff from clang-format here.diff --git a/lldb/include/lldb/Interpreter/CommandObject.h b/lldb/include/lldb/Interpreter/CommandObject.h
index b99de56f53..36423b7e2c 100644
--- a/lldb/include/lldb/Interpreter/CommandObject.h
+++ b/lldb/include/lldb/Interpreter/CommandObject.h
@@ -226,8 +226,8 @@ public:
static bool IsPairType(ArgumentRepetitionType arg_repeat_type);
- static std::optional<ArgumentRepetitionType>
- ArgRepetitionFromString(llvm::StringRef string);
+ 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 932eaa8b8a..8968ebf139 100644
--- a/lldb/include/lldb/Interpreter/ScriptInterpreter.h
+++ b/lldb/include/lldb/Interpreter/ScriptInterpreter.h
@@ -473,11 +473,12 @@ public:
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) {
+ 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;
}
@@ -525,7 +526,7 @@ public:
dest.clear();
return false;
}
-
+
virtual StructuredData::ObjectSP
GetOptionsForCommandObject(StructuredData::GenericSP cmd_obj_sp) {
return {};
@@ -535,15 +536,15 @@ public:
GetArgumentsForCommandObject(StructuredData::GenericSP cmd_obj_sp) {
return {};
}
-
+
virtual bool SetOptionValueForCommandObject(
- StructuredData::GenericSP cmd_obj_sp, ExecutionContext *exe_ctx,
+ 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) {
+ virtual void
+ OptionParsingStartedForCommandObject(StructuredData::GenericSP cmd_obj_sp) {
return;
}
diff --git a/lldb/source/Commands/CommandObjectCommands.cpp b/lldb/source/Commands/CommandObjectCommands.cpp
index 30da2a0bb3..1683b218c7 100644
--- a/lldb/source/Commands/CommandObjectCommands.cpp
+++ b/lldb/source/Commands/CommandObjectCommands.cpp
@@ -1247,7 +1247,6 @@ private:
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
@@ -1259,22 +1258,22 @@ private:
/// 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:
+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(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();
+ ScriptInterpreter *scripter =
+ m_interpreter.GetDebugger().GetScriptInterpreter();
if (!scripter) {
error.SetErrorString("No script interpreter for SetOptionValue.");
return error;
@@ -1291,10 +1290,10 @@ private:
// 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);
+ 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);
@@ -1302,8 +1301,8 @@ private:
}
void OptionParsingStarting(ExecutionContext *execution_context) override {
- ScriptInterpreter *scripter =
- m_interpreter.GetDebugger().GetScriptInterpreter();
+ ScriptInterpreter *scripter =
+ m_interpreter.GetDebugger().GetScriptInterpreter();
if (!scripter || !m_cmd_obj_sp)
return;
@@ -1315,9 +1314,10 @@ private:
return {};
return llvm::ArrayRef(m_options_definition_up.get(), m_num_options);
}
-
- static Status ParseUsageMaskFromArray(StructuredData::ObjectSP obj_sp,
- size_t counter, uint32_t &usage_mask) {
+
+ 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
@@ -1329,10 +1329,10 @@ private:
usage_mask = LLDB_OPT_SET_ALL;
return error;
}
-
+
usage_mask = 0;
-
- StructuredData::UnsignedInteger *uint_val =
+
+ StructuredData::UnsignedInteger *uint_val =
obj_sp->GetAsUnsignedInteger();
if (uint_val) {
// If this is an integer, then this specifies a single group:
@@ -1354,9 +1354,8 @@ private:
}
// 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 {
+ 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();
@@ -1371,34 +1370,38 @@ private:
StructuredData::Array *arr_val = obj->GetAsArray();
if (!arr_val) {
error.SetErrorStringWithFormatv(
- "Group element not an int or array of integers for element {0}",
+ "Group element not an int or array of integers for element {0}",
counter);
- return false;
+ 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}",
+ "Subranges of a group not a start and a stop for element {0}",
counter);
- return false;
+ 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;
+ 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;
+ " 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);
+ "{1} for element {2}",
+ start, end, counter);
return false;
}
for (uint32_t i = start; i <= end; i++) {
@@ -1409,8 +1412,7 @@ private:
array_val->ForEach(groups_accumulator);
return error;
}
-
-
+
Status SetOptionsFromArray(StructuredData::Dictionary &options) {
Status error;
m_num_options = options.GetSize();
@@ -1420,35 +1422,37 @@ private:
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]
- (llvm::StringRef long_option, StructuredData::Object *object) -> bool {
+ auto add_element = [this, &error, &counter, &short_opt_counter](
+ llvm::StringRef long_option,
+ StructuredData::Object *object) -> bool {
StructuredData::Dictionary *opt_dict = object->GetAsDictionary();
if (!opt_dict) {
- error.SetErrorString("Value in options dictionary 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];
-
+
// 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) {
- error = ParseUsageMaskFromArray(obj_sp, counter,
- option_def.usage_mask);
+ error =
+ ParseUsageMaskFromArray(obj_sp, counter, option_def.usage_mask);
if (error.Fail())
return false;
}
@@ -1460,163 +1464,175 @@ private:
StructuredData::Boolean *boolean_val = obj_sp->GetAsBoolean();
if (!boolean_val) {
error.SetErrorStringWithFormatv("'required' field is not a boolean "
- "for option {0}", counter);
+ "for option {0}",
+ counter);
return false;
- }
- option_def.required = boolean_val->GetValue();
+ }
+ 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
+ // 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);
+ "option {0}",
+ counter);
return false;
} else if (short_str.size() != 1) {
error.SetErrorStringWithFormatv("short_option field has extra "
- "characters for option {0}", counter);
+ "characters for option {0}",
+ counter);
return false;
}
- short_option = (int) short_str[0];
+ short_option = (int)short_str[0];
} else {
- // If the short option is not provided, then we need a unique value
+ // 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 is the key from the outer dict:
if (long_option.empty()) {
- error.SetErrorStringWithFormatv("empty long_option for option {0}",
- counter);
+ error.SetErrorStringWithFormatv("empty long_option for option {0}",
+ counter);
return false;
}
auto inserted = g_string_storer.insert(long_option.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();
+ StructuredData::UnsignedInteger *uint_val =
+ obj_sp->GetAsUnsignedInteger();
if (!uint_val) {
error.SetErrorStringWithFormatv("Value type must be an unsigned "
- "integer");
+ "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);
+ "CommandArgumentType bounds",
+ val_type);
return false;
}
- option_def.argument_type = (CommandArgumentType) val_type;
+ 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();
+ StructuredData::UnsignedInteger *uint_val =
+ obj_sp->GetAsUnsignedInteger();
if (!uint_val) {
error.SetErrorStringWithFormatv("Completion type must be an "
- "unsigned integer for option {0}", counter);
+ "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);
+ "beyond the CompletionType bounds",
+ completion_type);
return false;
}
- option_def.completion_type = (CommandArgumentType) completion_type;
+ option_def.completion_type = (CommandArgumentType)completion_type;
} else
option_def.completion_type = eNoCompletion;
-
+
// Usage Text:
std::string usage_text;
obj_sp = opt_dict->GetValueForKey("help");
if (!obj_sp) {
error.SetErrorStringWithFormatv("required usage missing from option "
- "{0}", counter);
+ "{0}",
+ counter);
return false;
}
llvm::StringRef usage_stref;
usage_stref = obj_sp->GetStringValue();
if (usage_stref.empty()) {
- error.SetErrorStringWithFormatv("empty usage text for option {0}",
- counter);
+ error.SetErrorStringWithFormatv("empty usage text for option {0}",
+ counter);
return false;
}
m_usage_container[counter] = usage_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);
+ "option {0}",
+ counter);
return false;
}
size_t num_elem = array->GetSize();
size_t enum_ctr = 0;
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
// 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 {
+ 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);
+ "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);
+ "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);
+ "{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] = EnumValueStorage(value_cstr_str,
- usage_cstr_str, enum_ctr);
-
+ curr_elem[enum_ctr] =
+ EnumValueStorage(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
+ // 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);
@@ -1626,11 +1642,11 @@ private:
counter++;
return true;
}; // end of add_element
-
+
options.ForEach(add_element);
return error;
}
-
+
private:
struct EnumValueStorage {
EnumValueStorage() {
@@ -1638,30 +1654,31 @@ private:
element.usage = "usage not set";
element.value = 0;
}
-
- 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)) {
+
+ 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);
}
-
- EnumValueStorage(const EnumValueStorage &in) : value(in.value),
- usage(in.usage) {
+
+ EnumValueStorage(const EnumValueStorage &in)
+ : value(in.value), usage(in.usage) {
SetElement(in.element.value);
}
-
+
EnumValueStorage &operator=(const EnumValueStorage &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();
+ element.usage = usage.data();
}
-
+
std::string value;
std::string usage;
OptionEnumValueElement element;
@@ -1672,10 +1689,10 @@ private:
// 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
+ // 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.
+ // 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<EnumValueStorage>> m_enum_storage;
@@ -1687,16 +1704,16 @@ private:
};
public:
- static CommandObjectSP Create(CommandInterpreter &interpreter,
- std::string name,
- StructuredData::GenericSP cmd_obj_sp,
- ScriptedCommandSynchronicity synch,
- CommandReturnObject &result) {
+ 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());
+ 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();
@@ -1706,7 +1723,7 @@ public:
opt_error.AsCString());
if (arg_error.Fail())
result.AppendErrorWithFormat("%sfailed to parse argument definitions: %s",
- opt_error.Fail() ? ", also " : "",
+ opt_error.Fail() ? ", also " : "",
arg_error.AsCString());
if (!result.Succeeded())
@@ -1716,12 +1733,12 @@ 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),
+ 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();
@@ -1734,15 +1751,15 @@ public:
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);
+ StructuredData::ObjectSP options_object_sp =
+ scripter->GetOptionsForCommandObject(cmd_obj_sp);
// It's okay not to have an options dict.
if (options_object_sp) {
// 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();
+ StructuredData::Dictionary *options_dict =
+ options_object_sp->GetAsDictionary();
// but if it exists, it has to be an array.
if (options_dict) {
m_options_error = m_options.SetOptionsFromArray(*(options_dict));
@@ -1756,49 +1773,51 @@ public:
}
// 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);
+ StructuredData::ObjectSP args_object_sp =
+ scripter->GetArgumentsForCommandObject(cmd_obj_sp);
if (args_object_sp) {
- StructuredData::Array *args_array = args_object_sp->GetAsArray();
+ 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 {
+ 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 {
+ 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.
+ // 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 {
+
+ 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);
+ "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");
+ StructuredData::ObjectSP obj_sp =
+ arg_dict->GetValueForKey("arg_type");
if (obj_sp) {
- StructuredData::UnsignedInteger *uint_val
- = obj_sp->GetAsUnsignedInteger();
+ StructuredData::UnsignedInteger *uint_val =
+ obj_sp->GetAsUnsignedInteger();
if (!uint_val) {
report_error("value type must be an unsigned integer");
return false;
@@ -1808,7 +1827,7 @@ public:
report_error("value type beyond ArgumentRepetitionType bounds");
return false;
}
- arg_type = (CommandArgumentType) arg_type_int;
+ arg_type = (CommandArgumentType)arg_type_int;
}
// Repeat Value:
obj_sp = arg_dict->GetValueForKey("repeat");
@@ -1825,29 +1844,31 @@ public:
return false;
}
arg_repetition = *repeat;
- }
-
+ }
+
// Usage Mask:
obj_sp = arg_dict->GetValueForKey("groups");
- m_args_error = CommandOptions::ParseUsageMaskFromArray(obj_sp,
- counter, arg_opt_set_association);
- this_entry.emplace_back(arg_type, arg_repetition,
- arg_opt_set_association);
+ 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++;
return true;
};
StructuredData::Array *args_array = object->GetAsArray();
if (!args_array) {
m_args_error.SetErrorStringWithFormatv("Argument definition element "
- "{0} is not an array", counter);
+ "{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);
+ "{0} is empty",
+ counter);
return false;
}
m_arguments.push_back(this_entry);
@@ -1899,22 +1920,20 @@ public:
SetHelpLong(docstring);
return CommandObjectParsed::GetHelpLong();
}
-
- Options *GetOptions() override { return &m_options; }
+ Options *GetOptions() override { return &m_options; }
protected:
- void DoExecute(Args &args,
- CommandReturnObject &result) override {
+ void 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)) {
+ !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...
@@ -2327,10 +2346,10 @@ protected:
"'{0}'", m_options.m_class_name);
return;
}
-
+
if (m_options.m_parsed_command) {
- new_cmd_sp = CommandObjectScriptingObjectParsed::Create(m_interpreter,
- m_cmd_name, cmd_obj_sp, m_synchronicity, result);
+ new_cmd_sp = CommandObjectScriptingObjectParsed::Create(
+ m_interpreter, m_cmd_name, cmd_obj_sp, m_synchronicity, result);
if (!result.Succeeded())
return;
} else
diff --git a/lldb/source/Interpreter/CommandObject.cpp b/lldb/source/Interpreter/CommandObject.cpp
index fd1d33d949..82d6204933 100644
--- a/lldb/source/Interpreter/CommandObject.cpp
+++ b/lldb/source/Interpreter/CommandObject.cpp
@@ -447,21 +447,21 @@ bool CommandObject::IsPairType(ArgumentRepetitionType arg_repeat_type) {
(arg_repeat_type == eArgRepeatPairRangeOptional);
}
-std::optional<ArgumentRepetitionType>
+std::optional<ArgumentRepetitionType>
CommandObject::ArgRepetitionFromString(llvm::StringRef string) {
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({});
+ .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/PythonDataObjects.h b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
index 88c1bb7e72..e0a7aef193 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
@@ -194,8 +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<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 c1a11b9134..781061c056 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h
@@ -213,12 +213,11 @@ public:
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 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 dadcde6126..9768b38188 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -2802,7 +2802,7 @@ bool ScriptInterpreterPythonImpl::RunScriptBasedParsedCommand(
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);
@@ -2817,7 +2817,6 @@ bool ScriptInterpreterPythonImpl::RunScriptBasedParsedCommand(
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.
@@ -2936,7 +2935,7 @@ uint32_t ScriptInterpreterPythonImpl::GetFlagsForCommandObject(
return result;
}
-StructuredData::ObjectSP
+StructuredData::ObjectSP
ScriptInterpreterPythonImpl::GetOptionsForCommandObject(
StructuredData::GenericSP cmd_obj_sp) {
StructuredData::ObjectSP result = {};
@@ -2981,10 +2980,10 @@ ScriptInterpreterPythonImpl::GetOptionsForCommandObject(
PyErr_Clear();
return {};
}
- return py_return.CreateStructuredObject();
+ return py_return.CreateStructuredObject();
}
-StructuredData::ObjectSP
+StructuredData::ObjectSP
ScriptInterpreterPythonImpl::GetArgumentsForCommandObject(
StructuredData::GenericSP cmd_obj_sp) {
StructuredData::ObjectSP result = {};
@@ -3029,11 +3028,10 @@ ScriptInterpreterPythonImpl::GetArgumentsForCommandObject(
PyErr_Clear();
return {};
}
- return py_return.CreateStructuredObject();
+ return py_return.CreateStructuredObject();
}
-void
-ScriptInterpreterPythonImpl::OptionParsingStartedForCommandObject(
+void ScriptInterpreterPythonImpl::OptionParsingStartedForCommandObject(
StructuredData::GenericSP cmd_obj_sp) {
Locker py_lock(this, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock);
@@ -3041,7 +3039,7 @@ ScriptInterpreterPythonImpl::OptionParsingStartedForCommandObject(
static char callee_name[] = "option_parsing_started";
if (!cmd_obj_sp)
- return ;
+ return;
PythonObject implementor(PyRefType::Borrowed,
(PyObject *)cmd_obj_sp->GetValue());
@@ -3067,10 +3065,9 @@ ScriptInterpreterPythonImpl::OptionParsingStartedForCommandObject(
if (PyErr_Occurred())
PyErr_Clear();
- // option_parsing_starting doesn't return anything, ignore anything but
+ // option_parsing_starting doesn't return anything, ignore anything but
// python errors.
- unwrapOrSetPythonException(
- As<bool>(implementor.CallMethod(callee_name)));
+ unwrapOrSetPythonException(As<bool>(implementor.CallMethod(callee_name)));
// if it fails, print the error but otherwise go on
if (PyErr_Occurred()) {
@@ -3080,8 +3077,7 @@ ScriptInterpreterPythonImpl::OptionParsingStartedForCommandObject(
}
}
-bool
-ScriptInterpreterPythonImpl::SetOptionValueForCommandObject(
+bool ScriptInterpreterPythonImpl::SetOptionValueForCommandObject(
StructuredData::GenericSP cmd_obj_sp, ExecutionContext *exe_ctx,
llvm::StringRef long_option, llvm::StringRef value) {
StructuredData::ObjectSP result = {};
@@ -3116,15 +3112,15 @@ ScriptInterpreterPythonImpl::SetOptionValueForCommandObject(
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())));
+
+ 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()) {
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
index fcd21dff61..a89b121754 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
@@ -182,13 +182,12 @@ public:
lldb_private::CommandReturnObject &cmd_retobj, Status &error,
const lldb_private::ExecutionContext &exe_ctx) override;
- virtual bool RunScriptBasedParsedCommand(
- StructuredData::GenericSP impl_obj_sp, Args& args,
+ 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;
@@ -219,7 +218,7 @@ public:
bool GetLongHelpForCommandObject(StructuredData::GenericSP cmd_obj_sp,
std::string &dest) override;
-
+
StructuredData::ObjectSP
GetOptionsForCommandObject(StructuredData::GenericSP cmd_obj_sp) override;
@@ -228,7 +227,7 @@ public:
bool SetOptionValueForCommandObject(StructuredData::GenericSP cmd_obj_sp,
ExecutionContext *exe_ctx,
- llvm::StringRef long_option,
+ llvm::StringRef long_option,
llvm::StringRef value) override;
void OptionParsingStartedForCommandObject(
diff --git a/lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp b/lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
index 5f0cc4c23d..57d37395b5 100644
--- a/lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
+++ b/lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
@@ -219,7 +219,7 @@ bool lldb_private::python::SWIGBridge::LLDBSwigPythonCallCommandObject(
}
bool lldb_private::python::SWIGBridge::LLDBSwigPythonCallParsedCommandObject(
- PyObject *implementor, lldb::DebuggerSP debugger,
+ PyObject *implementor, lldb::DebuggerSP debugger,
StructuredDataImpl &args_impl,
lldb_private::CommandReturnObject &cmd_retobj,
lldb::ExecutionContextRefSP exe_ctx_ref_sp) {
|
|
I described this in some more detail here: https://discourse.llvm.org/t/rfc-parsed-commands-in-python/74532 |
|
Note, I haven't run this though black or clang-format yet... |
bulbazord
reviewed
Nov 2, 2023
lldb/bindings/python/CMakeLists.txt
Outdated
Comment on lines
99
to
100
| "${LLDB_SOURCE_DIR}/examples/python/symbolication.py" | ||
| "${LLDB_SOURCE_DIR}/examples/python/parsed_cmd.py") |
There was a problem hiding this comment.
Please sort these
Also, I wonder if we should put the ) on a separate line for code archaeology sake?
| @@ -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, | |||
There was a problem hiding this comment.
Spelling mistake: implementor -> implementer
There was a problem hiding this comment.
I'll get rid of function altogether later this week, replacing by a call to the ScriptedPythonInterface methods
There was a problem hiding this comment.
It's spelled incorrectly in the whole rest of this file except for the thread plan instances. We should either get rid of all of these by using ScriptedPythonInterface, or fix all the spellings to be consistent, but that seems better as a separate commit.
There was a problem hiding this comment.
Let's fix the rest of the spellings in a follow up.
There was a problem hiding this comment.
Note, whoever spelt this originally was sure implementor was what they wanted to use, the C++ files use the same spelling.
Comment on lines
844
to
845
| if (!pfunc.IsAllocated()) | ||
| return false; |
There was a problem hiding this comment.
Can we update the CommandReturnObject with useful feedback about what failed? The bool return value is super opaque.
There was a problem hiding this comment.
I added this here, but we really need a more general mechanism for informing the user when the python code we've been handed is lacking some affordance. Most of these wrappers don't have a CommandReturnObject to complain through.
Comment on lines
849
to
857
| // 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. |
There was a problem hiding this comment.
Do you have a backtrace or other useful info here? It would be useful to actually write out what you want the code to be instead of this so future contributors can reproduce the crash.
There was a problem hiding this comment.
My guess is that the backtrace only contains for python internal non symbolicated frames that are basically useless for debugging. I've experienced that myself multiple times.
| if (!pfunc.IsAllocated()) | ||
| return false; | ||
|
|
||
| auto cmd_retobj_arg = SWIGBridge::ToSWIGWrapper(cmd_retobj); |
There was a problem hiding this comment.
Why are you doing this here instead of in the invocation of pfunc below like you do for all the other arguments?
There was a problem hiding this comment.
That must have been problematic in development. This sort of compressed style is less good when you are debugging...
Comment on lines
1660
to
1650
| EnumValueStorer(std::string &in_str_val, std::string &in_usage, | ||
| size_t in_value) : value(in_str_val), usage(in_usage) { |
There was a problem hiding this comment.
I recommend passing by value and moving instead of passing by value and copying.
There was a problem hiding this comment.
So in general, my (admittedly simplified) understanding of this:
The parameter being a reference type means that when you construct value with value(in_str_val) you'll end up copying no matter what. It doesn't matter if you passed an r-value to EnumValueStorer, you're going to end up constructing 2 std::string objects instead of 1.
However, if in_str_val was type std::string and you performed a move (e.g. value(std::move(in_str_val))), then if you pass an r-value to the constructor, you'll end up avoiding a copy (and end up only constructing 1 std::string total). It sounds like a minor optimization, but unnecessary copies are one of those "death by a thousand paper cuts" kind of things, especially if you have lots of tiny extra allocations.
Maybe some of the newer copy elision guarantees from c++17 make this a non-issue? I think I may need to do some more reading...
There was a problem hiding this comment.
I made the change, but I hope along with your last sentence that this sort of voodoo is not really required to make the call performant. That's a bit TOO fiddly.
Comment on lines
+1708
to
+1732
| 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) { |
There was a problem hiding this comment.
I think it would be worth creating a static factory method instead of having a fallible constructor for this. If you have no ScriptInterpreter, this fails but you still get a half-baked CommandObjectScriptingObjectParsed object.
There was a problem hiding this comment.
That's CommandObjectScriptingObjectParsed::Create, just above this comment.
Comment on lines
452
to
463
| 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 {}; |
There was a problem hiding this comment.
I'd recommend using llvm::StringSwitch for something like this.
Comment on lines
+2758
to
+2772
| bool ScriptInterpreterPythonImpl::RunScriptBasedParsedCommand( | ||
| StructuredData::GenericSP impl_obj_sp, Args &args, | ||
| ScriptedCommandSynchronicity synchronicity, | ||
| lldb_private::CommandReturnObject &cmd_retobj, Status &error, |
There was a problem hiding this comment.
Why return a bool instead of a Status object? You've got an error mutable ref already, that could just be the return value
There was a problem hiding this comment.
In this patch, I was just adding the Parsed command to the Raw one that was already there, without changing more than I needed to. If we want to go back and clean up little things like this in both interfaces, I'd rather do that as a separate piece of work.
| if (PyErr_Occurred()) | ||
| PyErr_Clear(); | ||
|
|
||
| // FIXME: this should really be a void function |
There was a problem hiding this comment.
The function being called is a void return, but I didn't see a way to call that. I was explaining why I ignored the return, but I made the comment clearer
hawkinsw
reviewed
Nov 2, 2023
lldb/examples/python/parsed_cmd.py
Outdated
|
|
||
| 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. |
There was a problem hiding this comment.
Suggested change
| You can access the option values using varname you passed in when defining the option. | |
| You can access the option values using the varname you passed in when defining the option. |
hawkinsw
reviewed
Nov 2, 2023
lldb/examples/python/parsed_cmd.py
Outdated
| # 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 |
There was a problem hiding this comment.
Seeing the hardcoded string literals throughout this function makes me think it might be good to abstract those behind some constants so that they could be changed in the future?
There was a problem hiding this comment.
These shouldn't be used outside of the parser itself, so I'm not sure that we'd get anything out of one more layer of abstraction here.
hawkinsw
reviewed
Nov 2, 2023
|
@jimingham Looks like great work -- I hope my few comments were helpful! |
clayborg
reviewed
Nov 2, 2023
There was a problem hiding this comment.
I like this idea. Since python is so dynamic, I wonder if we can just add extra metadata to the standard "argparse" objects so that users can just write their stuff in the most pythonic way possible using import argparse and then adding some extra metadata that lldb can access. For example something like:
$ python3
Python 3.8.9 (default, Jul 27 2021, 02:53:04)
[Clang 7.1.0 (tags/RELEASE_710/final)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import argparse
>>> ap = argparse.ArgumentParser()
>>> v = ap.add_argument('-v')
>>> print(v)
_StoreAction(option_strings=['-v'], dest='v', nargs=None, const=None, default=None, type=None, choices=None, help=None, metavar=None)
>>> print(type(v))
<class 'argparse._StoreAction'>
>>> v.lldb_type = lldb.eArgTypeBoolean
>>> v.lldb_type
lldb.eArgTypeBoolean
Then users can use the standard argparse module and just add a bit of extra metadata to it that LLDB can then access when needed. This would remove the need for a custom LLDBOVParser class.
lldb/examples/python/parsed_cmd.py
Outdated
| """ | ||
| 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. |
There was a problem hiding this comment.
s/class for you command/class for your command/
There was a problem hiding this comment.
The problem with that approach is that completion happens in C++ in the CommandInterpreter, and if argparse were doing the parsing, to support completion the CommandInterpreter would have to call back to argparse to figure out which option was being completed.
What I tried to do is make the interface for defining your options look the same as what you would have written for argparse so that if you have an extant command it will be easy to convert it and if you know argparse, you know how to define lldb commands already, the only difference being you don't use argparse, you add it to our object instead.
Look at the test_commands.py in the tests to get a sense of what that looks like.
There was a problem hiding this comment.
I converted the lldb/examples/python/cmdtemplate.py to the new form in the latest patch, if you want to get a sense of what you have to do to convert from one form to the other. I think this is pretty straightforward. But it also can't be a straight translation because you need to be able to specify the lldb.eArgType... . Anyway, have a look at the diff between the two, and if you can think of anything to make this easier, I'm happy for suggestions (maybe as a follow-on patch, if this looks roughly okay, however...)
|
I actually run against a hand-build Python with debug symbols, so it was in fact a backtrace that contains only python internal functions, and even though they had symbols I still couldn't tell what was going on... There must be a bunch of handy tricks for debugging the Python interpreter, but I don't know them.
Jim
… On Nov 1, 2023, at 11:06 PM, Med Ismail Bennani ***@***.***> wrote:
@medismailben commented on this pull request.
In lldb/bindings/python/python-wrapper.swig <#70734 (comment)>:
> + // 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.
My guess is that the backtrace only contains for python internal non symbolicated frames that are basically useless for debugging. I've experienced that myself multiple times.
—
Reply to this email directly, view it on GitHub <#70734 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW4NABS6EBDGMGSXDE3YCMZ6HAVCNFSM6AAAAAA6WXATNCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMBZGUYTCNRTGE>.
You are receiving this because you were mentioned.
|
|
I addressed most of the review comments - a few I had questions about - and also did the clang-format & python-deformatting. |
You can test this locally with the following command:darker --check --diff -r 997ffce43c6d2d3f647eb091c732665049b1f47f...ea2b242fdd67885a5ea5b78cedcce4f1cd7462b4 lldb/examples/python/templates/parsed_cmd.py lldb/test/API/commands/command/script/add/TestAddParsedCommand.py lldb/test/API/commands/command/script/add/test_commands.py lldb/examples/python/cmdtemplate.pyView the diff from darker here.--- examples/python/cmdtemplate.py 2024-02-12 23:21:46.000000 +0000
+++ examples/python/cmdtemplate.py 2024-02-13 01:11:00.544627 +0000
@@ -12,10 +12,11 @@
import inspect
import lldb
import sys
from lldb.plugins.parsed_cmd import ParsedCommand
+
class FrameStatCommand(ParsedCommand):
program = "framestats"
@classmethod
def register_lldb_command(cls, debugger, module_name):
@@ -24,68 +25,68 @@
'The "{0}" command has been installed, type "help {0}" or "{0} '
'--help" for detailed help.'.format(cls.program)
)
def setup_command_definition(self):
-
self.ov_parser.add_option(
"i",
"in-scope",
- help = "in_scope_only = True",
- value_type = lldb.eArgTypeBoolean,
- dest = "bool_arg",
- default = True,
+ help="in_scope_only = True",
+ value_type=lldb.eArgTypeBoolean,
+ dest="bool_arg",
+ default=True,
)
self.ov_parser.add_option(
"i",
"in-scope",
- help = "in_scope_only = True",
- value_type = lldb.eArgTypeBoolean,
- dest = "inscope",
+ help="in_scope_only = True",
+ value_type=lldb.eArgTypeBoolean,
+ dest="inscope",
default=True,
)
-
+
self.ov_parser.add_option(
"a",
"arguments",
- help = "arguments = True",
- value_type = lldb.eArgTypeBoolean,
- dest = "arguments",
- default = True,
+ help="arguments = True",
+ value_type=lldb.eArgTypeBoolean,
+ dest="arguments",
+ default=True,
)
self.ov_parser.add_option(
"l",
"locals",
- help = "locals = True",
- value_type = lldb.eArgTypeBoolean,
- dest = "locals",
- default = True,
+ help="locals = True",
+ value_type=lldb.eArgTypeBoolean,
+ dest="locals",
+ default=True,
)
self.ov_parser.add_option(
"s",
"statics",
- help = "statics = True",
- value_type = lldb.eArgTypeBoolean,
- dest = "statics",
- default = True,
+ help="statics = True",
+ value_type=lldb.eArgTypeBoolean,
+ dest="statics",
+ default=True,
)
def get_short_help(self):
return "Example command for use in debugging"
def get_long_help(self):
- return ("This command is meant to be an example of how to make "
+ return (
+ "This command is meant to be an example of how to make "
"an LLDB command that does something useful, follows "
"best practices, and exploits the SB API. "
"Specifically, this command computes the aggregate "
"and average size of the variables in the current "
"frame and allows you to tweak exactly which variables "
- "are to be accounted in the computation.")
-
+ "are to be accounted in the computation."
+ )
def __init__(self, debugger, unused):
super().__init__(debugger, unused)
def __call__(self, debugger, command, exe_ctx, result):
@@ -95,11 +96,14 @@
if not frame.IsValid():
result.SetError("invalid frame")
return
variables_list = frame.GetVariables(
- self.ov_parser.arguments, self.ov_parser.locals, self.ov_parser.statics, self.ov_parser.inscope
+ self.ov_parser.arguments,
+ self.ov_parser.locals,
+ self.ov_parser.statics,
+ self.ov_parser.inscope,
)
variables_count = variables_list.GetSize()
if variables_count == 0:
print("no variables here", file=result)
return
--- examples/python/templates/parsed_cmd.py 2024-02-13 01:07:15.000000 +0000
+++ examples/python/templates/parsed_cmd.py 2024-02-13 01:11:00.619006 +0000
@@ -50,14 +50,15 @@
import inspect
import lldb
import sys
from abc import abstractmethod
+
class LLDBOptionValueParser:
"""
This class holds the option definitions for the command, and when
- the command is run, you can ask the parser for the current values. """
+ the command is run, you can ask the parser for the current values."""
def __init__(self):
# 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 = {}
@@ -78,46 +79,46 @@
low_in = in_value.lower()
if low_in in ["y", "yes", "t", "true", "1"]:
value = True
error = False
-
+
if not value and low_in in ["n", "no", "f", "false", "0"]:
value = False
error = False
return (value, error)
@staticmethod
def to_int(in_value):
- #FIXME: Not doing errors yet...
+ # FIXME: Not doing errors yet...
return (int(in_value), False)
@staticmethod
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
+ 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):
try:
@@ -129,39 +130,39 @@
# 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
+ 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):
return cls.completion_table.get(arg_type, lldb.eNoCompletion)
@@ -177,26 +178,26 @@
if not elem:
return False
return "enum_values" in elem
def option_parsing_started(self):
- """ This makes the ivars for all the "dest" values in the array and gives them
- their default values. You should not have to call this by hand, though if
- you have some option that needs to do some work when a new command invocation
- starts, you can override this to handle your special option. """
+ """This makes the ivars for all the "dest" values in the array and gives them
+ their default values. You should not have to call this by hand, though if
+ you have some option that needs to do some work when a new command invocation
+ starts, you can override this to handle your special option."""
for key, elem in self.options_dict.items():
- elem['_value_set'] = False
+ elem["_value_set"] = False
try:
object.__setattr__(self, elem["dest"], elem["default"])
except AttributeError:
# It isn't an error not to have a "dest" variable name, you'll
# just have to manage this option's value on your own.
continue
def set_enum_value(self, enum_values, input):
- """ This sets the value for an enum option, you should not have to call this
- by hand. """
+ """This sets the value for an enum option, you should not have to call this
+ by hand."""
candidates = []
for candidate in enum_values:
# The enum_values are a two element list of value & help string.
value = candidate[0]
if value.startswith(input):
@@ -204,50 +205,59 @@
if len(candidates) == 1:
return (candidates[0], False)
else:
return (input, True)
-
+
def set_option_value(self, exe_ctx, opt_name, opt_value):
- """ This sets a single option value. This will handle most option
+ """This sets a single option value. This will handle most option
value types, but if you have an option that has some complex behavior,
you can override this to implement that behavior, and then pass the
- rest of the options to the base class implementation. """
+ rest of the options to the base class implementation."""
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)
+ (value, error) = __class__.translate_value(elem["value_type"], opt_value)
if error:
return False
-
+
object.__setattr__(self, elem["dest"], value)
elem["_value_set"] = True
return True
def was_set(self, opt_name):
- """ Call this in the __call__ method of your command to determine
- whether this option was set on the command line. It is sometimes
- useful to know whether an option has the default value because the
- user set it explicitly (was_set -> True) or not. """
+ """Call this in the __call__ method of your command to determine
+ whether this option was set on the command line. It is sometimes
+ useful to know whether an option has the default value because the
+ user set it explicitly (was_set -> True) or not."""
elem = self.get_option_element(opt_name)
if not elem:
return False
try:
return elem["_value_set"]
except AttributeError:
return False
- def add_option(self, short_option, long_option, help, default,
- dest = None, required=False, groups = None,
- value_type=lldb.eArgTypeNone, completion_type=None,
- enum_values=None):
+ def add_option(
+ self,
+ short_option,
+ long_option,
+ help,
+ default,
+ dest=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
help: a usage string for this option, will print in the command help
default: the initial value for this option (if it has a value)
@@ -258,46 +268,49 @@
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.
+ 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 dest:
dest = long_option
if not completion_type:
completion_type = self.determine_completion(value_type)
-
- dict = {"short_option" : short_option,
- "required" : required,
- "help" : help,
- "value_type" : value_type,
- "completion_type" : completion_type,
- "dest" : dest,
- "default" : default}
+
+ dict = {
+ "short_option": short_option,
+ "required": required,
+ "help": help,
+ "value_type": value_type,
+ "completion_type": completion_type,
+ "dest": dest,
+ "default": default,
+ }
if enum_values:
dict["enum_values"] = enum_values
if groups:
dict["groups"] = groups
self.options_dict[long_option] = dict
- def make_argument_element(self, arg_type, repeat = "optional", groups = None):
- element = {"arg_type" : arg_type, "repeat" : repeat}
+ 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
+
class ParsedCommand:
def __init__(self, debugger, unused):
self.debugger = debugger
self.ov_parser = LLDBOptionValueParser()
self.setup_command_definition()
-
+
def get_options_definition(self):
return self.get_parser().options_dict
def get_flags(self):
return 0
@@ -305,11 +318,11 @@
def get_args_definition(self):
return self.get_parser().args_array
# The base class will handle calling these methods
# when appropriate.
-
+
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)
@@ -327,11 +340,11 @@
# These are the two "pure virtual" methods:
@abstractmethod
def __call__(self, debugger, args_array, exe_ctx, result):
"""This is the command callback. The option values are
provided by the 'dest' properties on the parser.
-
+
args_array: This is the list of arguments provided.
exe_ctx: Gives the SBExecutionContext on which the
command should operate.
result: Any results of the command should be
written into this SBCommandReturnObject.
@@ -345,16 +358,16 @@
options and argument definitions for the command."""
raise NotImplementedError()
@staticmethod
def do_register_cmd(cls, debugger, module_name):
- """ Add any commands contained in this module to LLDB """
+ """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)
+ "for detailed help.".format(cls.program)
)
--- test/API/commands/command/script/add/TestAddParsedCommand.py 2024-01-21 18:42:58.000000 +0000
+++ test/API/commands/command/script/add/TestAddParsedCommand.py 2024-02-13 01:11:00.669297 +0000
@@ -14,11 +14,11 @@
NO_DEBUG_INFO_TESTCASE = True
def test(self):
self.pycmd_tests()
- def check_help_options(self, cmd_name, opt_list, substrs = []):
+ 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.
@@ -28,119 +28,172 @@
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)
+ 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"])
+ 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)
+ 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>"]
-
+ 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)
+ 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)
+ 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)
-
+ 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)
-
+ 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):"])
+ 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"])
+ 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):"])
+ 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)
+ # 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")
+ 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")
+ # 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)}")
+ 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")
+ 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")
-
+ # 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"])
+ self.expect(
+ "two-args 'First Argument' 'Second Argument'",
+ substrs=["0: First Argument", "1: Second Argument"],
+ )
--- test/API/commands/command/script/add/test_commands.py 2024-02-12 23:21:46.000000 +0000
+++ test/API/commands/command/script/add/test_commands.py 2024-02-13 01:11:00.730069 +0000
@@ -4,30 +4,36 @@
import inspect
import sys
import lldb
from lldb.plugins.parsed_cmd import ParsedCommand
+
class ReportingCmd(ParsedCommand):
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 long_option, elem in opt_def.items():
dest = elem["dest"]
- result.AppendMessage(f"{long_option} (set: {elem['_value_set']}): {object.__getattribute__(self.ov_parser, dest)}\n")
+ result.AppendMessage(
+ f"{long_option} (set: {elem['_value_set']}): {object.__getattribute__(self.ov_parser, dest)}\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")
-
+ 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)
@@ -39,62 +45,65 @@
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],
- dest = "bool_arg",
- default = True
+ value_type=lldb.eArgTypeBoolean,
+ groups=[1, 2],
+ dest="bool_arg",
+ default=True,
)
self.ov_parser.add_option(
"s",
"shlib-name",
"A shared library name.",
value_type=lldb.eArgTypeShlibName,
- groups = [1, [3,4]],
- dest = "shlib_name",
- default = None
+ groups=[1, [3, 4]],
+ dest="shlib_name",
+ default=None,
)
self.ov_parser.add_option(
"d",
"disk-file-name",
"An on disk filename",
- value_type = lldb.eArgTypeFilename,
- dest = "disk_file_name",
- default = None
+ value_type=lldb.eArgTypeFilename,
+ dest="disk_file_name",
+ default=None,
)
self.ov_parser.add_option(
"l",
"line-num",
"A line number",
- value_type = lldb.eArgTypeLineNum,
- groups = 3,
- dest = "line_num",
- default = 0
- )
-
+ value_type=lldb.eArgTypeLineNum,
+ groups=3,
+ dest="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,
- dest = "enum_option",
- default = "foo"
- )
-
+ enum_values=[
+ ["foo", "does foo things"],
+ ["bar", "does bar things"],
+ ["baz", "does baz things"],
+ ],
+ groups=4,
+ dest="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):
@@ -103,17 +112,20 @@
@classmethod
def register_lldb_command(cls, debugger, module_name):
ParsedCommand.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")])
-
+ 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):
@@ -126,46 +138,63 @@
def setup_command_definition(self):
self.ov_parser.add_option(
"l",
"language",
"language defaults to None",
- value_type = lldb.eArgTypeLanguage,
- groups = [1,2],
- dest = "language",
- default = None
+ value_type=lldb.eArgTypeLanguage,
+ groups=[1, 2],
+ dest="language",
+ default=None,
)
self.ov_parser.add_option(
"c",
"log-channel",
"log channel - defaults to lldb",
value_type=lldb.eArgTypeLogChannel,
- groups = [1, 3],
- dest = "log_channel",
- default = "lldb"
+ groups=[1, 3],
+ dest="log_channel",
+ default="lldb",
)
self.ov_parser.add_option(
"p",
"process-name",
"A process name, defaults to None",
- value_type = lldb.eArgTypeProcessName,
- dest = "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])])
-
+ value_type=lldb.eArgTypeProcessName,
+ dest="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(
|
CommandObjectParsed way to specify options and arguments so that these commands can be "first class citizens" with build-in commands.
jimingham
force-pushed
the
parsed-commands
branch
from
aaeb653 to
580f68d
Compare
Also remove a couple stray prints in parsed_cmd.py.
No reason to have unnecessary differences from the way argparse did things. This will make it easier to convert. `help` however is not an optional argument in the new setup.
|
Okay, I made one more trivial change, I switched the |
|
At this point, I'd like to get this in so I can iterate on it in future PR's rather than churning this one. If folks have a bit to look over this, that would be great. Thanks! |
lldb/examples/python/parsed_cmd.py
Outdated
| You can access the option values using the 'dest' string 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 | ||
| use the was_set API. |
There was a problem hiding this comment.
From what, is this an SB API thing or some python builtin?
There was a problem hiding this comment.
If you have a one line example like the ones above that'd be good.
There was a problem hiding this comment.
Yes, it's a part of the ov_parser API. I added examples.
lldb/examples/python/parsed_cmd.py
Outdated
| import sys | ||
| from abc import abstractmethod | ||
|
|
||
| class LLDBOVParser: |
There was a problem hiding this comment.
I think I said this before, but I would just expand the class name to LLDBOptionValueParser (despite how Java enterprise ready (tm) that sounds).
It'll be read many more times than it's written and this example file is already enough to think about without chasing down what this acronym means. If the user's code is going to be littered with the literal name, then maybe keep it short but otherwise it's just one more unknown for them.
There was a problem hiding this comment.
self.ov_parser is fine as a variable name since any IDE helper will show this type name for it.
There was a problem hiding this comment.
Yes, I agree, nobody will ever type this one because that's not even the class you override. So there's no reason for it to have a mysterious name.
lldb/examples/python/parsed_cmd.py
Outdated
| value = True | ||
| error = False | ||
|
|
||
| if not value and low_in == "n" or low_in == "no" or low_in == "f" or low_in == "false" or low_in == "0": |
There was a problem hiding this comment.
if low_in in [<all of the possible things>]: is slightly easier to read and update if needed.
Also error is always set false so move that out of the ifs to before the return.
There was a problem hiding this comment.
string in array of strings is definitely easier to read, thanks!
I don't think the second comment is right, however. Not all input values will make it into either of those if statements, for instance "low_in = definitely_not_a_boolean" will be rejected by both if's and so "error" will be left at True.
All of this conversion code should actually go away, because it's silly to have two versions of OptionArgParser::ToBool, one in C++ and one here in Python. This is just a shortcut for now because this patch was already getting somewhat unwieldily.
lldb/examples/python/parsed_cmd.py
Outdated
| try: | ||
| return cls.completion_table[arg_type] | ||
| except KeyError: | ||
| return lldb.eNoCompletion |
There was a problem hiding this comment.
Could be cls.completion_table.get(arg_type, lldb.eNoCompletion).
There was a problem hiding this comment.
Yes, apparently I learned about get between writing this method and the next one...
lldb/examples/python/parsed_cmd.py
Outdated
Comment on lines
169
to
170
| # It isn't an error not to have a target, you'll just have to set and | ||
| # get this option value on your own. |
There was a problem hiding this comment.
Indent to match continue.
There was a problem hiding this comment.
Also what is "target" here? Is that the option you want to set the value on?
There was a problem hiding this comment.
Right, I think I made the comment a little clearer. I don't want to go too much into this right now. One way to extend these commands for fancier option setting is to override set_option_value and handle your special options first, then delegate the others to the base class method. But I think it would be more convenient to add a way to do custom setters. Anyway, I'm not decided on that yet.
lldb/examples/python/parsed_cmd.py
Outdated
Comment on lines
196
to
200
| if not error: | ||
| object.__setattr__(self, elem["dest"], value) | ||
| elem["_value_set"] = True | ||
| return True | ||
| return False |
There was a problem hiding this comment.
if error:
return False
object.__setattr__(self, elem["dest"], value)
elem["_value_set"] = True
return True
Seems simpler to me.
bulbazord
approved these changes
Feb 10, 2024
medismailben
requested changes
Feb 11, 2024
lldb/examples/python/parsed_cmd.py
Outdated
Comment on lines
1
to
49
| """ | ||
| 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 your command that inherits from ParsedCommandBase. | ||
| That will make an LLDBOptionValueParser 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 | ||
| 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_list, exe_ctx, result): | ||
|
|
||
| The arguments will be a list of strings. | ||
|
|
||
| You can access the option values using the 'dest' string you passed in when defining the option. | ||
| And if you need to know whether a given option was set by the user or not, you can | ||
| use the was_set API. | ||
|
|
||
| So for instance, if you have an option whose "dest" is "my_option", then: | ||
|
|
||
| self.get_parser().my_option | ||
|
|
||
| will fetch the value, and: | ||
|
|
||
| self.get_parser().was_set("my_option") | ||
|
|
||
| will return True if the user set this option, and False if it was left at its default | ||
| value. | ||
|
|
||
| There are example commands in the lldb testsuite at: | ||
|
|
||
| llvm-project/lldb/test/API/commands/command/script/add/test_commands.py | ||
| """ |
There was a problem hiding this comment.
Although I really like this kind of narrated documentation style, I don't think its concise enough for users and might discourage some from using this feature because the documentation was too long to read.
I think we should keep this, but still provide docstrings comments with arguments and return types for every methods of this class, like we do in the scripted process base class implementation:
llvm-project/lldb/examples/python/templates/scripted_process.py
Lines 64 to 75 in 2c3ba9f
Or at least the methods that the user might use.
There was a problem hiding this comment.
I added some docs, see what you think.
| @@ -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]>, | |||
There was a problem hiding this comment.
Let's swap the short-option between script_add_class & script_add_completion_type. Scripted Process uses -C (capital C) in process launch & process attach to specify the python class. Same for scripted thread plans with thread step-scripted.
Let's try to stay consistent for our users.
There was a problem hiding this comment.
I didn't add either of these options, they predate this patch. This would break anybody that's adding scripted commands. I don't remember why it's this way, but I don't think we can change it.
| 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">, |
There was a problem hiding this comment.
This one also needs to be swapped.
lldb/examples/python/parsed_cmd.py
Outdated
| def add_argument_set(self, arguments): | ||
| self.args_array.append(arguments) | ||
|
|
||
| class ParsedCommandBase: |
There was a problem hiding this comment.
If this is supposed to be the base implementation, I think it needs to move to https://github.com/llvm/llvm-project/tree/main/lldb/examples/python/templates in its own file.
Also, I'd drop the Base suffix on the class name, since we don't do this for other base implementation classes.
medismailben
approved these changes
Feb 13, 2024
There was a problem hiding this comment.
Awesome! Thanks for addressing my comments @jimingham :) LGTM!
| @@ -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]>, | |||
|
|
||
| class LLDBOptionValueParser: | ||
| """ | ||
| This class is holds the option definitions for the command, and when |
There was a problem hiding this comment.
nit: typo
Suggested change
| This class is holds the option definitions for the command, and when | |
| This class holds the option definitions for the command, and when |
jimingham
added a commit
to jimingham/from-apple-llvm-project
that referenced
this pull request
Feb 15, 2024
…ectParsed (llvm#70734) This allows you to specify options and arguments and their definitions and then have lldb handle the completions, help, etc. in the same way that lldb does for its parsed commands internally. This feature has some design considerations as well as the code, so I've also set up an RFC, but I did this one first and will put the RFC address in here once I've pushed it... Note, the lldb "ParsedCommand interface" doesn't actually do all the work that it should. For instance, saying the type of an option that has a completer doesn't automatically hook up the completer, and ditto for argument values. We also do almost no work to verify that the arguments match their definition, or do auto-completion for them. This patch allows you to make a command that's bug-for-bug compatible with built-in ones, but I didn't want to stall it on getting the auto-command checking to work all the way correctly. As an overall design note, my primary goal here was to make an interface that worked well in the script language. For that I needed, for instance, to have a property-based way to get all the option values that were specified. It was much more convenient to do that by making a fairly bare-bones C interface to define the options and arguments of a command, and set their values, and then wrap that in a Python class (installed along with the other bits of the lldb python module) which you can then derive from to make your new command. This approach will also make it easier to experiment. See the file test_commands.py in the test case for examples of how this works. (cherry picked from commit a69ecb2)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows you to specify options and arguments and their definitions and then have lldb handle the completions, help, etc. in the same way that lldb does for its parsed commands internally.
This feature has some design considerations as well as the code, so I've also set up an RFC, but I did this one first and will put the RFC address in here once I've pushed it...
Note, the lldb "ParsedCommand interface" doesn't actually do all the work that it should. For instance, saying the type of an option that has a completer doesn't automatically hook up the completer, and ditto for argument values. We also do almost no work to verify that the arguments match their definition, or do auto-completion for them. This patch allows you to make a command that's bug-for-bug compatible with built-in ones, but I didn't want to stall it on getting the auto-command checking to work all the way correctly.
As an overall design note, my primary goal here was to make an interface that worked well in the script language. For that I needed, for instance, to have a property-based way to get all the option values that were specified. It was much more convenient to do that by making a fairly bare-bones C interface to define the options and arguments of a command, and set their values, and then wrap that in a Python class (installed along with the other bits of the lldb python module) which you can then derive from to make your new command. This approach will also make it easier to experiment.
See the file test_commands.py in the test case for examples of how this works.