labath added inline comments.
================
Comment at: lldb/include/lldb/API/SBError.h:95
private:
- std::unique_ptr<lldb_private::Status> m_opaque_up;
+ std::shared_ptr<lldb_private::Status> m_opaque_sp;
----------------
This is technically an ABI break (changes `sizeof(SBError)`). I don't care, but
someone might.
================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.cpp:27
template <typename T> const char *GetPythonValueFormatString(T t);
+// FIXME: This should probably be a PyObject * instead of PythonObject
+template <> const char *GetPythonValueFormatString(python::PythonObject*) {
return "O"; }
----------------
Yes, it should.
================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h:35
namespace lldb_private {
+ namespace python {
+ typedef struct swig_type_info swig_type_info;
----------------
we don't indent namespaces
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:186
+ StatusSP status_sp = std::make_shared<Status>(error);
+ PythonObject* sb_error = new PythonObject(ToSWIGWrapper(status_sp));
+
----------------
mib wrote:
> @labath In order to pass down the `Status&` to python, I create a `StatusSP`
> (to avoid leaking the `lldb_private::Status` type to python), and turn that
> into a `python::PyObject` by calling `ToSWIGWrapper`. This is why I moved its
> declaration to `SWIGPythonBridge.h`.
>
> Finally, the `python::PyObject` is wrapped into another `PythonObject*` so it
> can be passed to `GetPythonValueFormatString` and `PyObject_CallMethod` in
> `ScriptedPythonInterface::Dispatch`
>
> I tried to follow the logic explained in 7f09ab0, however, when
> `ToSWIGWrapper` is called at runtime, it crashes in Python:
>
> ```
> * thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS
> (code=1, address=0x10)
> frame #0: 0x00000001056f3c3c Python`PyTuple_New + 212
> frame #1: 0x0000000135524720
> liblldb.16.0.0git.dylib`SWIG_Python_NewShadowInstance(data=0x00006000011d6340,
> swig_this=0x0000000105ec5d70) at LLDBWrapPython.cpp:2284:28
> * frame #2: 0x0000000135515a00
> liblldb.16.0.0git.dylib`SWIG_Python_NewPointerObj(self=0x0000000000000000,
> ptr=0x0000600001dc4040, type=0x000000014448c140, flags=1) at
> LLDBWrapPython.cpp:2395:22
> frame #3: 0x00000001355157f4
> liblldb.16.0.0git.dylib`lldb_private::python::ToSWIGHelper(obj=0x0000600001dc4040,
> info=0x000000014448c140) at python-swigsafecast.swig:5:29
> frame #4: 0x0000000135515e14
> liblldb.16.0.0git.dylib`lldb_private::python::ToSWIGWrapper(status_sp=std::__1::shared_ptr<lldb_private::Status>::element_type
> @ 0x0000600000ac9a18 strong=3 weak=1) at python-swigsafecast.swig:37:10
> frame #5: 0x00000001367383c4
> liblldb.16.0.0git.dylib`lldb_private::ScriptedProcessPythonInterface::ReadMemoryAtAddress(this=0x00006000011cd2c0,
> address=8034160640, size=512, error=0x000000016b35c650) at
> ScriptedProcessPythonInterface.cpp:161:45
> ```
>
> Am I doing something wrong or maybe I'm missing something ?
>
>
Well.. I'm not sure if this is the cause of the crash, but one problem I see is
the passing of `PythonObject*` to the Dispatch function, which forwards it
unmodified to the `PyObject_CallMethod` function. Python clearly does not know
anything about our lldb_private::python::PythonObject wrappers. You'll either
need to pass a PyObject here, or teach Dispatch how to unwrap a PythonObject.
I also don't think that it was necessary to move all of this code around. I
don't see why would Status be any different from say `StructuredDataImpl`,
which is already passed as a plain reference (no smart pointers). Using an
lldb-private object inside `python-swigsafecast.swig` is still ok, as that code
is still part of liblldb. We already include [[
https://github.com/llvm/llvm-project/blob/main/lldb/bindings/python/python.swig#L121
| some ]] internal headers from the swig files, but I think that in this case
a forward declaration should be enough as well (you may need to add a
`SBError(const Status &)` constructor). The opposite (moving this code to
"regular" files) is not ideal as well, as you've needed to add a bunch of SB
forward declarations into `SWIGPythonBridge`, and split the implementation of
ToSWIGWrapper into two files.
I also don't think the shared_ptr transition is helping with anything (and it
is an ABI break). It might be necessary if the problem was that the SBError
object was being copied, and you couldn't get a hold of the copy which the user
code has modified, but in that case you'd also have to modify the objects copy
constructors to do a shallow (instead of a deep) copy (which in turn could
break other code which was relying on this).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134033/new/
https://reviews.llvm.org/D134033
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
