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

[Andrew Burgess <aburgess@redhat.com>]

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