[pushed/ob] cplus_demangle_fill_component: Handle DEMANGLE_COMPONENT_RVALUE_REFERENCE (Re: [PATCH] libiberty: Initialize d_printing in all cplus_demangle_* functions.)
[added back gcc-patches, obvious libiberty patch below] On 03/14/2017 10:50 AM, Pedro Alves wrote: > On 03/14/2017 09:04 AM, Mark Wielaard wrote: > >> That looks good. But note that there is one behavioral change. >> cplus_demangle_fill_component is defined as: >> >> /* Fill in most component types with a left subtree and a right >>subtree. Returns non-zero on success, zero on failure, such as an >>unrecognized or inappropriate component type. */ >> >> And gdb/cp-name-parser.y fill_comp does: >> >> i = cplus_demangle_fill_component (ret, d_type, lhs, rhs); >> gdb_assert (i); >> >> So you have an extra sanity check. Where before you might have silently >> created a bogus demangle_component. Which I assume is what you want. > > Indeed, and I think it is. > >> But it might depend on what gdb_assert precisely does > > gdb_assert triggers the infamous: > > A problem internal to GDB has been detected, > further debugging may prove unreliable. > Quit this debugging session? (y or n) > >> and if the parser input is normally "sane" or not. > > The only thing that is validated is that we don't create > a component with a left/right subtree when that doesn't make sense > for the component type. I think trying to create such a component > would be a parser/grammar/production bug, even with invalid input. > > I had run into that assert in fill_comp yesterday actually, with a slightly > different/larger patch. At first I saw the cplus_demangle_fill_component > declaration in demangle.h, but didn't see the implementation in > cp-demangle.c, and > thought that was just some oversight/left over. So I though I'd add one. I > factored > out a cplus_demangle_fill_component out of cp-demangle.c:d_make_comp, > following the > same pattern used by the other cplus_demangle_fill* / d_make* function pairs. > > Only after did I notice that actually, there's an implementation of > cplus_demangle_fill_component in cplus-demint.c... AFAICS, that's only > used by GDB. No other tool in the binutils-gdb repo, nor GCC's repo uses it, > AFAICS. > So I figured that I'd remove the cplus-demint.c implementation, in favor of > the new implementation in cp-demangle.c based on d_make_comp. And _that_ ran > into the > assertion, because the implementations are exactly the same. GDB fills in > some types with > NULL left components and fills them in later. For example, for > DEMANGLE_COMPONENT_POINTER: > > ptr_operator : '*' qualifiers_opt > - { $$.comp = make_empty (DEMANGLE_COMPONENT_POINTER); > - $$.comp->u.s_binary.left = > $$.comp->u.s_binary.right = NULL; > + { $$.comp = fill_comp (DEMANGLE_COMPONENT_POINTER, > NULL, NULL); > $$.last = _left ($$.comp); > $$.comp = d_qualify ($$.comp, $2, 0); } > > Note how cp-demangle.c:d_make_comp's validations are stricter than > cp-demint.c:cplus_demangle_fill_component's. The former only allows > lazy-filling for a few cases: > > [...] > /* These are allowed to have no parameters--in some cases they >will be filled in later. */ > case DEMANGLE_COMPONENT_FUNCTION_TYPE: > [...] > > While cp-demint.c:cplus_demangle_fill_component, the version that > GDB uses, allows that in all cases. IOW, passing in a NULL left/right subtree > to cplus_demangle_fill_component when the component type expects one is OK, > assuming > that the caller will fill them in afterwards. I crossed checked the types in > the new fill_comp calls with the checks inside cplus_demangle_fill_component > now, > and I believe they're all OK. > > Maybe it'd be possible to avoid this lazy filling in, but I didn't > bother trying. Funny enough, I was going to apply the gdb patch today, but gdb meanwhile gained a new make_empty call for DEMANGLE_COMPONENT_RVALUE_REFERENCE, and making that new code use fill_comp/cplus_demangle_fill_component too triggered the sanity check discussed above... This is just a latent bug being exposed now. I've pushed the obvious patch below to both gcc trunk and binutils-gdb master. >From b1a42fdfa31937d7e05df34afee970ac0ad239e1 Mon Sep 17 00:00:00 2001 From: Pedro AlvesDate: Mon, 27 Mar 2017 13:56:49 +0100 Subject: [PATCH] cplus_demangle_fill_component: Handle DEMANGLE_COMPONENT_RVALUE_REFERENCE This patch almost a decade ago: ... 2007-08-31 Douglas Gregor * cp-demangle.c (d_dump): Handle DEMANGLE_COMPONENT_RVALUE_REFERENCE. (d_make_comp): Ditto. ... ... missed doing the same change to cplus_demangle_fill_component that was done to d_make_comp. I.e., teach it to only validate that we're not passing in a "right" subtree. GDB has recently (finally) learned about rvalue references, and a change to make it use cplus_demangle_fill_component more ran into an assertion because of this. (GDB is the only user of
Re: [PATCH] libiberty: Initialize d_printing in all cplus_demangle_* functions.
On 03/13/2017 06:29 PM, Mark Wielaard wrote: > O, sorry. I should have let Nick known about the gdb regressions I found. > Besides this patch gdb needs the following one-liner fix: > > diff --git a/gdb/cp-name-parser.y b/gdb/cp-name-parser.y > index fd1e949..5278c05 100644 > --- a/gdb/cp-name-parser.y > +++ b/gdb/cp-name-parser.y > @@ -201,6 +201,7 @@ make_empty (enum demangle_component_type d_type) > { >struct demangle_component *ret = d_grab (); >ret->type = d_type; > + ret->d_printing = 0; >return ret; > } Should gdb be memsetting instead to avoid having to know about libiberty's "internals"? Either version is pre-approved for GDB. Thanks, Pedro Alves
Re: [PATCH] libiberty: Initialize d_printing in all cplus_demangle_* functions.
On Mon, Mar 13, 2017 at 07:11:50PM +0100, Markus Trippelsdorf wrote: > On 2017.03.12 at 23:05 +0100, Mark Wielaard wrote: > > While integrating the d_printing recursion guard change into gdb I > > noticed we forgot to initialize the demangle_component d_printing > > field in cplus_demangle_fill_{name,extended_operator,ctor,dtor}. > > As is done in cplus_demangle_fill_{component,builtin_type,operator}. > > It happened to work because in gcc all demangle_components were > > allocated through d_make_empty. But gdb has its own allocation > > mechanism (as might other users). > > Nick has synced the binutils-gdb repro with gcc's. > I think you should commit your fix as obvious. Ian just approved it and I checked it in. gcc svn r246105. > And it would be nice if Nick could sync again, after your patch went in. O, sorry. I should have let Nick known about the gdb regressions I found. Besides this patch gdb needs the following one-liner fix: diff --git a/gdb/cp-name-parser.y b/gdb/cp-name-parser.y index fd1e949..5278c05 100644 --- a/gdb/cp-name-parser.y +++ b/gdb/cp-name-parser.y @@ -201,6 +201,7 @@ make_empty (enum demangle_component_type d_type) { struct demangle_component *ret = d_grab (); ret->type = d_type; + ret->d_printing = 0; return ret; } Cheers, Mark
Re: [PATCH] libiberty: Initialize d_printing in all cplus_demangle_* functions.
On 2017.03.12 at 23:05 +0100, Mark Wielaard wrote: > While integrating the d_printing recursion guard change into gdb I > noticed we forgot to initialize the demangle_component d_printing > field in cplus_demangle_fill_{name,extended_operator,ctor,dtor}. > As is done in cplus_demangle_fill_{component,builtin_type,operator}. > It happened to work because in gcc all demangle_components were > allocated through d_make_empty. But gdb has its own allocation > mechanism (as might other users). Nick has synced the binutils-gdb repro with gcc's. I think you should commit your fix as obvious. And it would be nice if Nick could sync again, after your patch went in. -- Markus
Re: [PATCH] libiberty: Initialize d_printing in all cplus_demangle_* functions.
On Sun, Mar 12, 2017 at 3:05 PM, Mark Wielaardwrote: > While integrating the d_printing recursion guard change into gdb I > noticed we forgot to initialize the demangle_component d_printing > field in cplus_demangle_fill_{name,extended_operator,ctor,dtor}. > As is done in cplus_demangle_fill_{component,builtin_type,operator}. > It happened to work because in gcc all demangle_components were > allocated through d_make_empty. But gdb has its own allocation > mechanism (as might other users). > > libiberty/ChangeLog: > >* cp-demangle.c (cplus_demangle_fill_name): Initialize >demangle_component d_printing. >(cplus_demangle_fill_extended_operator): Likewise. >(cplus_demangle_fill_ctor): Likewise. >(cplus_demangle_fill_dtor): Likewise. This is OK. Thanks. Ian
[PATCH] libiberty: Initialize d_printing in all cplus_demangle_* functions.
While integrating the d_printing recursion guard change into gdb I noticed we forgot to initialize the demangle_component d_printing field in cplus_demangle_fill_{name,extended_operator,ctor,dtor}. As is done in cplus_demangle_fill_{component,builtin_type,operator}. It happened to work because in gcc all demangle_components were allocated through d_make_empty. But gdb has its own allocation mechanism (as might other users). libiberty/ChangeLog: * cp-demangle.c (cplus_demangle_fill_name): Initialize demangle_component d_printing. (cplus_demangle_fill_extended_operator): Likewise. (cplus_demangle_fill_ctor): Likewise. (cplus_demangle_fill_dtor): Likewise. --- libiberty/ChangeLog | 8 libiberty/cp-demangle.c | 4 2 files changed, 12 insertions(+) diff --git a/libiberty/ChangeLog b/libiberty/ChangeLog index e93e327..b513fce 100644 --- a/libiberty/ChangeLog +++ b/libiberty/ChangeLog @@ -1,3 +1,11 @@ +2017-03-12 Mark Wielaard+ + * cp-demangle.c (cplus_demangle_fill_name): Initialize + demangle_component d_printing. + (cplus_demangle_fill_extended_operator): Likewise. + (cplus_demangle_fill_ctor): Likewise. + (cplus_demangle_fill_dtor): Likewise. + 2017-03-08 Mark Wielaard PR demangler/70909 diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c index 341a418..04832ff 100644 --- a/libiberty/cp-demangle.c +++ b/libiberty/cp-demangle.c @@ -854,6 +854,7 @@ cplus_demangle_fill_name (struct demangle_component *p, const char *s, int len) { if (p == NULL || s == NULL || len == 0) return 0; + p->d_printing = 0; p->type = DEMANGLE_COMPONENT_NAME; p->u.s_name.s = s; p->u.s_name.len = len; @@ -869,6 +870,7 @@ cplus_demangle_fill_extended_operator (struct demangle_component *p, int args, { if (p == NULL || args < 0 || name == NULL) return 0; + p->d_printing = 0; p->type = DEMANGLE_COMPONENT_EXTENDED_OPERATOR; p->u.s_extended_operator.args = args; p->u.s_extended_operator.name = name; @@ -888,6 +890,7 @@ cplus_demangle_fill_ctor (struct demangle_component *p, || (int) kind < gnu_v3_complete_object_ctor || (int) kind > gnu_v3_object_ctor_group) return 0; + p->d_printing = 0; p->type = DEMANGLE_COMPONENT_CTOR; p->u.s_ctor.kind = kind; p->u.s_ctor.name = name; @@ -907,6 +910,7 @@ cplus_demangle_fill_dtor (struct demangle_component *p, || (int) kind < gnu_v3_deleting_dtor || (int) kind > gnu_v3_object_dtor_group) return 0; + p->d_printing = 0; p->type = DEMANGLE_COMPONENT_DTOR; p->u.s_dtor.kind = kind; p->u.s_dtor.name = name; -- 1.8.3.1