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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D67589.222021.patch
Type: text/x-patch
Size: 17688 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20190926/644fd2fc/attachment-0001.bin>