[lldb/python] Fix dangling Event and CommandReturnObject references

[labath]

Summary

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.

[mib]

This patch looks good to me, however, may be it would be good to have a IsInitialized/ IsCleared method for all SB-types to make sure the object hasn't been cleared after going out-of-scope ?

I'm suggesting this because not all SB-types have a IsValid method and for those who have it, the implementation varies a lot from class to class. So comparing the SB object against its default constructed value could be a way to check if the object was well-formed.

Having the object reseted by SWIG if it goes out-of-scope could collide with this approach. What do you think ?

[labath]

This isn't a feature I would want advertise too broadly, or add special APIs to support it. The way I see it, if a user needs to check if an object has been cleared, something has gone wrong already. So I'd rather do something to discourage this use case instead of providing more support for it. Crashing is definitely discouraging, but maybe a bit too extreme. If there was a simple way to throw an exception in this case, then I think that would be a good compromise.

The reason I started looking into all of this is because of a bug report where the user wanted to stash a debugger object and access it later (which is a semi-reasonable thing to do, I'd say). I haven't heard of anybody trying to store SBCommandReturnObjects, nor I intend to start encouraging that. The only reason I wrote this patch is to tie up loose ends.

Having the object reseted by SWIG if it goes out-of-scope could collide with this approach. What do you think ?

I'm not sure what you meant by that, but I don't consider this behavior as set in stone. If we come up with a different/better way to handle this, then we can just change this code.

[mib]

In D116162#3211322, @labath wrote:

This isn't a feature I would want advertise too broadly, or add special APIs to support it. The way I see it, if a user needs to check if an object has been cleared, something has gone wrong already. So I'd rather do something to discourage this use case instead of providing more support for it. Crashing is definitely discouraging, but maybe a bit too extreme. If there was a simple way to throw an exception in this case, then I think that would be a good compromise.

The reason I started looking into all of this is because of a bug report where the user wanted to stash a debugger object and access it later (which is a semi-reasonable thing to do, I'd say). I haven't heard of anybody trying to store SBCommandReturnObjects, nor I intend to start encouraging that. The only reason I wrote this patch is to tie up loose ends.

Having the object reseted by SWIG if it goes out-of-scope could collide with this approach. What do you think ?

I'm not sure what you meant by that, but I don't consider this behavior as set in stone. If we come up with a different/better way to handle this, then we can just change this code.

Sounds good ! I don't have any other objection with this patch then.

lldb/bindings/python/python-swigsafecast.swig

 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);
 }
 
 PythonObject ToSWIGWrapper(lldb::ValueObjectSP value_sp) {
   return ToSWIGWrapper(std::make_unique<lldb::SBValue>(std::move(value_sp)));
 }
 
return ToSWIGHelper(new lldb::SBTypeSummaryOptions(summary_options),
                       SWIGTYPE_p_lldb__SBTypeSummaryOptions);
 }
 
 PythonObject ToSWIGWrapper(const SymbolContext &sym_ctx) {
   return ToSWIGHelper(new lldb::SBSymbolContext(sym_ctx),
                       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/bindings/python/python-wrapper.swig

bool lldb_private::LLDBSWIGPythonCallThreadPlan(
   PythonObject self(PyRefType::Borrowed, static_cast<PyObject *>(implementor));
   auto pfunc = self.ResolveName<PythonCallable>(method_name);
 
   if (!pfunc.IsAllocated())
     return false;
 
   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();
 
   if (PyErr_Occurred()) {
     got_error = true;
     printf("Return value was neither false nor true for call to %s.\n",
            method_name);
     PyErr_Print();
void *lldb_private::LLDBSWIGPython_CastPyObjectToSBMemoryRegionInfo(PyObject *
   return sb_ptr;
 }
 
 bool lldb_private::LLDBSwigPythonCallCommand(
     const char *python_function_name, const char *session_dictionary_name,
     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>(
       session_dictionary_name);
   auto pfunc = PythonObject::ResolveNameWithDictionary<PythonCallable>(
       python_function_name, dict);
 
   if (!pfunc.IsAllocated())
     return false;
 
   auto argc = pfunc.GetArgInfo();
   if (!argc) {
     llvm::consumeError(argc.takeError());
     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;
 }
 
 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);
 
   PythonObject self(PyRefType::Borrowed, implementor);
   auto pfunc = self.ResolveName<PythonCallable>("__call__");
 
   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;
 }
 
 void *lldb_private::LLDBSWIGPythonCreateOSPlugin(
     const char *python_class_name, const char *session_dictionary_name,
     const lldb::ProcessSP &process_sp) {
   if (python_class_name == NULL || python_class_name[0] == '\0' ||

lldb/test/API/commands/command/script/TestCommandScript.py

# check that the execution context is passed in to commands that ask for it
         # http://llvm.org/bugs/show_bug.cgi?id=11569
         # LLDBSwigPythonCallCommand crashes when a command script returns an
         # object
         self.runCmd('command script add -f bug11569 bug11569')
         # This should not crash.
         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"])

lldb/test/API/commands/command/script/persistence.py

 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)