Fix crash on SBCommandReturnObject & assignment

[jankratochvil]

Summary

I was writing an SB API client and it was crashing on:

bool DoExecute(SBDebugger dbg, char **command, SBCommandReturnObject &result) {
  result = subcommand(dbg, "help");

That is because SBCommandReturnObject &result gets initialized inside LLDB by:

bool DoExecute(Args &command, CommandReturnObject &result) override {
  // std::unique_ptr gets initialized here from `&result`!!!
  SBCommandReturnObject sb_return(&result);
  DoExecute(...);
  sb_return.Release();

[labath]

+ Jim for SB API.

I ran into this quirk of SBCommandReturnObject a couple of weeks ago, and then ran away screaming.

I agree that this approach isn't nice, but probably there isn't a nice approach to this at this point. One possibility you could consider is storing a shared_ptr<CommandRO> inside SBCommandRO and encoding the ownership into the deleter of the shared_ptr (regular deleter => owned, noop deleter => unowned).

lldb/include/lldb/API/SBCommandReturnObject.h

24

Add a comment to explain the purpose of this function?

lldb/packages/Python/lldbsuite/test/api/command-return-object/main.cpp

18

Is that intentional? If so, why?

23

This looks weird. Please use a separate declaration for the variable and the class.

[clayborg]

lldb/source/API/SBCommandInterpreter.cpp

165–172

Could this code just create a local SBCommandReturnObject and then copy the CommandReturnObject back into "result"?

 bool DoExecute(Args &command, CommandReturnObject &result) override {
    SBCommandReturnObject sb_return;
    SBCommandInterpreter sb_interpreter(&m_interpreter);
    SBDebugger debugger_sb(m_interpreter.GetDebugger().shared_from_this());
    bool ret = m_backend->DoExecute(
        debugger_sb, (char **)command.GetArgumentVector(), sb_return);
    std::swap(result, sb_return.ref());
    return ret;
}

[jankratochvil]

In D67589#1670966, @labath wrote:

I agree that this approach isn't nice, but probably there isn't a nice approach to this at this point. One possibility you could consider is storing a shared_ptr<CommandRO> inside SBCommandRO and encoding the ownership into the deleter of the shared_ptr (regular deleter => owned, noop deleter => unowned).

Maybe also possibly cheaper llvm::PointerIntPair representing an associated bool for the deletion.

lldb/packages/Python/lldbsuite/test/api/command-return-object/main.cpp

18

It has no purpose there for this bugfix but when already writing a testcase I wanted to test this generally fragile functionality.

lldb/source/API/SBCommandInterpreter.cpp

165–172

I made it that way first but it breaks the testsuite. For example due to:

lldb/source/Commands/CommandObjectCommands.cpp:
1236	  bool DoExecute(llvm::StringRef raw_command_line,
1237	                 CommandReturnObject &result) override {
...
1242	    result.SetStatus(eReturnStatusInvalid);
 - ctored CommandReturnObject has eReturnStatusStarted.
1244	    if (!scripter ||
1245	        !scripter->RunScriptBasedCommand(m_function_name.c_str(),
1246	                                         raw_command_line, m_synchro, result,
1247	                                         error, m_exe_ctx)) {
1248	      result.AppendError(error.AsCString());
1249	      result.SetStatus(eReturnStatusFailed);
1250	    } else {
1251	      // Don't change the status if the command already set it...
1252	      if (result.GetStatus() == eReturnStatusInvalid) {
1253	        if (result.GetOutputData().empty())
1254	          result.SetStatus(eReturnStatusSuccessFinishNoResult);
1255	        else
1256	          result.SetStatus(eReturnStatusSuccessFinishResult);
1257	      }
1258	    }

So it would be possible but that would require refactorization more of the code in callers which I find outside of the scope of this patch.
This patch tries to make this bugfix fully transparent to existing code.

[jankratochvil]

lldb/packages/Python/lldbsuite/test/api/command-return-object/main.cpp

18

If you mean the line:

+     if (!result.Succeeded())

That is not really needed for this testcase but I think this is how a real world implementation should behave so why not here, just 2 lines of code.

[clayborg]

lldb/include/lldb/API/SBCommandReturnObject.h

22–27

Easier to just make SBCommandReturnObject::ref() protected and friend any users or just make it public?

97–98

Remove if we make SBCommandReturnObject::ref() protected and friend any users or just make it public?

106

Easier to just make SBCommandReturnObject::ref() protected and friend any users or just make it public?

lldb/source/API/SBCommandReturnObject.cpp

21–24

Easier to just make SBCommandReturnObject::ref() protected and friend any users or just make it public?

[labath]

In D67589#1672686, @jankratochvil wrote:

In D67589#1670966, @labath wrote:

I agree that this approach isn't nice, but probably there isn't a nice approach to this at this point. One possibility you could consider is storing a shared_ptr<CommandRO> inside SBCommandRO and encoding the ownership into the deleter of the shared_ptr (regular deleter => owned, noop deleter => unowned).

Maybe also possibly cheaper llvm::PointerIntPair representing an associated bool for the deletion.

We couldn't use PointerIntPair due to the our abi stability promise (http://lldb.llvm.org/resources/sbapi.html). One could try to implement something like that by hand, but I don't think it's worth the trouble.

lldb/packages/Python/lldbsuite/test/api/command-return-object/main.cpp

18

Nope, I meant the "result = result" line. Testing self-assignment is fine, but you may want to call that out explicitly, as otherwise someone may just come along and delete the "obviously useless" code.

[jankratochvil]

In D67589#1673756, @labath wrote:

We couldn't use PointerIntPair due to the our abi stability promise (http://lldb.llvm.org/resources/sbapi.html).

Thanks for this document! I did not know it. But isn't the document trying to keep ABI compatibility despite it is not explicitly stated there? Or otherwise why are there so many restrictions.

As then even using std::shared_ptr I think violates the ABI compatibility promise there as currently SBCommandReturnObject contains std::unique_ptr<lldb_private::CommandReturnObject> m_opaque_up. But maybe this discussion goes too far from this patch.

lldb/include/lldb/API/SBCommandReturnObject.h

22–27

I tried to make everyone a friend with SBCommandReturnObject but I could not. The problem is these two

SWIGEXPORT bool LLDBSwigPythonCallCommand
SWIGEXPORT bool LLDBSwigPythonCallCommandObject

from lldb/scripts/Python/python-wrapper.swig have SWIGEXPORT which is defined only in build directory tools/lldb/scripts/LLDBWrapPython.cpp` so one cannot access it from lldb/include/lldb/API/SBCommandReturnObject.h:

error: unknown type name 'SWIGEXPORT'

And when I remove SWIGEXPORT there I get:

llvm-monorepo-clangassert/tools/lldb/scripts/LLDBWrapPython.cpp:78030:1: error: declaration of 'LLDBSwigPythonCallCommandObject' has a different language linkage
LLDBSwigPythonCallCommandObject
llvm-monorepo/lldb/include/lldb/API/SBCommandReturnObject.h:20:2: note: previous declaration is here
 LLDBSwigPythonCallCommandObject

But it is true making it public is safe enough, lldb_private::CommandReturnObject is opaque to SB API users anyway. I found it too bold to write it that way myself, thanks.

[clayborg]

In D67589#1673771, @jankratochvil wrote:

In D67589#1673756, @labath wrote:

We couldn't use PointerIntPair due to the our abi stability promise (http://lldb.llvm.org/resources/sbapi.html).

Thanks for this document! I did not know it. But isn't the document trying to keep ABI compatibility despite it is not explicitly stated there? Or otherwise why are there so many restrictions.

We are making all efforts to vend a stable C++ API. Thus the restrictions. If anyone makes a binary that contains a lldb::SB object we can't change the size of the object. That means no inheritance (size of object could change), no virtual functions (vtable size would change), and no changing the ivars since their size defines the size of the object. Then all function calls to the LLDB API are just fancy long mangled name lookups just like a C API, and thus our API is stable. Then people can have our classes as ivars in their classes, and if liblldb.so gets updated they can still run without having to re-link.

As then even using std::shared_ptr I think violates the ABI compatibility promise there as currently SBCommandReturnObject contains std::unique_ptr<lldb_private::CommandReturnObject> m_opaque_up. But maybe this discussion goes too far from this patch.

We can replace the std::unique_ptr<lldb_private::CommandReturnObject> m_opaque_up with another unique pointer to an internal object (std::unique_ptr<lldb_private::CommandReturnObjectImpl> m_opaque_up;). We could make a struct:

lldb_private::CommandReturnObjectImpl {
  bool owned;
  std::unique_ptr<lldb_private::CommandReturnObject> m_opaque_up;
};

And then release the object without freeing it if needed in the CommandReturnObjectImpl destructor. This keeps the ABI stable since the size of lldb::SBCommandReturnObject doesn't change. The constructors aren't inlined for just this purpose (another API thing).

[jankratochvil]

In D67589#1674269, @clayborg wrote:

We are making all efforts to vend a stable C++ API.

IIUC you mean "stable C++ ABI" here. Thanks for the clarification. Maybe http://lldb.llvm.org/resources/sbapi.html could say that and it would be much more clear.

lldb_private::CommandReturnObjectImpl {
  bool owned;
  std::unique_ptr<lldb_private::CommandReturnObject> m_opaque_up;
};

Is this a request to rework this patch this way? If so isn't it safer / more clear to do it rather this way?

lldb_private::CommandReturnObjectImpl {
  bool owned;
  lldb_private::CommandReturnObject *m_opaque_ptr;
  ~CommandReturnObjectImpl() { if (owned) delete m_opaque_ptr; }
};

[labath]

In D67589#1674640, @jankratochvil wrote:

Maybe http://lldb.llvm.org/resources/sbapi.html could say that and it would be much more clear.

That's probably a good idea, though I would still keep the list of restrictions spelled out as ABI can be broken in a lot of subtle and unobvious ways (at least to a person who has never tried to maintain a stable abi).

lldb_private::CommandReturnObjectImpl {
  bool owned;
  std::unique_ptr<lldb_private::CommandReturnObject> m_opaque_up;
};

Is this a request to rework this patch this way? If so isn't it safer / more clear to do it rather this way?

lldb_private::CommandReturnObjectImpl {
  bool owned;
  lldb_private::CommandReturnObject *m_opaque_ptr;
  ~CommandReturnObjectImpl() { if (owned) delete m_opaque_ptr; }
};

I think I would like that better than the swap trick. Since you're now inside a pimpl, you can replace the two members with a llvm::PointerIntPair, if you are so inclined.

[jankratochvil]

Changed @clayborg's lldb_private::CommandReturnObjectImpl to lldb_private::SBCommandReturnObjectImpl - both variants are used in LLDB SB*.cpp and the SB* looks more logical to me.
Used @clayborg's lldb_private::SBCommandReturnObjectImpl::m_owned with conditional in dtor but personally I would use two inherited classes + virtual dtor instead.

In D67589#1676511, @labath wrote:

I think I would like that better than the swap trick. Since you're now inside a pimpl, you can replace the two members with a llvm::PointerIntPair, if you are so inclined.

Not done. I proposed llvm::PointerIntPair originally to match your goal of keeping single pointer in SBCommandReturnObject. But when there is already a new instance of lldb_private::CommandReturnObjectImpl then llvm::PointerIntPair therein would be more a burden both in the source code and for CPU.

m_opaque_up type	ABI compatible	CPU overhead	formal http://lldb.llvm.org/resources/sbapi.html compliance
shared_ptr with two deleters	no (size 16)	high ('lock' instructions)	yes (a bug in the sbapi.html doc?)
llvm::PointerIntPair	yes (size 8)	low	no (sort of)
unique_ptr<SBCommandReturnObjectImpl>	yes (size 8)	high (extra object allocation)	yes

But I am fine with this patch implementing SBCommandReturnObjectImpl as this is not any performance critical part of code.

[labath]

This looks good to me, but please do give a chance for other to look this over too. Thanks for cleaning this up.

In D67589#1684847, @jankratochvil wrote:

m_opaque_up type	ABI compatible
llvm::PointerIntPair	yes (size 8)

This is true, but it also inflicts the SB stability requirements onto the PointerIntPair class. While I don't imagine the sizeof(PointerIntPair) will ever change, I don't think this is a good thing to do, just on principle.

lldb/packages/Python/lldbsuite/test/api/command-return-object/Makefile

2

I'm pretty sure this line is not needed.

lldb/source/API/SBCommandReturnObject.cpp

122–123

I think get()->Clear() or ref().Clear() would be easier on the eye.

[jankratochvil]

This comment has been deleted.

lldb/packages/Python/lldbsuite/test/api/command-return-object/Makefile

2

Removed in current testsuite: rL373061

[shafik]

@jankratochvil it looks like this PR broke the green dragon build: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/1926/

The specific test that is failing is TestFunctionStarts.py see the more detailed log here: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/1926/testReport/junit/lldb-Suite/macosx_function-starts/TestFunctionStarts_py/

To clarify, reverting this change fixes the test on my side.

[jankratochvil]

In D67589#1686150, @shafik wrote:

@jankratochvil it looks like this PR broke the green dragon build: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/1926/

This PR is not yet checked-in. Let's move this discussion to rL373061.

[jankratochvil]

lldb/packages/Python/lldbsuite/test/api/command-return-object/Makefile

2

So I do not understand how MAKE_DSYM works but global removal of MAKE_DSYM := NO in rL373061 broke OSX so I have reverted it by rL373110. Keeping MAKE_DSYM := NO stays omitted for this testcase.

[labath]

lldb/packages/Python/lldbsuite/test/api/command-return-object/Makefile

2

Yeah, the test which failed is doing pretty funky stuff, so I can imagine it can get hurt by removing MAKE_DSYM. Omitting it for this test should be safe though. (Probably all other tests except TestFunctionStarts.py too, but I don't think you have to bother with that.)

[jankratochvil]

Had to disable the testcase on Windows: rL373787

[labath]

(Ideally, all of these tests would be just (gtest) unit tests. There's no need to pull in python to do something our build system already knows perfectly well to do.)

lldb/include/lldb/API/SBCommandReturnObject.h

 
 #include <stdio.h>
 
 #include <memory>
 
 #include "lldb/API/SBDefines.h"
 
 namespace lldb {
+class LLDB_API SBCommandReturnObject;
+}
+
+namespace lldb_private {
+CommandReturnObject &
+SBCommandReturnObject_ref(lldb::SBCommandReturnObject &sb_cmd);
+}
+
+namespace lldb {
 
 class LLDB_API SBCommandReturnObject {
 public:
   SBCommandReturnObject();
 
   SBCommandReturnObject(const lldb::SBCommandReturnObject &rhs);
 
   ~SBCommandReturnObject();
 
   const lldb::SBCommandReturnObject &
   operator=(const lldb::SBCommandReturnObject &rhs);
 
-  SBCommandReturnObject(lldb_private::CommandReturnObject *ptr);
-
-  lldb_private::CommandReturnObject *Release();
-
   explicit operator bool() const;
 
   bool IsValid() const;
 
   const char *GetOutput();
 
   const char *GetError();
 
public:
   void SetError(lldb::SBError &error,
                 const char *fallback_error_cstr = nullptr);
 
   void SetError(const char *error_cstr);
 
 protected:
   friend class SBCommandInterpreter;
   friend class SBOptions;
+  friend lldb_private::CommandReturnObject &
+  lldb_private::SBCommandReturnObject_ref(SBCommandReturnObject &sb_cmd);
 
   lldb_private::CommandReturnObject *operator->() const;
 
   lldb_private::CommandReturnObject *get() const;
 
   lldb_private::CommandReturnObject &operator*() const;
 
   lldb_private::CommandReturnObject &ref() const;
 
   void SetLLDBObjectPtr(lldb_private::CommandReturnObject *ptr);
 
 private:
   std::unique_ptr<lldb_private::CommandReturnObject> m_opaque_up;
 };
 
 } // namespace lldb
 
 #endif // LLDB_SBCommandReturnObject_h_

lldb/packages/Python/lldbsuite/test/api/command-return-object/Makefile

+MAKE_DSYM := NO
+
+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/packages/Python/lldbsuite/test/api/command-return-object/TestSBCommandReturnObject.py

+"""Test the lldb public C++ api for returning SBCommandReturnObject."""
+
+from __future__ import print_function
+
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestSBCommandReturnObject(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+    NO_DEBUG_INFO_TESTCASE = True
+
+    @skipIfNoSBHeaders
+    def test_sb_command_return_object(self):
+        env = {self.dylibPath: self.getLLDBLibraryEnvVal()}
+
+        self.driver_exe = self.getBuildArtifact("command-return-object")
+        self.buildDriver('main.cpp', self.driver_exe)
+        self.addTearDownHook(lambda: os.remove(self.driver_exe))
+        self.signBinary(self.driver_exe)
+
+        if self.TraceOn():
+            print("Running test %s" % self.driver_exe)
+            check_call([self.driver_exe, self.driver_exe], env=env)
+        else:
+            with open(os.devnull, 'w') as fnull:
+                check_call([self.driver_exe, self.driver_exe],
+                           env=env, stdout=fnull, stderr=fnull)

lldb/packages/Python/lldbsuite/test/api/command-return-object/main.cpp

+#include "lldb/API/SBCommandInterpreter.h"
+#include "lldb/API/SBCommandReturnObject.h"
+#include "lldb/API/SBDebugger.h"
+
+using namespace lldb;
+
+static SBCommandReturnObject subcommand(SBDebugger &dbg, const char *cmd) {
+  SBCommandReturnObject Result;
+  dbg.GetCommandInterpreter().HandleCommand(cmd, Result);
+  return Result;
+}
+
+static class CommandCrasher : public SBCommandPluginInterface {
+public:
+  bool DoExecute(SBDebugger dbg, char **command,
+                 SBCommandReturnObject &result) {
+    result = subcommand(dbg, "help");
+    result = result;
+    if (!result.Succeeded())
+      return false;
+    return true;
+  }
+} crasher;
+
+int main() {
+  SBDebugger::Initialize();
+  SBDebugger dbg = SBDebugger::Create(false /*source_init_files*/);
+  SBCommandInterpreter interp = dbg.GetCommandInterpreter();
+  interp.AddCommand("crasher", &crasher, nullptr /*help*/);
+  SBCommandReturnObject Result;
+  dbg.GetCommandInterpreter().HandleCommand("crasher", Result);
+}

lldb/scripts/Python/python-wrapper.swig

)
     int valid_cast = SWIG_ConvertPtr(data, (void**)&sb_ptr, SWIGTYPE_p_lldb__SBValue, 0);
 
     if (valid_cast == -1)
         return NULL;
 
     return sb_ptr;
 }
 
-// Currently, SBCommandReturnObjectReleaser wraps a unique pointer to an
-// lldb_private::CommandReturnObject. This means that the destructor for the
-// SB object will deallocate its contained CommandReturnObject. Because that
-// object is used as the real return object for Python-based commands, we want
-// it to stay around. Thus, we release the unique pointer before returning from
-// LLDBSwigPythonCallCommand, and to guarantee that the release will occur no
-// matter how we exit from the function, we have a releaser object whose
-// destructor does the right thing for us
-class SBCommandReturnObjectReleaser
-{
-public:
-    SBCommandReturnObjectReleaser (lldb::SBCommandReturnObject &obj) :
-        m_command_return_object_ref (obj)
-    {
-    }
-
-    ~SBCommandReturnObjectReleaser ()
-    {
-        m_command_return_object_ref.Release();
-    }
-private:
-    lldb::SBCommandReturnObject &m_command_return_object_ref;
-};
-
 SWIGEXPORT bool
 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
 )
 {
     using namespace lldb_private;
-    lldb::SBCommandReturnObject cmd_retobj_sb(&cmd_retobj);
-    SBCommandReturnObjectReleaser cmd_retobj_sb_releaser(cmd_retobj_sb);
+    lldb::SBCommandReturnObject cmd_retobj_sb;
+    std::swap(cmd_retobj, SBCommandReturnObject_ref(cmd_retobj_sb));
     lldb::SBDebugger debugger_sb(debugger);
     lldb::SBExecutionContext exe_ctx_sb(exe_ctx_ref_sp);
 
     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;
 
     // pass the pointer-to cmd_retobj_sb or watch the underlying object disappear from under you
     // see comment above for SBCommandReturnObjectReleaser for further details
     auto argc = pfunc.GetNumArguments();
     PythonObject debugger_arg(PyRefType::Owned, SBTypeToSWIGWrapper(debugger_sb));
     PythonObject exe_ctx_arg(PyRefType::Owned, SBTypeToSWIGWrapper(exe_ctx_sb));
     PythonObject cmd_retobj_arg(PyRefType::Owned, SBTypeToSWIGWrapper(&cmd_retobj_sb));
 
     if (argc.count == 5 || argc.is_bound_method || argc.has_varargs)
         pfunc(debugger_arg, PythonString(args), exe_ctx_arg, cmd_retobj_arg, dict);
     else
         pfunc(debugger_arg, PythonString(args), cmd_retobj_arg, dict);
 
+    std::swap(cmd_retobj, SBCommandReturnObject_ref(cmd_retobj_sb));
     return true;
 }
 
 SWIGEXPORT bool
 LLDBSwigPythonCallCommandObject
 (
     PyObject *implementor,
     lldb::DebuggerSP& debugger,
     const char* args,
     lldb_private::CommandReturnObject& cmd_retobj,
     lldb::ExecutionContextRefSP exe_ctx_ref_sp
 )
 {
     using namespace lldb_private;
-    lldb::SBCommandReturnObject cmd_retobj_sb(&cmd_retobj);
-    SBCommandReturnObjectReleaser cmd_retobj_sb_releaser(cmd_retobj_sb);
+    lldb::SBCommandReturnObject cmd_retobj_sb;
+    std::swap(cmd_retobj, SBCommandReturnObject_ref(cmd_retobj_sb));
     lldb::SBDebugger debugger_sb(debugger);
     lldb::SBExecutionContext exe_ctx_sb(exe_ctx_ref_sp);
 
     PyErr_Cleaner py_err_cleaner(true);
 
     PythonObject self(PyRefType::Borrowed, implementor);
     auto pfunc = self.ResolveName<PythonCallable>("__call__");
 
     if (!pfunc.IsAllocated())
         return false;
 
     // pass the pointer-to cmd_retobj_sb or watch the underlying object disappear from under you
     // see comment above for SBCommandReturnObjectReleaser for further details
     PythonObject debugger_arg(PyRefType::Owned, SBTypeToSWIGWrapper(debugger_sb));
     PythonObject exe_ctx_arg(PyRefType::Owned, SBTypeToSWIGWrapper(exe_ctx_sb));
     PythonObject cmd_retobj_arg(PyRefType::Owned, SBTypeToSWIGWrapper(&cmd_retobj_sb));
 
     pfunc(debugger_arg, PythonString(args), exe_ctx_arg, cmd_retobj_arg);
 
+    std::swap(cmd_retobj, SBCommandReturnObject_ref(cmd_retobj_sb));
     return true;
 }
 
 SWIGEXPORT void*
 LLDBSWIGPythonCreateOSPlugin
 (
     const char *python_class_name,
     const char *session_dictionary_name,

 }
 %}
 
 
 %runtime %{
 // Forward declaration to be inserted at the start of LLDBWrapPython.h
 #include "lldb/API/SBDebugger.h"
 #include "lldb/API/SBValue.h"
+#include "lldb/Interpreter/CommandReturnObject.h"
 
 SWIGEXPORT lldb::ValueObjectSP
 LLDBSWIGPython_GetValueObjectSPFromSBValue (void* data)
 {
     lldb::ValueObjectSP valobj_sp;
     if (data)
     {
         lldb::SBValue* sb_ptr = (lldb::SBValue *)data;

lldb/source/API/SBCommandInterpreter.cpp

CommandPluginInterfaceImplementation(CommandInterpreter &interpreter,
                                        uint32_t flags = 0)
       : CommandObjectParsed(interpreter, name, help, syntax, flags),
         m_backend(backend) {}
 
   bool IsRemovable() const override { return true; }
 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
-    SBCommandReturnObject sb_return(&result);
+    SBCommandReturnObject sb_return;
+    std::swap(result, SBCommandReturnObject_ref(sb_return));
     SBCommandInterpreter sb_interpreter(&m_interpreter);
     SBDebugger debugger_sb(m_interpreter.GetDebugger().shared_from_this());
     bool ret = m_backend->DoExecute(
         debugger_sb, (char **)command.GetArgumentVector(), sb_return);
-    sb_return.Release();
+    std::swap(result, SBCommandReturnObject_ref(sb_return));
     return ret;
   }
   std::shared_ptr<lldb::SBCommandPluginInterface> m_backend;
 };
 
 SBCommandInterpreter::SBCommandInterpreter(CommandInterpreter *interpreter)
     : m_opaque_ptr(interpreter) {
   LLDB_RECORD_CONSTRUCTOR(SBCommandInterpreter,
                           (lldb_private::CommandInterpreter *), interpreter);

lldb/source/API/SBCommandReturnObject.cpp

 #include "lldb/API/SBStream.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Status.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
+lldb_private::CommandReturnObject &
+lldb_private::SBCommandReturnObject_ref(SBCommandReturnObject &sb_cmd) {
+  return sb_cmd.ref();
+}
+
 SBCommandReturnObject::SBCommandReturnObject()
     : m_opaque_up(new CommandReturnObject()) {
   LLDB_RECORD_CONSTRUCTOR_NO_ARGS(SBCommandReturnObject);
 }
 
 SBCommandReturnObject::SBCommandReturnObject(const SBCommandReturnObject &rhs)
     : m_opaque_up() {
   LLDB_RECORD_CONSTRUCTOR(SBCommandReturnObject,
                           (const lldb::SBCommandReturnObject &), rhs);
 
   m_opaque_up = clone(rhs.m_opaque_up);
 }
 
-SBCommandReturnObject::SBCommandReturnObject(CommandReturnObject *ptr)
-    : m_opaque_up(ptr) {
-  LLDB_RECORD_CONSTRUCTOR(SBCommandReturnObject,
-                          (lldb_private::CommandReturnObject *), ptr);
-}
-
 SBCommandReturnObject::~SBCommandReturnObject() = default;
 
-CommandReturnObject *SBCommandReturnObject::Release() {
-  LLDB_RECORD_METHOD_NO_ARGS(lldb_private::CommandReturnObject *,
-                             SBCommandReturnObject, Release);
-
-  return LLDB_RECORD_RESULT(m_opaque_up.release());
-}
-
 const SBCommandReturnObject &SBCommandReturnObject::
 operator=(const SBCommandReturnObject &rhs) {
   LLDB_RECORD_METHOD(
       const lldb::SBCommandReturnObject &,
       SBCommandReturnObject, operator=,(const lldb::SBCommandReturnObject &),
       rhs);
 
   if (this != &rhs)
if (num_bytes)
       return ::fprintf(fh, "%s", GetError());
   }
   return 0;
 }
 
 void SBCommandReturnObject::Clear() {
   LLDB_RECORD_METHOD_NO_ARGS(void, SBCommandReturnObject, Clear);
 
   if (m_opaque_up)
     m_opaque_up->Clear();
 }
 
 lldb::ReturnStatus SBCommandReturnObject::GetStatus() {
   LLDB_RECORD_METHOD_NO_ARGS(lldb::ReturnStatus, SBCommandReturnObject,
                              GetStatus);
 
   return (m_opaque_up ? m_opaque_up->GetStatus() : lldb::eReturnStatusInvalid);
 }

 namespace lldb_private {
 namespace repro {
 
 template <>
 void RegisterMethods<SBCommandReturnObject>(Registry &R) {
   LLDB_REGISTER_CONSTRUCTOR(SBCommandReturnObject, ());
   LLDB_REGISTER_CONSTRUCTOR(SBCommandReturnObject,
                             (const lldb::SBCommandReturnObject &));
-  LLDB_REGISTER_CONSTRUCTOR(SBCommandReturnObject,
-                            (lldb_private::CommandReturnObject *));
-  LLDB_REGISTER_METHOD(lldb_private::CommandReturnObject *,
-                       SBCommandReturnObject, Release, ());
   LLDB_REGISTER_METHOD(
       const lldb::SBCommandReturnObject &,
       SBCommandReturnObject, operator=,(const lldb::SBCommandReturnObject &));
   LLDB_REGISTER_METHOD_CONST(bool, SBCommandReturnObject, IsValid, ());
   LLDB_REGISTER_METHOD_CONST(bool, SBCommandReturnObject, operator bool, ());
   LLDB_REGISTER_METHOD(const char *, SBCommandReturnObject, GetOutput, ());
   LLDB_REGISTER_METHOD(const char *, SBCommandReturnObject, GetError, ());
   LLDB_REGISTER_METHOD(size_t, SBCommandReturnObject, GetOutputSize, ());