From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 8NhuLIoF0WTFRzoAWB0awg (envelope-from ) for ; Mon, 07 Aug 2023 10:54:02 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.a=rsa-sha256 header.s=default header.b=dJIklD6c; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id AF07D1E0BB; Mon, 7 Aug 2023 10:54:02 -0400 (EDT) Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (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 89C111E028 for ; Mon, 7 Aug 2023 10:54:00 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 123FE3858280 for ; Mon, 7 Aug 2023 14:54:00 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 123FE3858280 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1691420040; bh=kJLERGfOhXrFIv257lwZQDVt1eGJlcYp2DtQFHT63D4=; h=To:Cc:Subject:In-Reply-To:References:Date:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=dJIklD6cCGHvavQNWsk4BuniZ/1tjMhyao1XlVjvJ27lcv7STnvesiBtMasJFg2KF Q+9m8rx/RXy14wP9psDu1VXW3I+A6cIdA1NBDiahBx55skRUKq/QySW/to9C00TYBa Q9Olm2DIXJZLJka2tgV63j2ZuGGNVyFbMWkCigGY= Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 9DA263858D28 for ; Mon, 7 Aug 2023 14:53:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9DA263858D28 Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-447-YHXdtqbXNr6PmrQRgSDcuQ-1; Mon, 07 Aug 2023 10:53:33 -0400 X-MC-Unique: YHXdtqbXNr6PmrQRgSDcuQ-1 Received: by mail-wr1-f72.google.com with SMTP id ffacd0b85a97d-3178ddc3d94so2563655f8f.1 for ; Mon, 07 Aug 2023 07:53:33 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1691420012; x=1692024812; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=kJLERGfOhXrFIv257lwZQDVt1eGJlcYp2DtQFHT63D4=; b=IpcR1wQ2GXbhieINuAAgf49ZKbAsmJdw42VQSzizN7N1Cy+v19vvOU5yDjBBtjVvBh hsQDsbDnZtKJCwoDk06KmvpyQFnF3cLfSoQ2UTbmgaMvZx1XihVNOsZ837mRp0Wp6suH 189/+v5QTOdzPVKKKmj+8ZjXJn1/p3ncJG/FlaS8JWB0TZA4PSNbsNr/wq2aNJ5EzKO8 5YYiMB6TSffOW0VEHuDLl1apu0EL2t7ZurSgGSnx9fVWIbx0v+rc2RRrkQaK4I9K7TxK 3i8Z5Uf5mHfIrDUQutrqYvnnkeQQ2D4qHEu0BnjJo/PEUFb6tZc7HZPrgDdaWB2ABzt2 Kmpw== X-Gm-Message-State: AOJu0YyK3HqJthLvuXy8mB1jA3EqEt5ihO1DSMJ8buI1YXmHAXBfOtdI zANYe1FEelrOUElqgDO95LuAqhV6dohsC6UnDWpVv11HzQ4BKu7h3Oy5Hmq8t8usY6ENtZp/CyI SvsSaarsWRlkfqGlbZr+5rxuChIgmx2K598OYQUxqHtJL0pHBmASNd5mw7vO7IEmBGjVql3B5vY Q2+JdnOg== X-Received: by 2002:a5d:4584:0:b0:317:e515:d624 with SMTP id p4-20020a5d4584000000b00317e515d624mr3025018wrq.45.1691420012077; Mon, 07 Aug 2023 07:53:32 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGGy2/96c5eORg42CGS23Ehs1ehVndFUiKSi/h8UOGulsoztdwFieefngN2+2R0Lze3rtLxFQ== X-Received: by 2002:a5d:4584:0:b0:317:e515:d624 with SMTP id p4-20020a5d4584000000b00317e515d624mr3024996wrq.45.1691420011359; Mon, 07 Aug 2023 07:53:31 -0700 (PDT) Received: from localhost ([31.111.84.232]) by smtp.gmail.com with ESMTPSA id m15-20020a056000008f00b0031417b0d338sm10862216wrx.87.2023.08.07.07.53.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 07 Aug 2023 07:53:30 -0700 (PDT) To: Matheus Branco Borella via Gdb-patches , gdb-patches@sourceware.org Cc: "dark.ryu.550@gmail.com" Subject: Re: [PATCH] Add support for creating new types from the Python API In-Reply-To: <20230526033002.172038-1-dark.ryu.550@gmail.com> References: <88e10ffd-8e87-b2bf-e6b3-f4567fb50e17@simark.ca> <20230526033002.172038-1-dark.ryu.550@gmail.com> Date: Mon, 07 Aug 2023 15:53:29 +0100 Message-ID: <87bkfieweu.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Andrew Burgess via Gdb-patches Reply-To: Andrew Burgess Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" Matheus Branco Borella via Gdb-patches writes: > From: "dark.ryu.550@gmail.com" > > On 1/6/23 20:00, simark@simark.ca: >> Unfortunately, I am unable to apply this patch as well, please send it >> using git-send-email. > > Should be all good to go now. I'm sorry for unearthing this patch after it's > been so long, but I hope it's not (too much of) a problem. I've updated the old > patch to work with the way symbol allocation is done now, since it changed from > six months ago, and I've also added a test case for it. > >> It would maybe be nice to be able to create arch-owned types too. For >> instance, you could create types just after firing up GDB, without even >> having an objfile loaded. It's not necessary to implement it at the >> same time, but does your approach leave us the option to do that at a >> later time? > > Hmm, I think it shouldn't be a problem. The way it works now, it already uses > `type_allocator` to do most of the heavy lifting, which can handle both ` > objfile`s and `arch`es. I can see a straightforward way to do that in using > keyword arguments (e.g. `objfile=` and `arch=`) to separate the two cases in > Python and doing a check on the C side for which of the two was used. > > --- > > This patch adds support for creating types from within the Python API. It does > so by exposing the `init_*_type` family of functions, defined in `gdbtypes.h` to > Python and having them return `gdb.Type` objects connected to the newly minted > types. > > These functions are accessible in the root of the gdb module and all require > a reference to a `gdb.Objfile`. Types created from this API are exclusively > objfile-owned. > > This patch also adds an extra type - `gdb.FloatFormat` - to support creation of > floating point types by letting users control the format from within Python. It > is missing, however, a way to specify half formats and validation functions. > > It is important to note that types created using this interface are not > automatically registered as a symbol, and so, types will become unreachable > unless used to create a value that otherwise references it or saved in some way. > > The main drawback of using the `init_*_type` family over implementing type > initialization by hand is that any type that's created gets immediately > allocated on its owner objfile's obstack, regardless of what its real > lifetime requirements are. The main implication of this is that types that > become unreachable will leak their memory for the lifetime of the objfile. I'd soften this from "leak their memory" to "remain live" -- it just feels like claiming there's a leak here is a little too harsh. > > Keeping track of the initialization of the type by hand would require a > deeper change to the existing type object infrastructure. A bit too ambitious > for a first patch, I'd say. > > if it were to be done though, we would gain the ability to only keep in the > obstack types that are known to be referenced in some other way - by allocating > and copying the data to the obstack as other objects are created that reference > it (eg. symbols). > --- > gdb/Makefile.in | 2 + > gdb/python/py-float-format.c | 321 +++++++++++++++++++++ > gdb/python/py-objfile.c | 12 + > gdb/python/py-type-init.c | 409 +++++++++++++++++++++++++++ > gdb/python/python-internal.h | 15 + > gdb/python/python.c | 41 +++ > gdb/testsuite/gdb.python/py-type.exp | 10 + Before this could be merged we would need at least: - Updates to the documentation, - A NEWS entry, - Significantly more tests. I have a few observations, but I think it will be easier to review once there are either some docs and tests that exercise all the parts, as I'll be able to see how everything is intended to work together without having to figure it out from the code. > 7 files changed, 810 insertions(+) > create mode 100644 gdb/python/py-float-format.c > create mode 100644 gdb/python/py-type-init.c > > diff --git a/gdb/Makefile.in b/gdb/Makefile.in > index 14b5dd0bad..108bcea69e 100644 > --- a/gdb/Makefile.in > +++ b/gdb/Makefile.in > @@ -431,6 +431,8 @@ SUBDIR_PYTHON_SRCS = \ > python/py-threadevent.c \ > python/py-tui.c \ > python/py-type.c \ > + python/py-type-init.c \ > + python/py-float-format.c \ > python/py-unwind.c \ > python/py-utils.c \ > python/py-value.c \ > diff --git a/gdb/python/py-float-format.c b/gdb/python/py-float-format.c > new file mode 100644 > index 0000000000..8fe92980f1 > --- /dev/null > +++ b/gdb/python/py-float-format.c > @@ -0,0 +1,321 @@ > +/* Accessibility of float format controls from inside the Python API > + > + Copyright (C) 2008-2023 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 . */ > + > +#include "defs.h" > +#include "python-internal.h" > +#include "floatformat.h" > + > +/* Structure backing the float format Python interface. */ > + > +struct float_format_object > +{ > + PyObject_HEAD > + struct floatformat format; > + > + struct floatformat *float_format () > + { > + return &this->format; > + } > +}; > + > +/* Initializes the float format type and registers it with the Python interpreter. */ Throughout this patch you have some lines that are longer than we like for GDB. Ideally we keep lines under 80 characters. I'm not going to point out every such line. > + > +static int CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION > +gdbpy_initialize_float_format (void) > +{ > + if (PyType_Ready (&float_format_object_type) < 0) > + return -1; > + > + if (gdb_pymodule_addobject (gdb_module, "FloatFormat", > + (PyObject *) &float_format_object_type) < 0) > + return -1; > + > + return 0; > +} > + > +GDBPY_INITIALIZE_FILE (gdbpy_initialize_float_format); > + > +#define INSTANCE_FIELD_GETTER(getter_name, field_name, field_type, field_conv) \ > + static PyObject * \ > + getter_name (PyObject *self, void *closure) \ > + { \ > + float_format_object *ff = (float_format_object*) self; \ > + field_type value = ff->float_format ()->field_name; \ > + return field_conv (value); \ > + } > + > +#define INSTANCE_FIELD_SETTER(getter_name, field_name, field_type, field_conv) \ > + static int \ > + getter_name (PyObject *self, PyObject* value, void *closure) \ Probably setter_name would be better here. Ideally every function and global macro or variable should have a comment. I think these would definitely be improved with a comment. > + { \ > + field_type native_value; \ > + if (!field_conv (value, &native_value)) \ > + return -1; \ > + float_format_object *ff = (float_format_object*) self; \ > + ff->float_format ()->field_name = native_value; \ > + return 0; \ > + } > + > +/* Converts from the intbit enum to a Python boolean. */ > + > +static PyObject * > +intbit_to_py (enum floatformat_intbit intbit) > +{ > + gdb_assert > + (intbit == floatformat_intbit_yes || > + intbit == floatformat_intbit_no); The '||' operator should be placed at the start of the new line. The line wrapping here is a little too aggressive, better would be: gdb_assert (intbit == floatformat_intbit_yes || intbit == floatformat_intbit_no); > + > + if (intbit == floatformat_intbit_no) > + Py_RETURN_FALSE; > + else > + Py_RETURN_TRUE; > +} > + > +/* Converts from a Python boolean to the intbit enum. */ > + > +static bool > +py_to_intbit (PyObject *object, enum floatformat_intbit *intbit) > +{ > + if (!PyObject_IsInstance (object, (PyObject*) &PyBool_Type)) > + { > + PyErr_SetString (PyExc_TypeError, "intbit must be True or False"); > + return false; > + } > + > + *intbit = PyObject_IsTrue (object) ? > + floatformat_intbit_yes : floatformat_intbit_no; Operator at the start of a line again, and use () to encourage alignment, so: *intbit = (PyObject_IsTrue (object) ? floatformat_intbit_yes : floatformat_intbit_no); > + return true; > +} > + > +/* Converts from a Python integer to a unsigned integer. */ > + > +static bool > +py_to_unsigned_int (PyObject *object, unsigned int *val) > +{ > + if (!PyObject_IsInstance (object, (PyObject*) &PyLong_Type)) > + { > + PyErr_SetString (PyExc_TypeError, "value must be an integer"); > + return false; > + } > + > + long native_val = PyLong_AsLong (object); > + if (native_val > (long) UINT_MAX) > + { > + PyErr_SetString (PyExc_ValueError, "value is too large"); > + return false; > + } > + if (native_val < 0) > + { > + PyErr_SetString (PyExc_ValueError, > + "value must not be smaller than zero"); > + return false; > + } > + > + *val = (unsigned int) native_val; > + return true; > +} > + > +/* Converts from a Python integer to a signed integer. */ > + > +static bool > +py_to_int(PyObject *object, int *val) > +{ > + if(!PyObject_IsInstance(object, (PyObject*)&PyLong_Type)) > + { > + PyErr_SetString(PyExc_TypeError, "value must be an integer"); > + return false; > + } > + > + long native_val = PyLong_AsLong(object); > + if(native_val > (long)INT_MAX) > + { > + PyErr_SetString(PyExc_ValueError, "value is too large"); > + return false; > + } > + > + *val = (int)native_val; > + return true; > +} > + > +INSTANCE_FIELD_GETTER (ffpy_get_totalsize, totalsize, > + unsigned int, PyLong_FromLong) > +INSTANCE_FIELD_GETTER (ffpy_get_sign_start, sign_start, > + unsigned int, PyLong_FromLong) > +INSTANCE_FIELD_GETTER (ffpy_get_exp_start, exp_start, > + unsigned int, PyLong_FromLong) > +INSTANCE_FIELD_GETTER (ffpy_get_exp_len, exp_len, > + unsigned int, PyLong_FromLong) > +INSTANCE_FIELD_GETTER (ffpy_get_exp_bias, exp_bias, int, PyLong_FromLong) > +INSTANCE_FIELD_GETTER (ffpy_get_exp_nan, exp_nan, > + unsigned int, PyLong_FromLong) > +INSTANCE_FIELD_GETTER (ffpy_get_man_start, man_start, > + unsigned int, PyLong_FromLong) > +INSTANCE_FIELD_GETTER (ffpy_get_man_len, man_len, > + unsigned int, PyLong_FromLong) > +INSTANCE_FIELD_GETTER (ffpy_get_intbit, intbit, > + enum floatformat_intbit, intbit_to_py) > +INSTANCE_FIELD_GETTER (ffpy_get_name, name, > + const char *, PyUnicode_FromString) > + > +INSTANCE_FIELD_SETTER (ffpy_set_totalsize, totalsize, > + unsigned int, py_to_unsigned_int) > +INSTANCE_FIELD_SETTER (ffpy_set_sign_start, sign_start, > + unsigned int, py_to_unsigned_int) > +INSTANCE_FIELD_SETTER (ffpy_set_exp_start, exp_start, > + unsigned int, py_to_unsigned_int) > +INSTANCE_FIELD_SETTER (ffpy_set_exp_len, exp_len, > + unsigned int, py_to_unsigned_int) > +INSTANCE_FIELD_SETTER (ffpy_set_exp_bias, exp_bias, int, py_to_int) > +INSTANCE_FIELD_SETTER (ffpy_set_exp_nan, exp_nan, > + unsigned int, py_to_unsigned_int) > +INSTANCE_FIELD_SETTER (ffpy_set_man_start, man_start, > + unsigned int, py_to_unsigned_int) > +INSTANCE_FIELD_SETTER (ffpy_set_man_len, man_len, > + unsigned int, py_to_unsigned_int) > +INSTANCE_FIELD_SETTER (ffpy_set_intbit, intbit, > + enum floatformat_intbit, py_to_intbit) > + > +/* Makes sure float formats created from Python always test as valid. */ > + > +static int > +ffpy_always_valid (const struct floatformat *fmt ATTRIBUTE_UNUSED, > + const void *from ATTRIBUTE_UNUSED) > +{ > + return 1; > +} > + > +/* Initializes new float format objects. */ > + > +static int > +ffpy_init (PyObject *self, > + PyObject *args ATTRIBUTE_UNUSED, > + PyObject *kwds ATTRIBUTE_UNUSED) > +{ > + auto ff = (float_format_object*) self; > + ff->format = floatformat (); > + ff->float_format ()->name = ""; > + ff->float_format ()->is_valid = ffpy_always_valid; > + return 0; > +} > + > +/* Retrieves a pointer to the underlying float format structure. */ Comments for extern functions should be placed in the header file, and a comment here should just say: /* See python/python-internal.h. */ I know there are lots of counter examples to this practice in python-internal.h, but they are all older code. Newer code is expected to follow the above style -- and there are also examples of this in that header too. > + > +struct floatformat * > +float_format_object_as_float_format (PyObject *self) > +{ > + if (!PyObject_IsInstance (self, (PyObject*) &float_format_object_type)) > + return nullptr; I'm not sure this is right. I believe PyObject_IsInstance can return 1 (if it is an instance), 0 (if it is not an instance), or -1 (on error). So: if (PyObject_IsInstance (self, (PyObject*) &float_format_object_type) <= 0) return nullptr; Might be better. Except, an exception is only set for the -1 case, and looking at the user of float_format_object_as_float_format, I think there is an expectation that an exception will have been set, so you might want to either always set an exception here, or only set an exception for the 0 case? Either way, this is exactly the sort of thing that the comment should be saying, e.g. /* Retrieves a pointer to the float format structure represented by SELF, assuming SELF is of type gdb.XXXX. If SELF is not of the correct type then return nullptr and set an exception. */ Actually, looking at some of the other code, I wonder if the right thing here is to switch to PyObject_TypeCheck? This avoids (possibly) calling back into Python code, and only returns 0/1. We seem to use _TypeCheck extensively in other places, is there a requirement for _IsInstance? > + return ((float_format_object*) self)->float_format (); > +} > + > +static gdb_PyGetSetDef float_format_object_getset[] = > +{ > + { "totalsize", ffpy_get_totalsize, ffpy_set_totalsize, > + "The total size of the floating point number, in bits.", nullptr }, > + { "sign_start", ffpy_get_sign_start, ffpy_set_sign_start, > + "The bit offset of the sign bit.", nullptr }, > + { "exp_start", ffpy_get_exp_start, ffpy_set_exp_start, > + "The bit offset of the start of the exponent.", nullptr }, > + { "exp_len", ffpy_get_exp_len, ffpy_set_exp_len, > + "The size of the exponent, in bits.", nullptr }, > + { "exp_bias", ffpy_get_exp_bias, ffpy_set_exp_bias, > + "Bias added to a \"true\" exponent to form the biased exponent.", nullptr }, > + { "exp_nan", ffpy_get_exp_nan, ffpy_set_exp_nan, > + "Exponent value which indicates NaN.", nullptr }, > + { "man_start", ffpy_get_man_start, ffpy_set_man_start, > + "The bit offset of the start of the mantissa.", nullptr }, > + { "man_len", ffpy_get_man_len, ffpy_set_man_len, > + "The size of the mantissa, in bits.", nullptr }, > + { "intbit", ffpy_get_intbit, ffpy_set_intbit, > + "Is the integer bit explicit or implicit?", nullptr }, > + { "name", ffpy_get_name, nullptr, > + "Internal name for debugging.", nullptr }, > + { nullptr } > +}; > + > +static PyMethodDef float_format_object_methods[] = > +{ > + { NULL } > +}; > + > +static PyNumberMethods float_format_object_as_number = { > + nullptr, /* nb_add */ > + nullptr, /* nb_subtract */ > + nullptr, /* nb_multiply */ > + nullptr, /* nb_remainder */ > + nullptr, /* nb_divmod */ > + nullptr, /* nb_power */ > + nullptr, /* nb_negative */ > + nullptr, /* nb_positive */ > + nullptr, /* nb_absolute */ > + nullptr, /* nb_nonzero */ > + nullptr, /* nb_invert */ > + nullptr, /* nb_lshift */ > + nullptr, /* nb_rshift */ > + nullptr, /* nb_and */ > + nullptr, /* nb_xor */ > + nullptr, /* nb_or */ > + nullptr, /* nb_int */ > + nullptr, /* reserved */ > + nullptr, /* nb_float */ > +}; I haven't dug into the implications of providing this structure with all the fields set to nullptr vs just providing nullptr for the tp_as_number field below. However, given this is not common in the GDB code, I think, if there is a reason for doing this, that it would be worth explaining in a comment. Similarly, you have an empty float_format_object_methods list, in other places we just set tp_methods to nullptr -- so is there a reason for the approach you've taken here? > + > +PyTypeObject float_format_object_type = > +{ > + PyVarObject_HEAD_INIT (NULL, 0) > + "gdb.FloatFormat", /*tp_name*/ > + sizeof (float_format_object), /*tp_basicsize*/ > + 0, /*tp_itemsize*/ > + nullptr, /*tp_dealloc*/ > + 0, /*tp_print*/ > + nullptr, /*tp_getattr*/ > + nullptr, /*tp_setattr*/ > + nullptr, /*tp_compare*/ > + nullptr, /*tp_repr*/ > + &float_format_object_as_number, /*tp_as_number*/ > + nullptr, /*tp_as_sequence*/ > + nullptr, /*tp_as_mapping*/ > + nullptr, /*tp_hash */ > + nullptr, /*tp_call*/ > + nullptr, /*tp_str*/ > + nullptr, /*tp_getattro*/ > + nullptr, /*tp_setattro*/ > + nullptr, /*tp_as_buffer*/ > + Py_TPFLAGS_DEFAULT, /*tp_flags*/ > + "GDB float format object", /* tp_doc */ > + nullptr, /* tp_traverse */ > + nullptr, /* tp_clear */ > + nullptr, /* tp_richcompare */ > + 0, /* tp_weaklistoffset */ > + nullptr, /* tp_iter */ > + nullptr, /* tp_iternext */ > + float_format_object_methods, /* tp_methods */ > + nullptr, /* tp_members */ > + float_format_object_getset, /* tp_getset */ > + nullptr, /* tp_base */ > + nullptr, /* tp_dict */ > + nullptr, /* tp_descr_get */ > + nullptr, /* tp_descr_set */ > + 0, /* tp_dictoffset */ > + ffpy_init, /* tp_init */ > + nullptr, /* tp_alloc */ > + PyType_GenericNew, /* tp_new */ > +}; > + > + > diff --git a/gdb/python/py-objfile.c b/gdb/python/py-objfile.c > index ad72f3f042..be2121c405 100644 > --- a/gdb/python/py-objfile.c > +++ b/gdb/python/py-objfile.c > @@ -704,6 +704,18 @@ objfile_to_objfile_object (struct objfile *objfile) > return gdbpy_ref<>::new_reference (result); > } > > +struct objfile * > +objfile_object_to_objfile (PyObject *self) > +{ > + if (!PyObject_TypeCheck (self, &objfile_object_type)) > + return nullptr; > + > + auto objfile_object = (struct objfile_object*) self; > + OBJFPY_REQUIRE_VALID (objfile_object); > + > + return objfile_object->objfile; > +} > + > static int CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION > gdbpy_initialize_objfile (void) > { > diff --git a/gdb/python/py-type-init.c b/gdb/python/py-type-init.c > new file mode 100644 > index 0000000000..a18cce6e51 > --- /dev/null > +++ b/gdb/python/py-type-init.c > @@ -0,0 +1,409 @@ > +/* Functionality for creating new types accessible from python. > + > + Copyright (C) 2008-2023 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 . */ > + > +#include "defs.h" > +#include "python-internal.h" > +#include "gdbtypes.h" > +#include "floatformat.h" > +#include "objfiles.h" > +#include "gdbsupport/gdb_obstack.h" > + > + > +/* Copies a null-terminated string into an objfile's obstack. */ > + > +static const char * > +copy_string (struct objfile *objfile, const char *py_str) > +{ > + unsigned int len = strlen (py_str); > + return obstack_strndup (&objfile->per_bfd->storage_obstack, > + py_str, len); > +} > + > +/* Creates a new type and returns a new gdb.Type associated with it. */ > + > +PyObject * > +gdbpy_init_type (PyObject *self, PyObject *args) > +{ > + PyObject *objfile_object; > + enum type_code code; > + int bit_length; > + const char *py_name; > + > + if(!PyArg_ParseTuple (args, "Oiis", &objfile_object, &code, > + &bit_length, &py_name)) > + return nullptr; I'm a huge fan of named arguments, and would like to see all of these converted to use named arguments. With an eye to Simon's suggestion, I would be tempted to name the first argument `owner` maybe? We could then use PyObject_TypeCheck to decide if the owner is a gdb.Objfile or a gdb.Architecture. I agree that supporting gdb.Architecture isn't a requirement for getting the patch merged, but I don't want to get stuck with an API that doesn't quite work, so if you were willing to give that a go, that would be great. > + > + struct objfile* objfile = objfile_object_to_objfile (objfile_object); > + if (objfile == nullptr) > + return nullptr; > + > + const char *name = copy_string (objfile, py_name); > + struct type *type; > + try > + { > + type_allocator allocator (objfile); > + type = allocator.new_type (code, bit_length, name); > + gdb_assert (type != nullptr); > + } > + catch (gdb_exception_error& ex) > + { > + GDB_PY_HANDLE_EXCEPTION (ex); > + } > + > + return type_to_type_object (type); > +} > + > +/* Creates a new integer type and returns a new gdb.Type associated with it. */ > + > +PyObject * > +gdbpy_init_integer_type (PyObject *self, PyObject *args) > +{ > + PyObject *objfile_object; > + int bit_size; > + int unsigned_p; > + const char *py_name; > + > + if (!PyArg_ParseTuple (args, "Oips", &objfile_object, &bit_size, > + &unsigned_p, &py_name)) > + return nullptr; > + > + struct objfile *objfile = objfile_object_to_objfile (objfile_object); > + if (objfile == nullptr) > + return nullptr; > + > + const char *name = copy_string (objfile, py_name); > + struct type *type; > + try > + { > + type_allocator allocator (objfile); > + type = init_integer_type (allocator, bit_size, unsigned_p, name); > + gdb_assert (type != nullptr); > + } > + catch (gdb_exception_error& ex) > + { > + GDB_PY_HANDLE_EXCEPTION (ex); > + } > + > + return type_to_type_object(type); > +} > + > +/* Creates a new character type and returns a new gdb.Type associated > + * with it. */ > + > +PyObject * > +gdbpy_init_character_type (PyObject *self, PyObject *args) > +{ > + > + PyObject *objfile_object; > + int bit_size; > + int unsigned_p; > + const char *py_name; > + > + if (!PyArg_ParseTuple (args, "Oips", &objfile_object, &bit_size, > + &unsigned_p, &py_name)) > + return nullptr; > + > + struct objfile *objfile = objfile_object_to_objfile (objfile_object); > + if (objfile == nullptr) > + return nullptr; > + > + const char *name = copy_string (objfile, py_name); > + struct type *type; > + try > + { > + type_allocator allocator (objfile); > + type = init_character_type (allocator, bit_size, unsigned_p, name); > + gdb_assert (type != nullptr); > + } > + catch (gdb_exception_error& ex) > + { > + GDB_PY_HANDLE_EXCEPTION (ex); > + } > + > + return type_to_type_object (type); > +} > + > +/* Creates a new boolean type and returns a new gdb.Type associated with it. */ > + > +PyObject * > +gdbpy_init_boolean_type (PyObject *self, PyObject *args) > +{ > + > + PyObject *objfile_object; > + int bit_size; > + int unsigned_p; > + const char *py_name; > + > + if (!PyArg_ParseTuple (args, "Oips", &objfile_object, &bit_size, > + &unsigned_p, &py_name)) > + return nullptr; > + > + struct objfile *objfile = objfile_object_to_objfile (objfile_object); > + if (objfile == nullptr) > + return nullptr; > + > + const char *name = copy_string (objfile, py_name); > + struct type *type; > + try > + { > + type_allocator allocator (objfile); > + type = init_boolean_type (allocator, bit_size, unsigned_p, name); > + gdb_assert (type != nullptr); > + } > + catch (gdb_exception_error& ex) > + { > + GDB_PY_HANDLE_EXCEPTION (ex); > + } > + > + return type_to_type_object (type); > +} > + > +/* Creates a new float type and returns a new gdb.Type associated with it. */ > + > +PyObject * > +gdbpy_init_float_type (PyObject *self, PyObject *args) > +{ > + PyObject *objfile_object, *float_format_object; > + const char *py_name; > + > + if (!PyArg_ParseTuple (args, "OOs", &objfile_object, > + &float_format_object, &py_name)) > + return nullptr; > + > + struct objfile *objfile = objfile_object_to_objfile (objfile_object); > + if (objfile == nullptr) > + return nullptr; > + > + struct floatformat *local_ff = float_format_object_as_float_format > + (float_format_object); > + if (local_ff == nullptr) > + return nullptr; > + > + /* Persist a copy of the format in the objfile's obstack. This guarantees that > + * the format won't outlive the type being created from it and that changes > + * made to the object used to create this type will not affect it after > + * creation. */ > + auto ff = OBSTACK_CALLOC > + (&objfile->objfile_obstack, > + 1, > + struct floatformat); > + memcpy (ff, local_ff, sizeof (struct floatformat)); > + > + /* We only support creating float types in the architecture's endianness, so > + * make sure init_float_type sees the float format structure we need it to. */ > + enum bfd_endian endianness = gdbarch_byte_order (objfile->arch()); > + gdb_assert (endianness < BFD_ENDIAN_UNKNOWN); > + > + const struct floatformat *per_endian[2] = { nullptr, nullptr }; > + per_endian[endianness] = ff; > + > + const char *name = copy_string (objfile, py_name); > + struct type *type; > + try > + { > + type_allocator allocator (objfile); > + type = init_float_type (allocator, -1, name, per_endian, endianness); > + gdb_assert (type != nullptr); > + } > + catch (gdb_exception_error& ex) > + { > + GDB_PY_HANDLE_EXCEPTION (ex); > + } > + > + return type_to_type_object (type); > +} > + > +/* Creates a new decimal float type and returns a new gdb.Type > + * associated with it. */ > + > +PyObject * > +gdbpy_init_decfloat_type (PyObject *self, PyObject *args) > +{ > + PyObject *objfile_object; > + int bit_length; > + const char *py_name; > + > + if (!PyArg_ParseTuple (args, "Ois", &objfile_object, &bit_length, &py_name)) > + return nullptr; > + > + struct objfile *objfile = objfile_object_to_objfile (objfile_object); > + if (objfile == nullptr) > + return nullptr; > + > + const char *name = copy_string (objfile, py_name); > + struct type *type; > + try > + { > + type_allocator allocator (objfile); > + type = init_decfloat_type (allocator, bit_length, name); > + gdb_assert (type != nullptr); > + } > + catch (gdb_exception_error& ex) > + { > + GDB_PY_HANDLE_EXCEPTION (ex); > + } > + > + return type_to_type_object (type); > +} > + > +/* Returns whether a given type can be used to create a complex type. */ > + > +PyObject * > +gdbpy_can_create_complex_type (PyObject *self, PyObject *args) > +{ > + > + PyObject *type_object; > + > + if (!PyArg_ParseTuple (args, "O", &type_object)) > + return nullptr; > + > + struct type *type = type_object_to_type (type_object); > + if (type == nullptr) > + return nullptr; > + > + bool can_create_complex = false; > + try > + { > + can_create_complex = can_create_complex_type (type); > + } > + catch (gdb_exception_error& ex) > + { > + GDB_PY_HANDLE_EXCEPTION (ex); > + } > + > + if (can_create_complex) > + Py_RETURN_TRUE; > + else > + Py_RETURN_FALSE; > +} > + > +/* Creates a new complex type and returns a new gdb.Type associated with it. */ > + > +PyObject * > +gdbpy_init_complex_type (PyObject *self, PyObject *args) > +{ > + > + PyObject *type_object; > + const char *py_name; > + > + if (!PyArg_ParseTuple (args, "Os", &type_object, &py_name)) > + return nullptr; > + > + struct type *type = type_object_to_type (type_object); > + if (type == nullptr) > + return nullptr; > + > + obstack *obstack; > + if (type->is_objfile_owned ()) > + obstack = &type->objfile_owner ()->objfile_obstack; > + else > + obstack = gdbarch_obstack (type->arch_owner ()); > + > + unsigned int len = strlen (py_name); > + const char *name = obstack_strndup (obstack, > + py_name, > + len); > + struct type *complex_type; > + try > + { > + complex_type = init_complex_type (name, type); > + gdb_assert (complex_type != nullptr); > + } > + catch (gdb_exception_error& ex) > + { > + GDB_PY_HANDLE_EXCEPTION (ex); > + } > + > + return type_to_type_object (complex_type); > +} > + > +/* Creates a new pointer type and returns a new gdb.Type associated with it. */ > + > +PyObject * > +gdbpy_init_pointer_type (PyObject *self, PyObject *args) > +{ I guess I was a little surprised to see this one. I'd sort-of expected that to get a pointer type you'd create a new type and then do: pointer_type = gdb.init_*_type(....).pointer() I guess this code below does allow for different pointer sizes ... and there's even a FIXME comment in gdbtypes.c pointing out that GDB only supports a single pointer size, so maybe this is working towards closing that issue. But I'm not sure how these pointers would be used ... maybe some tests will give examples of how this is different to calling gdb.Type.pointer() then I'll understand... > + PyObject *objfile_object, *type_object; > + int bit_length; > + const char *py_name; > + > + if (!PyArg_ParseTuple (args, "OOis", &objfile_object, &type_object, > + &bit_length, &py_name)) > + return nullptr; > + > + struct objfile *objfile = objfile_object_to_objfile (objfile_object); > + if (objfile == nullptr) > + return nullptr; > + > + struct type *type = type_object_to_type (type_object); > + if (type == nullptr) > + return nullptr; > + > + const char *name = copy_string (objfile, py_name); > + struct type *pointer_type = nullptr; > + try > + { > + type_allocator allocator (objfile); > + pointer_type = init_pointer_type (allocator, bit_length, > + name, type); > + gdb_assert (type != nullptr); > + } > + catch (gdb_exception_error& ex) > + { > + GDB_PY_HANDLE_EXCEPTION (ex); > + } > + > + return type_to_type_object (pointer_type); > +} > + > +/* Creates a new fixed point type and returns a new gdb.Type associated > + * with it. */ > + > +PyObject * > +gdbpy_init_fixed_point_type (PyObject *self, PyObject *args) > +{ > + > + PyObject *objfile_object; > + int bit_length; > + int unsigned_p; > + const char* py_name; > + > + if (!PyArg_ParseTuple (args, "Oips", &objfile_object, &bit_length, > + &unsigned_p, &py_name)) > + return nullptr; > + > + struct objfile *objfile = objfile_object_to_objfile (objfile_object); > + if (objfile == nullptr) > + return nullptr; > + > + const char *name = copy_string (objfile, py_name); > + struct type *type; > + try > + { > + type = init_fixed_point_type (objfile, bit_length, unsigned_p, > + name); > + gdb_assert (type != nullptr); > + } > + catch (gdb_exception_error& ex) > + { > + GDB_PY_HANDLE_EXCEPTION (ex); > + } > + > + return type_to_type_object (type); > +} > + > diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h > index dbd33570a7..73e2e6ce62 100644 > --- a/gdb/python/python-internal.h > +++ b/gdb/python/python-internal.h > @@ -289,6 +289,8 @@ extern PyTypeObject frame_object_type > CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("frame_object"); > extern PyTypeObject thread_object_type > CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("thread_object"); > +extern PyTypeObject float_format_object_type > + CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("float_format"); > > /* Ensure that breakpoint_object_type is initialized and return true. If > breakpoint_object_type can't be initialized then set a suitable Python > @@ -431,6 +433,17 @@ gdb::unique_xmalloc_ptr gdbpy_parse_command_name > PyObject *gdbpy_register_tui_window (PyObject *self, PyObject *args, > PyObject *kw); > > +PyObject *gdbpy_init_type (PyObject *self, PyObject *args); > +PyObject *gdbpy_init_integer_type (PyObject *self, PyObject *args); > +PyObject *gdbpy_init_character_type (PyObject *self, PyObject *args); > +PyObject *gdbpy_init_boolean_type (PyObject *self, PyObject *args); > +PyObject *gdbpy_init_float_type (PyObject *self, PyObject *args); > +PyObject *gdbpy_init_decfloat_type (PyObject *self, PyObject *args); > +PyObject *gdbpy_can_create_complex_type (PyObject *self, PyObject *args); > +PyObject *gdbpy_init_complex_type (PyObject *self, PyObject *args); > +PyObject *gdbpy_init_pointer_type (PyObject *self, PyObject *args); > +PyObject *gdbpy_init_fixed_point_type (PyObject *self, PyObject *args); > + > PyObject *symtab_and_line_to_sal_object (struct symtab_and_line sal); > PyObject *symtab_to_symtab_object (struct symtab *symtab); > PyObject *symbol_to_symbol_object (struct symbol *sym); > @@ -480,6 +493,8 @@ struct symtab *symtab_object_to_symtab (PyObject *obj); > struct symtab_and_line *sal_object_to_symtab_and_line (PyObject *obj); > frame_info_ptr frame_object_to_frame_info (PyObject *frame_obj); > struct gdbarch *arch_object_to_gdbarch (PyObject *obj); > +struct objfile *objfile_object_to_objfile (PyObject *self); > +struct floatformat *float_format_object_as_float_format (PyObject *self); These two should be marked 'extern' and (as I said earlier) should have a their comments here. > > /* Convert Python object OBJ to a program_space pointer. OBJ must be a > gdb.Progspace reference. Return nullptr if the gdb.Progspace is not > diff --git a/gdb/python/python.c b/gdb/python/python.c > index fd5a920cbd..288c8b355c 100644 > --- a/gdb/python/python.c > +++ b/gdb/python/python.c > @@ -2521,6 +2521,47 @@ Return current recording object." }, > "stop_recording () -> None.\n\ > Stop current recording." }, > > + /* Type initialization functions. */ > + { "init_type", gdbpy_init_type, METH_VARARGS, > + "init_type (objfile, type_code, bit_length, name) -> type\n\ > + Creates a new type with the given bit length and type code, owned\ > + by the given objfile." }, > + { "init_integer_type", gdbpy_init_integer_type, METH_VARARGS, > + "init_integer_type (objfile, bit_length, unsigned, name) -> type\n\ > + Creates a new integer type with the given bit length and \ > + signedness, owned by the given objfile." }, > + { "init_character_type", gdbpy_init_character_type, METH_VARARGS, > + "init_character_type (objfile, bit_length, unsigned, name) -> type\n\ > + Creates a new character type with the given bit length and \ > + signedness, owned by the given objfile." }, > + { "init_boolean_type", gdbpy_init_boolean_type, METH_VARARGS, > + "init_boolean_type (objfile, bit_length, unsigned, name) -> type\n\ > + Creates a new boolean type with the given bit length and \ > + signedness, owned by the given objfile." }, > + { "init_float_type", gdbpy_init_float_type, METH_VARARGS, > + "init_float_type (objfile, float_format, name) -> type\n\ > + Creates a new floating point type with the given bit length and \ > + format, owned by the given objfile." }, > + { "init_decfloat_type", gdbpy_init_decfloat_type, METH_VARARGS, > + "init_decfloat_type (objfile, bit_length, name) -> type\n\ > + Creates a new decimal float type with the given bit length,\ > + owned by the given objfile." }, > + { "can_create_complex_type", gdbpy_can_create_complex_type, METH_VARARGS, > + "can_create_complex_type (type) -> bool\n\ > + Returns whether a given type can form a new complex type." }, > + { "init_complex_type", gdbpy_init_complex_type, METH_VARARGS, > + "init_complex_type (base_type, name) -> type\n\ > + Creates a new complex type whose components belong to the\ > + given type, owned by the given objfile." }, > + { "init_pointer_type", gdbpy_init_pointer_type, METH_VARARGS, > + "init_pointer_type (objfile, target_type, bit_length, name) -> type\n\ > + Creates a new pointer type with the given bit length, pointing\ > + to the given target type, and owned by the given objfile." }, > + { "init_fixed_point_type", gdbpy_init_fixed_point_type, METH_VARARGS, > + "init_fixed_point_type (objfile, bit_length, unsigned, name) -> type\n\ > + Creates a new fixed point type with the given bit length and\ > + signedness, owned by the given objfile." }, > + > { "lookup_type", (PyCFunction) gdbpy_lookup_type, > METH_VARARGS | METH_KEYWORDS, > "lookup_type (name [, block]) -> type\n\ > diff --git a/gdb/testsuite/gdb.python/py-type.exp b/gdb/testsuite/gdb.python/py-type.exp > index c245d41a1a..aee2b4d60a 100644 > --- a/gdb/testsuite/gdb.python/py-type.exp > +++ b/gdb/testsuite/gdb.python/py-type.exp > @@ -388,3 +388,13 @@ if { [build_inferior "${binfile}-cxx" "c++"] == 0 } { > test_type_equality > } > } > + > +# Test python type construction > +gdb_test "python t = gdb.init_type(gdb.objfiles ()\[0\], gdb.TYPE_CODE_INT, 24, 'long short int')" \ > + "" "construct a new type from inside python" > +gdb_test "python print (t.code)" \ > + "8" "check the code for the python-constructed type" Rather than including '8' in here, I wonder if: gdb_test "python print (t.code == gdb.TYPE_CODE_INT)" \ "True" "check the code for the python-constructed type" would be better? Thanks, Andrew > +gdb_test "python print (t.sizeof)" \ > + "3" "check the size for the python-constructed type" > +gdb_test "python print (t.name)" \ > + "long short int" "check the name for the python-constructed type" > -- > 2.40.1