[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 = &d_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 Alves <pal...@redhat.com> Date: 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 <doug.gre...@gmail.com> * 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 cplus_demangle_fill_component in both the gcc and binutils-gdb trees.) libiberty/ChangeLog: 2017-03-27 Pedro Alves <pal...@redhat.com> * cp-demint.c (cplus_demangle_fill_component): Handle DEMANGLE_COMPONENT_RVALUE_REFERENCE. --- libiberty/ChangeLog | 5 +++++ libiberty/cp-demint.c | 1 + 2 files changed, 6 insertions(+) diff --git a/libiberty/ChangeLog b/libiberty/ChangeLog index b513fce..f6318e2 100644 --- a/libiberty/ChangeLog +++ b/libiberty/ChangeLog @@ -1,3 +1,8 @@ +2017-03-27 Pedro Alves <pal...@redhat.com> + + * cp-demint.c (cplus_demangle_fill_component): Handle + DEMANGLE_COMPONENT_RVALUE_REFERENCE. + 2017-03-12 Mark Wielaard <m...@klomp.org> * cp-demangle.c (cplus_demangle_fill_name): Initialize diff --git a/libiberty/cp-demint.c b/libiberty/cp-demint.c index 13a71d9..158b90a 100644 --- a/libiberty/cp-demint.c +++ b/libiberty/cp-demint.c @@ -106,6 +106,7 @@ cplus_demangle_fill_component (struct demangle_component *p, case DEMANGLE_COMPONENT_CONST_THIS: case DEMANGLE_COMPONENT_POINTER: case DEMANGLE_COMPONENT_REFERENCE: + case DEMANGLE_COMPONENT_RVALUE_REFERENCE: case DEMANGLE_COMPONENT_COMPLEX: case DEMANGLE_COMPONENT_IMAGINARY: case DEMANGLE_COMPONENT_VENDOR_TYPE: -- 2.5.5