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

[Pedro Alves <palves at redhat dot com>]

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.

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.

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

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.

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

Thanks,
Pedro Alves