From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id UJtWAmTlCmolQwUAWB0awg (envelope-from ) for ; Mon, 18 May 2026 06:09:40 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=V1bll5zT; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id E324B1E062; Mon, 18 May 2026 06:09:39 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-3.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_VALIDITY_CERTIFIED_BLOCKED, RCVD_IN_VALIDITY_RPBL_BLOCKED,RCVD_IN_VALIDITY_SAFE_BLOCKED autolearn=ham autolearn_force=no version=4.0.1 Received: from vm01.sourceware.org (vm01.sourceware.org [38.145.34.32]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id AF6041E062 for ; Mon, 18 May 2026 06:09:38 -0400 (EDT) Received: from vm01.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id A72F34BAE7F4 for ; Mon, 18 May 2026 10:09:37 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org A72F34BAE7F4 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=V1bll5zT Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 003974BB1C21 for ; Mon, 18 May 2026 10:00:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 003974BB1C21 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 003974BB1C21 Authentication-Results: sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1779098417; cv=none; b=BrGI4fkoRuuKXPmAmLu2vZ2nEqFXEPqZfDzCmVqByBSTuSSNFh0J67wNQQW2aPxVjy7BUIJXosEZvHBkpa32hLyOlk27xrQX3WtoBPtu9gdL1W14HCcuVrgLBK4SuFj7+FtaiaUAQJ/3tPO2ekkVPR+OJ3FTWcKptF/JJltYeEs= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1779098417; c=relaxed/simple; bh=b/2EvnXMQwmobXZIguxQCXgp7QF0RegZXg6thQqLahA=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=ua+gL9VM5ynr+AYubrKfF3w0cVREojHPkNiG47rUr9hvx4/c/GU3XpNAIsTTdHmIYdTLy/qU5UcT7hvEpxkeSfnTMpUsaxQx1JWqV8WpAh6wq3GxMo0ADVk0i6yLgkI6570yLGYft0T6A7K8dkxfwI1//UwVBJdsKbvZwr6/e2A= ARC-Authentication-Results: i=1; sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=V1bll5zT DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 003974BB1C21 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1779098416; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=HZDJ5DpuJamjThGBic2uV77vXheCAaPtkh6lLKBWbWE=; b=V1bll5zTJIHRd986GN5SUTQGdYT0L9xsQ2kfNFpxWyszMbNZHNEyiEiiWIVTQUDdlFW0Uw 7iyT9srtQMhyhjTxk84Gr6tmgyuZQW7fdPg9FoIcPdPVzbtAP93nqwCdO+L9Hn2NkHaEJy pv4RWP2cArpcbAP8CkgjpzPYnNmFmVc= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-316-83D8_WsvNICgTLHTz_lmSA-1; Mon, 18 May 2026 06:00:15 -0400 X-MC-Unique: 83D8_WsvNICgTLHTz_lmSA-1 X-Mimecast-MFC-AGG-ID: 83D8_WsvNICgTLHTz_lmSA_1779098414 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-48fe6894f3fso11987555e9.2 for ; Mon, 18 May 2026 03:00:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1779098414; x=1779703214; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=HZDJ5DpuJamjThGBic2uV77vXheCAaPtkh6lLKBWbWE=; b=iq4Fa9oVyIh4VXxTpWJhRN4jtoV6wA/auJpgS+V0s+73wyjCei4N08vkZw5/m8Wzlw pal7ieSWv83f1u7p9g2+zK9/NwWuAlHkddRr+8SNsiQbNxTKkXUebFwXCUJehZlf8usI dVTIUIqWUhpBokyM9oEHa9yz1l3s5GAgxf74ku+xBIKvAURzHiXrgA4LgA/MjYz8VKtG Aou0aPoX8Wdvj9q5AdfZE7mZJiMCsqyZNEIpfJlYJB13l0h+54YYZkfpH2WUowsjyNkA is0fB0m/n770HKeHYF7uSKu+qFfVHvhCJ0ogJZfDKlJx+Dke1v0ahpLU1VmGbelMqxnn MaKQ== X-Forwarded-Encrypted: i=1; AFNElJ+IKkGR5mFI5Z/D57lEvUOmkPJK4aSRFonDSyb8hHhX3pWdltKglB9su3GJGym5qU1ZfUW7AS025ovvtA==@sourceware.org X-Gm-Message-State: AOJu0Yy6LC3Aa/NYUBS/9ZXWC/t1Kv+/3g8395LZwxPlzAx2Xgj3gyni M0HhzTLaBwguCmupB6wibuGayganxqdl+ouOMF4WtdJGVzh6RMV3KLVgNC8jCifbi1N7DW4xqCO PreIkc0YLt7pgtXf0c+XW8v38yoZPltiQQMsYDmrzsdlWIOhwrMVHthg5R2/PR7Q= X-Gm-Gg: Acq92OHRHNdanSVyfU2mtrWpjZ5d8ziGcE163utTn7bhUdoo5KKNWQ+4IUZfnC3WX7o mjS8wmWRY52opNWKzwxCDlS1NaOcaoDUvBKQKyZ27zOrY+VtmFE/rk2lgkR0vwKSiWZzAEDZWJI ZFYOKSZOSijJy2HRUDMc5IwGA7S+euybq0xDuxNuvGN6ysbO0JZaiWzUrlJhB1sqi/7OUAbiz8M vMAhDJrgEzK4E6tYHd+v7M96sQFNAQru8YcvaxoyAU4jvJ/p9VX5JTrqVTb9oYUpgVxAix9EpRr 8FkmJ0MIYUU3mUMO0WqKiXIq0Ia5vJKBQGZvWeudpvoyDNFkEJZE8SSmlh6l7lIFFkltrt3Z/gm uOLKL+8EMv6quVeSt X-Received: by 2002:a05:600c:8599:b0:490:778:4fe8 with SMTP id 5b1f17b1804b1-4900778509emr65682615e9.25.1779098413570; Mon, 18 May 2026 03:00:13 -0700 (PDT) X-Received: by 2002:a05:600c:8599:b0:490:778:4fe8 with SMTP id 5b1f17b1804b1-4900778509emr65681695e9.25.1779098412779; Mon, 18 May 2026 03:00:12 -0700 (PDT) Received: from localhost ([31.111.84.232]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-48fe4c88495sm239906305e9.4.2026.05.18.03.00.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 18 May 2026 03:00:12 -0700 (PDT) From: Andrew Burgess To: Tom Tromey , gdb-patches@sourceware.org Cc: Tom Tromey Subject: Re: [PATCH v2 3/4] Add wrappers for Python implementation functions and methods In-Reply-To: <20260515-python-safety-initial-v2-3-6129cadf258a@tromey.com> References: <20260515-python-safety-initial-v2-0-6129cadf258a@tromey.com> <20260515-python-safety-initial-v2-3-6129cadf258a@tromey.com> Date: Mon, 18 May 2026 11:00:11 +0100 Message-ID: <874ik5ca1g.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 9uOhnnSYLzZ4Ot3W0C-MPIq-PqBpzegwd5_9zZVKxz8_1779098414 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org Tom Tromey 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 . */ > + > +#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> I think the SFINAE part here is wrong, like in the previous commit. I think gdb::Requires> might be what you mean. > +static inline PyObject * > +to_python (T value) > +{ > + if constexpr (std::is_signed::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 &&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 > +PyObject * > +wrapped_function (Args... args) > +{ > + try > + { > + using result_type = typename std::invoke_result_t; > + > + if constexpr (std::is_void_v) > + { > + 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 > +PyObject * > +wrapped_method (Ret (Class::*meth) (Args...), Class *self, Args... args) > +{ > + try > + { > + if constexpr (std::is_void_v) > + { > + (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 > +PyObject * > +fn_wrapper (PyObject *self, PyObject *args, PyObject *kw) > +{ > + return wrapped_function (gdbpy_borrowed_ref (args), > + gdbpy_opt_borrowed_ref (kw)); > +} > + > +template > +PyObject * > +varargs_wrapper (PyObject *self, PyObject *args, PyObject *kw) > +{ > + return wrapped_method (M, static_cast (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 > +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 (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 > +constexpr PyMethodDef > +varargs_function (std::string_view name, std::string_view doc) > +{ > + using namespace safety_details; > + return { > + name.data (), > + (PyCFunction) fn_wrapper, > + /* 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 > +constexpr PyMethodDef > +varargs_method (std::string_view name, std::string_view doc) > +{ > + using namespace safety_details; > + return { > + name.data (), > + (PyCFunction) varargs_wrapper, > + /* 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 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 (arg)); > +} > + > +/* A function that wraps a "get" method. */ > +template > +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 (arg)); > +} > + > +/* A function that wraps a "set" method. */ > +template > +int > +wrap_setter (PyObject *arg, PyObject *value, void *closure) > +{ > + using namespace safety_details; > + try > + { > + C *self = static_cast (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