From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 82938 invoked by alias); 14 Mar 2017 10:50:24 -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 82917 invoked by uid 89); 14 Mar 2017 10:50:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM autolearn=no version=3.3.2 spammy=infamous, iow, H*f:sk:1489482, H*i:sk:1489482 X-HELO: mail-wm0-f42.google.com Received: from mail-wm0-f42.google.com (HELO mail-wm0-f42.google.com) (74.125.82.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 14 Mar 2017 10:50:22 +0000 Received: by mail-wm0-f42.google.com with SMTP id v186so60641061wmd.0 for ; Tue, 14 Mar 2017 03:50:22 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=GT2FiFLOBNY16CrnNgx3fnVOywgP9Bimp+vAm59uAZw=; b=ctzyMOkTP65WQ9J+FPixcGWN4anjBv2jercfpGN2vYzZD0SYJbZvJqcInVeBiK/Cek 4jj+/WI+GgDsbVXJAQiXg6++qrlQvFb/qNoVaQ7zVNBVW5oMkrAMJIQIlVSB0ZpEOq6L mOtK8f15NZryRNVXJPs8dnpWneh0cPbiafxCSFbl1mwaPo/F8ZEEkYKu9gx8RfrVW11z eSrARPhHxfljhn562/wTxbLu1ZUgK7ttVvJUye5IjIvfAPas8ZAyt8zCWymm/mwYHXZw 7M4YKeVJcYeylQ3ckzk3WvL5o1UDTar29ctjvvCjjeJozqt8o/daBE4vrwpZ779kEQVv UCpQ== X-Gm-Message-State: AFeK/H3ZphA1kThiqt6pVXjE0ZmNrrhbd7rZW6ZJRYTkAJozLIiVz0BqxOU7MmeDEnwt2U/f X-Received: by 10.28.224.69 with SMTP id x66mr14243653wmg.21.1489488621081; Tue, 14 Mar 2017 03:50:21 -0700 (PDT) Received: from [192.168.0.101] ([37.189.166.198]) by smtp.gmail.com with ESMTPSA id k10sm14999118wmg.10.2017.03.14.03.50.17 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 14 Mar 2017 03:50:20 -0700 (PDT) Subject: 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 From: Pedro Alves Message-ID: Date: Tue, 14 Mar 2017 10:50: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: <1489482296.21350.77.camel@klomp.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-03/txt/msg00232.txt.bz2 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. Thanks, Pedro Alves