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