[lldb/Python] Make use of PythonObject and PythonFormat in callbacks (NFC)

[mib]

Summary

This patch extends the template specialization of PythonFormat structs
and makes use of the pre-existing PythonObject class to format arguments
and pass them to the right method, before calling it.

This is a preparatory patch to merge PythonFormat with SWIGPythonBridge's
GetPythonValueFormatString methods.

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

[labath]

Cool. Thanks for doing that.

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

191–194

Maybe something like:

template<typename T, char F> struct PassthroughFormat {
  static constexpr char format = F;
  static constexpr T get(T t) { return t; }
};

template<> struct PythonFormat<char *> : PassthroughFormat<char *, 's'>;
// etc.

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

1525

You need to do something with the error inside here. Log it perhaps (we have LLDB_LOG_ERROR for that)?

1558

same here (and elsewhere)

[mib]

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

191–194

More templates <3 !

[mib]

Implement @labath suggestions

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

inline llvm::Error keyError() {
   return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                  "key not in dict");
 }
 
 inline const char *py2_const_cast(const char *s) { return s; }
 
 enum class PyInitialValue { Invalid, Empty };
 
+// DOC: https://docs.python.org/3/c-api/arg.html#building-values
 template <typename T, typename Enable = void> struct PythonFormat;
 
-template <> struct PythonFormat<unsigned long long> {
-  static constexpr char format = 'K';
-  static auto get(unsigned long long value) { return value; }
+template <typename T, char F> struct PassthroughFormat {
+  static constexpr char format = F;
+  static constexpr T get(T t) { return t; }
 };
 
-template <> struct PythonFormat<long long> {
-  static constexpr char format = 'L';
-  static auto get(long long value) { return value; }
-};
-
-template <> struct PythonFormat<PyObject *> {
-  static constexpr char format = 'O';
-  static auto get(PyObject *value) { return value; }
-};
+template <> struct PythonFormat<char *> : PassthroughFormat<char *, 's'> {};
+template <> struct PythonFormat<char> : PassthroughFormat<char, 'b'> {};
+template <>
+struct PythonFormat<unsigned char> : PassthroughFormat<unsigned char, 'B'> {};
+template <> struct PythonFormat<short> : PassthroughFormat<short, 'h'> {};
+template <>
+struct PythonFormat<unsigned short> : PassthroughFormat<unsigned short, 'H'> {};
+template <> struct PythonFormat<int> : PassthroughFormat<int, 'i'> {};
+template <>
+struct PythonFormat<unsigned int> : PassthroughFormat<unsigned int, 'I'> {};
+template <> struct PythonFormat<long> : PassthroughFormat<long, 'l'> {};
+template <>
+struct PythonFormat<unsigned long> : PassthroughFormat<unsigned long, 'k'> {};
+template <>
+struct PythonFormat<long long> : PassthroughFormat<long long, 'L'> {};
+template <>
+struct PythonFormat<unsigned long long>
+    : PassthroughFormat<unsigned long long, 'K'> {};
+template <>
+struct PythonFormat<PyObject *> : PassthroughFormat<PyObject *, 'O'> {};
 
 template <typename T>
 struct PythonFormat<
     T, typename std::enable_if<std::is_base_of<PythonObject, T>::value>::type> {
   static constexpr char format = 'O';
   static auto get(const T &value) { return value.get(); }
 };
 

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

ScriptInterpreterPythonImpl::OSPlugin_CreatePluginObject(
   return StructuredData::GenericSP(
       new StructuredPythonObject(std::move(ret_val)));
 }
 
 StructuredData::DictionarySP ScriptInterpreterPythonImpl::OSPlugin_RegisterInfo(
     StructuredData::ObjectSP os_plugin_object_sp) {
   Locker py_lock(this, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock);
 
-  static char callee_name[] = "get_register_info";
-
   if (!os_plugin_object_sp)
-    return StructuredData::DictionarySP();
+    return {};
 
   StructuredData::Generic *generic = os_plugin_object_sp->GetAsGeneric();
   if (!generic)
-    return nullptr;
+    return {};
 
   PythonObject implementor(PyRefType::Borrowed,
                            (PyObject *)generic->GetValue());
 
   if (!implementor.IsAllocated())
-    return StructuredData::DictionarySP();
-
-  PythonObject pmeth(PyRefType::Owned,
-                     PyObject_GetAttrString(implementor.get(), callee_name));
-
-  if (PyErr_Occurred())
-    PyErr_Clear();
-
-  if (!pmeth.IsAllocated())
-    return StructuredData::DictionarySP();
+    return {};
 
-  if (PyCallable_Check(pmeth.get()) == 0) {
-    if (PyErr_Occurred())
-      PyErr_Clear();
+  llvm::Expected<PythonObject> expected_py_return =
+      implementor.CallMethod("get_register_info");
 
-    return StructuredData::DictionarySP();
+  if (!expected_py_return) {
+    llvm::consumeError(expected_py_return.takeError());
+    return {};
   }
 
-  if (PyErr_Occurred())
-    PyErr_Clear();
-
-  // right now we know this function exists and is callable..
-  PythonObject py_return(
-      PyRefType::Owned,
-      PyObject_CallMethod(implementor.get(), callee_name, nullptr));
+  PythonObject py_return = std::move(expected_py_return.get());
 
-  // if it fails, print the error but otherwise go on
-  if (PyErr_Occurred()) {
-    PyErr_Print();
-    PyErr_Clear();
-  }
   if (py_return.get()) {
     PythonDictionary result_dict(PyRefType::Borrowed, py_return.get());
     return result_dict.CreateStructuredDictionary();
   }
   return StructuredData::DictionarySP();
 }
 
 StructuredData::ArraySP ScriptInterpreterPythonImpl::OSPlugin_ThreadsInfo(
     StructuredData::ObjectSP os_plugin_object_sp) {
   Locker py_lock(this, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock);
-
-  static char callee_name[] = "get_thread_info";
-
   if (!os_plugin_object_sp)
-    return StructuredData::ArraySP();
+    return {};
 
   StructuredData::Generic *generic = os_plugin_object_sp->GetAsGeneric();
   if (!generic)
-    return nullptr;
+    return {};
 
   PythonObject implementor(PyRefType::Borrowed,
                            (PyObject *)generic->GetValue());
 
   if (!implementor.IsAllocated())
-    return StructuredData::ArraySP();
-
-  PythonObject pmeth(PyRefType::Owned,
-                     PyObject_GetAttrString(implementor.get(), callee_name));
-
-  if (PyErr_Occurred())
-    PyErr_Clear();
-
-  if (!pmeth.IsAllocated())
-    return StructuredData::ArraySP();
+    return {};
 
-  if (PyCallable_Check(pmeth.get()) == 0) {
-    if (PyErr_Occurred())
-      PyErr_Clear();
+  llvm::Expected<PythonObject> expected_py_return =
+      implementor.CallMethod("get_thread_info");
 
-    return StructuredData::ArraySP();
+  if (!expected_py_return) {
+    llvm::consumeError(expected_py_return.takeError());
+    return {};
   }
 
-  if (PyErr_Occurred())
-    PyErr_Clear();
-
-  // right now we know this function exists and is callable..
-  PythonObject py_return(
-      PyRefType::Owned,
-      PyObject_CallMethod(implementor.get(), callee_name, nullptr));
-
-  // if it fails, print the error but otherwise go on
-  if (PyErr_Occurred()) {
-    PyErr_Print();
-    PyErr_Clear();
-  }
+  PythonObject py_return = std::move(expected_py_return.get());
 
   if (py_return.get()) {
     PythonList result_list(PyRefType::Borrowed, py_return.get());
     return result_list.CreateStructuredArray();
   }
   return StructuredData::ArraySP();
 }
 
 StructuredData::StringSP
 ScriptInterpreterPythonImpl::OSPlugin_RegisterContextData(
     StructuredData::ObjectSP os_plugin_object_sp, lldb::tid_t tid) {
   Locker py_lock(this, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock);
 
-  static char callee_name[] = "get_register_data";
-  static char *param_format =
-      const_cast<char *>(GetPythonValueFormatString(tid));
-
   if (!os_plugin_object_sp)
-    return StructuredData::StringSP();
+    return {};
 
   StructuredData::Generic *generic = os_plugin_object_sp->GetAsGeneric();
   if (!generic)
-    return nullptr;
+    return {};
   PythonObject implementor(PyRefType::Borrowed,
                            (PyObject *)generic->GetValue());
 
   if (!implementor.IsAllocated())
-    return StructuredData::StringSP();
-
-  PythonObject pmeth(PyRefType::Owned,
-                     PyObject_GetAttrString(implementor.get(), callee_name));
-
-  if (PyErr_Occurred())
-    PyErr_Clear();
+    return {};
 
-  if (!pmeth.IsAllocated())
-    return StructuredData::StringSP();
+  llvm::Expected<PythonObject> expected_py_return =
+      implementor.CallMethod("get_register_data", tid);
 
-  if (PyCallable_Check(pmeth.get()) == 0) {
-    if (PyErr_Occurred())
-      PyErr_Clear();
-    return StructuredData::StringSP();
+  if (!expected_py_return) {
+    llvm::consumeError(expected_py_return.takeError());
+    return {};
   }
 
-  if (PyErr_Occurred())
-    PyErr_Clear();
-
-  // right now we know this function exists and is callable..
-  PythonObject py_return(
-      PyRefType::Owned,
-      PyObject_CallMethod(implementor.get(), callee_name, param_format, tid));
-
-  // if it fails, print the error but otherwise go on
-  if (PyErr_Occurred()) {
-    PyErr_Print();
-    PyErr_Clear();
-  }
+  PythonObject py_return = std::move(expected_py_return.get());
 
   if (py_return.get()) {
     PythonBytes result(PyRefType::Borrowed, py_return.get());
     return result.CreateStructuredString();
   }
-  return StructuredData::StringSP();
+  return {};
 }
 
 StructuredData::DictionarySP ScriptInterpreterPythonImpl::OSPlugin_CreateThread(
     StructuredData::ObjectSP os_plugin_object_sp, lldb::tid_t tid,
     lldb::addr_t context) {
   Locker py_lock(this, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock);
 
-  static char callee_name[] = "create_thread";
-  std::string param_format;
-  param_format += GetPythonValueFormatString(tid);
-  param_format += GetPythonValueFormatString(context);
-
   if (!os_plugin_object_sp)
-    return StructuredData::DictionarySP();
+    return {};
 
   StructuredData::Generic *generic = os_plugin_object_sp->GetAsGeneric();
   if (!generic)
-    return nullptr;
+    return {};
 
   PythonObject implementor(PyRefType::Borrowed,
                            (PyObject *)generic->GetValue());
 
   if (!implementor.IsAllocated())
-    return StructuredData::DictionarySP();
-
-  PythonObject pmeth(PyRefType::Owned,
-                     PyObject_GetAttrString(implementor.get(), callee_name));
-
-  if (PyErr_Occurred())
-    PyErr_Clear();
+    return {};
 
-  if (!pmeth.IsAllocated())
-    return StructuredData::DictionarySP();
+  llvm::Expected<PythonObject> expected_py_return =
+      implementor.CallMethod("create_thread", tid, context);
 
-  if (PyCallable_Check(pmeth.get()) == 0) {
-    if (PyErr_Occurred())
-      PyErr_Clear();
-    return StructuredData::DictionarySP();
+  if (!expected_py_return) {
+    llvm::consumeError(expected_py_return.takeError());
+    return {};
   }
 
-  if (PyErr_Occurred())
-    PyErr_Clear();
-
-  // right now we know this function exists and is callable..
-  PythonObject py_return(PyRefType::Owned,
-                         PyObject_CallMethod(implementor.get(), callee_name,
-                                             &param_format[0], tid, context));
-
-  // if it fails, print the error but otherwise go on
-  if (PyErr_Occurred()) {
-    PyErr_Print();
-    PyErr_Clear();
-  }
+  PythonObject py_return = std::move(expected_py_return.get());
 
   if (py_return.get()) {
     PythonDictionary result_dict(PyRefType::Borrowed, py_return.get());
     return result_dict.CreateStructuredDictionary();
   }
   return StructuredData::DictionarySP();
 }
 
lldb::ValueObjectSP ScriptInterpreterPythonImpl::GetSyntheticValue(
   return ret_val;
 }
 
 ConstString ScriptInterpreterPythonImpl::GetSyntheticTypeName(
     const StructuredData::ObjectSP &implementor_sp) {
   Locker py_lock(this,
                  Locker::AcquireLock | Locker::InitSession | Locker::NoSTDIN);
 
-  static char callee_name[] = "get_type_name";
-
-  ConstString ret_val;
-  bool got_string = false;
-  std::string buffer;
-
   if (!implementor_sp)
-    return ret_val;
+    return {};
 
   StructuredData::Generic *generic = implementor_sp->GetAsGeneric();
   if (!generic)
-    return ret_val;
+    return {};
+
   PythonObject implementor(PyRefType::Borrowed,
                            (PyObject *)generic->GetValue());
   if (!implementor.IsAllocated())
-    return ret_val;
-
-  PythonObject pmeth(PyRefType::Owned,
-                     PyObject_GetAttrString(implementor.get(), callee_name));
-
-  if (PyErr_Occurred())
-    PyErr_Clear();
+    return {};
 
-  if (!pmeth.IsAllocated())
-    return ret_val;
+  llvm::Expected<PythonObject> expected_py_return =
+      implementor.CallMethod("get_type_name");
 
-  if (PyCallable_Check(pmeth.get()) == 0) {
-    if (PyErr_Occurred())
-      PyErr_Clear();
-    return ret_val;
+  if (!expected_py_return) {
+    llvm::consumeError(expected_py_return.takeError());
+    return {};
   }
 
-  if (PyErr_Occurred())
-    PyErr_Clear();
+  PythonObject py_return = std::move(expected_py_return.get());
 
-  // right now we know this function exists and is callable..
-  PythonObject py_return(
-      PyRefType::Owned,
-      PyObject_CallMethod(implementor.get(), callee_name, nullptr));
-
-  // if it fails, print the error but otherwise go on
-  if (PyErr_Occurred()) {
-    PyErr_Print();
-    PyErr_Clear();
-  }
+  ConstString ret_val;
+  bool got_string = false;
+  std::string buffer;
 
   if (py_return.IsAllocated() && PythonString::Check(py_return.get())) {
     PythonString py_string(PyRefType::Borrowed, py_return.get());
     llvm::StringRef return_data(py_string.GetString());
     if (!return_data.empty()) {
       buffer.assign(return_data.data(), return_data.size());
       got_string = true;
     }

 }
 
 bool ScriptInterpreterPythonImpl::GetShortHelpForCommandObject(
     StructuredData::GenericSP cmd_obj_sp, std::string &dest) {
   dest.clear();
 
   Locker py_lock(this, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock);
 
-  static char callee_name[] = "get_short_help";
-
   if (!cmd_obj_sp)
     return false;
 
   PythonObject implementor(PyRefType::Borrowed,
                            (PyObject *)cmd_obj_sp->GetValue());
 
   if (!implementor.IsAllocated())
     return false;
 
-  PythonObject pmeth(PyRefType::Owned,
-                     PyObject_GetAttrString(implementor.get(), callee_name));
+  llvm::Expected<PythonObject> expected_py_return =
+      implementor.CallMethod("get_short_help");
 
-  if (PyErr_Occurred())
-    PyErr_Clear();
-
-  if (!pmeth.IsAllocated())
-    return false;
-
-  if (PyCallable_Check(pmeth.get()) == 0) {
-    if (PyErr_Occurred())
-      PyErr_Clear();
+  if (!expected_py_return) {
+    llvm::consumeError(expected_py_return.takeError());
     return false;
   }
 
-  if (PyErr_Occurred())
-    PyErr_Clear();
-
-  // Right now we know this function exists and is callable.
-  PythonObject py_return(
-      PyRefType::Owned,
-      PyObject_CallMethod(implementor.get(), callee_name, nullptr));
-
-  // If it fails, print the error but otherwise go on.
-  if (PyErr_Occurred()) {
-    PyErr_Print();
-    PyErr_Clear();
-  }
+  PythonObject py_return = std::move(expected_py_return.get());
 
   if (py_return.IsAllocated() && PythonString::Check(py_return.get())) {
     PythonString py_string(PyRefType::Borrowed, py_return.get());
     llvm::StringRef return_data(py_string.GetString());
     dest.assign(return_data.data(), return_data.size());
     return true;
   }
 
if (PyErr_Occurred()) {
     result = py_return;
   }
 
   return result;
 }
 
 bool ScriptInterpreterPythonImpl::GetLongHelpForCommandObject(
     StructuredData::GenericSP cmd_obj_sp, std::string &dest) {
-  bool got_string = false;
   dest.clear();
 
   Locker py_lock(this, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock);
 
-  static char callee_name[] = "get_long_help";
-
   if (!cmd_obj_sp)
     return false;
 
   PythonObject implementor(PyRefType::Borrowed,
                            (PyObject *)cmd_obj_sp->GetValue());
 
   if (!implementor.IsAllocated())
     return false;
 
-  PythonObject pmeth(PyRefType::Owned,
-                     PyObject_GetAttrString(implementor.get(), callee_name));
-
-  if (PyErr_Occurred())
-    PyErr_Clear();
-
-  if (!pmeth.IsAllocated())
-    return false;
-
-  if (PyCallable_Check(pmeth.get()) == 0) {
-    if (PyErr_Occurred())
-      PyErr_Clear();
+  llvm::Expected<PythonObject> expected_py_return =
+      implementor.CallMethod("get_long_help");
 
+  if (!expected_py_return) {
+    llvm::consumeError(expected_py_return.takeError());
     return false;
   }
 
-  if (PyErr_Occurred())
-    PyErr_Clear();
-
-  // right now we know this function exists and is callable..
-  PythonObject py_return(
-      PyRefType::Owned,
-      PyObject_CallMethod(implementor.get(), callee_name, nullptr));
-
-  // if it fails, print the error but otherwise go on
-  if (PyErr_Occurred()) {
-    PyErr_Print();
-    PyErr_Clear();
-  }
+  PythonObject py_return = std::move(expected_py_return.get());
 
+  bool got_string = false;
   if (py_return.IsAllocated() && PythonString::Check(py_return.get())) {
     PythonString str(PyRefType::Borrowed, py_return.get());
     llvm::StringRef str_data(str.GetString());
     dest.assign(str_data.data(), str_data.size());
     got_string = true;
   }
 
   return got_string;