labath added inline comments.
================ Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469 + + virtual int GetNumArgumentsForCallable(const char *callable_name) { + return -1; ---------------- 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.) 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