[Lldb-commits] [PATCH] D117071: [lldb/Plugins] Add support of multiple ScriptedThreads in a ScriptedProcess

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Continuing the discussion from the other patch, I've made some comments about who I think owns various objects going around in this patch. They should be read in the control-flow order (which happens to coincide with how they appear in the source code), not in the order the phabricator puts them at the bottom of the page. I'd appreciate it if you could go through them and point out any errors in my reasoning/assumptions.



================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:304
 
-  lldb::ThreadSP thread_sp;
-  thread_sp = std::make_shared<ScriptedThread>(*this, error);
-
-  if (!thread_sp || error.Fail())
-    return GetInterface().ErrorWithMessage<bool>(LLVM_PRETTY_FUNCTION,
-                                                 error.AsCString(), error);
+  StructuredData::DictionarySP thread_info_sp = GetInterface().GetThreadsInfo();
 
----------------
This looks like the "start" of this patch. IIUC, this shared_ptr is the only shared_ptr pointing to this object (`use_count() == 1`).


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:313
+      [this, &old_thread_list, &error,
+       &new_thread_list](ConstString key, StructuredData::Object *val) -> bool {
+    if (!val)
----------------
this object appears to come out of thread_info_sp. I am going to assume it is owned by that object.


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:332
+    lldb::ThreadSP thread_sp =
+        std::make_shared<ScriptedThread>(*this, error, val->GetAsGeneric());
+
----------------
This just does a cast so it is still owned by that object.


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:32
+ScriptedThread::ScriptedThread(ScriptedProcess &process, Status &error,
+                               StructuredData::Generic *script_object)
     : Thread(process, LLDB_INVALID_THREAD_ID), m_scripted_process(process),
----------------
Control continues here.  `script_object` is not owned by this function/class.


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:49
 
+  void *instance_obj = nullptr;
+  if (script_object)
----------------
This is when things start to get fuzzy, as this function seems to support both a nullptr and a non-nullptr argument. Not necessarily bad, but it makes reasoning about anything harder. It'd be better if this function could always be called with a valid object.


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:51
+  if (script_object)
+    instance_obj = script_object->GetValue();
+
----------------
I'm assuming this is a PyObject disguised as a void* and that it is still being owned(increffed) by the initial shared_ptr.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedThreadPythonInterface.cpp:34
     const llvm::StringRef class_name, ExecutionContext &exe_ctx,
-    StructuredData::DictionarySP args_sp) {
+    StructuredData::DictionarySP args_sp, void *instance_obj) {
 
----------------
continuing here


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedThreadPythonInterface.cpp:47
+  if (!instance_obj) {
+    instance_obj = LLDBSwigPythonCreateScriptedThread(
+        class_name.str().c_str(), m_interpreter.GetDictionaryName(), process_sp,
----------------
Now it gets really confusing. *If* `instance_obj` was non-null, then it will point to an object which is not owned by this function. *If* it was null, then it set to point to an object which *is* owned (or at least *should be* owned -- as noone else owns it) by this function.

This doesn't seem correct to me, but even if it is, it sure is confusing.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedThreadPythonInterface.cpp:56
   m_object_instance_sp =
-      StructuredData::GenericSP(new StructuredPythonObject(ret_val));
+      StructuredData::GenericSP(new StructuredPythonObject(instance_obj));
 
----------------
now this maybe-owned reference gets passed into a `StructuredPythonObject` which assumes it is getting a borrowed reference (it does an incref).

That seems correct in the instance_obj at entry != nullptr case, but not in the other one (=> leak).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117071/new/

https://reviews.llvm.org/D117071