eliminate nontrivial Reset(...) from TypedPythonObject

[lawrence_danna]

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

[labath]

I think the conversion of different string types should be handled differently. Please see inline comment for details. Otherwise, this looks fine.

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h

350–360

This is the annoying part about interacting with external "c" apis in llvm. Llvm really likes to have StringRefs everywhere, but since they're not guaranteed to be null-terminated, it usually ends up requiring a string copy (.str().c_str()).
Having the wrapper APIs take const char * is not really a good option because that pushes the problem down to its users (and/or creates redundant strlen()s everywhere), whereas the purpose of these APIs should be to create an interface that interacts nicely with the rest of the codebase.

There are two ways to get around that, depending on how much you want to optimize stuff:

1. Just have your functions take a StringRef and call .str().c_str() when you need a null-terminated version of it
2. Have your functions take a llvm::Twine. This class has a method https://llvm.org/doxygen/classllvm_1_1Twine.html#a5d968bc337af0be1f18813553a046df6, which enables you to avoid a string copy in the cases where the user passed in a null-terminated string, while avoiding to define multiple overloads of all your functions.

It is used as such:

auto my_foo_wrapper(const llvm::Twine &str) {
  llvm::SmallString<32 /*or whatever is the "typical" size*/> storage;
  llvm::StringRef s = str.toNullTerminatedStringRef(storage); // s is either the original const char *, or points to "storage"
  return foo(s.data() /*null terminated*/);
}

For this code I'd be fine with either option, but I'd like to avoid having two overloads of all functions (particularly if none of them is a StringRef).

422–424

Is this needed, given the "using" declaration above?

427

Isn't this already set by the superclass constructor?

735–743

Could you put this into the "python" namespace, since we already have it? Otherwise, it looks like a generic lldb_private function (which I would very much like to avoid it becoming).

[lawrence_danna]

fixed

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h

422–424

yes, it seems when you delete one override of a member function, it deletes all the other ones that you don't explicitly mention too. Actually it's the using line that should be deleted.

427

yes, fixed

[lawrence_danna]

spelling

[lawrence_danna]

use enable_if on CStringArg so the other constructors take precedence

[labath]

While I appreciate the desire to reduce "boilerplate", I also think there's a lot of value in consistency. None of the existing APIs in llvm do anything like this, and are perfectly happy to call toNullTerminatedStringRef manually. You've now introduced another string-like type, which people have to figure out how it works. If you really feel like you must avoid the extra line of code (because that's the only thing that this saves, really) then you could at least move the class into the implementation of the methods, so that at least the inferface is consistent with all other llvm functions taking an llvm::Twine. You could define an llvm::Twine constructor and a "const char *" conversion operator for this class, so that you can type Py_whatever(NullTerminated(twine)). This way we could also define a "char *" conversion operator on the class in the PY2 case, also avoiding the need for the py2_const_cast thingy.

[lawrence_danna]

In D69133#1715544, @labath wrote:

While I appreciate the desire to reduce "boilerplate", I also think there's a lot of value in consistency....

oh, yea somehow I thought going through the twine would still copy the string, I see you're right, it won't.

[lawrence_danna]

fixed

[labath]

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h

154

Actually, could you also write a short blurb about what is the purpose of this class.

[lawrence_danna]

comment on NullTerminated

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h

 template <typename T> T Retain(PyObject *obj) {
   assert(obj);
   assert(!PyErr_Occurred());
   T thing(PyRefType::Borrowed, obj);
   assert(thing.IsValid());
   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 };
 
 template <typename T, typename Enable = void> struct PythonFormat;
 
 template <> struct PythonFormat<unsigned long long> {
   static constexpr char format = 'K';
llvm::Expected<PythonObject> Call(const T &... t) const {
     const char format[] = {'(', PythonFormat<T>::format..., ')', 0};
     PyObject *obj = PyObject_CallFunction(m_py_obj, py2_const_cast(format),
                                           PythonFormat<T>::get(t)...);
     if (!obj)
       return exception();
     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);
   }
 
   llvm::Expected<bool> IsTrue() {
     if (!m_py_obj)
       return nullDeref();
     int r = PyObject_IsTrue(m_py_obj);
     if (r < 0)
       return exception();
     return !!r;
   }
 

 } // namespace python
 
 template <class T> class TypedPythonObject : public PythonObject {
 public:
   // override to perform implicit type conversions on Reset
   // 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);
     if (T::Check(py_obj))
       PythonObject::Reset(type, py_obj);
     else if (type == PyRefType::Owned)
       Py_DECREF(py_obj);
   }
 
-  TypedPythonObject(PyRefType type, PyObject *py_obj) { Reset(type, py_obj); }
-
   TypedPythonObject() {}
 };
 
 class PythonBytes : public TypedPythonObject<PythonBytes> {
 public:
   using TypedPythonObject::TypedPythonObject;
   explicit PythonBytes(llvm::ArrayRef<uint8_t> bytes);
   PythonBytes(const uint8_t *bytes, size_t length);
public:
 
   PythonList GetKeys() const;
 
   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::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;
 };
 
 class PythonModule : public TypedPythonObject<PythonModule> {
 public:
   using TypedPythonObject::TypedPythonObject;
 
static PythonModule ImportModule(llvm::StringRef name) {
     auto mod = Import(s.c_str());
     if (!mod) {
       llvm::consumeError(mod.takeError());
       return 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;
 };
 
 class PythonCallable : public TypedPythonObject<PythonCallable> {
 public:
   using TypedPythonObject::TypedPythonObject;
 
template <typename T> T unwrapOrSetPythonException(llvm::Expected<T> expected) {
   llvm::handleAllErrors(
       expected.takeError(), [](PythonException &E) { E.Restore(); },
       [](const llvm::ErrorInfoBase &E) {
         PyErr_SetString(PyExc_Exception, E.message().c_str());
       });
   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
 
 #endif // LLDB_PLUGINS_SCRIPTINTERPRETER_PYTHON_PYTHONDATAOBJECTS_H

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp

 #include <stdio.h>
 
 using namespace lldb_private;
 using namespace lldb;
 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)
     return obj.takeError();
   return obj.get().IsTrue();
 }
 
 template <>
StructuredData::StringSP PythonBytes::CreateStructuredString() const {
   return result;
 }
 
 PythonByteArray::PythonByteArray(llvm::ArrayRef<uint8_t> bytes)
     : PythonByteArray(bytes.data(), bytes.size()) {}
 
 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) {
   if (!py_obj)
     return false;
   return PyByteArray_Check(py_obj);
 }
 
StructuredData::BooleanSP PythonBoolean::CreateStructuredBoolean() const {
   result->SetValue(GetValue());
   return result;
 }
 
 // PythonList
 
 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) {
   if (!py_obj)
     return false;
   return PyList_Check(py_obj);
 }
 
StructuredData::ArraySP PythonList::CreateStructuredArray() const {
   }
   return result;
 }
 
 // PythonTuple
 
 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) {
   m_py_obj = PyTuple_New(objects.size());
 
   uint32_t idx = 0;
   for (auto object : objects) {
     if (object.IsValid())
StructuredData::ArraySP PythonTuple::CreateStructuredArray() const {
   }
   return result;
 }
 
 // PythonDictionary
 
 PythonDictionary::PythonDictionary(PyInitialValue value) {
   if (value == PyInitialValue::Empty)
-    Reset(PyRefType::Owned, PyDict_New());
+    *this = Take<PythonDictionary>(PyDict_New());
 }
 
 bool PythonDictionary::Check(PyObject *py_obj) {
   if (!py_obj)
     return false;
 
   return PyDict_Check(py_obj);
 }

 #else
   PyObject *o = PyDict_GetItem(m_py_obj, key.get());
 #endif
   if (!o)
     return keyError();
   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)
     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,
+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();
 }
 
 void PythonDictionary::SetItemForKey(const PythonObject &key,
                                      const PythonObject &value) {
   Error error = SetItem(key, value);

 
 PythonModule PythonModule::MainModule() { return AddModule("__main__"); }
 
 PythonModule PythonModule::AddModule(llvm::StringRef module) {
   std::string str = module.str();
   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);
 }
 
 bool PythonModule::Check(PyObject *py_obj) {
   if (!py_obj)
     return false;
 
   return PyModule_Check(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) {
   if (!py_obj)
     return false;
 
   return PyCallable_Check(py_obj);
 }
OwnedPythonFile(const PythonFile &file, bool borrowed, Args... args)
       : Base(args...), m_py_obj(file), m_borrowed(borrowed) {
     assert(m_py_obj);
   }
 
   ~OwnedPythonFile() override {
     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();
   }
 
   bool IsPythonSideValid() const {
     GIL takeGIL;
     auto closed = As<bool>(m_py_obj.GetAttribute("closed"));
     if (!closed) {
       llvm::consumeError(closed.takeError());

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

 #include <memory>
 #include <mutex>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string>
 
 using namespace lldb;
 using namespace lldb_private;
+using namespace lldb_private::python;
 
 // Defined in the SWIG source file
 #if PY_MAJOR_VERSION >= 3
 extern "C" PyObject *PyInit__lldb(void);
 
 #define LLDBSwigPyInit PyInit__lldb
 
 #else
PythonDictionary &ScriptInterpreterPythonImpl::GetSessionDictionary() {
   if (!main_module.IsValid())
     return m_session_dict;
 
   PythonDictionary main_dict(PyRefType::Borrowed,
                              PyModule_GetDict(main_module.get()));
   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;
 }
 
 static std::string GenerateUniqueName(const char *base_name_wanted,
                                       uint32_t &functions_counter,
                                       const void *name_token = nullptr) {
   StreamString sstr;
 
PythonDictionary globals(PyRefType::Borrowed,
                            PyModule_GetDict(main_module.get()));
   PythonObject py_error;
   bool ret_success = false;
   int success;
 
   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;
 
   py_error.Reset(PyRefType::Borrowed, PyErr_Occurred());
   if (py_error.IsValid())
     PyErr_Clear();
Status ScriptInterpreterPythonImpl::ExecuteMultipleLines(
   PythonObject &main_module = GetMainModule();
   PythonDictionary globals(PyRefType::Borrowed,
                            PyModule_GetDict(main_module.get()));
   PythonObject py_error;
 
   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;
 
   py_error.Reset(PyRefType::Borrowed, PyErr_Occurred());
   if (py_error.IsValid())
     PyErr_Clear();
 

lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

 using namespace lldb_private;
 using namespace lldb_private::python;
 
 class PythonDataObjectsTest : public PythonTestSuite {
 public:
   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();
   }
 
   void TearDown() override {
     m_sys_module.Reset();
     m_main_module.Reset();
     m_builtins_module.Reset();
TEST_F(PythonDataObjectsTest, TestOwnedReferences) {
   owned_long.Reset();
   EXPECT_EQ(original_refcnt, strong_ref.get()->ob_refcnt);
 }
 
 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());
 }
 
 TEST_F(PythonDataObjectsTest, TestBorrowedReferences) {
   PythonInteger long_value(PyRefType::Owned, PyLong_FromLong(3));
   Py_ssize_t original_refcnt = long_value.get()->ob_refcnt;
// the old implementation of GetArgInfo just doesn't work on builtins.
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
     EXPECT_EQ(arginfo.get().count, 1);
     EXPECT_EQ(arginfo.get().max_positional_args, 1u);
     EXPECT_EQ(arginfo.get().has_varargs, false);
   }
 
 #endif
 }
 No newline at end of file