Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [PATCH 0/4] Python safety initial work
@ 2026-05-15 18:23 Tom Tromey
  2026-05-15 18:23 ` [PATCH 1/4] Add gdbpy_borrowed_ref Tom Tromey
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Tom Tromey @ 2026-05-15 18:23 UTC (permalink / raw)
  To: gdb-patches; +Cc: 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>
---
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       | 321 +++++++++++++++++++++++++++++++++++++++++
 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, 830 insertions(+), 126 deletions(-)
---
base-commit: 776715ea683b93c0415fc4eb31b4fcec5f2153fc
change-id: 20260515-python-safety-initial-422bf34a8ce3

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/4] Add gdbpy_borrowed_ref
  2026-05-15 18:23 [PATCH 0/4] Python safety initial work Tom Tromey
@ 2026-05-15 18:23 ` Tom Tromey
  2026-05-15 18:23 ` [PATCH 2/4] Add wrappers for some Python APIs Tom Tromey
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Tom Tromey @ 2026-05-15 18:23 UTC (permalink / raw)
  To: gdb-patches; +Cc: 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 2/4] Add wrappers for some Python APIs
  2026-05-15 18:23 [PATCH 0/4] Python safety initial work Tom Tromey
  2026-05-15 18:23 ` [PATCH 1/4] Add gdbpy_borrowed_ref Tom Tromey
@ 2026-05-15 18:23 ` Tom Tromey
  2026-05-15 18:23 ` [PATCH 3/4] Add wrappers for Python implementation functions and methods Tom Tromey
  2026-05-15 18:23 ` [PATCH 4/4] Convert py-tui.c to the "python safety" approach Tom Tromey
  3 siblings, 0 replies; 5+ messages in thread
From: Tom Tromey @ 2026-05-15 18:23 UTC (permalink / raw)
  To: gdb-patches; +Cc: 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 3147156d5be..8921854a120 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -1343,4 +1343,6 @@ class gdbpy_memoizing_registry_storage
 extern int eval_python_command (const char *command, int start_symbol,
 				const char *filename = nullptr);
 
+#include "py-wrappers.h"
+
 #endif /* GDB_PYTHON_PYTHON_INTERNAL_H */

-- 
2.49.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 3/4] Add wrappers for Python implementation functions and methods
  2026-05-15 18:23 [PATCH 0/4] Python safety initial work Tom Tromey
  2026-05-15 18:23 ` [PATCH 1/4] Add gdbpy_borrowed_ref Tom Tromey
  2026-05-15 18:23 ` [PATCH 2/4] Add wrappers for some Python APIs Tom Tromey
@ 2026-05-15 18:23 ` Tom Tromey
  2026-05-15 18:23 ` [PATCH 4/4] Convert py-tui.c to the "python safety" approach Tom Tromey
  3 siblings, 0 replies; 5+ messages in thread
From: Tom Tromey @ 2026-05-15 18:23 UTC (permalink / raw)
  To: gdb-patches; +Cc: 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       | 321 +++++++++++++++++++++++++++++++++++++++++++
 gdb/python/python-internal.h |   1 +
 2 files changed, 322 insertions(+)

diff --git a/gdb/python/py-safety.h b/gdb/python/py-safety.h
new file mode 100644
index 00000000000..d84358b72b4
--- /dev/null
+++ b/gdb/python/py-safety.h
@@ -0,0 +1,321 @@
+/* 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)
+{
+  if (value)
+    Py_RETURN_TRUE;
+  Py_RETURN_FALSE;
+}
+
+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)
+    Py_RETURN_NONE;
+  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)
+    Py_RETURN_NONE;
+  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...);
+	  Py_RETURN_NONE;
+	}
+      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...);
+	  Py_RETURN_NONE;
+	}
+      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 8921854a120..ca667681cb7 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -1344,5 +1344,6 @@ extern int eval_python_command (const char *command, int start_symbol,
 				const char *filename = nullptr);
 
 #include "py-wrappers.h"
+#include "py-safety.h"
 
 #endif /* GDB_PYTHON_PYTHON_INTERNAL_H */

-- 
2.49.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 4/4] Convert py-tui.c to the "python safety" approach
  2026-05-15 18:23 [PATCH 0/4] Python safety initial work Tom Tromey
                   ` (2 preceding siblings ...)
  2026-05-15 18:23 ` [PATCH 3/4] Add wrappers for Python implementation functions and methods Tom Tromey
@ 2026-05-15 18:23 ` Tom Tromey
  3 siblings, 0 replies; 5+ messages in thread
From: Tom Tromey @ 2026-05-15 18:23 UTC (permalink / raw)
  To: gdb-patches; +Cc: 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 be19193770f..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);
-    }
-
-  Py_RETURN_NONE;
+  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 ())
-    Py_RETURN_TRUE;
-  Py_RETURN_FALSE;
-}
-
 /* 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 ();
-
-  Py_RETURN_NONE;
+  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);
-
-  Py_RETURN_NONE;
+  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 ca667681cb7..e656ca73617 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 045626b70fd..689d121ce2d 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-05-15 18:30 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-05-15 18:23 [PATCH 0/4] Python safety initial work Tom Tromey
2026-05-15 18:23 ` [PATCH 1/4] Add gdbpy_borrowed_ref Tom Tromey
2026-05-15 18:23 ` [PATCH 2/4] Add wrappers for some Python APIs Tom Tromey
2026-05-15 18:23 ` [PATCH 3/4] Add wrappers for Python implementation functions and methods Tom Tromey
2026-05-15 18:23 ` [PATCH 4/4] Convert py-tui.c to the "python safety" approach Tom Tromey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox