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

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Monday, December 11 2017, Simon Marchi wrote:

> 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.

Thanks, I adopted your version.

> What does OFFSET_BITPOS do?

Ah, I forgot to include it in the comment.  It is the offset value we
carry over when we are printing an inner struct.  This is needed because
otherwise we'd print inner structs starting at position 0, which is
something that Tom didn't like and suggested changing.

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

Yeah, this situation happens when we have a virtual method in a class.
Because of the vtable, the first field of the struct will start at
offset 8 (for 64-bit architectures), and in this case *endpos will be 0
because we won't have updated it, leading to a confusing message about a
8-byte hole in the beginning of the struct:

 ...
 50 /* offset    |  size */
 51 type = struct abc {
 52                          public:
 53 /* XXX  8-byte hole  */
 54 /*    8      |     8 */    void *field1;
 ...

In order to suppress this first message, I check for *endpos > 0.

I will add a comment to the code explaining this scenario.

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