From 0a07c9662e67124b00e375aa4a395998d218b220 Mon Sep 17 00:00:00 2001 From: Pavel Labath Date: Mon, 20 Dec 2021 14:36:27 +0100 Subject: [lldb/python] Fix dangling Event and CommandReturnObject references Unlike the rest of our SB objects, SBEvent and SBCommandReturnObject have the ability to hold non-owning pointers to their non-SB counterparts. This makes it hard to ensure the SB objects do not become dangling once their backing object goes away. While we could make these two objects behave like others, that would require plubming even more shared pointers through our internal code (Event objects are mostly prepared for it, CommandReturnObject are not). Doing so seems unnecessarily disruptive, given that (unlike for some of the other objects) I don't see any good reason why would someone want to hold onto these objects after the function terminates. For that reason, this patch implements a different approach -- the SB objects will still hold non-owning pointers, but they will be reset to the empty/default state as soon as the function terminates. This python code will not crash if the user decides to store these objects -- but the objects themselves will be useless/empty. Differential Revision: https://reviews.llvm.org/D116162 --- lldb/bindings/python/python-swigsafecast.swig | 43 +++++++++++++++++----- lldb/bindings/python/python-wrapper.swig | 19 ++++------ .../commands/command/script/TestCommandScript.py | 10 +++++ .../API/commands/command/script/persistence.py | 4 +- 4 files changed, 54 insertions(+), 22 deletions(-) diff --git a/lldb/bindings/python/python-swigsafecast.swig b/lldb/bindings/python/python-swigsafecast.swig index 7d639e664f53..eb684133abef 100644 --- a/lldb/bindings/python/python-swigsafecast.swig +++ b/lldb/bindings/python/python-swigsafecast.swig @@ -1,19 +1,32 @@ namespace lldb_private { namespace python { -PyObject *SBTypeToSWIGWrapper(lldb::SBEvent &event_sb) { - return SWIG_NewPointerObj(&event_sb, SWIGTYPE_p_lldb__SBEvent, 0); -} - -PyObject *SBTypeToSWIGWrapper(lldb::SBCommandReturnObject &cmd_ret_obj_sb) { - return SWIG_NewPointerObj(&cmd_ret_obj_sb, - SWIGTYPE_p_lldb__SBCommandReturnObject, 0); -} - PythonObject ToSWIGHelper(void *obj, swig_type_info *info) { return {PyRefType::Owned, SWIG_NewPointerObj(obj, info, SWIG_POINTER_OWN)}; } +/// A class that automatically clears an SB object when it goes out of scope. +/// Use for cases where the SB object points to a temporary/unowned entity. +template class ScopedPythonObject : PythonObject { +public: + ScopedPythonObject(T *sb, swig_type_info *info) + : PythonObject(ToSWIGHelper(sb, info)), m_sb(sb) {} + ~ScopedPythonObject() { + if (m_sb) + *m_sb = T(); + } + ScopedPythonObject(ScopedPythonObject &&rhs) + : PythonObject(std::move(rhs)), m_sb(std::exchange(rhs.m_sb, nullptr)) {} + ScopedPythonObject(const ScopedPythonObject &) = delete; + ScopedPythonObject &operator=(const ScopedPythonObject &) = delete; + ScopedPythonObject &operator=(ScopedPythonObject &&) = delete; + + const PythonObject &obj() const { return *this; } + +private: + T *m_sb; +}; + PythonObject ToSWIGWrapper(std::unique_ptr value_sb) { return ToSWIGHelper(value_sb.release(), SWIGTYPE_p_lldb__SBValue); } @@ -94,5 +107,17 @@ PythonObject ToSWIGWrapper(const SymbolContext &sym_ctx) { SWIGTYPE_p_lldb__SBSymbolContext); } +ScopedPythonObject +ToSWIGWrapper(CommandReturnObject &cmd_retobj) { + return ScopedPythonObject( + new lldb::SBCommandReturnObject(cmd_retobj), + SWIGTYPE_p_lldb__SBCommandReturnObject); +} + +ScopedPythonObject ToSWIGWrapper(Event *event) { + return ScopedPythonObject(new lldb::SBEvent(event), + SWIGTYPE_p_lldb__SBEvent); +} + } // namespace python } // namespace lldb_private diff --git a/lldb/bindings/python/python-wrapper.swig b/lldb/bindings/python/python-wrapper.swig index a2c1f756a0a2..4f1d65200b10 100644 --- a/lldb/bindings/python/python-wrapper.swig +++ b/lldb/bindings/python/python-wrapper.swig @@ -376,9 +376,8 @@ bool lldb_private::LLDBSWIGPythonCallThreadPlan( PythonObject result; if (event != nullptr) { - lldb::SBEvent sb_event(event); - PythonObject event_arg(PyRefType::Owned, SBTypeToSWIGWrapper(sb_event)); - result = pfunc(event_arg); + ScopedPythonObject event_arg = ToSWIGWrapper(event); + result = pfunc(event_arg.obj()); } else result = pfunc(); @@ -795,7 +794,6 @@ bool lldb_private::LLDBSwigPythonCallCommand( lldb::DebuggerSP debugger, const char *args, lldb_private::CommandReturnObject &cmd_retobj, lldb::ExecutionContextRefSP exe_ctx_ref_sp) { - lldb::SBCommandReturnObject cmd_retobj_sb(cmd_retobj); PyErr_Cleaner py_err_cleaner(true); auto dict = PythonModule::MainModule().ResolveName( @@ -812,14 +810,13 @@ bool lldb_private::LLDBSwigPythonCallCommand( return false; } PythonObject debugger_arg = ToSWIGWrapper(std::move(debugger)); - PythonObject cmd_retobj_arg(PyRefType::Owned, - SBTypeToSWIGWrapper(cmd_retobj_sb)); + auto cmd_retobj_arg = ToSWIGWrapper(cmd_retobj); if (argc.get().max_positional_args < 5u) - pfunc(debugger_arg, PythonString(args), cmd_retobj_arg, dict); + pfunc(debugger_arg, PythonString(args), cmd_retobj_arg.obj(), dict); else pfunc(debugger_arg, PythonString(args), - ToSWIGWrapper(std::move(exe_ctx_ref_sp)), cmd_retobj_arg, dict); + ToSWIGWrapper(std::move(exe_ctx_ref_sp)), cmd_retobj_arg.obj(), dict); return true; } @@ -828,7 +825,6 @@ bool lldb_private::LLDBSwigPythonCallCommandObject( PyObject *implementor, lldb::DebuggerSP debugger, const char *args, lldb_private::CommandReturnObject &cmd_retobj, lldb::ExecutionContextRefSP exe_ctx_ref_sp) { - lldb::SBCommandReturnObject cmd_retobj_sb(cmd_retobj); PyErr_Cleaner py_err_cleaner(true); @@ -838,11 +834,10 @@ bool lldb_private::LLDBSwigPythonCallCommandObject( if (!pfunc.IsAllocated()) return false; - PythonObject cmd_retobj_arg(PyRefType::Owned, - SBTypeToSWIGWrapper(cmd_retobj_sb)); + auto cmd_retobj_arg = ToSWIGWrapper(cmd_retobj); pfunc(ToSWIGWrapper(std::move(debugger)), PythonString(args), - ToSWIGWrapper(exe_ctx_ref_sp), cmd_retobj_arg); + ToSWIGWrapper(exe_ctx_ref_sp), cmd_retobj_arg.obj()); return true; } diff --git a/lldb/test/API/commands/command/script/TestCommandScript.py b/lldb/test/API/commands/command/script/TestCommandScript.py index 33e6a00a404f..eed36c95ec32 100644 --- a/lldb/test/API/commands/command/script/TestCommandScript.py +++ b/lldb/test/API/commands/command/script/TestCommandScript.py @@ -167,7 +167,17 @@ class CmdPythonTestCase(TestBase): self.runCmd('bug11569', check=False) def test_persistence(self): + """ + Ensure that function arguments meaningfully persist (and do not crash!) + even after the function terminates. + """ self.runCmd("command script import persistence.py") self.runCmd("command script add -f persistence.save_debugger save_debugger") self.expect("save_debugger", substrs=[str(self.dbg)]) + + # After the command completes, the debugger object should still be + # valid. self.expect("script str(persistence.debugger_copy)", substrs=[str(self.dbg)]) + # The result object will be replaced by an empty result object (in the + # "Started" state). + self.expect("script str(persistence.result_copy)", substrs=["Started"]) diff --git a/lldb/test/API/commands/command/script/persistence.py b/lldb/test/API/commands/command/script/persistence.py index bc08b4f4dcf4..5c0be34a7d18 100644 --- a/lldb/test/API/commands/command/script/persistence.py +++ b/lldb/test/API/commands/command/script/persistence.py @@ -1,9 +1,11 @@ import lldb debugger_copy = None +result_copy = None def save_debugger(debugger, command, context, result, internal_dict): - global debugger_copy + global debugger_copy, result_copy debugger_copy = debugger + result_copy = result result.AppendMessage(str(debugger)) result.SetStatus(lldb.eReturnStatusSuccessFinishResult) -- cgit v1.2.1