lawrence_danna created this revision.
lawrence_danna added reviewers: JDevlieghere, clayborg, labath, jingham.
Herald added a project: LLDB.
lawrence_danna added a parent revision: D68995: clean up the implementation of
PythonCallable::GetNumArguments.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D69014
Files:
lldb/packages/Python/lldbsuite/test/commands/command/script/TestCommandScript.py
lldb/packages/Python/lldbsuite/test/commands/command/script/callables.py
lldb/packages/Python/lldbsuite/test/commands/command/script/py_import
lldb/scripts/Python/python-wrapper.swig
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
Index: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
===================================================================
--- lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
@@ -643,8 +643,8 @@
auto arginfo = lambda.GetArgInfo();
ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
EXPECT_EQ(arginfo.get().count, 1);
+ EXPECT_EQ(arginfo.get().max_positional_args, 1);
EXPECT_EQ(arginfo.get().has_varargs, false);
- EXPECT_EQ(arginfo.get().is_bound_method, false);
}
{
@@ -655,8 +655,8 @@
auto arginfo = lambda.GetArgInfo();
ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
EXPECT_EQ(arginfo.get().count, 2);
+ EXPECT_EQ(arginfo.get().max_positional_args, 2);
EXPECT_EQ(arginfo.get().has_varargs, false);
- EXPECT_EQ(arginfo.get().is_bound_method, false);
}
{
@@ -667,8 +667,8 @@
auto arginfo = lambda.GetArgInfo();
ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
EXPECT_EQ(arginfo.get().count, 2);
+ EXPECT_EQ(arginfo.get().max_positional_args, INT_MAX);
EXPECT_EQ(arginfo.get().has_varargs, true);
- EXPECT_EQ(arginfo.get().is_bound_method, false);
}
{
@@ -687,16 +687,16 @@
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, 1); // FIXME, wrong
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, 2);
EXPECT_EQ(arginfo.get().has_varargs, false);
- EXPECT_EQ(arginfo.get().is_bound_method, false);
}
#if PY_MAJOR_VERSION >= 3 && PY_MINOR_VERSION >= 3
@@ -710,9 +710,9 @@
auto arginfo = hex.get().GetArgInfo();
ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
EXPECT_EQ(arginfo.get().count, 1);
+ EXPECT_EQ(arginfo.get().max_positional_args, 1);
EXPECT_EQ(arginfo.get().has_varargs, false);
- EXPECT_EQ(arginfo.get().is_bound_method, false);
}
#endif
}
\ No newline at end of file
Index: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
===================================================================
--- lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
@@ -628,6 +628,10 @@
using TypedPythonObject::TypedPythonObject;
struct ArgInfo {
+ /* the largest number of positional arguments this callable
+ * can accept, or INT_MAX if it's a varargs function and can
+ * accept an arbitrary number */
+ int max_positional_args;
/* 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.
@@ -638,8 +642,6 @@
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);
Index: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
===================================================================
--- lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
@@ -884,20 +884,22 @@
As<bool>(pyarginfo.get().GetAttribute("is_bound_method"));
if (!is_method)
return is_method.takeError();
- result.is_bound_method = is_method.get();
+
+ result.max_positional_args = result.has_varargs ? INT_MAX : result.count;
// FIXME emulate old broken behavior
- if (result.is_bound_method)
+ if (is_method.get())
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)) {
@@ -908,7 +910,7 @@
py_func_obj = PyMethod_GET_FUNCTION(__callable__.get());
PythonObject im_self = GetAttributeValue("im_self");
if (im_self.IsValid() && !im_self.IsNone())
- result.is_bound_method = true;
+ is_bound_method = true;
}
}
}
@@ -924,6 +926,9 @@
result.count = code->co_argcount;
result.has_varargs = !!(code->co_flags & CO_VARARGS);
+ result.max_positional_args = result.has_varargs ? INT_MAX : result.count - (int)is_bound_method;
+
+
#endif
return result;
Index: lldb/scripts/Python/python-wrapper.swig
===================================================================
--- lldb/scripts/Python/python-wrapper.swig
+++ lldb/scripts/Python/python-wrapper.swig
@@ -696,15 +696,19 @@
// 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 < 5)
pfunc(debugger_arg, PythonString(args), cmd_retobj_arg, dict);
+ else
+ pfunc(debugger_arg, PythonString(args), exe_ctx_arg, cmd_retobj_arg, dict);
return true;
}
Index: lldb/packages/Python/lldbsuite/test/commands/command/script/py_import
===================================================================
--- lldb/packages/Python/lldbsuite/test/commands/command/script/py_import
+++ lldb/packages/Python/lldbsuite/test/commands/command/script/py_import
@@ -11,3 +11,19 @@
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
Index: lldb/packages/Python/lldbsuite/test/commands/command/script/callables.py
===================================================================
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/commands/command/script/callables.py
@@ -0,0 +1,56 @@
+
+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)
+
+ @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)
+
+
+FooBarObj = FooBar()
+
Index: lldb/packages/Python/lldbsuite/test/commands/command/script/TestCommandScript.py
===================================================================
--- lldb/packages/Python/lldbsuite/test/commands/command/script/TestCommandScript.py
+++ lldb/packages/Python/lldbsuite/test/commands/command/script/TestCommandScript.py
@@ -4,7 +4,7 @@
from __future__ import print_function
-
+import sys
import lldb
from lldbsuite.test.decorators import *
from lldbsuite.test.lldbtest import *
@@ -22,6 +22,26 @@
def pycmd_tests(self):
self.runCmd("command source py_import")
+ if sys.version_info[:2] >= (3, 3) or sys.version_info.major < 3:
+ # Test a bunch of different kinds of python callables with
+ # both 4 and 5 positional arguments.
+ #
+ # This uses inspect.signature to get the right answer, which
+ # was introduced in python 3.3
+ #
+ # The old method without inspect.signature seems to actually be
+ # correct for python 2
+ 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"])
+
# Verify command that specifies eCommandRequiresTarget returns failure
# without a target.
self.expect('targetname',
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
