[lldb/Plugins] Improve error reporting with reading memory in Scripted Process

This patch improves the ScriptedPythonInterface::Dispatch method to
support passing lldb_private types to the python implementation.

This will allow, for instance, the Scripted Process python implementation
to report errors when reading memory back to lldb.

To do so, the Dispatch method will transform the private types in the
parameter pack into `PythonObject`s to be able to pass them down to the
python methods.

Then, if the call succeeded, the transformed arguments will be converted
back to their original type and re-assigned in the parameter pack, to
ensure pointers and references behaviours are preserved.

This patch also updates various scripted process python class and tests
to reflect this change.

rdar://100030995

Differential Revision: https://reviews.llvm.org/D134033

Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>

lldb/bindings/python/python-swigsafecast.swig

@@ -5,28 +5,6 @@ PythonObject ToSWIGHelper(void *obj, swig_type_info *info) {
   return {PyRefType::Owned, SWIG_NewPointerObj(obj, info, SWIG_POINTER_OWN)};
 }
 
-/// A class that automatically clears an SB object when it goes out of scope.
-/// Use for cases where the SB object points to a temporary/unowned entity.
-template <typename T> class ScopedPythonObject : PythonObject {
-public:
-  ScopedPythonObject(T *sb, swig_type_info *info)
-      : PythonObject(ToSWIGHelper(sb, info)), m_sb(sb) {}
-  ~ScopedPythonObject() {
-    if (m_sb)
-      *m_sb = T();
-  }
-  ScopedPythonObject(ScopedPythonObject &&rhs)
-      : PythonObject(std::move(rhs)), m_sb(std::exchange(rhs.m_sb, nullptr)) {}
-  ScopedPythonObject(const ScopedPythonObject &) = delete;
-  ScopedPythonObject &operator=(const ScopedPythonObject &) = delete;
-  ScopedPythonObject &operator=(ScopedPythonObject &&) = delete;
-
-  const PythonObject &obj() const { return *this; }
-
-private:
-  T *m_sb;
-};
-
 PythonObject ToSWIGWrapper(std::unique_ptr<lldb::SBValue> value_sb) {
   return ToSWIGHelper(value_sb.release(), SWIGTYPE_p_lldb__SBValue);
 }
@@ -55,6 +33,10 @@ PythonObject ToSWIGWrapper(lldb::BreakpointSP breakpoint_sp) {
                       SWIGTYPE_p_lldb__SBBreakpoint);
 }
 
+PythonObject ToSWIGWrapper(const Status& status) {
+  return ToSWIGHelper(new lldb::SBError(status), SWIGTYPE_p_lldb__SBError);
+}
+
 PythonObject ToSWIGWrapper(std::unique_ptr<lldb::SBStream> stream_sb) {
   return ToSWIGHelper(stream_sb.release(), SWIGTYPE_p_lldb__SBStream);
 }

lldb/examples/python/scripted_process/crashlog_scripted_process.py

@@ -103,7 +103,7 @@ class CrashLogScriptedProcess(ScriptedProcess):
     def get_registers_for_thread(self, tid: int):
         return {}
 
-    def read_memory_at_address(self, addr: int, size: int) -> lldb.SBData:
+    def read_memory_at_address(self, addr: int, size: int, error: lldb.SBError) -> lldb.SBData:
         # NOTE: CrashLogs don't contain any memory.
         return lldb.SBData()
 

lldb/examples/python/scripted_process/scripted_process.py

@@ -98,13 +98,14 @@ class ScriptedProcess(metaclass=ABCMeta):
         pass
 
     @abstractmethod
-    def read_memory_at_address(self, addr, size):
+    def read_memory_at_address(self, addr, size, error):
         """ Get a memory buffer from the scripted process at a certain address,
             of a certain size.
 
         Args:
             addr (int): Address from which we should start reading.
             size (int): Size of the memory to read.
+            error (lldb.SBError): Error object.
 
         Returns:
             lldb.SBData: An `lldb.SBData` buffer with the target byte size and

lldb/include/lldb/API/SBError.h

@@ -23,6 +23,8 @@ public:
 
   SBError(const lldb::SBError &rhs);
 
+  SBError(const lldb_private::Status &error);
+
   ~SBError();
 
   const SBError &operator=(const lldb::SBError &rhs);

lldb/source/API/SBError.cpp

@@ -25,6 +25,11 @@ SBError::SBError(const SBError &rhs) {
   m_opaque_up = clone(rhs.m_opaque_up);
 }
 
+SBError::SBError(const lldb_private::Status &status)
+    : m_opaque_up(new Status(status)) {
+  LLDB_INSTRUMENT_VA(this, status);
+}
+
 SBError::~SBError() = default;
 
 const SBError &SBError::operator=(const SBError &rhs) {

lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h

@@ -23,7 +23,68 @@
 #include "lldb/lldb-types.h"
 #include "llvm/Support/Error.h"
 
+namespace lldb {
+class SBEvent;
+class SBCommandReturnObject;
+class SBValue;
+class SBStream;
+class SBStructuredData;
+} // namespace lldb
+
 namespace lldb_private {
+namespace python {
+
+typedef struct swig_type_info swig_type_info;
+
+python::PythonObject ToSWIGHelper(void *obj, swig_type_info *info);
+
+/// A class that automatically clears an SB object when it goes out of scope.
+/// Use for cases where the SB object points to a temporary/unowned entity.
+template <typename T> class ScopedPythonObject : PythonObject {
+public:
+  ScopedPythonObject(T *sb, swig_type_info *info)
+      : PythonObject(ToSWIGHelper(sb, info)), m_sb(sb) {}
+  ~ScopedPythonObject() {
+    if (m_sb)
+      *m_sb = T();
+  }
+  ScopedPythonObject(ScopedPythonObject &&rhs)
+      : PythonObject(std::move(rhs)), m_sb(std::exchange(rhs.m_sb, nullptr)) {}
+  ScopedPythonObject(const ScopedPythonObject &) = delete;
+  ScopedPythonObject &operator=(const ScopedPythonObject &) = delete;
+  ScopedPythonObject &operator=(ScopedPythonObject &&) = delete;
+
+  const PythonObject &obj() const { return *this; }
+
+private:
+  T *m_sb;
+};
+
+PythonObject ToSWIGWrapper(lldb::ValueObjectSP value_sp);
+PythonObject ToSWIGWrapper(lldb::TargetSP target_sp);
+PythonObject ToSWIGWrapper(lldb::ProcessSP process_sp);
+PythonObject ToSWIGWrapper(lldb::ThreadPlanSP thread_plan_sp);
+PythonObject ToSWIGWrapper(lldb::BreakpointSP breakpoint_sp);
+PythonObject ToSWIGWrapper(const Status &status);
+PythonObject ToSWIGWrapper(const StructuredDataImpl &data_impl);
+PythonObject ToSWIGWrapper(lldb::ThreadSP thread_sp);
+PythonObject ToSWIGWrapper(lldb::StackFrameSP frame_sp);
+PythonObject ToSWIGWrapper(lldb::DebuggerSP debugger_sp);
+PythonObject ToSWIGWrapper(lldb::WatchpointSP watchpoint_sp);
+PythonObject ToSWIGWrapper(lldb::BreakpointLocationSP bp_loc_sp);
+PythonObject ToSWIGWrapper(lldb::ExecutionContextRefSP ctx_sp);
+PythonObject ToSWIGWrapper(const TypeSummaryOptions &summary_options);
+PythonObject ToSWIGWrapper(const SymbolContext &sym_ctx);
+
+PythonObject ToSWIGWrapper(std::unique_ptr<lldb::SBValue> value_sb);
+PythonObject ToSWIGWrapper(std::unique_ptr<lldb::SBStream> stream_sb);
+PythonObject ToSWIGWrapper(std::unique_ptr<lldb::SBStructuredData> data_sb);
+
+python::ScopedPythonObject<lldb::SBCommandReturnObject>
+ToSWIGWrapper(CommandReturnObject &cmd_retobj);
+python::ScopedPythonObject<lldb::SBEvent> ToSWIGWrapper(Event *event);
+
+} // namespace python
 
 // GetPythonValueFormatString provides a system independent type safe way to
 // convert a variable's type into a python value format. Python value formats

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp

@@ -8,6 +8,7 @@
 
 #include "lldb/Host/Config.h"
 #include "lldb/Utility/Log.h"
+#include "lldb/Utility/Status.h"
 #include "lldb/lldb-enumerations.h"
 
 #if LLDB_ENABLE_PYTHON
@@ -120,8 +121,15 @@ ScriptedProcessPythonInterface::GetRegistersForThread(lldb::tid_t tid) {
 
 lldb::DataExtractorSP ScriptedProcessPythonInterface::ReadMemoryAtAddress(
     lldb::addr_t address, size_t size, Status &error) {
-  return Dispatch<lldb::DataExtractorSP>("read_memory_at_address", error,
+  Status py_error;
-                                         address, size);
+  lldb::DataExtractorSP data_sp = Dispatch<lldb::DataExtractorSP>(
+      "read_memory_at_address", py_error, address, size, error);
+
+  // If there was an error on the python call, surface it to the user.
+  if (py_error.Fail())
+    error = py_error;
+
+  return data_sp;
 }
 
 StructuredData::ArraySP ScriptedProcessPythonInterface::GetLoadedImages() {

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.cpp

@@ -55,11 +55,11 @@ Status ScriptedPythonInterface::ExtractValueFromPythonObject<Status>(
     python::PythonObject &p, Status &error) {
   if (lldb::SBError *sb_error = reinterpret_cast<lldb::SBError *>(
           LLDBSWIGPython_CastPyObjectToSBError(p.get())))
-    error = m_interpreter.GetStatusFromSBError(*sb_error);
+    return m_interpreter.GetStatusFromSBError(*sb_error);
   else
     error.SetErrorString("Couldn't cast lldb::SBError to lldb::Status.");
 
-  return error;
+  return {};
 }
 
 template <>

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h

@@ -9,10 +9,14 @@
 #ifndef LLDB_PLUGINS_SCRIPTINTERPRETER_PYTHON_SCRIPTEDPYTHONINTERFACE_H
 #define LLDB_PLUGINS_SCRIPTINTERPRETER_PYTHON_SCRIPTEDPYTHONINTERFACE_H
 
-#include "lldb/Host/Config.h"
-
 #if LLDB_ENABLE_PYTHON
 
+#include <sstream>
+#include <tuple>
+#include <type_traits>
+#include <utility>
+
+#include "lldb/Host/Config.h"
 #include "lldb/Interpreter/ScriptedInterface.h"
 #include "lldb/Utility/DataBufferHeap.h"
 
@@ -34,7 +38,7 @@ protected:
   }
 
   template <typename T = StructuredData::ObjectSP, typename... Args>
-  T Dispatch(llvm::StringRef method_name, Status &error, Args... args) {
+  T Dispatch(llvm::StringRef method_name, Status &error, Args &&...args) {
     using namespace python;
     using Locker = ScriptInterpreterPythonImpl::Locker;
 
@@ -56,60 +60,117 @@ protected:
       return ErrorWithMessage<T>(caller_signature,
                                  "Python implementor not allocated.", error);
 
-    PythonObject pmeth(
+    std::tuple<Args...> original_args = std::forward_as_tuple(args...);
-        PyRefType::Owned,
+    auto transformed_args = TransformArgs(original_args);
-        PyObject_GetAttrString(implementor.get(), method_name.str().c_str()));
 
-    if (PyErr_Occurred())
+    llvm::Expected<PythonObject> expected_return_object =
-      PyErr_Clear();
+        llvm::make_error<llvm::StringError>("Not initialized.",
+                                            llvm::inconvertibleErrorCode());
+    std::apply(
+        [&implementor, &method_name, &expected_return_object](auto &&...args) {
+          llvm::consumeError(expected_return_object.takeError());
+          expected_return_object =
+              implementor.CallMethod(method_name.data(), args...);
+        },
+        transformed_args);
 
-    if (!pmeth.IsAllocated())
+    if (llvm::Error e = expected_return_object.takeError()) {
-      return ErrorWithMessage<T>(caller_signature,
+      error.SetErrorString(llvm::toString(std::move(e)).c_str());
-                                 "Python method not allocated.", error);
-
-    if (PyCallable_Check(pmeth.get()) == 0) {
-      if (PyErr_Occurred())
-        PyErr_Clear();
-      return ErrorWithMessage<T>(caller_signature,
-                                 "Python method not callable.", error);
-    }
-
-    if (PyErr_Occurred())
-      PyErr_Clear();
-
-    // TODO: make `const char *` when removing support for Python 2.
-    char *format = nullptr;
-    std::string format_buffer;
-
-    if (sizeof...(Args) > 0) {
-      FormatArgs(format_buffer, args...);
-      // TODO: make `const char *` when removing support for Python 2.
-      format = const_cast<char *>(format_buffer.c_str());
-    }
-
-    // TODO: make `const char *` when removing support for Python 2.
-    PythonObject py_return(
-        PyRefType::Owned,
-        PyObject_CallMethod(implementor.get(),
-                            const_cast<char *>(method_name.data()), format,
-                            args...));
-
-    if (PyErr_Occurred()) {
-      PyErr_Print();
-      PyErr_Clear();
       return ErrorWithMessage<T>(caller_signature,
                                  "Python method could not be called.", error);
     }
 
+    PythonObject py_return = std::move(expected_return_object.get());
+
     if (!py_return.IsAllocated())
       return ErrorWithMessage<T>(caller_signature, "Returned object is null.",
                                  error);
 
+    // Now that we called the python method with the transformed arguments,
+    // we need to interate again over both the original and transformed
+    // parameter pack, and transform back the parameter that were passed in
+    // the original parameter pack as references or pointers.
+    if (sizeof...(Args) > 0)
+      if (!ReassignPtrsOrRefsArgs(original_args, transformed_args))
+        return ErrorWithMessage<T>(
+            caller_signature,
+            "Couldn't re-assign reference and pointer arguments.", error);
+
     return ExtractValueFromPythonObject<T>(py_return, error);
   }
 
   Status GetStatusFromMethod(llvm::StringRef method_name);
 
+  template <typename T> struct transformation { using type = T; };
+  template <typename T, typename U> struct reverse_transformation {
+    static void Apply(ScriptedPythonInterface &obj, T &original_arg,
+                      U transformed_arg, Status &error) {
+      // If U is not a PythonObject, don't touch it!
+      return;
+    }
+  };
+
+  template <> struct transformation<Status> {
+    using type = python::PythonObject;
+  };
+  template <typename T> struct reverse_transformation<T, python::PythonObject> {
+    static void Apply(ScriptedPythonInterface &obj, T &original_arg,
+                      python::PythonObject transformed_arg, Status &error) {
+      original_arg =
+          obj.ExtractValueFromPythonObject<T>(transformed_arg, error);
+    }
+  };
+
+  template <typename T> typename transformation<T>::type Transform(T object) {
+    // No Transformation for generic usage
+    return {object};
+  }
+
+  template <> typename transformation<Status>::type Transform(Status arg) {
+    // Call SWIG Wrapper function
+    return python::ToSWIGWrapper(arg);
+  }
+
+  template <std::size_t... I, typename... Args>
+  auto TransformTuple(const std::tuple<Args...> &args,
+                      std::index_sequence<I...>) {
+    return std::make_tuple(Transform(std::get<I>(args))...);
+  }
+
+  // This will iterate over the Dispatch parameter pack and replace in-place
+  // every `lldb_private` argument that has a SB counterpart.
+  template <typename... Args>
+  auto TransformArgs(const std::tuple<Args...> &args) {
+    return TransformTuple(args, std::make_index_sequence<sizeof...(Args)>());
+  }
+
+  template <typename T, typename U>
+  void TransformBack(T &original_arg, U transformed_arg, Status &error) {
+    reverse_transformation<T, U>::Apply(*this, original_arg, transformed_arg,
+                                        error);
+  }
+
+  template <std::size_t... I, typename... Ts, typename... Us>
+  bool ReassignPtrsOrRefsArgs(std::tuple<Ts...> &original_args,
+                              std::tuple<Us...> &transformed_args,
+                              std::index_sequence<I...>) {
+    Status error;
+    (TransformBack(std::get<I>(original_args), std::get<I>(transformed_args),
+                   error),
+     ...);
+    return error.Success();
+  }
+
+  template <typename... Ts, typename... Us>
+  bool ReassignPtrsOrRefsArgs(std::tuple<Ts...> &original_args,
+                              std::tuple<Us...> &transformed_args) {
+    if (sizeof...(Ts) != sizeof...(Us))
+      return false;
+
+    return ReassignPtrsOrRefsArgs(original_args, transformed_args,
+                                  std::make_index_sequence<sizeof...(Ts)>());
+  }
+
   template <typename T, typename... Args>
   void FormatArgs(std::string &fmt, T arg, Args... args) const {
     FormatArgs(fmt, arg);
@@ -117,7 +178,7 @@ protected:
   }
 
   template <typename T> void FormatArgs(std::string &fmt, T arg) const {
-    fmt += GetPythonValueFormatString(arg);
+    fmt += python::PythonFormat<T>::format;
   }
 
   void FormatArgs(std::string &fmt) const {}

lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py

@@ -77,6 +77,12 @@ class ScriptedProcesTestCase(TestBase):
         self.assertEqual(process.GetProcessID(), 666)
         self.assertEqual(process.GetNumThreads(), 0)
 
+        addr = 0x500000000
+        buff = process.ReadMemory(addr, 4, error)
+        self.assertEqual(buff, None)
+        self.assertTrue(error.Fail())
+        self.assertEqual(error.GetCString(), "This is an invalid scripted process!")
+
         with open(log_file, 'r') as f:
             log = f.read()
 
@@ -109,9 +115,14 @@ class ScriptedProcesTestCase(TestBase):
         process = target.Launch(launch_info, error)
         self.assertTrue(process and process.IsValid(), PROCESS_IS_VALID)
         self.assertEqual(process.GetProcessID(), 42)
-
         self.assertEqual(process.GetNumThreads(), 1)
 
+        addr = 0x500000000
+        message = "Hello, world!"
+        buff = process.ReadCStringFromMemory(addr, len(message) + 1, error)
+        self.assertSuccess(error)
+        self.assertEqual(buff, message)
+
         thread = process.GetSelectedThread()
         self.assertTrue(thread, "Invalid thread.")
         self.assertEqual(thread.GetThreadID(), 0x19)

lldb/test/API/functionalities/scripted_process/dummy_scripted_process.py

@@ -20,11 +20,12 @@ class DummyScriptedProcess(ScriptedProcess):
     def get_registers_for_thread(self, tid: int):
         return {}
 
-    def read_memory_at_address(self, addr: int, size: int) -> lldb.SBData:
+    def read_memory_at_address(self, addr: int, size: int, error: lldb.SBError) -> lldb.SBData:
         data = lldb.SBData().CreateDataFromCString(
                                     self.target.GetByteOrder(),
                                     self.target.GetCodeByteSize(),
                                     "Hello, world!")
+
         return data
 
     def get_loaded_images(self):

lldb/test/API/functionalities/scripted_process/invalid_scripted_process.py

@@ -20,8 +20,9 @@ class InvalidScriptedProcess(ScriptedProcess):
     def get_registers_for_thread(self, tid: int):
         return {}
 
-    def read_memory_at_address(self, addr: int, size: int) -> lldb.SBData:
+    def read_memory_at_address(self, addr: int, size: int, error: lldb.SBError) -> lldb.SBData:
-        return None
+        error.SetErrorString("This is an invalid scripted process!")
+        return lldb.SBData()
 
     def get_loaded_images(self):
         return self.loaded_images

lldb/test/API/functionalities/scripted_process/stack_core_scripted_process.py

@@ -65,9 +65,8 @@ class StackCoreScriptedProcess(ScriptedProcess):
     def get_registers_for_thread(self, tid: int):
         return {}
 
-    def read_memory_at_address(self, addr: int, size: int) -> lldb.SBData:
+    def read_memory_at_address(self, addr: int, size: int, error: lldb.SBError) -> lldb.SBData:
         data = lldb.SBData()
-        error = lldb.SBError()
         bytes_read = self.corefile_process.ReadMemory(addr, size, error)
 
         if error.Fail():

lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp

@@ -270,3 +270,7 @@ bool lldb_private::LLDBSwigPythonStopHookCallHandleStop(
     lldb::StreamSP stream) {
   return false;
 }
+
+python::PythonObject lldb_private::python::ToSWIGWrapper(const Status &status) {
+  return python::PythonObject();
+}