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 ↗

(On Diff #220231)

Add a comment to explain the purpose of this function?

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

18 ↗

(On Diff #220231)

Is that intentional? If so, why?

23 ↗

(On Diff #220231)

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

[clayborg]

lldb/source/API/SBCommandInterpreter.cpp

165–172 ↗

(On Diff #220231)

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 ↗

(On Diff #220231)

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 ↗

(On Diff #220231)

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 ↗

(On Diff #220231)

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 ↗

(On Diff #220505)

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

99–100 ↗

(On Diff #220505)

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

108 ↗

(On Diff #220505)

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

lldb/source/API/SBCommandReturnObject.cpp

21–24 ↗

(On Diff #220505)

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 ↗

(On Diff #220231)

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 ↗

(On Diff #220505)

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

1 ↗

(On Diff #222021)

I'm pretty sure this line is not needed.

lldb/source/API/SBCommandReturnObject.cpp

142 ↗

(On Diff #222021)

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

1 ↗

(On Diff #222021)

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

1 ↗

(On Diff #222021)

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

1 ↗

(On Diff #222021)

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/trunk/include/lldb/API/SBCommandReturnObject.h

 #define LLDB_SBCommandReturnObject_h_
 
 #include <stdio.h>
 
 #include <memory>
 
 #include "lldb/API/SBDefines.h"
 
+namespace lldb_private {
+class SBCommandReturnObjectImpl;
+};
+
 namespace lldb {
 
 class LLDB_API SBCommandReturnObject {
 public:
   SBCommandReturnObject();
 
+  SBCommandReturnObject(lldb_private::CommandReturnObject &ref);
+
+  // rvalue ctor+assignment are incompatible with Reproducers.
+
   SBCommandReturnObject(const lldb::SBCommandReturnObject &rhs);
 
   ~SBCommandReturnObject();
 
-  const lldb::SBCommandReturnObject &
+  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:
 
   const char *GetError(bool only_if_no_immediate);
 
   void SetError(lldb::SBError &error,
                 const char *fallback_error_cstr = nullptr);
 
   void SetError(const char *error_cstr);
 
+  // ref() is internal for LLDB only.
+  lldb_private::CommandReturnObject &ref() const;
+
 protected:
   friend class SBCommandInterpreter;
   friend class SBOptions;
 
   lldb_private::CommandReturnObject *operator->() const;
 
   lldb_private::CommandReturnObject *get() const;
 
   lldb_private::CommandReturnObject &operator*() const;
 
-  lldb_private::CommandReturnObject &ref() const;
-
 private:
-  std::unique_ptr<lldb_private::CommandReturnObject> m_opaque_up;
+  std::unique_ptr<lldb_private::SBCommandReturnObjectImpl> m_opaque_up;
 };
 
 } // namespace lldb
 
 #endif // LLDB_SBCommandReturnObject_h_

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

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

lldb/trunk/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/trunk/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;
+}
+
+class CommandCrasher : public SBCommandPluginInterface {
+public:
+  bool DoExecute(SBDebugger dbg, char **command,
+                 SBCommandReturnObject &result) {
+    // Test assignment from a different SBCommandReturnObject instance.
+    result = subcommand(dbg, "help");
+    // Test also whether self-assignment is handled correctly.
+    result = result;
+    if (!result.Succeeded())
+      return false;
+    return true;
+  }
+};
+
+int main() {
+  SBDebugger::Initialize();
+  SBDebugger dbg = SBDebugger::Create(false /*source_init_files*/);
+  SBCommandInterpreter interp = dbg.GetCommandInterpreter();
+  static CommandCrasher crasher;
+  interp.AddCommand("crasher", &crasher, nullptr /*help*/);
+  SBCommandReturnObject Result;
+  dbg.GetCommandInterpreter().HandleCommand("crasher", Result);
+}

lldb/trunk/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(cmd_retobj);
     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())
(
     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(cmd_retobj);
     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__");
 

lldb/trunk/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(result);
     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();
     return ret;
   }
   std::shared_ptr<lldb::SBCommandPluginInterface> m_backend;
 };
 
 SBCommandInterpreter::SBCommandInterpreter(CommandInterpreter *interpreter)
     : m_opaque_ptr(interpreter) {
   LLDB_RECORD_CONSTRUCTOR(SBCommandInterpreter,

lldb/trunk/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;
 
+class lldb_private::SBCommandReturnObjectImpl {
+public:
+  SBCommandReturnObjectImpl()
+      : m_ptr(new CommandReturnObject()), m_owned(true) {}
+  SBCommandReturnObjectImpl(CommandReturnObject &ref)
+      : m_ptr(&ref), m_owned(false) {}
+  SBCommandReturnObjectImpl(const SBCommandReturnObjectImpl &rhs)
+      : m_ptr(new CommandReturnObject(*rhs.m_ptr)), m_owned(rhs.m_owned) {}
+  SBCommandReturnObjectImpl &operator=(const SBCommandReturnObjectImpl &rhs) {
+    SBCommandReturnObjectImpl copy(rhs);
+    std::swap(*this, copy);
+    return *this;
+  }
+  // rvalue ctor+assignment are not used by SBCommandReturnObject.
+  ~SBCommandReturnObjectImpl() {
+    if (m_owned)
+      delete m_ptr;
+  }
+
+  CommandReturnObject &operator*() const { return *m_ptr; }
+
+private:
+  CommandReturnObject *m_ptr;
+  bool m_owned;
+};
+
 SBCommandReturnObject::SBCommandReturnObject()
-    : m_opaque_up(new CommandReturnObject()) {
+    : m_opaque_up(new SBCommandReturnObjectImpl()) {
   LLDB_RECORD_CONSTRUCTOR_NO_ARGS(SBCommandReturnObject);
 }
 
+SBCommandReturnObject::SBCommandReturnObject(CommandReturnObject &ref)
+    : m_opaque_up(new SBCommandReturnObjectImpl(ref)) {
+  LLDB_RECORD_CONSTRUCTOR(SBCommandReturnObject,
+                          (lldb_private::CommandReturnObject &), ref);
+}
+
 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);
-  assert(ptr != nullptr);
-}
-
-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::
+SBCommandReturnObject &SBCommandReturnObject::
 operator=(const SBCommandReturnObject &rhs) {
   LLDB_RECORD_METHOD(
-      const lldb::SBCommandReturnObject &,
+      lldb::SBCommandReturnObject &,
       SBCommandReturnObject, operator=,(const lldb::SBCommandReturnObject &),
       rhs);
 
   if (this != &rhs)
     m_opaque_up = clone(rhs.m_opaque_up);
   return LLDB_RECORD_RESULT(*this);
 }
 
+SBCommandReturnObject::~SBCommandReturnObject() = default;
+
 bool SBCommandReturnObject::IsValid() const {
   LLDB_RECORD_METHOD_CONST_NO_ARGS(bool, SBCommandReturnObject, IsValid);
   return this->operator bool();
 }
 SBCommandReturnObject::operator bool() const {
   LLDB_RECORD_METHOD_CONST_NO_ARGS(bool, SBCommandReturnObject, operator bool);
 
   // This method is not useful but it needs to stay to keep SB API stable.
   return true;
 }
 
 const char *SBCommandReturnObject::GetOutput() {
   LLDB_RECORD_METHOD_NO_ARGS(const char *, SBCommandReturnObject, GetOutput);
 
-  ConstString output(m_opaque_up->GetOutputData());
+  ConstString output(ref().GetOutputData());
   return output.AsCString(/*value_if_empty*/ "");
 }
 
 const char *SBCommandReturnObject::GetError() {
   LLDB_RECORD_METHOD_NO_ARGS(const char *, SBCommandReturnObject, GetError);
 
-  ConstString output(m_opaque_up->GetErrorData());
+  ConstString output(ref().GetErrorData());
   return output.AsCString(/*value_if_empty*/ "");
 }
 
 size_t SBCommandReturnObject::GetOutputSize() {
   LLDB_RECORD_METHOD_NO_ARGS(size_t, SBCommandReturnObject, GetOutputSize);
 
-  return m_opaque_up->GetOutputData().size();
+  return ref().GetOutputData().size();
 }
 
 size_t SBCommandReturnObject::GetErrorSize() {
   LLDB_RECORD_METHOD_NO_ARGS(size_t, SBCommandReturnObject, GetErrorSize);
 
-  return m_opaque_up->GetErrorData().size();
+  return ref().GetErrorData().size();
 }
 
 size_t SBCommandReturnObject::PutOutput(FILE *fh) {
   LLDB_RECORD_METHOD(size_t, SBCommandReturnObject, PutOutput, (FILE *), fh);
 
   if (fh) {
     size_t num_bytes = GetOutputSize();
     if (num_bytes)
if (num_bytes)
       return ::fprintf(fh, "%s", GetError());
   }
   return 0;
 }
 
 void SBCommandReturnObject::Clear() {
   LLDB_RECORD_METHOD_NO_ARGS(void, SBCommandReturnObject, Clear);
 
-  m_opaque_up->Clear();
+  ref().Clear();
 }
 
 lldb::ReturnStatus SBCommandReturnObject::GetStatus() {
   LLDB_RECORD_METHOD_NO_ARGS(lldb::ReturnStatus, SBCommandReturnObject,
                              GetStatus);
 
-  return m_opaque_up->GetStatus();
+  return ref().GetStatus();
 }
 
 void SBCommandReturnObject::SetStatus(lldb::ReturnStatus status) {
   LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetStatus,
                      (lldb::ReturnStatus), status);
 
-  m_opaque_up->SetStatus(status);
+  ref().SetStatus(status);
 }
 
 bool SBCommandReturnObject::Succeeded() {
   LLDB_RECORD_METHOD_NO_ARGS(bool, SBCommandReturnObject, Succeeded);
 
-  return m_opaque_up->Succeeded();
+  return ref().Succeeded();
 }
 
 bool SBCommandReturnObject::HasResult() {
   LLDB_RECORD_METHOD_NO_ARGS(bool, SBCommandReturnObject, HasResult);
 
-  return m_opaque_up->HasResult();
+  return ref().HasResult();
 }
 
 void SBCommandReturnObject::AppendMessage(const char *message) {
   LLDB_RECORD_METHOD(void, SBCommandReturnObject, AppendMessage, (const char *),
                      message);
 
-  m_opaque_up->AppendMessage(message);
+  ref().AppendMessage(message);
 }
 
 void SBCommandReturnObject::AppendWarning(const char *message) {
   LLDB_RECORD_METHOD(void, SBCommandReturnObject, AppendWarning, (const char *),
                      message);
 
-  m_opaque_up->AppendWarning(message);
+  ref().AppendWarning(message);
 }
 
 CommandReturnObject *SBCommandReturnObject::operator->() const {
-  return m_opaque_up.get();
+  return &**m_opaque_up;
 }
 
 CommandReturnObject *SBCommandReturnObject::get() const {
-  return m_opaque_up.get();
+  return &**m_opaque_up;
 }
 
 CommandReturnObject &SBCommandReturnObject::operator*() const {
-  assert(m_opaque_up.get());
-  return *(m_opaque_up.get());
+  return **m_opaque_up;
 }
 
 CommandReturnObject &SBCommandReturnObject::ref() const {
-  assert(m_opaque_up.get());
-  return *(m_opaque_up.get());
+  return **m_opaque_up;
 }
 
 bool SBCommandReturnObject::GetDescription(SBStream &description) {
   LLDB_RECORD_METHOD(bool, SBCommandReturnObject, GetDescription,
                      (lldb::SBStream &), description);
 
   Stream &strm = description.ref();
 
   description.Printf("Error:  ");
-  lldb::ReturnStatus status = m_opaque_up->GetStatus();
+  lldb::ReturnStatus status = ref().GetStatus();
   if (status == lldb::eReturnStatusStarted)
     strm.PutCString("Started");
   else if (status == lldb::eReturnStatusInvalid)
     strm.PutCString("Invalid");
-  else if (m_opaque_up->Succeeded())
+  else if (ref().Succeeded())
     strm.PutCString("Success");
   else
     strm.PutCString("Fail");
 
   if (GetOutputSize() > 0)
     strm.Printf("\nOutput Message:\n%s", GetOutput());
 
   if (GetErrorSize() > 0)
void SBCommandReturnObject::SetImmediateErrorFile(FILE *fh) {
   SetImmediateErrorFile(fh, false);
 }
 
 void SBCommandReturnObject::SetImmediateOutputFile(FILE *fh,
                                                    bool transfer_ownership) {
   LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetImmediateOutputFile,
                      (FILE *, bool), fh, transfer_ownership);
 
-  m_opaque_up->SetImmediateOutputFile(fh, transfer_ownership);
+  ref().SetImmediateOutputFile(fh, transfer_ownership);
 }
 
 void SBCommandReturnObject::SetImmediateErrorFile(FILE *fh,
                                                   bool transfer_ownership) {
   LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetImmediateErrorFile,
                      (FILE *, bool), fh, transfer_ownership);
 
-  m_opaque_up->SetImmediateErrorFile(fh, transfer_ownership);
+  ref().SetImmediateErrorFile(fh, transfer_ownership);
 }
 
 void SBCommandReturnObject::PutCString(const char *string, int len) {
   LLDB_RECORD_METHOD(void, SBCommandReturnObject, PutCString,
                      (const char *, int), string, len);
 
   if (len == 0 || string == nullptr || *string == 0) {
     return;
   } else if (len > 0) {
     std::string buffer(string, len);
-    m_opaque_up->AppendMessage(buffer.c_str());
+    ref().AppendMessage(buffer.c_str());
   } else
-    m_opaque_up->AppendMessage(string);
+    ref().AppendMessage(string);
 }
 
 const char *SBCommandReturnObject::GetOutput(bool only_if_no_immediate) {
   LLDB_RECORD_METHOD(const char *, SBCommandReturnObject, GetOutput, (bool),
                      only_if_no_immediate);
 
   if (!only_if_no_immediate ||
-      m_opaque_up->GetImmediateOutputStream().get() == nullptr)
+      ref().GetImmediateOutputStream().get() == nullptr)
     return GetOutput();
   return nullptr;
 }
 
 const char *SBCommandReturnObject::GetError(bool only_if_no_immediate) {
   LLDB_RECORD_METHOD(const char *, SBCommandReturnObject, GetError, (bool),
                      only_if_no_immediate);
 
-  if (!only_if_no_immediate ||
-      m_opaque_up->GetImmediateErrorStream().get() == nullptr)
+  if (!only_if_no_immediate || ref().GetImmediateErrorStream().get() == nullptr)
     return GetError();
   return nullptr;
 }
 
 size_t SBCommandReturnObject::Printf(const char *format, ...) {
   va_list args;
   va_start(args, format);
-  size_t result = m_opaque_up->GetOutputStream().PrintfVarArg(format, args);
+  size_t result = ref().GetOutputStream().PrintfVarArg(format, args);
   va_end(args);
   return result;
 }
 
 void SBCommandReturnObject::SetError(lldb::SBError &error,
                                      const char *fallback_error_cstr) {
   LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetError,
                      (lldb::SBError &, const char *), error,
                      fallback_error_cstr);
 
   if (error.IsValid())
-    m_opaque_up->SetError(error.ref(), fallback_error_cstr);
+    ref().SetError(error.ref(), fallback_error_cstr);
   else if (fallback_error_cstr)
-    m_opaque_up->SetError(Status(), fallback_error_cstr);
+    ref().SetError(Status(), fallback_error_cstr);
 }
 
 void SBCommandReturnObject::SetError(const char *error_cstr) {
   LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetError, (const char *),
                      error_cstr);
 
   if (error_cstr)
-    m_opaque_up->SetError(error_cstr);
+    ref().SetError(error_cstr);
 }
 
 namespace lldb_private {
 namespace repro {
 
 template <>
 void RegisterMethods<SBCommandReturnObject>(Registry &R) {
   LLDB_REGISTER_CONSTRUCTOR(SBCommandReturnObject, ());
   LLDB_REGISTER_CONSTRUCTOR(SBCommandReturnObject,
-                            (const lldb::SBCommandReturnObject &));
+                            (lldb_private::CommandReturnObject &));
   LLDB_REGISTER_CONSTRUCTOR(SBCommandReturnObject,
-                            (lldb_private::CommandReturnObject *));
-  LLDB_REGISTER_METHOD(lldb_private::CommandReturnObject *,
-                       SBCommandReturnObject, Release, ());
+                            (const lldb::SBCommandReturnObject &));
   LLDB_REGISTER_METHOD(
-      const lldb::SBCommandReturnObject &,
+      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, ());
   LLDB_REGISTER_METHOD(size_t, SBCommandReturnObject, GetErrorSize, ());
   LLDB_REGISTER_METHOD(size_t, SBCommandReturnObject, PutOutput, (FILE *));