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 @@ -174,6 +174,27 @@ static auto get(const T &value) { return value.get(); } }; +/* This is a helper class for functions that wrap python C API functions + * that want a null-terminated c string as an argument. It's suboptimal + * for such wrappers to just take a StringRef, because those may not be + * null-terminated, so we'd wind up doing a full copy just to pass a + * fixed string. Instead, wrappers can just take their arguments as + * CStringArg, and callers can pass StringRefs, Twines, strings, or + * const char*, and the right thing will happen. */ +class CStringArg { + llvm::SmallString<32> storage; + const char *cstr; + +public: + CStringArg(const char *s) { cstr = s; } + CStringArg(const std::string &s) { cstr = s.c_str(); } + CStringArg(const llvm::Twine &twine) { + llvm::StringRef ref = twine.toNullTerminatedStringRef(storage); + cstr = ref.data(); + } + const char *str() const { return cstr; } +}; + class PythonObject { public: PythonObject() : m_py_obj(nullptr) {} @@ -323,10 +344,10 @@ return python::Take(obj); } - llvm::Expected GetAttribute(const char *name) const { + llvm::Expected GetAttribute(CStringArg name) const { if (!m_py_obj) return nullDeref(); - PyObject *obj = PyObject_GetAttrString(m_py_obj, name); + PyObject *obj = PyObject_GetAttrString(m_py_obj, name.str()); if (!obj) return exception(); return python::Take(obj); @@ -392,10 +413,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 +427,6 @@ Py_DECREF(py_obj); } - TypedPythonObject(PyRefType type, PyObject *py_obj) { Reset(type, py_obj); } - TypedPythonObject() {} }; @@ -562,9 +582,9 @@ const PythonObject &value); // DEPRECATED llvm::Expected GetItem(const PythonObject &key) const; - llvm::Expected GetItem(const char *key) const; + llvm::Expected GetItem(CStringArg 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(CStringArg key, const PythonObject &value) const; StructuredData::DictionarySP CreateStructuredDictionary() const; }; @@ -592,9 +612,9 @@ return std::move(mod.get()); } - static llvm::Expected Import(const char *name); + static llvm::Expected Import(CStringArg name); - llvm::Expected Get(const char *name); + llvm::Expected Get(CStringArg name); PythonDictionary GetDictionary() const; }; @@ -708,6 +728,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 @@ -278,7 +278,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 +522,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 +578,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 +649,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 +696,10 @@ return Retain(o); } -Expected PythonDictionary::GetItem(const char *key) const { +Expected PythonDictionary::GetItem(CStringArg key) const { if (!IsValid()) return nullDeref(); - PyObject *o = PyDict_GetItemString(m_py_obj, key); + PyObject *o = PyDict_GetItemString(m_py_obj, key.str()); if (PyErr_Occurred()) return exception(); if (!o) @@ -717,11 +717,11 @@ return Error::success(); } -Error PythonDictionary::SetItem(const char *key, +Error PythonDictionary::SetItem(CStringArg 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, key.str(), value.get()); if (r < 0) return exception(); return Error::success(); @@ -763,20 +763,20 @@ return PythonModule(PyRefType::Borrowed, PyImport_AddModule(str.c_str())); } -Expected PythonModule::Import(const char *name) { - PyObject *mod = PyImport_ImportModule(name); +Expected PythonModule::Import(CStringArg name) { + PyObject *mod = PyImport_ImportModule(name.str()); if (!mod) return exception(); return Take(mod); } -Expected PythonModule::Get(const char *name) { +Expected PythonModule::Get(CStringArg 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, name.str()); if (!item) return exception(); return Retain(item); @@ -790,7 +790,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 +1094,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());