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:
> jingham wrote:
> > jingham wrote:
> > > 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!
> > I did consider switching off of whether the user provided key & value
> > arguments. But that would mean that you could not write Python breakpoint
> > command functions that have default behaviors, but will do special things
> > if the user provided `--key` and `--value` arguments. That seemed like a
> > reasonable thing to support, so I switched to doing everything based on the
> > actual signature we were provided.
> Why not? If the last argument has a default value (`def
> my_fancy_callback(frame, bp_loc, dict, extra_args = None):`), then it should
> be callable both with three and four arguments, should it not? I think the
> `=None` part actually nicely documents that the callback is usable both with
> extra arguments, but also has a reasonable default behavior.
It's already weird that we have to tell people to pass the session dict. Now
we're saying you have to pass a defaulted arg after the dict; that seems even
weirder. I don't actually think lots of people are going to want to write
commands that take advantage of being passed extra arguments but want them to
work with older lldb's. The case I want to support is that you are always
going to get this extra args SBStructuredData, but it might be empty depending
on whether the user added --key and --value, and your command can do what it
wants based on the contents of the extra_args. That seems straightforwardly
supported by requiring the arguments.
I'm not sure we are solving any actual problem here?
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
