[lldb/Plugins] Improve error reporting with reading memory in Scripted Process
authorMed Ismail Bennani <medismail.bennani@gmail.com>
Fri, 18 Nov 2022 21:53:57 +0000 (13:53 -0800)
committerMed Ismail Bennani <medismail.bennani@gmail.com>
Fri, 18 Nov 2022 21:56:48 +0000 (13:56 -0800)
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>
14 files changed:
lldb/bindings/python/python-swigsafecast.swig
lldb/examples/python/scripted_process/crashlog_scripted_process.py
lldb/examples/python/scripted_process/scripted_process.py
lldb/include/lldb/API/SBError.h
lldb/source/API/SBError.cpp
lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.cpp
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h
lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
lldb/test/API/functionalities/scripted_process/dummy_scripted_process.py
lldb/test/API/functionalities/scripted_process/invalid_scripted_process.py
lldb/test/API/functionalities/scripted_process/stack_core_scripted_process.py
lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp

index aa5e8e5..a1e883b 100644 (file)
@@ -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);
 }
index 7ed5cc9..71cfff2 100644 (file)
@@ -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()
 
index 86ec1db..cc886c6 100644 (file)
@@ -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
index 30214e3..e698ee9 100644 (file)
@@ -23,6 +23,8 @@ public:
 
   SBError(const lldb::SBError &rhs);
 
+  SBError(const lldb_private::Status &error);
+
   ~SBError();
 
   const SBError &operator=(const lldb::SBError &rhs);
index ef4f726..1a03415 100644 (file)
@@ -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) {
index 7e18b0e..30634c8 100644 (file)
 #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
index ffce8c4..f05f8f8 100644 (file)
@@ -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() {
index d5c527c..8adc979 100644 (file)
@@ -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 <>
index 7c2fadc..ac1fe09 100644 (file)
@@ -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 {}
index 909413c..619e47c 100644 (file)
@@ -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)
index a687870..d4f7953 100644 (file)
@@ -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):
index 62623d5..62db547 100644 (file)
@@ -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:
-        return None
+    def read_memory_at_address(self, addr: int, size: int, error: lldb.SBError) -> lldb.SBData:
+        error.SetErrorString("This is an invalid scripted process!")
+        return lldb.SBData()
 
     def get_loaded_images(self):
         return self.loaded_images
index 67a1e53..67a1256 100644 (file)
@@ -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():
index 87e4a03..51ff7e4 100644 (file)
@@ -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();
+}