Implement pahole-like 'ptype /o' option

  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
  struct stap_probe {
  /* offset    |  size */
  /*    0      |    40 */    probe 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 {
				 const char *text;
				 VEC_stap_probe_arg_s *vec;
			     } args_u;
  } /* total size:   64 bytes */

The idea is to print offsets and sizes only for structs, so unions and
other types are mostly ignored.

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 intend
to submit a follow-up patch cleaning a few things up in this code.

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:
2017-11-21  Sergio Durigan Junior  <sergiodj@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_offset): New function.
	(c_type_print_base): Print offsets and sizes for struct
	fields.
	* typeprint.c (const struct type_print_options
	type_print_raw_options): Initialize 'print_offsets'.
	(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.

gdb/testsuite/ChangeLog:
2017-11-21  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:
2017-11-21  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR cli/16224
	* gdb.texinfo (ptype): Add new flag '/o'.
---
 gdb/NEWS                                 |   3 +
 gdb/c-typeprint.c                        | 144 ++++++++++++++++++++++++++++---
 gdb/doc/gdb.texinfo                      |   4 +
 gdb/testsuite/gdb.base/ptype-offsets.cc  |  77 +++++++++++++++++
 gdb/testsuite/gdb.base/ptype-offsets.exp |  52 +++++++++++
 gdb/typeprint.c                          |   8 +-
 gdb/typeprint.h                          |   3 +
 7 files changed, 276 insertions(+), 15 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/ptype-offsets.cc
 create mode 100644 gdb/testsuite/gdb.base/ptype-offsets.exp
  

On Tuesday, November 21 2017, I wrote:

[...]
> diff --git a/gdb/testsuite/gdb.base/ptype-offsets.cc b/gdb/testsuite/gdb.base/ptype-offsets.cc
> new file mode 100644
> index 0000000000..f6f845db74
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/ptype-offsets.cc
> @@ -0,0 +1,77 @@
> +/* 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;
> +
> +  /* 3-byte hole here.  */

This should be "7-byte hole".  I've updated my local tree.
  

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Tue, 21 Nov 2017 11:07:09 -0500
> 
>  gdb/NEWS                                 |   3 +
>  gdb/c-typeprint.c                        | 144 ++++++++++++++++++++++++++++---
>  gdb/doc/gdb.texinfo                      |   4 +
>  gdb/testsuite/gdb.base/ptype-offsets.cc  |  77 +++++++++++++++++
>  gdb/testsuite/gdb.base/ptype-offsets.exp |  52 +++++++++++
>  gdb/typeprint.c                          |   8 +-
>  gdb/typeprint.h                          |   3 +
>  7 files changed, 276 insertions(+), 15 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/ptype-offsets.cc
>  create mode 100644 gdb/testsuite/gdb.base/ptype-offsets.exp

The documentation parts are okay, but ...

> +@item o
> +Print the offsets and sizes of fields in a struct, similar to what the
> +@command{pahole} tool does.

... how about an example showing the output of this?  I, for one, have
never heard of 'pahole', so the reference to it won't help the likes
of myself to understand what will be produced.

And here:

> +  /o    print offsets and sizes of fields in a struct (like pahole)\n"));

I wonder whether we should mention 'pahole' at all.

Thanks.
  

On Tuesday, November 21 2017, Eli Zaretskii wrote:

>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Cc: Sergio Durigan Junior <sergiodj@redhat.com>
>> Date: Tue, 21 Nov 2017 11:07:09 -0500
>> 
>>  gdb/NEWS                                 |   3 +
>>  gdb/c-typeprint.c                        | 144 ++++++++++++++++++++++++++++---
>>  gdb/doc/gdb.texinfo                      |   4 +
>>  gdb/testsuite/gdb.base/ptype-offsets.cc  |  77 +++++++++++++++++
>>  gdb/testsuite/gdb.base/ptype-offsets.exp |  52 +++++++++++
>>  gdb/typeprint.c                          |   8 +-
>>  gdb/typeprint.h                          |   3 +
>>  7 files changed, 276 insertions(+), 15 deletions(-)
>>  create mode 100644 gdb/testsuite/gdb.base/ptype-offsets.cc
>>  create mode 100644 gdb/testsuite/gdb.base/ptype-offsets.exp
>
> The documentation parts are okay, but ...
>
>> +@item o
>> +Print the offsets and sizes of fields in a struct, similar to what the
>> +@command{pahole} tool does.
>
> ... how about an example showing the output of this?  I, for one, have
> never heard of 'pahole', so the reference to it won't help the likes
> of myself to understand what will be produced.

Good point.  I will include an example output, then.

> And here:
>
>> +  /o    print offsets and sizes of fields in a struct (like pahole)\n"));
>
> I wonder whether we should mention 'pahole' at all.

I thought that it was good to mention 'pahole' here because that was the
main motivation for writing this patch.  My feeling is that it's
benefitial to mention 'pahole' because even if the user doesn't know
what it is, this will be a good opportunity for them to learn :-).

But that's my personal opinion; I won't fight if you think the reference
should be removed.

Thanks,
  

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: gdb-patches@sourceware.org
> Date: Tue, 21 Nov 2017 12:00:14 -0500
> 
> >> +  /o    print offsets and sizes of fields in a struct (like pahole)\n"));
> >
> > I wonder whether we should mention 'pahole' at all.
> 
> I thought that it was good to mention 'pahole' here because that was the
> main motivation for writing this patch.  My feeling is that it's
> benefitial to mention 'pahole' because even if the user doesn't know
> what it is, this will be a good opportunity for them to learn :-).
> 
> But that's my personal opinion; I won't fight if you think the reference
> should be removed.

I don't have strong a opinion.  Let's see what others think.  If no
one chimes in, feel free to leave it unchanged.
  

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> This commit implements the pahole-like '/o' option for 'ptype', which
Sergio> prints the offsets and sizes of struct fields, reporting whenever
Sergio> there is a hole found.

Thanks for doing this!

Sergio> The idea is to print offsets and sizes only for structs, so unions and
Sergio> other types are mostly ignored.

I tried out the patch, since I want to add support for this feature to
the Rust language code.  I have two comments.

First, I think it would be valuable to descend into embedded structures.
This would make it simpler to review layout of the entire outer struct.
That is, like this:

    struct t {
      int x;
      int y;
    };

    struct s {
      int x;
      char c;
      struct t t;
    } s;

The old pahole.py did do this, like

    (gdb) pahole struct s
                  struct s {
     /*   0   4 */  int x
     /*   4   1 */  char c
      /* XXX 24 bit hole, try to pack */
     /*   8   8 */  struct t {
     /*   0   4 */    int x
     /*   4   4 */    int y
                    } t
                  } 

... though the old code's output is kind of confusing, restarting the
offset at 0 in the embedded struct.

Second, and similarly, descending into unions does seem to make sense
sometimes (pahole.py didn't do this, but it seems like it should have).

Continuing the above example, consider:

    union u {
      struct s s;
      int i;
    };

Here ptype shows:

    (gdb) ptype/o union u
    type = union u {
                               struct s s;
                               int i;
    }

(The formatting looks weird without the layout info here...)

I think one specific case where it is interesting to see the union's
layout is when you want to know why a union field in a struct is as
large as it is.  That is, what is the largest member of the union, in
case you want to try to shrink it?

Tom
  

On Sunday, November 26 2017, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Sergio> This commit implements the pahole-like '/o' option for 'ptype', which
> Sergio> prints the offsets and sizes of struct fields, reporting whenever
> Sergio> there is a hole found.
>
> Thanks for doing this!

Thanks for the insights :-).

> Sergio> The idea is to print offsets and sizes only for structs, so unions and
> Sergio> other types are mostly ignored.
>
> I tried out the patch, since I want to add support for this feature to
> the Rust language code.  I have two comments.
>
> First, I think it would be valuable to descend into embedded structures.
> This would make it simpler to review layout of the entire outer struct.
> That is, like this:
>
>     struct t {
>       int x;
>       int y;
>     };
>
>     struct s {
>       int x;
>       char c;
>       struct t t;
>     } s;
>
> The old pahole.py did do this, like
>
>     (gdb) pahole struct s
>                   struct s {
>      /*   0   4 */  int x
>      /*   4   1 */  char c
>       /* XXX 24 bit hole, try to pack */
>      /*   8   8 */  struct t {
>      /*   0   4 */    int x
>      /*   4   4 */    int y
>                     } t
>                   } 

I agree.  In order to test the patch, I've even hacked GDB and made
'ptype' print more levels instead of just 1.  However, due to the
idiosincrasies of the formatting function + the fact that this feature
would be better off as a second patch, I left it as TODO.

I think for 'ptype /o' the best would be to stick with 2 levels, because
otherwise the output gets too messed up.  But I've been thinking about
sending another patch to enable the multi-level ptype anyway.

> ... though the old code's output is kind of confusing, restarting the
> offset at 0 in the embedded struct.

The way my code is written, it also restarts the offset at 0.  Do you
think it'd be better to keep using the offset from the outter struct?

> Second, and similarly, descending into unions does seem to make sense
> sometimes (pahole.py didn't do this, but it seems like it should have).
>
> Continuing the above example, consider:
>
>     union u {
>       struct s s;
>       int i;
>     };
>
> Here ptype shows:
>
>     (gdb) ptype/o union u
>     type = union u {
>                                struct s s;
>                                int i;
>     }
>
> (The formatting looks weird without the layout info here...)
>
> I think one specific case where it is interesting to see the union's
> layout is when you want to know why a union field in a struct is as
> large as it is.  That is, what is the largest member of the union, in
> case you want to try to shrink it?

Good point.  I've implemented this on the patch.

Thanks,
  

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> I think for 'ptype /o' the best would be to stick with 2 levels, because
Sergio> otherwise the output gets too messed up.  But I've been thinking about
Sergio> sending another patch to enable the multi-level ptype anyway.

I see what you mean, though I wonder if it's better to just overflow and
let users with deeply-nested types just widen their terminals.

Alternatively the code could wrap_here() at strategic points.

>> ... though the old code's output is kind of confusing, restarting the
>> offset at 0 in the embedded struct.

Sergio> The way my code is written, it also restarts the offset at 0.  Do you
Sergio> think it'd be better to keep using the offset from the outter struct?

I am not 100% certain but I tend to think it's more understandable to
have the offsets refer to the outermost structure.

Tom
  

On Monday, November 27 2017, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Sergio> I think for 'ptype /o' the best would be to stick with 2 levels, because
> Sergio> otherwise the output gets too messed up.  But I've been thinking about
> Sergio> sending another patch to enable the multi-level ptype anyway.
>
> I see what you mean, though I wonder if it's better to just overflow and
> let users with deeply-nested types just widen their terminals.

OK, so your idea is to always "un-nest" structures/unions when doing
'ptype /o' (just like pahole.py does).  That works for me, but I'd like
to hear other opinions as well.

> Alternatively the code could wrap_here() at strategic points.

Yeah, I'll see if I can do that, but I don't know.

>>> ... though the old code's output is kind of confusing, restarting the
>>> offset at 0 in the embedded struct.
>
> Sergio> The way my code is written, it also restarts the offset at 0.  Do you
> Sergio> think it'd be better to keep using the offset from the outter struct?
>
> I am not 100% certain but I tend to think it's more understandable to
> have the offsets refer to the outermost structure.

OK, that can be arranged :-).

Thanks,
  

Changes from v2:

- Split the patch between #1 (code reorganization) and #2 (actual
  implementation of 'ptype /o').

- Add a few more tests; fix a few thinkos in the testcase.

- Remove leftover code.


Hi,

This is the third version of the patch, which is now a series.
Functionality hasn't changed; this is mostly a reorganization of the
patch into two patches (one for code movement, another for the actual
implementation of 'ptype /o').  Each patch contains an explanation of
what it does.

The docs have already been approved by Eli.
  

Changes from v3:

- Simplified need_access_label_p.

- Fixed a few typos.

- Improved comments on c_print_type_struct_field_offset and
  c_print_type_union_field_offset.

- Removed unused variable.

- Added @smallexample to docs.


Eli had approved the documentation bits for the previous patch, but
since this one had an addition (the inclusion of an @smallexample
showing the output of 'ptype /o'), I believe it'd be good if he could
review it again.

Thanks,
  

Changes from v4:

- Fixed issue with printing bitfield offsets.  Big thanks to Pedro for
  the help with figuring this out.

- Expanded testcase to cover the bitfield offset test.

- Fixed @command/@kbd on documentation.

- Greatly expanded documentation to explain more about various types
  of output that the command generates.

- Added a marker for /* vtable */ fields.


I honestly thought this patch was not going to be ready today.  It's
now apparently printing the offsets (and remaining bits) of bitfields
correctly, at least as far I have tested.

Eli, I think you need to review the docs once again because I included
a great deal of information about the format printed by the command.

Thanks.
  

Changes from v5:

- Dropped support for vtable; too complicated for now, will tackle
  this later.

- Fixed documentation issues.

- Improved output format based on Pedro's suggestions.

- "ptype /o" now defaults to "/tom"; added note on docs about this.

- Added a few more tests.  Added a description to the testcase.  Use
  {} instead of "" for strings.  Removed "optimize=-O0".
  

Changes from v6:

- Updated examples in the documentation.

- Removed stale comments about the test being x86_64-specific from
  tests.

- Added tests for "ptype /oTM", "ptype /TMo", and for the regression
  Pedro caught.

- Removed "print_offset_default_data".

- Fixed regression reported by Pedro.

- s/endpos/end_bitpos/

- Filtered out the languages that don't implement the feature so that
  their type printing output doesn't get clobbered by "ptype /o" when
  they use "common" code (which is not actually common; it lives
  inside c-typeprint.c).  I decided to do that by checking the current
  language on typeprint.c:whatis_exp, before setting the
  "print_offset" bit.


The documentation parts have been approved by Eli, and haven't
changed.
  

On Thursday, December 14 2017, I wrote:

> Changes from v6:
>
> - Updated examples in the documentation.
>
> - Removed stale comments about the test being x86_64-specific from
>   tests.
>
> - Added tests for "ptype /oTM", "ptype /TMo", and for the regression
>   Pedro caught.
>
> - Removed "print_offset_default_data".
>
> - Fixed regression reported by Pedro.
>
> - s/endpos/end_bitpos/
>
> - Filtered out the languages that don't implement the feature so that
>   their type printing output doesn't get clobbered by "ptype /o" when
>   they use "common" code (which is not actually common; it lives
>   inside c-typeprint.c).  I decided to do that by checking the current
>   language on typeprint.c:whatis_exp, before setting the
>   "print_offset" bit.
>
>
> The documentation parts have been approved by Eli, and haven't
> changed.

Patches have been pushed:

a27ed7d613ec91c3a79965d6bdab1fa96d559c85
7c1618381fdaa0697a211721ac31844f884797ac

Thanks for all the help with reviews and suggestions.