jingham marked an inline comment as done.
jingham added inline comments.
================
Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+
+ virtual int GetNumArgumentsForCallable(const char *callable_name) {
+ return -1;
----------------
labath wrote:
> labath wrote:
> > lawrence_danna wrote:
> > > jingham wrote:
> > > > lawrence_danna wrote:
> > > > > labath wrote:
> > > > > > jingham wrote:
> > > > > > > lawrence_danna wrote:
> > > > > > > > labath wrote:
> > > > > > > > > In light of varargs functions (`*args, **kwargs`), which are
> > > > > > > > > fairly popular in python, the concept of "number of arguments
> > > > > > > > > of a callable" does not seem that well defined. The current
> > > > > > > > > implementation seems to return the number of fixed arguments,
> > > > > > > > > which might be fine, but I think this behavior should be
> > > > > > > > > documented. Also, it would be good to modernize this function
> > > > > > > > > signature -- have it take a StringRef, and return a
> > > > > > > > > `Expected<unsigned (?)>` -- ongoing work by @lawrence_danna
> > > > > > > > > will make it possible to return errors from the python
> > > > > > > > > interpreter, and this will make it possible to display those,
> > > > > > > > > instead of just guessing that this is because the callable
> > > > > > > > > was not found (it could in fact be because the named thing is
> > > > > > > > > not a callable, of because resolving the name produced an
> > > > > > > > > exception, ...).
> > > > > > > > I just took a look at PythonCallable::GetNumArguments() and
> > > > > > > > it's horribly broken.
> > > > > > > >
> > > > > > > > It doesn't even work for the simplest test case I could think
> > > > > > > > of.
> > > > > > > >
> > > > > > > > ```
> > > > > > > > auto builtins = PythonModule::Import("builtins");
> > > > > > > > ASSERT_THAT_EXPECTED(builtins, llvm::Succeeded());
> > > > > > > > auto hex =
> > > > > > > > As<PythonCallable>(builtins.get().GetAttribute("hex"));
> > > > > > > > ASSERT_THAT_EXPECTED(hex, llvm::Succeeded());
> > > > > > > > EXPECT_EQ(hex.get().GetNumArguments().count, 1u);
> > > > > > > > ```
> > > > > > > >
> > > > > > > > we should really re-write it to use inspect.signature.
> > > > > > > Interesting. We use it for free functions (what you pass to the
> > > > > > > -F option of `breakpoint command add`) and for the __init__ and
> > > > > > > __call__ method in the little classes you can make up for
> > > > > > > scripted thread plans and for the class version of Python
> > > > > > > implemented command-line commands. We have tests for telling 3
> > > > > > > vrs. 4 vrs. not enough or too many, and they all pass. So it
> > > > > > > seems to work in the cases we currently need it to work for...
> > > > > > >
> > > > > > > "inspect.signature" is python 3 only, and the python 2 equivalent
> > > > > > > is deprecated. So it will take a little fancy footwork to use it
> > > > > > > in the transition period.
> > > > > > lol :)
> > > > > >
> > > > > > I would actually say that we should try not to use this
> > > > > > function(ality) wherever possible. Making decisions based on the
> > > > > > number of arguments the thing you're about to call takes sounds
> > > > > > weird. I don't want to get too involved in this, but I was
> > > > > > designing this, I'd just say that if one tries to pass arguments to
> > > > > > the callback then the callback MUST take three arguments (or we'll
> > > > > > abort processing the breakpoint command). If he wants his function
> > > > > > to be called both with arguments and without, he can just add a
> > > > > > default value to the third argument. (And if his function takes two
> > > > > > arguments, but he still tries to pass something... well, it's his
> > > > > > own fault).
> > > > > >
> > > > > > Anyway, feel free to ignore this comment, but I felt like I had to
> > > > > > say something. :)
> > > > > I completely agree with Pavel. Inspecting a function signature
> > > > > before calling it is a big code smell in python. If there's a way
> > > > > to avoid doing that introspection, that would be better.
> > > > Unfortunately, we originally designed this interface to take three
> > > > arguments, the frame pointer, the breakpoint location and the Python
> > > > session dict. Then it became clear that it would be better to add this
> > > > extra args argument (and in the case of Python based commands the
> > > > ExecutionContext pointer). At that point we had three choices, abandon
> > > > the improvement; switch to unconditionally passing the extra arguments,
> > > > and break everybody's extant uses; or switch off of the number of
> > > > arguments to decide whether the user had provided the old or new forms.
> > > >
> > > > My feeling about lldb Python scripts/commands etc. that people use in
> > > > the debugger is that a lot of users don't know how they work at all,
> > > > they just got them from somebody else; and many more figured out how to
> > > > write them for some specific purpose, and then pretty much forgot how
> > > > they worked. So suddenly breaking all these bits of functionality will
> > > > result in folks just deciding that this affordance is not reliable and
> > > > not worth their time, which would be a shame.
> > > >
> > > > So instead we accommodate both forms, which requires that we know which
> > > > one the user provided. If you see a better way to do this, (and are
> > > > willing to implement it because so far as I can see this method is
> > > > going to work just fine) dig in, I'm not wedded to the particular
> > > > approach. But I am not excited about penalizing our users because we
> > > > didn't get the API design right the first time through.
> > > makes sense.
> > >
> > > The only other way I can think of to solve it would be to have some
> > > indication in the `break com add` command of what signature it expects
> > > from the function. But that's really ugly too because now you're asking
> > > users to understand yet another option.
> > >
> > > I put up https://reviews.llvm.org/D68995 this morning which adds
> > > `inspect.signature` support for pythons that have it.
> > >
> > > Currently we have really gnarly ArgInfo tests like this:
> > > ```
> > > if (argc.count == 5 || argc.is_bound_method || argc.has_varargs)
> > > pfunc(debugger_arg, PythonString(args), exe_ctx_arg,
> > > cmd_retobj_arg, dict);
> > > else
> > > pfunc(debugger_arg, PythonString(args), cmd_retobj_arg, dict);
> > > ```
> > >
> > > 😖
> > >
> > > I think if we replace `count` with `max_positional_args` we should be
> > > able to replace that kindof test with just
> > > ```
> > > if (argc.max_positional_args < 5)
> > > old_version
> > > else
> > > new_version
> > > ```
> > >
> > >
> > So, how about this: Put extra_args as the last argument, instead of
> > inserting it in the middle (so the new signature becomes `frame, bp_loc,
> > dict, extra_args` instead of `frame, bp_loc, extra_args, dict`. Then
> > instead of
> > ```
> > if (arg_info.count == 3)
> > result = pfunc(frame_arg, bp_loc_arg, dict);
> > else if (arg_info.count == 4) {
> > lldb::SBStructuredData *args_value = new
> > lldb::SBStructuredData(args_impl);
> > PythonObject args_arg(PyRefType::Owned,
> > SBTypeToSWIGWrapper(args_value));
> > result = pfunc(frame_arg, bp_loc_arg, args_arg, dict);
> > ```
> > we do:
> > ```
> > if (args_impl.was_specified())
> > pfunc(frame_arg, bp_loc_arg, dict, args_arg)
> > else
> > pfunc(frame_arg, bp_loc_arg, dict);
> > ```
> > All existing scripts will not specify the extra arguments, so they will
> > work as usual. New scripts which do pass extra arguments will have to use
> > the new signature. New scripts can also put `args = None` in their python
> > signature, so that they are callable both with and without arguments,
> > should they so desire. (If we don't want to support the `=None` use case
> > then we can even keep the arguments in the same order as in this patch.)
> >
> > Is there some reason why that would not work?
> > The only other way I can think of to solve it would be to have some
> > indication in the break com add command of what signature it expects from
> > the function.
>
> That's kind of what I'm getting at. I am hoping that the presence of the
> `--key`, `--value` options can be used as an indicator of that signature.
> (Though maybe I am misunderstanding something and I should shut up.)
The is_bound_method test is cheesy. We didn't offer a "class with __call__"
for Python based commands until after we added the ExecutionContext argument,
so this check knows that if you are providing a class method, then you are
probably also providing the correct number of arguments. max_positional_args
seems a more explicit approach.
I think the varargs check is to allow you to write a command that takes the old
three arguments and the ExecutionContext as a vararg, so the same Python
function could work with an older lldb that didn't send the exe_ctx but take
advantage of the better interface if it was present. After all, the fallback
of using the currently selected "target/process/thread/frame" inside the
function will mostly work.
For the affordances taking the "extra_args", like breakpoint commands and
scripted breakpoints and the like, it's hard to see how you could have a
reasonable fallback to "you didn't tell me what function to look for..." So
for these I want to count the fixed arguments, I don't think it is necessary to
allow them to be passed as varargs.
Thanks for fixing up the PythonObjects code, BTW!
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68671/new/
https://reviews.llvm.org/D68671
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
