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).
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?
>
> >
> > - Arnaldo
> >
> > > $ btfdiff examples/vmlinux-aarch64
> > > --- /tmp/btfdiff.dwarf.81KCPb 2019-03-07 18:20:13.153319625 -0300
> > > +++ /tmp/btfdiff.btf.g1QkcZ 2019-03-07 18:20:13.928328675 -0300
>
> <snip>
