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