jankratochvil updated this revision to Diff 222021.
jankratochvil edited the summary of this revision.
jankratochvil added a comment.
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 <https://reviews.llvm.org/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.
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67589/new/
https://reviews.llvm.org/D67589
Files:
lldb/include/lldb/API/SBCommandReturnObject.h
lldb/packages/Python/lldbsuite/test/api/command-return-object/Makefile
lldb/packages/Python/lldbsuite/test/api/command-return-object/TestSBCommandReturnObject.py
lldb/packages/Python/lldbsuite/test/api/command-return-object/main.cpp
lldb/scripts/Python/python-wrapper.swig
lldb/source/API/SBCommandInterpreter.cpp
lldb/source/API/SBCommandReturnObject.cpp
Index: lldb/source/API/SBCommandReturnObject.cpp
===================================================================
--- lldb/source/API/SBCommandReturnObject.cpp
+++ lldb/source/API/SBCommandReturnObject.cpp
@@ -18,11 +18,43 @@
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,
@@ -31,26 +63,10 @@
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);
@@ -59,6 +75,8 @@
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();
@@ -73,27 +91,27 @@
const char *SBCommandReturnObject::GetOutput() {
LLDB_RECORD_METHOD_NO_ARGS(const char *, SBCommandReturnObject, GetOutput);
- ConstString output(m_opaque_up->GetOutputData());
+ ConstString output((*this)->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((*this)->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 (*this)->GetOutputData().size();
}
size_t SBCommandReturnObject::GetErrorSize() {
LLDB_RECORD_METHOD_NO_ARGS(size_t, SBCommandReturnObject, GetErrorSize);
- return m_opaque_up->GetErrorData().size();
+ return (*this)->GetErrorData().size();
}
size_t SBCommandReturnObject::PutOutput(FILE *fh) {
@@ -121,65 +139,63 @@
void SBCommandReturnObject::Clear() {
LLDB_RECORD_METHOD_NO_ARGS(void, SBCommandReturnObject, Clear);
- m_opaque_up->Clear();
+ (*this)->Clear();
}
lldb::ReturnStatus SBCommandReturnObject::GetStatus() {
LLDB_RECORD_METHOD_NO_ARGS(lldb::ReturnStatus, SBCommandReturnObject,
GetStatus);
- return m_opaque_up->GetStatus();
+ return (*this)->GetStatus();
}
void SBCommandReturnObject::SetStatus(lldb::ReturnStatus status) {
LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetStatus,
(lldb::ReturnStatus), status);
- m_opaque_up->SetStatus(status);
+ (*this)->SetStatus(status);
}
bool SBCommandReturnObject::Succeeded() {
LLDB_RECORD_METHOD_NO_ARGS(bool, SBCommandReturnObject, Succeeded);
- return m_opaque_up->Succeeded();
+ return (*this)->Succeeded();
}
bool SBCommandReturnObject::HasResult() {
LLDB_RECORD_METHOD_NO_ARGS(bool, SBCommandReturnObject, HasResult);
- return m_opaque_up->HasResult();
+ return (*this)->HasResult();
}
void SBCommandReturnObject::AppendMessage(const char *message) {
LLDB_RECORD_METHOD(void, SBCommandReturnObject, AppendMessage, (const char *),
message);
- m_opaque_up->AppendMessage(message);
+ (*this)->AppendMessage(message);
}
void SBCommandReturnObject::AppendWarning(const char *message) {
LLDB_RECORD_METHOD(void, SBCommandReturnObject, AppendWarning, (const char *),
message);
- m_opaque_up->AppendWarning(message);
+ (*this)->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) {
@@ -189,12 +205,12 @@
Stream &strm = description.ref();
description.Printf("Error: ");
- lldb::ReturnStatus status = m_opaque_up->GetStatus();
+ lldb::ReturnStatus status = (*this)->GetStatus();
if (status == lldb::eReturnStatusStarted)
strm.PutCString("Started");
else if (status == lldb::eReturnStatusInvalid)
strm.PutCString("Invalid");
- else if (m_opaque_up->Succeeded())
+ else if ((*this)->Succeeded())
strm.PutCString("Success");
else
strm.PutCString("Fail");
@@ -227,7 +243,7 @@
LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetImmediateOutputFile,
(FILE *, bool), fh, transfer_ownership);
- m_opaque_up->SetImmediateOutputFile(fh, transfer_ownership);
+ (*this)->SetImmediateOutputFile(fh, transfer_ownership);
}
void SBCommandReturnObject::SetImmediateErrorFile(FILE *fh,
@@ -235,7 +251,7 @@
LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetImmediateErrorFile,
(FILE *, bool), fh, transfer_ownership);
- m_opaque_up->SetImmediateErrorFile(fh, transfer_ownership);
+ (*this)->SetImmediateErrorFile(fh, transfer_ownership);
}
void SBCommandReturnObject::PutCString(const char *string, int len) {
@@ -246,9 +262,9 @@
return;
} else if (len > 0) {
std::string buffer(string, len);
- m_opaque_up->AppendMessage(buffer.c_str());
+ (*this)->AppendMessage(buffer.c_str());
} else
- m_opaque_up->AppendMessage(string);
+ (*this)->AppendMessage(string);
}
const char *SBCommandReturnObject::GetOutput(bool only_if_no_immediate) {
@@ -256,7 +272,7 @@
only_if_no_immediate);
if (!only_if_no_immediate ||
- m_opaque_up->GetImmediateOutputStream().get() == nullptr)
+ (*this)->GetImmediateOutputStream().get() == nullptr)
return GetOutput();
return nullptr;
}
@@ -266,7 +282,7 @@
only_if_no_immediate);
if (!only_if_no_immediate ||
- m_opaque_up->GetImmediateErrorStream().get() == nullptr)
+ (*this)->GetImmediateErrorStream().get() == nullptr)
return GetError();
return nullptr;
}
@@ -274,7 +290,7 @@
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 = (*this)->GetOutputStream().PrintfVarArg(format, args);
va_end(args);
return result;
}
@@ -286,9 +302,9 @@
fallback_error_cstr);
if (error.IsValid())
- m_opaque_up->SetError(error.ref(), fallback_error_cstr);
+ (*this)->SetError(error.ref(), fallback_error_cstr);
else if (fallback_error_cstr)
- m_opaque_up->SetError(Status(), fallback_error_cstr);
+ (*this)->SetError(Status(), fallback_error_cstr);
}
void SBCommandReturnObject::SetError(const char *error_cstr) {
@@ -296,7 +312,7 @@
error_cstr);
if (error_cstr)
- m_opaque_up->SetError(error_cstr);
+ (*this)->SetError(error_cstr);
}
namespace lldb_private {
@@ -306,13 +322,11 @@
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, ());
Index: lldb/source/API/SBCommandInterpreter.cpp
===================================================================
--- lldb/source/API/SBCommandInterpreter.cpp
+++ lldb/source/API/SBCommandInterpreter.cpp
@@ -162,12 +162,11 @@
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;
Index: lldb/scripts/Python/python-wrapper.swig
===================================================================
--- lldb/scripts/Python/python-wrapper.swig
+++ lldb/scripts/Python/python-wrapper.swig
@@ -650,30 +650,6 @@
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
(
@@ -686,8 +662,7 @@
)
{
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);
@@ -724,8 +699,7 @@
)
{
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);
Index: lldb/packages/Python/lldbsuite/test/api/command-return-object/main.cpp
===================================================================
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/api/command-return-object/main.cpp
@@ -0,0 +1,35 @@
+#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);
+}
Index: lldb/packages/Python/lldbsuite/test/api/command-return-object/TestSBCommandReturnObject.py
===================================================================
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/api/command-return-object/TestSBCommandReturnObject.py
@@ -0,0 +1,31 @@
+"""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)
Index: lldb/packages/Python/lldbsuite/test/api/command-return-object/Makefile
===================================================================
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/api/command-return-object/Makefile
@@ -0,0 +1,5 @@
+MAKE_DSYM := NO
+
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/include/lldb/API/SBCommandReturnObject.h
===================================================================
--- lldb/include/lldb/API/SBCommandReturnObject.h
+++ lldb/include/lldb/API/SBCommandReturnObject.h
@@ -15,23 +15,27 @@
#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;
@@ -86,6 +90,9 @@
void SetError(const char *error_cstr);
+ // ref() is internal for LLDB only.
+ lldb_private::CommandReturnObject &ref() const;
+
protected:
friend class SBCommandInterpreter;
friend class SBOptions;
@@ -96,10 +103,8 @@
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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
