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

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 9 inline comments as done.
mib added inline comments.


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:49
 
+  void *instance_obj = nullptr;
+  if (script_object)
----------------
labath wrote:
> 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.
I understand but even if it might be possible to do it for ScriptedThreads, we want to give the user multiple way of creating them (in lldb or directly from python).


================
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,
----------------
labath wrote:
> 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.
Correct, when `instance_obj` is null at entry, it gets allocated by  `LLDBSwigPythonCreateScriptedThread` which creates an owned reference as you mentioned in https://reviews.llvm.org/D117065#inline-1120555, and that reference gets passed to `m_object_instance_sp`.

So in my understanding, at this stage, instance_obj `use_count() == 1`.


================
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));
 
----------------
labath wrote:
> 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).
I might be missing something but I don't understand why does the `StructuredPythonObject` **expects** a borrowed reference ?
Why can't it wrap an owned reference ?

I think checking the `PyObject` type (Borrowed/Owned) on the `StructuredPythonObject` constructor would allow us to `incref` it only in the case of a borrowed ref, and fix the leak in the case of an owned ref, right ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117071