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

  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(+)
  

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
  

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
  

>> +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
  

>> 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
  

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