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

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

Thanks for looking into this.

On Tuesday, December 12 2017, Pedro Alves wrote:

> On 12/12/2017 06:19 AM, Sergio Durigan Junior wrote:
>
>> 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.
>
> 0 seems right to me.  The bitfield's type is int, with size 4,
> and it lives at byte offset 0:
>
>  <2><53>: Abbrev Number: 5 (DW_TAG_member)
>     <54>   DW_AT_name        : (indirect string, offset: 0x4ae): b
>     <58>   DW_AT_decl_file   : 1
>     <59>   DW_AT_decl_line   : 7
>     <5a>   DW_AT_type        : <0x71>
>     <5e>   DW_AT_byte_size   : 4
>     <5f>   DW_AT_bit_size    : 10
>     <60>   DW_AT_bit_offset  : 7
>     <61>   DW_AT_data_member_location: 0     <<<
>
> Replacing the 0 with 1, like:
>
>     int                        b:10;                 /*     1: 7  4 */
>
> would look incorrect to me, because that'd make '(char*)&aa + 1 + 4'
> (address of containing object + byte offset + byte size) overshoot the
> size of the containing object.

OK.  The GDB patch is printing "1:15" because the offset is calculated
as:

  (bitpos + offset_bitpos) / TARGET_CHAR_BIT

In this particular case, offset_bitpos is zero because we're not inside
an inner struct, so we don't care about it.  bitpos is TYPE_FIELD_BITPOS
(type, field_idx), which is 15 in this case, because sizeof (aa.aa) +
bitsof (aa.a) = 15.  When we divide it by TARGET_CHAR_BIT, we get 1.  It
seems using TYPE_FIELD_BITPOS is not reliable when dealing with
bitfields.

So there is something I'm not accounting here.

> What is that number after ":"  in bitfields supposed to mean in
> pahole's output (and I assume that that's what you're trying
> to emulate)?  We're missing documentation for that.

Yeah.  I tried to find documentation on the output, but couldn't.
Yesterday I was reading pahole's code.  From what I understand, it is
the number of bits left in the object.

> It seems like it's supposed to mean the number of bits left in the
> containing anonymous object (i.e., in the 4 bytes of the declared
> int)?  Then "0:7" seems right, given:
>
> sizeof (int) * 8 - bitsof(aa.a) - bitsof(aa.a) - bits(aa.b)
>    => sizeof (int) * 8 - 7 - 10 
>    => 7

I guess you meant:

  sizeof (int) * 8 - bitsof (aa.a) - bitsof(aa.b)

Right?

But yeah, it makes sense when you consider that the first bitfield got
"promoted" from "unsigned char" to "int".  I haven't figured out a way
to keep track of these type changes in order to calculate the right
offsets, remaining space and holes.

> It took me a while to get to this conclusion (and writing a lot
> of text that I ended up deleting... :-P), because I originally
> assumed that this was meant to be the field's bit offset.
>
>> 
>> 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...
>
> A 253-bit hole does look odd.

Yes.  I haven't checked all the offsets too; there may be
inconsistencies.

>> 
>> 
>> 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.
>> 
>
> Joel said that we'd re-evaluate Wednesday, so there's still some time.

OK, I'll keep trying here.

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/