diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h --- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h @@ -151,6 +151,30 @@ return std::move(thing); } +// This class can be used like a utility function to convert from +// a llvm-friendly Twine into a null-terminated const char *, +// which is the form python C APIs want their strings in. +// +// Example: +// const llvm::Twine &some_twine; +// PyFoo_Bar(x, y, z, NullTerminated(some_twine)); +// +// Why a class instead of a function? If the twine isn't already null +// terminated, it will need a temporary buffer to copy the string +// into. We need that buffer to stick around for the lifetime of the +// statement. +class NullTerminated { + const char *str; + llvm::SmallString<32> storage; + +public: + NullTerminated(const llvm::Twine &twine) { + llvm::StringRef ref = twine.toNullTerminatedStringRef(storage); + str = ref.begin(); + } + operator const char *() { return str; } +}; + } // namespace python enum class PyInitialValue { Invalid, Empty }; @@ -323,10 +347,11 @@ return python::Take(obj); } - llvm::Expected GetAttribute(const char *name) const { + llvm::Expected GetAttribute(const llvm::Twine &name) const { + using namespace python; if (!m_py_obj) return nullDeref(); - PyObject *obj = PyObject_GetAttrString(m_py_obj, name); + PyObject *obj = PyObject_GetAttrString(m_py_obj, NullTerminated(name)); if (!obj) return exception(); return python::Take(obj); @@ -392,10 +417,11 @@ // This can be eliminated once we drop python 2 support. static void Convert(PyRefType &type, PyObject *&py_obj) {} - using PythonObject::Reset; + void Reset() { PythonObject::Reset(); } - void Reset(PyRefType type, PyObject *py_obj) { - Reset(); + void Reset(PyRefType type, PyObject *py_obj) = delete; + + TypedPythonObject(PyRefType type, PyObject *py_obj) { if (!py_obj) return; T::Convert(type, py_obj); @@ -405,8 +431,6 @@ Py_DECREF(py_obj); } - TypedPythonObject(PyRefType type, PyObject *py_obj) { Reset(type, py_obj); } - TypedPythonObject() {} }; @@ -562,9 +586,9 @@ const PythonObject &value); // DEPRECATED llvm::Expected GetItem(const PythonObject &key) const; - llvm::Expected GetItem(const char *key) const; + llvm::Expected GetItem(const llvm::Twine &key) const; llvm::Error SetItem(const PythonObject &key, const PythonObject &value) const; - llvm::Error SetItem(const char *key, const PythonObject &value) const; + llvm::Error SetItem(const llvm::Twine &key, const PythonObject &value) const; StructuredData::DictionarySP CreateStructuredDictionary() const; }; @@ -592,9 +616,9 @@ return std::move(mod.get()); } - static llvm::Expected Import(const char *name); + static llvm::Expected Import(const llvm::Twine &name); - llvm::Expected Get(const char *name); + llvm::Expected Get(const llvm::Twine &name); PythonDictionary GetDictionary() const; }; @@ -708,6 +732,17 @@ return T(); } +namespace python { +// This is only here to help incrementally migrate old, exception-unsafe +// code. +template T unwrapIgnoringErrors(llvm::Expected expected) { + if (expected) + return std::move(expected.get()); + llvm::consumeError(expected.takeError()); + return T(); +} +} // namespace python + } // namespace lldb_private #endif diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp --- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp +++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp @@ -34,6 +34,7 @@ using llvm::cantFail; using llvm::Error; using llvm::Expected; +using llvm::Twine; template <> Expected python::As(Expected &&obj) { if (!obj) @@ -278,7 +279,7 @@ PythonByteArray::PythonByteArray(const uint8_t *bytes, size_t length) { const char *str = reinterpret_cast(bytes); - Reset(PyRefType::Owned, PyByteArray_FromStringAndSize(str, length)); + *this = Take(PyByteArray_FromStringAndSize(str, length)); } bool PythonByteArray::Check(PyObject *py_obj) { @@ -522,11 +523,11 @@ PythonList::PythonList(PyInitialValue value) { if (value == PyInitialValue::Empty) - Reset(PyRefType::Owned, PyList_New(0)); + *this = Take(PyList_New(0)); } PythonList::PythonList(int list_size) { - Reset(PyRefType::Owned, PyList_New(list_size)); + *this = Take(PyList_New(list_size)); } bool PythonList::Check(PyObject *py_obj) { @@ -578,11 +579,11 @@ PythonTuple::PythonTuple(PyInitialValue value) { if (value == PyInitialValue::Empty) - Reset(PyRefType::Owned, PyTuple_New(0)); + *this = Take(PyTuple_New(0)); } PythonTuple::PythonTuple(int tuple_size) { - Reset(PyRefType::Owned, PyTuple_New(tuple_size)); + *this = Take(PyTuple_New(tuple_size)); } PythonTuple::PythonTuple(std::initializer_list objects) { @@ -649,7 +650,7 @@ PythonDictionary::PythonDictionary(PyInitialValue value) { if (value == PyInitialValue::Empty) - Reset(PyRefType::Owned, PyDict_New()); + *this = Take(PyDict_New()); } bool PythonDictionary::Check(PyObject *py_obj) { @@ -696,10 +697,10 @@ return Retain(o); } -Expected PythonDictionary::GetItem(const char *key) const { +Expected PythonDictionary::GetItem(const Twine &key) const { if (!IsValid()) return nullDeref(); - PyObject *o = PyDict_GetItemString(m_py_obj, key); + PyObject *o = PyDict_GetItemString(m_py_obj, NullTerminated(key)); if (PyErr_Occurred()) return exception(); if (!o) @@ -717,11 +718,11 @@ return Error::success(); } -Error PythonDictionary::SetItem(const char *key, +Error PythonDictionary::SetItem(const Twine &key, const PythonObject &value) const { if (!IsValid() || !value.IsValid()) return nullDeref(); - int r = PyDict_SetItemString(m_py_obj, key, value.get()); + int r = PyDict_SetItemString(m_py_obj, NullTerminated(key), value.get()); if (r < 0) return exception(); return Error::success(); @@ -763,20 +764,20 @@ return PythonModule(PyRefType::Borrowed, PyImport_AddModule(str.c_str())); } -Expected PythonModule::Import(const char *name) { - PyObject *mod = PyImport_ImportModule(name); +Expected PythonModule::Import(const Twine &name) { + PyObject *mod = PyImport_ImportModule(NullTerminated(name)); if (!mod) return exception(); return Take(mod); } -Expected PythonModule::Get(const char *name) { +Expected PythonModule::Get(const Twine &name) { if (!IsValid()) return nullDeref(); PyObject *dict = PyModule_GetDict(m_py_obj); if (!dict) return exception(); - PyObject *item = PyDict_GetItemString(dict, name); + PyObject *item = PyDict_GetItemString(dict, NullTerminated(name)); if (!item) return exception(); return Retain(item); @@ -790,7 +791,9 @@ } PythonDictionary PythonModule::GetDictionary() const { - return PythonDictionary(PyRefType::Borrowed, PyModule_GetDict(m_py_obj)); + if (!IsValid()) + return PythonDictionary(); + return Retain(PyModule_GetDict(m_py_obj)); } bool PythonCallable::Check(PyObject *py_obj) { @@ -1092,6 +1095,8 @@ assert(m_py_obj); GIL takeGIL; Close(); + // we need to ensure the python object is released while we still + // hold the GIL m_py_obj.Reset(); } diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp --- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp +++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp @@ -55,6 +55,7 @@ using namespace lldb; using namespace lldb_private; +using namespace lldb_private::python; // Defined in the SWIG source file #if PY_MAJOR_VERSION >= 3 @@ -765,19 +766,16 @@ if (!main_dict.IsValid()) return m_session_dict; - PythonObject item = main_dict.GetItemForKey(PythonString(m_dictionary_name)); - m_session_dict.Reset(PyRefType::Borrowed, item.get()); + m_session_dict = unwrapIgnoringErrors( + As(main_dict.GetItem(m_dictionary_name))); return m_session_dict; } PythonDictionary &ScriptInterpreterPythonImpl::GetSysModuleDictionary() { if (m_sys_module_dict.IsValid()) return m_sys_module_dict; - - PythonObject sys_module(PyRefType::Borrowed, PyImport_AddModule("sys")); - if (sys_module.IsValid()) - m_sys_module_dict.Reset(PyRefType::Borrowed, - PyModule_GetDict(sys_module.get())); + PythonModule sys_module = unwrapIgnoringErrors(PythonModule::Import("sys")); + m_sys_module_dict = sys_module.GetDictionary(); return m_sys_module_dict; } @@ -1053,9 +1051,8 @@ PythonDictionary locals = GetSessionDictionary(); if (!locals.IsValid()) { - locals.Reset( - PyRefType::Owned, - PyObject_GetAttrString(globals.get(), m_dictionary_name.c_str())); + locals = unwrapIgnoringErrors( + As(globals.GetAttribute(m_dictionary_name))); } if (!locals.IsValid()) @@ -1204,9 +1201,8 @@ PythonDictionary locals = GetSessionDictionary(); if (!locals.IsValid()) - locals.Reset( - PyRefType::Owned, - PyObject_GetAttrString(globals.get(), m_dictionary_name.c_str())); + locals = unwrapIgnoringErrors( + As(globals.GetAttribute(m_dictionary_name))); if (!locals.IsValid()) locals = globals; diff --git a/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp b/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp --- a/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp +++ b/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp @@ -27,8 +27,7 @@ void SetUp() override { PythonTestSuite::SetUp(); - PythonString sys_module("sys"); - m_sys_module.Reset(PyRefType::Owned, PyImport_Import(sys_module.get())); + m_sys_module = unwrapIgnoringErrors(PythonModule::Import("sys")); m_main_module = PythonModule::MainModule(); m_builtins_module = PythonModule::BuiltinsModule(); } @@ -70,13 +69,10 @@ PythonDictionary dict(PyInitialValue::Empty); PyObject *new_dict = PyDict_New(); - dict.Reset(PyRefType::Owned, new_dict); + dict = Take(new_dict); EXPECT_EQ(new_dict, dict.get()); - dict.Reset(PyRefType::Owned, nullptr); - EXPECT_EQ(nullptr, dict.get()); - - dict.Reset(PyRefType::Owned, PyDict_New()); + dict = Take(PyDict_New()); EXPECT_NE(nullptr, dict.get()); dict.Reset(); EXPECT_EQ(nullptr, dict.get());