Author: Lawrence D'Anna Date: 2019-10-19T18:43:49Z New Revision: 722b61892454b3217d73ec486e52156c5a92b5b3
URL: https://github.com/llvm/llvm-project/commit/722b61892454b3217d73ec486e52156c5a92b5b3 DIFF: https://github.com/llvm/llvm-project/commit/722b61892454b3217d73ec486e52156c5a92b5b3.diff LOG: eliminate nontrivial Reset(...) from TypedPythonObject Summary: This deletes `Reset(...)`, except for the no-argument form `Reset()` from `TypedPythonObject`, and therefore from `PythonString`, `PythonList`, etc. It updates the various callers to use assignment, `As<>`, `Take<>`, and `Retain<>`, as appropriate. followon to https://reviews.llvm.org/D69080 Reviewers: JDevlieghere, clayborg, labath, jingham Reviewed By: labath Subscribers: lldb-commits Tags: #lldb Differential Revision: https://reviews.llvm.org/D69133 llvm-svn: 375350 Added: Modified: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp Removed: ################################################################################ diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp index 2b70762e368f..d0d593656efd 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp +++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp @@ -34,6 +34,7 @@ using namespace lldb_private::python; using llvm::cantFail; using llvm::Error; using llvm::Expected; +using llvm::Twine; template <> Expected<bool> python::As<bool>(Expected<PythonObject> &&obj) { if (!obj) @@ -278,7 +279,7 @@ PythonByteArray::PythonByteArray(llvm::ArrayRef<uint8_t> bytes) PythonByteArray::PythonByteArray(const uint8_t *bytes, size_t length) { const char *str = reinterpret_cast<const char *>(bytes); - Reset(PyRefType::Owned, PyByteArray_FromStringAndSize(str, length)); + *this = Take<PythonByteArray>(PyByteArray_FromStringAndSize(str, length)); } bool PythonByteArray::Check(PyObject *py_obj) { @@ -522,11 +523,11 @@ StructuredData::BooleanSP PythonBoolean::CreateStructuredBoolean() const { PythonList::PythonList(PyInitialValue value) { if (value == PyInitialValue::Empty) - Reset(PyRefType::Owned, PyList_New(0)); + *this = Take<PythonList>(PyList_New(0)); } PythonList::PythonList(int list_size) { - Reset(PyRefType::Owned, PyList_New(list_size)); + *this = Take<PythonList>(PyList_New(list_size)); } bool PythonList::Check(PyObject *py_obj) { @@ -578,11 +579,11 @@ StructuredData::ArraySP PythonList::CreateStructuredArray() const { PythonTuple::PythonTuple(PyInitialValue value) { if (value == PyInitialValue::Empty) - Reset(PyRefType::Owned, PyTuple_New(0)); + *this = Take<PythonTuple>(PyTuple_New(0)); } PythonTuple::PythonTuple(int tuple_size) { - Reset(PyRefType::Owned, PyTuple_New(tuple_size)); + *this = Take<PythonTuple>(PyTuple_New(tuple_size)); } PythonTuple::PythonTuple(std::initializer_list<PythonObject> objects) { @@ -649,7 +650,7 @@ StructuredData::ArraySP PythonTuple::CreateStructuredArray() const { PythonDictionary::PythonDictionary(PyInitialValue value) { if (value == PyInitialValue::Empty) - Reset(PyRefType::Owned, PyDict_New()); + *this = Take<PythonDictionary>(PyDict_New()); } bool PythonDictionary::Check(PyObject *py_obj) { @@ -696,10 +697,10 @@ PythonDictionary::GetItem(const PythonObject &key) const { return Retain<PythonObject>(o); } -Expected<PythonObject> PythonDictionary::GetItem(const char *key) const { +Expected<PythonObject> 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 @@ Error PythonDictionary::SetItem(const PythonObject &key, 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 @@ PythonModule PythonModule::AddModule(llvm::StringRef module) { return PythonModule(PyRefType::Borrowed, PyImport_AddModule(str.c_str())); } -Expected<PythonModule> PythonModule::Import(const char *name) { - PyObject *mod = PyImport_ImportModule(name); +Expected<PythonModule> PythonModule::Import(const Twine &name) { + PyObject *mod = PyImport_ImportModule(NullTerminated(name)); if (!mod) return exception(); return Take<PythonModule>(mod); } -Expected<PythonObject> PythonModule::Get(const char *name) { +Expected<PythonObject> 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<PythonObject>(item); @@ -790,7 +791,9 @@ bool PythonModule::Check(PyObject *py_obj) { } PythonDictionary PythonModule::GetDictionary() const { - return PythonDictionary(PyRefType::Borrowed, PyModule_GetDict(m_py_obj)); + if (!IsValid()) + return PythonDictionary(); + return Retain<PythonDictionary>(PyModule_GetDict(m_py_obj)); } bool PythonCallable::Check(PyObject *py_obj) { @@ -1092,6 +1095,8 @@ template <typename Base> class OwnedPythonFile : public Base { 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/PythonDataObjects.h b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h index 0cdb63f17c99..634d6e896015 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h @@ -151,6 +151,30 @@ template <typename T> T Retain(PyObject *obj) { 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 @@ class PythonObject { return python::Take<PythonObject>(obj); } - llvm::Expected<PythonObject> GetAttribute(const char *name) const { + llvm::Expected<PythonObject> 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<PythonObject>(obj); @@ -392,10 +417,11 @@ template <class T> class TypedPythonObject : public PythonObject { // 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 @@ template <class T> class TypedPythonObject : public PythonObject { Py_DECREF(py_obj); } - TypedPythonObject(PyRefType type, PyObject *py_obj) { Reset(type, py_obj); } - TypedPythonObject() {} }; @@ -562,9 +586,9 @@ class PythonDictionary : public TypedPythonObject<PythonDictionary> { const PythonObject &value); // DEPRECATED llvm::Expected<PythonObject> GetItem(const PythonObject &key) const; - llvm::Expected<PythonObject> GetItem(const char *key) const; + llvm::Expected<PythonObject> 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 @@ class PythonModule : public TypedPythonObject<PythonModule> { return std::move(mod.get()); } - static llvm::Expected<PythonModule> Import(const char *name); + static llvm::Expected<PythonModule> Import(const llvm::Twine &name); - llvm::Expected<PythonObject> Get(const char *name); + llvm::Expected<PythonObject> Get(const llvm::Twine &name); PythonDictionary GetDictionary() const; }; @@ -708,6 +732,17 @@ template <typename T> T unwrapOrSetPythonException(llvm::Expected<T> expected) { return T(); } +namespace python { +// This is only here to help incrementally migrate old, exception-unsafe +// code. +template <typename T> T unwrapIgnoringErrors(llvm::Expected<T> 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/ScriptInterpreterPython.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp index 70654a423848..8f7133524163 100644 --- 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 @@ PythonDictionary &ScriptInterpreterPythonImpl::GetSessionDictionary() { 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<PythonDictionary>(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 @@ bool ScriptInterpreterPythonImpl::ExecuteOneLineWithReturn( PythonDictionary locals = GetSessionDictionary(); if (!locals.IsValid()) { - locals.Reset( - PyRefType::Owned, - PyObject_GetAttrString(globals.get(), m_dictionary_name.c_str())); + locals = unwrapIgnoringErrors( + As<PythonDictionary>(globals.GetAttribute(m_dictionary_name))); } if (!locals.IsValid()) @@ -1204,9 +1201,8 @@ Status ScriptInterpreterPythonImpl::ExecuteMultipleLines( PythonDictionary locals = GetSessionDictionary(); if (!locals.IsValid()) - locals.Reset( - PyRefType::Owned, - PyObject_GetAttrString(globals.get(), m_dictionary_name.c_str())); + locals = unwrapIgnoringErrors( + As<PythonDictionary>(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 index e51c6f0eb0c8..c01dade44408 100644 --- a/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp +++ b/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp @@ -27,8 +27,7 @@ class PythonDataObjectsTest : public PythonTestSuite { 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 @@ TEST_F(PythonDataObjectsTest, TestResetting) { PythonDictionary dict(PyInitialValue::Empty); PyObject *new_dict = PyDict_New(); - dict.Reset(PyRefType::Owned, new_dict); + dict = Take<PythonDictionary>(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<PythonDictionary>(PyDict_New()); EXPECT_NE(nullptr, dict.get()); dict.Reset(); EXPECT_EQ(nullptr, dict.get()); _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits