On Mon, Mar 11, 2019 at 7:53 AM Arnaldo Carvalho de Melo
<arnaldo.melo@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxx> wrote:
> > >
> > > On Fri, Mar 8, 2019 at 10:45 AM Arnaldo Carvalho de Melo
> > > <arnaldo.melo@xxxxxxxxx> 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.
Yep, makes sense.
>
> 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.
Sounds good!
>
> - Arnaldo
