[RFA 0/2] Support ptype/o in Rust

[RFA 0/2] Support ptype/o in Rust

[Tom Tromey]

This adds support for ptype/o to the Rust code.

The first patch slightly refactors the existing ptype/o code.  The
utility functions are now public methods on struct print_offset_data.

The second patch changes the Rust language code.  I would self-approve
this one but it required a change outside of Rust.  Perhaps this check
ought to have been a flag on the language_defn.

I noticed that ptype/o generates somewhat funny output:

    /* offset    |  size */  type = union simple::Union {
    /*                 1 */    f1: i8,
    /*                 1 */    f2: u8,

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

Here, I think it might be cleaner to put the "total size" information
on the same line as the trailing "}" (and of course not indent it),
like:

    /* offset    |  size */  type = union simple::Union {
    /*                 1 */    f1: i8,
    /*                 1 */    f2: u8,
    /* total size      1 */  }

If you agree I can at least file a bug or maybe implement it.

Additionally I noticed that in C, in most cases fields are indented 4
spaces, but with ptype/o the outermost fields are only indented 2
spaces (relative to the "type =" text).  I think this is probably
unintended as well, but I thought I'd ask... ?

Regression tested on x86-64 Fedora 26.

Tom

[RFA 1/2] Move ptype/o printing code to typeprint.c

[Tom Tromey]

This moves the hole-printing support code for ptype/o from
c-typeprint.c to be methods on print_offset_data.  This allows the
code to be used from non-C languages.

gdb/ChangeLog
2018-06-08  Tom Tromey  <tom@tromey.com>

	* typeprint.h (struct print_offset_data) <update, finish,
	maybe_print_hole>: New methods.
	<indentation>: New constant.
	* typeprint.c (print_offset_data::indentation): Define.
	(print_offset_data::maybe_print_hole, print_offset_data::update)
	(print_offset_data::finish): Move from c-typeprint.c and rename.
	* c-typeprint.c (OFFSET_SPC_LEN): Remove.
	(print_spaces_filtered_with_print_options): Update.
	(c_print_type_union_field_offset, maybe_print_hole)
	(c_print_type_struct_field_offset): Move to typeprint.c and
	rename.
	(c_type_print_base_struct_union): Update.
---
 gdb/ChangeLog     |  15 +++++
 gdb/c-typeprint.c | 160 ++----------------------------------------------------
 gdb/typeprint.c   | 122 +++++++++++++++++++++++++++++++++++++++++
 gdb/typeprint.h   |  33 +++++++++++
 4 files changed, 174 insertions(+), 156 deletions(-)

diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
index d7eaa5433dd..c167e212ded 100644
--- a/gdb/c-typeprint.c
+++ b/gdb/c-typeprint.c
@@ -32,14 +32,6 @@
 #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
@@ -913,7 +905,7 @@ print_spaces_filtered_with_print_options
   if (!flags->print_offsets)
     print_spaces_filtered (level, stream);
   else
-    print_spaces_filtered (level + OFFSET_SPC_LEN, stream);
+    print_spaces_filtered (level + print_offset_data::indentation, stream);
 }
 
 /* Output an access specifier to STREAM, if needed.  LAST_ACCESS is the
@@ -956,127 +948,6 @@ output_access_specifier (struct ui_file *stream,
   return last_access;
 }
 
-/* Print information about field at index FIELD_IDX of the union type
-   TYPE.  Since union fields don't have the concept of offsets, we
-   just print their sizes.
-
-   The output is strongly based on pahole(1).  */
-
-static void
-c_print_type_union_field_offset (struct type *type, unsigned int field_idx,
-				 struct ui_file *stream)
-{
-  struct type *ftype = check_typedef (TYPE_FIELD_TYPE (type, field_idx));
-
-  fprintf_filtered (stream, "/*              %4u */", TYPE_LENGTH (ftype));
-}
-
-/* Helper function for ptype/o implementation that prints information
-   about a hole, if necessary.  STREAM is where to print.  BITPOS is
-   the bitpos of the current field.  PODATA is the offset-printing
-   state.  FOR_WHAT is a string describing the purpose of the
-   hole.  */
-
-static void
-maybe_print_hole (struct ui_file *stream, unsigned int bitpos,
-		  struct print_offset_data *podata, const char *for_what)
-{
-  /* We check for PODATA->END_BITPOS > 0 because there is a specific
-     scenario when PODATA->END_BITPOS 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 PODATA->END_BITPOS > 0 here, GDB will report
-     a hole before the first field, which is not accurate.  */
-  if (podata->end_bitpos > 0 && podata->end_bitpos < bitpos)
-    {
-      /* If PODATA->END_BITPOS 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 - podata->end_bitpos;
-      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 %s  */\n", hole_bit,
-			  for_what);
-
-      if (hole_byte > 0)
-	fprintf_filtered (stream, "/* XXX %2u-byte %s */\n", hole_byte,
-			  for_what);
-    }
-}
-
-/* Print information about field at index FIELD_IDX of the struct type
-   TYPE.
-
-   PODATA->END_BITPOS 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.
-
-   PODATA->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,
-				  struct ui_file *stream,
-				  struct print_offset_data *podata)
-{
-  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;
-
-  maybe_print_hole (stream, bitpos, podata, "hole");
-
-  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) + podata->offset_bitpos;
-
-      /* The position of the field, relative to the beginning of the
-	 struct, and how many bits are left to be used in this
-	 container.  */
-      fprintf_filtered (stream, "/* %4u:%2u", bit_offset / TARGET_CHAR_BIT,
-			fieldsize_bit - (relative_bitpos + bitsize));
-      fieldsize_bit = bitsize;
-    }
-  else
-    {
-      /* The position of the field, relative to the beginning of the
-	 struct.  */
-      fprintf_filtered (stream, "/* %4u",
-			(bitpos + podata->offset_bitpos) / TARGET_CHAR_BIT);
-
-      fprintf_filtered (stream, "   ");
-    }
-
-  fprintf_filtered (stream, "   |  %4u */", fieldsize_byte);
-
-  podata->end_bitpos = bitpos + fieldsize_bit;
-}
-
 /* Return true if an access label (i.e., "public:", "private:",
    "protected:") needs to be printed for TYPE.  */
 
@@ -1289,20 +1160,7 @@ c_type_print_base_struct_union (struct type *type, struct ui_file *stream,
 	  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, stream, podata);
-		    }
-		  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);
-	    }
+	    podata->update (type, i, stream);
 
 	  print_spaces_filtered (level + 4, stream);
 	  if (is_static)
@@ -1560,19 +1418,9 @@ c_type_print_base_struct_union (struct type *type, struct ui_file *stream,
       if (flags->print_offsets)
 	{
 	  if (show > 0)
-	    {
-	      unsigned int bitpos = TYPE_LENGTH (type) * TARGET_CHAR_BIT;
-	      maybe_print_hole (stream, bitpos, podata, "padding");
-
-	      fputs_filtered ("\n", stream);
-	      print_spaces_filtered_with_print_options (level + 4,
-							stream,
-							flags);
-	      fprintf_filtered (stream, "/* total size (bytes): %4u */\n",
-				TYPE_LENGTH (type));
-	    }
+	    podata->finish (type, level, stream);
 
-	  print_spaces_filtered (OFFSET_SPC_LEN, stream);
+	  print_spaces_filtered (print_offset_data::indentation, stream);
 	  if (level == 0)
 	    print_spaces_filtered (2, stream);
 	}
diff --git a/gdb/typeprint.c b/gdb/typeprint.c
index 222fc0a06b1..66ba0a87c6a 100644
--- a/gdb/typeprint.c
+++ b/gdb/typeprint.c
@@ -65,6 +65,128 @@ static struct type_print_options default_ptype_flags =
 
 \f
 
+const int print_offset_data::indentation = 23;
+
+
+/* See typeprint.h.  */
+
+void
+print_offset_data::maybe_print_hole (struct ui_file *stream,
+				     unsigned int bitpos,
+				     const char *for_what)
+{
+  /* We check for END_BITPOS > 0 because there is a specific
+     scenario when END_BITPOS 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 END_BITPOS > 0 here, GDB will report
+     a hole before the first field, which is not accurate.  */
+  if (end_bitpos > 0 && end_bitpos < bitpos)
+    {
+      /* If END_BITPOS 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 - end_bitpos;
+      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 %s  */\n", hole_bit,
+			  for_what);
+
+      if (hole_byte > 0)
+	fprintf_filtered (stream, "/* XXX %2u-byte %s */\n", hole_byte,
+			  for_what);
+    }
+}
+
+/* See typeprint.h.  */
+
+void
+print_offset_data::update (struct type *type, unsigned int field_idx,
+			   struct ui_file *stream)
+{
+  if (field_is_static (&TYPE_FIELD (type, field_idx)))
+    {
+      print_spaces_filtered (indentation, stream);
+      return;
+    }
+
+  struct type *ftype = check_typedef (TYPE_FIELD_TYPE (type, field_idx));
+  if (TYPE_CODE (type) == TYPE_CODE_UNION)
+    {
+      /* Since union fields don't have the concept of offsets, we just
+	 print their sizes.  */
+      fprintf_filtered (stream, "/*              %4u */", TYPE_LENGTH (ftype));
+      return;
+    }
+
+  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;
+
+  maybe_print_hole (stream, bitpos, "hole");
+
+  if (TYPE_FIELD_PACKED (type, field_idx))
+    {
+      /* We're dealing with a bitfield.  Print how many bits are left
+	 to be used.  */
+      unsigned int bitsize = TYPE_FIELD_BITSIZE (type, field_idx);
+      /* The bitpos relative to the beginning of our container
+	 field.  */
+      unsigned int relative_bitpos;
+
+      /* The following was copied from
+	 value.c:value_primitive_field.  */
+      if ((bitpos % fieldsize_bit) + bitsize <= fieldsize_bit)
+	relative_bitpos = bitpos % fieldsize_bit;
+      else
+	relative_bitpos = bitpos % TARGET_CHAR_BIT;
+
+      /* This is the exact offset (in bits) of this bitfield.  */
+      unsigned int bit_offset
+	= (bitpos - relative_bitpos) + offset_bitpos;
+
+      /* The position of the field, relative to the beginning of the
+	 struct, and how many bits are left to be used in this
+	 container.  */
+      fprintf_filtered (stream, "/* %4u:%2u", bit_offset / TARGET_CHAR_BIT,
+			fieldsize_bit - (relative_bitpos + bitsize));
+      fieldsize_bit = bitsize;
+    }
+  else
+    {
+      /* The position of the field, relative to the beginning of the
+	 struct.  */
+      fprintf_filtered (stream, "/* %4u",
+			(bitpos + offset_bitpos) / TARGET_CHAR_BIT);
+
+      fprintf_filtered (stream, "   ");
+    }
+
+  fprintf_filtered (stream, "   |  %4u */", fieldsize_byte);
+
+  end_bitpos = bitpos + fieldsize_bit;
+}
+
+/* See typeprint.h.  */
+
+void
+print_offset_data::finish (struct type *type, int level,
+			   struct ui_file *stream)
+{
+  unsigned int bitpos = TYPE_LENGTH (type) * TARGET_CHAR_BIT;
+  maybe_print_hole (stream, bitpos, "padding");
+
+  fputs_filtered ("\n", stream);
+  print_spaces_filtered (level + 4 + print_offset_data::indentation, stream);
+  fprintf_filtered (stream, "/* total size (bytes): %4u */\n",
+		    TYPE_LENGTH (type));
+}
+
+\f
+
 /* A hash function for a typedef_field.  */
 
 static hashval_t
diff --git a/gdb/typeprint.h b/gdb/typeprint.h
index 74006b3058f..edd8c396c87 100644
--- a/gdb/typeprint.h
+++ b/gdb/typeprint.h
@@ -38,6 +38,39 @@ struct print_offset_data
      field (where we expect the current field to be if there is no
      hole).  */
   unsigned int end_bitpos = 0;
+
+  /* Print information about field at index FIELD_IDX of the struct type
+     TYPE and update this object.
+
+     If the field is static, it simply prints the correct number of
+     spaces.
+
+     The output is strongly based on pahole(1).  */
+  void update (struct type *type, unsigned int field_idx,
+	       struct ui_file *stream);
+
+  /* Call when all fields have been printed.  This will print
+     information about any padding that may exist.  LEVEL is the
+     desired indentation level.  */
+  void finish (struct type *type, int level, struct ui_file *stream);
+
+  /* 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.  */
+  static const int indentation;
+
+private:
+
+  /* Helper function for ptype/o implementation that prints
+     information about a hole, if necessary.  STREAM is where to
+     print.  BITPOS is the bitpos of the current field.  FOR_WHAT is a
+     string describing the purpose of the hole.  */
+
+  void maybe_print_hole (struct ui_file *stream, unsigned int bitpos,
+			 const char *for_what);
 };
 
 struct type_print_options
-- 
2.13.6

Re: [RFA 1/2] Move ptype/o printing code to typeprint.c

[Sergio Durigan Junior]

On Saturday, June 23 2018, Tom Tromey wrote:

> This moves the hole-printing support code for ptype/o from
> c-typeprint.c to be methods on print_offset_data.  This allows the
> code to be used from non-C languages.

Thanks, Tom.  I looked at the patch, and it seems OK to me.  Just one
small nit.

> gdb/ChangeLog
> 2018-06-08  Tom Tromey  <tom@tromey.com>
>
> 	* typeprint.h (struct print_offset_data) <update, finish,
> 	maybe_print_hole>: New methods.
> 	<indentation>: New constant.
> 	* typeprint.c (print_offset_data::indentation): Define.
> 	(print_offset_data::maybe_print_hole, print_offset_data::update)
> 	(print_offset_data::finish): Move from c-typeprint.c and rename.
> 	* c-typeprint.c (OFFSET_SPC_LEN): Remove.
> 	(print_spaces_filtered_with_print_options): Update.
> 	(c_print_type_union_field_offset, maybe_print_hole)
> 	(c_print_type_struct_field_offset): Move to typeprint.c and
> 	rename.
> 	(c_type_print_base_struct_union): Update.
> ---
>  gdb/ChangeLog     |  15 +++++
>  gdb/c-typeprint.c | 160 ++----------------------------------------------------
>  gdb/typeprint.c   | 122 +++++++++++++++++++++++++++++++++++++++++
>  gdb/typeprint.h   |  33 +++++++++++
>  4 files changed, 174 insertions(+), 156 deletions(-)
>
> diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
> index d7eaa5433dd..c167e212ded 100644
> --- a/gdb/c-typeprint.c
> +++ b/gdb/c-typeprint.c
> @@ -32,14 +32,6 @@
>  #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
> @@ -913,7 +905,7 @@ print_spaces_filtered_with_print_options
>    if (!flags->print_offsets)
>      print_spaces_filtered (level, stream);
>    else
> -    print_spaces_filtered (level + OFFSET_SPC_LEN, stream);
> +    print_spaces_filtered (level + print_offset_data::indentation, stream);
>  }
>  
>  /* Output an access specifier to STREAM, if needed.  LAST_ACCESS is the
> @@ -956,127 +948,6 @@ output_access_specifier (struct ui_file *stream,
>    return last_access;
>  }
>  
> -/* Print information about field at index FIELD_IDX of the union type
> -   TYPE.  Since union fields don't have the concept of offsets, we
> -   just print their sizes.
> -
> -   The output is strongly based on pahole(1).  */
> -
> -static void
> -c_print_type_union_field_offset (struct type *type, unsigned int field_idx,
> -				 struct ui_file *stream)
> -{
> -  struct type *ftype = check_typedef (TYPE_FIELD_TYPE (type, field_idx));
> -
> -  fprintf_filtered (stream, "/*              %4u */", TYPE_LENGTH (ftype));
> -}
> -
> -/* Helper function for ptype/o implementation that prints information
> -   about a hole, if necessary.  STREAM is where to print.  BITPOS is
> -   the bitpos of the current field.  PODATA is the offset-printing
> -   state.  FOR_WHAT is a string describing the purpose of the
> -   hole.  */
> -
> -static void
> -maybe_print_hole (struct ui_file *stream, unsigned int bitpos,
> -		  struct print_offset_data *podata, const char *for_what)
> -{
> -  /* We check for PODATA->END_BITPOS > 0 because there is a specific
> -     scenario when PODATA->END_BITPOS 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 PODATA->END_BITPOS > 0 here, GDB will report
> -     a hole before the first field, which is not accurate.  */
> -  if (podata->end_bitpos > 0 && podata->end_bitpos < bitpos)
> -    {
> -      /* If PODATA->END_BITPOS 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 - podata->end_bitpos;
> -      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 %s  */\n", hole_bit,
> -			  for_what);
> -
> -      if (hole_byte > 0)
> -	fprintf_filtered (stream, "/* XXX %2u-byte %s */\n", hole_byte,
> -			  for_what);
> -    }
> -}
> -
> -/* Print information about field at index FIELD_IDX of the struct type
> -   TYPE.
> -
> -   PODATA->END_BITPOS 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.
> -
> -   PODATA->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,
> -				  struct ui_file *stream,
> -				  struct print_offset_data *podata)
> -{
> -  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;
> -
> -  maybe_print_hole (stream, bitpos, podata, "hole");
> -
> -  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) + podata->offset_bitpos;
> -
> -      /* The position of the field, relative to the beginning of the
> -	 struct, and how many bits are left to be used in this
> -	 container.  */
> -      fprintf_filtered (stream, "/* %4u:%2u", bit_offset / TARGET_CHAR_BIT,
> -			fieldsize_bit - (relative_bitpos + bitsize));
> -      fieldsize_bit = bitsize;
> -    }
> -  else
> -    {
> -      /* The position of the field, relative to the beginning of the
> -	 struct.  */
> -      fprintf_filtered (stream, "/* %4u",
> -			(bitpos + podata->offset_bitpos) / TARGET_CHAR_BIT);
> -
> -      fprintf_filtered (stream, "   ");
> -    }
> -
> -  fprintf_filtered (stream, "   |  %4u */", fieldsize_byte);
> -
> -  podata->end_bitpos = bitpos + fieldsize_bit;
> -}
> -
>  /* Return true if an access label (i.e., "public:", "private:",
>     "protected:") needs to be printed for TYPE.  */
>  
> @@ -1289,20 +1160,7 @@ c_type_print_base_struct_union (struct type *type, struct ui_file *stream,
>  	  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, stream, podata);
> -		    }
> -		  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);
> -	    }
> +	    podata->update (type, i, stream);
>  
>  	  print_spaces_filtered (level + 4, stream);
>  	  if (is_static)
> @@ -1560,19 +1418,9 @@ c_type_print_base_struct_union (struct type *type, struct ui_file *stream,
>        if (flags->print_offsets)
>  	{
>  	  if (show > 0)
> -	    {
> -	      unsigned int bitpos = TYPE_LENGTH (type) * TARGET_CHAR_BIT;
> -	      maybe_print_hole (stream, bitpos, podata, "padding");
> -
> -	      fputs_filtered ("\n", stream);
> -	      print_spaces_filtered_with_print_options (level + 4,
> -							stream,
> -							flags);
> -	      fprintf_filtered (stream, "/* total size (bytes): %4u */\n",
> -				TYPE_LENGTH (type));
> -	    }
> +	    podata->finish (type, level, stream);
>  
> -	  print_spaces_filtered (OFFSET_SPC_LEN, stream);
> +	  print_spaces_filtered (print_offset_data::indentation, stream);
>  	  if (level == 0)
>  	    print_spaces_filtered (2, stream);
>  	}
> diff --git a/gdb/typeprint.c b/gdb/typeprint.c
> index 222fc0a06b1..66ba0a87c6a 100644
> --- a/gdb/typeprint.c
> +++ b/gdb/typeprint.c
> @@ -65,6 +65,128 @@ static struct type_print_options default_ptype_flags =
>  
>  \f
>  
> +const int print_offset_data::indentation = 23;

A "/* See typeprint.h.  */" comment would be nice here, IMO.

> +
> +
> +/* See typeprint.h.  */
> +
> +void
> +print_offset_data::maybe_print_hole (struct ui_file *stream,
> +				     unsigned int bitpos,
> +				     const char *for_what)
> +{
> +  /* We check for END_BITPOS > 0 because there is a specific
> +     scenario when END_BITPOS 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 END_BITPOS > 0 here, GDB will report
> +     a hole before the first field, which is not accurate.  */
> +  if (end_bitpos > 0 && end_bitpos < bitpos)
> +    {
> +      /* If END_BITPOS 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 - end_bitpos;
> +      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 %s  */\n", hole_bit,
> +			  for_what);
> +
> +      if (hole_byte > 0)
> +	fprintf_filtered (stream, "/* XXX %2u-byte %s */\n", hole_byte,
> +			  for_what);
> +    }
> +}
> +
> +/* See typeprint.h.  */
> +
> +void
> +print_offset_data::update (struct type *type, unsigned int field_idx,
> +			   struct ui_file *stream)
> +{
> +  if (field_is_static (&TYPE_FIELD (type, field_idx)))
> +    {
> +      print_spaces_filtered (indentation, stream);
> +      return;
> +    }
> +
> +  struct type *ftype = check_typedef (TYPE_FIELD_TYPE (type, field_idx));
> +  if (TYPE_CODE (type) == TYPE_CODE_UNION)
> +    {
> +      /* Since union fields don't have the concept of offsets, we just
> +	 print their sizes.  */
> +      fprintf_filtered (stream, "/*              %4u */", TYPE_LENGTH (ftype));
> +      return;
> +    }
> +
> +  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;
> +
> +  maybe_print_hole (stream, bitpos, "hole");
> +
> +  if (TYPE_FIELD_PACKED (type, field_idx))
> +    {
> +      /* We're dealing with a bitfield.  Print how many bits are left
> +	 to be used.  */
> +      unsigned int bitsize = TYPE_FIELD_BITSIZE (type, field_idx);
> +      /* The bitpos relative to the beginning of our container
> +	 field.  */
> +      unsigned int relative_bitpos;
> +
> +      /* The following was copied from
> +	 value.c:value_primitive_field.  */
> +      if ((bitpos % fieldsize_bit) + bitsize <= fieldsize_bit)
> +	relative_bitpos = bitpos % fieldsize_bit;
> +      else
> +	relative_bitpos = bitpos % TARGET_CHAR_BIT;
> +
> +      /* This is the exact offset (in bits) of this bitfield.  */
> +      unsigned int bit_offset
> +	= (bitpos - relative_bitpos) + offset_bitpos;
> +
> +      /* The position of the field, relative to the beginning of the
> +	 struct, and how many bits are left to be used in this
> +	 container.  */
> +      fprintf_filtered (stream, "/* %4u:%2u", bit_offset / TARGET_CHAR_BIT,
> +			fieldsize_bit - (relative_bitpos + bitsize));
> +      fieldsize_bit = bitsize;
> +    }
> +  else
> +    {
> +      /* The position of the field, relative to the beginning of the
> +	 struct.  */
> +      fprintf_filtered (stream, "/* %4u",
> +			(bitpos + offset_bitpos) / TARGET_CHAR_BIT);
> +
> +      fprintf_filtered (stream, "   ");
> +    }
> +
> +  fprintf_filtered (stream, "   |  %4u */", fieldsize_byte);
> +
> +  end_bitpos = bitpos + fieldsize_bit;
> +}
> +
> +/* See typeprint.h.  */
> +
> +void
> +print_offset_data::finish (struct type *type, int level,
> +			   struct ui_file *stream)
> +{
> +  unsigned int bitpos = TYPE_LENGTH (type) * TARGET_CHAR_BIT;
> +  maybe_print_hole (stream, bitpos, "padding");
> +
> +  fputs_filtered ("\n", stream);
> +  print_spaces_filtered (level + 4 + print_offset_data::indentation, stream);
> +  fprintf_filtered (stream, "/* total size (bytes): %4u */\n",
> +		    TYPE_LENGTH (type));
> +}
> +
> +\f
> +
>  /* A hash function for a typedef_field.  */
>  
>  static hashval_t
> diff --git a/gdb/typeprint.h b/gdb/typeprint.h
> index 74006b3058f..edd8c396c87 100644
> --- a/gdb/typeprint.h
> +++ b/gdb/typeprint.h
> @@ -38,6 +38,39 @@ struct print_offset_data
>       field (where we expect the current field to be if there is no
>       hole).  */
>    unsigned int end_bitpos = 0;
> +
> +  /* Print information about field at index FIELD_IDX of the struct type
> +     TYPE and update this object.
> +
> +     If the field is static, it simply prints the correct number of
> +     spaces.
> +
> +     The output is strongly based on pahole(1).  */
> +  void update (struct type *type, unsigned int field_idx,
> +	       struct ui_file *stream);
> +
> +  /* Call when all fields have been printed.  This will print
> +     information about any padding that may exist.  LEVEL is the
> +     desired indentation level.  */
> +  void finish (struct type *type, int level, struct ui_file *stream);
> +
> +  /* 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.  */
> +  static const int indentation;
> +
> +private:
> +
> +  /* Helper function for ptype/o implementation that prints
> +     information about a hole, if necessary.  STREAM is where to
> +     print.  BITPOS is the bitpos of the current field.  FOR_WHAT is a
> +     string describing the purpose of the hole.  */
> +
> +  void maybe_print_hole (struct ui_file *stream, unsigned int bitpos,
> +			 const char *for_what);
>  };
>  
>  struct type_print_options
> -- 
> 2.13.6

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [RFA 1/2] Move ptype/o printing code to typeprint.c

[Tom Tromey]

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

Sergio> Thanks, Tom.  I looked at the patch, and it seems OK to me.  Just one
Sergio> small nit.

>> +const int print_offset_data::indentation = 23;

Sergio> A "/* See typeprint.h.  */" comment would be nice here, IMO.

I made this change.

Tom

[RFA 2/2] Support ptype/o in Rust

[Tom Tromey]

This adds support for ptype/o to the Rust language code.

By default, the Rust compiler reorders fields to reduce padding.  So,
the Rust language code sorts the fields by offset before printing.
This may yield somewhat odd-looking results, but it is faithful to
"what really happens", and might be useful when doing lower-level
debugging.

The reordering can be disabled using #[repr(c)]; ptype/o might be more
useful in this case.

gdb/ChangeLog
2018-06-23  Tom Tromey  <tom@tromey.com>

	PR rust/22574:
	* typeprint.c (whatis_exp): Allow ptype/o for Rust.
	* rust-lang.c (rust_print_struct_def): Add podata parameter.
	Update.
	(rust_internal_print_type): Add podata parameter.
	(rust_print_type): Update.

gdb/testsuite/ChangeLog
2018-06-23  Tom Tromey  <tom@tromey.com>

	PR rust/22574:
	* gdb.rust/simple.exp (test_one_slice): Add ptype/o tests.
	* gdb.rust/simple.rs (struct SimpleLayout): New.
---
 gdb/ChangeLog                     |  9 +++++
 gdb/rust-lang.c                   | 82 ++++++++++++++++++++++++++++++---------
 gdb/testsuite/ChangeLog           |  6 +++
 gdb/testsuite/gdb.rust/simple.exp | 18 +++++++++
 gdb/testsuite/gdb.rust/simple.rs  |  8 ++++
 gdb/typeprint.c                   |  3 +-
 6 files changed, 106 insertions(+), 20 deletions(-)

diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
index d9807d0ac1a..af1144dde07 100644
--- a/gdb/rust-lang.c
+++ b/gdb/rust-lang.c
@@ -31,8 +31,10 @@
 #include "objfiles.h"
 #include "psymtab.h"
 #include "rust-lang.h"
+#include "typeprint.h"
 #include "valprint.h"
 #include "varobj.h"
+#include <algorithm>
 #include <string>
 #include <vector>
 
@@ -616,14 +618,14 @@ static void
 rust_internal_print_type (struct type *type, const char *varstring,
 			  struct ui_file *stream, int show, int level,
 			  const struct type_print_options *flags,
-			  bool for_rust_enum);
+			  bool for_rust_enum, print_offset_data *podata);
 
 /* Print a struct or union typedef.  */
 static void
 rust_print_struct_def (struct type *type, const char *varstring,
 		       struct ui_file *stream, int show, int level,
 		       const struct type_print_options *flags,
-		       bool for_rust_enum)
+		       bool for_rust_enum, print_offset_data *podata)
 {
   /* Print a tuple type simply.  */
   if (rust_tuple_type_p (type))
@@ -636,6 +638,13 @@ rust_print_struct_def (struct type *type, const char *varstring,
   if (TYPE_N_BASECLASSES (type) > 0)
     c_print_type (type, varstring, stream, show, level, flags);
 
+  if (flags->print_offsets)
+    {
+      /* Temporarily bump the level so that the output lines up
+	 correctly.  */
+      level += 2;
+    }
+
   /* Compute properties of TYPE here because, in the enum case, the
      rest of the code ends up looking only at the variant part.  */
   const char *tagname = TYPE_NAME (type);
@@ -674,16 +683,41 @@ rust_print_struct_def (struct type *type, const char *varstring,
 
   if (TYPE_NFIELDS (type) == 0 && !is_tuple)
     return;
-  if (for_rust_enum)
+  if (for_rust_enum && !flags->print_offsets)
     fputs_filtered (is_tuple_struct ? "(" : "{", stream);
   else
     fputs_filtered (is_tuple_struct ? " (\n" : " {\n", stream);
 
+  // When printing offsets, we rearrange the fields into storage
+  // order.  This lets us show holes more clearly.  We work using
+  // field indices here because it simplifies calls to
+  // print_offset_data::update below.
+  std::vector<int> fields;
   for (int i = 0; i < TYPE_NFIELDS (type); ++i)
     {
-      QUIT;
       if (field_is_static (&TYPE_FIELD (type, i)))
 	continue;
+      if (is_enum && i == enum_discriminant_index)
+	continue;
+      fields.push_back (i);
+    }
+  if (flags->print_offsets)
+    std::sort (fields.begin (), fields.end (),
+	       [&] (int a, int b)
+	       {
+		 return (TYPE_FIELD_BITPOS (type, a)
+			 < TYPE_FIELD_BITPOS (type, b));
+	       });
+
+  for (int i : fields)
+    {
+      QUIT;
+
+      gdb_assert (!field_is_static (&TYPE_FIELD (type, i)));
+      gdb_assert (! (is_enum && i == enum_discriminant_index));
+
+      if (flags->print_offsets)
+	podata->update (type, i, stream);
 
       /* We'd like to print "pub" here as needed, but rustc
 	 doesn't emit the debuginfo, and our types don't have
@@ -691,27 +725,35 @@ rust_print_struct_def (struct type *type, const char *varstring,
 
       /* For a tuple struct we print the type but nothing
 	 else.  */
-      if (!for_rust_enum)
+      if (!for_rust_enum || flags->print_offsets)
 	print_spaces_filtered (level + 2, stream);
       if (is_enum)
-	{
-	  if (i == enum_discriminant_index)
-	    continue;
-	  fputs_filtered (TYPE_FIELD_NAME (type, i), stream);
-	}
+	fputs_filtered (TYPE_FIELD_NAME (type, i), stream);
       else if (!is_tuple_struct)
 	fprintf_filtered (stream, "%s: ", TYPE_FIELD_NAME (type, i));
 
       rust_internal_print_type (TYPE_FIELD_TYPE (type, i), NULL,
 				stream, (is_enum ? show : show - 1),
-				level + 2, flags, is_enum);
-      if (!for_rust_enum)
+				level + 2, flags, is_enum, podata);
+      if (!for_rust_enum || flags->print_offsets)
 	fputs_filtered (",\n", stream);
+      /* Note that this check of "I" is ok because we only sorted the
+	 fields by offset when print_offsets was set, so we won't take
+	 this branch in that case.  */
       else if (i + 1 < TYPE_NFIELDS (type))
 	fputs_filtered (", ", stream);
     }
 
-  if (!for_rust_enum)
+  if (flags->print_offsets)
+    {
+      /* Undo the temporary level increase we did above.  */
+      level -= 2;
+      podata->finish (type, level, stream);
+      print_spaces_filtered (print_offset_data::indentation, stream);
+      if (level == 0)
+	print_spaces_filtered (2, stream);
+    }
+  if (!for_rust_enum || flags->print_offsets)
     print_spaces_filtered (level, stream);
   fputs_filtered (is_tuple_struct ? ")" : "}", stream);
 }
@@ -735,7 +777,7 @@ static void
 rust_internal_print_type (struct type *type, const char *varstring,
 			  struct ui_file *stream, int show, int level,
 			  const struct type_print_options *flags,
-			  bool for_rust_enum)
+			  bool for_rust_enum, print_offset_data *podata)
 {
   int i;
 
@@ -778,7 +820,7 @@ rust_internal_print_type (struct type *type, const char *varstring,
 	  if (i > 0)
 	    fputs_filtered (", ", stream);
 	  rust_internal_print_type (TYPE_FIELD_TYPE (type, i), "", stream,
-				    -1, 0, flags, false);
+				    -1, 0, flags, false, podata);
 	}
       fputs_filtered (")", stream);
       /* If it returns unit, we can omit the return type.  */
@@ -786,7 +828,7 @@ rust_internal_print_type (struct type *type, const char *varstring,
         {
           fputs_filtered (" -> ", stream);
           rust_internal_print_type (TYPE_TARGET_TYPE (type), "", stream,
-				    -1, 0, flags, false);
+				    -1, 0, flags, false, podata);
         }
       break;
 
@@ -796,7 +838,8 @@ rust_internal_print_type (struct type *type, const char *varstring,
 
 	fputs_filtered ("[", stream);
 	rust_internal_print_type (TYPE_TARGET_TYPE (type), NULL,
-				  stream, show - 1, level, flags, false);
+				  stream, show - 1, level, flags, false,
+				  podata);
 
 	if (TYPE_HIGH_BOUND_KIND (TYPE_INDEX_TYPE (type)) == PROP_LOCEXPR
 	    || TYPE_HIGH_BOUND_KIND (TYPE_INDEX_TYPE (type)) == PROP_LOCLIST)
@@ -811,7 +854,7 @@ rust_internal_print_type (struct type *type, const char *varstring,
     case TYPE_CODE_UNION:
     case TYPE_CODE_STRUCT:
       rust_print_struct_def (type, varstring, stream, show, level, flags,
-			     for_rust_enum);
+			     for_rust_enum, podata);
       break;
 
     case TYPE_CODE_ENUM:
@@ -856,8 +899,9 @@ rust_print_type (struct type *type, const char *varstring,
 		 struct ui_file *stream, int show, int level,
 		 const struct type_print_options *flags)
 {
+  print_offset_data podata;
   rust_internal_print_type (type, varstring, stream, show, level,
-			    flags, false);
+			    flags, false, &podata);
 }
 
 \f
diff --git a/gdb/testsuite/gdb.rust/simple.exp b/gdb/testsuite/gdb.rust/simple.exp
index ba90e061ce3..20fe8dd0a88 100644
--- a/gdb/testsuite/gdb.rust/simple.exp
+++ b/gdb/testsuite/gdb.rust/simple.exp
@@ -285,6 +285,24 @@ gdb_test "print parametrized" \
 
 gdb_test "print u" " = simple::Union {f1: -1, f2: 255}"
 
+gdb_test_sequence "ptype/o Union" "" {
+    "/\\* offset    |  size \\*/  type = union simple::Union {"
+    "/\\*                 1 \\*/    f1: i8,"
+    "/\\*                 1 \\*/    f2: u8,"
+    ""
+    "                           /\\* total size \\(bytes\\):    1 \\*/"
+    "                         }"
+}
+
+gdb_test_sequence "ptype/o SimpleLayout" "" {
+    "/\\* offset    |  size \\*/  type = struct simple::SimpleLayout {"
+    "/\\*    0      |     2 \\*/    f1: u16,"
+    "/\\*    2      |     2 \\*/    f2: u16,"
+    ""
+    "                           /\\* total size \\(bytes\\):    4 \\*/"
+    "                         }"
+}
+
 load_lib gdb-python.exp
 if {[skip_python_tests]} {
     continue
diff --git a/gdb/testsuite/gdb.rust/simple.rs b/gdb/testsuite/gdb.rust/simple.rs
index e5bbe521226..9d89361b753 100644
--- a/gdb/testsuite/gdb.rust/simple.rs
+++ b/gdb/testsuite/gdb.rust/simple.rs
@@ -85,6 +85,13 @@ union Union {
     f2: u8,
 }
 
+// A simple structure whose layout won't be changed by the compiler,
+// so that ptype/o testing will work on any platform.
+struct SimpleLayout {
+    f1: u16,
+    f2: u16
+}
+
 fn main () {
     let a = ();
     let b : [i32; 0] = [];
@@ -159,6 +166,7 @@ fn main () {
     };
 
     let u = Union { f2: 255 };
+    let v = SimpleLayout { f1: 8, f2: 9 };
 
     println!("{}, {}", x.0, x.1);        // set breakpoint here
     println!("{}", diff2(92, 45));
diff --git a/gdb/typeprint.c b/gdb/typeprint.c
index 66ba0a87c6a..c6a404665ba 100644
--- a/gdb/typeprint.c
+++ b/gdb/typeprint.c
@@ -488,7 +488,8 @@ whatis_exp (const char *exp, int show)
 		       feature.  */
 		    if (show > 0
 			&& (current_language->la_language == language_c
-			    || current_language->la_language == language_cplus))
+			    || current_language->la_language == language_cplus
+			    || current_language->la_language == language_rust))
 		      {
 			flags.print_offsets = 1;
 			flags.print_typedefs = 0;
-- 
2.13.6

Re: [RFA 2/2] Support ptype/o in Rust

[Simon Marchi]

On 2018-06-23 16:22, Tom Tromey wrote:
> @@ -674,16 +683,41 @@ rust_print_struct_def (struct type *type, const
> char *varstring,
> 
>    if (TYPE_NFIELDS (type) == 0 && !is_tuple)
>      return;
> -  if (for_rust_enum)
> +  if (for_rust_enum && !flags->print_offsets)
>      fputs_filtered (is_tuple_struct ? "(" : "{", stream);
>    else
>      fputs_filtered (is_tuple_struct ? " (\n" : " {\n", stream);
> 
> +  // When printing offsets, we rearrange the fields into storage
> +  // order.  This lets us show holes more clearly.  We work using
> +  // field indices here because it simplifies calls to
> +  // print_offset_data::update below.

Use /**/  comments.

Otherwise, both patches LGTM.

Simon

Re: [RFA 2/2] Support ptype/o in Rust

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

>> +  // When printing offsets, we rearrange the fields into storage
>> +  // order.  This lets us show holes more clearly.  We work using
>> +  // field indices here because it simplifies calls to
>> +  // print_offset_data::update below.

Simon> Use /**/  comments.

Oops, too much project switching.  Thanks for pointing this out.
I've fixed this.

Tom

Re: [RFA 0/2] Support ptype/o in Rust

[Sergio Durigan Junior]

On Saturday, June 23 2018, Tom Tromey wrote:

> This adds support for ptype/o to the Rust code.
>
> The first patch slightly refactors the existing ptype/o code.  The
> utility functions are now public methods on struct print_offset_data.

Thanks for the patches, Tom.

> The second patch changes the Rust language code.  I would self-approve
> this one but it required a change outside of Rust.  Perhaps this check
> ought to have been a flag on the language_defn.
>
> I noticed that ptype/o generates somewhat funny output:
>
>     /* offset    |  size */  type = union simple::Union {
>     /*                 1 */    f1: i8,
>     /*                 1 */    f2: u8,
>
> 			       /* total size (bytes):    1 */
> 			     }
>
> Here, I think it might be cleaner to put the "total size" information
> on the same line as the trailing "}" (and of course not indent it),
> like:
>
>     /* offset    |  size */  type = union simple::Union {
>     /*                 1 */    f1: i8,
>     /*                 1 */    f2: u8,
>     /* total size      1 */  }
>
> If you agree I can at least file a bug or maybe implement it.

Your version does look better (and IIRC, one of the early versions of
the patch printed "total size" at column 0), but I think it may be a bit
confusing when we're dealing with inner structures.  For example:

  $ cat 2.c
  struct a
  {
    int a1;
    char a2;
    int a3;
  };

  struct b
  {
    struct a b1;
    int b2;
    char b3;
  };

  struct c
  {
    struct a c1;
    struct b c2;
    char c3;
    int c4;
  };

  int main ()
  {
    struct c foo;

    return 0;
  }

We have:

  (gdb) ptype /o struct c
  /* offset    |  size */  type = struct c {
  /*    0      |    12 */    struct a {
  /*    0      |     4 */        int a1;
  /*    4      |     1 */        char a2;
  /* XXX  3-byte hole */
  /*    8      |     4 */        int a3;

                                 /* total size (bytes):   12 */
                             } c1;
  /*   12      |    20 */    struct b {
  /*   12      |    12 */        struct a {
  /*   12      |     4 */            int a1;
  /*   16      |     1 */            char a2;
  /* XXX  3-byte hole */
  /*   20      |     4 */            int a3;

                                     /* total size (bytes):   12 */
                                 } b1;
  /*   24      |     4 */        int b2;
  /*   28      |     1 */        char b3;
  /* XXX  3-byte padding */

                                 /* total size (bytes):   20 */
                             } c2;
  /*   32      |     1 */    char c3;
  /* XXX  3-byte hole */
  /*   36      |     4 */    int c4;

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

Even though it's a bit uglier than your solution, IMO it's easier to
understand and correlate the sizes with their respective structs.
Compare this to:

  (gdb) ptype /o struct c
  /* offset    |  size */  type = struct c {
  /*    0      |    12 */    struct a {
  /*    0      |     4 */        int a1;
  /*    4      |     1 */        char a2;
  /* XXX  3-byte hole */
  /*    8      |     4 */        int a3;
  /* total size (bytes):   12 */
                             } c1;
  /*   12      |    20 */    struct b {
  /*   12      |    12 */        struct a {
  /*   12      |     4 */            int a1;
  /*   16      |     1 */            char a2;
  /* XXX  3-byte hole */
  /*   20      |     4 */            int a3;
  /* total size (bytes):   12 */
                                 } b1;
  /*   24      |     4 */        int b2;
  /*   28      |     1 */        char b3;
  /* XXX  3-byte padding */
  /* total size (bytes):   20 */
                             } c2;
  /*   32      |     1 */    char c3;
  /* XXX  3-byte hole */
  /*   36      |     4 */    int c4;
  /* total size (bytes):   40 */
                           }

Of course, it's still possible to read the output and interpret it
correctly, but it demands a bit more effort, I think.

Maybe a solution would be to be a bit more verbose, like:

  /* total size of struct a (bytes):... */

> Additionally I noticed that in C, in most cases fields are indented 4
> spaces, but with ptype/o the outermost fields are only indented 2
> spaces (relative to the "type =" text).  I think this is probably
> unintended as well, but I thought I'd ask... ?

I don't really remember why I made this decision.  I guess it had to do
with the fact that using 4 whitespaces to indent would consume a lot of
unnecessary space, and since ptype/o prints more information than the
regular ptype, every whitespace counts.  I vaguely remember having this
thought, so that may be the reason, after all.  Or maybe it also had
something to do with increasing the readability?

Anyway, TBH I don't have a strong opinion here.  If you want to indent
the outermost fields by 4 spaces, I won't oppose.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [RFA 0/2] Support ptype/o in Rust

[Tom Tromey]

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

Sergio> Even though it's a bit uglier than your solution, IMO it's easier to
Sergio> understand and correlate the sizes with their respective structs.
Sergio> Compare this to:

Sergio>   (gdb) ptype /o struct c
Sergio>   /* offset    |  size */  type = struct c {
Sergio>   /*    0      |    12 */    struct a {
Sergio>   /*    0      |     4 */        int a1;
Sergio>   /*    4      |     1 */        char a2;
Sergio>   /* XXX  3-byte hole */
Sergio>   /*    8      |     4 */        int a3;
Sergio>   /* total size (bytes):   12 */
Sergio>                              } c1;
Sergio>   /*   12      |    20 */    struct b {
Sergio>   /*   12      |    12 */        struct a {
Sergio>   /*   12      |     4 */            int a1;
Sergio>   /*   16      |     1 */            char a2;
Sergio>   /* XXX  3-byte hole */
Sergio>   /*   20      |     4 */            int a3;
Sergio>   /* total size (bytes):   12 */
Sergio>                                  } b1;
Sergio>   /*   24      |     4 */        int b2;
Sergio>   /*   28      |     1 */        char b3;
Sergio>   /* XXX  3-byte padding */
Sergio>   /* total size (bytes):   20 */
Sergio>                              } c2;
Sergio>   /*   32      |     1 */    char c3;
Sergio>   /* XXX  3-byte hole */
Sergio>   /*   36      |     4 */    int c4;
Sergio>   /* total size (bytes):   40 */
Sergio>                            }

My idea was to line up the total size with the "}" so it would instead
appear as:

  (gdb) ptype /o struct c
  /* offset    |  size */  type = struct c {
  /*    0      |    12 */    struct a {
  /*    0      |     4 */        int a1;
  /*    4      |     1 */        char a2;
  /* XXX  3-byte hole */
  /*    8      |     4 */        int a3;
  /* total size:    12 */    } c1;
  /*   12      |    20 */    struct b {
  /*   12      |    12 */        struct a {
  /*   12      |     4 */            int a1;
  /*   16      |     1 */            char a2;
  /* XXX  3-byte hole */
  /*   20      |     4 */            int a3;
  /* total size:    12 */    } b1;
  /*   24      |     4 */        int b2;
  /*   28      |     1 */        char b3;
  /* XXX  3-byte padding */
  /* total size:    20 */    } c2;
  /*   32      |     1 */    char c3;
  /* XXX  3-byte hole */
  /*   36      |     4 */    int c4;
  /* total size:    40 */  }

But yeah, maybe that's a bit harder to read than the status quo.

Sergio> Anyway, TBH I don't have a strong opinion here.  If you want to indent
Sergio> the outermost fields by 4 spaces, I won't oppose.

I'm on the fence.  It works ok now; it just made some of the code a
little uglier.

Tom