[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

Thu Oct 10 18:17:31 PDT 2019

jingham added a comment.

In D68671#1704803 <https://reviews.llvm.org/D68671#1704803>, @clayborg wrote:

> Looks good overall. A few questions:
>
> - should we do a global "s/extra_args/args/" edit in this patch? Not sure what "extra_" is adding? Can we remove?

Since we still support the form that doesn't take extra_args, they do feel "extra" to me.  I also like the "extra" because it helps make it clear these are just free bits of data that the callback system just passes along.  After all, there are some fixed arguments to the call back that do have specific meanings.

Note, I called the equivalent feature "extra_args" for the scripted breakpoint resolvers (added last year) and scripted thread plans, so if we change it here, we need to change it in those places as well.  If we're going to do that, I'll do it as a follow on patch because I don't want to mix those changes in with this patch.  But as I said, I like the "extra".

> - Can the structured data not be a dictionary? Valid StructuredData could be "malloc" or "[0x1000, 0x2000]" or "true", etc. If the data is required to be a dictionary we should provide lldb::SBError support for the new API calls to ensure we can let the user know what they did wrong. Be good to test this as well.

The only place where I treat the extra_args as a Dictionary is when I use as a way to tell whether the extra_args is empty, which I need to write the right wrapper function in GenerateBreakpointCommandCallbackData.  That function is little annoying because it doesn't know whether it is calling a function or some text gotten from the I/O handler, so I can't count the function arguments there to decide which signature to use.

Let me see if I can make this cleaner so I don't have to rely on that.  I really don't intend to limit fancier uses of the extra args.

Note, the command-line doesn't allow you to make anything but a dictionary (through the -k -v option pairs).  I'm actually happy with that, it enforces some kind of structure on the breakpoint command callback that you write.  You'll thank me when you want to add a second or third parameter...

Plus once you write something you are going to use frequently, you can do:

(lldb) command alias caller_break breakpoint command add -F MyCommands.break_when_parent -k func_name -v %1
(lldb) caller_break Caller -n Callee

That's how you would really want to use it.

> - is it possible to update the args for a callback function? That would seem like a good thing you might want to do? And if so, to test?

There's no way to do this.  "breakpoint command add" is a bit poorly named, since it really overwrites the current command rather than adding to it.  But that's the way it has always worked.   So only giving you the option to provide a new pair {python function, extra_args} is in keeping with its behavior.

People have asked for a "breakpoint command modify" that could chain python callbacks, or incrementally add command-line commands.  That would also naturally allow you to change/add to the extra args in place.  But that's a whole other piece of work that I don't want to take on here.

> - If it is possible to update the structured data, and if the structured data can be anything (array, string, boolean, dictionary), we might want an option like: --json "..." to be able to set or update from the command line? The current -k -v options seem to imply it must be a dict and only one level deep?

I thought about adding --json when making the OptionGroupPythonClassWithDict.  The completionist side of me was sorely tempted, especially since it would be quite easy to do.  But the options for "break set" are already several pages long, so I don't want to add more options to it just "because I can".

For all the use cases I can think of, a simple one level dictionary will suffice.  If you want to do something fancier, you can make an arbitrarily complicated SBStructuredData in Python, and then use SBBreakpoint.SetScriptCallbackFunction to pass it in.  And if you want to do that something fancier a lot, you can make this into a python command.

One useful enhancement that wouldn't overburden "break set" would be to call ParseJSON on the -v option value, and if that succeeded add the resultant StructuredData::ObjectSP as the value.  That would allow you to pass lists in for the value, which does seem useful.  You can of course do this yourself by getting the string value out in your callback and passing it to SBStructuredData.ParseFromJSON().  So I don't think it's important to add this now. I'd also need to assure myself that this wouldn't surprise people by munging up legitimate string values.

But anyway, I'd prefer to do that as a follow on, since I'm running out of time to play with this this time round the wheel...

Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68671/new/

https://reviews.llvm.org/D68671