From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 127415 invoked by alias); 13 Dec 2017 04:50:18 -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 127406 invoked by uid 89); 13 Dec 2017 04:50:17 -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,KAM_SHORT,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=alert!, ALERT!, thanking X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 13 Dec 2017 04:50:14 +0000 Received: from [10.0.0.11] (192-222-251-162.qc.cable.ebox.net [192.222.251.162]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 1EFCA1E51A; Tue, 12 Dec 2017 23:50:12 -0500 (EST) Subject: Re: [PATCH v5 2/2] Implement pahole-like 'ptype /o' option To: Sergio Durigan Junior , GDB Patches Cc: Tom Tromey , Eli Zaretskii , Simon Marchi , Pedro Alves , Keith Seitz References: <20171121160709.23248-1-sergiodj@redhat.com> <20171213031724.22721-1-sergiodj@redhat.com> <20171213031724.22721-3-sergiodj@redhat.com> From: Simon Marchi Message-ID: Date: Wed, 13 Dec 2017 04:50:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <20171213031724.22721-3-sergiodj@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-12/txt/msg00296.txt.bz2 On 2017-12-12 10:17 PM, Sergio Durigan Junior wrote: > +/* Print information about field at index FIELD_IDX of the struct type > + TYPE. > + > + ENDPOS is the one-past-the-end bit position of the previous field > + (where we expect this field to be if there is no hole). At the > + end, ENDPOS is updated to the one-past-the-end bit position of the > + current field. > + > + OFFSET_BITPOS is the offset value we carry over when we are > + printing a struct that is inside another struct; this is useful so > + that the offset is constantly incremented (if we didn't carry it > + over, the offset would be reset to zero when printing the inner > + struct). > + > + The output is strongly based on pahole(1). */ > + > +static void > +c_print_type_struct_field_offset (struct type *type, unsigned int field_idx, > + unsigned int *endpos, struct ui_file *stream, > + unsigned int offset_bitpos) > +{ > + struct type *ftype = check_typedef (TYPE_FIELD_TYPE (type, field_idx)); > + unsigned int bitpos = TYPE_FIELD_BITPOS (type, field_idx); > + unsigned int fieldsize_byte = TYPE_LENGTH (ftype); > + unsigned int fieldsize_bit = fieldsize_byte * TARGET_CHAR_BIT; > + > + /* We check for *ENDPOS > 0 because there is a specific scenario > + when *ENDPOS can be zero and BITPOS can be > 0: when we are > + dealing with a struct/class with a virtual method. Because of > + the vtable, the first field of the struct/class will have an > + offset of sizeof (void *) (the size of the vtable). If we do not > + check for *ENDPOS > 0 here, GDB will report a hole before the > + first field, which is not accurate. */ > + if (*endpos > 0 && *endpos < bitpos) > + { > + /* If ENDPOS is smaller than the current type's bitpos, it means > + there's a hole in the struct, so we report it here. */ > + unsigned int hole = bitpos - *endpos; > + unsigned int hole_byte = hole / TARGET_CHAR_BIT; > + unsigned int hole_bit = hole % TARGET_CHAR_BIT; > + > + if (hole_bit > 0) > + fprintf_filtered (stream, "/* XXX %2u-bit hole */\n", hole_bit); > + > + if (hole_byte > 0) > + fprintf_filtered (stream, "/* XXX %2u-byte hole */\n", hole_byte); > + } > + > + if (TYPE_FIELD_PACKED (type, field_idx)) > + { > + /* We're dealing with a bitfield. Print how many bits are left > + to be used. */ > + unsigned int bitsize = TYPE_FIELD_BITSIZE (type, field_idx); > + /* The bitpos relative to the beginning of our container > + field. */ > + unsigned int relative_bitpos; > + > + /* The following was copied from > + value.c:value_primitive_field. */ > + if ((bitpos % fieldsize_bit) + bitsize <= fieldsize_bit) > + relative_bitpos = bitpos % fieldsize_bit; > + else > + relative_bitpos = bitpos % TARGET_CHAR_BIT; > + > + /* This is the exact offset (in bits) of this bitfield. */ > + unsigned int bit_offset > + = (bitpos - relative_bitpos) + offset_bitpos; > + > + /* The position of the field, relative to the beginning of the > + struct, and how many bits are left to be used in this > + container. */ > + fprintf_filtered (stream, "/* %4u:%2u", bit_offset / TARGET_CHAR_BIT, > + fieldsize_bit - (relative_bitpos + bitsize)); > + fieldsize_bit = bitsize; > + } I won't pretend I understand this part. I'll let Pedro look at it, or maybe try again tomorrow, it's getting late here :) > @@ -1041,25 +1183,63 @@ c_type_print_base_struct_union (struct type *type, struct ui_file *stream, > the debug info, they should be artificial. */ > if ((i == vptr_fieldno && type == basetype) > || TYPE_FIELD_ARTIFICIAL (type, i)) > - continue; > + { > + if (flags->print_offsets) > + c_print_type_vtable_offset_marker (type, i, stream, > + flags->offset_bitpos); > + continue; > + } With that version, the comment above the if would need to be updted. Note that the vtable marker won't be printed when looking at a child class. With the following: struct foo { virtual ~foo() = default; int a; short b; }; struct bar : foo { virtual ~bar () = default; int z; short c, d, e; }; we get this: (gdb) ptype /o foo /* offset | size */ type = struct foo { /* 0 | 8 */ /* vtable */ public: /* 8 | 4 */ int a; /* 12 | 2 */ short b; ~foo(); } /* total size: 16 bytes */ (gdb) ptype /o bar /* offset | size */ type = struct bar : public foo { /* 16 | 4 */ int z; /* 20 | 2 */ short c; /* 22 | 2 */ short d; /* 24 | 2 */ short e; public: ~bar(); } /* total size: 32 bytes */ If we print the vtable, I think it would make sense to print it for the child class too. And if we do, it would also make sense to display the fields of base classes too, otherwise there would be a gap in the vtable offset and the first field offset. I think it would be useful in order to show the holes between the fields of the base class and the child class. In the example above, it would show that there's a two byte hole between b and z. Putting z at the end of bar could save some space. But doing so would require to change ptype's behavior significantly, making it recurse in parent classes. Should we do that only if /o is specified? If this makes the patch too complex, I would suggest merging it without the vtable field, and take at adding it after. We don't have to include all the features we can think of in the first iteration. For that use case, it's also interesting to see what pahole does too: struct bar : foo { /* struct foo ; */ /* 0 16 */ /* XXX last struct has 2 bytes of padding */ int z; /* 16 4 */ short int c; /* 20 2 */ short int d; /* 22 2 */ short int e; /* 24 2 */ void bar(class bar *, const class bar &); void bar(class bar *); virtual void ~bar(class bar *, int); /* size: 32, cachelines: 1, members: 5 */ /* padding: 6 */ /* paddings: 1, sum paddings: 2 */ /* last cacheline: 32 bytes */ /* BRAIN FART ALERT! 32 != 10 + 0(holes), diff = 22 */ }; I am not sure why it brain farts. Looks like it's comparing the size of the whole structure (including fields of the base class, 32 bytes) with the size of the fields of bar (10 bytes) and is surprised it's not the same size. > diff --git a/gdb/testsuite/gdb.base/ptype-offsets.exp b/gdb/testsuite/gdb.base/ptype-offsets.exp > new file mode 100644 > index 0000000000..04a887a703 > --- /dev/null > +++ 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 . > + > +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 > +} > + > +if { [prepare_for_testing "failed to prepare" $testfile $srcfile \ > + { debug c++ optimize=-O0 }] } { > + return -1 > +} > + > +# Test general offset printing, ctor/dtor printing, union, formatting. > +gdb_test "ptype /o struct abc" \ > + [multi_line \ > +"type = struct abc {" \ > +"/\\\* offset | size \\\*/" \ > +" public:" \ > +"/\\\* 8 | 8 \\\*/ void \\\*field1;" \ > +"/\\\* 16:31 | 4 \\\*/ unsigned int field2 : 1;" \ > +"/\\\* XXX 7-bit hole \\\*/" \ > +"/\\\* XXX 3-byte hole \\\*/" \ > +"/\\\* 20 | 4 \\\*/ int field3;" \ > +"/\\\* 24 | 1 \\\*/ char field4;" \ > +"/\\\* XXX 7-byte hole \\\*/" \ > +"/\\\* 32 | 8 \\\*/ uint64_t field5;" \ > +"/\\\* 40 | 8 \\\*/ union {" \ > +"/\\\* 8 \\\*/ void \\\*field6;" \ > +"/\\\* 4 \\\*/ int field7;" \ > +" } /\\\* total size: 8 bytes \\\*/ field8;" \ > +"" \ > +" abc\\(void\\);" \ > +" ~abc\\(\\);" \ > +"} /\\\* total size: 48 bytes \\\*/"] \ > + "ptype offset struct abc" > + > +# Test nested structs. > +gdb_test "ptype /o struct pqr" \ > + [multi_line \ > +"type = struct pqr {" \ > +"/\\\* offset | size \\\*/" \ > +"/\\\* 0 | 4 \\\*/ int f1;" \ > +"/\\\* XXX 4-byte hole \\\*/" \ > +"/\\\* 8 | 16 \\\*/ struct xyz {" \ > +"/\\\* 8 | 4 \\\*/ int f1;" \ > +"/\\\* 12 | 1 \\\*/ char f2;" \ > +"/\\\* XXX 3-byte hole \\\*/" \ > +"/\\\* 16 | 8 \\\*/ void \\\*f3;" \ > +"/\\\* 24 | 24 \\\*/ struct tuv {" \ > +"/\\\* 24 | 4 \\\*/ int a1;" \ > +"/\\\* XXX 4-byte hole \\\*/" \ > +"/\\\* 32 | 8 \\\*/ char *a2;" \ > +"/\\\* 40 | 4 \\\*/ int a3;" \ > +" } /\\\* total size: 24 bytes \\\*/ f4;" \ > +" } /\\\* total size: 40 bytes \\\*/ ff2;" \ > +"/\\\* 48 | 1 \\\*/ char ff3;" \ > +"} /\\\* total size: 56 bytes \\\*/"] \ > + "ptype offset struct pqr" > + > +# 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. > +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" > + > +# Test printing a struct that contains an union, and that also > +# contains a struct. > +gdb_test "ptype /o struct poi" \ > + [multi_line \ > +"/\\\* offset | size \\\*/" \ > +"/\\\* 0 | 4 \\\*/ int f1;" \ > +"/\\\* XXX 4-byte hole \\\*/" \ > +"/\\\* 8 | 40 \\\*/ union qwe {" \ > +"/\\\* 24 \\\*/ struct tuv {" \ > +"/\\\* 8 | 4 \\\*/ int a1;" \ > +"/\\\* XXX 4-byte hole \\\*/" \ > +"/\\\* 16 | 8 \\\*/ char \\\*a2;" \ > +"/\\\* 24 | 4 \\\*/ int a3;" \ > +" } /\\\* total size: 24 bytes \\\*/ fff1;" \ > +"/\\\* 40 \\\*/ struct xyz {" \ > +"/\\\* 8 | 4 \\\*/ int f1;" \ > +"/\\\* 12 | 1 \\\*/ char f2;" \ > +"/\\\* XXX 3-byte hole \\\*/" \ > +"/\\\* 16 | 8 \\\*/ void \\\*f3;" \ > +"/\\\* 24 | 24 \\\*/ struct tuv {" \ > +"/\\\* 24 | 4 \\\*/ int a1;" \ > +"/\\\* XXX 4-byte hole \\\*/" \ > +"/\\\* 32 | 8 \\\*/ char \\\*a2;" \ > +"/\\\* 40 | 4 \\\*/ int a3;" \ > +" } /\\\* total size: 24 bytes \\\*/ f4;" \ > +" } /\\\* total size: 40 bytes \\\*/ fff2;" \ > +" } /\\\* total size: 40 bytes \\\*/ f2;" \ > +"/\\\* 48 | 2 \\\*/ uint16_t f3;" \ > +"/\\\* XXX 6-byte hole */" \ > +"/\\\* 56 | 56 \\\*/ struct pqr {" \ > +"/\\\* 56 | 4 \\\*/ int ff1;" \ > +"/\\\* XXX 4-byte hole \\\*/" \ > +"/\\\* 64 | 40 \\\*/ struct xyz {" \ > +"/\\\* 64 | 4 \\\*/ int f1;" \ > +"/\\\* 68 | 1 \\\*/ char f2;" \ > +"/\\\* XXX 3-byte hole \\\*/" \ > +"/\\\* 72 | 8 \\\*/ void \\\*f3;" \ > +"/\\\* 80 | 24 \\\*/ struct tuv {" \ > +"/\\\* 80 | 4 \\\*/ int a1;" \ > +"/\\\* XXX 4-byte hole \\\*/" \ > +"/\\\* 88 | 8 \\\*/ char \\\*a2;" \ > +"/\\\* 96 | 4 \\\*/ int a3;" \ > +" } /\\\* total size: 24 bytes \\\*/ f4;" \ > +" } /\\\* total size: 40 bytes \\\*/ ff2;" \ > +"/\\\* 104 | 1 \\\*/ char ff3;" \ > +" } /\\\* total size: 56 bytes \\\*/ f4;" \ > +"} /\\\* total size: 112 bytes \\\*/"] \ > + "ptype offset struct poi" > + > +# Test printing a struct with several bitfields, laid out in various > +# ways. > +# > +# Because dealing with bitfields and offsets is difficult, it can be > +# tricky to confirm that the output of the this command is accurate. > +# A nice way to do that is to use GDB's "x" command and print the > +# actual memory layout of the struct. In order to differentiate > +# betweent bitfields and non-bitfield variables, one can assign "-1" "betweent" > +# to every bitfield in the struct. An example of the output of "x" > +# using "struct tyu" is: > +# > +# (gdb) x/24xb &e > +# 0x7fffffffd540: 0xff 0xff 0xff 0x1f 0x00 0x00 0x00 0x00 > +# 0x7fffffffd548: 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff > +# 0x7fffffffd550: 0xff 0x00 0x00 0x00 0x00 0x00 0x00 0x00 > +# > +# Thanks to Pedro Alves for coming up with this method (which can be > +# used to inspect the other cases, of course). I'm all for thanking Pedro, but I would suggest putting this last remark in the commit message :) Simon