[v5,2/2] Implement pahole-like 'ptype /o' option
Commit Message
This commit implements the pahole-like '/o' option for 'ptype', which
prints the offsets and sizes of struct fields, reporting whenever
there is a hole found.
The output is heavily based on pahole(1), with a few modifications
here and there to adjust it to our reality. Here's an example:
(gdb) ptype /o stap_probe
/* offset | size */
struct stap_probe {
/* 0 | 40 */ struct probe {
/* 0 | 8 */ const probe_ops *pops;
/* 8 | 8 */ gdbarch *arch;
/* 16 | 8 */ const char *name;
/* 24 | 8 */ const char *provider;
/* 32 | 8 */ CORE_ADDR address;
} /* total size: 40 bytes */ p;
/* 40 | 8 */ CORE_ADDR sem_addr;
/* 48:31 | 4 */ unsigned int args_parsed : 1;
/* XXX 7-bit hole */
/* XXX 7-byte hole */
/* 56 | 8 */ union {
/* 8 */ const char *text;
/* 8 */ VEC_stap_probe_arg_s *vec;
} /* total size: 8 bytes */ args_u;
} /* total size: 64 bytes */
A big part of this patch handles the formatting logic of 'ptype',
which is a bit messy. I tried to be not very invasive, but I had to
do some cleanups here and there to make life easier.
This patch is the start of a long-term work I'll do to flush the local
patches we carry for Fedora GDB. In this specific case, I'm aiming at
upstreaming the feature implemented by the 'pahole.py' script that is
shipped with Fedora GDB:
<https://src.fedoraproject.org/rpms/gdb/blob/master/f/gdb-archer.patch#_311>
This has been regression-tested on the BuildBot. There's a new
testcase for it, along with an update to the documentation. I also
thought it was worth mentioning this feature in the NEWS file.
gdb/ChangeLog:
yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
Pedro Alves <palves@redhat.com>
PR cli/16224
* NEWS (Changes since GDB 8.0): Mention new '/o' flag.
* c-typeprint.c (OFFSET_SPC_LEN): New define.
(print_spaces_filtered_with_print_options): New function.
(output_access_specifier): Take new argument FLAGS. Modify
function to call 'print_spaces_filtered_with_print_options'.
(c_print_type_vtable_offset_marker): New function.
(c_print_type_union_field_offset): New function.
(c_print_type_struct_field_offset): New function.
(c_type_print_base_struct_union): Print offsets and sizes for
struct/union/class fields.
* typeprint.c (const struct type_print_options
type_print_raw_options): Initialize 'print_offsets' and
'offset_bitpos'.
(static struct type_print_options default_ptype_flags):
Likewise.
(whatis_exp): Handle '/o' option.
(_initialize_typeprint): Add '/o' flag to ptype's help.
* typeprint.h (struct type_print_options) <print_offsets>: New
field.
<offset_bitpos>: Likewise.
gdb/testsuite/ChangeLog:
yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
PR cli/16224
* gdb.base/ptype-offsets.cc: New file.
* gdb.base/ptype-offsets.exp: New file.
gdb/doc/ChangeLog:
yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
PR cli/16224
* gdb.texinfo (ptype): Add documentation for new flag '/o'.
---
gdb/NEWS | 3 +
gdb/c-typeprint.c | 248 ++++++++++++++++++++++++++++---
gdb/doc/gdb.texinfo | 115 ++++++++++++++
gdb/testsuite/gdb.base/ptype-offsets.cc | 158 ++++++++++++++++++++
gdb/testsuite/gdb.base/ptype-offsets.exp | 192 ++++++++++++++++++++++++
gdb/typeprint.c | 15 +-
gdb/typeprint.h | 9 ++
7 files changed, 719 insertions(+), 21 deletions(-)
create mode 100644 gdb/testsuite/gdb.base/ptype-offsets.cc
create mode 100644 gdb/testsuite/gdb.base/ptype-offsets.exp
Comments
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 <ancestor>; */ /* 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 <http://www.gnu.org/licenses/>.
> +
> +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
> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Tom Tromey <tom@tromey.com>, Eli Zaretskii <eliz@gnu.org>, Simon Marchi <simon.marchi@ericsson.com>, Pedro Alves <palves@redhat.com>, Keith Seitz <keiths@redhat.com>, Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Tue, 12 Dec 2017 22:17:24 -0500
>
> This commit implements the pahole-like '/o' option for 'ptype', which
> +Issuing a @kbd{ptype /o struct tuv} would print:
^
Please insert "command" after the command, where I indicated.
Otherwise the sentence reads awkward.
> +Notice the format of the first column of comments. There, you can
> +find two parts separated by the @code{|} character: the @emph{offset},
^^^^^^^^
@samp, not @code.
The documentation parts are okay with these fixed.
Thanks.
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?
> +++ 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 <http://www.gnu.org/licenses/>.
> +
Please add an intro comment describing what this testcase is about.
> +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.
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
> + { debug c++ optimize=-O0 }] } {
> + return -1
> +}
Weren't you going to remove that optimize thing? :-)
> +# 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.)
> +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?
> @@ -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.
> + (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
[Sending user-interface suggestions separately from core
functionality review.]
On 12/13/2017 03:17 AM, Sergio Durigan Junior wrote:
> (gdb) ptype /o stap_probe
> /* offset | size */
> struct stap_probe {
> /* 0 | 40 */ struct probe {
> /* 0 | 8 */ const probe_ops *pops;
> /* 8 | 8 */ gdbarch *arch;
> /* 16 | 8 */ const char *name;
> /* 24 | 8 */ const char *provider;
> /* 32 | 8 */ CORE_ADDR address;
> } /* total size: 40 bytes */ p;
> /* 40 | 8 */ CORE_ADDR sem_addr;
> /* 48:31 | 4 */ unsigned int args_parsed : 1;
> /* XXX 7-bit hole */
> /* XXX 7-byte hole */
> /* 56 | 8 */ union {
> /* 8 */ const char *text;
> /* 8 */ VEC_stap_probe_arg_s *vec;
> } /* total size: 8 bytes */ args_u;
> } /* total size: 64 bytes */
I've experimented with (different versions of) of this patch
against gdb a bit, and I have to say that I'm finding myself
a bit confused by the currently produced output. It feels
like the output should be a bit more obvious and easier to
skim quickly.
For example, I was looking at gdb's minimal_symbol (what I had
used when I noticed the bitfield handling in earlier versions was
incorrect):
(top-gdb) ptype/tom minimal_symbol
/* offset | size */
type = struct minimal_symbol {
/* 0 | 32 */ struct general_symbol_info {
/* 0 | 8 */ const char *name;
/* 8 | 8 */ union {
/* 8 */ LONGEST ivalue;
/* 8 */ const block *block;
/* 8 */ const gdb_byte *bytes;
/* 8 */ CORE_ADDR address;
/* 8 */ const common_block *common_block;
/* 8 */ symbol *chain;
} /* total size: 8 bytes */ value;
/* 16 | 8 */ union {
/* 8 */ obstack *obstack;
/* 8 */ const char *demangled_name;
} /* total size: 8 bytes */ language_specific;
/* 24:27 | 4 */ language language : 5;
/* 24:26 | 4 */ unsigned int ada_mangled : 1;
/* XXX 2-bit hole */
/* XXX 1-byte hole */
/* 26 | 2 */ short section;
} /* total size: 32 bytes */ mginfo;
/* 32 | 8 */ unsigned long size;
/* 40 | 8 */ const char *filename;
/* 48:28 | 4 */ minimal_symbol_type type : 4;
/* 48:27 | 4 */ unsigned int created_by_gdb : 1;
/* 48:26 | 4 */ unsigned int target_flag_1 : 1;
/* 48:25 | 4 */ unsigned int target_flag_2 : 1;
/* 48:24 | 4 */ unsigned int has_size : 1;
/* XXX 7-byte hole */
/* 56 | 8 */ minimal_symbol *hash_next;
/* 64 | 8 */ minimal_symbol *demangled_hash_next;
} /* total size: 72 bytes */
(top-gdb)
and I have the following observations:
#1 - left vs right
We print the extra offset/size info on the leftside of the
screen, shifting the whole type to the right. If we were to put
the new info on the right, then a "ptype S" vs "ptype/o S" would
look mostly the same, except for the extra info that appears
on the right, and it'd map better to what you'd write in
actual code (who puts comments on the left side, right?)
pahole(1) prints the info on the right side. Was there a
particular reason gdb's version prints it on the left hand
side? Maybe the Python version did that too (did it? I haven't
tried it) because it'd be difficult with the Python
implementation otherwise?
But maybe it's just me. On to the next point in
such case.
#2 - shift "type =" header to the right as well?
Currently, the leading "type =" and ending "}" lines
are aligned on the left, along with the offset/size info.
I think shifting those to the right as well might make the
output nicer. The idea being that /o would split the
output in two more-clearly-separated columns, with the new info
panned hard on the left column, and old ptype info on
the right column.
I.e., currently we get:
~~~~~~~~~~~~~~~~~~~~~~~~~~~
(top-gdb) ptype/o minimal_symbol
/* offset | size */
type = struct minimal_symbol {
/* 0 | 32 */ struct general_symbol_info {
/* 0 | 8 */ const char *name;
...
} /* total size: 72 bytes */
~~~~~~~~~~~~~~~~~~~~~~~~~~~
And I'd propose instead:
~~~~~~~~~~~~~~~~~~~~~~~~~~~
(top-gdb) ptype/o minimal_symbol
/* offset | size */
type = struct minimal_symbol {
/* 0 | 32 */ struct general_symbol_info {
/* 0 | 8 */ const char *name;
...
} /* total size: 72 bytes */
~~~~~~~~~~~~~~~~~~~~~~~~~~~
Or even saving one line:
~~~~~~~~~~~~~~~~~~~~~~~~~~~
(top-gdb) ptype/o minimal_symbol
/* offset | size */ type = struct minimal_symbol {
/* 0 | 32 */ struct general_symbol_info {
/* 0 | 8 */ const char *name;
...
} /* total size: 72 bytes */
~~~~~~~~~~~~~~~~~~~~~~~~~~~
Or in full (compare to the version further above):
(top-gdb) ptype/o minimal_symbol
/* offset | size */ type = struct minimal_symbol {
/* 0 | 32 */ struct general_symbol_info {
/* 0 | 8 */ const char *name;
/* 8 | 8 */ union {
/* 8 */ LONGEST ivalue;
/* 8 */ const block *block;
/* 8 */ const gdb_byte *bytes;
/* 8 */ CORE_ADDR address;
/* 8 */ const common_block *common_block;
/* 8 */ symbol *chain;
} /* total size: 8 bytes */ value;
/* 16 | 8 */ union {
/* 8 */ obstack *obstack;
/* 8 */ const char *demangled_name;
} /* total size: 8 bytes */ language_specific;
/* 24:27 | 4 */ language language : 5;
/* 24:26 | 4 */ unsigned int ada_mangled : 1;
/* XXX 2-bit hole */
/* XXX 1-byte hole */
/* 26 | 2 */ short section;
} /* total size: 32 bytes */ mginfo;
/* 32 | 8 */ unsigned long size;
/* 40 | 8 */ const char *filename;
/* 48:28 | 4 */ minimal_symbol_type type : 4;
/* 48:27 | 4 */ unsigned int created_by_gdb : 1;
/* 48:26 | 4 */ unsigned int target_flag_1 : 1;
/* 48:25 | 4 */ unsigned int target_flag_2 : 1;
/* 48:24 | 4 */ unsigned int has_size : 1;
/* XXX 7-byte hole */
/* 56 | 8 */ minimal_symbol *hash_next;
/* 64 | 8 */ minimal_symbol *demangled_hash_next;
} /* total size: 72 bytes */
(top-gdb)
#3 - I also find the position of those
"/* total size: 8 bytes */" markers a bit hard
to grok/spot.
I'd suggest considering putting them on the left column
along with the rest of the offset/size info as well, like:
(top-gdb) ptype/tom minimal_symbol
/* offset | size */
type = struct minimal_symbol {
/* 0 | 32 */ struct general_symbol_info {
/* 0 | 8 */ const char *name;
/* 8 | 8 */ union {
/* 8 */ LONGEST ivalue;
/* 8 */ const block *block;
/* 8 */ const gdb_byte *bytes;
/* 8 */ CORE_ADDR address;
/* 8 */ const common_block *common_block;
/* 8 */ symbol *chain;
/* total size: 8 */ } value;
/* 16 | 8 */ union {
/* 8 */ obstack *obstack;
/* 8 */ const char *demangled_name;
/* total size: 8 */ } language_specific;
/* 24:27 | 4 */ language language : 5;
/* 24:26 | 4 */ unsigned int ada_mangled : 1;
/* XXX 2-bit hole */
/* XXX 1-byte hole */
/* 26 | 2 */ short section;
/* total size: 32 */ } mginfo;
/* 32 | 8 */ unsigned long size;
/* 40 | 8 */ const char *filename;
/* 48:28 | 4 */ minimal_symbol_type type : 4;
/* 48:27 | 4 */ unsigned int created_by_gdb : 1;
/* 48:26 | 4 */ unsigned int target_flag_1 : 1;
/* 48:25 | 4 */ unsigned int target_flag_2 : 1;
/* 48:24 | 4 */ unsigned int has_size : 1;
/* XXX 7-byte hole */
/* 56 | 8 */ minimal_symbol *hash_next;
/* 64 | 8 */ minimal_symbol *demangled_hash_next;
} /* total size: 72 bytes */
(top-gdb)
Or if you don't like it, consider putting that
"total size:" info on a separate line before the struct/union
is closed, like pahole does:
/* size: 128, cachelines: 2, members: 4 */
/* sum members: 124, holes: 1, sum holes: 4 */
};
That may be even better because it provides a place
to put extra info, like pahole does (cacheline info etc.).
Thanks,
Pedro Alves
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 <http://www.gnu.org/licenses/>.
>> +
>
> 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,
On Wednesday, December 13 2017, Eli Zaretskii wrote:
>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Cc: Tom Tromey <tom@tromey.com>, Eli Zaretskii <eliz@gnu.org>, Simon
>> Marchi <simon.marchi@ericsson.com>, Pedro Alves <palves@redhat.com>,
>> Keith Seitz <keiths@redhat.com>, Sergio Durigan Junior
>> <sergiodj@redhat.com>
>> Date: Tue, 12 Dec 2017 22:17:24 -0500
>>
>> This commit implements the pahole-like '/o' option for 'ptype', which
>> +Issuing a @kbd{ptype /o struct tuv} would print:
> ^
> Please insert "command" after the command, where I indicated.
> Otherwise the sentence reads awkward.
Done.
>> +Notice the format of the first column of comments. There, you can
>> +find two parts separated by the @code{|} character: the @emph{offset},
> ^^^^^^^^
> @samp, not @code.
Fixed.
> The documentation parts are okay with these fixed.
Thanks,
On Wednesday, December 13 2017, Pedro Alves wrote:
> [Sending user-interface suggestions separately from core
> functionality review.]
Thanks.
> On 12/13/2017 03:17 AM, Sergio Durigan Junior wrote:
>> (gdb) ptype /o stap_probe
>> /* offset | size */
>> struct stap_probe {
>> /* 0 | 40 */ struct probe {
>> /* 0 | 8 */ const probe_ops *pops;
>> /* 8 | 8 */ gdbarch *arch;
>> /* 16 | 8 */ const char *name;
>> /* 24 | 8 */ const char *provider;
>> /* 32 | 8 */ CORE_ADDR address;
>> } /* total size: 40 bytes */ p;
>> /* 40 | 8 */ CORE_ADDR sem_addr;
>> /* 48:31 | 4 */ unsigned int args_parsed : 1;
>> /* XXX 7-bit hole */
>> /* XXX 7-byte hole */
>> /* 56 | 8 */ union {
>> /* 8 */ const char *text;
>> /* 8 */ VEC_stap_probe_arg_s *vec;
>> } /* total size: 8 bytes */ args_u;
>> } /* total size: 64 bytes */
>
> I've experimented with (different versions of) of this patch
> against gdb a bit, and I have to say that I'm finding myself
> a bit confused by the currently produced output. It feels
> like the output should be a bit more obvious and easier to
> skim quickly.
>
> For example, I was looking at gdb's minimal_symbol (what I had
> used when I noticed the bitfield handling in earlier versions was
> incorrect):
>
> (top-gdb) ptype/tom minimal_symbol
> /* offset | size */
> type = struct minimal_symbol {
> /* 0 | 32 */ struct general_symbol_info {
> /* 0 | 8 */ const char *name;
> /* 8 | 8 */ union {
> /* 8 */ LONGEST ivalue;
> /* 8 */ const block *block;
> /* 8 */ const gdb_byte *bytes;
> /* 8 */ CORE_ADDR address;
> /* 8 */ const common_block *common_block;
> /* 8 */ symbol *chain;
> } /* total size: 8 bytes */ value;
> /* 16 | 8 */ union {
> /* 8 */ obstack *obstack;
> /* 8 */ const char *demangled_name;
> } /* total size: 8 bytes */ language_specific;
> /* 24:27 | 4 */ language language : 5;
> /* 24:26 | 4 */ unsigned int ada_mangled : 1;
> /* XXX 2-bit hole */
> /* XXX 1-byte hole */
> /* 26 | 2 */ short section;
> } /* total size: 32 bytes */ mginfo;
> /* 32 | 8 */ unsigned long size;
> /* 40 | 8 */ const char *filename;
> /* 48:28 | 4 */ minimal_symbol_type type : 4;
> /* 48:27 | 4 */ unsigned int created_by_gdb : 1;
> /* 48:26 | 4 */ unsigned int target_flag_1 : 1;
> /* 48:25 | 4 */ unsigned int target_flag_2 : 1;
> /* 48:24 | 4 */ unsigned int has_size : 1;
> /* XXX 7-byte hole */
> /* 56 | 8 */ minimal_symbol *hash_next;
> /* 64 | 8 */ minimal_symbol *demangled_hash_next;
> } /* total size: 72 bytes */
> (top-gdb)
>
> and I have the following observations:
>
> #1 - left vs right
>
> We print the extra offset/size info on the leftside of the
> screen, shifting the whole type to the right. If we were to put
> the new info on the right, then a "ptype S" vs "ptype/o S" would
> look mostly the same, except for the extra info that appears
> on the right, and it'd map better to what you'd write in
> actual code (who puts comments on the left side, right?)
> pahole(1) prints the info on the right side. Was there a
> particular reason gdb's version prints it on the left hand
> side? Maybe the Python version did that too (did it? I haven't
> tried it) because it'd be difficult with the Python
> implementation otherwise?
I agree, and when I started working on this feature the first plan was
to print things on the right. I even had a patch that did that. But
the main problem was that it was hard to properly align the /* */
blocks. For example, "ptype /o" prints nested structures, which means
that the we may print things that are aligned at level + 8, level + 12,
or even further. This would make the /* */ blocks be aligned
differently, which IMHO makes the output much harder to read. pahole
(the tool) solves that by printing nested structures separately, but if
we were to do that we'd need a bit refactorization of the current code.
About the Python version of pahole, it also prints the /* */ blocks on
the left side, though in an uglier way IMHO:
(gdb) pahole struct wer
struct wer {
/* 0 24 */ struct tuv {
/* 0 4 */ int a1
/* XXX 32 bit hole, try to pack */
/* 8 8 */ char * a2
/* 16 4 */ int a3
} tuv
/* 24 24 */ struct tyu {
/* 0 0 */ int a1
/* 0 0 */ int a2
/* 0 2 */ int a3
/* 3 0 */ char a4
/* XXX 35 bit hole, try to pack */
/* 8 8 */ long a5
/* 16 0 */ int a6
/* 16 0 */ long a7
} a1
}
> But maybe it's just me. On to the next point in
> such case.
>
> #2 - shift "type =" header to the right as well?
>
> Currently, the leading "type =" and ending "}" lines
> are aligned on the left, along with the offset/size info.
> I think shifting those to the right as well might make the
> output nicer. The idea being that /o would split the
> output in two more-clearly-separated columns, with the new info
> panned hard on the left column, and old ptype info on
> the right column.
>
> I.e., currently we get:
>
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (top-gdb) ptype/o minimal_symbol
> /* offset | size */
> type = struct minimal_symbol {
> /* 0 | 32 */ struct general_symbol_info {
> /* 0 | 8 */ const char *name;
> ...
> } /* total size: 72 bytes */
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> And I'd propose instead:
>
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (top-gdb) ptype/o minimal_symbol
> /* offset | size */
> type = struct minimal_symbol {
> /* 0 | 32 */ struct general_symbol_info {
> /* 0 | 8 */ const char *name;
> ...
> } /* total size: 72 bytes */
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Or even saving one line:
>
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (top-gdb) ptype/o minimal_symbol
> /* offset | size */ type = struct minimal_symbol {
> /* 0 | 32 */ struct general_symbol_info {
> /* 0 | 8 */ const char *name;
> ...
> } /* total size: 72 bytes */
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>
> Or in full (compare to the version further above):
>
> (top-gdb) ptype/o minimal_symbol
> /* offset | size */ type = struct minimal_symbol {
> /* 0 | 32 */ struct general_symbol_info {
> /* 0 | 8 */ const char *name;
> /* 8 | 8 */ union {
0> /* 8 */ LONGEST ivalue;
> /* 8 */ const block *block;
> /* 8 */ const gdb_byte *bytes;
> /* 8 */ CORE_ADDR address;
> /* 8 */ const common_block *common_block;
> /* 8 */ symbol *chain;
> } /* total size: 8 bytes */ value;
> /* 16 | 8 */ union {
> /* 8 */ obstack *obstack;
> /* 8 */ const char *demangled_name;
> } /* total size: 8 bytes */ language_specific;
> /* 24:27 | 4 */ language language : 5;
> /* 24:26 | 4 */ unsigned int ada_mangled : 1;
> /* XXX 2-bit hole */
> /* XXX 1-byte hole */
> /* 26 | 2 */ short section;
> } /* total size: 32 bytes */ mginfo;
> /* 32 | 8 */ unsigned long size;
> /* 40 | 8 */ const char *filename;
> /* 48:28 | 4 */ minimal_symbol_type type : 4;
> /* 48:27 | 4 */ unsigned int created_by_gdb : 1;
> /* 48:26 | 4 */ unsigned int target_flag_1 : 1;
> /* 48:25 | 4 */ unsigned int target_flag_2 : 1;
> /* 48:24 | 4 */ unsigned int has_size : 1;
> /* XXX 7-byte hole */
> /* 56 | 8 */ minimal_symbol *hash_next;
> /* 64 | 8 */ minimal_symbol *demangled_hash_next;
> } /* total size: 72 bytes */
> (top-gdb)
Yeah, this makes sense. I'll see about doing this.
> #3 - I also find the position of those
> "/* total size: 8 bytes */" markers a bit hard
> to grok/spot.
>
> I'd suggest considering putting them on the left column
> along with the rest of the offset/size info as well, like:
>
> (top-gdb) ptype/tom minimal_symbol
> /* offset | size */
> type = struct minimal_symbol {
> /* 0 | 32 */ struct general_symbol_info {
> /* 0 | 8 */ const char *name;
> /* 8 | 8 */ union {
> /* 8 */ LONGEST ivalue;
> /* 8 */ const block *block;
> /* 8 */ const gdb_byte *bytes;
> /* 8 */ CORE_ADDR address;
> /* 8 */ const common_block *common_block;
> /* 8 */ symbol *chain;
> /* total size: 8 */ } value;
> /* 16 | 8 */ union {
> /* 8 */ obstack *obstack;
> /* 8 */ const char *demangled_name;
> /* total size: 8 */ } language_specific;
> /* 24:27 | 4 */ language language : 5;
> /* 24:26 | 4 */ unsigned int ada_mangled : 1;
> /* XXX 2-bit hole */
> /* XXX 1-byte hole */
> /* 26 | 2 */ short section;
> /* total size: 32 */ } mginfo;
> /* 32 | 8 */ unsigned long size;
> /* 40 | 8 */ const char *filename;
> /* 48:28 | 4 */ minimal_symbol_type type : 4;
> /* 48:27 | 4 */ unsigned int created_by_gdb : 1;
> /* 48:26 | 4 */ unsigned int target_flag_1 : 1;
> /* 48:25 | 4 */ unsigned int target_flag_2 : 1;
> /* 48:24 | 4 */ unsigned int has_size : 1;
> /* XXX 7-byte hole */
> /* 56 | 8 */ minimal_symbol *hash_next;
> /* 64 | 8 */ minimal_symbol *demangled_hash_next;
> } /* total size: 72 bytes */
> (top-gdb)
>
> Or if you don't like it, consider putting that
> "total size:" info on a separate line before the struct/union
> is closed, like pahole does:
>
> /* size: 128, cachelines: 2, members: 4 */
> /* sum members: 124, holes: 1, sum holes: 4 */
> };
>
> That may be even better because it provides a place
> to put extra info, like pahole does (cacheline info etc.).
I like the idea of putting them on a separate line; not sure how hard
that will be. I'll take a look at implemeting this.
Thanks,
On Wednesday, December 13 2017, I wrote:
> On Wednesday, December 13 2017, Pedro Alves wrote:
>
>> 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.
It seems like the algorithm for calculating bitfield offsets is not
working correctly on BE machines. This is not only for "ptype /o", but
also for regular print commands. For example, consider this test:
struct tyu
{
int a1 : 1;
int a2 : 3;
int a3 : 23;
char a4 : 2;
int64_t a5;
int a6 : 5;
int64_t a7 : 3;
};
int
main (int argc, char *argv[])
{
struct tyu e;
e.a1 = e.a2 = e.a3 = e.a4 = e.a6 = e.a7 = -1;
return 0;
}
After stopping GDB at the "return 0;" line, here's what we see when we
print "e" on x86_64:
(gdb) p e
$1 = {a1 = -1, a2 = -1, a3 = -1, a4 = -1 '\377', a5 = 140737488344880, a6 = -1, a7 = -1}
While on PPC64BE:
(gdb) p e
$1 = {a1 = -1, a2 = 3, a3 = 3, a4 = 3 '\003', a5 = 70367536153528, a6 = -1, a7 = -1}
As for "ptype /o", the offsets printed on x86_64 and PPC64BE are the
same:
x86_64:
/* offset | size */ type = struct tyu {
/* 0:31 | 4 */ int a1 : 1;
/* 0:28 | 4 */ int a2 : 3;
/* 0: 5 | 4 */ int a3 : 23;
/* 3: 3 | 1 */ char a4 : 2;
/* XXX 3-bit hole */
/* XXX 4-byte hole */
/* 8 | 8 */ int64_t a5;
/* 16:27 | 4 */ int a6 : 5;
/* 16:56 | 8 */ int64_t a7 : 3;
/* total size (bytes): 24 */
}
PPC64BE:
/* offset | size */ type = struct tyu {
/* 0:31 | 4 */ int a1 : 1;
/* 0:28 | 4 */ int a2 : 3;
/* 0: 5 | 4 */ int a3 : 23;
/* 3: 3 | 1 */ char a4 : 2;
/* XXX 3-bit hole */
/* XXX 4-byte hole */
/* 8 | 8 */ int64_t a5;
/* 16:27 | 4 */ int a6 : 5;
/* 16:56 | 8 */ int64_t a7 : 3;
/* total size (bytes): 24 */
}
On 12/13/2017 08:36 PM, Sergio Durigan Junior wrote:
> On Wednesday, December 13 2017, I wrote:
>> OK, I'll confirm on PPC64BE.
Thanks.
>
> It seems like the algorithm for calculating bitfield offsets is not
> working correctly on BE machines. This is not only for "ptype /o", but
> also for regular print commands. For example, consider this test:
>
> struct tyu
> {
> int a1 : 1;
>
> int a2 : 3;
>
> int a3 : 23;
>
> char a4 : 2;
>
> int64_t a5;
>
> int a6 : 5;
>
> int64_t a7 : 3;
> };
>
> int
> main (int argc, char *argv[])
> {
> struct tyu e;
>
> e.a1 = e.a2 = e.a3 = e.a4 = e.a6 = e.a7 = -1;
>
> return 0;
> }
>
> After stopping GDB at the "return 0;" line, here's what we see when we
> print "e" on x86_64:
>
> (gdb) p e
> $1 = {a1 = -1, a2 = -1, a3 = -1, a4 = -1 '\377', a5 = 140737488344880, a6 = -1, a7 = -1}
>
> While on PPC64BE:
>
> (gdb) p e
> $1 = {a1 = -1, a2 = 3, a3 = 3, a4 = 3 '\003', a5 = 70367536153528, a6 = -1, a7 = -1}
>
You didn't initialize e.a5, so even the x86_64 version looks
wrong at first. You're seeing stack/register garbage in
the padding holes.
You should make that "e" a global to make sure all its
underlying bytes are clear, including padding. Or memset it.
The former is easier.
a2, a3 and a4 in the PPC64 version do look odd. Though
maybe that's something do to with the expression you used.
Does it make a difference if you initialize all fields
with separate statements, like:
e.a1 = -1;
e.a2 = -1;
etc.
> As for "ptype /o", the offsets printed on x86_64 and PPC64BE are the
> same:
So it sounds like we could remove the x86-64 check in the
testcase and let it run on all lp64 targets? Does it pass
cleanly on PPC64?
Thanks,
Pedro Alves
On 12/13/2017 09:22 PM, Pedro Alves wrote:
> On 12/13/2017 08:36 PM, Sergio Durigan Junior wrote:
>> On Wednesday, December 13 2017, I wrote:
>
>>> OK, I'll confirm on PPC64BE.
>
> Thanks.
>
>>
>> It seems like the algorithm for calculating bitfield offsets is not
>> working correctly on BE machines. This is not only for "ptype /o", but
>> also for regular print commands. For example, consider this test:
>>
>> struct tyu
>> {
>> int a1 : 1;
>>
>> int a2 : 3;
>>
>> int a3 : 23;
>>
>> char a4 : 2;
>>
>> int64_t a5;
>>
>> int a6 : 5;
>>
>> int64_t a7 : 3;
>> };
>>
>> int
>> main (int argc, char *argv[])
>> {
>> struct tyu e;
>>
>> e.a1 = e.a2 = e.a3 = e.a4 = e.a6 = e.a7 = -1;
>>
>> return 0;
>> }
>>
>> After stopping GDB at the "return 0;" line, here's what we see when we
>> print "e" on x86_64:
>>
>> (gdb) p e
>> $1 = {a1 = -1, a2 = -1, a3 = -1, a4 = -1 '\377', a5 = 140737488344880, a6 = -1, a7 = -1}
>>
>> While on PPC64BE:
>>
>> (gdb) p e
>> $1 = {a1 = -1, a2 = 3, a3 = 3, a4 = 3 '\003', a5 = 70367536153528, a6 = -1, a7 = -1}
>>
>
> You didn't initialize e.a5, so even the x86_64 version looks
> wrong at first. You're seeing stack/register garbage in
> the padding holes.
>
> You should make that "e" a global to make sure all its
> underlying bytes are clear, including padding. Or memset it.
> The former is easier.
>
> a2, a3 and a4 in the PPC64 version do look odd. Though
> maybe that's something do to with the expression you used.
>
> Does it make a difference if you initialize all fields
> with separate statements, like:
>
> e.a1 = -1;
> e.a2 = -1;
> etc.
Actually, I notice now that a4 is plain "char", not
"signed char" and that looks like is the issue.
Plain "char" is unsigned on PPC64. And then given an
expression like:
e.a1 = e.a2 = e.a3 = e.a4 ...
e.a4 ends up with an unsigned value (3). And so
a2 and a3 end up with the same value too (3), and then
a1 ends up "-1" anyway because it's a 1-bit field, i.e.,
no matter what you put there, it ends up being -1,
because all it fits in it is the sign bit.
Thanks,
Pedro Alves
On Wednesday, December 13 2017, Pedro Alves wrote:
> On 12/13/2017 08:36 PM, Sergio Durigan Junior wrote:
>> On Wednesday, December 13 2017, I wrote:
>
>>> OK, I'll confirm on PPC64BE.
>
> Thanks.
>
>>
>> It seems like the algorithm for calculating bitfield offsets is not
>> working correctly on BE machines. This is not only for "ptype /o", but
>> also for regular print commands. For example, consider this test:
>>
>> struct tyu
>> {
>> int a1 : 1;
>>
>> int a2 : 3;
>>
>> int a3 : 23;
>>
>> char a4 : 2;
>>
>> int64_t a5;
>>
>> int a6 : 5;
>>
>> int64_t a7 : 3;
>> };
>>
>> int
>> main (int argc, char *argv[])
>> {
>> struct tyu e;
>>
>> e.a1 = e.a2 = e.a3 = e.a4 = e.a6 = e.a7 = -1;
>>
>> return 0;
>> }
>>
>> After stopping GDB at the "return 0;" line, here's what we see when we
>> print "e" on x86_64:
>>
>> (gdb) p e
>> $1 = {a1 = -1, a2 = -1, a3 = -1, a4 = -1 '\377', a5 = 140737488344880, a6 = -1, a7 = -1}
>>
>> While on PPC64BE:
>>
>> (gdb) p e
>> $1 = {a1 = -1, a2 = 3, a3 = 3, a4 = 3 '\003', a5 = 70367536153528, a6 = -1, a7 = -1}
>>
>
> You didn't initialize e.a5, so even the x86_64 version looks
> wrong at first. You're seeing stack/register garbage in
> the padding holes.
I should have initialized e.a5 to 0 in order to make the problem easier
to spot. I did that now:
$1 = {a1 = -1, a2 = 3, a3 = 3, a4 = 3 '\003', a5 = 0, a6 = -1, a7 = -1}
> You should make that "e" a global to make sure all its
> underlying bytes are clear, including padding. Or memset it.
> The former is easier.
>
> a2, a3 and a4 in the PPC64 version do look odd. Though
> maybe that's something do to with the expression you used.
>
> Does it make a difference if you initialize all fields
> with separate statements, like:
>
> e.a1 = -1;
> e.a2 = -1;
> etc.
After memset'ing the variable to 0, and separating all assignments, I
get:
[sergiodj@gcc1-power7 build-gdb]$ g++ -g ptype-offsets.cc
ptype-offsets.cc: In function ‘int main(int, char**)’:
ptype-offsets.cc:170:8: warning: large integer implicitly truncated to unsigned type [-Woverflow]
e.a4 = -1;
^
$1 = {a1 = -1, a2 = -1, a3 = -1, a4 = 3 '\003', a5 = 0, a6 = -1, a7 = -1}
After changing the declaration of a4 to "signed char a4 : 2;":
$1 = {a1 = -1, a2 = -1, a3 = -1, a4 = -1 '\377', a5 = 0, a6 = -1, a7 = -1}
>> As for "ptype /o", the offsets printed on x86_64 and PPC64BE are the
>> same:
>
> So it sounds like we could remove the x86-64 check in the
> testcase and let it run on all lp64 targets? Does it pass
> cleanly on PPC64?
I'm checking this right now, because I have to readjust the test due to
the changes in the output format.
@@ -3,6 +3,9 @@
*** Changes since GDB 8.0
+* The 'ptype' command now accepts a '/o' flag, which prints the
+ offsets and sizes of fields in a struct, like the pahole(1) tool.
+
* New "--readnever" command line option instructs GDB to not read each
symbol file's symbolic debug information. This makes startup faster
but at the expense of not being able to perform symbolic debugging.
@@ -32,6 +32,14 @@
#include "cp-abi.h"
#include "cp-support.h"
+/* When printing the offsets of a struct and its fields (i.e., 'ptype
+ /o'; type_print_options::print_offsets), we use this many
+ characters when printing the offset information at the beginning of
+ the line. This is needed in order to generate the correct amount
+ of whitespaces when no offset info should be printed for a certain
+ field. */
+#define OFFSET_SPC_LEN 23
+
/* A list of access specifiers used for printing. */
enum access_specifier
@@ -836,21 +844,36 @@ c_type_print_template_args (const struct type_print_options *flags,
fputs_filtered (_("] "), stream);
}
+/* Use 'print_spaces_filtered', but take into consideration the
+ type_print_options FLAGS in order to determine how many whitespaces
+ will be printed. */
+
+static void
+print_spaces_filtered_with_print_options
+ (int level, struct ui_file *stream, const struct type_print_options *flags)
+{
+ if (!flags->print_offsets)
+ print_spaces_filtered (level, stream);
+ else
+ print_spaces_filtered (level + OFFSET_SPC_LEN, stream);
+}
+
/* Output an access specifier to STREAM, if needed. LAST_ACCESS is the
last access specifier output (typically returned by this function). */
static enum access_specifier
output_access_specifier (struct ui_file *stream,
enum access_specifier last_access,
- int level, bool is_protected, bool is_private)
+ int level, bool is_protected, bool is_private,
+ const struct type_print_options *flags)
{
if (is_protected)
{
if (last_access != s_protected)
{
last_access = s_protected;
- fprintfi_filtered (level + 2, stream,
- "protected:\n");
+ print_spaces_filtered_with_print_options (level + 2, stream, flags);
+ fprintf_filtered (stream, "protected:\n");
}
}
else if (is_private)
@@ -858,8 +881,8 @@ output_access_specifier (struct ui_file *stream,
if (last_access != s_private)
{
last_access = s_private;
- fprintfi_filtered (level + 2, stream,
- "private:\n");
+ print_spaces_filtered_with_print_options (level + 2, stream, flags);
+ fprintf_filtered (stream, "private:\n");
}
}
else
@@ -867,15 +890,132 @@ output_access_specifier (struct ui_file *stream,
if (last_access != s_public)
{
last_access = s_public;
- fprintfi_filtered (level + 2, stream,
- "public:\n");
+ print_spaces_filtered_with_print_options (level + 2, stream, flags);
+ fprintf_filtered (stream, "public:\n");
}
}
return last_access;
}
-/* Return true is an access label (i.e., "public:", "private:",
+static void
+c_print_type_vtable_offset_marker (struct type *type, unsigned int field_idx,
+ struct ui_file *stream,
+ unsigned int offset_bitpos)
+{
+ unsigned int bitpos = TYPE_FIELD_BITPOS (type, field_idx);
+ unsigned int offset = (bitpos + offset_bitpos) / TARGET_CHAR_BIT;
+
+ fprintf_filtered (stream, "/* %4u | %4u */ /* vtable */\n", offset,
+ TYPE_LENGTH (TYPE_FIELD_TYPE (type, field_idx)));
+}
+
+/* Print information about field at index FIELD_IDX of the union type
+ TYPE. Since union fields don't have the concept of offsets, we
+ just print their sizes.
+
+ The output is strongly based on pahole(1). */
+
+static void
+c_print_type_union_field_offset (struct type *type, unsigned int field_idx,
+ struct ui_file *stream)
+{
+ struct type *ftype = check_typedef (TYPE_FIELD_TYPE (type, field_idx));
+
+ fprintf_filtered (stream, "/* %4u */", TYPE_LENGTH (ftype));
+}
+
+/* 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;
+ }
+ else
+ {
+ /* The position of the field, relative to the beginning of the
+ struct. */
+ fprintf_filtered (stream, "/* %4u",
+ (bitpos + offset_bitpos) / TARGET_CHAR_BIT);
+
+ fprintf_filtered (stream, " ");
+ }
+
+ fprintf_filtered (stream, " | %4u */", fieldsize_byte);
+
+ *endpos = bitpos + fieldsize_bit;
+}
+
+/* Return true if an access label (i.e., "public:", "private:",
"protected:") needs to be printed for TYPE. */
static bool
@@ -1032,6 +1172,8 @@ c_type_print_base_struct_union (struct type *type, struct ui_file *stream,
int len = TYPE_NFIELDS (type);
vptr_fieldno = get_vptr_fieldno (type, &basetype);
+ unsigned int endpos = 0;
+
for (int i = TYPE_N_BASECLASSES (type); i < len; i++)
{
QUIT;
@@ -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;
+ }
if (need_access_label)
{
section_type = output_access_specifier
(stream, section_type, level,
TYPE_FIELD_PROTECTED (type, i),
- TYPE_FIELD_PRIVATE (type, i));
+ TYPE_FIELD_PRIVATE (type, i), flags);
+ }
+
+ bool is_static = field_is_static (&TYPE_FIELD (type, i));
+
+ if (flags->print_offsets)
+ {
+ if (!is_static)
+ {
+ if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
+ c_print_type_struct_field_offset (type, i, &endpos, stream,
+ flags->offset_bitpos);
+ else if (TYPE_CODE (type) == TYPE_CODE_UNION)
+ c_print_type_union_field_offset (type, i, stream);
+ }
+ else
+ print_spaces_filtered (OFFSET_SPC_LEN, stream);
}
print_spaces_filtered (level + 4, stream);
- if (field_is_static (&TYPE_FIELD (type, i)))
+ if (is_static)
fprintf_filtered (stream, "static ");
+
+ int newshow = show - 1;
+
+ if (flags->print_offsets
+ && (TYPE_CODE (TYPE_FIELD_TYPE (type, i)) == TYPE_CODE_STRUCT
+ || TYPE_CODE (TYPE_FIELD_TYPE (type, i)) == TYPE_CODE_UNION))
+ {
+ /* If we're printing offsets and this field's type is
+ either a struct or an union, then we're interested in
+ expanding it. */
+ ++newshow;
+
+ /* Make sure we carry our offset when we expand the
+ struct. */
+ local_flags.offset_bitpos
+ = flags->offset_bitpos + TYPE_FIELD_BITPOS (type, i);
+ }
+
c_print_type (TYPE_FIELD_TYPE (type, i),
TYPE_FIELD_NAME (type, i),
- stream, show - 1, level + 4,
+ stream, newshow, level + 4,
&local_flags);
- if (!field_is_static (&TYPE_FIELD (type, i))
- && TYPE_FIELD_PACKED (type, i))
+ if (!is_static && TYPE_FIELD_PACKED (type, i))
{
/* It is a bitfield. This code does not attempt
to look at the bitpos and reconstruct filler,
@@ -1122,9 +1302,10 @@ c_type_print_base_struct_union (struct type *type, struct ui_file *stream,
section_type = output_access_specifier
(stream, section_type, level,
TYPE_FN_FIELD_PROTECTED (f, j),
- TYPE_FN_FIELD_PRIVATE (f, j));
+ TYPE_FN_FIELD_PRIVATE (f, j), flags);
- print_spaces_filtered (level + 4, stream);
+ print_spaces_filtered_with_print_options (level + 4, stream,
+ flags);
if (TYPE_FN_FIELD_VIRTUAL_P (f, j))
fprintf_filtered (stream, "virtual ");
else if (TYPE_FN_FIELD_STATIC_P (f, j))
@@ -1143,9 +1324,16 @@ c_type_print_base_struct_union (struct type *type, struct ui_file *stream,
&& !is_full_physname_constructor /* " " */
&& !is_type_conversion_operator (type, i, j))
{
+ unsigned int old_po = local_flags.print_offsets;
+
+ /* Temporarily disable print_offsets, because it
+ would mess with indentation. */
+ local_flags.print_offsets = 0;
c_print_type (TYPE_TARGET_TYPE (TYPE_FN_FIELD_TYPE (f, j)),
"", stream, -1, 0,
&local_flags);
+ local_flags.print_offsets = old_po;
+
fputs_filtered (" ", stream);
}
if (TYPE_FN_FIELD_STUB (f, j))
@@ -1226,9 +1414,16 @@ c_type_print_base_struct_union (struct type *type, struct ui_file *stream,
for (int i = 0; i < TYPE_NESTED_TYPES_COUNT (type); ++i)
{
- print_spaces_filtered (level + 4, stream);
+ unsigned int old_po = semi_local_flags.print_offsets;
+
+ /* Temporarily disable print_offsets, because it
+ would mess with indentation. */
+ print_spaces_filtered_with_print_options (level + 4, stream,
+ flags);
+ semi_local_flags.print_offsets = 0;
c_print_type (TYPE_NESTED_TYPES_FIELD_TYPE (type, i),
"", stream, show, level + 4, &semi_local_flags);
+ semi_local_flags.print_offsets = old_po;
fprintf_filtered (stream, ";\n");
}
}
@@ -1254,11 +1449,17 @@ c_type_print_base_struct_union (struct type *type, struct ui_file *stream,
section_type = output_access_specifier
(stream, section_type, level,
TYPE_TYPEDEF_FIELD_PROTECTED (type, i),
- TYPE_TYPEDEF_FIELD_PRIVATE (type, i));
+ TYPE_TYPEDEF_FIELD_PRIVATE (type, i), flags);
}
- print_spaces_filtered (level + 4, stream);
+ print_spaces_filtered_with_print_options (level + 4, stream,
+ flags);
fprintf_filtered (stream, "typedef ");
+ unsigned int old_po = semi_local_flags.print_offsets;
+ /* Temporarily disable print_offsets, because it
+ would mess with indentation. */
+ semi_local_flags.print_offsets = 0;
+
/* We want to print typedefs with substitutions
from the template parameters or globally-known
typedefs but not local typedefs. */
@@ -1266,13 +1467,21 @@ c_type_print_base_struct_union (struct type *type, struct ui_file *stream,
TYPE_TYPEDEF_FIELD_NAME (type, i),
stream, show - 1, level + 4,
&semi_local_flags);
+ semi_local_flags.print_offsets = old_po;
fprintf_filtered (stream, ";\n");
}
}
+ if (flags->print_offsets && level > 0)
+ print_spaces_filtered (OFFSET_SPC_LEN, stream);
+
fprintfi_filtered (level, stream, "}");
}
+ if (show > 0 && flags->print_offsets)
+ fprintf_filtered (stream, " /* total size: %4u bytes */",
+ TYPE_LENGTH (type));
+
do_cleanups (local_cleanups);
}
@@ -1300,7 +1509,6 @@ c_type_print_base (struct type *type, struct ui_file *stream,
{
int i;
int len;
- int j;
QUIT;
@@ -17211,6 +17211,121 @@ names are substituted when printing other types.
@item T
Print typedefs defined in the class. This is the default, but the flag
exists in case you change the default with @command{set print type typedefs}.
+
+@item o
+Print the offsets and sizes of fields in a struct, similar to what the
+@command{pahole} tool does.
+
+For example, given the following declarations:
+
+@smallexample
+struct tuv
+@{
+ int a1;
+ char *a2;
+ int a3;
+@};
+
+struct xyz
+@{
+ int f1;
+ char f2;
+ void *f3;
+ struct tuv f4;
+@};
+
+union qwe
+@{
+ struct tuv fff1;
+ struct xyz fff2;
+@};
+
+struct tyu
+@{
+ int a1 : 1;
+ int a2 : 3;
+ int a3 : 23;
+ char a4 : 2;
+ int64_t a5;
+ int a6 : 5;
+ int64_t a7 : 3;
+@};
+@end smallexample
+
+Issuing a @kbd{ptype /o struct tuv} 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
+
+Notice the format of the first column of comments. There, you can
+find two parts separated by the @code{|} character: the @emph{offset},
+which indicates where the field is located inside the struct, in
+bytes, and the @emph{size} of the field. Another interesting line is
+the marker of a @emph{hole} in the struct, indicating that it may be
+possible to pack the struct and make it use less space by reorganizing
+its fields.
+
+It is also possible to print offsets inside an union:
+
+@smallexample
+(@value{GDBP}) ptype /o union qwe
+/* offset | size */
+union qwe @{
+/* 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 */
+@end smallexample
+
+In this case, since @code{struct tuv} and @code{struct xyz} occupy the
+same space (because we are dealing with an union), the offset is not
+printed for them. However, you can still examine the offset of each
+of these structures' fields.
+
+Another useful scenario is printing the offsets of a struct containing
+bitfields:
+
+@smallexample
+(@value{GDBP}) ptype /o struct tyu
+/* offset | size */
+struct tyu @{
+/* 0:31 | 4 */ int a1 : 1;
+/* 0:28 | 4 */ int a2 : 3;
+/* 0: 5 | 4 */ int a3 : 23;
+/* 3: 3 | 1 */ char a4 : 2;
+/* XXX 3-bit hole */
+/* XXX 4-byte hole */
+/* 8 | 8 */ int64_t a5;
+/* 16:27 | 4 */ int a6 : 5;
+/* 16:56 | 8 */ int64_t a7 : 3;
+@} /* total size: 24 bytes */
+@end smallexample
+
+Note how the offset information is now extended to also include how
+many bits are left to be used in each bitfield.
@end table
@kindex ptype
new file mode 100644
@@ -0,0 +1,158 @@
+/* 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 <http://www.gnu.org/licenses/>. */
+
+/* This file will be used to test 'ptype /o' on x86_64 only. */
+
+#include <stdint.h>
+
+/* A struct with many types of fields, in order to test 'ptype
+ /o'. */
+
+struct abc
+{
+ /* Virtual destructor. */
+ virtual ~abc ()
+ {}
+
+ /* 8-byte address. Because of the virtual destructor above, this
+ field's offset will be 8. */
+ void *field1;
+
+ /* No hole here. */
+
+ /* 4-byte int bitfield of 1-bit. */
+ unsigned int field2 : 1;
+
+ /* 31-bit hole here. */
+
+ /* 4-byte int. */
+ int field3;
+
+ /* No hole here. */
+
+ /* 1-byte char. */
+ char field4;
+
+ /* 7-byte hole here. */
+
+ /* 8-byte int. */
+ uint64_t field5;
+
+ /* We just print the offset and size of a union, ignoring its
+ fields. */
+ union
+ {
+ /* 8-byte address. */
+ void *field6;
+
+ /* 4-byte int. */
+ int field7;
+ } field8;
+
+ /* Empty constructor. */
+ abc ()
+ {}
+};
+
+/* This struct will be nested inside 'struct xyz'. */
+
+struct tuv
+{
+ int a1;
+
+ char *a2;
+
+ int a3;
+};
+
+/* This struct will be nested inside 'struct pqr'. */
+
+struct xyz
+{
+ int f1;
+
+ char f2;
+
+ void *f3;
+
+ struct tuv f4;
+};
+
+/* A struct with a nested struct. */
+
+struct pqr
+{
+ int ff1;
+
+ struct xyz ff2;
+
+ char ff3;
+};
+
+/* A union with two nested structs. */
+
+union qwe
+{
+ struct tuv fff1;
+
+ struct xyz fff2;
+};
+
+/* 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;
+};
+
+int
+main (int argc, char *argv[])
+{
+ struct abc foo;
+ struct pqr bar;
+ union qwe c;
+ struct poi d;
+ struct tyu e;
+ uint8_t i;
+
+ return 0;
+}
new file mode 100644
@@ -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 <http://www.gnu.org/licenses/>.
+
+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"
+# 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).
+gdb_test "ptype /o struct tyu" \
+ [multi_line \
+"/\\\* offset | size \\\*/" \
+"struct tyu {" \
+"/\\\* 0:31 | 4 \\\*/ int a1 : 1;" \
+"/\\\* 0:28 | 4 \\\*/ int a2 : 3;" \
+"/\\\* 0: 5 | 4 \\\*/ int a3 : 23;" \
+"/\\\* 3: 3 | 1 \\\*/ char a4 : 2;" \
+"/\\\* XXX 3-bit hole \\\*/" \
+"/\\\* XXX 4-byte hole \\\*/" \
+"/\\\* 8 | 8 \\\*/ int64_t a5;" \
+"/\\\* 16:27 | 4 \\\*/ int a6 : 5;" \
+"/\\\* 16:56 | 8 \\\*/ int64_t a7 : 3;" \
+"} /\\\* total size: 24 bytes \\\*/"] \
+ "ptype offset struct tyu"
+
+# Test that we don't print any header when issuing a "ptype /o" on a
+# non-struct, non-union, non-class type.
+gdb_test "ptype /o int" "int" "ptype offset int"
+gdb_test "ptype /o uint8_t" "char" "ptype offset uint8_t"
@@ -42,6 +42,8 @@ const struct type_print_options type_print_raw_options =
1, /* raw */
1, /* print_methods */
1, /* print_typedefs */
+ 0, /* print_offsets */
+ 0, /* offset_bitpos */
0, /* print_nested_type_limit */
NULL, /* local_typedefs */
NULL, /* global_table */
@@ -55,6 +57,8 @@ static struct type_print_options default_ptype_flags =
0, /* raw */
1, /* print_methods */
1, /* print_typedefs */
+ 0, /* print_offsets */
+ 0, /* offset_bitpos */
0, /* print_nested_type_limit */
NULL, /* local_typedefs */
NULL, /* global_table */
@@ -440,6 +444,9 @@ whatis_exp (const char *exp, int show)
case 'T':
flags.print_typedefs = 1;
break;
+ case 'o':
+ flags.print_offsets = 1;
+ break;
default:
error (_("unrecognized flag '%c'"), *exp);
}
@@ -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 &&
+ (TYPE_CODE (type) == TYPE_CODE_STRUCT
+ || TYPE_CODE (type) == TYPE_CODE_UNION))
+ fprintf_filtered (gdb_stdout, "/* offset | size */\n");
+
printf_filtered ("type = ");
if (!flags.raw)
@@ -759,7 +771,8 @@ Available FLAGS are:\n\
/m do not print methods defined in a class\n\
/M print methods defined in a class\n\
/t do not print typedefs defined in a class\n\
- /T print typedefs defined in a class"));
+ /T print typedefs defined in a class\n\
+ /o print offsets and sizes of fields in a struct (like pahole)\n"));
set_cmd_completer (c, expression_completer);
c = add_com ("whatis", class_vars, whatis_command,
@@ -35,6 +35,15 @@ struct type_print_options
/* True means print typedefs in a class. */
unsigned int print_typedefs : 1;
+ /* True means to print offsets, a la 'pahole'. */
+ unsigned int print_offsets : 1;
+
+ /* 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;
+
/* The number of nested type definitions to print. -1 == all. */
int print_nested_type_limit;