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 v2] Implement pahole-like 'ptype /o' option


On 2017-11-28 04:21 PM, Sergio Durigan Junior wrote:
> Changes from v1:
> 
> - Address Tom's comments (the command now prints offset information
>   about unions, and the offset info is carried on for nested structs).
> 
> - Address Eli's comments.
> 
> - Extended testcase.
> 
> - A "bit" of cleanup on 'c_type_print_base'.
> 
> 
> 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.

Seems like you'll need to do a bit of conflict handling with the code from

  Record and output access specifiers for nested typedefs
  c191a6875b118fce30e7dc4d9e4bd20eff850270

:(

> gdb/ChangeLog:
> 2017-11-28  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_union_field_offset): New function.
> 	(c_print_type_struct_field_offset): New function.
> 	(need_access_label_p): New function, with contents from
> 	'c_type_print_base'.
> 	(c_type_print_base_struct_union): Likewise.
> 	(c_type_print_base): Print offsets and sizes for struct
> 	fields.  Struct/union handling code move to functions
> 	mentioned above.
> 	* 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:
> 2017-11-28  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-28  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	PR cli/16224
> 	* gdb.texinfo (ptype): Add new flag '/o'.
> ---
>  gdb/NEWS                                 |    3 +
>  gdb/c-typeprint.c                        | 1016 +++++++++++++++++-------------
>  gdb/doc/gdb.texinfo                      |    4 +
>  gdb/testsuite/gdb.base/ptype-offsets.cc  |  113 ++++
>  gdb/testsuite/gdb.base/ptype-offsets.exp |   77 +++
>  gdb/typeprint.c                          |   15 +-
>  gdb/typeprint.h                          |    9 +
>  7 files changed, 812 insertions(+), 425 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/ptype-offsets.cc
>  create mode 100644 gdb/testsuite/gdb.base/ptype-offsets.exp
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 754ce103bd..1247021046 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -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.
> +
>  * GDB now uses the GNU MPFR library, if available, to emulate target
>    floating-point arithmetic during expression evaluation when the target
>    uses different floating-point formats than the host.  At least version
> diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
> index ed5a1a4b8a..39334ccf88 100644
> --- a/gdb/c-typeprint.c
> +++ b/gdb/c-typeprint.c
> @@ -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)

Missing spaces here.

> +{
> +  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,14 +890,569 @@ 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;
>  }
>  
> +/* Print information about the offset of TYPE inside its union.
> +   FIELD_IDX represents the index of this TYPE inside the union.  We
> +   just print the type size, and nothing more.
> +
> +   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 the offset of TYPE inside its struct.
> +   FIELD_IDX represents the index of this TYPE inside the struct, and
> +   ENDPOS is the end position of the previous type (this is how we
> +   calculate whether there are holes in the struct).  At the end,
> +   ENDPOS is updated.
> +
> +   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;
> +
> +  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);
> +    }
> +
> +  /* The position of the field, relative to the beginning of the
> +     struct.  Assume this number will have 4 digits.  */
> +  fprintf_filtered (stream, "/* %4u",
> +		    (bitpos + offset_bitpos) / TARGET_CHAR_BIT);
> +
> +  if (TYPE_FIELD_PACKED (type, field_idx))
> +    {
> +      /* We're dealing with a bitfield.  Print how many bits are left
> +	 to be used.  */
> +      fieldsize_bit = TYPE_FIELD_BITSIZE (type, field_idx);
> +      fprintf_filtered (stream, ":%u",
> +			fieldsize_byte * TARGET_CHAR_BIT - fieldsize_bit);
> +    }
> +  else
> +    {
> +      fieldsize_bit = fieldsize_byte * TARGET_CHAR_BIT;
> +      fprintf_filtered (stream, "   ");
> +    }
> +
> +  fprintf_filtered (stream, "   |  %4u */", fieldsize_byte);
> +
> +  *endpos = bitpos + fieldsize_bit;
> +}
> +
> +/* Return true is an access label (i.e., "public:", "private:",
> +   "protected:") needs to be printed for TYPE.  */
> +
> +static bool
> +need_access_label_p (struct type *type)
> +{
> +  bool need_access_label = false;
> +  int i, j;
> +  int len, len2;
> +
> +  if (TYPE_DECLARED_CLASS (type))
> +    {
> +      QUIT;
> +      len = TYPE_NFIELDS (type);
> +      for (i = TYPE_N_BASECLASSES (type); i < len; i++)
> +	if (!TYPE_FIELD_PRIVATE (type, i))
> +	  {
> +	    need_access_label = true;
> +	    break;
> +	  }
> +      QUIT;
> +      if (!need_access_label)
> +	{
> +	  len2 = TYPE_NFN_FIELDS (type);
> +	  for (j = 0; j < len2; j++)
> +	    {
> +	      len = TYPE_FN_FIELDLIST_LENGTH (type, j);
> +	      for (i = 0; i < len; i++)
> +		if (!TYPE_FN_FIELD_PRIVATE (TYPE_FN_FIELDLIST1 (type,
> +								j), i))
> +		  {
> +		    need_access_label = true;
> +		    break;
> +		  }
> +	      if (need_access_label)
> +		break;
> +	    }
> +	}
> +      QUIT;
> +      if (!need_access_label)
> +	{
> +	  for (i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); ++i)
> +	    {
> +	      if (!TYPE_TYPEDEF_FIELD_PRIVATE (type, i))
> +		{
> +		  need_access_label = true;
> +		  break;
> +		}
> +	    }
> +	}
> +    }
> +  else
> +    {
> +      QUIT;
> +      len = TYPE_NFIELDS (type);
> +      for (i = TYPE_N_BASECLASSES (type); i < len; i++)
> +	if (TYPE_FIELD_PRIVATE (type, i)
> +	    || TYPE_FIELD_PROTECTED (type, i))
> +	  {
> +	    need_access_label = true;
> +	    break;
> +	  }
> +      QUIT;
> +      if (!need_access_label)
> +	{
> +	  len2 = TYPE_NFN_FIELDS (type);
> +	  for (j = 0; j < len2; j++)
> +	    {
> +	      QUIT;
> +	      len = TYPE_FN_FIELDLIST_LENGTH (type, j);
> +	      for (i = 0; i < len; i++)
> +		if (TYPE_FN_FIELD_PROTECTED (TYPE_FN_FIELDLIST1 (type,
> +								 j), i)
> +		    || TYPE_FN_FIELD_PRIVATE (TYPE_FN_FIELDLIST1 (type,
> +								  j),
> +					      i))
> +		  {
> +		    need_access_label = true;
> +		    break;
> +		  }
> +	      if (need_access_label)
> +		break;
> +	    }
> +	}
> +      QUIT;
> +      if (!need_access_label)
> +	{
> +	  for (i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); ++i)
> +	    {
> +	      if (TYPE_TYPEDEF_FIELD_PROTECTED (type, i)
> +		  || TYPE_TYPEDEF_FIELD_PRIVATE (type, i))
> +		{
> +		  need_access_label = true;
> +		  break;
> +		}
> +	    }
> +	}
> +    }
> +  return need_access_label;
> +}
> +
> +/* Helper for 'c_type_print_base' that handles structs and unions.
> +   For a description of the arguments, see 'c_type_print_base'.  */
> +
> +static void
> +c_type_print_base_struct_union (struct type *type, struct ui_file *stream,
> +				int show, int level,
> +				const struct type_print_options *flags)
> +{
> +  struct type_print_options local_flags = *flags;
> +  struct type_print_options semi_local_flags = *flags;
> +  struct cleanup *local_cleanups = make_cleanup (null_cleanup, NULL);
> +
> +  local_flags.local_typedefs = NULL;
> +  semi_local_flags.local_typedefs = NULL;
> +
> +  if (!flags->raw)
> +    {
> +      if (flags->local_typedefs)
> +	local_flags.local_typedefs
> +	  = copy_typedef_hash (flags->local_typedefs);
> +      else
> +	local_flags.local_typedefs = create_typedef_hash ();
> +
> +      make_cleanup_free_typedef_hash (local_flags.local_typedefs);
> +    }
> +
> +  c_type_print_modifier (type, stream, 0, 1);
> +  if (TYPE_CODE (type) == TYPE_CODE_UNION)
> +    fprintf_filtered (stream, "union ");
> +  else if (TYPE_DECLARED_CLASS (type))
> +    fprintf_filtered (stream, "class ");
> +  else
> +    fprintf_filtered (stream, "struct ");
> +
> +  /* Print the tag if it exists.  The HP aCC compiler emits a
> +     spurious "{unnamed struct}"/"{unnamed union}"/"{unnamed
> +     enum}" tag for unnamed struct/union/enum's, which we don't
> +     want to print.  */
> +  if (TYPE_TAG_NAME (type) != NULL
> +      && !startswith (TYPE_TAG_NAME (type), "{unnamed"))
> +    {
> +      /* When printing the tag name, we are still effectively
> +	 printing in the outer context, hence the use of FLAGS
> +	 here.  */
> +      print_name_maybe_canonical (TYPE_TAG_NAME (type), flags, stream);
> +      if (show > 0)
> +	fputs_filtered (" ", stream);
> +    }
> +
> +  if (show < 0)
> +    {
> +      /* If we just printed a tag name, no need to print anything
> +	 else.  */
> +      if (TYPE_TAG_NAME (type) == NULL)
> +	fprintf_filtered (stream, "{...}");
> +    }
> +  else if (show > 0 || TYPE_TAG_NAME (type) == NULL)
> +    {
> +      struct type *basetype;
> +
> +      c_type_print_template_args (&local_flags, type, stream);
> +
> +      /* Add in template parameters when printing derivation info.  */
> +      add_template_parameters (local_flags.local_typedefs, type);
> +      cp_type_print_derivation_info (stream, type, &local_flags);
> +
> +      /* This holds just the global typedefs and the template
> +	 parameters.  */
> +      semi_local_flags.local_typedefs
> +	= copy_typedef_hash (local_flags.local_typedefs);
> +      if (semi_local_flags.local_typedefs)
> +	make_cleanup_free_typedef_hash (semi_local_flags.local_typedefs);
> +
> +      /* Now add in the local typedefs.  */
> +      recursively_update_typedef_hash (local_flags.local_typedefs, type);
> +
> +      fprintf_filtered (stream, "{\n");
> +      if (TYPE_NFIELDS (type) == 0 && TYPE_NFN_FIELDS (type) == 0
> +	  && TYPE_TYPEDEF_FIELD_COUNT (type) == 0)
> +	{
> +	  if (TYPE_STUB (type))
> +	    fprintfi_filtered (level + 4, stream,
> +			       _("<incomplete type>\n"));
> +	  else
> +	    fprintfi_filtered (level + 4, stream,
> +			       _("<no data fields>\n"));
> +	}
> +
> +      /* Start off with no specific section type, so we can print
> +	 one for the first field we find, and use that section type
> +	 thereafter until we find another type.  */
> +      enum access_specifier section_type = s_none;
> +
> +      /* For a class, if all members are private, there's no need
> +	 for a "private:" label; similarly, for a struct or union
> +	 masquerading as a class, if all members are public, there's
> +	 no need for a "public:" label.  */
> +      bool need_access_label = need_access_label_p (type);
> +
> +      /* If there is a base class for this type,
> +	 do not print the field that it occupies.  */
> +
> +      int len = TYPE_NFIELDS (type);
> +      int vptr_fieldno = get_vptr_fieldno (type, &basetype);
> +      unsigned int endpos = 0;
> +
> +      for (int i = TYPE_N_BASECLASSES (type); i < len; i++)
> +	{
> +	  QUIT;
> +
> +	  /* If we have a virtual table pointer, omit it.  Even if
> +	     virtual table pointers are not specifically marked in
> +	     the debug info, they should be artificial.  */
> +	  if ((i == vptr_fieldno && type == basetype)
> +	      || TYPE_FIELD_ARTIFICIAL (type, i))
> +	    continue;
> +
> +	  if (need_access_label)
> +	    {
> +	      section_type = output_access_specifier
> +		(stream, section_type, level,
> +		 TYPE_FIELD_PROTECTED (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 (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, newshow, level + 4,
> +			&local_flags);
> +	  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,
> +		 unnamed fields.  This would lead to misleading
> +		 results if the compiler does not put out fields
> +		 for such things (I don't know what it does).  */
> +	      fprintf_filtered (stream, " : %d",
> +				TYPE_FIELD_BITSIZE (type, i));
> +	    }
> +	  fprintf_filtered (stream, ";\n");
> +	}
> +
> +      /* If there are both fields and methods, put a blank line
> +	 between them.  Make sure to count only method that we
> +	 will display; artificial methods will be hidden.  */
> +      len = TYPE_NFN_FIELDS (type);
> +      if (!flags->print_methods)
> +	len = 0;
> +      int real_len = 0;
> +      for (int i = 0; i < len; i++)
> +	{
> +	  struct fn_field *f = TYPE_FN_FIELDLIST1 (type, i);
> +	  int len2 = TYPE_FN_FIELDLIST_LENGTH (type, i);
> +	  int j;
> +
> +	  for (j = 0; j < len2; j++)
> +	    if (!TYPE_FN_FIELD_ARTIFICIAL (f, j))
> +	      real_len++;
> +	}
> +      if (real_len > 0 && section_type != s_none)
> +	fprintf_filtered (stream, "\n");
> +
> +      /* C++: print out the methods.  */
> +      for (int i = 0; i < len; i++)
> +	{
> +	  struct fn_field *f = TYPE_FN_FIELDLIST1 (type, i);
> +	  int j, len2 = TYPE_FN_FIELDLIST_LENGTH (type, i);
> +	  const char *method_name = TYPE_FN_FIELDLIST_NAME (type, i);
> +	  const char *name = type_name_no_tag (type);
> +	  int is_constructor = name && strcmp (method_name,
> +					       name) == 0;
> +
> +	  for (j = 0; j < len2; j++)
> +	    {
> +	      const char *mangled_name;
> +	      gdb::unique_xmalloc_ptr<char> mangled_name_holder;
> +	      char *demangled_name;
> +	      const char *physname = TYPE_FN_FIELD_PHYSNAME (f, j);
> +	      int is_full_physname_constructor =
> +		TYPE_FN_FIELD_CONSTRUCTOR (f, j)
> +		|| is_constructor_name (physname)
> +		|| is_destructor_name (physname)
> +		|| method_name[0] == '~';
> +
> +	      /* Do not print out artificial methods.  */
> +	      if (TYPE_FN_FIELD_ARTIFICIAL (f, j))
> +		continue;
> +
> +	      QUIT;
> +	      section_type = output_access_specifier
> +		(stream, section_type, level,
> +		 TYPE_FN_FIELD_PROTECTED (f, j),
> +		 TYPE_FN_FIELD_PRIVATE (f, j),
> +		 flags);
> +
> +	      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))
> +		fprintf_filtered (stream, "static ");
> +	      if (TYPE_TARGET_TYPE (TYPE_FN_FIELD_TYPE (f, j)) == 0)
> +		{
> +		  /* Keep GDB from crashing here.  */
> +		  fprintf_filtered (stream,
> +				    _("<undefined type> %s;\n"),
> +				    TYPE_FN_FIELD_PHYSNAME (f, j));
> +		  break;
> +		}
> +	      else if (!is_constructor	/* Constructors don't
> +					   have declared
> +					   types.  */
> +		       && !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))
> +		{
> +		  /* Build something we can demangle.  */
> +		  mangled_name_holder.reset (gdb_mangle_name (type, i, j));
> +		  mangled_name = mangled_name_holder.get ();
> +		}
> +	      else
> +		mangled_name = TYPE_FN_FIELD_PHYSNAME (f, j);
> +
> +	      demangled_name =
> +		gdb_demangle (mangled_name,
> +			      DMGL_ANSI | DMGL_PARAMS);
> +	      if (demangled_name == NULL)
> +		{
> +		  /* In some cases (for instance with the HP
> +		     demangling), if a function has more than 10
> +		     arguments, the demangling will fail.
> +		     Let's try to reconstruct the function
> +		     signature from the symbol information.  */
> +		  if (!TYPE_FN_FIELD_STUB (f, j))
> +		    {
> +		      int staticp = TYPE_FN_FIELD_STATIC_P (f, j);
> +		      struct type *mtype = TYPE_FN_FIELD_TYPE (f, j);
> +
> +		      cp_type_print_method_args (mtype,
> +						 "",
> +						 method_name,
> +						 staticp,
> +						 stream, &local_flags);
> +		    }
> +		  else
> +		    fprintf_filtered (stream,
> +				      _("<badly mangled name '%s'>"),
> +				      mangled_name);
> +		}
> +	      else
> +		{
> +		  char *p;
> +		  char *demangled_no_class
> +		    = remove_qualifiers (demangled_name);
> +
> +		  /* Get rid of the `static' appended by the
> +		     demangler.  */
> +		  p = strstr (demangled_no_class, " static");
> +		  if (p != NULL)
> +		    {
> +		      int length = p - demangled_no_class;
> +		      char *demangled_no_static;
> +
> +		      demangled_no_static
> +			= (char *) xmalloc (length + 1);
> +		      strncpy (demangled_no_static,
> +			       demangled_no_class, length);
> +		      *(demangled_no_static + length) = '\0';
> +		      fputs_filtered (demangled_no_static, stream);
> +		      xfree (demangled_no_static);
> +		    }
> +		  else
> +		    fputs_filtered (demangled_no_class, stream);
> +		  xfree (demangled_name);
> +		}
> +
> +	      fprintf_filtered (stream, ";\n");
> +	    }
> +	}
> +
> +      /* Print typedefs defined in this class.  */
> +
> +      if (TYPE_TYPEDEF_FIELD_COUNT (type) != 0 && flags->print_typedefs)
> +	{
> +	  if (TYPE_NFIELDS (type) != 0 || TYPE_NFN_FIELDS (type) != 0)
> +	    fprintf_filtered (stream, "\n");
> +
> +	  for (int i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); i++)
> +	    {
> +	      struct type *target = TYPE_TYPEDEF_FIELD_TYPE (type, i);
> +
> +	      /* Dereference the typedef declaration itself.  */
> +	      gdb_assert (TYPE_CODE (target) == TYPE_CODE_TYPEDEF);
> +	      target = TYPE_TARGET_TYPE (target);
> +
> +	      if (need_access_label)
> +		{
> +		  section_type = output_access_specifier
> +		    (stream, section_type, level,
> +		     TYPE_TYPEDEF_FIELD_PROTECTED (type, i),
> +		     TYPE_TYPEDEF_FIELD_PRIVATE (type, i),
> +		     flags);
> +		}
> +	      print_spaces_filtered_with_print_options (level + 4,
> +							stream, flags);
> +	      fprintf_filtered (stream, "typedef ");
> +
> +	      /* We want to print typedefs with substitutions
> +		 from the template parameters or globally-known
> +		 typedefs but not local typedefs.  */
> +	      c_print_type (target,
> +			    TYPE_TYPEDEF_FIELD_NAME (type, i),
> +			    stream, show - 1, level + 4,
> +			    &semi_local_flags);
> +	      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);
> +}
> +
>  /* Print the name of the type (or the ultimate pointer target,
>     function value or array element), or the description of a structure
>     or union.
> @@ -898,10 +1476,8 @@ c_type_print_base (struct type *type, struct ui_file *stream,
>  		   int show, int level, const struct type_print_options *flags)
>  {
>    int i;
> -  int len, real_len;
> -  enum access_specifier section_type;
> -  int need_access_label = 0;
> -  int j, len2;
> +  int len;
> +  int j;
>  
>    QUIT;
>  
> @@ -918,15 +1494,16 @@ c_type_print_base (struct type *type, struct ui_file *stream,
>       folk tend to expect things like "class5 *foo" rather than "struct
>       class5 *foo".  */
>  
> -  if (show <= 0
> -      && TYPE_NAME (type) != NULL)
> +  struct type *ttype = check_typedef (type);

Is there a reason for introducing this new variable and doing the check_typedef before the if?

> +
> +  if (show <= 0 && TYPE_NAME (type) != NULL)
>      {
>        c_type_print_modifier (type, stream, 0, 1);
>        print_name_maybe_canonical (TYPE_NAME (type), flags, stream);
>        return;
>      }
>  
> -  type = check_typedef (type);
> +  type = ttype;
>  
>    switch (TYPE_CODE (type))
>      {
> @@ -958,416 +1535,7 @@ c_type_print_base (struct type *type, struct ui_file *stream,
>  
>      case TYPE_CODE_STRUCT:
>      case TYPE_CODE_UNION:
> -      {
> -	struct type_print_options local_flags = *flags;
> -	struct type_print_options semi_local_flags = *flags;
> -	struct cleanup *local_cleanups = make_cleanup (null_cleanup, NULL);
> -
> -	local_flags.local_typedefs = NULL;
> -	semi_local_flags.local_typedefs = NULL;
> -
> -	if (!flags->raw)
> -	  {
> -	    if (flags->local_typedefs)
> -	      local_flags.local_typedefs
> -		= copy_typedef_hash (flags->local_typedefs);
> -	    else
> -	      local_flags.local_typedefs = create_typedef_hash ();
> -
> -	    make_cleanup_free_typedef_hash (local_flags.local_typedefs);
> -	  }
> -
> -	c_type_print_modifier (type, stream, 0, 1);
> -	if (TYPE_CODE (type) == TYPE_CODE_UNION)
> -	  fprintf_filtered (stream, "union ");
> -	else if (TYPE_DECLARED_CLASS (type))
> -	  fprintf_filtered (stream, "class ");
> -	else
> -	  fprintf_filtered (stream, "struct ");
> -
> -	/* Print the tag if it exists.  The HP aCC compiler emits a
> -	   spurious "{unnamed struct}"/"{unnamed union}"/"{unnamed
> -	   enum}" tag for unnamed struct/union/enum's, which we don't
> -	   want to print.  */
> -	if (TYPE_TAG_NAME (type) != NULL
> -	    && !startswith (TYPE_TAG_NAME (type), "{unnamed"))
> -	  {
> -	    /* When printing the tag name, we are still effectively
> -	       printing in the outer context, hence the use of FLAGS
> -	       here.  */
> -	    print_name_maybe_canonical (TYPE_TAG_NAME (type), flags, stream);
> -	    if (show > 0)
> -	      fputs_filtered (" ", stream);
> -	  }
> -
> -	if (show < 0)
> -	  {
> -	    /* If we just printed a tag name, no need to print anything
> -	       else.  */
> -	    if (TYPE_TAG_NAME (type) == NULL)
> -	      fprintf_filtered (stream, "{...}");
> -	  }
> -	else if (show > 0 || TYPE_TAG_NAME (type) == NULL)
> -	  {
> -	    struct type *basetype;
> -	    int vptr_fieldno;
> -
> -	    c_type_print_template_args (&local_flags, type, stream);
> -
> -	    /* Add in template parameters when printing derivation info.  */
> -	    add_template_parameters (local_flags.local_typedefs, type);
> -	    cp_type_print_derivation_info (stream, type, &local_flags);
> -
> -	    /* This holds just the global typedefs and the template
> -	       parameters.  */
> -	    semi_local_flags.local_typedefs
> -	      = copy_typedef_hash (local_flags.local_typedefs);
> -	    if (semi_local_flags.local_typedefs)
> -	      make_cleanup_free_typedef_hash (semi_local_flags.local_typedefs);
> -
> -	    /* Now add in the local typedefs.  */
> -	    recursively_update_typedef_hash (local_flags.local_typedefs, type);
> -
> -	    fprintf_filtered (stream, "{\n");
> -	    if (TYPE_NFIELDS (type) == 0 && TYPE_NFN_FIELDS (type) == 0
> -		&& TYPE_TYPEDEF_FIELD_COUNT (type) == 0)
> -	      {
> -		if (TYPE_STUB (type))
> -		  fprintfi_filtered (level + 4, stream,
> -				     _("<incomplete type>\n"));
> -		else
> -		  fprintfi_filtered (level + 4, stream,
> -				     _("<no data fields>\n"));
> -	      }
> -
> -	    /* Start off with no specific section type, so we can print
> -	       one for the first field we find, and use that section type
> -	       thereafter until we find another type.  */
> -
> -	    section_type = s_none;
> -
> -	    /* For a class, if all members are private, there's no need
> -	       for a "private:" label; similarly, for a struct or union
> -	       masquerading as a class, if all members are public, there's
> -	       no need for a "public:" label.  */
> -
> -	    if (TYPE_DECLARED_CLASS (type))
> -	      {
> -		QUIT;
> -		len = TYPE_NFIELDS (type);
> -		for (i = TYPE_N_BASECLASSES (type); i < len; i++)
> -		  if (!TYPE_FIELD_PRIVATE (type, i))
> -		    {
> -		      need_access_label = 1;
> -		      break;
> -		    }
> -		QUIT;
> -		if (!need_access_label)
> -		  {
> -		    len2 = TYPE_NFN_FIELDS (type);
> -		    for (j = 0; j < len2; j++)
> -		      {
> -			len = TYPE_FN_FIELDLIST_LENGTH (type, j);
> -			for (i = 0; i < len; i++)
> -			  if (!TYPE_FN_FIELD_PRIVATE (TYPE_FN_FIELDLIST1 (type,
> -									  j), i))
> -			    {
> -			      need_access_label = 1;
> -			      break;
> -			    }
> -			if (need_access_label)
> -			  break;
> -		      }
> -		  }
> -		QUIT;
> -		if (!need_access_label)
> -		  {
> -		    for (i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); ++i)
> -		      {
> -			if (!TYPE_TYPEDEF_FIELD_PRIVATE (type, i))
> -			  {
> -			    need_access_label = 1;
> -			    break;
> -			  }
> -		      }
> -		  }
> -	      }
> -	    else
> -	      {
> -		QUIT;
> -		len = TYPE_NFIELDS (type);
> -		for (i = TYPE_N_BASECLASSES (type); i < len; i++)
> -		  if (TYPE_FIELD_PRIVATE (type, i)
> -		      || TYPE_FIELD_PROTECTED (type, i))
> -		    {
> -		      need_access_label = 1;
> -		      break;
> -		    }
> -		QUIT;
> -		if (!need_access_label)
> -		  {
> -		    len2 = TYPE_NFN_FIELDS (type);
> -		    for (j = 0; j < len2; j++)
> -		      {
> -			QUIT;
> -			len = TYPE_FN_FIELDLIST_LENGTH (type, j);
> -			for (i = 0; i < len; i++)
> -			  if (TYPE_FN_FIELD_PROTECTED (TYPE_FN_FIELDLIST1 (type,
> -									   j), i)
> -			      || TYPE_FN_FIELD_PRIVATE (TYPE_FN_FIELDLIST1 (type,
> -									    j),
> -							i))
> -			    {
> -			      need_access_label = 1;
> -			      break;
> -			    }
> -			if (need_access_label)
> -			  break;
> -		      }
> -		  }
> -		QUIT;
> -		if (!need_access_label)
> -		  {
> -		    for (i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); ++i)
> -		      {
> -			if (TYPE_TYPEDEF_FIELD_PROTECTED (type, i)
> -			    || TYPE_TYPEDEF_FIELD_PRIVATE (type, i))
> -			  {
> -			    need_access_label = 1;
> -			    break;
> -			  }
> -		      }
> -		  }
> -	      }
> -
> -	    /* If there is a base class for this type,
> -	       do not print the field that it occupies.  */
> -
> -	    len = TYPE_NFIELDS (type);
> -	    vptr_fieldno = get_vptr_fieldno (type, &basetype);
> -	    for (i = TYPE_N_BASECLASSES (type); i < len; i++)
> -	      {
> -		QUIT;
> -
> -		/* If we have a virtual table pointer, omit it.  Even if
> -		   virtual table pointers are not specifically marked in
> -		   the debug info, they should be artificial.  */
> -		if ((i == vptr_fieldno && type == basetype)
> -		    || TYPE_FIELD_ARTIFICIAL (type, i))
> -		  continue;
> -
> -		if (need_access_label)
> -		  {
> -		    section_type = output_access_specifier
> -		      (stream, section_type, level,
> -		       TYPE_FIELD_PROTECTED (type, i),
> -		       TYPE_FIELD_PRIVATE (type, i));
> -		  }
> -
> -		print_spaces_filtered (level + 4, stream);
> -		if (field_is_static (&TYPE_FIELD (type, i)))
> -		  fprintf_filtered (stream, "static ");
> -		c_print_type (TYPE_FIELD_TYPE (type, i),
> -			      TYPE_FIELD_NAME (type, i),
> -			      stream, show - 1, level + 4,
> -			      &local_flags);
> -		if (!field_is_static (&TYPE_FIELD (type, i))
> -		    && TYPE_FIELD_PACKED (type, i))
> -		  {
> -		    /* It is a bitfield.  This code does not attempt
> -		       to look at the bitpos and reconstruct filler,
> -		       unnamed fields.  This would lead to misleading
> -		       results if the compiler does not put out fields
> -		       for such things (I don't know what it does).  */
> -		    fprintf_filtered (stream, " : %d",
> -				      TYPE_FIELD_BITSIZE (type, i));
> -		  }
> -		fprintf_filtered (stream, ";\n");
> -	      }
> -
> -	  /* If there are both fields and methods, put a blank line
> -	     between them.  Make sure to count only method that we
> -	     will display; artificial methods will be hidden.  */
> -	  len = TYPE_NFN_FIELDS (type);
> -	  if (!flags->print_methods)
> -	    len = 0;
> -	  real_len = 0;
> -	  for (i = 0; i < len; i++)
> -	    {
> -	      struct fn_field *f = TYPE_FN_FIELDLIST1 (type, i);
> -	      int len2 = TYPE_FN_FIELDLIST_LENGTH (type, i);
> -	      int j;
> -
> -	      for (j = 0; j < len2; j++)
> -		if (!TYPE_FN_FIELD_ARTIFICIAL (f, j))
> -		  real_len++;
> -	    }
> -	  if (real_len > 0 && section_type != s_none)
> -	    fprintf_filtered (stream, "\n");
> -
> -	  /* C++: print out the methods.  */
> -	  for (i = 0; i < len; i++)
> -	    {
> -	      struct fn_field *f = TYPE_FN_FIELDLIST1 (type, i);
> -	      int j, len2 = TYPE_FN_FIELDLIST_LENGTH (type, i);
> -	      const char *method_name = TYPE_FN_FIELDLIST_NAME (type, i);
> -	      const char *name = type_name_no_tag (type);
> -	      int is_constructor = name && strcmp (method_name,
> -						   name) == 0;
> -
> -	      for (j = 0; j < len2; j++)
> -		{
> -		  const char *mangled_name;
> -		  gdb::unique_xmalloc_ptr<char> mangled_name_holder;
> -		  char *demangled_name;
> -		  const char *physname = TYPE_FN_FIELD_PHYSNAME (f, j);
> -		  int is_full_physname_constructor =
> -		    TYPE_FN_FIELD_CONSTRUCTOR (f, j)
> -		    || is_constructor_name (physname)
> -		    || is_destructor_name (physname)
> -		    || method_name[0] == '~';
> -
> -		  /* Do not print out artificial methods.  */
> -		  if (TYPE_FN_FIELD_ARTIFICIAL (f, j))
> -		    continue;
> -
> -		  QUIT;
> -		  section_type = output_access_specifier
> -		    (stream, section_type, level,
> -		     TYPE_FN_FIELD_PROTECTED (f, j),
> -		     TYPE_FN_FIELD_PRIVATE (f, j));
> -
> -		  print_spaces_filtered (level + 4, stream);
> -		  if (TYPE_FN_FIELD_VIRTUAL_P (f, j))
> -		    fprintf_filtered (stream, "virtual ");
> -		  else if (TYPE_FN_FIELD_STATIC_P (f, j))
> -		    fprintf_filtered (stream, "static ");
> -		  if (TYPE_TARGET_TYPE (TYPE_FN_FIELD_TYPE (f, j)) == 0)
> -		    {
> -		      /* Keep GDB from crashing here.  */
> -		      fprintf_filtered (stream,
> -					_("<undefined type> %s;\n"),
> -					TYPE_FN_FIELD_PHYSNAME (f, j));
> -		      break;
> -		    }
> -		  else if (!is_constructor	/* Constructors don't
> -						   have declared
> -						   types.  */
> -			   && !is_full_physname_constructor  /* " " */
> -			   && !is_type_conversion_operator (type, i, j))
> -		    {
> -		      c_print_type (TYPE_TARGET_TYPE (TYPE_FN_FIELD_TYPE (f, j)),
> -				    "", stream, -1, 0,
> -				    &local_flags);
> -		      fputs_filtered (" ", stream);
> -		    }
> -		  if (TYPE_FN_FIELD_STUB (f, j))
> -		    {
> -		      /* Build something we can demangle.  */
> -		      mangled_name_holder.reset (gdb_mangle_name (type, i, j));
> -		      mangled_name = mangled_name_holder.get ();
> -		    }
> -		  else
> -		    mangled_name = TYPE_FN_FIELD_PHYSNAME (f, j);
> -
> -		  demangled_name =
> -		    gdb_demangle (mangled_name,
> -				  DMGL_ANSI | DMGL_PARAMS);
> -		  if (demangled_name == NULL)
> -		    {
> -		      /* In some cases (for instance with the HP
> -			 demangling), if a function has more than 10
> -			 arguments, the demangling will fail.
> -			 Let's try to reconstruct the function
> -			 signature from the symbol information.  */
> -		      if (!TYPE_FN_FIELD_STUB (f, j))
> -			{
> -			  int staticp = TYPE_FN_FIELD_STATIC_P (f, j);
> -			  struct type *mtype = TYPE_FN_FIELD_TYPE (f, j);
> -
> -			  cp_type_print_method_args (mtype,
> -						     "",
> -						     method_name,
> -						     staticp,
> -						     stream, &local_flags);
> -			}
> -		      else
> -			fprintf_filtered (stream,
> -					  _("<badly mangled name '%s'>"),
> -					  mangled_name);
> -		    }
> -		  else
> -		    {
> -		      char *p;
> -		      char *demangled_no_class
> -			= remove_qualifiers (demangled_name);
> -
> -		      /* Get rid of the `static' appended by the
> -			 demangler.  */
> -		      p = strstr (demangled_no_class, " static");
> -		      if (p != NULL)
> -			{
> -			  int length = p - demangled_no_class;
> -			  char *demangled_no_static;
> -
> -			  demangled_no_static
> -			    = (char *) xmalloc (length + 1);
> -			  strncpy (demangled_no_static,
> -				   demangled_no_class, length);
> -			  *(demangled_no_static + length) = '\0';
> -			  fputs_filtered (demangled_no_static, stream);
> -			  xfree (demangled_no_static);
> -			}
> -		      else
> -			fputs_filtered (demangled_no_class, stream);
> -		      xfree (demangled_name);
> -		    }
> -
> -		  fprintf_filtered (stream, ";\n");
> -		}
> -	    }
> -
> -	  /* Print typedefs defined in this class.  */
> -
> -	  if (TYPE_TYPEDEF_FIELD_COUNT (type) != 0 && flags->print_typedefs)
> -	    {
> -	      if (TYPE_NFIELDS (type) != 0 || TYPE_NFN_FIELDS (type) != 0)
> -		fprintf_filtered (stream, "\n");
> -
> -	      for (i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); i++)
> -		{
> -		  struct type *target = TYPE_TYPEDEF_FIELD_TYPE (type, i);
> -
> -		  /* Dereference the typedef declaration itself.  */
> -		  gdb_assert (TYPE_CODE (target) == TYPE_CODE_TYPEDEF);
> -		  target = TYPE_TARGET_TYPE (target);
> -
> -		  if (need_access_label)
> -		    {
> -		      section_type = output_access_specifier
> -			(stream, section_type, level,
> -			 TYPE_TYPEDEF_FIELD_PROTECTED (type, i),
> -			 TYPE_TYPEDEF_FIELD_PRIVATE (type, i));
> -		    }
> -		  print_spaces_filtered (level + 4, stream);
> -		  fprintf_filtered (stream, "typedef ");
> -
> -		  /* We want to print typedefs with substitutions
> -		     from the template parameters or globally-known
> -		     typedefs but not local typedefs.  */
> -		  c_print_type (target,
> -				TYPE_TYPEDEF_FIELD_NAME (type, i),
> -				stream, show - 1, level + 4,
> -				&semi_local_flags);
> -		  fprintf_filtered (stream, ";\n");
> -		}
> -	    }
> -
> -	    fprintfi_filtered (level, stream, "}");
> -	  }
> -
> -	do_cleanups (local_cleanups);
> -      }
> +      c_type_print_base_struct_union (type, stream, show, level, flags);

Moving this code to a new function is a good idea, but can you do it in its own
preliminary patch?  It is hard to review changes when code is moved and changed
at the same time.

>        break;
>  
>      case TYPE_CODE_ENUM:
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 675f6e7bc8..f7a45dd5dd 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -17095,6 +17095,10 @@ 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.
>  @end table
>  
>  @kindex ptype
> diff --git a/gdb/testsuite/gdb.base/ptype-offsets.cc b/gdb/testsuite/gdb.base/ptype-offsets.cc
> new file mode 100644
> index 0000000000..f9a57fd3db
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/ptype-offsets.cc
> @@ -0,0 +1,113 @@
> +/* 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;
> +};
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  struct abc foo;
> +  struct pqr bar;
> +
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/ptype-offsets.exp b/gdb/testsuite/gdb.base/ptype-offsets.exp
> new file mode 100644
> index 0000000000..4f84416dc5
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/ptype-offsets.exp
> @@ -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/>.
> +
> +standard_testfile .cc ptype-offsets.cc

You can reduce this to just

  standard_testfile ptype-offsets.cc

or

  standard_testfile .cc

It is harmless, but specifying .cc and ptype-offsets.cc will cause you to have two
variables, srcfile and srcfile2, with the value "ptype-offsets.cc", and from what
I see you don't use srcfile2.

> +
> +# 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"

work -> works

> +    return 0
> +}
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
> +	  { debug c++ optimize=-O0 }] } {

optimize=-O0 seems unnecessary to me, I've never seen it specified explicitly in a test.

> +    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"

You could save some backslashes by using {} as string delimiters, if you prefer.
Otherwise, maybe a new proc multi_line_string_to_regexp, which combines multi_line
and string_to_regexp, would be nice.  I mention this as a suggestion for future
improvement, I wouldn't want to delay the merge of this patch just for that.

> +
> +# 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"

You could throw a few more tests in there, for example a union with two structs,
to verify that the offset does go back to 0 when showing the second struct:

  /* offset    |  size */
  type = union my_union {
  /*                 8 */    struct my_struct_1 {
  /*    0      |     4 */        int a;
  /*    4      |     4 */        int b;
                             } /* total size:    8 bytes */ s1;
  /*                 8 */    struct my_struct_2 {
  /*    0      |     4 */        int c;
  /*    4      |     4 */        int d;
                             } /* total size:    8 bytes */ s2;
  } /* total size:    8 bytes */

I also noticed that the offset is not shown in front of the struct-in-union,
as show above, but it is in the case of struct-in-struct:

  /* offset    |  size */
  type = struct my_struct_3 {
  /*    0      |     8 */    struct my_struct_1 {
  /*    0      |     4 */        int a;
  /*    4      |     4 */        int b;
                             } /* total size:    8 bytes */ s1;
  /*    8      |     8 */    struct my_struct_2 {
  /*    8      |     4 */        int c;
  /*   12      |     4 */        int d;
                             } /* total size:    8 bytes */ s2;
  } /* total size:   16 bytes */

Is this difference on purpose?

Finally, a test case on a non-struct/union type, to check that the header is not printed.

> diff --git a/gdb/typeprint.c b/gdb/typeprint.c
> index 427af17ad7..1463e802ad 100644
> --- a/gdb/typeprint.c
> +++ b/gdb/typeprint.c
> @@ -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 */
>    NULL,				/* local_typedefs */
>    NULL,				/* global_table */
>    NULL				/* global_printers */
> @@ -54,6 +56,8 @@ static struct type_print_options default_ptype_flags =
>    0,				/* raw */
>    1,				/* print_methods */
>    1,				/* print_typedefs */
> +  0,				/* print_offsets */
> +  0,				/* offset_bitpos */
>    NULL,				/* local_typedefs */
>    NULL,				/* global_table */
>    NULL				/* global_printers */
> @@ -438,6 +442,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);
>  		}
> @@ -497,6 +504,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)
> @@ -722,7 +734,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,
> diff --git a/gdb/typeprint.h b/gdb/typeprint.h
> index a458aa4e2f..a2a5285012 100644
> --- a/gdb/typeprint.h
> +++ b/gdb/typeprint.h
> @@ -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 off
> +     the offset of the outter struct.  */
> +  unsigned int offset_bitpos;
> +
>    /* If not NULL, a local typedef hash table used when printing a
>       type.  */
>    struct typedef_hash_table *local_typedefs;
> 

Thanks!

Simon


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