Re: [PATCH 13/18] -Wwrite-strings: Wrap PyGetSetDef for construction with string literals

Sergio Durigan Junior
2017-04-05T15:48:50Z

On Wednesday, April 05 2017, Pedro Alves wrote:

> On 04/04/2017 07:39 PM, Sergio Durigan Junior wrote:
>
>> I liked the approach. As with the other Python patch, I'm more inclined
>> to use an explicit "gdb_PyGetSetDef" everywhere, just to be clear that
>> we are using our own version of it. This can save time when debugging.
>
> OK, here's what that looks like. Commit log updated to reflect the
> change too. WDYT?

LGTM. Thanks!

> From 566fd938f2919985db6a955d769ba619534964e8 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Wed, 5 Apr 2017 13:22:25 +0100
> Subject: [PATCH] -Wwrite-strings: Wrap PyGetSetDef for construction with
> string literals
>
> Unfortunately, PyGetSetDef's 'name' and 'doc' members are 'char *'
> instead of 'const char *', meaning that in order to list-initialize
> PyGetSetDef arrays using string literals requires writing explicit
> 'char *' casts. For example:
>
> static PyGetSetDef value_object_getset[] = {
> - { "address", valpy_get_address, NULL, "The address of the value.",
> + { (char *) "address", valpy_get_address, NULL,
> + (char *) "The address of the value.",
> NULL },
> - { "is_optimized_out", valpy_get_is_optimized_out, NULL,
> - "Boolean telling whether the value is optimized "
> + { (char *) "is_optimized_out", valpy_get_is_optimized_out, NULL,
> + (char *) "Boolean telling whether the value is optimized "
> "out (i.e., not available).",
> NULL },
> - { "type", valpy_get_type, NULL, "Type of the value.", NULL },
> - { "dynamic_type", valpy_get_dynamic_type, NULL,
> - "Dynamic type of the value.", NULL },
> - { "is_lazy", valpy_get_is_lazy, NULL,
> - "Boolean telling whether the value is lazy (not fetched yet\n\
> + { (char *) "type", valpy_get_type, NULL,
> + (char *) "Type of the value.", NULL },
> + { (char *) "dynamic_type", valpy_get_dynamic_type, NULL,
> + (char *) "Dynamic type of the value.", NULL },
> + { (char *) "is_lazy", valpy_get_is_lazy, NULL,
> + (char *) "Boolean telling whether the value is lazy (not fetched yet\n\
> from the inferior). A lazy value is fetched when needed, or when\n\
> the \"fetch_lazy()\" method is called.", NULL },
> {NULL} /* Sentinel */
>
> We have ~20 such arrays, and I first wrote a patch that fixed all of
> them like that... It's not pretty...
>
> One way to make these a bit less ugly would be add a new macro that
> hides the casts, like:
>
> #define GDBPY_GSDEF(NAME, GET, SET, DOC, CLOSURE) \
> { (char *) NAME, GET, SET, (char *) DOC, CLOSURE }
>
> and then use it like:
>
> static PyGetSetDef value_object_getset[] = {
> GDBPY_GSDEF ("address", valpy_get_address, NULL,
> "The address of the value.", NULL),
> GDBPY_GSDEF ("is_optimized_out", valpy_get_is_optimized_out, NULL,
> "Boolean telling whether the value is optimized ", NULL),
> {NULL} /* Sentinel */
> };
>
> But since we have C++11, which gives us constexpr and list
> initialization, I thought of a way that requires no changes where the
> arrays are initialized:
>
> We add a new type that extends PyGetSetDef (called gdb_PyGetSetDef),
> and add constexpr constructors that accept const 'name' and 'doc', and
> then list/aggregate initialization simply "calls" these matching
> constructors instead.
>
> I put "calls" in quotes, because given "constexpr", it's all done at
> compile time, and there's no overhead either in binary size or at run
> time. In fact, we get identical binaries, before/after this change.
>
> Unlike the fixes that fix some old Python API to match the API of more
> recent Python, this switches to using explicit "gdb_PyGetSetDef"
> everywhere, just to be clear that we are using our own version of it.
>
> gdb/ChangeLog:
> yyyy-mm-dd Pedro Alves <palves@redhat.com>
>
> * python/python-internal.h (gdb_PyGetSetDef): New type.
> * python/py-block.c (block_object_getset)
> (breakpoint_object_getset): Now a gdb_PyGetSetDef array.
> * python/py-event.c (event_object_getset)
> (finish_breakpoint_object_getset): Likewise.
> * python/py-inferior.c (inferior_object_getset): Likewise.
> * python/py-infthread.c (thread_object_getset): Likewise.
> * python/py-lazy-string.c (lazy_string_object_getset): Likewise.
> * python/py-linetable.c (linetable_entry_object_getset): Likewise.
> * python/py-objfile.c (objfile_getset): Likewise.
> * python/py-progspace.c (pspace_getset): Likewise.
> * python/py-record-btrace.c (btpy_insn_getset, btpy_call_getset):
> Likewise.
> * python/py-record.c (recpy_record_getset): Likewise.
> * python/py-symbol.c (symbol_object_getset): Likewise.
> * python/py-symtab.c (symtab_object_getset, sal_object_getset):
> Likewise.
> * python/py-type.c (type_object_getset, field_object_getset):
> Likewise.
> * python/py-value.c (value_object_getset): Likewise.
> ---
> gdb/python/py-block.c | 2 +-
> gdb/python/py-breakpoint.c | 2 +-
> gdb/python/py-event.c | 2 +-
> gdb/python/py-finishbreakpoint.c | 2 +-
> gdb/python/py-inferior.c | 2 +-
> gdb/python/py-infthread.c | 2 +-
> gdb/python/py-lazy-string.c | 2 +-
> gdb/python/py-linetable.c | 2 +-
> gdb/python/py-objfile.c | 2 +-
> gdb/python/py-progspace.c | 2 +-
> gdb/python/py-record-btrace.c | 4 ++--
> gdb/python/py-record.c | 2 +-
> gdb/python/py-symbol.c | 2 +-
> gdb/python/py-symtab.c | 4 ++--
> gdb/python/py-type.c | 4 ++--
> gdb/python/py-value.c | 2 +-
> gdb/python/python-internal.h | 30 ++++++++++++++++++++++++++++++
> 17 files changed, 49 insertions(+), 19 deletions(-)
>
> diff --git a/gdb/python/py-block.c b/gdb/python/py-block.c
> index f477d4a..840c842 100644
> --- a/gdb/python/py-block.c
> +++ b/gdb/python/py-block.c
> @@ -461,7 +461,7 @@ Return true if this block is valid, false if not." },
> {NULL} /* Sentinel */
> };
>
> -static PyGetSetDef block_object_getset[] = {
> +static gdb_PyGetSetDef block_object_getset[] = {
> { "start", blpy_get_start, NULL, "Start address of the block.", NULL },
> { "end", blpy_get_end, NULL, "End address of the block.", NULL },
> { "function", blpy_get_function, NULL,
> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
> index 724a7ed..34f46fb 100644
> --- a/gdb/python/py-breakpoint.c
> +++ b/gdb/python/py-breakpoint.c
> @@ -1048,7 +1048,7 @@ local_setattro (PyObject *self, PyObject *name, PyObject *v)
> return PyObject_GenericSetAttr ((PyObject *)self, name, v);
> }
>
> -static PyGetSetDef breakpoint_object_getset[] = {
> +static gdb_PyGetSetDef breakpoint_object_getset[] = {
> { "enabled", bppy_get_enabled, bppy_set_enabled,
> "Boolean telling whether the breakpoint is enabled.", NULL },
> { "silent", bppy_get_silent, bppy_set_silent,
> diff --git a/gdb/python/py-event.c b/gdb/python/py-event.c
> index 127dcc7..dc1d73e 100644
> --- a/gdb/python/py-event.c
> +++ b/gdb/python/py-event.c
> @@ -114,7 +114,7 @@ evpy_emit_event (PyObject *event,
> return 0;
> }
>
> -static PyGetSetDef event_object_getset[] =
> +static gdb_PyGetSetDef event_object_getset[] =
> {
> { "__dict__", gdb_py_generic_dict, NULL,
> "The __dict__ for this event.", &event_object_type },
> diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
> index 106fe34..76189b8 100644
> --- a/gdb/python/py-finishbreakpoint.c
> +++ b/gdb/python/py-finishbreakpoint.c
> @@ -426,7 +426,7 @@ gdbpy_initialize_finishbreakpoints (void)
> return 0;
> }
>
> -static PyGetSetDef finish_breakpoint_object_getset[] = {
> +static gdb_PyGetSetDef finish_breakpoint_object_getset[] = {
> { "return_value", bpfinishpy_get_returnvalue, NULL,
> "gdb.Value object representing the return value, if any. \
> None otherwise.", NULL },
> diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
> index 46a0aad..77fc543 100644
> --- a/gdb/python/py-inferior.c
> +++ b/gdb/python/py-inferior.c
> @@ -827,7 +827,7 @@ gdbpy_initialize_inferior (void)
> &membuf_object_type);
> }
>
> -static PyGetSetDef inferior_object_getset[] =
> +static gdb_PyGetSetDef inferior_object_getset[] =
> {
> { "num", infpy_get_num, NULL, "ID of inferior, as assigned by GDB.", NULL },
> { "pid", infpy_get_pid, NULL, "PID of inferior, as assigned by the OS.",
> diff --git a/gdb/python/py-infthread.c b/gdb/python/py-infthread.c
> index 5482bf9..626c15c 100644
> --- a/gdb/python/py-infthread.c
> +++ b/gdb/python/py-infthread.c
> @@ -304,7 +304,7 @@ gdbpy_initialize_thread (void)
> (PyObject *) &thread_object_type);
> }
>
> -static PyGetSetDef thread_object_getset[] =
> +static gdb_PyGetSetDef thread_object_getset[] =
> {
> { "name", thpy_get_name, thpy_set_name,
> "The name of the thread, as set by the user or the OS.", NULL },
> diff --git a/gdb/python/py-lazy-string.c b/gdb/python/py-lazy-string.c
> index ab3f411..1999033 100644
> --- a/gdb/python/py-lazy-string.c
> +++ b/gdb/python/py-lazy-string.c
> @@ -300,7 +300,7 @@ static PyMethodDef lazy_string_object_methods[] = {
> };
>
>
> -static PyGetSetDef lazy_string_object_getset[] = {
> +static gdb_PyGetSetDef lazy_string_object_getset[] = {
> { "address", stpy_get_address, NULL, "Address of the string.", NULL },
> { "encoding", stpy_get_encoding, NULL, "Encoding of the string.", NULL },
> { "length", stpy_get_length, NULL, "Length of the string.", NULL },
> diff --git a/gdb/python/py-linetable.c b/gdb/python/py-linetable.c
> index a5e57b0..8d17aab 100644
> --- a/gdb/python/py-linetable.c
> +++ b/gdb/python/py-linetable.c
> @@ -550,7 +550,7 @@ PyTypeObject ltpy_iterator_object_type = {
> };
>
>
> -static PyGetSetDef linetable_entry_object_getset[] = {
> +static gdb_PyGetSetDef linetable_entry_object_getset[] = {
> { "line", ltpy_entry_get_line, NULL,
> "The line number in the source file.", NULL },
> { "pc", ltpy_entry_get_pc, NULL,
> diff --git a/gdb/python/py-objfile.c b/gdb/python/py-objfile.c
> index 105d88a..6a47c17 100644
> --- a/gdb/python/py-objfile.c
> +++ b/gdb/python/py-objfile.c
> @@ -670,7 +670,7 @@ Add FILE_NAME to the list of files containing debug info for the objfile." },
> { NULL }
> };
>
> -static PyGetSetDef objfile_getset[] =
> +static gdb_PyGetSetDef objfile_getset[] =
> {
> { "__dict__", gdb_py_generic_dict, NULL,
> "The __dict__ for this objfile.", &objfile_object_type },
> diff --git a/gdb/python/py-progspace.c b/gdb/python/py-progspace.c
> index 1e06a75..edabba4 100644
> --- a/gdb/python/py-progspace.c
> +++ b/gdb/python/py-progspace.c
> @@ -378,7 +378,7 @@ gdbpy_initialize_pspace (void)
>
> \f
>
> -static PyGetSetDef pspace_getset[] =
> +static gdb_PyGetSetDef pspace_getset[] =
> {
> { "__dict__", gdb_py_generic_dict, NULL,
> "The __dict__ for this progspace.", &pspace_object_type },
> diff --git a/gdb/python/py-record-btrace.c b/gdb/python/py-record-btrace.c
> index c816332..6d08121 100644
> --- a/gdb/python/py-record-btrace.c
> +++ b/gdb/python/py-record-btrace.c
> @@ -903,7 +903,7 @@ recpy_bt_goto (PyObject *self, PyObject *args)
>
> /* BtraceInstruction members. */
>
> -struct PyGetSetDef btpy_insn_getset[] =
> +struct gdb_PyGetSetDef btpy_insn_getset[] =
> {
> { "number", btpy_number, NULL, "instruction number", NULL},
> { "error", btpy_insn_error, NULL, "error number for gaps", NULL},
> @@ -920,7 +920,7 @@ executed speculatively", NULL},
>
> /* BtraceFunctionCall members. */
>
> -static PyGetSetDef btpy_call_getset[] =
> +static gdb_PyGetSetDef btpy_call_getset[] =
> {
> { "number", btpy_number, NULL, "function call number", NULL},
> { "level", btpy_call_level, NULL, "call stack level", NULL},
> diff --git a/gdb/python/py-record.c b/gdb/python/py-record.c
> index 72922a4..60c0a7c 100644
> --- a/gdb/python/py-record.c
> +++ b/gdb/python/py-record.c
> @@ -175,7 +175,7 @@ Rewind to given location."},
>
> /* Record member list. */
>
> -static PyGetSetDef recpy_record_getset[] = {
> +static gdb_PyGetSetDef recpy_record_getset[] = {
> { "ptid", recpy_ptid, NULL, "Current thread.", NULL },
> { "method", recpy_method, NULL, "Current recording method.", NULL },
> { "format", recpy_format, NULL, "Current recording format.", NULL },
> diff --git a/gdb/python/py-symbol.c b/gdb/python/py-symbol.c
> index b71cfb4..05b002f 100644
> --- a/gdb/python/py-symbol.c
> +++ b/gdb/python/py-symbol.c
> @@ -560,7 +560,7 @@ gdbpy_initialize_symbols (void)
>
> \f
>
> -static PyGetSetDef symbol_object_getset[] = {
> +static gdb_PyGetSetDef symbol_object_getset[] = {
> { "type", sympy_get_type, NULL,
> "Type of the symbol.", NULL },
> { "symtab", sympy_get_symtab, NULL,
> diff --git a/gdb/python/py-symtab.c b/gdb/python/py-symtab.c
> index 09cab22..53b160e 100644
> --- a/gdb/python/py-symtab.c
> +++ b/gdb/python/py-symtab.c
> @@ -544,7 +544,7 @@ gdbpy_initialize_symtabs (void)
>
> \f
>
> -static PyGetSetDef symtab_object_getset[] = {
> +static gdb_PyGetSetDef symtab_object_getset[] = {
> { "filename", stpy_get_filename, NULL,
> "The symbol table's source filename.", NULL },
> { "objfile", stpy_get_objfile, NULL, "The symtab's objfile.",
> @@ -606,7 +606,7 @@ PyTypeObject symtab_object_type = {
> symtab_object_getset /*tp_getset */
> };
>
> -static PyGetSetDef sal_object_getset[] = {
> +static gdb_PyGetSetDef sal_object_getset[] = {
> { "symtab", salpy_get_symtab, NULL, "Symtab object.", NULL },
> { "pc", salpy_get_pc, NULL, "Return the symtab_and_line's pc.", NULL },
> { "last", salpy_get_last, NULL,
> diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c
> index f071006..12b6310 100644
> --- a/gdb/python/py-type.c
> +++ b/gdb/python/py-type.c
> @@ -1413,7 +1413,7 @@ gdbpy_initialize_types (void)
>
> \f
>
> -static PyGetSetDef type_object_getset[] =
> +static gdb_PyGetSetDef type_object_getset[] =
> {
> { "code", typy_get_code, NULL,
> "The code for this type.", NULL },
> @@ -1587,7 +1587,7 @@ PyTypeObject type_object_type =
> 0, /* tp_new */
> };
>
> -static PyGetSetDef field_object_getset[] =
> +static gdb_PyGetSetDef field_object_getset[] =
> {
> { "__dict__", gdb_py_generic_dict, NULL,
> "The __dict__ for this field.", &field_object_type },
> diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
> index bb42e8b..9c0470f 100644
> --- a/gdb/python/py-value.c
> +++ b/gdb/python/py-value.c
> @@ -1767,7 +1767,7 @@ gdbpy_initialize_values (void)
>
> \f
>
> -static PyGetSetDef value_object_getset[] = {
> +static gdb_PyGetSetDef value_object_getset[] = {
> { "address", valpy_get_address, NULL, "The address of the value.",
> NULL },
> { "is_optimized_out", valpy_get_is_optimized_out, NULL,
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index 55efd75..d4ed170 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -286,6 +286,36 @@ gdb_PySys_SetPath (const GDB_PYSYS_SETPATH_CHAR *path)
>
> #define PySys_SetPath gdb_PySys_SetPath
>
> +/* Wrap PyGetSetDef to allow convenient construction with string
> + literals. Unfortunately, PyGetSetDef's 'name' and 'doc' members
> + are 'char *' instead of 'const char *', meaning that in order to
> + list-initialize PyGetSetDef arrays with string literals (and
> + without the wrapping below) would require writing explicit 'char *'
> + casts. Instead, we extend PyGetSetDef and add constexpr
> + constructors that accept const 'name' and 'doc', hiding the ugly
> + casts here in a single place. */
> +
> +struct gdb_PyGetSetDef : PyGetSetDef
> +{
> + constexpr gdb_PyGetSetDef (const char *name_, getter get_, setter set_,
> + const char *doc_, void *closure_)
> + : PyGetSetDef {const_cast<char *> (name_), get_, set_,
> + const_cast<char *> (doc_), closure_}
> + {}
> +
> + /* Alternative constructor that allows omitting the closure in list
> + initialization. */
> + constexpr gdb_PyGetSetDef (const char *name_, getter get_, setter set_,
> + const char *doc_)
> + : gdb_PyGetSetDef {name_, get_, set_, doc_, NULL}
> + {}
> +
> + /* Constructor for the sentinel entries. */
> + constexpr gdb_PyGetSetDef (std::nullptr_t)
> + : gdb_PyGetSetDef { NULL, NULL, NULL, NULL, NULL }
> + {}
> +};
> +
> /* In order to be able to parse symtab_and_line_to_sal_object function
> a real symtab_and_line structure is needed. */
> #include "symtab.h"
> --
> 2.5.5

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible