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

4 files changed, 178 insertions, 48 deletions

diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h b/lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h
index 7e18b0e..30634c8 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h
+++ b/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
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
index ffce8c4..f05f8f8 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
+++ b/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() {
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.cpp
index d5c527c..8adc979 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.cpp
+++ b/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 <>
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h
index 7c2fadc..ac1fe09 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h
+++ b/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,59 +60,116 @@ protected:
       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 @@ 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 {}