[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

(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.)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

Had to disable the testcase on Windows: rL373787 



Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Jan Kratochvil via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL373775: [lldb] Fix crash on SBCommandReturnObject  
assignment (authored by jankratochvil, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D67589?vs=222114=223263#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589

Files:
  lldb/trunk/include/lldb/API/SBCommandReturnObject.h
  lldb/trunk/packages/Python/lldbsuite/test/api/command-return-object/Makefile
  
lldb/trunk/packages/Python/lldbsuite/test/api/command-return-object/TestSBCommandReturnObject.py
  lldb/trunk/packages/Python/lldbsuite/test/api/command-return-object/main.cpp
  lldb/trunk/scripts/Python/python-wrapper.swig
  lldb/trunk/source/API/SBCommandInterpreter.cpp
  lldb/trunk/source/API/SBCommandReturnObject.cpp

Index: lldb/trunk/packages/Python/lldbsuite/test/api/command-return-object/main.cpp
===
--- lldb/trunk/packages/Python/lldbsuite/test/api/command-return-object/main.cpp
+++ lldb/trunk/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 , const char *cmd) {
+  SBCommandReturnObject Result;
+  dbg.GetCommandInterpreter().HandleCommand(cmd, Result);
+  return Result;
+}
+
+class CommandCrasher : public SBCommandPluginInterface {
+public:
+  bool DoExecute(SBDebugger dbg, char **command,
+ SBCommandReturnObject ) {
+// 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", , nullptr /*help*/);
+  SBCommandReturnObject Result;
+  dbg.GetCommandInterpreter().HandleCommand("crasher", Result);
+}
Index: lldb/trunk/packages/Python/lldbsuite/test/api/command-return-object/TestSBCommandReturnObject.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/api/command-return-object/TestSBCommandReturnObject.py
+++ lldb/trunk/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/trunk/packages/Python/lldbsuite/test/api/command-return-object/Makefile
===
--- lldb/trunk/packages/Python/lldbsuite/test/api/command-return-object/Makefile
+++ lldb/trunk/packages/Python/lldbsuite/test/api/command-return-object/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/trunk/source/API/SBCommandReturnObject.cpp
===
--- lldb/trunk/source/API/SBCommandReturnObject.cpp
+++ lldb/trunk/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 )
+  : m_ptr(), m_owned(false) {}
+  SBCommandReturnObjectImpl(const SBCommandReturnObjectImpl )
+  : m_ptr(new CommandReturnObject(*rhs.m_ptr)), m_owned(rhs.m_owned) {}
+  SBCommandReturnObjectImpl =(const 

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: 
lldb/packages/Python/lldbsuite/test/api/command-return-object/Makefile:1
+MAKE_DSYM := NO
+

jankratochvil wrote:
> jankratochvil wrote:
> > labath wrote:
> > > I'm pretty sure this line is not needed.
> > Removed in current testsuite: rL373061
> 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.
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.)


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil marked an inline comment as done.
jankratochvil added inline comments.



Comment at: 
lldb/packages/Python/lldbsuite/test/api/command-return-object/Makefile:1
+MAKE_DSYM := NO
+

jankratochvil wrote:
> labath wrote:
> > I'm pretty sure this line is not needed.
> Removed in current testsuite: rL373061
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.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

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 
.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

@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/


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil updated this revision to Diff 222114.

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 )
+  : m_ptr(), m_owned(false) {}
+  SBCommandReturnObjectImpl(const SBCommandReturnObjectImpl )
+  : m_ptr(new CommandReturnObject(*rhs.m_ptr)), m_owned(rhs.m_owned) {}
+  SBCommandReturnObjectImpl =(const SBCommandReturnObjectImpl ) {
+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 *() 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 )
+: m_opaque_up(new SBCommandReturnObjectImpl(ref)) {
+  LLDB_RECORD_CONSTRUCTOR(SBCommandReturnObject,
+  (lldb_private::CommandReturnObject &), ref);
+}
+
 SBCommandReturnObject::SBCommandReturnObject(const SBCommandReturnObject )
 : 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 ::
 operator=(const SBCommandReturnObject ) {
   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(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) {
@@ -121,65 +139,63 @@
 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 

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil marked 2 inline comments as done.
jankratochvil added a comment.

std:




Comment at: 
lldb/packages/Python/lldbsuite/test/api/command-return-object/Makefile:1
+MAKE_DSYM := NO
+

labath wrote:
> I'm pretty sure this line is not needed.
Removed in current testsuite: rL373061


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

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.




Comment at: 
lldb/packages/Python/lldbsuite/test/api/command-return-object/Makefile:1
+MAKE_DSYM := NO
+

I'm pretty sure this line is not needed.



Comment at: lldb/source/API/SBCommandReturnObject.cpp:142
 
-  m_opaque_up->Clear();
+  (*this)->Clear();
 }

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


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Jan Kratochvil via Phabricator via lldb-commits]

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 , @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` | 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 )
+  : m_ptr(), m_owned(false) {}
+  SBCommandReturnObjectImpl(const SBCommandReturnObjectImpl )
+  : m_ptr(new CommandReturnObject(*rhs.m_ptr)), m_owned(rhs.m_owned) {}
+  SBCommandReturnObjectImpl =(const SBCommandReturnObjectImpl ) {
+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 *() 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 )
+: m_opaque_up(new SBCommandReturnObjectImpl(ref)) {
+  LLDB_RECORD_CONSTRUCTOR(SBCommandReturnObject,
+  (lldb_private::CommandReturnObject &), ref);
+}
+
 SBCommandReturnObject::SBCommandReturnObject(const SBCommandReturnObject )
 : 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 ::
 operator=(const SBCommandReturnObject ) {
   LLDB_RECORD_METHOD(
-  const lldb::SBCommandReturnObject &,
+  lldb::SBCommandReturnObject &,
   SBCommandReturnObject, operator=,(const lldb::SBCommandReturnObject &),
   rhs);
 
@@ -59,6 +75,8 

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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 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.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

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 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; }
  };


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

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 m_opaque_up`. But maybe 
> this discussion goes too far from this patch.

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

  lldb_private::CommandReturnObjectImpl {
bool owned;
std::unique_ptr 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).


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil updated this revision to Diff 220649.
jankratochvil marked an inline comment as done.

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
@@ -31,21 +31,8 @@
   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 ::
 operator=(const SBCommandReturnObject ) {
   LLDB_RECORD_METHOD(
@@ -338,10 +325,6 @@
   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 &));
Index: lldb/source/API/SBCommandInterpreter.cpp
===
--- lldb/source/API/SBCommandInterpreter.cpp
+++ lldb/source/API/SBCommandInterpreter.cpp
@@ -162,12 +162,13 @@
 
 protected:
   bool DoExecute(Args , CommandReturnObject ) override {
-SBCommandReturnObject sb_return();
+SBCommandReturnObject sb_return;
+std::swap(result, sb_return.ref());
 SBCommandInterpreter sb_interpreter(_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, sb_return.ref());
 return ret;
   }
   std::shared_ptr 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 ) :
-m_command_return_object_ref (obj)
-{
-}
-
-~SBCommandReturnObjectReleaser ()
-{
-m_command_return_object_ref.Release();
-}
-private:
-lldb::SBCommandReturnObject _command_return_object_ref;
-};
-
 SWIGEXPORT bool
 LLDBSwigPythonCallCommand
 (
@@ -686,8 +662,8 @@
 )
 {
 using namespace lldb_private;
-lldb::SBCommandReturnObject cmd_retobj_sb(_retobj);
-SBCommandReturnObjectReleaser cmd_retobj_sb_releaser(cmd_retobj_sb);
+lldb::SBCommandReturnObject cmd_retobj_sb;
+std::swap(cmd_retobj, cmd_retobj_sb.ref());
 lldb::SBDebugger debugger_sb(debugger);
 lldb::SBExecutionContext exe_ctx_sb(exe_ctx_ref_sp);
 
@@ -710,6 +686,7 @@
 else
 pfunc(debugger_arg, PythonString(args), cmd_retobj_arg, dict);
 
+std::swap(cmd_retobj, cmd_retobj_sb.ref());
 return true;
 }
 
@@ -724,8 +701,8 @@
 )
 {
 using namespace lldb_private;
-lldb::SBCommandReturnObject cmd_retobj_sb(_retobj);
-SBCommandReturnObjectReleaser cmd_retobj_sb_releaser(cmd_retobj_sb);
+lldb::SBCommandReturnObject cmd_retobj_sb;
+

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil marked 6 inline comments as done.
jankratochvil added a comment.

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 m_opaque_up`. But maybe 
this discussion goes too far from this patch.




Comment at: lldb/include/lldb/API/SBCommandReturnObject.h:22-27
+namespace lldb_private {
+// Wrap SBCommandReturnObject::ref() so that any LLDB internal function can
+// call it but still no SB API users can call it.
+CommandReturnObject &
+SBCommandReturnObject_ref(lldb::SBCommandReturnObject _cmd);
+} // namespace lldb_private

clayborg wrote:
> Easier to just make SBCommandReturnObject::ref() protected and friend any 
> users or just make it public?
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.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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 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.




Comment at: 
lldb/packages/Python/lldbsuite/test/api/command-return-object/main.cpp:18
+result = subcommand(dbg, "help");
+result = result;
+if (!result.Succeeded())

jankratochvil wrote:
> jankratochvil wrote:
> > labath wrote:
> > > Is that intentional? If so, why?
> > It has no purpose there for this bugfix but when already writing a testcase 
> > I wanted to test this generally fragile functionality.
> > 
> 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.
> 
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.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/include/lldb/API/SBCommandReturnObject.h:22-27
+namespace lldb_private {
+// Wrap SBCommandReturnObject::ref() so that any LLDB internal function can
+// call it but still no SB API users can call it.
+CommandReturnObject &
+SBCommandReturnObject_ref(lldb::SBCommandReturnObject _cmd);
+} // namespace lldb_private

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



Comment at: lldb/include/lldb/API/SBCommandReturnObject.h:99-100
   friend class SBOptions;
+  friend lldb_private::CommandReturnObject &
+  lldb_private::SBCommandReturnObject_ref(SBCommandReturnObject _cmd);
 

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



Comment at: lldb/include/lldb/API/SBCommandReturnObject.h:108
 
   lldb_private::CommandReturnObject () const;
 

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



Comment at: lldb/source/API/SBCommandReturnObject.cpp:21-24
+lldb_private::CommandReturnObject &
+lldb_private::SBCommandReturnObject_ref(SBCommandReturnObject _cmd) {
+  return sb_cmd.ref();
+}

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


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil marked an inline comment as done.
jankratochvil added inline comments.



Comment at: 
lldb/packages/Python/lldbsuite/test/api/command-return-object/main.cpp:18
+result = subcommand(dbg, "help");
+result = result;
+if (!result.Succeeded())

jankratochvil wrote:
> labath wrote:
> > Is that intentional? If so, why?
> It has no purpose there for this bugfix but when already writing a testcase I 
> wanted to test this generally fragile functionality.
> 
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.



Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil updated this revision to Diff 220505.

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,6 +18,11 @@
 using namespace lldb;
 using namespace lldb_private;
 
+lldb_private::CommandReturnObject &
+lldb_private::SBCommandReturnObject_ref(SBCommandReturnObject _cmd) {
+  return sb_cmd.ref();
+}
+
 SBCommandReturnObject::SBCommandReturnObject()
 : m_opaque_up(new CommandReturnObject()) {
   LLDB_RECORD_CONSTRUCTOR_NO_ARGS(SBCommandReturnObject);
@@ -31,21 +36,8 @@
   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 ::
 operator=(const SBCommandReturnObject ) {
   LLDB_RECORD_METHOD(
@@ -338,10 +330,6 @@
   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 &));
Index: lldb/source/API/SBCommandInterpreter.cpp
===
--- lldb/source/API/SBCommandInterpreter.cpp
+++ lldb/source/API/SBCommandInterpreter.cpp
@@ -162,12 +162,13 @@
 
 protected:
   bool DoExecute(Args , CommandReturnObject ) override {
-SBCommandReturnObject sb_return();
+SBCommandReturnObject sb_return;
+std::swap(result, SBCommandReturnObject_ref(sb_return));
 SBCommandInterpreter sb_interpreter(_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 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 ) :
-m_command_return_object_ref (obj)
-{
-}
-
-~SBCommandReturnObjectReleaser ()
-{
-m_command_return_object_ref.Release();
-}
-private:
-lldb::SBCommandReturnObject _command_return_object_ref;
-};
-
 SWIGEXPORT bool
 LLDBSwigPythonCallCommand
 (
@@ -686,8 +662,8 @@
 )
 {
 using namespace lldb_private;
-lldb::SBCommandReturnObject cmd_retobj_sb(_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);
 
@@ -710,6 +686,7 @@
 else
 

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil marked 3 inline comments as done.
jankratochvil added a comment.

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 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.




Comment at: 
lldb/packages/Python/lldbsuite/test/api/command-return-object/main.cpp:18
+result = subcommand(dbg, "help");
+result = result;
+if (!result.Succeeded())

labath wrote:
> Is that intentional? If so, why?
It has no purpose there for this bugfix but when already writing a testcase I 
wanted to test this generally fragile functionality.




Comment at: lldb/source/API/SBCommandInterpreter.cpp:165-172
+SBCommandReturnObject sb_return;
+std::swap(result, SBCommandReturnObject_ref(sb_return));
 SBCommandInterpreter sb_interpreter(_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, SBCommandReturnObject_ref(sb_return));

clayborg wrote:
> Could this code just create a local SBCommandReturnObject and then copy the 
> CommandReturnObject back into "result"?
> 
> ```
>  bool DoExecute(Args , CommandReturnObject ) override {
> SBCommandReturnObject sb_return;
> SBCommandInterpreter sb_interpreter(_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;
> }
> ```
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 ) override {
...
1242result.SetStatus(eReturnStatusInvalid);
 - ctored CommandReturnObject has eReturnStatusStarted.
1244if (!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) {
1253if (result.GetOutputData().empty())
1254  result.SetStatus(eReturnStatusSuccessFinishNoResult);
1255else
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.



Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/source/API/SBCommandInterpreter.cpp:165-172
+SBCommandReturnObject sb_return;
+std::swap(result, SBCommandReturnObject_ref(sb_return));
 SBCommandInterpreter sb_interpreter(_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, SBCommandReturnObject_ref(sb_return));

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

```
 bool DoExecute(Args , CommandReturnObject ) override {
SBCommandReturnObject sb_return;
SBCommandInterpreter sb_interpreter(_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;
}
```


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Pavel Labath via Phabricator via lldb-commits]

labath added a reviewer: jingham.
labath added a comment.

+ 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 inside SBCommandRO and encoding the ownership into the 
deleter of the shared_ptr (regular deleter => owned, noop deleter => unowned).




Comment at: lldb/include/lldb/API/SBCommandReturnObject.h:24
+CommandReturnObject &
+SBCommandReturnObject_ref(lldb::SBCommandReturnObject _cmd);
+}

Add a comment to explain the purpose of this function?



Comment at: 
lldb/packages/Python/lldbsuite/test/api/command-return-object/main.cpp:18
+result = subcommand(dbg, "help");
+result = result;
+if (!result.Succeeded())

Is that intentional? If so, why?



Comment at: 
lldb/packages/Python/lldbsuite/test/api/command-return-object/main.cpp:23
+  }
+} crasher;
+

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


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil created this revision.
jankratochvil added reviewers: labath, JDevlieghere.
jankratochvil added a project: LLDB.
Herald added a subscriber: abidh.

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

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

That is because `SBCommandReturnObject ` gets initialized inside LLDB by:

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

This is somehow a most simple fix. I do not like it much.
I think there could be two implementations for superclass 
`SBCommandReturnObject` for internal storage vs. external storage but that 
looked overcomplicated to me.
(Also sure there is missing a move-assignment operator.)


Repository:
  rLLDB LLDB

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,6 +18,11 @@
 using namespace lldb;
 using namespace lldb_private;
 
+lldb_private::CommandReturnObject &
+lldb_private::SBCommandReturnObject_ref(SBCommandReturnObject _cmd) {
+  return sb_cmd.ref();
+}
+
 SBCommandReturnObject::SBCommandReturnObject()
 : m_opaque_up(new CommandReturnObject()) {
   LLDB_RECORD_CONSTRUCTOR_NO_ARGS(SBCommandReturnObject);
@@ -31,21 +36,8 @@
   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 ::
 operator=(const SBCommandReturnObject ) {
   LLDB_RECORD_METHOD(
@@ -338,10 +330,6 @@
   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 &));
Index: lldb/source/API/SBCommandInterpreter.cpp
===
--- lldb/source/API/SBCommandInterpreter.cpp
+++ lldb/source/API/SBCommandInterpreter.cpp
@@ -162,12 +162,13 @@
 
 protected:
   bool DoExecute(Args , CommandReturnObject ) override {
-SBCommandReturnObject sb_return();
+SBCommandReturnObject sb_return;
+std::swap(result, SBCommandReturnObject_ref(sb_return));
 SBCommandInterpreter sb_interpreter(_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 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