labath added inline comments.
================
Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+
+ virtual int GetNumArgumentsForCallable(const char *callable_name) {
+ return -1;
----------------
jingham wrote:
> 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?
What I find weird is that the meaning of a positional argument changes
depending on what comes after it. So if I define a function like `def
foo(a,b,c)`, then `c` will be a session dictionary. But if I define it as `def
foo(a,b,c,d):` then the dict becomes `d`. I don't find the `=None` confusing,
particularly as that means we could just have one function signature we
advertise, and if the users write it that way then it will "happen" to work on
older lldb's too. But then again, I've never written any breakpoint callbacks,
so if you think that these concerns are unfounded, then feel free to go ahead.
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
