[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 <[email protected]>

Author: medismailben

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 @@ def get_thread_with_id(self, tid: int):
     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 @@ def get_registers_for_thread(self, tid):
         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 @@ class LLDB_API SBError {
 
   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,
-                                         address, size);
+  Status py_error;
+  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 @@ class ScriptedPythonInterface : virtual public ScriptedInterface {
   }
 
   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,59 +60,116 @@ class ScriptedPythonInterface : virtual public ScriptedInterface {
       return ErrorWithMessage<T>(caller_signature,
                                  "Python implementor not allocated.", error);
 
-    PythonObject pmeth(
-        PyRefType::Owned,
-        PyObject_GetAttrString(implementor.get(), method_name.str().c_str()));
+    std::tuple<Args...> original_args = std::forward_as_tuple(args...);
+    auto transformed_args = TransformArgs(original_args);
+
+    llvm::Expected<PythonObject> expected_return_object =
+        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 (llvm::Error e = expected_return_object.takeError()) {
+      error.SetErrorString(llvm::toString(std::move(e)).c_str());
+      return ErrorWithMessage<T>(caller_signature,
+                                 "Python method could not be called.", error);
+    }
 
-    if (PyErr_Occurred())
-      PyErr_Clear();
+    PythonObject py_return = std::move(expected_return_object.get());
 
-    if (!pmeth.IsAllocated())
-      return ErrorWithMessage<T>(caller_signature,
-                                 "Python method not allocated.", error);
+    if (!py_return.IsAllocated())
+      return ErrorWithMessage<T>(caller_signature, "Returned object is null.",
+                                 error);
 
-    if (PyCallable_Check(pmeth.get()) == 0) {
-      if (PyErr_Occurred())
-        PyErr_Clear();
-      return ErrorWithMessage<T>(caller_signature,
-                                 "Python method not callable.", 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);
 
-    if (PyErr_Occurred())
-      PyErr_Clear();
+    return ExtractValueFromPythonObject<T>(py_return, error);
+  }
 
-    // TODO: make `const char *` when removing support for Python 2.
-    char *format = nullptr;
-    std::string format_buffer;
+  Status GetStatusFromMethod(llvm::StringRef method_name);
 
-    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());
+  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);
     }
+  };
 
-    // 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...));
+  template <typename T> typename transformation<T>::type Transform(T object) {
+    // No Transformation for generic usage
+    return {object};
+  }
 
-    if (PyErr_Occurred()) {
-      PyErr_Print();
-      PyErr_Clear();
-      return ErrorWithMessage<T>(caller_signature,
-                                 "Python method could not be called.", error);
-    }
+  template <> typename transformation<Status>::type Transform(Status arg) {
+    // Call SWIG Wrapper function
+    return python::ToSWIGWrapper(arg);
+  }
 
-    if (!py_return.IsAllocated())
-      return ErrorWithMessage<T>(caller_signature, "Returned object is null.",
-                                 error);
+  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))...);
+  }
 
-    return ExtractValueFromPythonObject<T>(py_return, error);
+  // 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)>());
   }
 
-  Status GetStatusFromMethod(llvm::StringRef method_name);
+  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 {
@@ -117,7 +178,7 @@ class ScriptedPythonInterface : virtual public ScriptedInterface {
   }
 
   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 {}