labath created this revision.
labath added reviewers: JDevlieghere, mib.
labath requested review of this revision.
Herald added a project: LLDB.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D116162
Files:
lldb/bindings/python/python-swigsafecast.swig
lldb/bindings/python/python-wrapper.swig
lldb/test/API/commands/command/script/TestCommandScript.py
lldb/test/API/commands/command/script/persistence.py
Index: lldb/test/API/commands/command/script/persistence.py
===================================================================
--- lldb/test/API/commands/command/script/persistence.py
+++ 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)
Index: lldb/test/API/commands/command/script/TestCommandScript.py
===================================================================
--- lldb/test/API/commands/command/script/TestCommandScript.py
+++ lldb/test/API/commands/command/script/TestCommandScript.py
@@ -167,7 +167,17 @@
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"])
Index: lldb/bindings/python/python-wrapper.swig
===================================================================
--- lldb/bindings/python/python-wrapper.swig
+++ lldb/bindings/python/python-wrapper.swig
@@ -376,9 +376,8 @@
PythonObject result;
if (event != nullptr) {
- lldb::SBEvent sb_event(event);
- PythonObject event_arg(PyRefType::Owned, SBTypeToSWIGWrapper(sb_event));
- result = pfunc(event_arg);
+ ScopedPythonObject<SBEvent> event_arg = ToSWIGWrapper(event);
+ result = pfunc(event_arg.obj());
} else
result = pfunc();
@@ -795,7 +794,6 @@
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<PythonDictionary>(
@@ -812,14 +810,13 @@
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 @@
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 @@
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;
}
Index: lldb/bindings/python/python-swigsafecast.swig
===================================================================
--- lldb/bindings/python/python-swigsafecast.swig
+++ 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 <typename T> 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<lldb::SBValue> value_sb) {
return ToSWIGHelper(value_sb.release(), SWIGTYPE_p_lldb__SBValue);
}
@@ -94,5 +107,17 @@
SWIGTYPE_p_lldb__SBSymbolContext);
}
+ScopedPythonObject<lldb::SBCommandReturnObject>
+ToSWIGWrapper(CommandReturnObject &cmd_retobj) {
+ return ScopedPythonObject<lldb::SBCommandReturnObject>(
+ new lldb::SBCommandReturnObject(cmd_retobj),
+ SWIGTYPE_p_lldb__SBCommandReturnObject);
+}
+
+ScopedPythonObject<lldb::SBEvent> ToSWIGWrapper(Event *event) {
+ return ScopedPythonObject<lldb::SBEvent>(new lldb::SBEvent(event),
+ SWIGTYPE_p_lldb__SBEvent);
+}
+
} // namespace python
} // namespace lldb_private
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
