From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12795 invoked by alias); 14 Dec 2017 14:50:34 -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 11288 invoked by uid 89); 14 Dec 2017 14:50:33 -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 14:50:32 +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 557BAC053FA7; Thu, 14 Dec 2017 14:50:31 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4B4646C531; Thu, 14 Dec 2017 14:50:19 +0000 (UTC) Subject: Re: [PATCH v6 2/2] Implement pahole-like 'ptype /o' option To: Sergio Durigan Junior , GDB Patches References: <20171121160709.23248-1-sergiodj@redhat.com> <20171214024816.29629-1-sergiodj@redhat.com> <20171214024816.29629-3-sergiodj@redhat.com> Cc: Tom Tromey , Eli Zaretskii , Simon Marchi , Keith Seitz From: Pedro Alves Message-ID: Date: Thu, 14 Dec 2017 14: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: <20171214024816.29629-3-sergiodj@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-12/txt/msg00346.txt.bz2 On 12/14/2017 02:48 AM, Sergio Durigan Junior wrote: > A big part of this patch handles the formatting logic of 'ptype', > which is a bit messy. The code to handle bitfield offsets, however, > took some time to craft. My thanks to Pedro Alves for figuring things > out and pointing me to the right direction, as well as coming up with > a way to inspect the layout of structs with bitfields (see testcase > for comments). Why, thanks. :-) > +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. > + 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... > + > +#include > + > + > +# Test only works on x86_64 LP64 targets. That's how we guarantee Remove reference to x86_64; it's no longer true. > +# 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". > +/* 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. > --- 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? Thanks, Pedro Alves