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

326–338

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).

401

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

406

Isn't this already set by the superclass constructor?

720–728

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

401

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.

406

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);
 }
 
 } // 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';
   static auto get(unsigned long long value) { return value; }
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 {
     if (!m_py_obj)
       return nullDeref();
     PyObject *obj = PyObject_GetAttrString(m_py_obj, name);
     if (!obj)
       return exception();
     return python::Take<PythonObject>(obj);
   }
 
+  llvm::Expected<PythonObject> GetAttribute(const std::string &name) const {
+    return GetAttribute(name.c_str());
+  }
+
   llvm::Expected<bool> IsTrue() {
     if (!m_py_obj)
       return nullDeref();
     int r = PyObject_IsTrue(m_py_obj);
     if (r < 0)
       return exception();
     return !!r;
   }

 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(PyRefType type, PyObject *py_obj) {
-    Reset();
+  void Reset() { PythonObject::Reset(); }
+
+  void Reset(PyRefType type, PyObject *py_obj) = delete;
+
+  TypedPythonObject(PyRefType type, PyObject *py_obj) {
+    m_py_obj = nullptr;
     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 std::string &key) const {
+    return GetItem(key.c_str());
+  }
   llvm::Error SetItem(const PythonObject &key, const PythonObject &value) const;
   llvm::Error SetItem(const char *key, const PythonObject &value) const;
 
   StructuredData::DictionarySP CreateStructuredDictionary() const;
 };
 
 class PythonModule : public TypedPythonObject<PythonModule> {
 public:
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();
 }
 
+// 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 lldb_private
 
 #endif
 
 #endif // LLDB_PLUGINS_SCRIPTINTERPRETER_PYTHON_PYTHONDATAOBJECTS_H

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

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);
 }

 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