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

[lawrence_danna]

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.

[labath]

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.

[jingham]

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.

[lawrence_danna]

added tests for call objects, and a fix for python2

[lawrence_danna]

In D69014#1711400, @jingham wrote:

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]

review fixes

[lawrence_danna]

fixed

[lawrence_danna]

comment

[lawrence_danna]

rebased

[lawrence_danna]

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>

[labath]

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).

[lawrence_danna]

rebased

[lawrence_danna]

@jingham , @labath what do you think?

[labath]

I am happy with that.

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

 """
 Test lldb Python commands.
 """
 
 from __future__ import print_function
 
-
+import sys
 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 
 
 class CmdPythonTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
     NO_DEBUG_INFO_TESTCASE = True
 
     def test(self):
         self.build()
         self.pycmd_tests()
 
     def pycmd_tests(self):
         self.runCmd("command source py_import")
 
+        # Test a bunch of different kinds of python callables with
+        # both 4 and 5 positional arguments.
+        self.expect("foobar", substrs=["All good"])
+        self.expect("foobar4", substrs=["All good"])
+        self.expect("vfoobar", substrs=["All good"])
+        self.expect("v5foobar", substrs=["All good"])
+        self.expect("sfoobar", substrs=["All good"])
+        self.expect("cfoobar", substrs=["All good"])
+        self.expect("ifoobar", substrs=["All good"])
+        self.expect("sfoobar4", substrs=["All good"])
+        self.expect("cfoobar4", substrs=["All good"])
+        self.expect("ifoobar4", substrs=["All good"])
+        self.expect("ofoobar", substrs=["All good"])
+        self.expect("ofoobar4", substrs=["All good"])
+
         # Verify command that specifies eCommandRequiresTarget returns failure
         # without a target.
         self.expect('targetname',
                     substrs=['a.out'], matching=False, error=True)
 
         exe = self.getBuildArtifact("a.out")
         self.expect("file " + exe,
                     patterns=["Current executable set to .*a.out"])

lldb/packages/Python/lldbsuite/test/commands/command/script/callables.py

+
+from __future__ import print_function
+
+import lldb
+
+# bunch of different kinds of python callables that should
+# all work as commands.
+
+def check(debugger, command, context, result, internal_dict):
+    if (not isinstance(debugger, lldb.SBDebugger) or
+        not isinstance(command, str) or
+        not isinstance(result, lldb.SBCommandReturnObject) or
+        not isinstance(internal_dict, dict) or
+        (not context is None and
+        not isinstance(context, lldb.SBExecutionContext))):
+      raise Exception()
+    result.AppendMessage("All good.")
+
+def vfoobar(*args):
+    check(*args)
+
+def v5foobar(debugger, command, context, result, internal_dict, *args):
+    check(debugger, command, context, result, internal_dict)
+
+def foobar(debugger, command, context, result, internal_dict):
+    check(debugger, command, context, result, internal_dict)
+
+def foobar4(debugger, command, result, internal_dict):
+    check(debugger, command, None, result, internal_dict)
+
+class FooBar:
+    @staticmethod
+    def sfoobar(debugger, command, context, result, internal_dict):
+      check(debugger, command, context, result, internal_dict)
+
+    @classmethod
+    def cfoobar(cls, debugger, command, context, result, internal_dict):
+      check(debugger, command, context, result, internal_dict)
+
+    def ifoobar(self, debugger, command, context, result, internal_dict):
+      check(debugger, command, context, result, internal_dict)
+
+    def __call__(self, debugger, command, context, result, internal_dict):
+      check(debugger, command, context, result, internal_dict)
+
+    @staticmethod
+    def sfoobar4(debugger, command, result, internal_dict):
+      check(debugger, command, None, result, internal_dict)
+
+    @classmethod
+    def cfoobar4(cls, debugger, command, result, internal_dict):
+      check(debugger, command, None, result, internal_dict)
+
+    def ifoobar4(self, debugger, command, result, internal_dict):
+      check(debugger, command, None, result, internal_dict)
+
+class FooBar4:
+    def __call__(self, debugger, command, result, internal_dict):
+      check(debugger, command, None, result, internal_dict)
+
+FooBarObj = FooBar()
+
+FooBar4Obj = FooBar4()
 No newline at end of file

lldb/packages/Python/lldbsuite/test/commands/command/script/py_import

 script import sys, os
 script sys.path.append(os.path.join(os.getcwd(), os.pardir))
 script import welcome
 script import bug11569
 command script add welcome --class welcome.WelcomeCommand
 command script add targetname --class welcome.TargetnameCommand
 command script add longwait --function welcome.print_wait_impl
 command script import mysto.py --allow-reload
 command script add tell_sync --function welcome.check_for_synchro --synchronicity sync
 command script add tell_async --function welcome.check_for_synchro --synchronicity async
 command script add tell_curr --function welcome.check_for_synchro --synchronicity curr
 command script add takes_exe_ctx --function welcome.takes_exe_ctx
 command script import decorated.py
+
+
+command script import callables.py
+
+command script add -f callables.foobar foobar
+command script add -f callables.foobar4 foobar4
+command script add -f callables.vfoobar vfoobar
+command script add -f callables.v5foobar v5foobar
+
+command script add -f callables.FooBar.sfoobar sfoobar
+command script add -f callables.FooBar.cfoobar cfoobar
+command script add -f callables.FooBarObj.ifoobar ifoobar
+
+command script add -f callables.FooBar.sfoobar4 sfoobar4
+command script add -f callables.FooBar.cfoobar4 cfoobar4
+command script add -f callables.FooBarObj.ifoobar4 ifoobar4
+
+command script add -f callables.FooBarObj ofoobar
+command script add -f callables.FooBar4Obj ofoobar4

lldb/scripts/Python/python-wrapper.swig

)
     auto dict = PythonModule::MainModule().ResolveName<PythonDictionary>(session_dictionary_name);
     auto pfunc = PythonObject::ResolveNameWithDictionary<PythonCallable>(python_function_name, dict);
 
     if (!pfunc.IsAllocated())
         return false;
 
     // pass the pointer-to cmd_retobj_sb or watch the underlying object disappear from under you
     // see comment above for SBCommandReturnObjectReleaser for further details
-    auto argc = pfunc.GetNumArguments();
+    auto argc = pfunc.GetArgInfo();
+    if (!argc) {
+        llvm::consumeError(argc.takeError());
+        return false;
+    }
     PythonObject debugger_arg(PyRefType::Owned, SBTypeToSWIGWrapper(debugger_sb));
     PythonObject exe_ctx_arg(PyRefType::Owned, SBTypeToSWIGWrapper(exe_ctx_sb));
     PythonObject cmd_retobj_arg(PyRefType::Owned, SBTypeToSWIGWrapper(&cmd_retobj_sb));
 
-    if (argc.count == 5 || argc.is_bound_method || argc.has_varargs)
-        pfunc(debugger_arg, PythonString(args), exe_ctx_arg, cmd_retobj_arg, dict);
-    else
+    if (argc.get().max_positional_args < 5u)
         pfunc(debugger_arg, PythonString(args), cmd_retobj_arg, dict);
+    else
+        pfunc(debugger_arg, PythonString(args), exe_ctx_arg, cmd_retobj_arg, dict);
 
     return true;
 }
 
 SWIGEXPORT bool
 LLDBSwigPythonCallCommandObject
 (
     PyObject *implementor,

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

public:
   PythonDictionary GetDictionary() const;
 };
 
 class PythonCallable : public TypedPythonObject<PythonCallable> {
 public:
   using TypedPythonObject::TypedPythonObject;
 
   struct ArgInfo {
+    /* the largest number of positional arguments this callable
+     * can accept, or UNBOUNDED, ie UINT_MAX if it's a varargs
+     * function and can accept an arbitrary number */
+    unsigned max_positional_args;
+    static constexpr unsigned UNBOUNDED = UINT_MAX; // FIXME c++17 inline
     /* the number of positional arguments, including optional ones,
      * and excluding varargs.  If this is a bound method, then the
      * count will still include a +1 for self.
      *
      * FIXME. That's crazy.  This should be replaced with
      * an accurate min and max for positional args.
      */
     int count;
     /* does the callable have positional varargs? */
     bool has_varargs : 1; // FIXME delete this
-    /* is the callable a bound method written in python? */
-    bool is_bound_method : 1; // FIXME delete this
   };
 
   static bool Check(PyObject *py_obj);
 
   llvm::Expected<ArgInfo> GetArgInfo() const;
 
   llvm::Expected<ArgInfo> GetInitArgInfo() const;
 

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp

#if PY_MAJOR_VERSION >= 3 && PY_MINOR_VERSION >= 3
   }
 
   Expected<PythonObject> pyarginfo = get_arg_info.Call(*this);
   if (!pyarginfo)
     return pyarginfo.takeError();
   result.count = cantFail(As<long long>(pyarginfo.get().GetAttribute("count")));
   result.has_varargs =
       cantFail(As<bool>(pyarginfo.get().GetAttribute("has_varargs")));
-  result.is_bound_method =
+  bool is_method =
       cantFail(As<bool>(pyarginfo.get().GetAttribute("is_bound_method")));
+  result.max_positional_args =
+      result.has_varargs ? ArgInfo::UNBOUNDED : result.count;
 
   // FIXME emulate old broken behavior
-  if (result.is_bound_method)
+  if (is_method)
     result.count++;
 
 #else
-
+  bool is_bound_method = false;
   PyObject *py_func_obj = m_py_obj;
   if (PyMethod_Check(py_func_obj)) {
     py_func_obj = PyMethod_GET_FUNCTION(py_func_obj);
     PythonObject im_self = GetAttributeValue("im_self");
     if (im_self.IsValid() && !im_self.IsNone())
-      result.is_bound_method = true;
+      is_bound_method = true;
   } else {
     // see if this is a callable object with an __call__ method
     if (!PyFunction_Check(py_func_obj)) {
       PythonObject __call__ = GetAttributeValue("__call__");
       if (__call__.IsValid()) {
         auto __callable__ = __call__.AsType<PythonCallable>();
         if (__callable__.IsValid()) {
           py_func_obj = PyMethod_GET_FUNCTION(__callable__.get());
-          PythonObject im_self = GetAttributeValue("im_self");
+          PythonObject im_self = __callable__.GetAttributeValue("im_self");
           if (im_self.IsValid() && !im_self.IsNone())
-            result.is_bound_method = true;
+            is_bound_method = true;
         }
       }
     }
   }
 
   if (!py_func_obj)
     return result;
 
   PyCodeObject *code = (PyCodeObject *)PyFunction_GET_CODE(py_func_obj);
   if (!code)
     return result;
 
   result.count = code->co_argcount;
   result.has_varargs = !!(code->co_flags & CO_VARARGS);
+  result.max_positional_args = result.has_varargs
+                                   ? ArgInfo::UNBOUNDED
+                                   : (result.count - (int)is_bound_method);
 
 #endif
 
   return result;
 }
 
+constexpr unsigned
+    PythonCallable::ArgInfo::UNBOUNDED; // FIXME delete after c++17
+
 PythonCallable::ArgInfo PythonCallable::GetNumArguments() const {
   auto arginfo = GetArgInfo();
   if (!arginfo) {
     llvm::consumeError(arginfo.takeError());
     return ArgInfo{};
   }
   return arginfo.get();
 }

lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

TEST_F(PythonDataObjectsTest, TestCallable) {
   {
     PyObject *o = PyRun_String("lambda x : x", Py_eval_input, globals.get(),
                                globals.get());
     ASSERT_FALSE(o == NULL);
     auto lambda = Take<PythonCallable>(o);
     auto arginfo = lambda.GetArgInfo();
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
     EXPECT_EQ(arginfo.get().count, 1);
+    EXPECT_EQ(arginfo.get().max_positional_args, 1u);
     EXPECT_EQ(arginfo.get().has_varargs, false);
-    EXPECT_EQ(arginfo.get().is_bound_method, false);
   }
 
   {
     PyObject *o = PyRun_String("lambda x,y=0: x", Py_eval_input, globals.get(),
                                globals.get());
     ASSERT_FALSE(o == NULL);
     auto lambda = Take<PythonCallable>(o);
     auto arginfo = lambda.GetArgInfo();
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
     EXPECT_EQ(arginfo.get().count, 2);
+    EXPECT_EQ(arginfo.get().max_positional_args, 2u);
     EXPECT_EQ(arginfo.get().has_varargs, false);
-    EXPECT_EQ(arginfo.get().is_bound_method, false);
   }
 
   {
     PyObject *o = PyRun_String("lambda x,y=0, **kw: x", Py_eval_input,
                                globals.get(), globals.get());
     ASSERT_FALSE(o == NULL);
     auto lambda = Take<PythonCallable>(o);
     auto arginfo = lambda.GetArgInfo();
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
     EXPECT_EQ(arginfo.get().count, 2);
+    EXPECT_EQ(arginfo.get().max_positional_args, 2u);
     EXPECT_EQ(arginfo.get().has_varargs, false);
   }
 
   {
     PyObject *o = PyRun_String("lambda x,y,*a: x", Py_eval_input, globals.get(),
                                globals.get());
     ASSERT_FALSE(o == NULL);
     auto lambda = Take<PythonCallable>(o);
     auto arginfo = lambda.GetArgInfo();
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
     EXPECT_EQ(arginfo.get().count, 2);
+    EXPECT_EQ(arginfo.get().max_positional_args,
+              PythonCallable::ArgInfo::UNBOUNDED);
     EXPECT_EQ(arginfo.get().has_varargs, true);
-    EXPECT_EQ(arginfo.get().is_bound_method, false);
   }
 
   {
     PyObject *o = PyRun_String("lambda x,y,*a,**kw: x", Py_eval_input,
                                globals.get(), globals.get());
     ASSERT_FALSE(o == NULL);
     auto lambda = Take<PythonCallable>(o);
     auto arginfo = lambda.GetArgInfo();
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
     EXPECT_EQ(arginfo.get().count, 2);
+    EXPECT_EQ(arginfo.get().max_positional_args,
+              PythonCallable::ArgInfo::UNBOUNDED);
     EXPECT_EQ(arginfo.get().has_varargs, true);
   }
 
   {
     const char *script = R"(
 class Foo:
   def bar(self, x):
      return x
+  @classmethod
+  def classbar(cls, x):
+     return x
+  @staticmethod
+  def staticbar(x):
+     return x
+  def __call__(self, x):
+     return x
+obj = Foo()
 bar_bound   = Foo().bar
+bar_class   = Foo().classbar
+bar_static  = Foo().staticbar
 bar_unbound = Foo.bar
 )";
     PyObject *o =
         PyRun_String(script, Py_file_input, globals.get(), globals.get());
     ASSERT_FALSE(o == NULL);
     Take<PythonObject>(o);
 
     auto bar_bound = As<PythonCallable>(globals.GetItem("bar_bound"));
     ASSERT_THAT_EXPECTED(bar_bound, llvm::Succeeded());
     auto arginfo = bar_bound.get().GetArgInfo();
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
     EXPECT_EQ(arginfo.get().count, 2); // FIXME, wrong
+    EXPECT_EQ(arginfo.get().max_positional_args, 1u);
     EXPECT_EQ(arginfo.get().has_varargs, false);
-    EXPECT_EQ(arginfo.get().is_bound_method, true);
 
     auto bar_unbound = As<PythonCallable>(globals.GetItem("bar_unbound"));
     ASSERT_THAT_EXPECTED(bar_unbound, llvm::Succeeded());
     arginfo = bar_unbound.get().GetArgInfo();
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
     EXPECT_EQ(arginfo.get().count, 2);
+    EXPECT_EQ(arginfo.get().max_positional_args, 2u);
+    EXPECT_EQ(arginfo.get().has_varargs, false);
+
+    auto bar_class = As<PythonCallable>(globals.GetItem("bar_class"));
+    ASSERT_THAT_EXPECTED(bar_class, llvm::Succeeded());
+    arginfo = bar_class.get().GetArgInfo();
+    ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
+    EXPECT_EQ(arginfo.get().max_positional_args, 1u);
+    EXPECT_EQ(arginfo.get().has_varargs, false);
+
+    auto bar_static = As<PythonCallable>(globals.GetItem("bar_static"));
+    ASSERT_THAT_EXPECTED(bar_static, llvm::Succeeded());
+    arginfo = bar_static.get().GetArgInfo();
+    ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
+    EXPECT_EQ(arginfo.get().max_positional_args, 1u);
+    EXPECT_EQ(arginfo.get().has_varargs, false);
+
+    auto obj = As<PythonCallable>(globals.GetItem("obj"));
+    ASSERT_THAT_EXPECTED(obj, llvm::Succeeded());
+    arginfo = obj.get().GetArgInfo();
+    ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
+    EXPECT_EQ(arginfo.get().max_positional_args, 1u);
     EXPECT_EQ(arginfo.get().has_varargs, false);
-    EXPECT_EQ(arginfo.get().is_bound_method, false);
   }
 
 #if PY_MAJOR_VERSION >= 3 && PY_MINOR_VERSION >= 3
 
   // the old implementation of GetArgInfo just doesn't work on builtins.
 
   {
     auto builtins = PythonModule::BuiltinsModule();
     auto hex = As<PythonCallable>(builtins.GetAttribute("hex"));
     ASSERT_THAT_EXPECTED(hex, llvm::Succeeded());
     auto arginfo = hex.get().GetArgInfo();
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
     EXPECT_EQ(arginfo.get().count, 1);
+    EXPECT_EQ(arginfo.get().max_positional_args, 1u);
     EXPECT_EQ(arginfo.get().has_varargs, false);
-    EXPECT_EQ(arginfo.get().is_bound_method, false);
   }
 
 #endif
 }
 No newline at end of file