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