[Lldb-commits] [lldb] 7f09ab0 - [lldb] Fix [some] leaks in python bindings
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Mon Nov 22 06:27:21 PST 2021
Author: Pavel Labath
Date: 2021-11-22T15:14:52+01:00
New Revision: 7f09ab08de5ae8f8bd77f5c02c70b991277eb1ae
URL: https://github.com/llvm/llvm-project/commit/7f09ab08de5ae8f8bd77f5c02c70b991277eb1ae
DIFF: https://github.com/llvm/llvm-project/commit/7f09ab08de5ae8f8bd77f5c02c70b991277eb1ae.diff
LOG: [lldb] Fix [some] leaks in python bindings
Using an lldb_private object in the bindings involves three steps
- wrapping the object in it's lldb::SB variant
- using swig to convert/wrap that to a PyObject
- wrapping *that* in a lldb_private::python::PythonObject
Our SBTypeToSWIGWrapper was only handling the middle part. This doesn't
just result in increased boilerplate in the callers, but is also a
functionality problem, as it's very hard to get the lifetime of of all
of these objects right. Most of the callers are creating the SB object
(step 1) on the stack, which means that we end up with dangling python
objects after the function terminates. Most of the time this isn't a
problem, because the python code does not need to persist the objects.
However, there are legitimate cases where they can do it (and even if
the use case is not completely legitimate, crashing is not the best
response to that).
For this reason, some of our code creates the SB object on the heap, but
it has another problem -- it never gets cleaned up.
This patch begins to add a new function (ToSWIGWrapper), which does all
of the three steps, while properly taking care of ownership. In the
first step, I have converted most of the leaky code (except for
SBStructuredData, which needs a bit more work).
Differential Revision: https://reviews.llvm.org/D114259
Added:
Modified:
lldb/bindings/python/python-swigsafecast.swig
lldb/bindings/python/python-wrapper.swig
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
Removed:
################################################################################
diff --git a/lldb/bindings/python/python-swigsafecast.swig b/lldb/bindings/python/python-swigsafecast.swig
index aa2bcfb8c8ae9..fdd3b4e62c102 100644
--- a/lldb/bindings/python/python-swigsafecast.swig
+++ b/lldb/bindings/python/python-swigsafecast.swig
@@ -1,23 +1,14 @@
+namespace lldb_private {
+namespace python {
+
PyObject *SBTypeToSWIGWrapper(lldb::SBEvent &event_sb) {
return SWIG_NewPointerObj(&event_sb, SWIGTYPE_p_lldb__SBEvent, 0);
}
-PyObject *SBTypeToSWIGWrapper(lldb::SBProcess &process_sb) {
- return SWIG_NewPointerObj(&process_sb, SWIGTYPE_p_lldb__SBProcess, 0);
-}
-
PyObject *SBTypeToSWIGWrapper(lldb::SBThread &thread_sb) {
return SWIG_NewPointerObj(&thread_sb, SWIGTYPE_p_lldb__SBThread, 0);
}
-PyObject *SBTypeToSWIGWrapper(lldb::SBThreadPlan &thread_plan_sb) {
- return SWIG_NewPointerObj(&thread_plan_sb, SWIGTYPE_p_lldb__SBThreadPlan, 0);
-}
-
-PyObject *SBTypeToSWIGWrapper(lldb::SBTarget &target_sb) {
- return SWIG_NewPointerObj(&target_sb, SWIGTYPE_p_lldb__SBTarget, 0);
-}
-
PyObject *SBTypeToSWIGWrapper(lldb::SBFrame &frame_sb) {
return SWIG_NewPointerObj(&frame_sb, SWIGTYPE_p_lldb__SBFrame, 0);
}
@@ -26,10 +17,6 @@ PyObject *SBTypeToSWIGWrapper(lldb::SBDebugger &debugger_sb) {
return SWIG_NewPointerObj(&debugger_sb, SWIGTYPE_p_lldb__SBDebugger, 0);
}
-PyObject *SBTypeToSWIGWrapper(lldb::SBBreakpoint &breakpoint_sb) {
- return SWIG_NewPointerObj(&breakpoint_sb, SWIGTYPE_p_lldb__SBBreakpoint, 0);
-}
-
PyObject *SBTypeToSWIGWrapper(lldb::SBWatchpoint &watchpoint_sb) {
return SWIG_NewPointerObj(&watchpoint_sb, SWIGTYPE_p_lldb__SBWatchpoint, 0);
}
@@ -40,10 +27,6 @@ SBTypeToSWIGWrapper(lldb::SBBreakpointLocation &breakpoint_location_sb) {
SWIGTYPE_p_lldb__SBBreakpointLocation, 0);
}
-PyObject *SBTypeToSWIGWrapper(lldb::SBValue &value_sb) {
- return SWIG_NewPointerObj(&value_sb, SWIGTYPE_p_lldb__SBValue, 0);
-}
-
PyObject *SBTypeToSWIGWrapper(lldb::SBCommandReturnObject &cmd_ret_obj_sb) {
return SWIG_NewPointerObj(&cmd_ret_obj_sb,
SWIGTYPE_p_lldb__SBCommandReturnObject, 0);
@@ -70,3 +53,38 @@ PyObject *SBTypeToSWIGWrapper(lldb::SBSymbolContext &sym_ctx_sb) {
PyObject *SBTypeToSWIGWrapper(lldb::SBStream &stream_sb) {
return SWIG_NewPointerObj(&stream_sb, SWIGTYPE_p_lldb__SBStream, 0);
}
+
+PythonObject ToSWIGHelper(void *obj, swig_type_info *info) {
+ return {PyRefType::Owned, SWIG_NewPointerObj(obj, info, SWIG_POINTER_OWN)};
+}
+
+PythonObject ToSWIGWrapper(std::unique_ptr<lldb::SBValue> value_sb) {
+ return ToSWIGHelper(value_sb.release(), SWIGTYPE_p_lldb__SBValue);
+}
+
+PythonObject ToSWIGWrapper(lldb::ValueObjectSP value_sp) {
+ return ToSWIGWrapper(std::make_unique<lldb::SBValue>(std::move(value_sp)));
+}
+
+PythonObject ToSWIGWrapper(lldb::TargetSP target_sp) {
+ return ToSWIGHelper(new lldb::SBTarget(std::move(target_sp)),
+ SWIGTYPE_p_lldb__SBTarget);
+}
+
+PythonObject ToSWIGWrapper(lldb::ProcessSP process_sp) {
+ return ToSWIGHelper(new lldb::SBProcess(std::move(process_sp)),
+ SWIGTYPE_p_lldb__SBProcess);
+}
+
+PythonObject ToSWIGWrapper(lldb::ThreadPlanSP thread_plan_sp) {
+ return ToSWIGHelper(new lldb::SBThreadPlan(std::move(thread_plan_sp)),
+ SWIGTYPE_p_lldb__SBThreadPlan);
+}
+
+PythonObject ToSWIGWrapper(lldb::BreakpointSP breakpoint_sp) {
+ return ToSWIGHelper(new lldb::SBBreakpoint(std::move(breakpoint_sp)),
+ SWIGTYPE_p_lldb__SBBreakpoint);
+}
+
+} // namespace python
+} // namespace lldb_private
diff --git a/lldb/bindings/python/python-wrapper.swig b/lldb/bindings/python/python-wrapper.swig
index 6dc8ca1703905..8159423c62bbd 100644
--- a/lldb/bindings/python/python-wrapper.swig
+++ b/lldb/bindings/python/python-wrapper.swig
@@ -145,7 +145,6 @@ LLDBSwigPythonCallTypeScript
std::string& retval
)
{
- lldb::SBValue sb_value (valobj_sp);
lldb::SBTypeSummaryOptions sb_options(options_sp.get());
retval.clear();
@@ -195,7 +194,7 @@ LLDBSwigPythonCallTypeScript
return false;
}
- PythonObject value_arg(PyRefType::Owned, SBTypeToSWIGWrapper(sb_value));
+ PythonObject value_arg = ToSWIGWrapper(valobj_sp);
PythonObject options_arg(PyRefType::Owned, SBTypeToSWIGWrapper(sb_options));
if (argc.get().max_positional_args < 3)
@@ -227,11 +226,10 @@ LLDBSwigPythonCreateSyntheticProvider
if (!pfunc.IsAllocated())
Py_RETURN_NONE;
- // FIXME: SBValue leaked here
- lldb::SBValue *sb_value = new lldb::SBValue(valobj_sp);
+ auto sb_value = std::make_unique<lldb::SBValue>(valobj_sp);
sb_value->SetPreferSyntheticValue(false);
- PythonObject val_arg(PyRefType::Owned, SBTypeToSWIGWrapper(*sb_value));
+ PythonObject val_arg = ToSWIGWrapper(std::move(sb_value));
if (!val_arg.IsAllocated())
Py_RETURN_NONE;
@@ -295,12 +293,7 @@ LLDBSwigPythonCreateScriptedProcess
return nullptr;
}
- // FIXME: SBTarget leaked here
- PythonObject target_arg(
- PyRefType::Owned, SBTypeToSWIGWrapper(*new lldb::SBTarget(target_sp)));
-
- if (!target_arg.IsAllocated())
- Py_RETURN_NONE;
+ PythonObject target_arg = ToSWIGWrapper(target_sp);
llvm::Expected<PythonCallable::ArgInfo> arg_info = pfunc.GetArgInfo();
if (!arg_info) {
@@ -354,14 +347,6 @@ LLDBSwigPythonCreateScriptedThread
return nullptr;
}
- // FIXME: This leaks the SBProcess object
- PythonObject process_arg(
- PyRefType::Owned,
- SBTypeToSWIGWrapper(*new lldb::SBProcess(process_sp)));
-
- if (!process_arg.IsAllocated())
- Py_RETURN_NONE;
-
llvm::Expected<PythonCallable::ArgInfo> arg_info = pfunc.GetArgInfo();
if (!arg_info) {
llvm::handleAllErrors(
@@ -379,7 +364,7 @@ LLDBSwigPythonCreateScriptedThread
if (arg_info.get().max_positional_args == 2) {
// FIXME: SBStructuredData leaked here
PythonObject args_arg(PyRefType::Owned, SBTypeToSWIGWrapper(*new lldb::SBStructuredData(args_impl)));
- result = pfunc(process_arg, args_arg);
+ result = pfunc(ToSWIGWrapper(process_sp), args_arg);
} else {
error_string.assign("wrong number of arguments in __init__, should be 2 (not including self)");
Py_RETURN_NONE;
@@ -415,13 +400,7 @@ LLDBSwigPythonCreateScriptedThreadPlan
return nullptr;
}
- // FIXME: SBThreadPlan leaked here
- PythonObject tp_arg(
- PyRefType::Owned,
- SBTypeToSWIGWrapper(*new lldb::SBThreadPlan(thread_plan_sp)));
-
- if (!tp_arg.IsAllocated())
- Py_RETURN_NONE;
+ PythonObject tp_arg = ToSWIGWrapper(thread_plan_sp);
llvm::Expected<PythonCallable::ArgInfo> arg_info = pfunc.GetArgInfo();
if (!arg_info) {
@@ -507,15 +486,11 @@ LLDBSWIGPythonCallThreadPlan
return false;
}
-SWIGEXPORT void *
-LLDBSwigPythonCreateScriptedBreakpointResolver
-(
- const char *python_class_name,
- const char *session_dictionary_name,
+SWIGEXPORT void *LLDBSwigPythonCreateScriptedBreakpointResolver(
+ const char *python_class_name, const char *session_dictionary_name,
lldb_private::StructuredDataImpl *args_impl,
- lldb::BreakpointSP &breakpoint_sp
-)
-{
+ const lldb::BreakpointSP &breakpoint_sp) {
+
if (python_class_name == NULL || python_class_name[0] == '\0' || !session_dictionary_name)
Py_RETURN_NONE;
@@ -527,16 +502,11 @@ LLDBSwigPythonCreateScriptedBreakpointResolver
if (!pfunc.IsAllocated())
return nullptr;
- // FIXME: SBBreakpoint leaked here
- lldb::SBBreakpoint *bkpt_value = new lldb::SBBreakpoint(breakpoint_sp);
-
- PythonObject bkpt_arg(PyRefType::Owned, SBTypeToSWIGWrapper(*bkpt_value));
-
// FIXME: SBStructuredData leaked here
lldb::SBStructuredData *args_value = new lldb::SBStructuredData(args_impl);
PythonObject args_arg(PyRefType::Owned, SBTypeToSWIGWrapper(*args_value));
- PythonObject result = pfunc(bkpt_arg, args_arg, dict);
+ PythonObject result = pfunc(ToSWIGWrapper(breakpoint_sp), args_arg, dict);
// FIXME: At this point we should check that the class we found supports all the methods
// that we need.
@@ -637,16 +607,11 @@ LLDBSwigPythonCreateScriptedStopHook
return nullptr;
}
- // FIXME: SBTarget leaked here
- lldb::SBTarget *target_val
- = new lldb::SBTarget(target_sp);
- PythonObject target_arg(PyRefType::Owned, SBTypeToSWIGWrapper(*target_val));
-
// FIXME: SBStructuredData leaked here
lldb::SBStructuredData *args_value = new lldb::SBStructuredData(args_impl);
PythonObject args_arg(PyRefType::Owned, SBTypeToSWIGWrapper(*args_value));
- PythonObject result = pfunc(target_arg, args_arg, dict);
+ PythonObject result = pfunc(ToSWIGWrapper(target_sp), args_arg, dict);
if (result.IsAllocated())
{
@@ -1076,13 +1041,7 @@ LLDBSWIGPythonCreateOSPlugin
if (!pfunc.IsAllocated())
Py_RETURN_NONE;
- // FIXME: This leaks the SBProcess object
- lldb::SBProcess *process_sb = new lldb::SBProcess(process_sp);
- PythonObject process_arg(PyRefType::Owned, SBTypeToSWIGWrapper(*process_sb));
- if (!process_arg.IsAllocated())
- Py_RETURN_NONE;
-
- auto result = pfunc(process_arg);
+ auto result = pfunc(ToSWIGWrapper(process_sp));
if (result.IsAllocated())
return result.release();
@@ -1147,21 +1106,15 @@ LLDBSWIGPython_GetDynamicSetting (void* module, const char* setting, const lldb:
if (!pfunc.IsAllocated())
Py_RETURN_NONE;
- lldb::SBTarget target_sb(target_sp);
- PythonObject target_arg(PyRefType::Owned, SBTypeToSWIGWrapper(target_sb));
- auto result = pfunc(target_arg, PythonString(setting));
+ auto result = pfunc(ToSWIGWrapper(target_sp), PythonString(setting));
return result.release();
}
-SWIGEXPORT bool
-LLDBSWIGPythonRunScriptKeywordProcess
-(const char* python_function_name,
-const char* session_dictionary_name,
-lldb::ProcessSP& process,
-std::string& output)
+SWIGEXPORT bool LLDBSWIGPythonRunScriptKeywordProcess(
+ const char *python_function_name, const char *session_dictionary_name,
+ const lldb::ProcessSP &process, std::string &output) {
-{
if (python_function_name == NULL || python_function_name[0] == '\0' || !session_dictionary_name)
return false;
@@ -1173,9 +1126,7 @@ std::string& output)
if (!pfunc.IsAllocated())
return false;
- lldb::SBProcess process_sb(process);
- PythonObject process_arg(PyRefType::Owned, SBTypeToSWIGWrapper(process_sb));
- auto result = pfunc(process_arg, dict);
+ auto result = pfunc(ToSWIGWrapper(process), dict);
output = result.Str().GetString().str();
@@ -1210,14 +1161,10 @@ std::string& output)
return true;
}
-SWIGEXPORT bool
-LLDBSWIGPythonRunScriptKeywordTarget
-(const char* python_function_name,
-const char* session_dictionary_name,
-lldb::TargetSP& target,
-std::string& output)
+SWIGEXPORT bool LLDBSWIGPythonRunScriptKeywordTarget(
+ const char *python_function_name, const char *session_dictionary_name,
+ const lldb::TargetSP &target, std::string &output) {
-{
if (python_function_name == NULL || python_function_name[0] == '\0' || !session_dictionary_name)
return false;
@@ -1229,9 +1176,7 @@ std::string& output)
if (!pfunc.IsAllocated())
return false;
- lldb::SBTarget target_sb(target);
- PythonObject target_arg(PyRefType::Owned, SBTypeToSWIGWrapper(target_sb));
- auto result = pfunc(target_arg, dict);
+ auto result = pfunc(ToSWIGWrapper(target), dict);
output = result.Str().GetString().str();
@@ -1266,14 +1211,10 @@ std::string& output)
return true;
}
-SWIGEXPORT bool
-LLDBSWIGPythonRunScriptKeywordValue
-(const char* python_function_name,
-const char* session_dictionary_name,
-lldb::ValueObjectSP& value,
-std::string& output)
+SWIGEXPORT bool LLDBSWIGPythonRunScriptKeywordValue(
+ const char *python_function_name, const char *session_dictionary_name,
+ const lldb::ValueObjectSP &value, std::string &output) {
-{
if (python_function_name == NULL || python_function_name[0] == '\0' || !session_dictionary_name)
return false;
@@ -1285,9 +1226,7 @@ std::string& output)
if (!pfunc.IsAllocated())
return false;
- lldb::SBValue value_sb(value);
- PythonObject value_arg(PyRefType::Owned, SBTypeToSWIGWrapper(value_sb));
- auto result = pfunc(value_arg, dict);
+ auto result = pfunc(ToSWIGWrapper(value), dict);
output = result.Str().GetString().str();
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
index c1f4c2d3b4d3c..e8b1ad7c6c693 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -127,7 +127,7 @@ extern "C" bool LLDBSWIGPythonCallThreadPlan(void *implementor,
extern "C" void *LLDBSwigPythonCreateScriptedBreakpointResolver(
const char *python_class_name, const char *session_dictionary_name,
- lldb_private::StructuredDataImpl *args, lldb::BreakpointSP &bkpt_sp);
+ lldb_private::StructuredDataImpl *args, const lldb::BreakpointSP &bkpt_sp);
extern "C" unsigned int
LLDBSwigPythonCallBreakpointResolver(void *implementor, const char *method_name,
@@ -196,7 +196,7 @@ LLDBSwigPython_GetRecognizedArguments(void *implementor,
extern "C" bool LLDBSWIGPythonRunScriptKeywordProcess(
const char *python_function_name, const char *session_dictionary_name,
- lldb::ProcessSP &process, std::string &output);
+ const lldb::ProcessSP &process, std::string &output);
extern "C" bool LLDBSWIGPythonRunScriptKeywordThread(
const char *python_function_name, const char *session_dictionary_name,
@@ -204,7 +204,7 @@ extern "C" bool LLDBSWIGPythonRunScriptKeywordThread(
extern "C" bool LLDBSWIGPythonRunScriptKeywordTarget(
const char *python_function_name, const char *session_dictionary_name,
- lldb::TargetSP &target, std::string &output);
+ const lldb::TargetSP &target, std::string &output);
extern "C" bool LLDBSWIGPythonRunScriptKeywordFrame(
const char *python_function_name, const char *session_dictionary_name,
@@ -212,7 +212,7 @@ extern "C" bool LLDBSWIGPythonRunScriptKeywordFrame(
extern "C" bool LLDBSWIGPythonRunScriptKeywordValue(
const char *python_function_name, const char *session_dictionary_name,
- lldb::ValueObjectSP &value, std::string &output);
+ const lldb::ValueObjectSP &value, std::string &output);
extern "C" void *
LLDBSWIGPython_GetDynamicSetting(void *module, const char *setting,
@@ -2653,11 +2653,11 @@ bool ScriptInterpreterPythonImpl::RunScriptFormatKeyword(
}
{
- ProcessSP process_sp(process->shared_from_this());
Locker py_lock(this,
Locker::AcquireLock | Locker::InitSession | Locker::NoSTDIN);
ret_val = LLDBSWIGPythonRunScriptKeywordProcess(
- impl_function, m_dictionary_name.c_str(), process_sp, output);
+ impl_function, m_dictionary_name.c_str(), process->shared_from_this(),
+ output);
if (!ret_val)
error.SetErrorString("python script evaluation failed");
}
@@ -2753,11 +2753,10 @@ bool ScriptInterpreterPythonImpl::RunScriptFormatKeyword(
}
{
- ValueObjectSP value_sp(value->GetSP());
Locker py_lock(this,
Locker::AcquireLock | Locker::InitSession | Locker::NoSTDIN);
ret_val = LLDBSWIGPythonRunScriptKeywordValue(
- impl_function, m_dictionary_name.c_str(), value_sp, output);
+ impl_function, m_dictionary_name.c_str(), value->GetSP(), output);
if (!ret_val)
error.SetErrorString("python script evaluation failed");
}
diff --git a/lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp b/lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
index 7a4b6328a2f54..230fcf7ba38af 100644
--- a/lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
+++ b/lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
@@ -129,7 +129,7 @@ extern "C" bool LLDBSWIGPythonCallThreadPlan(void *implementor,
extern "C" void *LLDBSwigPythonCreateScriptedBreakpointResolver(
const char *python_class_name, const char *session_dictionary_name,
- lldb_private::StructuredDataImpl *args, lldb::BreakpointSP &bkpt_sp) {
+ lldb_private::StructuredDataImpl *args, const lldb::BreakpointSP &bkpt_sp) {
return nullptr;
}
@@ -248,7 +248,7 @@ LLDBSwigPython_GetRecognizedArguments(void *implementor,
extern "C" bool LLDBSWIGPythonRunScriptKeywordProcess(
const char *python_function_name, const char *session_dictionary_name,
- lldb::ProcessSP &process, std::string &output) {
+ const lldb::ProcessSP &process, std::string &output) {
return false;
}
@@ -260,7 +260,7 @@ extern "C" bool LLDBSWIGPythonRunScriptKeywordThread(
extern "C" bool LLDBSWIGPythonRunScriptKeywordTarget(
const char *python_function_name, const char *session_dictionary_name,
- lldb::TargetSP &target, std::string &output) {
+ const lldb::TargetSP &target, std::string &output) {
return false;
}
@@ -272,7 +272,7 @@ extern "C" bool LLDBSWIGPythonRunScriptKeywordFrame(
extern "C" bool LLDBSWIGPythonRunScriptKeywordValue(
const char *python_function_name, const char *session_dictionary_name,
- lldb::ValueObjectSP &value, std::string &output) {
+ const lldb::ValueObjectSP &value, std::string &output) {
return false;
}
More information about the lldb-commits
mailing list