[PATCH] Convert py-connection.c to the Python safety API

[PATCH] Convert py-connection.c to the Python safety API

[Tom Tromey]

This changes much of py-connection.c to the Python safety API.

A few things aren't yet changed:

* Event emission
* target_to_connection_object

These will be dealt with later.

Note that this patch fixes a reference leak in the "send_packet"
command.  Previously it did:

-      packet_obj = PyUnicode_AsASCIIString (packet_obj);

However PyUnicode_AsASCIIString returns a new reference; and this
reference was never released.

This patch includes the noargs_function template function from another
patch I sent.  The two are identical.

I made a couple of safety-related changes to py-wrappers.h.  Note that
there is one special case where the buffer returned by
PyBytes_AsStringAndSize may be written to.  If gdb ever needs to
exploit this case, I think a new API should be introduced.
---
 gdb/python/py-connection.c   | 316 +++++++++++++++--------------------
 gdb/python/py-safety.h       |  21 +++
 gdb/python/py-wrappers.h     |  29 +++-
 gdb/python/python-internal.h |   2 +-
 gdb/python/python.c          |   4 +-
 5 files changed, 180 insertions(+), 192 deletions(-)

diff --git a/gdb/python/py-connection.c b/gdb/python/py-connection.c
index bc738669b79..7df2618bcd3 100644
--- a/gdb/python/py-connection.c
+++ b/gdb/python/py-connection.c
@@ -40,6 +40,53 @@ struct connection_object : public PyObject
      indicates that this Python object is now in the invalid state (see
      the is_valid() method below).  */
   struct process_stratum_target *target;
+
+  /* Require that this object be valid.  */
+  void require () const;
+
+  /* Implement is_valid method.  */
+  bool is_valid () const
+  {
+    return target != nullptr;
+  }
+
+  /* Return the id number of this connection.  */
+  int get_connection_num () const
+  {
+    require ();
+    return target->connection_number;
+  }
+
+  /* Return a string that gives the short name for this connection type.  */
+  const char *get_connection_type () const
+  {
+    require ();
+    return target->shortname ();
+  }
+
+  /* Return a string that gives a longer description of this
+     connection type.  */
+  const char *get_description () const
+  {
+    require ();
+    return target->longname ();
+  }
+
+  /* Return a string that gives additional details about this
+     connection, or None, if there are no additional details for this
+     connection type.  */
+  const char *get_connection_details () const
+  {
+    require ();
+    return target->connection_string ();
+  }
+
+  /* Implement repr() for gdb.TargetConnection.  */
+  gdbpy_ref<> repr ();
+
+  /* The send_packet method.  */
+  gdbpy_ref<> send_packet (gdbpy_borrowed_ref<> args,
+			   gdbpy_opt_borrowed_ref<> kw);
 };
 
 static_assert (gdb::is_python_allocatable_v<connection_object>);
@@ -48,23 +95,20 @@ extern PyTypeObject connection_object_type;
 
 extern PyTypeObject remote_connection_object_type;
 
-/* Require that CONNECTION be valid.  */
-#define CONNPY_REQUIRE_VALID(connection)			\
-  do {								\
-    if (connection->target == nullptr)				\
-      {								\
-	PyErr_SetString (PyExc_RuntimeError,			\
-			 _("Connection no longer exists."));	\
-	return nullptr;						\
-      }								\
-  } while (0)
-
 /* A map between process_stratum targets and the Python object representing
    them.  We actually hold a gdbpy_ref around the Python object so that
    reference counts are handled correctly when entries are deleted.  */
 static gdb::unordered_map<process_stratum_target *,
 			  gdbpy_ref<connection_object>> all_connection_objects;
 
+void
+connection_object::require () const
+{
+  if (target == nullptr)
+    gdbpy_err_set_string (PyExc_RuntimeError,
+			  _("Connection no longer exists."));
+}
+
 /* Return a reference to a gdb.TargetConnection object for TARGET.  If
    TARGET is nullptr then a reference to None is returned.
 
@@ -107,27 +151,26 @@ target_to_connection_object (process_stratum_target *target)
 /* Return a list of gdb.TargetConnection objects, one for each currently
    active connection.  The returned list is in no particular order.  */
 
-PyObject *
-gdbpy_connections (PyObject *self, PyObject *args)
+gdbpy_ref<>
+gdbpy_connections ()
 {
-  gdbpy_ref<> list (PyList_New (0));
-  if (list == nullptr)
-    return nullptr;
+  gdbpy_ref<> list = gdbpy_new_list (0);
 
   for (process_stratum_target *target : all_non_exited_process_targets ())
     {
       gdb_assert (target != nullptr);
 
       gdbpy_ref<> conn = target_to_connection_object (target);
+      /* FIXME: Python safety.  target_to_connection_object should
+	 throw on error.  */
       if (conn == nullptr)
 	return nullptr;
       gdb_assert (conn.get () != Py_None);
 
-      if (PyList_Append (list.get (), conn.get ()) < 0)
-	return nullptr;
+      gdbpy_list_append (list, conn);
     }
 
-  return list.release ();
+  return list;
 }
 
 /* Emit a connection event for TARGET to REGISTRY.  Return 0 on success, or
@@ -193,90 +236,20 @@ connpy_connection_dealloc (PyObject *obj)
 
 /* Implement repr() for gdb.TargetConnection.  */
 
-static PyObject *
-connpy_repr (PyObject *obj)
+gdbpy_ref<>
+connection_object::repr ()
 {
-  connection_object *self = (connection_object *) obj;
-  process_stratum_target *target = self->target;
-
   if (target == nullptr)
-    return gdb_py_invalid_object_repr (obj);
-
-  return PyUnicode_FromFormat ("<%s num=%d, what=\"%s\">",
-			       gdbpy_py_obj_tp_name (obj).c_str (),
-			       target->connection_number,
-			       make_target_connection_string (target).c_str ());
-}
-
-/* Implementation of gdb.TargetConnection.is_valid() -> Boolean.  Returns
-   True if this connection object is still associated with a
-   process_stratum_target, otherwise, returns False.  */
-
-static PyObject *
-connpy_is_valid (PyObject *self, PyObject *args)
-{
-  connection_object *conn = (connection_object *) self;
-
-  if (conn->target == nullptr)
-    return py_false ().release ();
-
-  return py_true ().release ();
-}
-
-/* Return the id number of this connection.  */
-
-static PyObject *
-connpy_get_connection_num (PyObject *self, void *closure)
-{
-  connection_object *conn = (connection_object *) self;
-
-  CONNPY_REQUIRE_VALID (conn);
-
-  auto num = conn->target->connection_number;
-  return gdb_py_object_from_longest (num).release ();
-}
-
-/* Return a string that gives the short name for this connection type.  */
-
-static PyObject *
-connpy_get_connection_type (PyObject *self, void *closure)
-{
-  connection_object *conn = (connection_object *) self;
-
-  CONNPY_REQUIRE_VALID (conn);
-
-  const char *shortname = conn->target->shortname ();
-  return host_string_to_python_string (shortname).release ();
-}
-
-/* Return a string that gives a longer description of this connection type.  */
-
-static PyObject *
-connpy_get_description (PyObject *self, void *closure)
-{
-  connection_object *conn = (connection_object *) self;
-
-  CONNPY_REQUIRE_VALID (conn);
-
-  const char *longname = conn->target->longname ();
-  return host_string_to_python_string (longname).release ();
-}
-
-/* Return a string that gives additional details about this connection, or
-   None, if there are no additional details for this connection type.  */
-
-static PyObject *
-connpy_get_connection_details (PyObject *self, void *closure)
-{
-  connection_object *conn = (connection_object *) self;
-
-  CONNPY_REQUIRE_VALID (conn);
-
-  const char *details = conn->target->connection_string ();
-  if (details != nullptr)
-    return host_string_to_python_string (details).release ();
-  else
-    return py_none ().release ();
+    /* FIXME: Python safety.  gdb_py_invalid_object_repr ought to
+       throw on error, and return gdbpy_ref<>, but currently does
+       not.  */
+    return gdbpy_ref<> (gdb_py_invalid_object_repr (this));
+
+  return (gdbpy_unicode_from_format
+	  ("<%s num=%d, what=\"%s\">",
+	   gdbpy_py_obj_tp_name (this).c_str (),
+	   target->connection_number,
+	   make_target_connection_string (target).c_str ()));
 }
 
 /* Python specific initialization for this file.  */
@@ -310,16 +283,16 @@ struct py_send_packet_callbacks : public send_remote_packet_callbacks
   void sending (gdb::array_view<const char> &buf) override
   { /* Nothing.  */ }
 
-  /* When the result is returned create a Python object and assign this
-     into M_RESULT.  If for any reason we can't create a Python object to
-     represent the result then M_RESULT is set to nullptr, and Python's
-     internal error flags will be set.  If the result we got back from the
-     remote is empty then set the result to None.  */
+  /* When the result is returned create a Python object and assign
+     this into M_RESULT.  If for any reason we can't create a Python
+     object to represent the result then an exception is thrown.  If
+     the result we got back from the remote is empty then set the
+     result to None.  */
 
   void received (gdb::array_view<const char> &buf) override
   {
     if (buf.size () > 0 && buf.data ()[0] != '\0')
-      m_result.reset (PyBytes_FromStringAndSize (buf.data (), buf.size ()));
+      m_result = gdbpy_bytes_from_string_and_size (buf);
     else
       {
 	/* We didn't get back any result data; set the result to None.  */
@@ -327,22 +300,14 @@ struct py_send_packet_callbacks : public send_remote_packet_callbacks
       }
   }
 
-  /* Get a reference to the result as a Python object.  It is invalid to
-     call this before sending a packet to the remote and processing the
-     reply.
+  /* Return the resulting Python object.  It is invalid to call this
+     before sending a packet to the remote and processing the reply.
 
-     The result value is setup in the RECEIVED call above.  If the RECEIVED
-     call causes an error then the result value will be set to nullptr,
-     and the error reason is left stored in Python's global error state.
+     The result value is setup in the RECEIVED call above.  */
 
-     It is important that the result is inspected immediately after sending
-     a packet to the remote, and any error fetched,  calling any other
-     Python functions that might clear the error state, or rely on an error
-     not being set will cause undefined behavior.  */
-
-  gdbpy_ref<> result () const
+  gdbpy_ref<> &&result ()
   {
-    return m_result;
+    return std::move (m_result);
   }
 
 private:
@@ -357,70 +322,45 @@ struct py_send_packet_callbacks : public send_remote_packet_callbacks
    the packet to be sent must be non-empty, otherwise an exception will be
    thrown.  */
 
-static PyObject *
-connpy_send_packet (PyObject *self, PyObject *args, PyObject *kw)
+gdbpy_ref<>
+connection_object::send_packet (gdbpy_borrowed_ref<> args,
+				gdbpy_opt_borrowed_ref<> kw)
 {
-  connection_object *conn = (connection_object *) self;
-
-  CONNPY_REQUIRE_VALID (conn);
+  require ();
 
   static const char *keywords[] = {"packet", nullptr};
   PyObject *packet_obj;
 
-  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "O", keywords,
-					&packet_obj))
-    return nullptr;
+  gdbpy_arg_parse_tuple_and_keywords (args, kw, "O", keywords, &packet_obj);
 
   /* If the packet is a unicode string then convert it to a bytes object.  */
+  gdbpy_ref<> ascii_object;
   if (PyUnicode_Check (packet_obj))
     {
       /* We encode the string to bytes using the ascii codec, if this fails
 	 then a suitable error will have been set.  */
-      packet_obj = PyUnicode_AsASCIIString (packet_obj);
-      if (packet_obj == nullptr)
-	return nullptr;
+      ascii_object = gdbpy_unicode_as_ascii_string (packet_obj);
+      packet_obj = ascii_object.get ();
     }
 
   /* Check the packet is now a bytes object.  */
   if (!PyBytes_Check (packet_obj))
-    {
-      PyErr_SetString (PyExc_TypeError, _("Packet is not a bytes object"));
-      return nullptr;
-    }
+    gdbpy_err_set_string (PyExc_TypeError, _("Packet is not a bytes object"));
 
   Py_ssize_t packet_len = 0;
-  char *packet_str_nonconst = nullptr;
-  if (PyBytes_AsStringAndSize (packet_obj, &packet_str_nonconst,
-			       &packet_len) < 0)
-    return nullptr;
-  const char *packet_str = packet_str_nonconst;
-  gdb_assert (packet_str != nullptr);
+  const char *packet_str = nullptr;
+  gdbpy_bytes_as_string_and_size (packet_obj, &packet_str, &packet_len);
 
   if (packet_len == 0)
-    {
-      PyErr_SetString (PyExc_ValueError, _("Packet must not be empty"));
-      return nullptr;
-    }
+    gdbpy_err_set_string (PyExc_ValueError, _("Packet must not be empty"));
 
-  try
-    {
-      scoped_restore_current_thread restore_thread;
-      switch_to_target_no_thread (conn->target);
-
-      gdb::array_view<const char> view (packet_str, packet_len);
-      py_send_packet_callbacks callbacks;
-      send_remote_packet (view, &callbacks);
-      PyObject *result = callbacks.result ().release ();
-      /* If we encountered an error converting the reply to a Python
-	 object, then the result here can be nullptr.  In that case, Python
-	 should be aware that an error occurred.  */
-      gdb_assert ((result == nullptr) == (PyErr_Occurred () != nullptr));
-      return result;
-    }
-  catch (const gdb_exception &except)
-    {
-      return gdbpy_handle_gdb_exception (nullptr, except);
-    }
+  scoped_restore_current_thread restore_thread;
+  switch_to_target_no_thread (target);
+
+  gdb::array_view<const char> view (packet_str, packet_len);
+  py_send_packet_callbacks callbacks;
+  send_remote_packet (view, &callbacks);
+  return callbacks.result ();
 }
 
 /* Global initialization for this file.  */
@@ -437,36 +377,48 @@ GDBPY_INITIALIZE_FILE (gdbpy_initialize_connection);
 
 static PyMethodDef connection_object_methods[] =
 {
-  { "is_valid", connpy_is_valid, METH_NOARGS,
+  noargs_method<connection_object, &connection_object::is_valid> ("is_valid",
     "is_valid () -> Boolean.\n\
-Return true if this TargetConnection is valid, false if not." },
-  { NULL }
+Return true if this TargetConnection is valid, false if not."),
+  { nullptr }
 };
 
 /* Methods for the gdb.RemoteTargetConnection object type.  */
 
 static PyMethodDef remote_connection_object_methods[] =
 {
-  { "send_packet", (PyCFunction) connpy_send_packet,
-    METH_VARARGS | METH_KEYWORDS,
+  varargs_method<connection_object, &connection_object::send_packet>
+   ("send_packet",
     "send_packet (PACKET) -> Bytes\n\
-Send PACKET to a remote target, return the reply as a bytes array." },
-  { NULL }
+Send PACKET to a remote target, return the reply as a bytes array."),
+  { nullptr }
 };
 
 /* Attributes for the gdb.TargetConnection object type.  */
 
 static gdb_PyGetSetDef connection_object_getset[] =
 {
-  { "num", connpy_get_connection_num, NULL,
-    "ID number of this connection, as assigned by GDB.", NULL },
-  { "type", connpy_get_connection_type, NULL,
-    "A short string that is the name for this connection type.", NULL },
-  { "description", connpy_get_description, NULL,
-    "A longer string describing this connection type.", NULL },
-  { "details", connpy_get_connection_details, NULL,
-    "A string containing additional connection details.", NULL },
-  { NULL }
+  { "num",
+    wrap_getter<connection_object, &connection_object::get_connection_num>,
+    nullptr,
+    "ID number of this connection, as assigned by GDB.",
+    nullptr },
+  { "type",
+    wrap_getter<connection_object, &connection_object::get_connection_type>,
+    nullptr,
+    "A short string that is the name for this connection type.",
+    nullptr },
+  { "description",
+    wrap_getter<connection_object, &connection_object::get_description>,
+    nullptr,
+    "A longer string describing this connection type.",
+    nullptr },
+  { "details",
+    wrap_getter<connection_object, &connection_object::get_connection_details>,
+    nullptr,
+    "A string containing additional connection details.",
+    nullptr },
+  { nullptr }
 };
 
 /* Define the gdb.TargetConnection object type.  */
@@ -482,7 +434,7 @@ PyTypeObject connection_object_type =
   0,				  /* tp_getattr */
   0,				  /* tp_setattr */
   0,				  /* tp_compare */
-  connpy_repr,			  /* tp_repr */
+  wrap_tp_callback<connection_object, &connection_object::repr>, /* tp_repr */
   0,				  /* tp_as_number */
   0,				  /* tp_as_sequence */
   0,				  /* tp_as_mapping */
@@ -525,7 +477,7 @@ PyTypeObject remote_connection_object_type =
   0,				  /* tp_getattr */
   0,				  /* tp_setattr */
   0,				  /* tp_compare */
-  connpy_repr,			  /* tp_repr */
+  wrap_tp_callback<connection_object, &connection_object::repr>, /* tp_repr */
   0,				  /* tp_as_number */
   0,				  /* tp_as_sequence */
   0,				  /* tp_as_mapping */
diff --git a/gdb/python/py-safety.h b/gdb/python/py-safety.h
index 3294f38c8b6..06324868817 100644
--- a/gdb/python/py-safety.h
+++ b/gdb/python/py-safety.h
@@ -233,6 +233,27 @@ varargs_wrapper (PyObject *self, PyObject *args, PyObject *kw)
 
 } /* namespace safety_details */
 
+/* Create a PyMethodDef for a no-argument function.  It takes the
+   underlying function F as template parameters, and the name and
+   documentation as arguments.  The function F is wrapped to call
+   to_python and to catch exceptions per the safety protocol.  F
+   should not accept any arguments.  */
+template<auto F>
+constexpr PyMethodDef
+noargs_function (const char *name, const char *doc)
+{
+  using namespace safety_details;
+  return {
+    name,
+    [] (PyObject *self, PyObject *args) -> PyObject *
+    {
+      return wrapped_function<F> ();
+    },
+    METH_NOARGS,
+    doc,
+  };
+}
+
 /* Create a PyMethodDef for a no-argument method.  It takes the
    underlying class C and a pointer-to-method M as template
    parameters, and the name and documentation as arguments.  The
diff --git a/gdb/python/py-wrappers.h b/gdb/python/py-wrappers.h
index 6c2b5e4d41e..c93722646c8 100644
--- a/gdb/python/py-wrappers.h
+++ b/gdb/python/py-wrappers.h
@@ -81,14 +81,18 @@ gdbpy_bytes_as_string (gdbpy_borrowed_ref<> ref)
   return result;
 }
 
-/* Wrapper for PyBytes_AsStringAndSize.  */
+/* Wrapper for PyBytes_AsStringAndSize.  Note that, unlike the
+   underlying Python function, this attempts to be const-correct --
+   the caller should not write to the returned buffer.  */
 static inline void
 gdbpy_bytes_as_string_and_size (gdbpy_borrowed_ref<> ref,
-				char **buffer,
+				const char **buffer,
 				Py_ssize_t *length)
 {
-  if (PyBytes_AsStringAndSize (ref, buffer, length) == -1)
+  char *temp;
+  if (PyBytes_AsStringAndSize (ref, &temp, length) == -1)
     throw gdb_python_exception ();
+  *buffer = temp;
 }
 
 /* Wrapper for PyBytes_FromString.  */
@@ -103,13 +107,14 @@ gdbpy_bytes_from_string (const char *str)
 }
 
 /* Wrapper for PyBytes_FromStringAndSize.  */
-static inline gdbpy_ref<>
-gdbpy_bytes_from_string_and_size (const char *str, Py_ssize_t len)
+template<typename T>
+gdbpy_ref<>
+gdbpy_bytes_from_string_and_size (gdb::array_view<T> data)
 {
   /* Python allows STR==nullptr but it leaves the object
      uninitialized, and I think we should avoid this in gdb.  */
-  gdb_assert (str != nullptr);
-  gdbpy_ref<> result (PyBytes_FromStringAndSize (str, len));
+  gdb_assert (data.data () != nullptr);
+  gdbpy_ref<> result (PyBytes_FromStringAndSize (data.data (), data.size ()));
   if (result == nullptr)
     throw gdb_python_exception ();
   return result;
@@ -217,6 +222,16 @@ gdbpy_unicode_from_format (const char *fmt, Arg... args)
   return result;
 }
 
+/* Wrapper for PyUnicode_AsASCIIString.  */
+static inline gdbpy_ref<>
+gdbpy_unicode_as_ascii_string (gdbpy_borrowed_ref<> arg)
+{
+  gdbpy_ref<> result (PyUnicode_AsASCIIString (arg));
+  if (result == nullptr)
+    throw gdb_python_exception ();
+  return result;
+}
+
 /* Wrapper for PyErr_SetString.  This always throws.  */
 [[noreturn]] static inline void
 gdbpy_err_set_string (gdbpy_borrowed_ref<> type, const char *str)
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 2e8f35729cd..9d6c8eb4a01 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -541,7 +541,7 @@ PyObject *gdbpy_buffer_to_membuf (gdb::unique_xmalloc_ptr<gdb_byte> buffer,
 
 struct process_stratum_target;
 gdbpy_ref<> target_to_connection_object (process_stratum_target *target);
-PyObject *gdbpy_connections (PyObject *self, PyObject *args);
+gdbpy_ref<> gdbpy_connections ();
 
 const struct block *block_object_to_block (PyObject *obj);
 struct symbol *symbol_object_to_symbol (PyObject *obj);
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 14c243b135e..c7ca29f9b01 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -3279,9 +3279,9 @@ Register a TUI window constructor."),
     "architecture_names () -> List.\n\
 Return a list of all the architecture names GDB understands." },
 
-  { "connections", gdbpy_connections, METH_NOARGS,
+  noargs_function<gdbpy_connections> ("connections",
     "connections () -> List.\n\
-Return a list of gdb.TargetConnection objects." },
+Return a list of gdb.TargetConnection objects."),
 
   { "format_address", (PyCFunction) gdbpy_format_address,
     METH_VARARGS | METH_KEYWORDS,
-- 
2.49.0