From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 126159 invoked by alias); 27 Mar 2017 14:28:07 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 126079 invoked by uid 89); 27 Mar 2017 14:28:05 -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=gained, filling, inappropriate, comp 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; Mon, 27 Mar 2017 14:28:03 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 75767C0A57C4; Mon, 27 Mar 2017 14:28:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 75767C0A57C4 Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 75767C0A57C4 Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id B5E8B7F45C; Mon, 27 Mar 2017 14:27:58 +0000 (UTC) Subject: [pushed/ob] cplus_demangle_fill_component: Handle DEMANGLE_COMPONENT_RVALUE_REFERENCE (Re: [PATCH] libiberty: Initialize d_printing in all cplus_demangle_* functions.) To: Mark Wielaard References: <1489356354-27648-1-git-send-email-mark@klomp.org> <20170313181150.GA287@x4> <20170313182959.GC2167@stream> <1489437334.21350.72.camel@klomp.org> <1489482296.21350.77.camel@klomp.org> Cc: Markus Trippelsdorf , gdb-patches@sourceware.org, Nathan Sidwell , Ian Lance Taylor , Nick Clifton , "gcc-patches@gcc.gnu.org" From: Pedro Alves Message-ID: Date: Mon, 27 Mar 2017 14:44:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-03/txt/msg01393.txt.bz2 [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 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 * 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 * 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 + + * cp-demint.c (cplus_demangle_fill_component): Handle + DEMANGLE_COMPONENT_RVALUE_REFERENCE. + 2017-03-12 Mark Wielaard * 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