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

[Simon Marchi <simon.marchi@ericsson.com>]

Hi Sergio,

LGTM, with some nits.

On 2017-12-11 02:57 PM, Sergio Durigan Junior wrote:
> @@ -867,14 +890,86 @@ 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 wording confuses me a bit.  TYPE is not inside a struct; the struct
contains fields, not types.  And TYPE is the struct type IIUC, not the
field type.  So it should maybe be something like:

  Print information about field at index FIELD_IDX of the struct type TYPE.
  ENDPOS is the one-past-the-end bit position of the previous field (where
  we expect this field to be if there is no hole).  At the end, ENDPOS is
  updated to the one-past-the-end bit position of the current field.

What does OFFSET_BITPOS do?

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

Why do you check for *endpos > 0?  Did you see a case where *endpos is 0
and bitpos > 0?  That would mean that there's a "hole" before the first field.
Would we want to show it as a hole anyway?

Simon