[Lldb-commits] [PATCH] D13617: Fix ref-counting of Python objects

[Greg Clayton via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

See inlined comments.


================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:73-79
@@ -73,2 +72,9 @@
 
-    class PythonObject
+enum class PyRefType
+{
+    Borrowed, // We are not given ownership of the incoming PyObject.
+              // We cannot safely hold it without calling Py_INCREF.
+    Owned     // We have ownership of the incoming PyObject.  We should
+              // not call Py_INCREF.
+};
+
----------------
clayborg wrote:
> Might be nicer to move into the PyRef class and just name these "Type" and "InitialValue"?
It seems like we don't need an enum for this, this can just be a bool named "dont_incref" or some other name?

================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:73-85
@@ -73,2 +72,15 @@
 
-    class PythonObject
+enum class PyRefType
+{
+    Borrowed, // We are not given ownership of the incoming PyObject.
+              // We cannot safely hold it without calling Py_INCREF.
+    Owned     // We have ownership of the incoming PyObject.  We should
+              // not call Py_INCREF.
+};
+
+enum class PyInitialValue
+{
+    Invalid,
+    Empty
+};
+
----------------
Might be nicer to move into the PyRef class and just name these "Type" and "InitialValue"?

================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:87
@@ -75,1 +86,3 @@
+
+class PyRef
     {
----------------
Why did we need to rename PythonObject to PyRef? It can be done, but seems different from the the rest of the classes here. Everything else is spelled out, I would prefer this stay as PythonObject.

================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:90-91
@@ -76,4 +89,4 @@
     public:
-        PythonObject () :
-            m_py_obj(NULL)
+      PyRef()
+          : m_py_obj(nullptr)
         {
----------------
indent is wrong here

================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:192
@@ +191,3 @@
+
+        operator PyObject *() const { return m_py_obj; }
+
----------------
I really don't like these implicit conversions as they make for bad things that can happen. We already see issues with Reset as you mention above. I would rather just have a "PyObject *get() const;" method that we use to explicitly do this. It gets event worse when you already have an implicit bool operator...

================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:206-211
@@ -162,8 +205,8 @@
     public:
-        PythonString ();
-        PythonString (PyObject *o);
-        PythonString (const PythonObject &object);
-        PythonString (llvm::StringRef string);
-        PythonString (const char *string);
-        virtual ~PythonString ();
+      PythonString();
+      PythonString(PyRefType type, PyObject *o);
+      PythonString(const PythonString &object);
+      explicit PythonString(llvm::StringRef string);
+      explicit PythonString(const char *string);
+      ~PythonString() override;
 
----------------
indent is wrong here

================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:234-238
@@ -188,8 +233,7 @@
     public:
-        
-        PythonInteger ();
-        PythonInteger (PyObject* py_obj);
-        PythonInteger (const PythonObject &object);
-        PythonInteger (int64_t value);
-        virtual ~PythonInteger ();
+      PythonInteger();
+      PythonInteger(PyRefType type, PyObject *o);
+      PythonInteger(const PythonInteger &object);
+      explicit PythonInteger(int64_t value);
+      ~PythonInteger() override;
 
----------------
indent is wrong here

================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:258-261
@@ -211,6 +257,6 @@
     public:
-      PythonList();
-        PythonList (PyObject* py_obj);
-        PythonList (const PythonObject &object);
-        virtual ~PythonList ();
+      PythonList(PyInitialValue value);
+      PythonList(PyRefType type, PyObject *o);
+      PythonList(const PythonList &list);
+      ~PythonList() override;
 
----------------
indent is wrong here

================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:284-287
@@ -237,6 +283,6 @@
     public:
-        PythonDictionary();
-        PythonDictionary (PyObject* object);
-        PythonDictionary (const PythonObject &object);
-        virtual ~PythonDictionary ();
+      PythonDictionary(PyInitialValue value);
+      PythonDictionary(PyRefType type, PyObject *o);
+      PythonDictionary(const PythonDictionary &dict);
+      ~PythonDictionary() override;
 
----------------
indent is wrong here

================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:304
@@ -261,15 +303,3 @@
 
-        typedef bool (*DictionaryIteratorCallback)(PythonString* key, PythonDictionary* dict);
-        
-        PythonList
-        GetKeys () const;
-        
-        PythonString
-        GetKeyAtPosition (uint32_t pos) const;
-        
-        PythonObject
-        GetValueAtPosition (uint32_t pos) const;
-        
-        void
-        SetItemForKey (const PythonString &key, PyObject *value);
+        void SetItemForKey(const PythonString &key, PyObject *value);
 
----------------
We should use "const PyRef &object" here instead of "PyObject *value". I know the old code used PyObject, but it is probably better to switch over.

================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:306
@@ -275,4 +305,3 @@
 
-        void
-        SetItemForKey (const PythonString &key, const PythonObject& value);
+        void SetItemForKey(const char *key, PyObject *value);
 
----------------
We should use "const PyRef &object" here instead of "PyObject *value". I know the old code used PyObject, but it is probably better to switch over.

================
Comment at: source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp:183-201
@@ -204,1 +182,21 @@
+ScriptInterpreterPython::ScriptInterpreterPython(CommandInterpreter &interpreter)
+    : ScriptInterpreter(interpreter, eScriptLanguagePython)
+    , IOHandlerDelegateMultiline("DONE")
+    , m_saved_stdin()
+    , m_saved_stdout()
+    , m_saved_stderr()
+    , m_main_module()
+    , m_lldb_module()
+    , m_session_dict(PyInitialValue::Invalid)
+    , m_sys_module_dict(PyInitialValue::Invalid)
+    , m_run_one_line_function()
+    , m_run_one_line_str_global()
+    , m_dictionary_name(interpreter.GetDebugger().GetInstanceName().AsCString())
+    , m_terminal_state()
+    , m_active_io_handler(eIOHandlerNone)
+    , m_session_is_active(false)
+    , m_pty_slave_is_open(false)
+    , m_valid_session(true)
+    , m_lock_count(0)
+    , m_command_thread_state(nullptr)
 {
----------------
please move : to previous line and commas to the end.


http://reviews.llvm.org/D13617