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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D67589#1673771 <https://reviews.llvm.org/D67589#1673771>, @jankratochvil wrote:

> In D67589#1673756 <https://reviews.llvm.org/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).


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67589