Page MenuHomePhabricator

[LLDB] bugfix: command script add -f doesn't work for some callables
ClosedPublic

Authored by lawrence_danna on Oct 15 2019, 5:43 PM.

Details

Summary

When users define a debugger command from python, they provide a callable
object. Because the signature of the function has been extended, LLDB
needs to inspect the number of parameters the callable can take.

The rule it was using to decide was weird, apparently not tested, and
giving wrong results for some kinds of python callables.

This patch replaces the weird rule with a simple one: if the callable can
take 5 arguments, it gets the 5 argument version of the signature.
Otherwise it gets the old 4 argument version.

It also adds tests with a bunch of different kinds of python callables
with both 4 and 5 arguments.

Diff Detail

Event Timeline

lawrence_danna created this revision.Oct 15 2019, 5:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2019, 5:43 PM

The code itself looks fine to me. Jim, is this ok from a scripting/compatibility/whatnot perspective?

lldb/packages/Python/lldbsuite/test/commands/command/script/TestCommandScript.py
25

Do we have to worry about 3.0<=python<=3.3 actually? Unlike 2.7, these have actually been EOLed already, and I would expect/hope that anyone who is not stuck at 2.7 has upgraded past them..

lldb/scripts/Python/python-wrapper.swig
701–702

There is a possibility to shorten this to return errorToBool(argc.takeError()) though I am undecided whether that makes things clearer or not, so I am mainly writing this to make you aware of that possibility. I'll leave that up to you to decide whether to use it...

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
610

Make this unsigned, and add a symbolic constant (static constexpr unsigned) for the "varargs" value.

I've also considered making the vararg-ness more explicit via Optional<unsigned> but (U)INTMAX is actually a pretty good value for the varargs and it enables you to handle this case with a single comparison, so the explicitness is probably not worth it.

IIUC, the only external change in this patch is that before if you implemented your Python command using a class with an __call__ method, it would have to be the signature that took exe_ctx. Since this is now switching off of the number of arguments (less self) you could make an __call__ form that didn't take the extra argument. I certainly want to discourage that, since the form without the exe_ctx will do the wrong thing in some contexts (e.g. if the command is used in breakpoint callbacks or stop-hooks). It looks like that would be easy to prevent, you know that you've found call right above where you check. So I think it would be better to explicitly disallow callforms that don't take the exe_ctx.

This is different from the extra_args cases in the breakpoint callbacks and thread plans and so forth. In those cases, if you don't need extra_args, there's no good reason why you should have to have them. But in the case of Python commands, it's actually more correct to get the state you are operating on from the exe_ctx. Otherwise you have to fall back on the currently selected process/thread/frame/etc, and there are contexts (like when you have multiple targets all hitting breakpoints simultaneously) where it does not make sense to have the currently selected state track what process/thread/frame are pertinent to the operation of a breakpoint command or stop hook. So I do want to gently discourage this form of command.

added tests for call objects, and a fix for python2

lawrence_danna added a comment.EditedOct 16 2019, 12:36 PM

IIUC, the only external change in this patch is that before if you implemented your Python command using a class with an __call__ method, it would have to be the signature that took exe_ctx. Since this is now switching off of the number of arguments (less self) you could make an __call__ form that didn't take the extra argument. I certainly want to discourage that, since the form without the exe_ctx will do the wrong thing in some contexts (e.g. if the command is used in breakpoint callbacks or stop-hooks). It looks like that would be easy to prevent, you know that you've found call right above where you check. So I think it would be better to explicitly disallow callforms that don't take the exe_ctx.

This is different from the extra_args cases in the breakpoint callbacks and thread plans and so forth. In those cases, if you don't need extra_args, there's no good reason why you should have to have them. But in the case of Python commands, it's actually more correct to get the state you are operating on from the exe_ctx. Otherwise you have to fall back on the currently selected process/thread/frame/etc, and there are contexts (like when you have multiple targets all hitting breakpoints simultaneously) where it does not make sense to have the currently selected state track what process/thread/frame are pertinent to the operation of a breakpoint command or stop hook. So I do want to gently discourage this form of command.

Right now what happens on trunk is that commands implemented with __call__ just don't work at all on any version of python with either 4 or 5 arguments.

If you look at my latest update, you can see I fix a bug in the old implementation GetNumArguments where it looks for im_self on the original object, not the __call__. I fixed it to look at the __call__ attribute, but that is not even entirely correct either, because the python data model states that __call__ is one of the magic methods that must be set on the class, and cannot be overridden for individual objects. Oh wait, no, that was the python 2.7 data model, it looks like they changed it in 3 so foo() really is synonymous with foo.__call__()

My point here is that python callables are complicated. They are code. Their implementation is liable to change, both because Python itself will change, and because users will change what they put in them. I understand the backwards-compatibility argument for why we should inspect them, but even so, we should inspect them as little as possible. Ideally the only inspection we'll ever do is to ask "can this callable take N arguments?". And when we ask that question we should ask it in the documented way, not by peeking inside method wrappers and python code objects and trying to understand them.

lawrence_danna marked 3 inline comments as done.

review fixes

Harbormaster completed remote builds in B39674: Diff 225297.

Also, @jingham, only the #if branch for python2 has easy access to the __call__ attribute. We could test for it in python3 but it wouldn't do what you're thinking it would:

In [1]: def f(): 
   ...:     return 1 
   ...:                                                                                                                                                                                                                  

In [2]: f.__call__                                                                                                                                                                                                       
Out[2]: <method-wrapper '__call__' of function object at 0x102e62e18>

In [3]: type(f).__call__                                                                                                                                                                                                 
Out[3]: <slot wrapper '__call__' of 'function' objects>

I also think we shouldn't go out of our way (and I consider any kind of introspection as "going out of our way") to forbid calling with a "deprecated" signature for some callables even though there's technically no backward compatibility to worry about (because it was not possible to call those callables in the past). For "gentle discouragement" I'd rather just print a warning whenever we call any callable with a deprecated signature. That warning can explain the problem and how to fix it (and it's strength can be ramped up over time if we want to eventually remove these).

labath accepted this revision.Oct 18 2019, 11:45 PM

I am happy with that.

This revision is now accepted and ready to land.Oct 18 2019, 11:45 PM
This revision was automatically updated to reflect the committed changes.