[PATCH 0/2] two ptype/o changes

[PATCH 0/2] two ptype/o changes

[Tom Tromey]

This series changes ptype/o in a couple of ways.

Last week I spent quite some time puzzling over the meaning of the
'offset' for a bitfield in ptype/o output.  After talking with Sergio
on irc, I ended up concluding that gdb should instead print the bit
offset, not the number of bits remaining in the field's allocation.

That is patch #2.  Then, I noticed that the tests did not fail,
despite the output changing.  It turns out the tests weren't working
correctly, so I wrote patch #1.

Tested on x86-64 Fedora 29.  Let me know what you think.

Patch #2 includes a doc change.

Tom

[PATCH 2/2] Change ptype/o to print bit offset

[Tom Tromey]

Consider this short C example:

    struct inner
    {
      unsigned x;
      unsigned y : 3;
      unsigned z : 3;
    };

    struct outer
    {
      unsigned char o : 3;
      struct inner i __attribute__ ((packed));
    };

When I use "ptype/o" on this, I get:

    (gdb) ptype/o struct outer
    /* offset    |  size */  type = struct outer {
    /*    0: 5   |     1 */    unsigned char o : 3;
    /* XXX  5-bit hole  */
    /*    1      |     8 */    struct inner {
    /*    1      |     4 */        unsigned int x;
    /*    5:29   |     4 */        unsigned int y : 3;
    /*    5:26   |     4 */        unsigned int z : 3;
    /* XXX  2-bit padding  */
    /* XXX  3-byte padding */

				   /* total size (bytes):    8 */
			       } i;

			       /* total size (bytes):    9 */
			     }

In the location of "o" ("0: 5"), the "5" means "there are 5 bits left
relative to the size of the underlying type.

I find this very difficult to follow.  On irc, Sergio said that this
choice came because it is what pahole does.  However, I think it's not
very useful, and maybe is just an artifact of the way that
DW_AT_bit_offset was defined in DWARF 3.

This patch changes ptype/o to print the offset of a bitfield in a more
natural way, that is, using the bit number according to the platform's
bit numbering.

With this patch, the output is now:

    (gdb) ptype/o struct outer
    /* offset    |  size */  type = struct outer {
    /*    0: 0   |     1 */    unsigned char o : 3;
    /* XXX  5-bit hole  */
    /*    1      |     8 */    struct inner {
    /*    1      |     4 */        unsigned int x;
    /*    5: 0   |     4 */        unsigned int y : 3;
    /*    5: 3   |     4 */        unsigned int z : 3;
    /* XXX  2-bit padding  */
    /* XXX  3-byte padding */

				   /* total size (bytes):    8 */
			       } i;

			       /* total size (bytes):    9 */
			     }

This is better, IMO, because now the "offset" of a bitfield is
consistent with the offset of an ordinary member, referring to its
offset from the start of the structure.

gdb/ChangeLog
2019-04-29  Tom Tromey  <tromey@adacore.com>

	* typeprint.c (print_offset_data::update): Print the bit offset,
	not the number of bits remaining.

gdb/doc/ChangeLog
2019-04-29  Tom Tromey  <tromey@adacore.com>

	* gdb.texinfo (Symbols): Document change to ptype/o.

gdb/testsuite/ChangeLog
2019-04-29  Tom Tromey  <tromey@adacore.com>

	* gdb.base/ptype-offsets.exp: Update tests.
---
 gdb/ChangeLog                            |  5 ++++
 gdb/doc/ChangeLog                        |  4 ++++
 gdb/doc/gdb.texinfo                      |  9 +++----
 gdb/testsuite/ChangeLog                  |  4 ++++
 gdb/testsuite/gdb.base/ptype-offsets.exp | 30 ++++++++++++------------
 gdb/typeprint.c                          | 30 ++++++------------------
 6 files changed, 40 insertions(+), 42 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index cf8333d86be..83528a3385e 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -17803,15 +17803,16 @@ bitfields:
 /* 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;
+/*   16: 0   |     4 */    int a6 : 5;
+/*   16: 5   |     8 */    int64_t a7 : 3;
+"/* XXX  7-byte padding  */
 
                            /* total size (bytes):   24 */
                          @}
 @end smallexample
 
-Note how the offset information is now extended to also include how
-many bits are left to be used in each bitfield.
+Note how the offset information is now extended to also include the
+first bit of the bitfield.
 @end table
 
 @kindex ptype
diff --git a/gdb/testsuite/gdb.base/ptype-offsets.exp b/gdb/testsuite/gdb.base/ptype-offsets.exp
index 12b3a746005..6116520bbd7 100644
--- a/gdb/testsuite/gdb.base/ptype-offsets.exp
+++ b/gdb/testsuite/gdb.base/ptype-offsets.exp
@@ -38,7 +38,7 @@ gdb_test "ptype /o struct abc" \
 "/* offset    |  size */  type = struct abc \{" \
 "                         public:" \
 "/*    8      |     8 */    void *field1;" \
-"/*   16:31   |     4 */    unsigned int field2 : 1;" \
+"/*   16: 0   |     4 */    unsigned int field2 : 1;" \
 "/* XXX  7-bit hole   */" \
 "/* XXX  3-byte hole  */" \
 "/*   20      |     4 */    int field3;" \
@@ -63,7 +63,7 @@ gdb_test "ptype /oTM struct abc" \
 "/* offset    |  size */  type = struct abc \{" \
 "                         public:" \
 "/*    8      |     8 */    void *field1;" \
-"/*   16:31   |     4 */    unsigned int field2 : 1;" \
+"/*   16: 0   |     4 */    unsigned int field2 : 1;" \
 "/* XXX  7-bit hole   */" \
 "/* XXX  3-byte hole  */" \
 "/*   20      |     4 */    int field3;" \
@@ -93,7 +93,7 @@ gdb_test "ptype /TMo struct abc" \
 "/* offset    |  size */  type = struct abc \{" \
 "                         public:" \
 "/*    8      |     8 */    void *field1;" \
-"/*   16:31   |     4 */    unsigned int field2 : 1;" \
+"/*   16: 0   |     4 */    unsigned int field2 : 1;" \
 "/* XXX  7-bit hole   */" \
 "/* XXX  3-byte hole  */" \
 "/*   20      |     4 */    int field3;" \
@@ -264,15 +264,15 @@ gdb_test "ptype /o struct poi" \
 gdb_test "ptype /o struct tyu" \
     [string_to_regexp [multi_line \
 "/* offset    |  size */  type = struct tyu \{" \
-"/*    0:31   |     4 */    int a1 : 1;" \
-"/*    0:28   |     4 */    int a2 : 3;" \
-"/*    0: 5   |     4 */    int a3 : 23;" \
+"/*    0: 0   |     4 */    int a1 : 1;" \
+"/*    0: 1   |     4 */    int a2 : 3;" \
+"/*    0: 4   |     4 */    int a3 : 23;" \
 "/*    3: 3   |     1 */    signed 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;" \
+"/*   16: 0   |     4 */    int a6 : 5;" \
+"/*   16: 5   |     8 */    int64_t a7 : 3;" \
 "/* XXX  7-byte padding  */" \
 "" \
 "                           /* total size (bytes):   24 */" \
@@ -293,8 +293,8 @@ gdb_test "ptype /o struct asd" \
 "" \
 "                                   /* total size (bytes):    8 */" \
 "                               \} f3;" \
-"/*   24:27   |     4 */        int f4 : 5;" \
-"/*   24:26   |     4 */        unsigned int f5 : 1;" \
+"/*   24: 0   |     4 */        int f4 : 5;" \
+"/*   24: 5   |     4 */        unsigned int f5 : 1;" \
 "/* XXX  2-bit hole   */" \
 "/* XXX  1-byte hole  */" \
 "/*   26      |     2 */        short f6;" \
@@ -304,11 +304,11 @@ gdb_test "ptype /o struct asd" \
 "                           \} f7;" \
 "/*   32      |     8 */    unsigned long f8;" \
 "/*   40      |     8 */    signed char *f9;" \
-"/*   48:28   |     4 */    int f10 : 4;" \
-"/*   48:27   |     4 */    unsigned int f11 : 1;" \
-"/*   48:26   |     4 */    unsigned int f12 : 1;" \
-"/*   48:25   |     4 */    unsigned int f13 : 1;" \
-"/*   48:24   |     4 */    unsigned int f14 : 1;" \
+"/*   48: 0   |     4 */    int f10 : 4;" \
+"/*   48: 4   |     4 */    unsigned int f11 : 1;" \
+"/*   48: 5   |     4 */    unsigned int f12 : 1;" \
+"/*   48: 6   |     4 */    unsigned int f13 : 1;" \
+"/*   48: 7   |     4 */    unsigned int f14 : 1;" \
 "/* XXX  7-byte hole  */" \
 "/*   56      |     8 */    void *f15;" \
 "/*   64      |     8 */    void *f16;" \
diff --git a/gdb/typeprint.c b/gdb/typeprint.c
index d1cdfe11cc0..7a44dbdb313 100644
--- a/gdb/typeprint.c
+++ b/gdb/typeprint.c
@@ -131,32 +131,16 @@ print_offset_data::update (struct type *type, unsigned int field_idx,
 
   maybe_print_hole (stream, bitpos, "hole");
 
-  if (TYPE_FIELD_PACKED (type, field_idx))
+  if (TYPE_FIELD_PACKED (type, field_idx)
+      || offset_bitpos % TARGET_CHAR_BIT != 0)
     {
-      /* 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;
+      /* We're dealing with a bitfield.  Print the bit offset.  */
+      fieldsize_bit = TYPE_FIELD_BITSIZE (type, field_idx);
 
-      /* This is the exact offset (in bits) of this bitfield.  */
-      unsigned int bit_offset
-	= (bitpos - relative_bitpos) + offset_bitpos;
+      unsigned real_bitpos = 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;
+      fprintf_filtered (stream, "/* %4u:%2u", real_bitpos / TARGET_CHAR_BIT,
+			real_bitpos % TARGET_CHAR_BIT);
     }
   else
     {
-- 
2.20.1

Re: [PATCH 2/2] Change ptype/o to print bit offset

[Eli Zaretskii]

> From: Tom Tromey <tromey@adacore.com>
> Cc: Tom Tromey <tromey@adacore.com>
> Date: Mon, 29 Apr 2019 12:31:05 -0600
> 
> With this patch, the output is now:
> 
>     (gdb) ptype/o struct outer
>     /* offset    |  size */  type = struct outer {
>     /*    0: 0   |     1 */    unsigned char o : 3;
>     /* XXX  5-bit hole  */
>     /*    1      |     8 */    struct inner {
>     /*    1      |     4 */        unsigned int x;
>     /*    5: 0   |     4 */        unsigned int y : 3;
>     /*    5: 3   |     4 */        unsigned int z : 3;
>     /* XXX  2-bit padding  */
>     /* XXX  3-byte padding */

This loses information, because now the bits part is just a trivial
conversion of the field size in the declaration.  The current display
shows something that cannot be trivially gleaned by looking at the
bitfield sizes.

I'm not objecting to the change, I'm just saying we lose something
here.

> gdb/doc/ChangeLog
> 2019-04-29  Tom Tromey  <tromey@adacore.com>
> 
> 	* gdb.texinfo (Symbols): Document change to ptype/o.

This seems to be a mechanical change, so OK.

Do we need to call this out in NEWS?

Thanks.

Re: [PATCH 2/2] Change ptype/o to print bit offset

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

>> /*    5: 0   |     4 */        unsigned int y : 3;
>> /*    5: 3   |     4 */        unsigned int z : 3;
>> /* XXX  2-bit padding  */
>> /* XXX  3-byte padding */

Eli> This loses information, because now the bits part is just a trivial
Eli> conversion of the field size in the declaration.  The current display
Eli> shows something that cannot be trivially gleaned by looking at the
Eli> bitfield sizes.

Eli> I'm not objecting to the change, I'm just saying we lose something
Eli> here.

We don't actually lose anything here, because the padding is spelled out
explicitly in the comments that are printed.


Also, consider an example like this, from an Ada test case:

/* offset    |  size */  type = struct aggregates__nested_packed {
/*    0: 5   |     1 */    <range type> q000 : 3;
/*    0:23   |     8 */    struct aggregates__packed_rec {
/*    0      |     4 */        integer packed_array_assign_w;
/*    4: 5   |     1 */        <range type> packed_array_assign_x : 3;
/*    4: 2   |     1 */        <range type> packed_array_assign_y : 3;
/* XXX  2-bit padding  */
/* XXX  3-byte padding */

                               /* total size (bytes):    8 */
                           } r000 : 38;

The "0:23" here is, IMO, actively confusing.

Tom

Re: [PATCH 2/2] Change ptype/o to print bit offset

[John Baldwin]

On 4/29/19 11:31 AM, Tom Tromey wrote:
> This is better, IMO, because now the "offset" of a bitfield is
> consistent with the offset of an ordinary member, referring to its
> offset from the start of the structure.

I agree with this.  This is what I use ptype /o for (to get starting
offsets).  pahole's format may make sense if the bits were numbered
N -> 0 instead of 0 -> N, or if bitfields used "high" bits instead of
"low" bits, but the convention I always see is to use 0 for LSB and
N for MSB.  I'm also assuming that compilers start with the LSB when
assigning bits to bitfield members.

-- 
John Baldwin

Re: [PATCH 0/2] two ptype/o changes

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Tom> This series changes ptype/o in a couple of ways.
Tom> Last week I spent quite some time puzzling over the meaning of the
Tom> 'offset' for a bitfield in ptype/o output.  After talking with Sergio
Tom> on irc, I ended up concluding that gdb should instead print the bit
Tom> offset, not the number of bits remaining in the field's allocation.

Tom> That is patch #2.  Then, I noticed that the tests did not fail,
Tom> despite the output changing.  It turns out the tests weren't working
Tom> correctly, so I wrote patch #1.

Tom> Tested on x86-64 Fedora 29.  Let me know what you think.

Tom> Patch #2 includes a doc change.

I'm checking this in now.

Tom