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

Pedro Alves
2017-04-05T12:35:30Z

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?

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