[lldb/Plugins] Introduce Scripted Interface Factory

[mib]

Summary

This patch splits the previous ScriptedProcessPythonInterface into
multiple specific classes:

1. The ScriptedInterface abstract class that carries the interface instance object and its virtual pure abstract creation method.

2. The ScriptedPythonInterface that holds a generic Dispatch method that can be used by various interfaces to call python methods and also keeps a reference to the Python Script Interpreter instance.

3. The ScriptedProcessInterface that describes the base Scripted Process model with all the methods used in the underlying script.

All these components are used to refactor the ScriptedProcessPythonInterface
class, making it more modular.

This patch is also a requirement for the upcoming work on ScriptedThread.

Signed-off-by: Med Ismail Bennani <[e-mail]>

[JDevlieghere]

lldb/bindings/python/python-wrapper.swig

291

Unrelated whitespace change?

lldb/include/lldb/Interpreter/ScriptedInterface.h

26

We generally don't use const when passing by value.

31

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp

28

Why is this needed?

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

38–39

Why was this moved up? The lock should be as tight as possible.

91

93

Seems like you're always calling error_with_message with a constant string, so we can avoid allocating a std::string.

[jingham]

This is a good start, but it seems like there's still a lot of boilerplate once you get to the python side that could be trimmed down.

For instance, ScriptedProcessPythonInterface::GetThreadWithID is almost entirely generic. The only things that make it specific to this use are the method name that's called, and the fact that it passes an lldb::tid_t to and returns an SBStructuredData.

You've started to take care of the latter by building up a set of generic "call a method, return X" routines, and surely an SBStructuredData one will be pretty common.

Then you can add a way to pass arguments to the generic routines. Maybe an array of some little struct that has either the duple "python format string/value" or a PythonObject so the dispatcher can call the code appropriately?

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

33–45

It seems like there is a whole lot of duplicated code here. The only difference is in what we do with the returned PythonObject.

Maybe we should have a centralized "dispatch" method and then these routines differ in the conversion.

Also, it seems a shame to throw away error info, but that's what the GetGenericInteger & GetGenericString do when the object instance isn't valid, etc. llvm::Optional is good for forcing validity checks, but has no information on why the operation didn't succeed. It would be better to have all these methods take a status object for errors. You can still return the result wrapped in an optional.

When you are developing one of these classes, it would be nice to be told "no such method" for instance so you can go check if you misspelled it, or something, rather than just getting llvm::None that eventually results in some more remote error.

[mib]

Address @JDevlieghere and implement the generic ScriptedPythonInterface::Dispatch method that @jingham suggested.

This method uses templated variadic arguments and a templated return type to be able to call any python method. This allows removing the type-specific helper functions.

[jingham]

I just started reading this but I have to go now. Had one comment about errors from Dispatch. I'll look more later.

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

80–84

It seems like we also do this bit every time we do Dispatch. Maybe Dispatch could check the outgoing obj & obj->IsValid and if they aren't then it can put an appropriate message in the "error" object. Then you would just have to check error.Fail().

That would also simplify the logging. You could always dump the error's GetCString to your error_with_message. Right now, you don't log the error's GetCString in the case where the object is valid, but error.Fail is true, which might end up dropping some useful info.

[JDevlieghere]

Nobody will be surprised that I like the templated solution for the dispatch method :-) A small comment similar to Jim's but otherwise this looks good to me.

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

38–39

This comment is marked as done, but the lock is still taken at the beginning of the function.

80–84

I had a similar observation but I was thinking that Dispatch should return an Expected<StructuredData::ObjectSP>. That way you can be more specific in your error message too and differentiate between the object being null, invalid or another error.

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

40–46

This is a common pattern in llvm, but totally up to preference

112

FormatArgs has an overload for the no-args case so I don't think you need this?

[mib]

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

80–84

TBH, I don't see the added value of using Expected<T> because I'll still need to check the expected object !expected which is the same as doing !obj.

Also, regarding @jingham's suggestion, since Dispatch's return type is templated, it is not guaranteed to have an operator bool() method (same with IsValid()) that's why the check is done after calling Dispatch, on a case by case basis.

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

112

I need it since PyObject_CallMethod expects a nullptr for the format string when the param pack is empty, not an empty string.

[mib]

Address @JDevlieghere feedbacks:

• Tighten to PythonInterpreter Lock.
• Use llvm's common pattern when checking SBError*

[JDevlieghere]

LGTM

[jankratochvil]

It broke Fedora 34 x86_64 buildbot (using gcc) lldb-x86_64-fedora = https://lab.llvm.org/staging/#/builders/16 - the build fails:
https://lab.llvm.org/staging/#/builders/16/builds/10450

[mib]

In D107521#2983069, @jankratochvil wrote:

It broke Fedora 34 x86_64 buildbot (using gcc) lldb-x86_64-fedora = https://lab.llvm.org/staging/#/builders/16 - the build fails:
https://lab.llvm.org/staging/#/builders/16/builds/10450

Looking! I'll try to send a quick fix ASAP!

[mib]

I have a fix. I'm testing it locally before landing it.

[mib]

In D107521#2983109, @mib wrote:

In D107521#2983069, @jankratochvil wrote:

It broke Fedora 34 x86_64 buildbot (using gcc) lldb-x86_64-fedora = https://lab.llvm.org/staging/#/builders/16 - the build fails:
https://lab.llvm.org/staging/#/builders/16/builds/10450

Looking! I'll try to send a quick fix ASAP!

This should get fixed with rG5f6f33da9ee6cbaef5f10b4a7be34a91d5185b2b

lldb/bindings/python/python-wrapper.swig

(
     const lldb::TargetSP& target_sp,
     lldb_private::StructuredDataImpl *args_impl,
     std::string &error_string
 )
 {
     if (python_class_name == NULL || python_class_name[0] == '\0' || !session_dictionary_name)
         Py_RETURN_NONE;
 
-
     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);

lldb/include/lldb/Interpreter/ScriptedInterface.h

+//===-- ScriptedInterface.h -------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_INTERPRETER_SCRIPTEDINTERFACE_H
+#define LLDB_INTERPRETER_SCRIPTEDINTERFACE_H
+
+#include "lldb/Core/StructuredDataImpl.h"
+#include "lldb/Target/ExecutionContext.h"
+#include "lldb/lldb-private.h"
+
+#include <string>
+
+namespace lldb_private {
+class ScriptedInterface {
+public:
+  ScriptedInterface() = default;
+  virtual ~ScriptedInterface() = default;
+
+  virtual StructuredData::GenericSP
+  CreatePluginObject(llvm::StringRef class_name, ExecutionContext &exe_ctx,
+                     StructuredData::DictionarySP args_sp) = 0;
+
+protected:
+  StructuredData::GenericSP m_object_instance_sp;
+};
+} // namespace lldb_private
+#endif // LLDB_INTERPRETER_SCRIPTEDINTERFACE_H

lldb/include/lldb/Interpreter/ScriptedProcessInterface.h

 //===-- ScriptedProcessInterface.h ------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_INTERPRETER_SCRIPTEDPROCESSINTERFACE_H
 #define LLDB_INTERPRETER_SCRIPTEDPROCESSINTERFACE_H
 
 #include "lldb/Core/StructuredDataImpl.h"
 #include "lldb/Interpreter/ScriptInterpreter.h"
+#include "lldb/Interpreter/ScriptedInterface.h"
+
 #include "lldb/lldb-private.h"
 
 #include <string>
 
 namespace lldb_private {
-class ScriptedProcessInterface {
+class ScriptedProcessInterface : virtual public ScriptedInterface {
 public:
-  ScriptedProcessInterface() : m_object_instance_sp(nullptr) {}
-
-  virtual ~ScriptedProcessInterface() = default;
-
-  virtual StructuredData::GenericSP
-  CreatePluginObject(const llvm::StringRef class_name, lldb::TargetSP target_sp,
-                     StructuredData::DictionarySP args_sp) {
+  StructuredData::GenericSP
+  CreatePluginObject(llvm::StringRef class_name, ExecutionContext &exe_ctx,
+                     StructuredData::DictionarySP args_sp) override {
     return nullptr;
   }
 
   virtual Status Launch() { return Status("ScriptedProcess did not launch"); }
 
   virtual Status Resume() { return Status("ScriptedProcess did not resume"); }
 
   virtual bool ShouldStop() { return true; }
ReadMemoryAtAddress(lldb::addr_t address, size_t size, Status &error) {
     return nullptr;
   }
 
   virtual StructuredData::DictionarySP GetLoadedImages() { return nullptr; }
 
   virtual lldb::pid_t GetProcessID() { return LLDB_INVALID_PROCESS_ID; }
 
   virtual bool IsAlive() { return true; }
-
-private:
-  StructuredData::ObjectSP m_object_instance_sp;
 };
 } // namespace lldb_private
 
 #endif // LLDB_INTERPRETER_SCRIPTEDPROCESSINTERFACE_H

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp

 #include "lldb/Interpreter/ScriptInterpreter.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/RegisterContext.h"
 
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Logging.h"
 #include "lldb/Utility/State.h"
 
 #include <mutex>
 
 LLDB_PLUGIN_DEFINE(ScriptedProcess)
 
 using namespace lldb;
 using namespace lldb_private;
 
 ConstString ScriptedProcess::GetPluginNameStatic() {
   static ConstString g_name("ScriptedProcess");
ScriptedProcess::ScriptedProcess(
 
   if (!m_interpreter) {
     error.SetErrorStringWithFormat("ScriptedProcess::%s () - ERROR: %s",
                                    __FUNCTION__,
                                    "Debugger has no Script Interpreter");
     return;
   }
 
-  StructuredData::ObjectSP object_sp = GetInterface().CreatePluginObject(
-      m_scripted_process_info.GetClassName().c_str(), target_sp,
+  ExecutionContext exe_ctx(target_sp, /*get_process=*/false);
+
+  StructuredData::GenericSP object_sp = GetInterface().CreatePluginObject(
+      m_scripted_process_info.GetClassName().c_str(), exe_ctx,
       m_scripted_process_info.GetDictionarySP());
 
   if (!object_sp || !object_sp->IsValid()) {
     error.SetErrorStringWithFormat("ScriptedProcess::%s () - ERROR: %s",
                                    __FUNCTION__,
                                    "Failed to create valid script object");
     return;
   }

lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt

 if(NOT LLDB_PYTHON_RELATIVE_PATH)
   message(FATAL_ERROR "LLDB_PYTHON_RELATIVE_PATH is not set.")
 endif()
 add_definitions(-DLLDB_PYTHON_RELATIVE_LIBDIR="${LLDB_PYTHON_RELATIVE_PATH}")
 
 if (LLDB_ENABLE_LIBEDIT)
   list(APPEND LLDB_LIBEDIT_LIBS ${LibEdit_LIBRARIES})
 endif()
 
 add_lldb_library(lldbPluginScriptInterpreterPython PLUGIN
   PythonDataObjects.cpp
   PythonReadline.cpp
   ScriptInterpreterPython.cpp
+  ScriptedPythonInterface.cpp
   ScriptedProcessPythonInterface.cpp
   SWIGPythonBridge.cpp
 
   LINK_LIBS
     lldbBreakpoint
     lldbCore
     lldbDataFormatters
     lldbHost

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

 //===-- ScriptInterpreterPython.h -------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_PLUGINS_SCRIPTINTERPRETER_PYTHON_SCRIPTINTERPRETERPYTHON_H
 #define LLDB_PLUGINS_SCRIPTINTERPRETER_PYTHON_SCRIPTINTERPRETERPYTHON_H
 
 #include "lldb/Host/Config.h"
 
 #if LLDB_ENABLE_PYTHON
 
-#include "ScriptedProcessPythonInterface.h"
-
 #include "lldb/Breakpoint/BreakpointOptions.h"
 #include "lldb/Core/IOHandler.h"
 #include "lldb/Core/StructuredDataImpl.h"
 #include "lldb/Interpreter/ScriptInterpreter.h"
 #include "lldb/lldb-private.h"
 
 #include <memory>
 #include <string>

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

 //===-- ScriptedProcessPythonInterface.h ------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_PLUGINS_SCRIPTINTERPRETER_PYTHON_SCRIPTEDPROCESSPYTHONINTERFACE_H
 #define LLDB_PLUGINS_SCRIPTINTERPRETER_PYTHON_SCRIPTEDPROCESSPYTHONINTERFACE_H
 
 #include "lldb/Host/Config.h"
 
 #if LLDB_ENABLE_PYTHON
 
+#include "ScriptedPythonInterface.h"
 #include "lldb/Interpreter/ScriptedProcessInterface.h"
 
 namespace lldb_private {
-class ScriptInterpreterPythonImpl;
-class ScriptedProcessPythonInterface : public ScriptedProcessInterface {
+class ScriptedProcessPythonInterface : public ScriptedProcessInterface,
+                                       public ScriptedPythonInterface {
 public:
-  ScriptedProcessPythonInterface(ScriptInterpreterPythonImpl &interpreter)
-      : ScriptedProcessInterface(), m_interpreter(interpreter) {}
+  ScriptedProcessPythonInterface(ScriptInterpreterPythonImpl &interpreter);
 
   StructuredData::GenericSP
-  CreatePluginObject(const llvm::StringRef class_name, lldb::TargetSP target_sp,
+  CreatePluginObject(const llvm::StringRef class_name,
+                     ExecutionContext &exe_ctx,
                      StructuredData::DictionarySP args_sp) override;
 
   Status Launch() override;
 
   Status Resume() override;
 
   bool ShouldStop() override;
 
public:
   lldb::DataExtractorSP ReadMemoryAtAddress(lldb::addr_t address, size_t size,
                                             Status &error) override;
 
   StructuredData::DictionarySP GetLoadedImages() override;
 
   lldb::pid_t GetProcessID() override;
 
   bool IsAlive() override;
-
-protected:
-  llvm::Optional<unsigned long long>
-  GetGenericInteger(llvm::StringRef method_name);
-  Status GetStatusFromMethod(llvm::StringRef method_name);
-
-private:
-  // The lifetime is managed by the ScriptInterpreter
-  ScriptInterpreterPythonImpl &m_interpreter;
-  StructuredData::GenericSP m_object_instance_sp;
 };
 } // namespace lldb_private
 
 #endif // LLDB_ENABLE_PYTHON
 #endif // LLDB_PLUGINS_SCRIPTINTERPRETER_PYTHON_SCRIPTEDPROCESSPYTHONINTERFACE_H

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

 //===-- ScriptedProcessPythonInterface.cpp --------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Host/Config.h"
+#include "lldb/Utility/Log.h"
+#include "lldb/Utility/Logging.h"
 #include "lldb/lldb-enumerations.h"
 
 #if LLDB_ENABLE_PYTHON
 
 // LLDB Python header must be included first
 #include "lldb-python.h"
 
 #include "SWIGPythonBridge.h"
 #include "ScriptInterpreterPythonImpl.h"
 #include "ScriptedProcessPythonInterface.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::python;
 using Locker = ScriptInterpreterPythonImpl::Locker;
 
+ScriptedProcessPythonInterface::ScriptedProcessPythonInterface(
+    ScriptInterpreterPythonImpl &interpreter)
+    : ScriptedProcessInterface(), ScriptedPythonInterface(interpreter) {}
+
 StructuredData::GenericSP ScriptedProcessPythonInterface::CreatePluginObject(
-    const llvm::StringRef class_name, lldb::TargetSP target_sp,
+    llvm::StringRef class_name, ExecutionContext &exe_ctx,
     StructuredData::DictionarySP args_sp) {
   if (class_name.empty())
     return {};
 
-  std::string error_string;
+  TargetSP target_sp = exe_ctx.GetTargetSP();
   StructuredDataImpl *args_impl = nullptr;
   if (args_sp) {
     args_impl = new StructuredDataImpl();
     args_impl->SetObjectSP(args_sp);
   }
-
-  void *ret_val;
-
-  {
+  std::string error_string;
 
-    Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
-                   Locker::FreeLock);
 
-    ret_val = LLDBSwigPythonCreateScriptedProcess(
-        class_name.str().c_str(), m_interpreter.GetDictionaryName(), target_sp,
-        args_impl, error_string);
-  }
+  void *ret_val = LLDBSwigPythonCreateScriptedProcess(
+
+  if (!ret_val)
+    return {};
 
   m_object_instance_sp =
       StructuredData::GenericSP(new StructuredPythonObject(ret_val));
 
   return m_object_instance_sp;
 }
 
 Status ScriptedProcessPythonInterface::Launch() {
   return GetStatusFromMethod("launch");
 }
 
 Status ScriptedProcessPythonInterface::Resume() {
   return GetStatusFromMethod("resume");
 }
 
 bool ScriptedProcessPythonInterface::ShouldStop() {
-  llvm::Optional<unsigned long long> should_stop =
-      GetGenericInteger("should_stop");
+  Status error;
+  StructuredData::ObjectSP obj = Dispatch("is_alive", error);
 
-  if (!should_stop)
+  auto error_with_message = [](llvm::StringRef message) {
+    LLDB_LOGF(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS),
+              "ScriptedProcess::%s ERROR = %s", __FUNCTION__, message.data());
     return false;
+  };
 
-  return static_cast<bool>(*should_stop);
-}
-
-Status ScriptedProcessPythonInterface::Stop() {
-  return GetStatusFromMethod("stop");
-}
-
-Status ScriptedProcessPythonInterface::GetStatusFromMethod(
-    llvm::StringRef method_name) {
-  Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
-                 Locker::FreeLock);
-
-  if (!m_object_instance_sp)
-    return Status("Python object ill-formed.");
-
-  if (!m_object_instance_sp)
-    return Status("Cannot convert Python object to StructuredData::Generic.");
-  PythonObject implementor(PyRefType::Borrowed,
-                           (PyObject *)m_object_instance_sp->GetValue());
-
-  if (!implementor.IsAllocated())
-    return Status("Python implementor not allocated.");
-
-  PythonObject pmeth(
-      PyRefType::Owned,
-      PyObject_GetAttrString(implementor.get(), method_name.str().c_str()));
-
-  if (PyErr_Occurred())
-    PyErr_Clear();
-
-  if (!pmeth.IsAllocated())
-    return Status("Python method not allocated.");
-
-  if (PyCallable_Check(pmeth.get()) == 0) {
-    if (PyErr_Occurred())
-      PyErr_Clear();
-    return Status("Python method not callable.");
-  }
-
-  if (PyErr_Occurred())
-    PyErr_Clear();
-
-  PythonObject py_return(PyRefType::Owned,
-                         PyObject_CallMethod(implementor.get(),
-                                             method_name.str().c_str(),
-                                             nullptr));
-
-  if (PyErr_Occurred()) {
-    PyErr_Print();
-    PyErr_Clear();
-    return Status("Python method could not be called.");
-  }
-
-  if (PyObject *py_ret_ptr = py_return.get()) {
-    lldb::SBError *sb_error =
-        (lldb::SBError *)LLDBSWIGPython_CastPyObjectToSBError(py_ret_ptr);
-
-    if (!sb_error)
-      return Status("Couldn't cast lldb::SBError to lldb::Status.");
-
-    Status status = m_interpreter.GetStatusFromSBError(*sb_error);
-
-    if (status.Fail())
-      return Status("error: %s", status.AsCString());
-
-    return status;
-  }
-
-  return Status("Returned object is null.");
-}
-
-llvm::Optional<unsigned long long>
-ScriptedProcessPythonInterface::GetGenericInteger(llvm::StringRef method_name) {
-  Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
-                 Locker::FreeLock);
-
-  if (!m_object_instance_sp)
-    return llvm::None;
-
-  if (!m_object_instance_sp)
-    return llvm::None;
-  PythonObject implementor(PyRefType::Borrowed,
-                           (PyObject *)m_object_instance_sp->GetValue());
-
-  if (!implementor.IsAllocated())
-    return llvm::None;
-
-  PythonObject pmeth(
-      PyRefType::Owned,
-      PyObject_GetAttrString(implementor.get(), method_name.str().c_str()));
-
-  if (PyErr_Occurred())
-    PyErr_Clear();
-
-  if (!pmeth.IsAllocated())
-    return llvm::None;
-
-  if (PyCallable_Check(pmeth.get()) == 0) {
-    if (PyErr_Occurred())
-      PyErr_Clear();
-    return llvm::None;
+  if (!obj || !obj->IsValid() || error.Fail()) {
+    return error_with_message(llvm::Twine("Null or invalid object (" +
+                                          llvm::Twine(error.AsCString()) +
+                                          llvm::Twine(")."))
+                                  .str());
   }
 
-  if (PyErr_Occurred())
-    PyErr_Clear();
-
-  PythonObject py_return(PyRefType::Owned,
-                         PyObject_CallMethod(implementor.get(),
-                                             method_name.str().c_str(),
-                                             nullptr));
-
-  if (PyErr_Occurred()) {
-    PyErr_Print();
-    PyErr_Clear();
-  }
+  return obj->GetBooleanValue();
 
-  if (!py_return.get())
-    return llvm::None;
-
-  llvm::Expected<unsigned long long> size = py_return.AsUnsignedLongLong();
-  // FIXME: Handle error.
-  if (!size)
-    return llvm::None;
-
-  return *size;
+Status ScriptedProcessPythonInterface::Stop() {
+  return GetStatusFromMethod("stop");
 }
 
 lldb::MemoryRegionInfoSP
 ScriptedProcessPythonInterface::GetMemoryRegionContainingAddress(
     lldb::addr_t address) {
   // TODO: Implement
-  return nullptr;
+  return {};
 }
 
 StructuredData::DictionarySP
 ScriptedProcessPythonInterface::GetThreadWithID(lldb::tid_t tid) {
-  // TODO: Implement
-  return nullptr;
-}
-
-StructuredData::DictionarySP
-ScriptedProcessPythonInterface::GetRegistersForThread(lldb::tid_t tid) {
-  // TODO: Implement
-  return nullptr;
-}
-
-lldb::DataExtractorSP ScriptedProcessPythonInterface::ReadMemoryAtAddress(
-    lldb::addr_t address, size_t size, Status &error) {
   Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
                  Locker::FreeLock);
 
-  auto error_with_message = [&error](llvm::StringRef message) {
-    error.SetErrorString(message);
-    return nullptr;
+  auto error_with_message = [](llvm::StringRef message) {
+    LLDB_LOGF(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS),
+              "ScriptedProcess::%s ERROR = %s", __FUNCTION__, message.data());
+    return StructuredData::DictionarySP();
   };
 
-  static char callee_name[] = "read_memory_at_address";
-  std::string param_format = GetPythonValueFormatString(address);
-  param_format += GetPythonValueFormatString(size);
-
-  if (!m_object_instance_sp)
-    return error_with_message("Python object ill-formed.");
-
-  if (!m_object_instance_sp)
-    return error_with_message("Python method not callable.");
-
-  PythonObject implementor(PyRefType::Borrowed,
-                           (PyObject *)m_object_instance_sp->GetValue());
+  Status error;
+  StructuredData::ObjectSP obj = Dispatch("get_thread_with_id", error, tid);
 
-  if (!implementor.IsAllocated())
-    return error_with_message("Python implementor not allocated.");
-
-  PythonObject pmeth(PyRefType::Owned,
-                     PyObject_GetAttrString(implementor.get(), callee_name));
-
-  if (PyErr_Occurred())
-    PyErr_Clear();
-
-  if (!pmeth.IsAllocated())
-    return error_with_message("Python method not allocated.");
-
-  if (PyCallable_Check(pmeth.get()) == 0) {
-    if (PyErr_Occurred())
-      PyErr_Clear();
-    return error_with_message("Python method not callable.");
+  if (!obj || !obj->IsValid() || error.Fail()) {
+    return error_with_message(llvm::Twine("Null or invalid object (" +
+                                          llvm::Twine(error.AsCString()) +
+                                          llvm::Twine(")."))
+                                  .str());
   }
 
-  if (PyErr_Occurred())
-    PyErr_Clear();
-
-  PythonObject py_return(PyRefType::Owned,
-                         PyObject_CallMethod(implementor.get(), callee_name,
-                                             param_format.c_str(), address,
-                                             size));
+  StructuredData::DictionarySP dict{obj->GetAsDictionary()};
 
-  if (PyErr_Occurred()) {
-    PyErr_Print();
-    PyErr_Clear();
-    return error_with_message("Python method could not be called.");
-  }
+  return dict;
 
-  if (PyObject *py_ret_ptr = py_return.get()) {
-    lldb::SBData *sb_data =
-        (lldb::SBData *)LLDBSWIGPython_CastPyObjectToSBData(py_ret_ptr);
-
-    if (!sb_data)
-      return error_with_message(
-          "Couldn't cast lldb::SBData to lldb::DataExtractor.");
-
-    return m_interpreter.GetDataExtractorFromSBData(*sb_data);
-  }
+StructuredData::DictionarySP
+ScriptedProcessPythonInterface::GetRegistersForThread(lldb::tid_t tid) {
+  // TODO: Implement
+  return {};
 
-  return error_with_message("Returned object is null.");
+lldb::DataExtractorSP ScriptedProcessPythonInterface::ReadMemoryAtAddress(
+    lldb::addr_t address, size_t size, Status &error) {
+  return Dispatch<lldb::DataExtractorSP>("read_memory_at_address", error,
+                                         address, size);
 }
 
 StructuredData::DictionarySP ScriptedProcessPythonInterface::GetLoadedImages() {
   // TODO: Implement
-  return nullptr;
+  return {};
 }
 
 lldb::pid_t ScriptedProcessPythonInterface::GetProcessID() {
-  llvm::Optional<unsigned long long> pid = GetGenericInteger("get_process_id");
-  return (!pid) ? LLDB_INVALID_PROCESS_ID : *pid;
+  Status error;
+  StructuredData::ObjectSP obj = Dispatch("get_process_id", error);
+
+  auto error_with_message = [](llvm::StringRef message) {
+    LLDB_LOGF(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS),
+              "ScriptedProcess::%s ERROR = %s", __FUNCTION__, message.data());
+    return LLDB_INVALID_PROCESS_ID;
+  };
+
+  if (!obj || !obj->IsValid() || error.Fail()) {
+    return error_with_message(llvm::Twine("Null or invalid object (" +
+                                          llvm::Twine(error.AsCString()) +
+                                          llvm::Twine(")."))
+                                  .str());
+  }
+
+  return obj->GetIntegerValue(LLDB_INVALID_PROCESS_ID);
 }
 
 bool ScriptedProcessPythonInterface::IsAlive() {
-  llvm::Optional<unsigned long long> is_alive = GetGenericInteger("is_alive");
+  Status error;
+  StructuredData::ObjectSP obj = Dispatch("is_alive", error);
 
-  if (!is_alive)
+  auto error_with_message = [](llvm::StringRef message) {
+    LLDB_LOGF(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS),
+              "ScriptedProcess::%s ERROR = %s", __FUNCTION__, message.data());
     return false;
+  };
+
+  if (!obj || !obj->IsValid() || error.Fail()) {
+    return error_with_message(llvm::Twine("Null or invalid object (" +
+                                          llvm::Twine(error.AsCString()) +
+                                          llvm::Twine(")."))
+                                  .str());
+  }
 
-  return static_cast<bool>(*is_alive);
+  return obj->GetBooleanValue();
 }
 
 #endif

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

+//===-- ScriptedPythonInterface.h -------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_PLUGINS_SCRIPTINTERPRETER_PYTHON_SCRIPTEDPYTHONINTERFACE_H
+#define LLDB_PLUGINS_SCRIPTINTERPRETER_PYTHON_SCRIPTEDPYTHONINTERFACE_H
+
+#include "lldb/Host/Config.h"
+
+#if LLDB_ENABLE_PYTHON
+
+#include "lldb/Interpreter/ScriptedInterface.h"
+#include "lldb/Utility/DataBufferHeap.h"
+
+#include "PythonDataObjects.h"
+#include "SWIGPythonBridge.h"
+#include "ScriptInterpreterPythonImpl.h"
+
+namespace lldb_private {
+class ScriptInterpreterPythonImpl;
+class ScriptedPythonInterface : virtual public ScriptedInterface {
+public:
+  ScriptedPythonInterface(ScriptInterpreterPythonImpl &interpreter);
+  virtual ~ScriptedPythonInterface() = default;
+
+protected:
+  template <typename T = StructuredData::ObjectSP>
+  T ExtractValueFromPythonObject(python::PythonObject &p, Status &error) {
+    return p.CreateStructuredObject();
+  }
+
+  template <>
+  Status 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);
+    else
+      error.SetErrorString("Couldn't cast lldb::SBError to lldb::Status.");
+
+    return error;
+  }
+
+  template <>
+  lldb::DataExtractorSP
+  ExtractValueFromPythonObject<lldb::DataExtractorSP>(python::PythonObject &p,
+                                                      Status &error) {
+    lldb::SBData *sb_data = reinterpret_cast<lldb::SBData *>(
+        LLDBSWIGPython_CastPyObjectToSBData(p.get()));
+
+    if (!sb_data) {
+      error.SetErrorString("Couldn't cast lldb::SBError to lldb::Status.");
+      return nullptr;
+    }
+
+    return m_interpreter.GetDataExtractorFromSBData(*sb_data);
+  }
+
+  template <typename T = StructuredData::ObjectSP, typename... Args>
+  T Dispatch(llvm::StringRef method_name, Status &error, Args... args) {
+    using namespace python;
+    using Locker = ScriptInterpreterPythonImpl::Locker;
+
+    auto error_with_message = [&method_name, &error](llvm::StringRef message) {
+      error.SetErrorStringWithFormatv(
+          "ScriptedPythonInterface::{0} ({1}) ERROR = {2}", __FUNCTION__,
+          method_name, message);
+      return T();
+    };
+
+    if (!m_object_instance_sp)
+      return error_with_message("Python object ill-formed");
+
+    Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
+                   Locker::FreeLock);
+
+    PythonObject implementor(PyRefType::Borrowed,
+                             (PyObject *)m_object_instance_sp->GetValue());
+
+    if (!implementor.IsAllocated())
+      return error_with_message("Python implementor not allocated.");
+
+    PythonObject pmeth(
+        PyRefType::Owned,
+        PyObject_GetAttrString(implementor.get(), method_name.str().c_str()));
+
+    if (PyErr_Occurred())
+      PyErr_Clear();
+
+    if (!pmeth.IsAllocated())
+      return error_with_message("Python method not allocated.");
+
+    if (PyCallable_Check(pmeth.get()) == 0) {
+      if (PyErr_Occurred())
+        PyErr_Clear();
+      return error_with_message("Python method not callable.");
+    }
+
+    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 error_with_message("Python method could not be called.");
+    }
+
+    if (!py_return.IsAllocated())
+      return error_with_message("Returned object is null.");
+
+    return ExtractValueFromPythonObject<T>(py_return, error);
+  }
+
+  Status GetStatusFromMethod(llvm::StringRef method_name);
+
+  template <typename T, typename... Args>
+  void FormatArgs(std::string &fmt, T arg, Args... args) const {
+    FormatArgs(fmt, arg);
+    FormatArgs(fmt, args...);
+  }
+
+  template <typename T> void FormatArgs(std::string &fmt, T arg) const {
+    fmt += GetPythonValueFormatString(arg);
+  }
+
+  void FormatArgs(std::string &fmt) const {}
+
+  // The lifetime is managed by the ScriptInterpreter
+  ScriptInterpreterPythonImpl &m_interpreter;
+};
+} // namespace lldb_private
+
+#endif // LLDB_ENABLE_PYTHON
+#endif // LLDB_PLUGINS_SCRIPTINTERPRETER_PYTHON_SCRIPTEDPYTHONINTERFACE_H

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

+//===-- ScriptedPythonInterface.cpp ---------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Host/Config.h"
+#include "lldb/Utility/Log.h"
+#include "lldb/Utility/Logging.h"
+#include "lldb/lldb-enumerations.h"
+
+#if LLDB_ENABLE_PYTHON
+
+// LLDB Python header must be included first
+#include "lldb-python.h"
+
+#include "SWIGPythonBridge.h"
+#include "ScriptInterpreterPythonImpl.h"
+#include "ScriptedPythonInterface.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+ScriptedPythonInterface::ScriptedPythonInterface(
+    ScriptInterpreterPythonImpl &interpreter)
+    : ScriptedInterface(), m_interpreter(interpreter) {}
+
+Status
+ScriptedPythonInterface::GetStatusFromMethod(llvm::StringRef method_name) {
+  Status error;
+  Dispatch<Status>(method_name, error);
+
+  return error;
+}
+
+#endif

lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp

 //===-- PythonTestSuite.cpp -----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "gtest/gtest.h"
 
 #include "Plugins/ScriptInterpreter/Python/lldb-python.h"
 
 #include "Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h"
 #include "Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h"
-#include "lldb/API/SBError.h"
+
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/HostInfo.h"
 
 #include "PythonTestSuite.h"
 
 using namespace lldb_private;
 class TestScriptInterpreterPython : public ScriptInterpreterPythonImpl {
 public: