This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v5 2/2] Implement pahole-like 'ptype /o' option


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]