lawrence_danna created this revision.
lawrence_danna added reviewers: JDevlieghere, clayborg, labath, jingham.
Herald added a project: LLDB.
The current implementation of PythonCallable::GetNumArguments
is not exception safe, has weird semantics, and is just plain
incorrect for some kinds of functions.
Python 3.3 introduces inspect.signature, which lets us easily
query for function signatures in a sane and documented way.
This patch leaves the old implementation in place for < 3.3,
but uses inspect.signature for modern pythons. It also leaves
the old weird semantics in place, but with FIXMEs grousing about
it. We should update the callers and fix the semantics in a
subsequent patch. It also adds some tests.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D68995
Files:
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
@@ -20,6 +20,7 @@
#include "PythonTestSuite.h"
using namespace lldb_private;
+using namespace lldb_private::python;
class PythonDataObjectsTest : public PythonTestSuite {
public:
@@ -626,3 +627,92 @@
}
}
}
+
+TEST_F(PythonDataObjectsTest, TestCallable) {
+
+ PythonDictionary globals(PyInitialValue::Empty);
+ auto builtins = PythonModule::BuiltinsModule();
+ llvm::Error error = globals.SetItem("__builtins__", builtins);
+ ASSERT_FALSE(error);
+
+ {
+ 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().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().has_varargs, false);
+ EXPECT_EQ(arginfo.get().is_bound_method, 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().has_varargs, true);
+ EXPECT_EQ(arginfo.get().is_bound_method, false);
+ }
+
+ {
+ const char *script = "class Foo: \n"
+ " def bar(self, x):\n"
+ " return x \n"
+ "bar_bound = Foo().bar \n"
+ "bar_unbound = Foo.bar \n";
+ 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().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().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().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
@@ -309,6 +309,10 @@
static llvm::Error exception(const char *s = nullptr) {
return llvm::make_error<PythonException>(s);
}
+ static llvm::Error keyError() {
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "key not in dict");
+ }
public:
template <typename... T>
@@ -328,6 +332,21 @@
return python::Take<PythonObject>(obj);
}
+ template <typename... T>
+ llvm::Expected<PythonObject> Call(const T &... t) const {
+ const char format[] = {'(', PythonFormat<T>::format..., ')', 0};
+#if PY_MAJOR_VERSION < 3
+ PyObject *obj = PyObject_CallFunction(m_py_obj, const_cast<char *>(format),
+ PythonFormat<T>::get(t)...);
+#else
+ PyObject *obj =
+ PyObject_CallFunction(m_py_obj, format, PythonFormat<T>::get(t)...);
+#endif
+ if (!obj)
+ return exception();
+ return python::Take<PythonObject>(obj);
+ }
+
llvm::Expected<PythonObject> GetAttribute(const char *name) const {
if (!m_py_obj)
return nullDeref();
@@ -386,6 +405,9 @@
template <>
llvm::Expected<long long> As<long long>(llvm::Expected<PythonObject> &&obj);
+template <>
+llvm::Expected<std::string> As<std::string>(llvm::Expected<PythonObject> &&obj);
+
} // namespace python
template <class T> class TypedPythonObject : public PythonObject {
@@ -559,8 +581,14 @@
PythonList GetKeys() const;
- PythonObject GetItemForKey(const PythonObject &key) const;
- void SetItemForKey(const PythonObject &key, const PythonObject &value);
+ PythonObject GetItemForKey(const PythonObject &key) const; // DEPRECATED
+ void SetItemForKey(const PythonObject &key,
+ const PythonObject &value); // DEPRECATED
+
+ llvm::Expected<PythonObject> GetItem(const PythonObject &key) const;
+ llvm::Expected<PythonObject> GetItem(const char *key) const;
+ llvm::Error SetItem(const PythonObject &key, const PythonObject &value) const;
+ llvm::Error SetItem(const char *key, const PythonObject &value) const;
StructuredData::DictionarySP CreateStructuredDictionary() const;
};
@@ -600,19 +628,31 @@
using TypedPythonObject::TypedPythonObject;
struct ArgInfo {
- size_t count;
- bool is_bound_method : 1;
- bool has_varargs : 1;
- bool has_kwargs : 1;
+ /* 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);
- ArgInfo GetNumArguments() const;
+ llvm::Expected<ArgInfo> GetArgInfo() const;
+
+ llvm::Expected<ArgInfo> GetInitArgInfo() const;
+
+ ArgInfo GetNumArguments() const; // DEPRECATED
// If the callable is a Py_Class, then find the number of arguments
// of the __init__ method.
- ArgInfo GetNumInitArguments() const;
+ ArgInfo GetNumInitArguments() const; // DEPRECATED
PythonObject operator()();
Index: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
===================================================================
--- lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
@@ -47,6 +47,20 @@
return obj.get().AsLongLong();
}
+template <>
+Expected<std::string> python::As<std::string>(Expected<PythonObject> &&obj) {
+ if (!obj)
+ return obj.takeError();
+ PyObject *str_obj = PyObject_Str(obj.get().get());
+ if (!obj)
+ return llvm::make_error<PythonException>();
+ auto str = Take<PythonString>(str_obj);
+ auto utf8 = str.AsUTF8();
+ if (!utf8)
+ return utf8.takeError();
+ return utf8.get();
+}
+
void StructuredPythonObject::Serialize(llvm::json::OStream &s) const {
s.value(llvm::formatv("Python Obj: {0:X}", GetValue()).str());
}
@@ -657,16 +671,62 @@
}
PythonObject PythonDictionary::GetItemForKey(const PythonObject &key) const {
- if (IsAllocated() && key.IsValid())
- return PythonObject(PyRefType::Borrowed,
- PyDict_GetItem(m_py_obj, key.get()));
- return PythonObject();
+ auto item = GetItem(key);
+ if (!item) {
+ llvm::consumeError(item.takeError());
+ return PythonObject();
+ }
+ return std::move(item.get());
+}
+
+Expected<PythonObject>
+PythonDictionary::GetItem(const PythonObject &key) const {
+ if (!IsValid())
+ return nullDeref();
+ PyObject *o = PyDict_GetItemWithError(m_py_obj, key.get());
+ if (PyErr_Occurred())
+ return exception();
+ if (!o)
+ return keyError();
+ return Retain<PythonObject>(o);
+}
+
+Expected<PythonObject> PythonDictionary::GetItem(const char *key) const {
+ if (!IsValid())
+ return nullDeref();
+ PyObject *o = PyDict_GetItemString(m_py_obj, key);
+ if (PyErr_Occurred())
+ return exception();
+ if (!o)
+ return keyError();
+ return Retain<PythonObject>(o);
+}
+
+Error PythonDictionary::SetItem(const PythonObject &key,
+ const PythonObject &value) const {
+ if (!IsValid() || !value.IsValid())
+ return nullDeref();
+ int r = PyDict_SetItem(m_py_obj, key.get(), value.get());
+ if (r < 0)
+ return exception();
+ return Error::success();
+}
+
+Error PythonDictionary::SetItem(const char *key,
+ const PythonObject &value) const {
+ if (!IsValid() || !value.IsValid())
+ return nullDeref();
+ int r = PyDict_SetItemString(m_py_obj, key, value.get());
+ if (r < 0)
+ return exception();
+ return Error::success();
}
void PythonDictionary::SetItemForKey(const PythonObject &key,
const PythonObject &value) {
- if (IsAllocated() && key.IsValid() && value.IsValid())
- PyDict_SetItem(m_py_obj, key.get(), value.get());
+ Error error = SetItem(key, value);
+ if (error)
+ llvm::consumeError(std::move(error));
}
StructuredData::DictionarySP
@@ -736,23 +796,97 @@
}
PythonCallable::ArgInfo PythonCallable::GetNumInitArguments() const {
- ArgInfo result = {0, false, false, false};
- if (!IsValid())
- return result;
-
- PythonObject __init__ = GetAttributeValue("__init__");
- if (__init__.IsValid() ) {
- auto __init_callable__ = __init__.AsType<PythonCallable>();
- if (__init_callable__.IsValid())
- return __init_callable__.GetNumArguments();
+ auto arginfo = GetInitArgInfo();
+ if (!arginfo) {
+ llvm::consumeError(arginfo.takeError());
+ return ArgInfo{};
}
- return result;
+ return arginfo.get();
}
-PythonCallable::ArgInfo PythonCallable::GetNumArguments() const {
- ArgInfo result = {0, false, false, false};
+Expected<PythonCallable::ArgInfo> PythonCallable::GetInitArgInfo() const {
if (!IsValid())
- return result;
+ return nullDeref();
+ auto init = As<PythonCallable>(GetAttribute("__init__"));
+ if (!init)
+ return init.takeError();
+ return init.get().GetArgInfo();
+}
+
+Expected<PythonCallable::ArgInfo> PythonCallable::GetArgInfo() const {
+ ArgInfo result = {};
+ if (!IsValid())
+ return nullDeref();
+
+#if PY_MAJOR_VERSION >= 3 && PY_MINOR_VERSION >= 3
+
+ const char *script =
+ "from inspect import signature, Parameter, ismethod \n"
+ "from collections import namedtuple \n"
+ "ArgInfo = namedtuple('ArgInfo', ['count', 'has_varargs', "
+ "'is_bound_method']) \n"
+ "def get_arg_info(f): \n"
+ " count = 0 \n"
+ " varargs = False \n"
+ " for parameter in signature(f).parameters.values(): \n"
+ " kind = parameter.kind \n"
+ " if kind in (Parameter.POSITIONAL_ONLY, \n"
+ " Parameter.POSITIONAL_OR_KEYWORD): \n"
+ " count += 1 \n"
+ " elif kind == Parameter.VAR_POSITIONAL: \n"
+ " varargs = True \n"
+ " elif kind in (Parameter.KEYWORD_ONLY, \n"
+ " Parameter.KEYWORD_ONLY): \n"
+ " pass \n"
+ " else: \n"
+ " raise Exception(f'unknown parameter kind: {kind}') \n"
+ " return ArgInfo(count, varargs, ismethod(f)) \n";
+
+ // this global is protected by the GIL
+ static PythonCallable get_arg_info;
+
+ if (!get_arg_info.IsValid()) {
+ PythonDictionary globals(PyInitialValue::Empty);
+
+ auto builtins = PythonModule::BuiltinsModule();
+ Error error = globals.SetItem("__builtins__", builtins);
+ if (error)
+ return std::move(error);
+ PyObject *o =
+ PyRun_String(script, Py_file_input, globals.get(), globals.get());
+ if (!o)
+ return exception();
+ Take<PythonObject>(o);
+ auto function = As<PythonCallable>(globals.GetItem("get_arg_info"));
+ if (!function)
+ return function.takeError();
+ get_arg_info = std::move(function.get());
+ }
+
+ Expected<PythonObject> pyarginfo = get_arg_info.Call(*this);
+ if (!pyarginfo)
+ return pyarginfo.takeError();
+ Expected<long long> count =
+ As<long long>(pyarginfo.get().GetAttribute("count"));
+ if (!count)
+ return count.takeError();
+ result.count = count.get();
+ Expected<bool> has_varargs =
+ As<bool>(pyarginfo.get().GetAttribute("has_varargs"));
+ if (!has_varargs)
+ return has_varargs.takeError();
+ result.has_varargs = has_varargs.get();
+ Expected<bool> is_method =
+ As<bool>(pyarginfo.get().GetAttribute("is_bound_method"));
+ if (!is_method)
+ return is_method.takeError();
+ result.is_bound_method = is_method.get();
+
+ // FIXME emulate old broken behavior
+ if (result.is_bound_method)
+ result.count++;
+
+#else
PyObject *py_func_obj = m_py_obj;
if (PyMethod_Check(py_func_obj)) {
@@ -785,10 +919,21 @@
result.count = code->co_argcount;
result.has_varargs = !!(code->co_flags & CO_VARARGS);
- result.has_kwargs = !!(code->co_flags & CO_VARKEYWORDS);
+
+#endif
+
return result;
}
+PythonCallable::ArgInfo PythonCallable::GetNumArguments() const {
+ auto arginfo = GetArgInfo();
+ if (!arginfo) {
+ llvm::consumeError(arginfo.takeError());
+ return ArgInfo{};
+ }
+ return arginfo.get();
+}
+
PythonObject PythonCallable::operator()() {
return PythonObject(PyRefType::Owned, PyObject_CallObject(m_py_obj, nullptr));
}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
