From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 127893 invoked by alias); 13 Dec 2017 17:13:14 -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 127882 invoked by uid 89); 13 Dec 2017 17:13:13 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.9 required=5.0 tests=BAYES_00,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,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; Wed, 13 Dec 2017 17:13:12 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E12052CE91A; Wed, 13 Dec 2017 17:13:10 +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 B7F7678DE6; Wed, 13 Dec 2017 17:13:10 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: GDB Patches , Tom Tromey , Eli Zaretskii , Simon Marchi , Keith Seitz Subject: Re: [PATCH v5 2/2] Implement pahole-like 'ptype /o' option References: <20171121160709.23248-1-sergiodj@redhat.com> <20171213031724.22721-1-sergiodj@redhat.com> <20171213031724.22721-3-sergiodj@redhat.com> <614d15fc-3793-8a55-b7cc-67570e8c46d3@redhat.com> Date: Wed, 13 Dec 2017 17:13:00 -0000 In-Reply-To: <614d15fc-3793-8a55-b7cc-67570e8c46d3@redhat.com> (Pedro Alves's message of "Wed, 13 Dec 2017 16:18:59 +0000") Message-ID: <87r2ryk0ux.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/msg00317.txt.bz2 On Wednesday, December 13 2017, Pedro Alves wrote: > On 12/13/2017 03:17 AM, Sergio Durigan Junior wrote: > >> +/* A struct with an union. */ >> + >> +struct poi >> +{ >> + int f1; >> + >> + union qwe f2; >> + >> + uint16_t f3; >> + >> + struct pqr f4; >> +}; >> + >> +/* A struct with bitfields. */ >> + >> +struct tyu >> +{ >> + int a1 : 1; >> + >> + int a2 : 3; >> + >> + int a3 : 23; >> + >> + char a4 : 2; >> + >> + int64_t a5; >> + >> + int a6 : 5; >> + >> + int64_t a7 : 3; >> +}; > > I think that the testcase should also make sure to exercise the new > offset computations in the case c_print_type_struct_field_offset's > 'offset_bitpos' parameter is > 0. Is it already covered? > I assume we'll need a test like tyu (struct with bitfields with > overlapping underlying objects), but that inherits some other > base structure? Ah, good point, I'll add this test. > >> +++ b/gdb/testsuite/gdb.base/ptype-offsets.exp >> @@ -0,0 +1,192 @@ >> +# This testcase is part of GDB, the GNU debugger. >> + >> +# Copyright 2017 Free Software Foundation, Inc. >> + >> +# This program is free software; you can redistribute it and/or modify >> +# it under the terms of the GNU General Public License as published by >> +# the Free Software Foundation; either version 3 of the License, or >> +# (at your option) any later version. >> +# >> +# This program is distributed in the hope that it will be useful, >> +# but WITHOUT ANY WARRANTY; without even the implied warranty of >> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> +# GNU General Public License for more details. >> +# >> +# You should have received a copy of the GNU General Public License >> +# along with this program. If not, see . >> + > > Please add an intro comment describing what this testcase is about. OK. >> +standard_testfile .cc >> + >> +# Test only works on x86_64 LP64 targets. That's how we guarantee >> +# that the expected holes will be present in the struct. >> +if { !([istarget "x86_64-*-*"] && [is_lp64_target]) } { >> + untested "test work only on x86_64 lp64" >> + return 0 >> +} > > I'm mildly worried about whether the bitfield handling is working > correctly on big endian machines. We may want to lift this > x86-64-only restriction, by using e.g., alignas(N) or > __attribute__((aligned(N)) to take care of most of the differences > between architectures and end up with few per-arch code in > the .exp. But I'm fine with starting with only x86-64 if you > confirm manually on e.g., a big endian PPC64 machine on the > compile farm, and we can extend the testcase in that direction > after this is merged. OK, I'll confirm on PPC64BE. >> + >> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile \ >> + { debug c++ optimize=-O0 }] } { >> + return -1 >> +} > > Weren't you going to remove that optimize thing? :-) Yes, sorry. Removed. >> +# Test that the offset is properly reset when we are printing an union >> +# and go inside two inner structs. >> +# This also tests a struct inside a struct inside an union. > > "a union". (two times here; there may be other places.) Fixed. >> +gdb_test "ptype /o union qwe" \ >> + [multi_line \ >> +"/\\\* offset | size \\\*/" \ >> +"/\\\* 24 \\\*/ struct tuv {" \ >> +"/\\\* 0 | 4 \\\*/ int a1;" \ >> +"/\\\* XXX 4-byte hole \\\*/" \ >> +"/\\\* 8 | 8 \\\*/ char \\\*a2;" \ >> +"/\\\* 16 | 4 \\\*/ int a3;" \ >> +" } /\\\* total size: 24 bytes \\\*/ fff1;" \ >> +"/\\\* 40 \\\*/ struct xyz {" \ >> +"/\\\* 0 | 4 \\\*/ int f1;" \ >> +"/\\\* 4 | 1 \\\*/ char f2;" \ >> +"/\\\* XXX 3-byte hole \\\*/" \ >> +"/\\\* 8 | 8 \\\*/ void \\\*f3;" \ >> +"/\\\* 16 | 24 \\\*/ struct tuv {" \ >> +"/\\\* 16 | 4 \\\*/ int a1;" \ >> +"/\\\* XXX 4-byte hole \\\*/" \ >> +"/\\\* 24 | 8 \\\*/ char \\\*a2;" \ >> +"/\\\* 32 | 4 \\\*/ int a3;" \ >> +" } /\\\* total size: 24 bytes \\\*/ f4;" \ >> +" } /\\\* total size: 40 bytes \\\*/ fff2;" \ >> +"} /\\\* total size: 40 bytes \\\*/"] \ >> + "ptype offset union qwe" > > Did you try using {} instead of "" for these strings, > avoiding all the escaping? Yes, Simon also made the same comment. I tried replacing by {} but it didn't work at the first attempt and since I was hacking other stuff at the moment I dind't bother tweaking it and just reverted to using "". If it's something you really want, I can do. Otherwise I'd prefer to leave it like that. >> @@ -499,6 +506,11 @@ whatis_exp (const char *exp, int show) >> real_type = value_rtti_type (val, &full, &top, &using_enc); >> } >> >> + if (flags.print_offsets && > > && goes on the next line. Fixed. >> + (TYPE_CODE (type) == TYPE_CODE_STRUCT >> + || TYPE_CODE (type) == TYPE_CODE_UNION)) >> + fprintf_filtered (gdb_stdout, "/* offset | size */\n"); >> + >> printf_filtered ("type = "); > > Thanks, > Pedro Alves 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/