[Lldb-commits] [PATCH] D134033: [lldb/Plugins] Improve error reporting with reading/writing memory in a Scripted Process (WIP)

Mon Sep 19 07:19:03 PDT 2022

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