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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:49
 
+  void *instance_obj = nullptr;
+  if (script_object)
----------------
mib wrote:
> 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).
While not ideal, I don't think this is a big deal. I might consider passing the `script_object` to the `CreatePluginObject` though -- it would make a typed interface, and avoid a branch here.


================
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));
 
----------------
mib wrote:
> 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 ?
> Why can't it wrap an owned reference ?
There's no reason it *can't* do that. It's just that it doesn't do that now.

> checking the PyObject type (Borrowed/Owned)
borrowedness/ownedness is not a real property of the PyObject. It's just an abstract notion describing the relationship between a reference (PyObject instance) and code handling that reference. If some code "owns" a reference it is responsible for (eventually) freeing (decreffing) it (or passing the ownership onto someone else, etc.) If some code "borrows" a reference, then it must not free it, and it must be careful to not use it after the actual owner frees it.

The reference received through the `instance_obj` is borrowed since the `thread_info_sp` destructor will eventually free it. The reference created through `LLDBSwigPythonCreateScriptedThread` is owned. The fact that you have owning and non-owning code paths converging means one of them is incorrect.

The fix is actually pretty straightforward -- you can change a borrowed reference into an owned one by increffing it yourself. Ideally the PythonObject helper class, as that way the ownership will be explicitly managed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117071