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@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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
