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

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.



================
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 &sb_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