[lldb/Interpreter] Make ScriptedInterface Object creation more generic

medismailben · medismailben · commit ef90c8a7f2f5 · 2023-10-24T20:40:43.000-07:00
This patch changes the way plugin objects used with Scripted Interfaces
are created.

Instead of implementing a different SWIG method to create the object for
every scripted interface, this patch makes the creation more generic by
re-using some of the ScriptedPythonInterface templated Dispatch code.

This patch also improves error handling of the object creation by
returning an `llvm::Expected`.

Signed-off-by: Med Ismail Bennani &lt;ismail@bennani.ma&gt;
diff --git a/lldb/bindings/python/python-wrapper.swig b/lldb/bindings/python/python-wrapper.swig
@@ -229,49 +229,6 @@ PythonObject lldb_private::python::SWIGBridge::LLDBSwigPythonCreateCommandObject
   return pfunc(SWIGBridge::ToSWIGWrapper(std::move(debugger_sp)), dict);
 }
 
-PythonObject lldb_private::python::SWIGBridge::LLDBSwigPythonCreateScriptedObject(
-    const char *python_class_name, const char *session_dictionary_name,
-    lldb::ExecutionContextRefSP exe_ctx_sp,
-    const lldb_private::StructuredDataImpl &args_impl,
-    std::string &error_string) {
-  if (python_class_name == NULL || python_class_name[0] == '\0' ||
-      !session_dictionary_name)
-    return PythonObject();
-
-  PyErr_Cleaner py_err_cleaner(true);
-
-  auto dict = PythonModule::MainModule().ResolveName<PythonDictionary>(
-      session_dictionary_name);
-  auto pfunc = PythonObject::ResolveNameWithDictionary<PythonCallable>(
-      python_class_name, dict);
-
-  if (!pfunc.IsAllocated()) {
-    error_string.append("could not find script class: ");
-    error_string.append(python_class_name);
-    return PythonObject();
-  }
-
-  llvm::Expected<PythonCallable::ArgInfo> arg_info = pfunc.GetArgInfo();
-  if (!arg_info) {
-    llvm::handleAllErrors(
-        arg_info.takeError(),
-        [&](PythonException &E) { error_string.append(E.ReadBacktrace()); },
-        [&](const llvm::ErrorInfoBase &E) {
-          error_string.append(E.message());
-        });
-    return PythonObject();
-  }
-
-  PythonObject result = {};
-  if (arg_info.get().max_positional_args == 2) {
-      result = pfunc(SWIGBridge::ToSWIGWrapper(exe_ctx_sp), SWIGBridge::ToSWIGWrapper(args_impl));
-  } else {
-    error_string.assign("wrong number of arguments in __init__, should be 2 "
-                        "(not including self)");
-  }
-  return result;
-}
-
 PythonObject lldb_private::python::SWIGBridge::LLDBSwigPythonCreateScriptedThreadPlan(
     const char *python_class_name, const char *session_dictionary_name,
     const lldb_private::StructuredDataImpl &args_impl,
diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h
@@ -25,11 +25,6 @@ class ScriptedInterface {
   ScriptedInterface() = default;
   virtual ~ScriptedInterface() = default;
 
-  virtual StructuredData::GenericSP
-  CreatePluginObject(llvm::StringRef class_name, ExecutionContext &exe_ctx,
-                     StructuredData::DictionarySP args_sp,
-                     StructuredData::Generic *script_obj = nullptr) = 0;
-
   StructuredData::GenericSP GetScriptObjectInstance() {
     return m_object_instance_sp;
   }
diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedPlatformInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedPlatformInterface.h
@@ -19,11 +19,12 @@
 namespace lldb_private {
 class ScriptedPlatformInterface : virtual public ScriptedInterface {
 public:
-  StructuredData::GenericSP
+  virtual llvm::Expected<StructuredData::GenericSP>
   CreatePluginObject(llvm::StringRef class_name, ExecutionContext &exe_ctx,
                      StructuredData::DictionarySP args_sp,
-                     StructuredData::Generic *script_obj = nullptr) override {
-    return {};
+                     StructuredData::Generic *script_obj = nullptr) {
+    llvm_unreachable(
+        "Cannot create an instance of the ScriptedPlatformInterface!");
   }
 
   virtual StructuredData::DictionarySP ListProcesses() { return {}; }
diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedProcessInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedProcessInterface.h
@@ -21,11 +21,12 @@
 namespace lldb_private {
 class ScriptedProcessInterface : virtual public ScriptedInterface {
 public:
-  StructuredData::GenericSP
+  virtual llvm::Expected<StructuredData::GenericSP>
   CreatePluginObject(llvm::StringRef class_name, ExecutionContext &exe_ctx,
                      StructuredData::DictionarySP args_sp,
-                     StructuredData::Generic *script_obj = nullptr) override {
-    return {};
+                     StructuredData::Generic *script_obj = nullptr) {
+    llvm_unreachable(
+        "Cannot create an instance of the ScriptedProcessInterface!");
   }
 
   virtual StructuredData::DictionarySP GetCapabilities() { return {}; }
diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadInterface.h
@@ -20,11 +20,12 @@
 namespace lldb_private {
 class ScriptedThreadInterface : virtual public ScriptedInterface {
 public:
-  StructuredData::GenericSP
+  virtual llvm::Expected<StructuredData::GenericSP>
   CreatePluginObject(llvm::StringRef class_name, ExecutionContext &exe_ctx,
                      StructuredData::DictionarySP args_sp,
-                     StructuredData::Generic *script_obj = nullptr) override {
-    return {};
+                     StructuredData::Generic *script_obj = nullptr) {
+    llvm_unreachable(
+        "Cannot create an instance of the ScriptedThreadInterface!");
   }
 
   virtual lldb::tid_t GetThreadID() { return LLDB_INVALID_THREAD_ID; }
diff --git a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -108,10 +108,17 @@ ScriptedProcess::ScriptedProcess(lldb::TargetSP target_sp,
   ExecutionContext exe_ctx(target_sp, /*get_process=*/false);
 
   // Create process script object
-  StructuredData::GenericSP object_sp = GetInterface().CreatePluginObject(
+  auto obj_or_err = GetInterface().CreatePluginObject(
       m_scripted_metadata.GetClassName(), exe_ctx,
       m_scripted_metadata.GetArgsSP());
 
+  if (!obj_or_err) {
+    error.SetErrorString("Failed to create script object.");
+    return;
+  }
+
+  StructuredData::GenericSP object_sp = *obj_or_err;
+
   if (!object_sp || !object_sp->IsValid()) {
     error.SetErrorStringWithFormat("ScriptedProcess::%s () - ERROR: %s",
                                    __FUNCTION__,
diff --git a/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp b/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
@@ -56,14 +56,16 @@ ScriptedThread::Create(ScriptedProcess &process,
   }
 
   ExecutionContext exe_ctx(process);
-  StructuredData::GenericSP owned_script_object_sp =
-      scripted_thread_interface->CreatePluginObject(
-          thread_class_name, exe_ctx, process.m_scripted_metadata.GetArgsSP(),
-          script_object);
+  auto obj_or_err = scripted_thread_interface->CreatePluginObject(
+      thread_class_name, exe_ctx, process.m_scripted_metadata.GetArgsSP(),
+      script_object);
 
-  if (!owned_script_object_sp)
+  if (!obj_or_err)
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "Failed to create script object.");
+
+  StructuredData::GenericSP owned_script_object_sp = *obj_or_err;
+
   if (!owned_script_object_sp->IsValid())
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "Created script object is invalid.");
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPlatformPythonInterface.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPlatformPythonInterface.cpp
@@ -29,29 +29,15 @@ ScriptedPlatformPythonInterface::ScriptedPlatformPythonInterface(
     ScriptInterpreterPythonImpl &interpreter)
     : ScriptedPlatformInterface(), ScriptedPythonInterface(interpreter) {}
 
-StructuredData::GenericSP ScriptedPlatformPythonInterface::CreatePluginObject(
+llvm::Expected<StructuredData::GenericSP>
+ScriptedPlatformPythonInterface::CreatePluginObject(
     llvm::StringRef class_name, ExecutionContext &exe_ctx,
     StructuredData::DictionarySP args_sp, StructuredData::Generic *script_obj) {
-  if (class_name.empty())
-    return {};
-
-  StructuredDataImpl args_impl(args_sp);
-  std::string error_string;
-
-  Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
-                 Locker::FreeLock);
-
-  lldb::ExecutionContextRefSP exe_ctx_ref_sp =
+  ExecutionContextRefSP exe_ctx_ref_sp =
       std::make_shared<ExecutionContextRef>(exe_ctx);
-
-  PythonObject ret_val = SWIGBridge::LLDBSwigPythonCreateScriptedObject(
-      class_name.str().c_str(), m_interpreter.GetDictionaryName(),
-      exe_ctx_ref_sp, args_impl, error_string);
-
-  m_object_instance_sp =
-      StructuredData::GenericSP(new StructuredPythonObject(std::move(ret_val)));
-
-  return m_object_instance_sp;
+  StructuredDataImpl sd_impl(args_sp);
+  return ScriptedPythonInterface::CreatePluginObject(class_name, script_obj,
+                                                     exe_ctx_ref_sp, sd_impl);
 }
 
 StructuredData::DictionarySP ScriptedPlatformPythonInterface::ListProcesses() {
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPlatformPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPlatformPythonInterface.h
@@ -22,7 +22,7 @@ class ScriptedPlatformPythonInterface : public ScriptedPlatformInterface,
 public:
   ScriptedPlatformPythonInterface(ScriptInterpreterPythonImpl &interpreter);
 
-  StructuredData::GenericSP
+  llvm::Expected<StructuredData::GenericSP>
   CreatePluginObject(const llvm::StringRef class_name,
                      ExecutionContext &exe_ctx,
                      StructuredData::DictionarySP args_sp,
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.cpp
@@ -33,29 +33,15 @@ ScriptedProcessPythonInterface::ScriptedProcessPythonInterface(
     ScriptInterpreterPythonImpl &interpreter)
     : ScriptedProcessInterface(), ScriptedPythonInterface(interpreter) {}
 
-StructuredData::GenericSP ScriptedProcessPythonInterface::CreatePluginObject(
+llvm::Expected<StructuredData::GenericSP>
+ScriptedProcessPythonInterface::CreatePluginObject(
     llvm::StringRef class_name, ExecutionContext &exe_ctx,
     StructuredData::DictionarySP args_sp, StructuredData::Generic *script_obj) {
-  if (class_name.empty())
-    return {};
-
-  StructuredDataImpl args_impl(args_sp);
-  std::string error_string;
-
-  Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
-                 Locker::FreeLock);
-
-  lldb::ExecutionContextRefSP exe_ctx_ref_sp =
+  ExecutionContextRefSP exe_ctx_ref_sp =
       std::make_shared<ExecutionContextRef>(exe_ctx);
-
-  PythonObject ret_val = SWIGBridge::LLDBSwigPythonCreateScriptedObject(
-      class_name.str().c_str(), m_interpreter.GetDictionaryName(),
-      exe_ctx_ref_sp, args_impl, error_string);
-
-  m_object_instance_sp =
-      StructuredData::GenericSP(new StructuredPythonObject(std::move(ret_val)));
-
-  return m_object_instance_sp;
+  StructuredDataImpl sd_impl(args_sp);
+  return ScriptedPythonInterface::CreatePluginObject(class_name, script_obj,
+                                                     exe_ctx_ref_sp, sd_impl);
 }
 
 StructuredData::DictionarySP ScriptedProcessPythonInterface::GetCapabilities() {
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.h
@@ -23,7 +23,7 @@ class ScriptedProcessPythonInterface : public ScriptedProcessInterface,
 public:
   ScriptedProcessPythonInterface(ScriptInterpreterPythonImpl &interpreter);
 
-  StructuredData::GenericSP
+  llvm::Expected<StructuredData::GenericSP>
   CreatePluginObject(const llvm::StringRef class_name,
                      ExecutionContext &exe_ctx,
                      StructuredData::DictionarySP args_sp,
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h
@@ -32,6 +32,98 @@ class ScriptedPythonInterface : virtual public ScriptedInterface {
   ScriptedPythonInterface(ScriptInterpreterPythonImpl &interpreter);
   ~ScriptedPythonInterface() override = default;
 
+  template <typename... Args>
+  llvm::Expected<StructuredData::GenericSP>
+  CreatePluginObject(llvm::StringRef class_name,
+                     StructuredData::Generic *script_obj, Args... args) {
+    using namespace python;
+    using Locker = ScriptInterpreterPythonImpl::Locker;
+
+    bool has_class_name = !class_name.empty();
+    bool has_interpreter_dict =
+        !(llvm::StringRef(m_interpreter.GetDictionaryName()).empty());
+    if (!has_class_name && !has_interpreter_dict && !script_obj) {
+      if (!has_class_name)
+        return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                       "Missing script class name.");
+      else if (!has_interpreter_dict)
+        return llvm::createStringError(
+            llvm::inconvertibleErrorCode(),
+            "Invalid script interpreter dictionary.");
+      else
+        return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                       "Missing scripting object.");
+    }
+
+    Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
+                   Locker::FreeLock);
+
+    PythonObject result = {};
+
+    if (script_obj) {
+      result = PythonObject(PyRefType::Borrowed,
+                            static_cast<PyObject *>(script_obj->GetValue()));
+    } else {
+      auto dict =
+          PythonModule::MainModule().ResolveName<python::PythonDictionary>(
+              m_interpreter.GetDictionaryName());
+      if (!dict.IsAllocated()) {
+        return llvm::createStringError(
+            llvm::inconvertibleErrorCode(),
+            "Could not find interpreter dictionary: %s",
+            m_interpreter.GetDictionaryName());
+      }
+
+      auto method =
+          PythonObject::ResolveNameWithDictionary<python::PythonCallable>(
+              class_name, dict);
+      if (!method.IsAllocated())
+        return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                       "Could not find script class: %s",
+                                       class_name.data());
+
+      std::tuple<Args...> original_args = std::forward_as_tuple(args...);
+      auto transformed_args = TransformArgs(original_args);
+
+      std::string error_string;
+      llvm::Expected<PythonCallable::ArgInfo> arg_info = method.GetArgInfo();
+      if (!arg_info) {
+        llvm::handleAllErrors(
+            arg_info.takeError(),
+            [&](PythonException &E) { error_string.append(E.ReadBacktrace()); },
+            [&](const llvm::ErrorInfoBase &E) {
+              error_string.append(E.message());
+            });
+        return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                       error_string);
+      }
+
+      llvm::Expected<PythonObject> expected_return_object =
+          llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                  "Resulting object is not initialized.");
+
+      std::apply(
+          [&method, &expected_return_object](auto &&...args) {
+            llvm::consumeError(expected_return_object.takeError());
+            expected_return_object = method(args...);
+          },
+          transformed_args);
+
+      if (llvm::Error e = expected_return_object.takeError())
+        return e;
+      result = std::move(expected_return_object.get());
+    }
+
+    if (!result.IsValid())
+      return llvm::createStringError(
+          llvm::inconvertibleErrorCode(),
+          "Resulting object is not a valid Python Object.");
+
+    m_object_instance_sp = StructuredData::GenericSP(
+        new StructuredPythonObject(std::move(result)));
+    return m_object_instance_sp;
+  }
+
 protected:
   template <typename T = StructuredData::ObjectSP>
   T ExtractValueFromPythonObject(python::PythonObject &p, Status &error) {
@@ -83,10 +175,6 @@ class ScriptedPythonInterface : virtual public ScriptedInterface {
 
     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
@@ -97,6 +185,8 @@ class ScriptedPythonInterface : virtual public ScriptedInterface {
             caller_signature,
             "Couldn't re-assign reference and pointer arguments.", error);
 
+    if (!py_return.IsAllocated())
+      return {};
     return ExtractValueFromPythonObject<T>(py_return, error);
   }
 
@@ -122,6 +212,14 @@ class ScriptedPythonInterface : virtual public ScriptedInterface {
     return python::SWIGBridge::ToSWIGWrapper(arg);
   }
 
+  python::PythonObject Transform(const StructuredDataImpl &arg) {
+    return python::SWIGBridge::ToSWIGWrapper(arg);
+  }
+
+  python::PythonObject Transform(lldb::ExecutionContextRefSP arg) {
+    return python::SWIGBridge::ToSWIGWrapper(arg);
+  }
+
   python::PythonObject Transform(lldb::ProcessAttachInfoSP arg) {
     return python::SWIGBridge::ToSWIGWrapper(arg);
   }
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPythonInterface.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPythonInterface.cpp
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPythonInterface.h
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h b/lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h
diff --git a/lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp b/lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
