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:

> On 2017-12-11 18:24, Sergio Durigan Junior wrote:
>>>> +
>>>> +   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.
>
> Ah ok that makes sense.  Yeah, a comment would be nice.  But now I'm
> thinking that it would be nice if GDB showed the vtable.  If I say the
> first field at offset 8, it would probably take me some time to get
> why, and would maybe think it's a bug.  But if we showed a fake field,
> such as:
>
>   /*    0      |     8 */    /* vtable */
>
> It would be immediately obvious.

OK, I did that.

BTW, a little status update.

Apparently the patch can't handle bitfields very well.  I've found a few
cases where the bitfield handling gets confused, printing wrong
offsets/sizes/holes.  Bitfields can be extremely complex to deal with
when it comes to offsets...

I spent hours trying to improve the patch, managed to make some
progress, but there are still corner cases to fix.  For example, the
patch doesn't deal well with this case:

struct aa {                                    
/*    0      |     1 */    char aa;            
/*    1: 1   |     1 */    unsigned char a : 7;                                                
/*    1:15   |     4 */    int b : 10;         
} /* total size:    4 bytes */                 

In this case, the bitfield "b" would be combined with the previous
bitfield "a", like pahole reports:

struct aa {                                    
        char                       aa;                   /*     0     1 */                     
        unsigned char              a:7;                  /*     1: 1  1 */                     

        /* Bitfield combined with previous fields */                                           

        int                        b:10;                 /*     0: 7  4 */                     
}

Confusing...  I'm not sure why pahole reports b's offset to be 0.

Also, the patch doesn't understand cases like this:

struct tyu
{
  unsigned int a1 : 1;
  unsigned int a2 : 3;
  unsigned int a9 : 23;
  /* PROBLEM HAPPENS HERE */
  unsigned char a0 : 2;
  uint64_t a3;
  unsigned int a4 : 5;
  uint64_t a5 : 3;
};

In this case, we're switching types in the middle of the bitfield.  The
bitfield "unsigned char a0" would be combined with the bitfields above
it, but the patch doesn't know that:

struct tyu {
/*    0:31   |     4 */    unsigned int a1 : 1;
/*    0:28   |     4 */    unsigned int a2 : 3;
/*    0: 5   |     4 */    unsigned int a9 : 23;
/*    3: 6   |     1 */    unsigned char a0 : 2;
/* XXX  3-bit hole   */
/* XXX  4-byte hole  */
/*    8      |     8 */    uint64_t a3;
/*   16:27   |     4 */    unsigned int a4 : 5;
/*   16:56   |     8 */    uint64_t a5 : 3;
} /* total size:   24 bytes */

Whereas pahole reports:

struct tyu {
        unsigned int               a1:1;                 /*     0:31  4 */
        unsigned int               a2:3;                 /*     0:28  4 */
        unsigned int               a9:23;                /*     0: 5  4 */

        /* XXX 253 bits hole, try to pack */
        /* Bitfield combined with next fields */

        unsigned char              a0:2;                 /*     3: 3  1 */

        /* XXX 6 bits hole, try to pack */
        /* XXX 4 bytes hole, try to pack */

        uint64_t                   a3;                   /*     8     8 */
        unsigned int               a4:5;                 /*    16:27  4 */
        uint64_t                   a5:3;                 /*    16:56  8 */
};

Hm, TBH pahole itself seems a bit confused here, saying there is a
253-bit hole...


Anyway, long story short, this is much more complex that I thought it
would be (TM).  I am extremely tired right now and can't continue, but I
intend to resume work tomorrow morning.  But I'd like to leave a few
options on the table:

1) Remove the patch from the 8.1 wishlist, which will unblock the
branching.

2) Remove the bitfield handling from the patch, leaving it
feature-incomplete, but working.

3) Push the patch with these known limitations, document them, mark them
as KFAIL in the testcase, and open a bug to fix them (I personally
wouldn't like this).

4) Wait more time until these issues are resolved.

WDYT?

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/