lawrence_danna updated this revision to Diff 225686.
lawrence_danna marked 6 inline comments as done.
lawrence_danna added a comment.
fixed
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69133/new/
https://reviews.llvm.org/D69133
Files:
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
Index: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
===================================================================
--- lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ 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<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());
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===================================================================
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ 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<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 @@
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 @@
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;
Index: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
===================================================================
--- lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
+++ 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, an 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<PythonObject>(obj);
}
- llvm::Expected<PythonObject> GetAttribute(const char *name) const {
+ llvm::Expected<PythonObject> 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<PythonObject>(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<PythonObject> GetItem(const PythonObject &key) const;
- llvm::Expected<PythonObject> GetItem(const char *key) const;
+ llvm::Expected<PythonObject> 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<PythonModule> Import(const char *name);
+ static llvm::Expected<PythonModule> Import(CStringArg name);
- llvm::Expected<PythonObject> Get(const char *name);
+ llvm::Expected<PythonObject> 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 <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
Index: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
===================================================================
--- lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
+++ 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<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 +522,11 @@
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 +578,11 @@
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 +649,7 @@
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 +696,10 @@
return Retain<PythonObject>(o);
}
-Expected<PythonObject> PythonDictionary::GetItem(const char *key) const {
+Expected<PythonObject> 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> PythonModule::Import(const char *name) {
- PyObject *mod = PyImport_ImportModule(name);
+Expected<PythonModule> PythonModule::Import(CStringArg name) {
+ PyObject *mod = PyImport_ImportModule(name.str());
if (!mod)
return exception();
return Take<PythonModule>(mod);
}
-Expected<PythonObject> PythonModule::Get(const char *name) {
+Expected<PythonObject> 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<PythonObject>(item);
@@ -790,7 +790,9 @@
}
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 +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();
}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
