From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 55092 invoked by alias); 5 Apr 2017 15:48:50 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 54909 invoked by uid 89); 5 Apr 2017 15:48:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 05 Apr 2017 15:48:37 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 092E680086 for ; Wed, 5 Apr 2017 15:48:26 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 092E680086 Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=sergiodj@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 092E680086 Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id B280F5C54B; Wed, 5 Apr 2017 15:48:25 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 13/18] -Wwrite-strings: Wrap PyGetSetDef for construction with string literals References: <1491326751-16180-1-git-send-email-palves@redhat.com> <1491326751-16180-14-git-send-email-palves@redhat.com> <877f30dnk0.fsf@redhat.com> Date: Wed, 05 Apr 2017 15:48:00 -0000 In-Reply-To: (Pedro Alves's message of "Wed, 5 Apr 2017 13:35:19 +0100") Message-ID: <87lgredfee.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-04/txt/msg00113.txt.bz2 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 > 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 > > * 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) > > > > -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) > > > > -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) > > > > -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) > > > > -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) > > > > -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 (name_), get_, set_, > + const_cast (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 http://sergiodj.net/