Re: [PATCH pahole] pahole: use 32-bit integers for iterations within CU

[Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>]

Em Sun, Mar 10, 2019 at 09:31:51PM -0700, Andrii Nakryiko escreveu:
> On Fri, Mar 8, 2019 at 11:42 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Fri, Mar 8, 2019 at 10:45 AM Arnaldo Carvalho de Melo
> > <arnaldo.melo@gmail.com> wrote:
> > >
> > > Em Thu, Mar 07, 2019 at 09:26:37PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Several looks similar and the low hanging fruit to investigate, seems to
> > > > be enum bitfields, and the others may as well end up being the same, in
> > > > miscalculated stats for structs embedded in other structs:
> > >
> > > I had little time to further look into this, but from what I've seen the
> > > good news is that it seems the problem is with the DWARF loader, the BTF
> > > one producing the right results 8-) I'll take a look at a fix you made
> > > for packed enums and probably use the same thing on the DWARF loader.
> >
> > Yeah, I suspected it might be related to base_type__name_to_size()
> > logic I removed for btf_loader. Ideally we should take the size from
> > DWARF data itself (assuming it's there) and remove
> > base_type__name_to_size() and related parts.
> 
> So I got a chance today to verify your changes from
> tmp.type_id_t-as-uint32_t branch. They look good to me. I've tested
> old and new version of pahole on few of my kernel images, both typical
> one and allyesconfig one. They both produced identical binaries after
> BTF encoding and deduplication, which seems to be good indication that
> nothing is broken. I hope you can push those changes into master soon.
> 
> I've also took a brief look at btfdiff differences for allyesconfig.
> There are not that many of them: just 16k of output text, which given
> 200k types is not a lot.
> 
> I've noticed that there are problems for packed enum fields, which are
> not handled properly neither in DWARF, nor in BTF.
> 
> Here's one example:
> 
>  struct myrb_dcdb {
>         unsigned int               target:4;             /*     0:28  4 */
>         unsigned int               channel:4;            /*     0:24  4 */
> 
> -       /* Bitfield combined with next fields */
> +       /* XXX 24 bits hole, try to pack */
> 
>         enum {
>                 MYRB_DCDB_XFER_NONE = 0,
>                 MYRB_DCDB_XFER_DEVICE_TO_SYSTEM = 1,
>                 MYRB_DCDB_XFER_SYSTEM_TO_DEVICE = 2,
>                 MYRB_DCDB_XFER_ILLEGAL = 3,
> -       } data_xfer:2;                                     /*     1     4 */
> +       } data_xfer:2;                                     /*     4     1 */
> 
>         /* Bitfield combined with previous fields */
> 
>         unsigned int               early_status:1;       /*     0:21  4 */
>         unsigned int               rsvd1:1;              /*     0:20  4 */
> 
> -       /* XXX 4 bits hole, try to pack */
> -       /* Bitfield combined with next fields */
> +       /* XXX 28 bits hole, try to pack */
> 
>         enum {
>                 MYRB_DCDB_TMO_24_HRS = 0,
>                 MYRB_DCDB_TMO_10_SECS = 1,
>                 MYRB_DCDB_TMO_60_SECS = 2,
>                 MYRB_DCDB_TMO_10_MINS = 3,
> -       } timeout:2;                                       /*     1     4 */
> +       } timeout:2;                                       /*     4     1 */
> 
>         /* Bitfield combined with previous fields */
> 
> @@ -324624,10 +324641,10 @@ struct myrb_dcdb {
>         unsigned char              rsvd2;                /*    87     1 */
> 
>         /* size: 88, cachelines: 2, members: 17 */
> -       /* bit holes: 2, sum bit holes: 16 bits */
> +       /* bit holes: 3, sum bit holes: 64 bits */
>         /* last cacheline: 24 bytes */
> 
> -       /* BRAIN FART ALERT! 88 != 83 + 0(holes), diff = 5 */
> +       /* BRAIN FART ALERT! 88 != 86 + 0(holes), diff = 2 */
>  };
> 
> 
> Both DWARF and BTF output emits BRAIN FART ALERT and both can't handle
> 2-bit enums.
> 
> Here's source code definition of that struct:
> 
> struct myrb_dcdb {
>         unsigned target:4;                               /* Byte 0 Bits 0-3 */
>         unsigned channel:4;                              /* Byte 0 Bits 4-7 */
>         enum {
>                 MYRB_DCDB_XFER_NONE =           0,
>                 MYRB_DCDB_XFER_DEVICE_TO_SYSTEM = 1,
>                 MYRB_DCDB_XFER_SYSTEM_TO_DEVICE = 2,
>                 MYRB_DCDB_XFER_ILLEGAL =        3
>         } __packed data_xfer:2;                         /* Byte 1 Bits 0-1 */
>         unsigned early_status:1;                        /* Byte 1 Bit 2 */
>         unsigned rsvd1:1;                               /* Byte 1 Bit 3 */
>         enum {
>                 MYRB_DCDB_TMO_24_HRS =  0,
>                 MYRB_DCDB_TMO_10_SECS = 1,
>                 MYRB_DCDB_TMO_60_SECS = 2,
>                 MYRB_DCDB_TMO_10_MINS = 3
>         } __packed timeout:2;                           /* Byte 1 Bits 4-5 */
>         unsigned no_autosense:1;                        /* Byte 1 Bit 6 */
>         unsigned allow_disconnect:1;                    /* Byte 1 Bit 7 */
>         unsigned short xfer_len_lo;                     /* Bytes 2-3 */
>         u32 dma_addr;                                   /* Bytes 4-7 */
>         unsigned char cdb_len:4;                        /* Byte 8 Bits 0-3 */
>         unsigned char xfer_len_hi4:4;                   /* Byte 8 Bits 4-7 */
>         unsigned char sense_len;                        /* Byte 9 */
>         unsigned char cdb[12];                          /* Bytes 10-21 */
>         unsigned char sense[64];                        /* Bytes 22-85 */
>         unsigned char status;                           /* Byte 86 */
>         unsigned char rsvd2;                            /* Byte 87 */
> };
> 
> I've checked raw BTF data for that struct:
> 
> [12835109] <STRUCT> 'myrb_dcdb' (17 members)
>     #00 target (off=0, sz=4) --> [12832925]
>     #01 channel (off=4, sz=4) --> [12832925]
>     #02 data_xfer (off=32, sz=2) --> [12835107]
>     #03 early_status (off=10, sz=1) --> [12832925]
>     #04 rsvd1 (off=11, sz=1) --> [12832925]
>     #05 timeout (off=36, sz=2) --> [12835108]
>     #06 no_autosense (off=14, sz=1) --> [12832925]
>     #07 allow_disconnect (off=15, sz=1) --> [12832925]
>     #08 xfer_len_lo (off=16, sz=0) --> [12832933]
>     #09 dma_addr (off=32, sz=0) --> [12832946]
>     #10 cdb_len (off=64, sz=4) --> [12832929]
>     #11 xfer_len_hi4 (off=68, sz=4) --> [12832929]
>     #12 sense_len (off=72, sz=0) --> [12832929]
>     #13 cdb (off=80, sz=0) --> [12835084]
>     #14 sense (off=176, sz=0) --> [12835110]
>     #15 status (off=688, sz=0) --> [12832929]
>     #16 rsvd2 (off=696, sz=0) --> [12832929]
> 
> off is a bit field offset, sz is bitfield size (0 if not bitfield).
> 
> Notice how data_xfer has correct sz=2, but off=32 is totally wrong and
> should be 8. Similar issue with timeout with sz=2 and off=36 (should
> be 12). So there seem to be some problem when doing DWARF to BTF
> conversion. I haven't had chance to dig deeper, I'll try to create a
> small repro and dig deeper when I get time (it's really hard to work
> with allyesconfig anything due to humongous sizes of data/log/output).

Right, what I'm doing now is to pick the structs that and having things
like:

[acme@quaco pahole]$ size examples/DAC960.o
   text	   data	    bss	    dec	    hex	filename
      0	      0	      0	      0	      0	examples/DAC960.o
[acme@quaco pahole]$ ls -la examples/DAC960.o
-rwxrwxr-x. 1 acme acme 10736 Mar  8 11:07 examples/DAC960.o
[acme@quaco pahole]$ ls -la examples/DAC960.c 
-rw-rw-r--. 1 acme acme 4480 Mar  8 11:04 examples/DAC960.c
[acme@quaco pahole]$ pahole --sizes examples/DAC960.o
myrb_enquiry2	128	0
[acme@quaco pahole]
[acme@quaco pahole]$ btfdiff examples/DAC960.o
--- /tmp/btfdiff.dwarf.VMTvcw	2019-03-11 11:51:07.858695537 -0300
+++ /tmp/btfdiff.btf.f75DjU	2019-03-11 11:51:07.860695576 -0300
@@ -52,18 +52,18 @@ struct myrb_enquiry2 {
 			MYRB_RAM_TYPE_EDO = 1,
 			MYRB_RAM_TYPE_SDRAM = 2,
 			MYRB_RAM_TYPE_Last = 7,
-		} ram:3;                                   /*    40     4 */
+		} ram:3;                                   /*    43     1 */
 		enum {
 			MYRB_ERR_CORR_None = 0,
 			MYRB_ERR_CORR_Parity = 1,
 			MYRB_ERR_CORR_ECC = 2,
 			MYRB_ERR_CORR_Last = 7,
-		} ec:3;                                    /*    40     4 */
-		unsigned char      fast_page:1;          /*    40: 1  1 */
-		unsigned char      low_power:1;          /*    40: 0  1 */
+		} ec:3;                                    /*    43     1 */
 
-		/* Bitfield combined with next fields */
+		/* Bitfield combined with previous fields */
 
+		unsigned char      fast_page:1;          /*    40: 1  1 */
+		unsigned char      low_power:1;          /*    40: 0  1 */
 		unsigned char      rsvd4;                /*    41     1 */
 	} mem_type;                                      /*    40     2 */
 	short unsigned int         clock_speed;          /*    42     2 */
@@ -94,18 +94,18 @@ struct myrb_enquiry2 {
 			MYRB_WIDTH_NARROW_8BIT = 0,
 			MYRB_WIDTH_WIDE_16BIT = 1,
 			MYRB_WIDTH_WIDE_32BIT = 2,
-		} bus_width:2;                             /*   106     4 */
+		} bus_width:2;                             /*   109     1 */
 		enum {
 			MYRB_SCSI_SPEED_FAST = 0,
 			MYRB_SCSI_SPEED_ULTRA = 1,
 			MYRB_SCSI_SPEED_ULTRA2 = 2,
-		} bus_speed:2;                             /*   106     4 */
+		} bus_speed:2;                             /*   109     1 */
+
+		/* Bitfield combined with previous fields */
+
 		unsigned char      differential:1;       /*   106: 3  1 */
 		unsigned char      rsvd10:3;             /*   106: 0  1 */
 	} scsi_cap;                                      /*   106     1 */
-
-	/* XXX last struct has 65533 bytes of padding */
-
 	unsigned char              rsvd11[5];            /*   107     5 */
 	short unsigned int         fw_build;             /*   112     2 */
 	enum {
@@ -127,5 +127,4 @@ struct myrb_enquiry2 {
 	unsigned char              rsvd14[8];            /*   120     8 */
 
 	/* size: 128, cachelines: 2, members: 46 */
-	/* paddings: 1, sum paddings: 65533 */
 };
[acme@quaco pahole]$ 
 
> In any case, what was your plan w.r.t. new release of pahole? Do you
> want to iron out these obscure bitfield problems first and add
> --progress before new version?

--progress can wait, what I would like would be to have btfdiff clean
for that allyesconfig kernel, fixing these last odd cases. Getting the
exact same output (modulo flat arrays and the show_private_classes used
in btfdiff) would inspire more confidence in the whole thing, I think.

I've added your Tested-by to the csets changing uint16_t to uint32_t and
pushing to master now. Will try to spend some time fixing these last
issues.

- Arnaldo