[PATCH v2 0/4] Python safety initial work

[PATCH v2 0/4] Python safety initial work

[Tom Tromey]

I rewrote my earlier Python safety series to use methods a while back.
And while I like the result, I got bogged down converting things, so I
thought I'd send some initial work, with the idea that if this looks
good, other code can be transformed in pieces.

Since this is extracted from a larger series, there is some code in
here that isn't currently used in-tree.  This is mainly true for the
gdb functions that wrap Python APIs, see patch 2.

Patch 4 shows what the end effect is, but the patch is somewhat hard
to read IMO.  So for instance now the 'title' getter for a TUI window
is just an ordinary method:

    const std::string &
    gdbpy_tui_window::title ()
    {
      require_valid ();
      return window->title ();
    }

Since it wants to only operate on a valid window, require_valid will
throw if that isn't the case.

Note there's no explicit error checking, no casts, no messing with
reference counts, and the return type is just a string.  All these
boilerplate details are handled by a template wrapper function.

Signed-off-by: Tom Tromey <tom@tromey.com>
---
Changes in v2:
- Avoids Py_RETURN_* macros
- Link to v1: https://inbox.sourceware.org/gdb-patches/20260515-python-safety-initial-v1-0-8f155338df57@tromey.com

---
Tom Tromey (4):
      Add gdbpy_borrowed_ref
      Add wrappers for some Python APIs
      Add wrappers for Python implementation functions and methods
      Convert py-tui.c to the "python safety" approach

 gdb/python/py-ref.h          |  80 +++++++++++
 gdb/python/py-safety.h       | 320 +++++++++++++++++++++++++++++++++++++++++
 gdb/python/py-tui.c          | 208 +++++++++++----------------
 gdb/python/py-wrappers.h     | 334 +++++++++++++++++++++++++++++++++++++++++++
 gdb/python/python-internal.h |   8 +-
 gdb/python/python.c          |   5 +-
 6 files changed, 829 insertions(+), 126 deletions(-)
---
base-commit: 5cb6772600a90b7a214b84277fc795c5d45e98fa
change-id: 20260515-python-safety-initial-422bf34a8ce3

Best regards,
-- 
Tom Tromey <tom@tromey.com>

[PATCH v2 1/4] Add gdbpy_borrowed_ref

[Tom Tromey]

This adds new gdbpy_opt_borrowed_ref and gdbpy_borrowed_ref classes.
These class is primarily for code "documentation" purposes -- it makes
it clear to the reader that a given reference is borrowed.  However,
they also add a tiny bit of safety, in that conversion to gdbpy_ref<>
will either be rejected (by the "opt" class) or acquire a new
reference.
---
 gdb/python/py-ref.h | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/gdb/python/py-ref.h b/gdb/python/py-ref.h
index dc0b14814af..ef6b9fb427c 100644
--- a/gdb/python/py-ref.h
+++ b/gdb/python/py-ref.h
@@ -41,6 +41,86 @@ struct gdbpy_ref_policy
 template<typename T = PyObject> using gdbpy_ref
   = gdb::ref_ptr<T, gdbpy_ref_policy>;
 
+/* A class representing an optional borrowed reference.  It is
+   "optional" because NULL is a valid value.
+
+   This is a simple wrapper for a PyObject*.  Aside from documenting
+   what the code does, the main advantage of using this is that
+   conversion to a gdbpy_ref<> is prevented.
+
+   An optional borrowed reference is only used in situations where
+   Python says NULL is valid.  For example, it is used as the type of
+   the "keywords" argument to a varargs method.  Most code should
+   prefer an ordinary gdbpy_borrowed_ref, see below.  */
+class gdbpy_opt_borrowed_ref
+{
+public:
+
+  gdbpy_opt_borrowed_ref (PyObject *obj)
+    : m_obj (obj)
+  {
+  }
+
+  template<typename T>
+  gdbpy_opt_borrowed_ref (const gdbpy_ref<T> &ref)
+    : m_obj (ref.get ())
+  {
+  }
+
+  operator PyObject * ()
+  {
+    return m_obj;
+  }
+
+  operator gdbpy_ref<> () = delete;
+
+protected:
+  PyObject *m_obj;
+};
+
+/* A borrowed reference that is guaranteed not to be NULL.
+
+   Like gdbpy_opt_borrowed_ref, this mostly serves a documentary
+   purpose.  However, it also allows a checked cast to any subclass of
+   PyObject, and conversion to a gdbpy_ref<> will automatically
+   acquire a new reference -- a safety improvement over plain
+   PyObject*.  */
+class gdbpy_borrowed_ref : public gdbpy_opt_borrowed_ref
+{
+public:
+
+  gdbpy_borrowed_ref (PyObject *obj)
+    : gdbpy_opt_borrowed_ref (obj)
+  {
+    gdb_assert (m_obj != nullptr);
+  }
+
+  template<typename T>
+  gdbpy_borrowed_ref (const gdbpy_ref<T> &ref)
+    : gdbpy_opt_borrowed_ref (ref)
+  {
+    gdb_assert (m_obj != nullptr);
+  }
+
+  gdbpy_borrowed_ref (std::nullptr_t) = delete;
+
+  /* Allow a (checked) conversion to any subclass of PyObject.  */
+  template<typename T,
+	   typename = std::is_convertible<T *, PyObject *>>
+  operator T * ()
+  {
+    gdb_assert (PyObject_TypeCheck (m_obj, T::corresponding_object_type));
+    return static_cast<T *> (m_obj);
+  }
+
+  /* When converting a borrowed reference to a gdbpy_ref<>, a new
+     reference is acquired.  */
+  operator gdbpy_ref<> ()
+  {
+    return gdbpy_ref<>::new_reference (m_obj);
+  }
+};
+
 /* A wrapper class for Python extension objects that have a __dict__ attribute.
 
    Any Python C object extension needing __dict__ should inherit from this

-- 
2.49.0

[PATCH v2 2/4] Add wrappers for some Python APIs

[Tom Tromey]

This adds some new functions that wrap Python APIs.  The wrapping
follows some proposed rules for Python safety in gdb:

* Functions returning a new reference return gdbpy_ref<>

* Errors are reported via exceptions, not special values

* Functions accepting a stolen reference take a gdbpy_ref<>&&
---
 gdb/python/py-wrappers.h     | 334 +++++++++++++++++++++++++++++++++++++++++++
 gdb/python/python-internal.h |   2 +
 2 files changed, 336 insertions(+)

diff --git a/gdb/python/py-wrappers.h b/gdb/python/py-wrappers.h
new file mode 100644
index 00000000000..78c1f8070cf
--- /dev/null
+++ b/gdb/python/py-wrappers.h
@@ -0,0 +1,334 @@
+/* Wrappers for some Python safety.
+
+   Copyright (C) 2026 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef GDB_PYTHON_PY_WRAPPERS_H
+#define GDB_PYTHON_PY_WRAPPERS_H
+
+#include "py-ref.h"
+
+/* Gdb implements its own wrappers for many Python APIs.  This is done
+   in an attempt to be more safe.
+
+   In particular, in gdb:
+
+   - APIs returning a new reference will return gdbpy_ref<>.  This
+   makes reference counting errors less likely.
+
+   - APIs will throw an exception rather than return a special value
+   (NULL or -1).  This makes error checking simpler.
+
+   - APIs requiring a stolen reference take a gdbpy_ref<>&&, to make
+   reference counting errors less likely.
+
+   This file holds the currently-defined wrappers.  If new APIs are
+   needed, the normal approach is to add a wrapper here.
+
+   APIs here are named after the underlying Python function, but using
+   lower case and an "_" at each word break.  */
+
+/* The type of exception thrown when the Python exception has been
+   set.  */
+struct gdb_python_exception
+{
+  gdb_python_exception ()
+  {
+    gdb_assert (PyErr_Occurred ());
+  }
+};
+
+template<typename T>
+gdbpy_ref<T>
+gdbpy_new ()
+{
+  gdbpy_ref<T> result (PyObject_New (T, T::corresponding_object_type));
+  if (result == nullptr)
+    throw gdb_python_exception ();
+  return result;
+}
+
+static inline gdbpy_ref<>
+gdbpy_bool_from_long (long value)
+{
+  /* This cannot fail.  */
+  return gdbpy_ref<> (PyBool_FromLong (value));
+}
+
+static inline char *
+gdbpy_bytes_as_string (gdbpy_borrowed_ref ref)
+{
+  char *result = PyBytes_AsString (ref);
+  if (result == nullptr)
+    throw gdb_python_exception ();
+  return result;
+}
+
+static inline void
+gdbpy_bytes_as_string_and_size (gdbpy_borrowed_ref ref,
+				char **buffer,
+				Py_ssize_t *length)
+{
+  if (PyBytes_AsStringAndSize (ref, buffer, length) == -1)
+    throw gdb_python_exception ();
+}
+
+static inline gdbpy_ref<>
+gdbpy_bytes_from_string (const char *str)
+{
+  gdb_assert (str != nullptr);
+  gdbpy_ref<> result (PyBytes_FromString (str));
+  if (result == nullptr)
+    throw gdb_python_exception ();
+  return result;
+}
+
+static inline gdbpy_ref<>
+gdbpy_bytes_from_string_and_size (const char *str, Py_ssize_t len)
+{
+  /* 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));
+  if (result == nullptr)
+    throw gdb_python_exception ();
+  return result;
+}
+
+static inline Py_ssize_t
+gdbpy_bytes_size (gdbpy_borrowed_ref ref)
+{
+  Py_ssize_t result = PyBytes_Size (ref);
+  if (result == -1)
+    throw gdb_python_exception ();
+  return result;
+}
+
+static inline gdbpy_ref<>
+gdbpy_new_list (Py_ssize_t len)
+{
+  gdbpy_ref<> result (PyList_New (len));
+  if (result == nullptr)
+    throw gdb_python_exception ();
+  return result;
+}
+
+static inline void
+gdbpy_list_append (gdbpy_borrowed_ref list, gdbpy_borrowed_ref val)
+{
+  if (PyList_Append (list, val) < 0)
+    throw gdb_python_exception ();
+}
+
+static inline gdbpy_ref<>
+gdbpy_new_dict ()
+{
+  gdbpy_ref<> result (PyDict_New ());
+  if (result == nullptr)
+    throw gdb_python_exception ();
+  return result;
+}
+
+static inline void
+gdbpy_dict_set_item_string (gdbpy_borrowed_ref dict,
+			    const char *key,
+			    gdbpy_borrowed_ref value)
+{
+  if (PyDict_SetItemString (dict, key, value) != 0)
+    throw gdb_python_exception ();
+}
+
+static inline void
+gdbpy_dict_del_item_string (gdbpy_borrowed_ref dict,
+			    const char *key)
+{
+  if (PyDict_DelItemString (dict, key) == -1)
+    throw gdb_python_exception ();
+}
+
+static inline gdbpy_borrowed_ref
+gdbpy_dict_get_item_with_error (gdbpy_borrowed_ref dict,
+				gdbpy_borrowed_ref key)
+{
+  PyObject *result = PyDict_GetItemWithError (dict, key);
+  if (result == nullptr)
+    throw gdb_python_exception ();
+  return result;
+}
+
+static inline gdbpy_ref<>
+gdbpy_dict_keys (gdbpy_borrowed_ref dict)
+{
+  gdbpy_ref<> result (PyDict_Keys (dict));
+  if (result == nullptr)
+    throw gdb_python_exception ();
+  return result;
+}
+
+static inline gdbpy_ref<>
+gdbpy_unicode_from_string (std::string_view str)
+{
+  gdbpy_ref<> result (PyUnicode_FromStringAndSize (str.data (), str.size ()));
+  if (result == nullptr)
+    throw gdb_python_exception ();
+  return result;
+}
+
+static inline gdbpy_ref<>
+gdbpy_unicode_from_format (const char *fmt, ...)
+{
+  va_list args;
+  va_start (args, fmt);
+  gdbpy_ref<> result (PyUnicode_FromFormatV (fmt, args));
+  if (result == nullptr)
+    throw gdb_python_exception ();
+  va_end (args);
+  return result;
+}
+
+[[noreturn]] static inline void
+gdbpy_err_set_string (gdbpy_borrowed_ref type, const char *str)
+{
+  PyErr_SetString (type, str);
+  throw gdb_python_exception ();
+}
+
+/* This is a template because PyErr_FormatV was only added in Python
+   3.5.  */
+template<typename... Arg>
+[[noreturn]] void
+gdbpy_err_format (gdbpy_borrowed_ref type, const char *fmt, Arg... args)
+{
+  PyErr_Format (type, fmt, std::forward<Arg> (args)...);
+  throw gdb_python_exception ();
+}
+
+template<typename... Arg>
+void
+gdbpy_arg_parse_tuple_and_keywords (gdbpy_borrowed_ref args,
+				    gdbpy_opt_borrowed_ref kw,
+				    const char *fmt,
+				    const char **keywords,
+				    Arg... outputs)
+{
+  /* It would be cool if callers could use references to the
+     out-parameters and also if gdbpy_borrowed_ref could be used for
+     those.  That requires some hairy template metaprogramming
+     though.  */
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, fmt, keywords,
+					std::forward<Arg> (outputs)...))
+    throw gdb_python_exception ();
+}
+
+static inline void
+gdbpy_arg_parse_tuple (gdbpy_borrowed_ref param, const char *format, ...)
+{
+  va_list args;
+  va_start (args, format);
+  if (!PyArg_VaParse (param, format, args))
+    throw gdb_python_exception ();
+  va_end (args);
+}
+
+static inline long
+gdbpy_long_as_long (gdbpy_borrowed_ref arg)
+{
+  long result = PyLong_AsLong (arg);
+  if (result == -1 && PyErr_Occurred ())
+    throw gdb_python_exception ();
+  return result;
+}
+
+static inline gdbpy_ref<>
+gdbpy_tuple_new (Py_ssize_t len)
+{
+  gdbpy_ref<> result (PyTuple_New (len));
+  if (result == nullptr)
+    throw gdb_python_exception ();
+  return result;
+}
+
+static inline gdbpy_borrowed_ref
+gdbpy_tuple_get_item (gdbpy_borrowed_ref tuple, Py_ssize_t pos)
+{
+  PyObject *result = PyTuple_GetItem (tuple, pos);
+  if (result == nullptr)
+    throw gdb_python_exception ();
+  return result;
+}
+
+static inline void
+gdbpy_tuple_set_item (gdbpy_borrowed_ref tuple, Py_ssize_t pos,
+		      gdbpy_ref<> &&item)
+{
+  if (PyTuple_SetItem (tuple, pos, item.release ()) == -1)
+    throw gdb_python_exception ();
+}
+
+static inline Py_ssize_t
+gdbpy_tuple_size (gdbpy_borrowed_ref tuple)
+{
+  Py_ssize_t result = PyTuple_Size (tuple);
+  if (result == -1)
+    throw gdb_python_exception ();
+  return result;
+}
+
+static inline Py_ssize_t
+gdbpy_sequence_size (gdbpy_borrowed_ref seq)
+{
+  Py_ssize_t result = PySequence_Size (seq);
+  if (result == -1)
+    throw gdb_python_exception ();
+  return result;
+}
+
+static inline gdbpy_ref<>
+gdbpy_sequence_get_item (gdbpy_borrowed_ref seq, Py_ssize_t i)
+{
+  gdbpy_ref<> result (PySequence_GetItem (seq, i));
+  if (result == nullptr)
+    throw gdb_python_exception ();
+  return result;
+}
+
+static inline void
+gdbpy_sequence_del_item (gdbpy_borrowed_ref seq, Py_ssize_t i)
+{
+  if (PySequence_DelItem (seq, i) == -1)
+    throw gdb_python_exception ();
+}
+
+static inline gdbpy_ref<>
+gdbpy_sequence_list (gdbpy_borrowed_ref seq)
+{
+  gdbpy_ref<> result (PySequence_List (seq));
+  if (result == nullptr)
+    throw gdb_python_exception ();
+  return result;
+}
+
+static inline gdbpy_ref<>
+gdbpy_sequence_concat (gdbpy_borrowed_ref first, gdbpy_borrowed_ref second)
+{
+  gdbpy_ref<> result (PySequence_Concat (first, second));
+  if (result == nullptr)
+    throw gdb_python_exception ();
+  return result;
+}
+
+#endif /* GDB_PYTHON_PY_WRAPPERS_H */
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 693829e920d..6df0c62e2b3 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -1382,4 +1382,6 @@ py_notimplemented ()
 #undef Py_RETURN_FALSE
 #undef Py_RETURN_NOTIMPLEMENTED
 
+#include "py-wrappers.h"
+
 #endif /* GDB_PYTHON_PYTHON_INTERNAL_H */

-- 
2.49.0

[PATCH v2 3/4] Add wrappers for Python implementation functions and methods

[Tom Tromey]

This adds some wrappers for Python implementation functions and
methods, and a couple of new constexpr functions to create PyMethodDef
entries.  This provides a few safety benefits:

* The new-style API approach (see previous patch) is implemented by
  the wrapper.  That is, exceptions are caught here and transformed.

* The implementation functions can now return any reasonable type,
  with automatic conversion by the wrapper.

* The function API and the appropriate METH_* flags are handled
  together, avoiding any possible discrepancy.

This approach also means that we can modify the old rule that gdb
calls must be wrapped in a try/catch -- the try/catch is now provided
by the wrapper function, so the implementation can be written in a
more natural way.

Note this patch is not 100% complete.  There should be one more
wrapper for case where a method takes a single argument (though we
probably cannot use METH_O unfortunately).  There may be some other
holes as well.
---
 gdb/python/py-safety.h       | 320 +++++++++++++++++++++++++++++++++++++++++++
 gdb/python/python-internal.h |   1 +
 2 files changed, 321 insertions(+)

diff --git a/gdb/python/py-safety.h b/gdb/python/py-safety.h
new file mode 100644
index 00000000000..62018dacc0f
--- /dev/null
+++ b/gdb/python/py-safety.h
@@ -0,0 +1,320 @@
+/* Wrappers for some Python safety.
+
+   Copyright (C) 2026 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef GDB_PYTHON_PY_SAFETY_H
+#define GDB_PYTHON_PY_SAFETY_H
+
+#include "gdbsupport/traits.h"
+#include "py-ref.h"
+#include "py-wrappers.h"
+#include "charset.h"
+
+/* This file holds wrapper templates for the various ways that gdb
+   code might be exposed to Python.  These wrappers are part of gdb's
+   "Python safety" approach -- utilities designed to try to prevent
+   refcount problems, missing error checks, and that also remove the
+   need to wrap calls into gdb in an explicit try/catch.
+
+   See py-wrappers.h for some more discussion of this.
+
+   Implementation functions and methods -- the stuff you write to
+   expose some part of gdb to Python -- are written in a certain
+   style.
+
+   An implementation function is a module-level function.  For example
+   something like 'gdb.register_window_type' is implemented as a
+   function.  An implementation method is a method of the gdb class
+   implementing a certain Python type; for example
+   'gdb.TuiWindow.width' is implemented via a method of
+   gdbpy_tui_window.
+
+   Each of these will accept gdbpy_borrowed_ref arguments (or in some
+   more limited situations, a gdbpy_opt_borrowed_ref) and return any
+   relevant type, which will be automatically converted (see the
+   'to_python' overloads below) to the correct Python type.  Note that
+   if the 'to_python' functions aren't appropriate in your case, you
+   can always use the escape hatch of returning a gdbpy_ref<> and
+   handling type conversion manually.
+
+   There are some constexpr functions below that wrap the
+   implementation methods, and create PyMethodDef objects and the
+   like.
+
+   Implementation methods are expected to use the wrappers in
+   py-wrappers.h and not generally call into Python directly.
+
+   Implementation details of the method-wrapping safety code are put
+   into this namespace, just to emphasize that these shouldn't be used
+   elsewhere.  Skip past the namespace to find the public APIs.  */
+namespace safety_details
+{
+/* Overloads of "to_python" are used by the safety wrappers to convert
+   a function's real return value to a Python object.  A new reference
+   will always be returned.  For the time being these are kept as a
+   detail of the method-wrapping code.  However we may want to
+   consider exposing these more generally.  */
+
+static inline PyObject *
+to_python (bool value)
+{
+  /* Note that this cannot fail.  */
+  return PyBool_FromLong (value);
+}
+
+template<typename T, typename = std::is_integral<T>>
+static inline PyObject *
+to_python (T value)
+{
+  if constexpr (std::is_signed<T>::value)
+    return gdb_py_object_from_longest (value).release ();
+  else
+    return gdb_py_object_from_ulongest (value).release ();
+}
+
+static inline PyObject *
+to_python (const char *value)
+{
+  if (value == nullptr)
+    return py_none ().release ();
+  return PyUnicode_Decode (value, strlen (value), host_charset (), nullptr);
+}
+
+static inline PyObject *
+to_python (std::string &&value)
+{
+  return PyUnicode_Decode (value.c_str (), value.size (),
+			   host_charset (), nullptr);
+}
+
+static inline PyObject *
+to_python (const std::string &value)
+{
+  return PyUnicode_Decode (value.c_str (), value.size (),
+			   host_charset (), nullptr);
+}
+
+static inline PyObject *
+to_python (gdb::unique_xmalloc_ptr<char> &&value)
+{
+  if (value == nullptr)
+    return py_none ().release ();
+  return PyUnicode_Decode (value.get (), strlen (value.get ()),
+			   host_charset (), nullptr);
+}
+
+static inline PyObject *
+to_python (gdbpy_ref<> &&value)
+{
+  return value.release ();
+}
+
+/* An instantiation of this function is used when calling a gdb method
+   from Python.  It accepts some number of arguments (normally
+   gdbpy_borrowed_ref or gdbpy_opt_borrowed_ref), and then then calls
+   the underlying function F.  Any exceptions are caught and
+   converted, and the return value of F is converted to a Python
+   object as appropriate.  */
+template<auto F, typename... Args>
+PyObject *
+wrapped_function (Args... args)
+{
+  try
+    {
+      using result_type = typename std::invoke_result_t<decltype (F), Args...>;
+
+      if constexpr (std::is_void_v<result_type>)
+	{
+	  F (args...);
+	  return py_none ().release ();
+	}
+      else
+	return to_python (F (args...));
+    }
+  catch (const gdb_python_exception &pye)
+    {
+      gdb_assert (PyErr_Occurred () != nullptr);
+      return nullptr;
+    }
+  catch (const gdb_exception &exc)
+    {
+      return gdbpy_handle_gdb_exception (nullptr, exc);
+    }
+}
+
+/* An instantiation of this function is used when calling a gdb method
+   from Python.  It accepts some number of arguments (normally
+   gdbpy_borrowed_ref or gdbpy_opt_borrowed_ref), and then then calls
+   the underlying function F.  Any exceptions are caught and
+   converted, and the return value of F is converted to a Python
+   object as appropriate.  */
+template<typename Class, typename Ret, typename... Args>
+PyObject *
+wrapped_method (Ret (Class::*meth) (Args...), Class *self, Args... args)
+{
+  try
+    {
+      if constexpr (std::is_void_v<Ret>)
+	{
+	  (self->*meth) (args...);
+	  return py_none ().release ();
+	}
+      else
+	return to_python ((self->*meth) (args...));
+    }
+  catch (const gdb_python_exception &pye)
+    {
+      gdb_assert (PyErr_Occurred () != nullptr);
+      return nullptr;
+    }
+  catch (const gdb_exception &exc)
+    {
+      return gdbpy_handle_gdb_exception (nullptr, exc);
+    }
+}
+
+template<auto F>
+PyObject *
+fn_wrapper (PyObject *self, PyObject *args, PyObject *kw)
+{
+  return wrapped_function<F> (gdbpy_borrowed_ref (args),
+			      gdbpy_opt_borrowed_ref (kw));
+}
+
+template<typename C, auto M>
+PyObject *
+varargs_wrapper (PyObject *self, PyObject *args, PyObject *kw)
+{
+  return wrapped_method (M, static_cast<C *> (self),
+			 gdbpy_borrowed_ref (args),
+			 gdbpy_opt_borrowed_ref (kw));
+}
+
+} /* namespace safety_details */
+
+/* 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
+   method M is wrapped to call to_python and to catch exceptions per
+   the safety protocol.  */
+template<typename C, auto M>
+constexpr PyMethodDef
+noargs_method (std::string_view name, std::string_view doc)
+{
+  using namespace safety_details;
+  return {
+    name.data (),
+    [] (PyObject *self, PyObject *args) -> PyObject *
+    {
+      return wrapped_method (M, static_cast<C *> (self));
+    },
+    METH_NOARGS,
+    doc.data (),
+  };
+}
+
+/* This is used to create the PyMethodDef for a varargs function.  It
+   takes the underlying implementation function as a template
+   argument, and also arguments for the method name and documentation
+   string.
+
+   The underlying function should accept a gdbpy_borrowed_ref argument
+   (the arguments to the Python function), and then a
+   gdbpy_opt_borrowed_ref for the keywords.  The function can return
+   any type (see the to_python overloads); and should throw an
+   exception on error.  If gdb_python_exception is thrown, the Python
+   exception must already have been set.
+
+   The gdb policy is that varargs methods must also accept keywords,
+   and this is enforced here.  */
+template<auto F>
+constexpr PyMethodDef
+varargs_function (std::string_view name, std::string_view doc)
+{
+  using namespace safety_details;
+  return {
+    name.data (),
+    (PyCFunction) fn_wrapper<F>,
+    /* gdb's rule is that varargs should also use keywords.  */
+    METH_VARARGS | METH_KEYWORDS,
+    doc.data (),
+  };
+}
+
+/* Like a varargs function, but this implements a method on some
+   Python type that gdb provides.  The implementation class and a
+   pointer-to-method must be specified.  */
+template<typename C, auto M>
+constexpr PyMethodDef
+varargs_method (std::string_view name, std::string_view doc)
+{
+  using namespace safety_details;
+  return {
+    name.data (),
+    (PyCFunction) varargs_wrapper<C, M>,
+    /* gdb's rule is that varargs should also use keywords.  */
+    METH_VARARGS | METH_KEYWORDS,
+    doc.data (),
+  };
+}
+
+/* A function that wraps a "repr" or "str" method.  */
+template<typename C, auto M>
+PyObject *
+wrap_repr (PyObject *arg)
+{
+  using namespace safety_details;
+  return wrapped_method (M, static_cast<C *> (arg));
+}
+
+/* A function that wraps a "get" method.  */
+template<typename C, auto M>
+PyObject *
+wrap_getter (PyObject *arg, void *closure)
+{
+  using namespace safety_details;
+  /* In gdb the closure argument is never used.  */
+  return wrapped_method (M, static_cast<C *> (arg));
+}
+
+/* A function that wraps a "set" method.  */
+template<typename C, void (C::*M) (gdbpy_opt_borrowed_ref)>
+int
+wrap_setter (PyObject *arg, PyObject *value, void *closure)
+{
+  using namespace safety_details;
+  try
+    {
+      C *self = static_cast<C *> (arg);
+      /* In gdb the closure argument is never used.  */
+      (self->*M) (gdbpy_opt_borrowed_ref (value));
+    }
+  catch (const gdb_python_exception &pye)
+    {
+      gdb_assert (PyErr_Occurred () != nullptr);
+      return -1;
+    }
+  catch (const gdb_exception &exc)
+    {
+      return gdbpy_handle_gdb_exception (-1, exc);
+    }
+
+  return 0;
+}
+
+#endif /* GDB_PYTHON_PY_SAFETY_H */
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 6df0c62e2b3..e4f35f8cd88 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -1383,5 +1383,6 @@ py_notimplemented ()
 #undef Py_RETURN_NOTIMPLEMENTED
 
 #include "py-wrappers.h"
+#include "py-safety.h"
 
 #endif /* GDB_PYTHON_PYTHON_INTERNAL_H */

-- 
2.49.0

[PATCH v2 4/4] Convert py-tui.c to the "python safety" approach

[Tom Tromey]

This patch mostly converts py-tui.c to use the new Python safety code.

In particular:

* All methods of gdb.TuiWindow are now implemented as straightforward
  methods of gdbpy_tui_window.

* gdb.register_tui_window is converted and simply returns 'void'.

I converted this particular file because it was relatively
straightforward, while also demonstrating most of the features of the
new approach.  For example, explicit result checks aren't needed,
try/catch can be removed, and the methods are now written in a natural
style.

Note that more conversion remains to be done here:

* gdbpy_tui_enabled hasn't been converted and still does explicit
  checks.

* There's one explicit check in gdbpy_tui_window::set_title.  Fully
  implementing the safety approach means that some low-level things
  should eventually be converted to throw; but some work has to be
  deferred until a lot of the work is complete.
---
 gdb/python/py-tui.c          | 208 ++++++++++++++++++-------------------------
 gdb/python/python-internal.h |   5 +-
 gdb/python/python.c          |   5 +-
 3 files changed, 92 insertions(+), 126 deletions(-)

diff --git a/gdb/python/py-tui.c b/gdb/python/py-tui.c
index 111e0f4c9e3..0755d7b92e3 100644
--- a/gdb/python/py-tui.c
+++ b/gdb/python/py-tui.c
@@ -50,7 +50,34 @@ struct gdbpy_tui_window: public PyObject
   tui_py_window *window;
 
   /* Return true if this object is valid.  */
-  bool is_valid () const;
+  bool is_valid ();
+
+  /* Require that this object be valid.  Throws exception if not.  */
+  void require_valid ()
+  {
+    if (!is_valid ())
+      gdbpy_err_format (PyExc_RuntimeError, _("TUI window is invalid."));
+  }
+
+  /* Erase the TUI window.  */
+  void erase ();
+
+  /* Python function that writes some text to a TUI window.  */
+  void write (gdbpy_borrowed_ref args, gdbpy_opt_borrowed_ref kw);
+
+  /* Return the width of the TUI window.  */
+  int width ();
+
+  /* Return the height of the TUI window.  */
+  int height ();
+
+  /* Return the title of the TUI window.  */
+  const std::string &title ();
+
+  /* Set the title of the TUI window.  */
+  void set_title (gdbpy_opt_borrowed_ref new_title);
+
+  static PyTypeObject *corresponding_object_type;
 };
 
 extern PyTypeObject gdbpy_tui_window_object_type;
@@ -150,7 +177,7 @@ class tui_py_window : public tui_win_info
 /* See gdbpy_tui_window declaration above.  */
 
 bool
-gdbpy_tui_window::is_valid () const
+gdbpy_tui_window::is_valid ()
 {
   return window != nullptr && tui_active;
 }
@@ -406,173 +433,109 @@ gdbpy_tui_window_maker::operator() (const char *win_name)
 
 /* Implement "gdb.register_window_type".  */
 
-PyObject *
-gdbpy_register_tui_window (PyObject *self, PyObject *args, PyObject *kw)
+void
+gdbpy_register_tui_window (gdbpy_borrowed_ref args,
+			   gdbpy_opt_borrowed_ref kw)
 {
   static const char *keywords[] = { "name", "constructor", nullptr };
-
   const char *name;
   PyObject *cons_obj;
+  gdbpy_arg_parse_tuple_and_keywords (args, kw, "sO", keywords,
+				      &name, &cons_obj);
 
-  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "sO", keywords,
-					&name, &cons_obj))
-    return nullptr;
-
-  try
-    {
-      gdbpy_tui_window_maker constr (gdbpy_ref<>::new_reference (cons_obj));
-      tui_register_window (name, constr);
-    }
-  catch (const gdb_exception &except)
-    {
-      return gdbpy_handle_gdb_exception (nullptr, except);
-    }
-
-  return py_none ().release ();
+  gdbpy_tui_window_maker constr (gdbpy_ref<>::new_reference (cons_obj));
+  tui_register_window (name, constr);
 }
 
 \f
 
-/* Require that "Window" be a valid window.  */
-
-#define REQUIRE_WINDOW(Window)					\
-    do {							\
-      if (!(Window)->is_valid ())				\
-	return PyErr_Format (PyExc_RuntimeError,		\
-			     _("TUI window is invalid."));	\
-    } while (0)
-
-/* Require that "Window" be a valid window.  */
-
-#define REQUIRE_WINDOW_FOR_SETTER(Window)			\
-    do {							\
-      if (!(Window)->is_valid ())				\
-	{							\
-	  PyErr_Format (PyExc_RuntimeError,			\
-			_("TUI window is invalid."));		\
-	  return -1;						\
-	}							\
-    } while (0)
-
-/* Python function which checks the validity of a TUI window
-   object.  */
-static PyObject *
-gdbpy_tui_is_valid (PyObject *self, PyObject *args)
-{
-  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
-
-  if (win->is_valid ())
-    return py_true ().release ();
-  return py_false ().release ();
-}
-
 /* Python function that erases the TUI window.  */
-static PyObject *
-gdbpy_tui_erase (PyObject *self, PyObject *args)
+void
+gdbpy_tui_window::erase ()
 {
-  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
-
-  REQUIRE_WINDOW (win);
-
-  win->window->erase ();
-
-  return py_none ().release ();
+  require_valid ();
+  window->erase ();
 }
 
 /* Python function that writes some text to a TUI window.  */
-static PyObject *
-gdbpy_tui_write (PyObject *self, PyObject *args, PyObject *kw)
+void
+gdbpy_tui_window::write (gdbpy_borrowed_ref args, gdbpy_opt_borrowed_ref kw)
 {
+  require_valid ();
+
   static const char *keywords[] = { "string", "full_window", nullptr };
 
-  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
   const char *text;
   int full_window = 0;
 
-  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "s|i", keywords,
-					&text, &full_window))
-    return nullptr;
-
-  REQUIRE_WINDOW (win);
+  gdbpy_arg_parse_tuple_and_keywords (args, kw, "s|i", keywords,
+				      &text, &full_window);
 
-  win->window->output (text, full_window);
-
-  return py_none ().release ();
+  window->output (text, full_window);
 }
 
-/* Return the width of the TUI window.  */
-static PyObject *
-gdbpy_tui_width (PyObject *self, void *closure)
+int
+gdbpy_tui_window::width ()
 {
-  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
-  REQUIRE_WINDOW (win);
-  gdbpy_ref<> result
-    = gdb_py_object_from_longest (win->window->viewport_width ());
-  return result.release ();
+  require_valid ();
+  return window->viewport_width ();
 }
 
-/* Return the height of the TUI window.  */
-static PyObject *
-gdbpy_tui_height (PyObject *self, void *closure)
+int
+gdbpy_tui_window::height ()
 {
-  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
-  REQUIRE_WINDOW (win);
-  gdbpy_ref<> result
-    = gdb_py_object_from_longest (win->window->viewport_height ());
-  return result.release ();
+  require_valid ();
+  return window->viewport_height ();
 }
 
-/* Return the title of the TUI window.  */
-static PyObject *
-gdbpy_tui_title (PyObject *self, void *closure)
+const std::string &
+gdbpy_tui_window::title ()
 {
-  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
-  REQUIRE_WINDOW (win);
-  return host_string_to_python_string (win->window->title ().c_str ()).release ();
+  require_valid ();
+  return window->title ();
 }
 
 /* Set the title of the TUI window.  */
-static int
-gdbpy_tui_set_title (PyObject *self, PyObject *newvalue, void *closure)
+void
+gdbpy_tui_window::set_title (gdbpy_opt_borrowed_ref new_title)
 {
-  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
+  require_valid ();
 
-  REQUIRE_WINDOW_FOR_SETTER (win);
-
-  if (newvalue == nullptr)
-    {
-      PyErr_Format (PyExc_TypeError, _("Cannot delete \"title\" attribute."));
-      return -1;
-    }
+  if (new_title == nullptr)
+    gdbpy_err_format (PyExc_TypeError,
+		      _("Cannot delete \"title\" attribute."));
 
+  /* FIXME: safety */
   gdb::unique_xmalloc_ptr<char> value
-    = python_string_to_host_string (newvalue);
+    = python_string_to_host_string (new_title);
   if (value == nullptr)
-    return -1;
+    throw gdb_python_exception ();
 
-  win->window->set_title (value.get ());
-  return 0;
+  window->set_title (value.get ());
 }
 
 static gdb_PyGetSetDef tui_object_getset[] =
 {
-  { "width", gdbpy_tui_width, NULL, "Width of the window.", NULL },
-  { "height", gdbpy_tui_height, NULL, "Height of the window.", NULL },
-  { "title", gdbpy_tui_title, gdbpy_tui_set_title, "Title of the window.",
-    NULL },
-  { NULL }  /* Sentinel */
+  { "width", wrap_getter<gdbpy_tui_window, &gdbpy_tui_window::width>, nullptr,
+    "Width of the window.", nullptr },
+  { "height", wrap_getter<gdbpy_tui_window, &gdbpy_tui_window::height>, nullptr,
+    "Height of the window.", nullptr },
+  { "title", wrap_getter<gdbpy_tui_window, &gdbpy_tui_window::title>,
+    wrap_setter<gdbpy_tui_window, &gdbpy_tui_window::set_title>,
+    "Title of the window.", nullptr },
+  { nullptr }  /* Sentinel */
 };
 
 static PyMethodDef tui_object_methods[] =
 {
-  { "is_valid", gdbpy_tui_is_valid, METH_NOARGS,
+  noargs_method<gdbpy_tui_window, &gdbpy_tui_window::is_valid> ("is_valid",
     "is_valid () -> Boolean\n\
-Return true if this TUI window is valid, false if not." },
-  { "erase", gdbpy_tui_erase, METH_NOARGS,
-    "Erase the TUI window." },
-  { "write", (PyCFunction) gdbpy_tui_write, METH_VARARGS | METH_KEYWORDS,
-    "Append a string to the TUI window." },
-  { NULL } /* Sentinel.  */
+Return true if this TUI window is valid, false if not."),
+  noargs_method<gdbpy_tui_window, &gdbpy_tui_window::erase>
+    ("erase", "Erase the TUI window."),
+  varargs_method<gdbpy_tui_window, &gdbpy_tui_window::write> ("write",
+    "Append a string to the TUI window."),
+  { nullptr } /* Sentinel.  */
 };
 
 PyTypeObject gdbpy_tui_window_object_type =
@@ -616,6 +579,9 @@ PyTypeObject gdbpy_tui_window_object_type =
   0,				  /* tp_alloc */
 };
 
+PyTypeObject *gdbpy_tui_window::corresponding_object_type
+    = &gdbpy_tui_window_object_type;
+
 /* Called when TUI is enabled or disabled.  */
 
 static void
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index e4f35f8cd88..9e05cdd67e7 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -496,8 +496,9 @@ gdb::unique_xmalloc_ptr<char> gdbpy_parse_command_name
   (const char *name, struct cmd_list_element ***base_list,
    struct cmd_list_element **start_list,
    struct cmd_list_element **prefix_cmd = nullptr);
-PyObject *gdbpy_register_tui_window (PyObject *self, PyObject *args,
-				     PyObject *kw);
+
+void gdbpy_register_tui_window (gdbpy_borrowed_ref args,
+				gdbpy_opt_borrowed_ref kw);
 
 gdbpy_ref<> symtab_and_line_to_sal_object (struct symtab_and_line sal);
 gdbpy_ref<> symtab_to_symtab_object (struct symtab *symtab);
diff --git a/gdb/python/python.c b/gdb/python/python.c
index fd254a340d8..849154ff20c 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -3274,10 +3274,9 @@ or None if not set." },
 Set the value of the convenience variable $NAME." },
 
 #ifdef TUI
-  { "register_window_type", (PyCFunction) gdbpy_register_tui_window,
-    METH_VARARGS | METH_KEYWORDS,
+  varargs_function<gdbpy_register_tui_window> ("register_window_type",
     "register_window_type (NAME, CONSTRUCTOR) -> None\n\
-Register a TUI window constructor." },
+Register a TUI window constructor."),
 #endif	/* TUI */
 
   { "architecture_names", gdbpy_all_architecture_names, METH_NOARGS,

-- 
2.49.0

Re: [PATCH v2 1/4] Add gdbpy_borrowed_ref

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

> This adds new gdbpy_opt_borrowed_ref and gdbpy_borrowed_ref classes.
> These class is primarily for code "documentation" purposes -- it makes

type: These CLASSES ARE primarily ....

> it clear to the reader that a given reference is borrowed.  However,
> they also add a tiny bit of safety, in that conversion to gdbpy_ref<>
> will either be rejected (by the "opt" class) or acquire a new
> reference.
> ---
>  gdb/python/py-ref.h | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
>
> diff --git a/gdb/python/py-ref.h b/gdb/python/py-ref.h
> index dc0b14814af..ef6b9fb427c 100644
> --- a/gdb/python/py-ref.h
> +++ b/gdb/python/py-ref.h
> @@ -41,6 +41,86 @@ struct gdbpy_ref_policy
>  template<typename T = PyObject> using gdbpy_ref
>    = gdb::ref_ptr<T, gdbpy_ref_policy>;
>  
> +/* A class representing an optional borrowed reference.  It is
> +   "optional" because NULL is a valid value.
> +
> +   This is a simple wrapper for a PyObject*.  Aside from documenting
> +   what the code does, the main advantage of using this is that
> +   conversion to a gdbpy_ref<> is prevented.
> +
> +   An optional borrowed reference is only used in situations where
> +   Python says NULL is valid.  For example, it is used as the type of
> +   the "keywords" argument to a varargs method.  Most code should
> +   prefer an ordinary gdbpy_borrowed_ref, see below.  */
> +class gdbpy_opt_borrowed_ref
> +{
> +public:
> +
> +  gdbpy_opt_borrowed_ref (PyObject *obj)
> +    : m_obj (obj)
> +  {
> +  }
> +
> +  template<typename T>
> +  gdbpy_opt_borrowed_ref (const gdbpy_ref<T> &ref)
> +    : m_obj (ref.get ())
> +  {
> +  }
> +
> +  operator PyObject * ()
> +  {
> +    return m_obj;
> +  }

Could/should this be marked as 'const'?

> +
> +  operator gdbpy_ref<> () = delete;
> +
> +protected:
> +  PyObject *m_obj;
> +};
> +
> +/* A borrowed reference that is guaranteed not to be NULL.
> +
> +   Like gdbpy_opt_borrowed_ref, this mostly serves a documentary
> +   purpose.  However, it also allows a checked cast to any subclass of
> +   PyObject, and conversion to a gdbpy_ref<> will automatically
> +   acquire a new reference -- a safety improvement over plain
> +   PyObject*.  */
> +class gdbpy_borrowed_ref : public gdbpy_opt_borrowed_ref
> +{
> +public:
> +
> +  gdbpy_borrowed_ref (PyObject *obj)
> +    : gdbpy_opt_borrowed_ref (obj)
> +  {
> +    gdb_assert (m_obj != nullptr);
> +  }
> +
> +  template<typename T>
> +  gdbpy_borrowed_ref (const gdbpy_ref<T> &ref)
> +    : gdbpy_opt_borrowed_ref (ref)
> +  {
> +    gdb_assert (m_obj != nullptr);
> +  }
> +
> +  gdbpy_borrowed_ref (std::nullptr_t) = delete;
> +
> +  /* Allow a (checked) conversion to any subclass of PyObject.  */
> +  template<typename T,
> +	   typename = std::is_convertible<T *, PyObject *>>
> +  operator T * ()
> +  {
> +    gdb_assert (PyObject_TypeCheck (m_obj, T::corresponding_object_type));
> +    return static_cast<T *> (m_obj);
> +  }

OK, please excuse my ignorance here, I might be completely wrong, but I
believe the line 'typename = std::is_convertible<T *, PyObject *>' is
here for the purpose of SFINAE.  I believe this was copied from
gdb_ref_ptr.h, but I think this line might be wrong, both there and
here.

I think std::is_convertible is either true or false, or the type will be
true_type or false_type, but in all cases, I think the type is well
defined, so there will never be substitution failure.

I think what you need here is:

  typename = gdb::Requires<std::is_convertible<T *, PyObject *>>

But it might be that I'm just not understanding what's going on here, in
which case, feel free to correct me.

> +
> +  /* When converting a borrowed reference to a gdbpy_ref<>, a new
> +     reference is acquired.  */
> +  operator gdbpy_ref<> ()
> +  {
> +    return gdbpy_ref<>::new_reference (m_obj);
> +  }

Again with the 'const' maybe?

Thanks,
Andrew

> +};
> +
>  /* A wrapper class for Python extension objects that have a __dict__ attribute.
>  
>     Any Python C object extension needing __dict__ should inherit from this
>
> -- 
> 2.49.0

Re: [PATCH v2 2/4] Add wrappers for some Python APIs

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

> This adds some new functions that wrap Python APIs.  The wrapping
> follows some proposed rules for Python safety in gdb:
>
> * Functions returning a new reference return gdbpy_ref<>
>
> * Errors are reported via exceptions, not special values
>
> * Functions accepting a stolen reference take a gdbpy_ref<>&&
> ---
>  gdb/python/py-wrappers.h     | 334 +++++++++++++++++++++++++++++++++++++++++++
>  gdb/python/python-internal.h |   2 +
>  2 files changed, 336 insertions(+)
>
> diff --git a/gdb/python/py-wrappers.h b/gdb/python/py-wrappers.h
> new file mode 100644
> index 00000000000..78c1f8070cf
> --- /dev/null
> +++ b/gdb/python/py-wrappers.h
> @@ -0,0 +1,334 @@
> +/* Wrappers for some Python safety.
> +
> +   Copyright (C) 2026 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef GDB_PYTHON_PY_WRAPPERS_H
> +#define GDB_PYTHON_PY_WRAPPERS_H
> +
> +#include "py-ref.h"
> +
> +/* Gdb implements its own wrappers for many Python APIs.  This is done
> +   in an attempt to be more safe.
> +
> +   In particular, in gdb:
> +
> +   - APIs returning a new reference will return gdbpy_ref<>.  This
> +   makes reference counting errors less likely.
> +
> +   - APIs will throw an exception rather than return a special value
> +   (NULL or -1).  This makes error checking simpler.
> +
> +   - APIs requiring a stolen reference take a gdbpy_ref<>&&, to make
> +   reference counting errors less likely.
> +
> +   This file holds the currently-defined wrappers.  If new APIs are
> +   needed, the normal approach is to add a wrapper here.
> +
> +   APIs here are named after the underlying Python function, but using
> +   lower case and an "_" at each word break.  */
> +
> +/* The type of exception thrown when the Python exception has been
> +   set.  */
> +struct gdb_python_exception
> +{
> +  gdb_python_exception ()
> +  {
> +    gdb_assert (PyErr_Occurred ());
> +  }
> +};
> +
> +template<typename T>
> +gdbpy_ref<T>
> +gdbpy_new ()
> +{
> +  gdbpy_ref<T> result (PyObject_New (T, T::corresponding_object_type));
> +  if (result == nullptr)
> +    throw gdb_python_exception ();
> +  return result;
> +}
> +
> +static inline gdbpy_ref<>
> +gdbpy_bool_from_long (long value)
> +{
> +  /* This cannot fail.  */
> +  return gdbpy_ref<> (PyBool_FromLong (value));
> +}
> +
> +static inline char *
> +gdbpy_bytes_as_string (gdbpy_borrowed_ref ref)
> +{
> +  char *result = PyBytes_AsString (ref);
> +  if (result == nullptr)
> +    throw gdb_python_exception ();
> +  return result;
> +}
> +
> +static inline void
> +gdbpy_bytes_as_string_and_size (gdbpy_borrowed_ref ref,
> +				char **buffer,
> +				Py_ssize_t *length)
> +{
> +  if (PyBytes_AsStringAndSize (ref, buffer, length) == -1)
> +    throw gdb_python_exception ();
> +}
> +
> +static inline gdbpy_ref<>
> +gdbpy_bytes_from_string (const char *str)
> +{
> +  gdb_assert (str != nullptr);
> +  gdbpy_ref<> result (PyBytes_FromString (str));
> +  if (result == nullptr)
> +    throw gdb_python_exception ();
> +  return result;
> +}
> +
> +static inline gdbpy_ref<>
> +gdbpy_bytes_from_string_and_size (const char *str, Py_ssize_t len)
> +{
> +  /* 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));
> +  if (result == nullptr)
> +    throw gdb_python_exception ();
> +  return result;
> +}
> +
> +static inline Py_ssize_t
> +gdbpy_bytes_size (gdbpy_borrowed_ref ref)
> +{
> +  Py_ssize_t result = PyBytes_Size (ref);
> +  if (result == -1)
> +    throw gdb_python_exception ();
> +  return result;
> +}
> +
> +static inline gdbpy_ref<>
> +gdbpy_new_list (Py_ssize_t len)
> +{
> +  gdbpy_ref<> result (PyList_New (len));
> +  if (result == nullptr)
> +    throw gdb_python_exception ();
> +  return result;
> +}
> +
> +static inline void
> +gdbpy_list_append (gdbpy_borrowed_ref list, gdbpy_borrowed_ref val)
> +{
> +  if (PyList_Append (list, val) < 0)
> +    throw gdb_python_exception ();
> +}
> +
> +static inline gdbpy_ref<>
> +gdbpy_new_dict ()
> +{
> +  gdbpy_ref<> result (PyDict_New ());
> +  if (result == nullptr)
> +    throw gdb_python_exception ();
> +  return result;
> +}
> +
> +static inline void
> +gdbpy_dict_set_item_string (gdbpy_borrowed_ref dict,
> +			    const char *key,
> +			    gdbpy_borrowed_ref value)
> +{
> +  if (PyDict_SetItemString (dict, key, value) != 0)
> +    throw gdb_python_exception ();
> +}
> +
> +static inline void
> +gdbpy_dict_del_item_string (gdbpy_borrowed_ref dict,
> +			    const char *key)
> +{
> +  if (PyDict_DelItemString (dict, key) == -1)
> +    throw gdb_python_exception ();
> +}
> +
> +static inline gdbpy_borrowed_ref
> +gdbpy_dict_get_item_with_error (gdbpy_borrowed_ref dict,
> +				gdbpy_borrowed_ref key)
> +{
> +  PyObject *result = PyDict_GetItemWithError (dict, key);
> +  if (result == nullptr)
> +    throw gdb_python_exception ();
> +  return result;

PyDict_GetItemWithError will return NULL *without* an exception set if
KEY is not found in DICT.  This will then trigger an assert in the
gdb_python_exception constructor. Because of the *cough* lack of
comments, it's unclear what the function's intended behaviour is, but
this feels like a bug.


> +}
> +
> +static inline gdbpy_ref<>
> +gdbpy_dict_keys (gdbpy_borrowed_ref dict)
> +{
> +  gdbpy_ref<> result (PyDict_Keys (dict));
> +  if (result == nullptr)
> +    throw gdb_python_exception ();
> +  return result;
> +}
> +
> +static inline gdbpy_ref<>
> +gdbpy_unicode_from_string (std::string_view str)
> +{
> +  gdbpy_ref<> result (PyUnicode_FromStringAndSize (str.data (), str.size ()));
> +  if (result == nullptr)
> +    throw gdb_python_exception ();
> +  return result;
> +}
> +
> +static inline gdbpy_ref<>
> +gdbpy_unicode_from_format (const char *fmt, ...)
> +{
> +  va_list args;
> +  va_start (args, fmt);
> +  gdbpy_ref<> result (PyUnicode_FromFormatV (fmt, args));
> +  if (result == nullptr)
> +    throw gdb_python_exception ();
> +  va_end (args);

If an exception is thrown in this case then we fail to call va_end,
which I believe is technically undefined behaviour (though in reality
it's probably harmless), still, we should fix this.

> +  return result;
> +}
> +
> +[[noreturn]] static inline void
> +gdbpy_err_set_string (gdbpy_borrowed_ref type, const char *str)
> +{
> +  PyErr_SetString (type, str);
> +  throw gdb_python_exception ();
> +}
> +
> +/* This is a template because PyErr_FormatV was only added in Python
> +   3.5.  */
> +template<typename... Arg>
> +[[noreturn]] void
> +gdbpy_err_format (gdbpy_borrowed_ref type, const char *fmt, Arg... args)
> +{
> +  PyErr_Format (type, fmt, std::forward<Arg> (args)...);

This isn't really a request for a change, more a question for my own
education.  My understanding is that std::forward is intended to be used
with forwarding reference arguments, which ARGS is not.  So in this case
ARGS will have been captured by-value, so there is no difference between
what you have now and:

  PyErr_Format (type, fmt, args...);

Am I correct here?  Or is std::forward doing something that I'm not
understanding?  Or did you mean to capture ARGS as 'Arg&&... args', in
which case std::forward would be needed / useful?

I had a look and there are actually loads of places in GDB where we use
std::forward on arguments that are captured by-value, so my assumption
here is that I'm not understanding this topic fully, but if I don't ask,
I'll never know.

> +  throw gdb_python_exception ();
> +}
> +
> +template<typename... Arg>
> +void
> +gdbpy_arg_parse_tuple_and_keywords (gdbpy_borrowed_ref args,
> +				    gdbpy_opt_borrowed_ref kw,
> +				    const char *fmt,
> +				    const char **keywords,
> +				    Arg... outputs)
> +{
> +  /* It would be cool if callers could use references to the
> +     out-parameters and also if gdbpy_borrowed_ref could be used for
> +     those.  That requires some hairy template metaprogramming
> +     though.  */
> +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, fmt, keywords,
> +					std::forward<Arg> (outputs)...))

Here's another place where std::forward is used on by-value OUTPUTS.

> +    throw gdb_python_exception ();
> +}
> +
> +static inline void
> +gdbpy_arg_parse_tuple (gdbpy_borrowed_ref param, const char *format, ...)
> +{
> +  va_list args;
> +  va_start (args, format);
> +  if (!PyArg_VaParse (param, format, args))
> +    throw gdb_python_exception ();
> +  va_end (args);

Exception causes va_end to be skipped again.

> +}
> +

Thanks,
Andrew

Re: [PATCH v2 3/4] Add wrappers for Python implementation functions and methods

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

> This adds some wrappers for Python implementation functions and
> methods, and a couple of new constexpr functions to create PyMethodDef
> entries.  This provides a few safety benefits:
>
> * The new-style API approach (see previous patch) is implemented by
>   the wrapper.  That is, exceptions are caught here and transformed.
>
> * The implementation functions can now return any reasonable type,
>   with automatic conversion by the wrapper.
>
> * The function API and the appropriate METH_* flags are handled
>   together, avoiding any possible discrepancy.
>
> This approach also means that we can modify the old rule that gdb
> calls must be wrapped in a try/catch -- the try/catch is now provided
> by the wrapper function, so the implementation can be written in a
> more natural way.
>
> Note this patch is not 100% complete.  There should be one more
> wrapper for case where a method takes a single argument (though we
> probably cannot use METH_O unfortunately).  There may be some other
> holes as well.
> ---
>  gdb/python/py-safety.h       | 320 +++++++++++++++++++++++++++++++++++++++++++
>  gdb/python/python-internal.h |   1 +
>  2 files changed, 321 insertions(+)
>
> diff --git a/gdb/python/py-safety.h b/gdb/python/py-safety.h
> new file mode 100644
> index 00000000000..62018dacc0f
> --- /dev/null
> +++ b/gdb/python/py-safety.h
> @@ -0,0 +1,320 @@
> +/* Wrappers for some Python safety.
> +
> +   Copyright (C) 2026 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef GDB_PYTHON_PY_SAFETY_H
> +#define GDB_PYTHON_PY_SAFETY_H
> +
> +#include "gdbsupport/traits.h"
> +#include "py-ref.h"
> +#include "py-wrappers.h"
> +#include "charset.h"
> +
> +/* This file holds wrapper templates for the various ways that gdb
> +   code might be exposed to Python.  These wrappers are part of gdb's
> +   "Python safety" approach -- utilities designed to try to prevent
> +   refcount problems, missing error checks, and that also remove the
> +   need to wrap calls into gdb in an explicit try/catch.
> +
> +   See py-wrappers.h for some more discussion of this.
> +
> +   Implementation functions and methods -- the stuff you write to
> +   expose some part of gdb to Python -- are written in a certain
> +   style.
> +
> +   An implementation function is a module-level function.  For example
> +   something like 'gdb.register_window_type' is implemented as a
> +   function.  An implementation method is a method of the gdb class
> +   implementing a certain Python type; for example
> +   'gdb.TuiWindow.width' is implemented via a method of
> +   gdbpy_tui_window.
> +
> +   Each of these will accept gdbpy_borrowed_ref arguments (or in some
> +   more limited situations, a gdbpy_opt_borrowed_ref) and return any
> +   relevant type, which will be automatically converted (see the
> +   'to_python' overloads below) to the correct Python type.  Note that
> +   if the 'to_python' functions aren't appropriate in your case, you
> +   can always use the escape hatch of returning a gdbpy_ref<> and
> +   handling type conversion manually.
> +
> +   There are some constexpr functions below that wrap the
> +   implementation methods, and create PyMethodDef objects and the
> +   like.
> +
> +   Implementation methods are expected to use the wrappers in
> +   py-wrappers.h and not generally call into Python directly.
> +
> +   Implementation details of the method-wrapping safety code are put
> +   into this namespace, just to emphasize that these shouldn't be used
> +   elsewhere.  Skip past the namespace to find the public APIs.  */
> +namespace safety_details
> +{
> +/* Overloads of "to_python" are used by the safety wrappers to convert
> +   a function's real return value to a Python object.  A new reference
> +   will always be returned.  For the time being these are kept as a
> +   detail of the method-wrapping code.  However we may want to
> +   consider exposing these more generally.  */
> +
> +static inline PyObject *
> +to_python (bool value)
> +{
> +  /* Note that this cannot fail.  */
> +  return PyBool_FromLong (value);
> +}
> +
> +template<typename T, typename = std::is_integral<T>>
> +static inline PyObject *
> +to_python (T value)
> +{
> +  if constexpr (std::is_signed<T>::value)
> +    return gdb_py_object_from_longest (value).release ();
> +  else
> +    return gdb_py_object_from_ulongest (value).release ();
> +}
> +
> +static inline PyObject *
> +to_python (const char *value)
> +{
> +  if (value == nullptr)
> +    return py_none ().release ();
> +  return PyUnicode_Decode (value, strlen (value), host_charset (), nullptr);
> +}
> +
> +static inline PyObject *
> +to_python (std::string &&value)
> +{
> +  return PyUnicode_Decode (value.c_str (), value.size (),
> +			   host_charset (), nullptr);
> +}
> +
> +static inline PyObject *
> +to_python (const std::string &value)
> +{
> +  return PyUnicode_Decode (value.c_str (), value.size (),
> +			   host_charset (), nullptr);
> +}
> +
> +static inline PyObject *
> +to_python (gdb::unique_xmalloc_ptr<char> &&value)
> +{
> +  if (value == nullptr)
> +    return py_none ().release ();
> +  return PyUnicode_Decode (value.get (), strlen (value.get ()),
> +			   host_charset (), nullptr);
> +}
> +
> +static inline PyObject *
> +to_python (gdbpy_ref<> &&value)
> +{
> +  return value.release ();
> +}
> +
> +/* An instantiation of this function is used when calling a gdb method
> +   from Python.  It accepts some number of arguments (normally
> +   gdbpy_borrowed_ref or gdbpy_opt_borrowed_ref), and then then calls
> +   the underlying function F.  Any exceptions are caught and
> +   converted, and the return value of F is converted to a Python
> +   object as appropriate.  */
> +template<auto F, typename... Args>
> +PyObject *
> +wrapped_function (Args... args)
> +{
> +  try
> +    {
> +      using result_type = typename std::invoke_result_t<decltype (F), Args...>;
> +
> +      if constexpr (std::is_void_v<result_type>)
> +	{
> +	  F (args...);
> +	  return py_none ().release ();
> +	}
> +      else
> +	return to_python (F (args...));
> +    }
> +  catch (const gdb_python_exception &pye)
> +    {
> +      gdb_assert (PyErr_Occurred () != nullptr);
> +      return nullptr;
> +    }
> +  catch (const gdb_exception &exc)
> +    {
> +      return gdbpy_handle_gdb_exception (nullptr, exc);
> +    }
> +}
> +
> +/* An instantiation of this function is used when calling a gdb method
> +   from Python.  It accepts some number of arguments (normally
> +   gdbpy_borrowed_ref or gdbpy_opt_borrowed_ref), and then then calls
> +   the underlying function F.  Any exceptions are caught and
> +   converted, and the return value of F is converted to a Python
> +   object as appropriate.  */
> +template<typename Class, typename Ret, typename... Args>
> +PyObject *
> +wrapped_method (Ret (Class::*meth) (Args...), Class *self, Args... args)
> +{
> +  try
> +    {
> +      if constexpr (std::is_void_v<Ret>)
> +	{
> +	  (self->*meth) (args...);
> +	  return py_none ().release ();
> +	}
> +      else
> +	return to_python ((self->*meth) (args...));
> +    }
> +  catch (const gdb_python_exception &pye)
> +    {
> +      gdb_assert (PyErr_Occurred () != nullptr);
> +      return nullptr;
> +    }
> +  catch (const gdb_exception &exc)
> +    {
> +      return gdbpy_handle_gdb_exception (nullptr, exc);
> +    }
> +}
> +
> +template<auto F>
> +PyObject *
> +fn_wrapper (PyObject *self, PyObject *args, PyObject *kw)
> +{
> +  return wrapped_function<F> (gdbpy_borrowed_ref (args),
> +			      gdbpy_opt_borrowed_ref (kw));
> +}
> +
> +template<typename C, auto M>
> +PyObject *
> +varargs_wrapper (PyObject *self, PyObject *args, PyObject *kw)
> +{
> +  return wrapped_method (M, static_cast<C *> (self),
> +			 gdbpy_borrowed_ref (args),
> +			 gdbpy_opt_borrowed_ref (kw));
> +}
> +
> +} /* namespace safety_details */
> +
> +/* 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
> +   method M is wrapped to call to_python and to catch exceptions per
> +   the safety protocol.  */
> +template<typename C, auto M>
> +constexpr PyMethodDef
> +noargs_method (std::string_view name, std::string_view doc)

Using std::string_view seems a little suspect here.  Calling
std::string_view::data doesn't guarantee a NULL terminated string, and
creating a string_view from a C-string only encodes the length of the
C-string, not 'length + 1' which would be required to capture the NULL
terminator.

Now given how this gets used in the next patch we're going to be fine,
you pass a C-string, Python will read outside the bounds of the
string_view and find the NULL terminator, but that all just seems a
little ... iffy.

If we don't actually care about the length of the string_view, then
should we be using string_view at all?  Shouldn't we just be honest with
ourselves and pass through 'const char *'?

There are other uses of string_view below, and the same thoughts apply.

> +{
> +  using namespace safety_details;
> +  return {
> +    name.data (),
> +    [] (PyObject *self, PyObject *args) -> PyObject *
> +    {
> +      return wrapped_method (M, static_cast<C *> (self));
> +    },
> +    METH_NOARGS,
> +    doc.data (),
> +  };
> +}
> +
> +/* This is used to create the PyMethodDef for a varargs function.  It
> +   takes the underlying implementation function as a template
> +   argument, and also arguments for the method name and documentation
> +   string.
> +
> +   The underlying function should accept a gdbpy_borrowed_ref argument
> +   (the arguments to the Python function), and then a
> +   gdbpy_opt_borrowed_ref for the keywords.  The function can return
> +   any type (see the to_python overloads); and should throw an
> +   exception on error.  If gdb_python_exception is thrown, the Python
> +   exception must already have been set.
> +
> +   The gdb policy is that varargs methods must also accept keywords,
> +   and this is enforced here.  */
> +template<auto F>
> +constexpr PyMethodDef
> +varargs_function (std::string_view name, std::string_view doc)
> +{
> +  using namespace safety_details;
> +  return {
> +    name.data (),
> +    (PyCFunction) fn_wrapper<F>,
> +    /* gdb's rule is that varargs should also use keywords.  */
> +    METH_VARARGS | METH_KEYWORDS,
> +    doc.data (),
> +  };
> +}
> +
> +/* Like a varargs function, but this implements a method on some
> +   Python type that gdb provides.  The implementation class and a
> +   pointer-to-method must be specified.  */
> +template<typename C, auto M>
> +constexpr PyMethodDef
> +varargs_method (std::string_view name, std::string_view doc)
> +{
> +  using namespace safety_details;
> +  return {
> +    name.data (),
> +    (PyCFunction) varargs_wrapper<C, M>,
> +    /* gdb's rule is that varargs should also use keywords.  */
> +    METH_VARARGS | METH_KEYWORDS,
> +    doc.data (),
> +  };
> +}
> +
> +/* A function that wraps a "repr" or "str" method.  */
> +template<typename C, auto M>
> +PyObject *
> +wrap_repr (PyObject *arg)

Maybe something like 'wrap_tp_callback' might be better?  I just know
every time I see 'wrap_repr' for something that isn't 'repr' I'm going
to assume copy & paste error, and then have to go and remind myself that
'wrap_repr' has wider uses.  Now's a great time to find a better name
before this starts getting used more widely..

Thanks,
Andrew

> +{
> +  using namespace safety_details;
> +  return wrapped_method (M, static_cast<C *> (arg));
> +}
> +
> +/* A function that wraps a "get" method.  */
> +template<typename C, auto M>
> +PyObject *
> +wrap_getter (PyObject *arg, void *closure)
> +{
> +  using namespace safety_details;
> +  /* In gdb the closure argument is never used.  */
> +  return wrapped_method (M, static_cast<C *> (arg));
> +}
> +
> +/* A function that wraps a "set" method.  */
> +template<typename C, void (C::*M) (gdbpy_opt_borrowed_ref)>
> +int
> +wrap_setter (PyObject *arg, PyObject *value, void *closure)
> +{
> +  using namespace safety_details;
> +  try
> +    {
> +      C *self = static_cast<C *> (arg);
> +      /* In gdb the closure argument is never used.  */
> +      (self->*M) (gdbpy_opt_borrowed_ref (value));
> +    }
> +  catch (const gdb_python_exception &pye)
> +    {
> +      gdb_assert (PyErr_Occurred () != nullptr);
> +      return -1;
> +    }
> +  catch (const gdb_exception &exc)
> +    {
> +      return gdbpy_handle_gdb_exception (-1, exc);
> +    }
> +
> +  return 0;
> +}
> +
> +#endif /* GDB_PYTHON_PY_SAFETY_H */
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index 6df0c62e2b3..e4f35f8cd88 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -1383,5 +1383,6 @@ py_notimplemented ()
>  #undef Py_RETURN_NOTIMPLEMENTED
>  
>  #include "py-wrappers.h"
> +#include "py-safety.h"
>  
>  #endif /* GDB_PYTHON_PYTHON_INTERNAL_H */
>
> -- 
> 2.49.0

Re: [PATCH v2 3/4] Add wrappers for Python implementation functions and methods

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

> This adds some wrappers for Python implementation functions and
> methods, and a couple of new constexpr functions to create PyMethodDef
> entries.  This provides a few safety benefits:
>
> * The new-style API approach (see previous patch) is implemented by
>   the wrapper.  That is, exceptions are caught here and transformed.
>
> * The implementation functions can now return any reasonable type,
>   with automatic conversion by the wrapper.
>
> * The function API and the appropriate METH_* flags are handled
>   together, avoiding any possible discrepancy.
>
> This approach also means that we can modify the old rule that gdb
> calls must be wrapped in a try/catch -- the try/catch is now provided
> by the wrapper function, so the implementation can be written in a
> more natural way.
>
> Note this patch is not 100% complete.  There should be one more
> wrapper for case where a method takes a single argument (though we
> probably cannot use METH_O unfortunately).  There may be some other
> holes as well.

Maybe reword this so it doesn't imply that THIS patch is not complete,
but rather the implementation as a whole is not complete and will
require future follow on patches.  What you've got is good enough to
start using it.


> ---
>  gdb/python/py-safety.h       | 320 +++++++++++++++++++++++++++++++++++++++++++
>  gdb/python/python-internal.h |   1 +
>  2 files changed, 321 insertions(+)
>
> diff --git a/gdb/python/py-safety.h b/gdb/python/py-safety.h
> new file mode 100644
> index 00000000000..62018dacc0f
> --- /dev/null
> +++ b/gdb/python/py-safety.h
> @@ -0,0 +1,320 @@
> +/* Wrappers for some Python safety.
> +
> +   Copyright (C) 2026 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef GDB_PYTHON_PY_SAFETY_H
> +#define GDB_PYTHON_PY_SAFETY_H
> +
> +#include "gdbsupport/traits.h"
> +#include "py-ref.h"
> +#include "py-wrappers.h"
> +#include "charset.h"
> +
> +/* This file holds wrapper templates for the various ways that gdb
> +   code might be exposed to Python.  These wrappers are part of gdb's
> +   "Python safety" approach -- utilities designed to try to prevent
> +   refcount problems, missing error checks, and that also remove the
> +   need to wrap calls into gdb in an explicit try/catch.
> +
> +   See py-wrappers.h for some more discussion of this.
> +
> +   Implementation functions and methods -- the stuff you write to
> +   expose some part of gdb to Python -- are written in a certain
> +   style.
> +
> +   An implementation function is a module-level function.  For example
> +   something like 'gdb.register_window_type' is implemented as a
> +   function.  An implementation method is a method of the gdb class
> +   implementing a certain Python type; for example
> +   'gdb.TuiWindow.width' is implemented via a method of
> +   gdbpy_tui_window.
> +
> +   Each of these will accept gdbpy_borrowed_ref arguments (or in some
> +   more limited situations, a gdbpy_opt_borrowed_ref) and return any
> +   relevant type, which will be automatically converted (see the
> +   'to_python' overloads below) to the correct Python type.  Note that
> +   if the 'to_python' functions aren't appropriate in your case, you
> +   can always use the escape hatch of returning a gdbpy_ref<> and
> +   handling type conversion manually.
> +
> +   There are some constexpr functions below that wrap the
> +   implementation methods, and create PyMethodDef objects and the
> +   like.
> +
> +   Implementation methods are expected to use the wrappers in
> +   py-wrappers.h and not generally call into Python directly.
> +
> +   Implementation details of the method-wrapping safety code are put
> +   into this namespace, just to emphasize that these shouldn't be used
> +   elsewhere.  Skip past the namespace to find the public APIs.  */
> +namespace safety_details
> +{
> +/* Overloads of "to_python" are used by the safety wrappers to convert
> +   a function's real return value to a Python object.  A new reference
> +   will always be returned.  For the time being these are kept as a
> +   detail of the method-wrapping code.  However we may want to
> +   consider exposing these more generally.  */
> +
> +static inline PyObject *
> +to_python (bool value)
> +{
> +  /* Note that this cannot fail.  */
> +  return PyBool_FromLong (value);
> +}
> +
> +template<typename T, typename = std::is_integral<T>>

I think the SFINAE part here is wrong, like in the previous commit.  I
think gdb::Requires<std::is_integral<T>> might be what you mean.

> +static inline PyObject *
> +to_python (T value)
> +{
> +  if constexpr (std::is_signed<T>::value)
> +    return gdb_py_object_from_longest (value).release ();
> +  else
> +    return gdb_py_object_from_ulongest (value).release ();
> +}
> +
> +static inline PyObject *
> +to_python (const char *value)
> +{
> +  if (value == nullptr)
> +    return py_none ().release ();
> +  return PyUnicode_Decode (value, strlen (value), host_charset (), nullptr);
> +}
> +
> +static inline PyObject *
> +to_python (std::string &&value)
> +{
> +  return PyUnicode_Decode (value.c_str (), value.size (),
> +			   host_charset (), nullptr);
> +}
> +
> +static inline PyObject *
> +to_python (const std::string &value)
> +{
> +  return PyUnicode_Decode (value.c_str (), value.size (),
> +			   host_charset (), nullptr);
> +}
> +
> +static inline PyObject *
> +to_python (gdb::unique_xmalloc_ptr<char> &&value)
> +{
> +  if (value == nullptr)
> +    return py_none ().release ();
> +  return PyUnicode_Decode (value.get (), strlen (value.get ()),
> +			   host_charset (), nullptr);
> +}

Sorry for the double review.

None of the to_python functions check their return values for error, for
example PyUnicode_Decode can fail and return NULL, but you don't check
for this.

But this is OK.  to_python is only used at the point where we transition
back from GDB's C++ code to the Python internals, so if PyUnicode_Decode
(for example) returns NULL and sets an exception, this will be caught by
Python.

I have two pieces of feedback on this:

 1. I think this should be explicitly called out in the comment above
    the to_python functions, rather than making everyone figure out that
    this is not a mistake.

 2. The comment in this to_python function:

    > +static inline PyObject *
    > +to_python (bool value)
    > +{
    > +  /* Note that this cannot fail.  */
    > +  return PyBool_FromLong (value);
    > +}
    > +

    Is (IMHO) confusing.  It implies the lack of error checking here is
    because PyBool_FromLong cannot fail, which is why, when I look at
    later to_python functions which include calls that *can* fail, I
    asked myself, where's the error checking.

    I think this comment should just go.

> +
> +static inline PyObject *
> +to_python (gdbpy_ref<> &&value)
> +{
> +  return value.release ();
> +}
> +
> +/* An instantiation of this function is used when calling a gdb method
> +   from Python.  It accepts some number of arguments (normally
> +   gdbpy_borrowed_ref or gdbpy_opt_borrowed_ref), and then then calls

typo: ".... then THEN calls ..."

> +   the underlying function F.  Any exceptions are caught and
> +   converted, and the return value of F is converted to a Python
> +   object as appropriate.  */
> +template<auto F, typename... Args>
> +PyObject *
> +wrapped_function (Args... args)
> +{
> +  try
> +    {
> +      using result_type = typename std::invoke_result_t<decltype (F), Args...>;
> +
> +      if constexpr (std::is_void_v<result_type>)
> +	{
> +	  F (args...);
> +	  return py_none ().release ();
> +	}
> +      else
> +	return to_python (F (args...));
> +    }
> +  catch (const gdb_python_exception &pye)
> +    {
> +      gdb_assert (PyErr_Occurred () != nullptr);
> +      return nullptr;
> +    }
> +  catch (const gdb_exception &exc)
> +    {
> +      return gdbpy_handle_gdb_exception (nullptr, exc);
> +    }
> +}
> +
> +/* An instantiation of this function is used when calling a gdb method
> +   from Python.  It accepts some number of arguments (normally
> +   gdbpy_borrowed_ref or gdbpy_opt_borrowed_ref), and then then calls
> +   the underlying function F.  Any exceptions are caught and
> +   converted, and the return value of F is converted to a Python
> +   object as appropriate.  */

This comment needs updating.  It also include the 'then then' typo from
the wrapped_function comment, but also references function F, when it
should be taking about method CLASS::METH or maybe just METH?  Anyway,
certainly not F.

> +template<typename Class, typename Ret, typename... Args>
> +PyObject *
> +wrapped_method (Ret (Class::*meth) (Args...), Class *self, Args... args)
> +{
> +  try
> +    {
> +      if constexpr (std::is_void_v<Ret>)
> +	{
> +	  (self->*meth) (args...);
> +	  return py_none ().release ();
> +	}
> +      else
> +	return to_python ((self->*meth) (args...));
> +    }
> +  catch (const gdb_python_exception &pye)
> +    {
> +      gdb_assert (PyErr_Occurred () != nullptr);
> +      return nullptr;
> +    }
> +  catch (const gdb_exception &exc)
> +    {
> +      return gdbpy_handle_gdb_exception (nullptr, exc);
> +    }
> +}
> +
> +template<auto F>
> +PyObject *
> +fn_wrapper (PyObject *self, PyObject *args, PyObject *kw)
> +{
> +  return wrapped_function<F> (gdbpy_borrowed_ref (args),
> +			      gdbpy_opt_borrowed_ref (kw));
> +}
> +
> +template<typename C, auto M>
> +PyObject *
> +varargs_wrapper (PyObject *self, PyObject *args, PyObject *kw)
> +{
> +  return wrapped_method (M, static_cast<C *> (self),
> +			 gdbpy_borrowed_ref (args),
> +			 gdbpy_opt_borrowed_ref (kw));
> +}
> +
> +} /* namespace safety_details */
> +
> +/* 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
> +   method M is wrapped to call to_python and to catch exceptions per
> +   the safety protocol.  */
> +template<typename C, auto M>
> +constexpr PyMethodDef
> +noargs_method (std::string_view name, std::string_view doc)
> +{
> +  using namespace safety_details;
> +  return {
> +    name.data (),
> +    [] (PyObject *self, PyObject *args) -> PyObject *
> +    {
> +      return wrapped_method (M, static_cast<C *> (self));
> +    },
> +    METH_NOARGS,
> +    doc.data (),
> +  };
> +}
> +
> +/* This is used to create the PyMethodDef for a varargs function.  It
> +   takes the underlying implementation function as a template
> +   argument, and also arguments for the method name and documentation
> +   string.
> +
> +   The underlying function should accept a gdbpy_borrowed_ref argument
> +   (the arguments to the Python function), and then a
> +   gdbpy_opt_borrowed_ref for the keywords.  The function can return
> +   any type (see the to_python overloads); and should throw an
> +   exception on error.  If gdb_python_exception is thrown, the Python
> +   exception must already have been set.
> +
> +   The gdb policy is that varargs methods must also accept keywords,
> +   and this is enforced here.  */
> +template<auto F>
> +constexpr PyMethodDef
> +varargs_function (std::string_view name, std::string_view doc)
> +{
> +  using namespace safety_details;
> +  return {
> +    name.data (),
> +    (PyCFunction) fn_wrapper<F>,
> +    /* gdb's rule is that varargs should also use keywords.  */
> +    METH_VARARGS | METH_KEYWORDS,
> +    doc.data (),
> +  };
> +}
> +
> +/* Like a varargs function, but this implements a method on some
> +   Python type that gdb provides.  The implementation class and a
> +   pointer-to-method must be specified.  */
> +template<typename C, auto M>
> +constexpr PyMethodDef
> +varargs_method (std::string_view name, std::string_view doc)
> +{
> +  using namespace safety_details;
> +  return {
> +    name.data (),
> +    (PyCFunction) varargs_wrapper<C, M>,
> +    /* gdb's rule is that varargs should also use keywords.  */
> +    METH_VARARGS | METH_KEYWORDS,
> +    doc.data (),
> +  };
> +}
> +
> +/* A function that wraps a "repr" or "str" method.  */
> +template<typename C, auto M>

In wrap_setter below you are explicit about the signature of M.  I much
prefer the explicit form, but here in wrap_repr and in wrap_getter you
use 'auto'.  Could we switch to the explicit form in these two too?

I know this is partly my personally preference, but I prefer to reserve
'auto' for places where either the type is unknown (e.g. lambda
functions) or for templates where the type can be different in different
instantiations.

Thanks,
Andrew

> +PyObject *
> +wrap_repr (PyObject *arg)
> +{
> +  using namespace safety_details;
> +  return wrapped_method (M, static_cast<C *> (arg));
> +}
> +
> +/* A function that wraps a "get" method.  */
> +template<typename C, auto M>
> +PyObject *
> +wrap_getter (PyObject *arg, void *closure)
> +{
> +  using namespace safety_details;
> +  /* In gdb the closure argument is never used.  */
> +  return wrapped_method (M, static_cast<C *> (arg));
> +}
> +
> +/* A function that wraps a "set" method.  */
> +template<typename C, void (C::*M) (gdbpy_opt_borrowed_ref)>
> +int
> +wrap_setter (PyObject *arg, PyObject *value, void *closure)
> +{
> +  using namespace safety_details;
> +  try
> +    {
> +      C *self = static_cast<C *> (arg);
> +      /* In gdb the closure argument is never used.  */
> +      (self->*M) (gdbpy_opt_borrowed_ref (value));
> +    }
> +  catch (const gdb_python_exception &pye)
> +    {
> +      gdb_assert (PyErr_Occurred () != nullptr);
> +      return -1;
> +    }
> +  catch (const gdb_exception &exc)
> +    {
> +      return gdbpy_handle_gdb_exception (-1, exc);
> +    }
> +
> +  return 0;
> +}
> +
> +#endif /* GDB_PYTHON_PY_SAFETY_H */
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index 6df0c62e2b3..e4f35f8cd88 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -1383,5 +1383,6 @@ py_notimplemented ()
>  #undef Py_RETURN_NOTIMPLEMENTED
>  
>  #include "py-wrappers.h"
> +#include "py-safety.h"
>  
>  #endif /* GDB_PYTHON_PYTHON_INTERNAL_H */
>
> -- 
> 2.49.0

Re: [PATCH v2 4/4] Convert py-tui.c to the "python safety" approach

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

> This patch mostly converts py-tui.c to use the new Python safety code.
>
> In particular:
>
> * All methods of gdb.TuiWindow are now implemented as straightforward
>   methods of gdbpy_tui_window.
>
> * gdb.register_tui_window is converted and simply returns 'void'.

typo: gdb.register_tui_window should be gdbpy_register_tui_window I think.

>
> I converted this particular file because it was relatively
> straightforward, while also demonstrating most of the features of the
> new approach.  For example, explicit result checks aren't needed,
> try/catch can be removed, and the methods are now written in a natural
> style.
>
> Note that more conversion remains to be done here:
>
> * gdbpy_tui_enabled hasn't been converted and still does explicit
>   checks.
>
> * There's one explicit check in gdbpy_tui_window::set_title.  Fully
>   implementing the safety approach means that some low-level things
>   should eventually be converted to throw; but some work has to be
>   deferred until a lot of the work is complete.

I like the new approach, and think it looks like a great improvement
over the existing code.

> ---
>  gdb/python/py-tui.c          | 208 ++++++++++++++++++-------------------------
>  gdb/python/python-internal.h |   5 +-
>  gdb/python/python.c          |   5 +-
>  3 files changed, 92 insertions(+), 126 deletions(-)
>
> diff --git a/gdb/python/py-tui.c b/gdb/python/py-tui.c
> index 111e0f4c9e3..0755d7b92e3 100644
> --- a/gdb/python/py-tui.c
> +++ b/gdb/python/py-tui.c
> @@ -50,7 +50,34 @@ struct gdbpy_tui_window: public PyObject
>    tui_py_window *window;
>  
>    /* Return true if this object is valid.  */
> -  bool is_valid () const;
> +  bool is_valid ();

Dropping 'const' here is unfortunate, and I think is a consequence of
either noargs_method or maybe wrapped_method requiring non-const.

Can we add a 'const' aware overload of noargs_method, or wrapped_method,
or whatever, and allow is_valid to retain the const qualifier?

I think it would be a shame if a consequence of this work is that we end
up dropping the const qualifiers in many places.

If this works, then there might be other places in the wrapper functions
(patch #3) where we could/should add const overloads.

> +
> +  /* Require that this object be valid.  Throws exception if not.  */
> +  void require_valid ()
> +  {
> +    if (!is_valid ())
> +      gdbpy_err_format (PyExc_RuntimeError, _("TUI window is invalid."));
> +  }
> +
> +  /* Erase the TUI window.  */
> +  void erase ();
> +
> +  /* Python function that writes some text to a TUI window.  */
> +  void write (gdbpy_borrowed_ref args, gdbpy_opt_borrowed_ref kw);
> +
> +  /* Return the width of the TUI window.  */
> +  int width ();
> +
> +  /* Return the height of the TUI window.  */
> +  int height ();
> +
> +  /* Return the title of the TUI window.  */
> +  const std::string &title ();
> +
> +  /* Set the title of the TUI window.  */
> +  void set_title (gdbpy_opt_borrowed_ref new_title);
> +
> +  static PyTypeObject *corresponding_object_type;
>  };
>  
>  extern PyTypeObject gdbpy_tui_window_object_type;
> @@ -150,7 +177,7 @@ class tui_py_window : public tui_win_info
>  /* See gdbpy_tui_window declaration above.  */
>  
>  bool
> -gdbpy_tui_window::is_valid () const
> +gdbpy_tui_window::is_valid ()
>  {
>    return window != nullptr && tui_active;
>  }
> @@ -406,173 +433,109 @@ gdbpy_tui_window_maker::operator() (const char *win_name)
>  
>  /* Implement "gdb.register_window_type".  */
>  
> -PyObject *
> -gdbpy_register_tui_window (PyObject *self, PyObject *args, PyObject *kw)
> +void
> +gdbpy_register_tui_window (gdbpy_borrowed_ref args,
> +			   gdbpy_opt_borrowed_ref kw)
>  {
>    static const char *keywords[] = { "name", "constructor", nullptr };
> -
>    const char *name;
>    PyObject *cons_obj;
> +  gdbpy_arg_parse_tuple_and_keywords (args, kw, "sO", keywords,
> +				      &name, &cons_obj);
>  
> -  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "sO", keywords,
> -					&name, &cons_obj))
> -    return nullptr;
> -
> -  try
> -    {
> -      gdbpy_tui_window_maker constr (gdbpy_ref<>::new_reference (cons_obj));
> -      tui_register_window (name, constr);
> -    }
> -  catch (const gdb_exception &except)
> -    {
> -      return gdbpy_handle_gdb_exception (nullptr, except);
> -    }
> -
> -  return py_none ().release ();
> +  gdbpy_tui_window_maker constr (gdbpy_ref<>::new_reference (cons_obj));
> +  tui_register_window (name, constr);
>  }
>  
>  \f
>  
> -/* Require that "Window" be a valid window.  */
> -
> -#define REQUIRE_WINDOW(Window)					\
> -    do {							\
> -      if (!(Window)->is_valid ())				\
> -	return PyErr_Format (PyExc_RuntimeError,		\
> -			     _("TUI window is invalid."));	\
> -    } while (0)
> -
> -/* Require that "Window" be a valid window.  */
> -
> -#define REQUIRE_WINDOW_FOR_SETTER(Window)			\
> -    do {							\
> -      if (!(Window)->is_valid ())				\
> -	{							\
> -	  PyErr_Format (PyExc_RuntimeError,			\
> -			_("TUI window is invalid."));		\
> -	  return -1;						\
> -	}							\
> -    } while (0)
> -
> -/* Python function which checks the validity of a TUI window
> -   object.  */
> -static PyObject *
> -gdbpy_tui_is_valid (PyObject *self, PyObject *args)
> -{
> -  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
> -
> -  if (win->is_valid ())
> -    return py_true ().release ();
> -  return py_false ().release ();
> -}
> -
>  /* Python function that erases the TUI window.  */
> -static PyObject *
> -gdbpy_tui_erase (PyObject *self, PyObject *args)
> +void
> +gdbpy_tui_window::erase ()
>  {
> -  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
> -
> -  REQUIRE_WINDOW (win);
> -
> -  win->window->erase ();
> -
> -  return py_none ().release ();
> +  require_valid ();
> +  window->erase ();
>  }
>  
>  /* Python function that writes some text to a TUI window.  */
> -static PyObject *
> -gdbpy_tui_write (PyObject *self, PyObject *args, PyObject *kw)
> +void
> +gdbpy_tui_window::write (gdbpy_borrowed_ref args, gdbpy_opt_borrowed_ref kw)
>  {
> +  require_valid ();
> +
>    static const char *keywords[] = { "string", "full_window", nullptr };
>  
> -  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
>    const char *text;
>    int full_window = 0;
>  
> -  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "s|i", keywords,
> -					&text, &full_window))
> -    return nullptr;
> -
> -  REQUIRE_WINDOW (win);
> +  gdbpy_arg_parse_tuple_and_keywords (args, kw, "s|i", keywords,
> +				      &text, &full_window);
>  
> -  win->window->output (text, full_window);
> -
> -  return py_none ().release ();
> +  window->output (text, full_window);
>  }
>  
> -/* Return the width of the TUI window.  */
> -static PyObject *
> -gdbpy_tui_width (PyObject *self, void *closure)
> +int
> +gdbpy_tui_window::width ()
>  {
> -  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
> -  REQUIRE_WINDOW (win);
> -  gdbpy_ref<> result
> -    = gdb_py_object_from_longest (win->window->viewport_width ());
> -  return result.release ();
> +  require_valid ();
> +  return window->viewport_width ();
>  }
>  
> -/* Return the height of the TUI window.  */
> -static PyObject *
> -gdbpy_tui_height (PyObject *self, void *closure)
> +int
> +gdbpy_tui_window::height ()
>  {
> -  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
> -  REQUIRE_WINDOW (win);
> -  gdbpy_ref<> result
> -    = gdb_py_object_from_longest (win->window->viewport_height ());
> -  return result.release ();
> +  require_valid ();
> +  return window->viewport_height ();
>  }
>  
> -/* Return the title of the TUI window.  */
> -static PyObject *
> -gdbpy_tui_title (PyObject *self, void *closure)
> +const std::string &
> +gdbpy_tui_window::title ()
>  {
> -  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
> -  REQUIRE_WINDOW (win);
> -  return host_string_to_python_string (win->window->title ().c_str ()).release ();
> +  require_valid ();
> +  return window->title ();
>  }
>  
>  /* Set the title of the TUI window.  */
> -static int
> -gdbpy_tui_set_title (PyObject *self, PyObject *newvalue, void *closure)
> +void
> +gdbpy_tui_window::set_title (gdbpy_opt_borrowed_ref new_title)
>  {
> -  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
> +  require_valid ();
>  
> -  REQUIRE_WINDOW_FOR_SETTER (win);
> -
> -  if (newvalue == nullptr)
> -    {
> -      PyErr_Format (PyExc_TypeError, _("Cannot delete \"title\" attribute."));
> -      return -1;
> -    }
> +  if (new_title == nullptr)
> +    gdbpy_err_format (PyExc_TypeError,
> +		      _("Cannot delete \"title\" attribute."));
>  
> +  /* FIXME: safety */

Please can you expand this comment.  I know this is something you're
actively working on, and the plan is that this comment probably
shouldn't exist in the code for too long.  But it might, so we should
assume that it will, and write this accordingly.

For me, and 'FIXME' command should explain (1) what needs fixing, and
(2) why it cannot currently be fixed.  Armed with these two pieces of
information I can, in the future, make an informed decision about
whether the issue has been, or can now, be fixed.

I'll be honest, even with the commit message hint, I don't really
understand what it is that needs fixing here, I guess it's that you'd
like python_string_to_host_string to throw rather than return NULL?  But
the issue is that this function is used in non-safety code, so cannot
(currently) be changed?  So we'd eventually have:

    gdb::unique_xmalloc_ptr<char> value
      = python_string_to_host_string (new_title);
    gdb_assert (value != nullptr);
    ... etc ...


Thanks,
Andrew

Re: [PATCH v2 1/4] Add gdbpy_borrowed_ref

[Tom Tromey]

Andrew> OK, please excuse my ignorance here, I might be completely wrong, but I
Andrew> believe the line 'typename = std::is_convertible<T *, PyObject *>' is
Andrew> here for the purpose of SFINAE.  I believe this was copied from
Andrew> gdb_ref_ptr.h, but I think this line might be wrong, both there and
Andrew> here.

I sent a separate patch to fix up gdb_ref_ptr.h.

Tom

Re: [PATCH v2 1/4] Add gdbpy_borrowed_ref

[Tom Tromey]

>> This adds new gdbpy_opt_borrowed_ref and gdbpy_borrowed_ref classes.
>> These class is primarily for code "documentation" purposes -- it makes

Andrew> type: These CLASSES ARE primarily ....

Fixed.

Andrew> Could/should this be marked as 'const'?

Yeah, plus the other ones you pointed out.

>> +  /* Allow a (checked) conversion to any subclass of PyObject.  */
>> +  template<typename T,
>> +	   typename = std::is_convertible<T *, PyObject *>>
>> +  operator T * ()
>> +  {
>> +    gdb_assert (PyObject_TypeCheck (m_obj, T::corresponding_object_type));
>> +    return static_cast<T *> (m_obj);
>> +  }

Andrew> OK, please excuse my ignorance here, I might be completely wrong, but I
Andrew> believe the line 'typename = std::is_convertible<T *, PyObject *>' is
Andrew> here for the purpose of SFINAE.  I believe this was copied from
Andrew> gdb_ref_ptr.h, but I think this line might be wrong, both there and
Andrew> here.

Yep, oops.

I fixed this here & sent a separate patch for gdb_ref_ptr.h.

Tom

Re: [PATCH v2 2/4] Add wrappers for some Python APIs

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

>> +static inline gdbpy_borrowed_ref
>> +gdbpy_dict_get_item_with_error (gdbpy_borrowed_ref dict,
>> +				gdbpy_borrowed_ref key)
>> +{
>> +  PyObject *result = PyDict_GetItemWithError (dict, key);
>> +  if (result == nullptr)
>> +    throw gdb_python_exception ();
>> +  return result;

Andrew> PyDict_GetItemWithError will return NULL *without* an exception set if
Andrew> KEY is not found in DICT.  This will then trigger an assert in the
Andrew> gdb_python_exception constructor. Because of the *cough* lack of
Andrew> comments, it's unclear what the function's intended behaviour is, but
Andrew> this feels like a bug.

Thanks for catching this, it is indeed a bug.

I added small comments to most of the functions, and then expanded on
them where I thought it made sense.  In most cases I figured the big
comment at the top of the file covers the details.

I changed this particular function to return gdbpy_opt_borrowed_ref in
the case where PyDict_GetItemWithError returns NULL without setting the
exception.

>> +static inline gdbpy_ref<>
>> +gdbpy_unicode_from_format (const char *fmt, ...)
>> +{
>> +  va_list args;
>> +  va_start (args, fmt);
>> +  gdbpy_ref<> result (PyUnicode_FromFormatV (fmt, args));
>> +  if (result == nullptr)
>> +    throw gdb_python_exception ();
>> +  va_end (args);

Andrew> If an exception is thrown in this case then we fail to call va_end,
Andrew> which I believe is technically undefined behaviour (though in reality
Andrew> it's probably harmless), still, we should fix this.

I used a template function here and elsewhere in this file.

Note that this discovery means that many other uses of va_end in gdb are
already wrong.

>> +template<typename... Arg>
>> +[[noreturn]] void
>> +gdbpy_err_format (gdbpy_borrowed_ref type, const char *fmt, Arg... args)
>> +{
>> +  PyErr_Format (type, fmt, std::forward<Arg> (args)...);

Andrew> This isn't really a request for a change, more a question for my own
Andrew> education.  My understanding is that std::forward is intended to be used
Andrew> with forwarding reference arguments, which ARGS is not.

Yeah, I agree with your analysis.  We don't need perfect forwarding here
so I've removed the std::forward.

Tom

Re: [PATCH v2 4/4] Convert py-tui.c to the "python safety" approach

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

>> * gdb.register_tui_window is converted and simply returns 'void'.

Andrew> typo: gdb.register_tui_window should be gdbpy_register_tui_window I think.

I used the Python name rather than the C++ name.
I'll change it though.

>> /* Return true if this object is valid.  */
>> -  bool is_valid () const;
>> +  bool is_valid ();

Andrew> Dropping 'const' here is unfortunate, and I think is a consequence of
Andrew> either noargs_method or maybe wrapped_method requiring non-const.

I will look but perhaps add the overloads on an as-needed basis.

Andrew> Can we add a 'const' aware overload of noargs_method, or wrapped_method,
Andrew> or whatever, and allow is_valid to retain the const qualifier?

Yep, I fixed this.

Andrew> If this works, then there might be other places in the wrapper functions
Andrew> (patch #3) where we could/should add const overloads.

>> +  /* FIXME: safety */

Andrew> Please can you expand this comment.  I know this is something you're
Andrew> actively working on, and the plan is that this comment probably
Andrew> shouldn't exist in the code for too long.  But it might, so we should
Andrew> assume that it will, and write this accordingly.

I did this.

Andrew> I'll be honest, even with the commit message hint, I don't really
Andrew> understand what it is that needs fixing here, I guess it's that you'd
Andrew> like python_string_to_host_string to throw rather than return NULL?  But
Andrew> the issue is that this function is used in non-safety code, so cannot
Andrew> (currently) be changed?

Yes, that's correct.

Tom

Re: [PATCH v2 3/4] Add wrappers for Python implementation functions and methods

[Tom Tromey]

>> +noargs_method (std::string_view name, std::string_view doc)

Andrew> Using std::string_view seems a little suspect here.

Yeah.  I think some earlier attempt at this required string_view, but
I've changed it to 'const char *' and it's fine.

>> +/* A function that wraps a "repr" or "str" method.  */
>> +template<typename C, auto M>
>> +PyObject *
>> +wrap_repr (PyObject *arg)

Andrew> Maybe something like 'wrap_tp_callback' might be better?  I just know
Andrew> every time I see 'wrap_repr' for something that isn't 'repr' I'm going
Andrew> to assume copy & paste error, and then have to go and remind myself that
Andrew> 'wrap_repr' has wider uses.  Now's a great time to find a better name
Andrew> before this starts getting used more widely..

I changed it to wrap_tp_callback.

Tom

Re: [PATCH v2 3/4] Add wrappers for Python implementation functions and methods

[Tom Tromey]

>> Note this patch is not 100% complete.  There should be one more
>> wrapper for case where a method takes a single argument (though we
>> probably cannot use METH_O unfortunately).  There may be some other
>> holes as well.

Andrew> Maybe reword this so it doesn't imply that THIS patch is not complete,
Andrew> but rather the implementation as a whole is not complete and will
Andrew> require future follow on patches.  What you've got is good enough to
Andrew> start using it.

I updated the text a bit.

>> +template<typename T, typename = std::is_integral<T>>

Andrew> I think the SFINAE part here is wrong, like in the previous commit.  I
Andrew> think gdb::Requires<std::is_integral<T>> might be what you mean.

Fixed.

Andrew> None of the to_python functions check their return values for error, for
Andrew> example PyUnicode_Decode can fail and return NULL, but you don't check
Andrew> for this.

Andrew> But this is OK.  to_python is only used at the point where we transition
Andrew> back from GDB's C++ code to the Python internals, so if PyUnicode_Decode
Andrew> (for example) returns NULL and sets an exception, this will be caught by
Andrew> Python.

Andrew> I have two pieces of feedback on this:

Andrew>  1. I think this should be explicitly called out in the comment above
Andrew>     the to_python functions, rather than making everyone figure out that
Andrew>     this is not a mistake.

I updated the comment that precedes the to_python functions as a whole.

>> +  /* Note that this cannot fail.  */

Andrew>     Is (IMHO) confusing.  It implies the lack of error checking here is
Andrew>     because PyBool_FromLong cannot fail, which is why, when I look at
Andrew>     later to_python functions which include calls that *can* fail, I
Andrew>     asked myself, where's the error checking.

Andrew>     I think this comment should just go.

Deleted.

>> +   gdbpy_borrowed_ref or gdbpy_opt_borrowed_ref), and then then calls

Andrew> typo: ".... then THEN calls ..."

Fixed.

>> +/* An instantiation of this function is used when calling a gdb method
>> +   from Python.  It accepts some number of arguments (normally
>> +   gdbpy_borrowed_ref or gdbpy_opt_borrowed_ref), and then then calls
>> +   the underlying function F.  Any exceptions are caught and
>> +   converted, and the return value of F is converted to a Python
>> +   object as appropriate.  */

Andrew> This comment needs updating.  It also include the 'then then' typo from
Andrew> the wrapped_function comment, but also references function F, when it
Andrew> should be taking about method CLASS::METH or maybe just METH?  Anyway,
Andrew> certainly not F.

Fixed.

>> +/* A function that wraps a "repr" or "str" method.  */
>> +template<typename C, auto M>

Andrew> In wrap_setter below you are explicit about the signature of M.  I much
Andrew> prefer the explicit form, but here in wrap_repr and in wrap_getter you
Andrew> use 'auto'.  Could we switch to the explicit form in these two too?

There are two issues with changing.

One is that while a setter should probably just return void, a getter
could return anything.  And, requiring a specific return type for tp_str
or tp_repr seems a bit heavy, like maybe it would be convenient to
return std::string in some spots or gdbpy_ref<> in others.

The other issue is that 'auto' means it automatically accepts const- or
non-const-methods.  This can be handled by overloads of course.

Anyway I left this as is but we can discuss further if you want.

Tom

Re: [PATCH v2 3/4] Add wrappers for Python implementation functions and methods

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>> Note this patch is not 100% complete.  There should be one more
>>> wrapper for case where a method takes a single argument (though we
>>> probably cannot use METH_O unfortunately).  There may be some other
>>> holes as well.
>
> Andrew> Maybe reword this so it doesn't imply that THIS patch is not complete,
> Andrew> but rather the implementation as a whole is not complete and will
> Andrew> require future follow on patches.  What you've got is good enough to
> Andrew> start using it.
>
> I updated the text a bit.
>
>>> +template<typename T, typename = std::is_integral<T>>
>
> Andrew> I think the SFINAE part here is wrong, like in the previous commit.  I
> Andrew> think gdb::Requires<std::is_integral<T>> might be what you mean.
>
> Fixed.
>
> Andrew> None of the to_python functions check their return values for error, for
> Andrew> example PyUnicode_Decode can fail and return NULL, but you don't check
> Andrew> for this.
>
> Andrew> But this is OK.  to_python is only used at the point where we transition
> Andrew> back from GDB's C++ code to the Python internals, so if PyUnicode_Decode
> Andrew> (for example) returns NULL and sets an exception, this will be caught by
> Andrew> Python.
>
> Andrew> I have two pieces of feedback on this:
>
> Andrew>  1. I think this should be explicitly called out in the comment above
> Andrew>     the to_python functions, rather than making everyone figure out that
> Andrew>     this is not a mistake.
>
> I updated the comment that precedes the to_python functions as a whole.
>
>>> +  /* Note that this cannot fail.  */
>
> Andrew>     Is (IMHO) confusing.  It implies the lack of error checking here is
> Andrew>     because PyBool_FromLong cannot fail, which is why, when I look at
> Andrew>     later to_python functions which include calls that *can* fail, I
> Andrew>     asked myself, where's the error checking.
>
> Andrew>     I think this comment should just go.
>
> Deleted.
>
>>> +   gdbpy_borrowed_ref or gdbpy_opt_borrowed_ref), and then then calls
>
> Andrew> typo: ".... then THEN calls ..."
>
> Fixed.
>
>>> +/* An instantiation of this function is used when calling a gdb method
>>> +   from Python.  It accepts some number of arguments (normally
>>> +   gdbpy_borrowed_ref or gdbpy_opt_borrowed_ref), and then then calls
>>> +   the underlying function F.  Any exceptions are caught and
>>> +   converted, and the return value of F is converted to a Python
>>> +   object as appropriate.  */
>
> Andrew> This comment needs updating.  It also include the 'then then' typo from
> Andrew> the wrapped_function comment, but also references function F, when it
> Andrew> should be taking about method CLASS::METH or maybe just METH?  Anyway,
> Andrew> certainly not F.
>
> Fixed.
>
>>> +/* A function that wraps a "repr" or "str" method.  */
>>> +template<typename C, auto M>
>
> Andrew> In wrap_setter below you are explicit about the signature of M.  I much
> Andrew> prefer the explicit form, but here in wrap_repr and in wrap_getter you
> Andrew> use 'auto'.  Could we switch to the explicit form in these two too?
>
> There are two issues with changing.
>
> One is that while a setter should probably just return void, a getter
> could return anything.  And, requiring a specific return type for tp_str
> or tp_repr seems a bit heavy, like maybe it would be convenient to
> return std::string in some spots or gdbpy_ref<> in others.
>
> The other issue is that 'auto' means it automatically accepts const- or
> non-const-methods.  This can be handled by overloads of course.
>
> Anyway I left this as is but we can discuss further if you want.

No, that's fine, thanks for explaining the benefit here.  I'm perfectly
happy with 'auto' when it's serving a good purpose like this.

Thanks,
Andrew