From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6861 invoked by alias); 8 Mar 2017 22:53:54 -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 6849 invoked by uid 89); 8 Mar 2017 22:53:53 -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=crying 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, 08 Mar 2017 22:53:52 +0000 Received: from smtp.corp.redhat.com (int-mx16.intmail.prod.int.phx2.redhat.com [10.5.11.28]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 806402BA0EB for ; Wed, 8 Mar 2017 22:53:51 +0000 (UTC) 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 A0430BEDA0; Wed, 8 Mar 2017 22:53:49 +0000 (UTC) Subject: Re: [PATCH] c++/8218: Destructors w/arguments. To: Keith Seitz , gdb-patches@sourceware.org References: <1489010611-28451-1-git-send-email-keiths@redhat.com> From: Pedro Alves Message-ID: <2ca278e9-a52a-c082-77f5-5ed60e39091e@redhat.com> Date: Wed, 08 Mar 2017 22:53: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: <1489010611-28451-1-git-send-email-keiths@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-03/txt/msg00116.txt.bz2 Hi Keith, In principle, I like the change, as it's obviously a desirable behavior change, but I have a few concerns. See below. > gdb/ChangeLog > > PR c++/8218 > * c-typeprint.c (cp_type_print_method_args): Start printing arguments > at index 0, ignoring STATCIP. > Skip artificial arguments. > > gdb/testsuite/ChangeLog > > PR c++/8218 > * c-typeprint.c (cp_type_print_method_args): Start printing arguments > at index 0, ignoring STATCIP. > Skip artificial arguments. Wrong entry for testsuite. > --- > gdb/ChangeLog | 7 +++++++ > gdb/c-typeprint.c | 11 ++++++++--- > gdb/testsuite/ChangeLog | 8 ++++++++ > gdb/testsuite/gdb.cp/templates.exp | 24 +++++++++++++++--------- > 4 files changed, 38 insertions(+), 12 deletions(-) > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index 3ac5170..66cdfbd 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,3 +1,10 @@ > +2017-03-08 Keith Seitz > + > + PR c++/8218 > + * c-typeprint.c (cp_type_print_method_args): Start printing arguments > + at index 0, ignoring STATCIP. > + Skip artificial arguments. > + > 2017-02-21 Jan Kratochvil > > * dwarf2read.c (dwarf2_record_block_ranges): Add forgotten BASEADDR. > diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c > index 75f6d61..e504618 100644 > --- a/gdb/c-typeprint.c > +++ b/gdb/c-typeprint.c > @@ -226,13 +226,18 @@ cp_type_print_method_args (struct type *mtype, const char *prefix, > language_cplus, DMGL_ANSI); > fputs_filtered ("(", stream); > > - /* Skip the class variable. */ > - i = staticp ? 0 : 1; > + i = 0; > if (nargs > i) > { > while (i < nargs) > { > - c_print_type (args[i++].type, "", stream, 0, 0, flags); > + struct field arg = args[i++]; The manipulation of 'i' looks a bit obscure. This is crying for a "for", IMO: if (nargs > 0) { for (int i = 0; i < nargs; i++) { struct field arg = args[i]; > + > + /* Skip any artificial arguments. */ > + if (FIELD_ARTIFICIAL (arg)) > + continue; Can we trust that FIELD_ARTIFICIAL is always set on the implicit this argument on all debug formats? I.e., does it work with stabs? Also, even for DWARF, does it work with debug info produced by older GCCs? And older dwarf revisions? -gdwarf-2, etc. Can you poke a bit please? I wouldn't be surprised if we still needed to treat the first argument specially. Can you comment on the treatment c_type_print_args gives to artificial arguments that Tom pointed out in the PR? Also that function's intro comment talks about C++ "this". Is that stale? Thanks, Pedro Alves