From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1942 invoked by alias); 14 Dec 2017 20:29:13 -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 1931 invoked by uid 89); 14 Dec 2017 20:29:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.9 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= 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; Thu, 14 Dec 2017 20:29:11 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E69BC4A700; Thu, 14 Dec 2017 20:29:09 +0000 (UTC) Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A493B4145; Thu, 14 Dec 2017 20:29:09 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: GDB Patches , Tom Tromey , Eli Zaretskii , Simon Marchi , Keith Seitz Subject: Re: [PATCH v6 2/2] Implement pahole-like 'ptype /o' option References: <20171121160709.23248-1-sergiodj@redhat.com> <20171214024816.29629-1-sergiodj@redhat.com> <20171214024816.29629-3-sergiodj@redhat.com> Date: Thu, 14 Dec 2017 20:29:00 -0000 In-Reply-To: (Pedro Alves's message of "Thu, 14 Dec 2017 14:50:18 +0000") Message-ID: <877etpgiju.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-12/txt/msg00356.txt.bz2 Thanks for the review. On Thursday, December 14 2017, Pedro Alves wrote: > On 12/14/2017 02:48 AM, Sergio Durigan Junior wrote: > >> +Issuing a @kbd{ptype /o struct tuv} command would print: >> + >> +@smallexample >> +(@value{GDBP}) ptype /o struct tuv >> +/* offset | size */ >> +struct tuv @{ >> +/* 0 | 4 */ int a1; >> +/* XXX 4-byte hole */ >> +/* 8 | 8 */ char *a2; >> +/* 16 | 4 */ int a3; >> +@} /* total size: 24 bytes */ >> +@end smallexample > > These examples need to be updated per the new output format. Done. >> + You should have received a copy of the GNU General Public License >> + along with this program. If not, see . */ >> + >> +/* This file will be used to test 'ptype /o' on x86_64 only. */ > > No longer true... Removed. >> + >> +#include >> + > >> + >> +# Test only works on x86_64 LP64 targets. That's how we guarantee > > Remove reference to x86_64; it's no longer true. Done. >> +# that the expected holes will be present in the struct. >> +if { ![is_lp64_target] } { >> + untested "test work only on lp64 targets" >> + return 0 >> +} >> + >> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile \ >> + { debug c++ }] } { >> + return -1 >> +} > > I think we're missing a test for "ptype /oTM", to make sure that > we can still override the fact that /o implies /tm ? > I'd add at least one for "/oTM" and one for "/TMo". The > latter ends up being the same as "/o". Hm, OK. Added the tests. >> +/* The default values for a struct print_offset_data. */ >> + >> +struct print_offset_data print_offset_default_data = >> +{ >> + 0, /* offset_bitpos */ >> + 0, /* endpos */ >> +}; >> + > > Do we really need this object ? How about just defining > the default values in-class ? See below. Indeed. I removed it in favour of initializing in-class. >> --- a/gdb/typeprint.h >> +++ b/gdb/typeprint.h >> @@ -24,6 +24,22 @@ struct ui_file; >> struct typedef_hash_table; >> struct ext_lang_type_printers; >> >> +struct print_offset_data >> +{ >> + /* The offset to be applied to bitpos when PRINT_OFFSETS is true. >> + This is needed for when we are printing nested structs and want >> + to make sure that the printed offset for each field carries over >> + the offset of the outter struct. */ >> + unsigned int offset_bitpos; > > So here: > > unsigned int offset_bitpos = 0; > >> + >> + /* ENDPOS is the one-past-the-end bit position of the previous field >> + (where we expect the current field to be if there is no >> + hole). */ >> + unsigned int endpos; > > and > > unsigned int endpos = 0; > > Then you default-constructed print_offset_data objects have > the fields automatically zeroed: > > print_offset_data podata; > > while at it, wouldn't it be better to name this one "end_bitpos" or > something like that with "bit" in it as well? Right; endpos is too generic indeed. Renamed to end_bitpos. Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/