On 1/10/19 3:03 PM, Andrii Nakryiko wrote: > On Thu, Jan 10, 2019 at 10:10 AM Arnaldo Carvalho de Melo > <acme@xxxxxxxxxx> wrote: >> >> Em Thu, Jan 10, 2019 at 02:50:32PM -0300, Arnaldo Carvalho de Melo escreveu: >>> So, the size for an enumeration in BTF is in bytes, not in bits, as in >>> CTF, fixed with the patch below, looking at the other cases now. >> >> We're down to packed data structs, probably in the BTF encoder, see >> below. >> >> $ diff -u /tmp/tcp.o.dwarf.c /tmp/tcp.o.btf.c >> --- /tmp/tcp.o.dwarf.c 2019-01-10 12:10:26.890521322 -0300 >> +++ /tmp/tcp.o.btf.c 2019-01-10 14:46:04.584743337 -0300 >> @@ -585,11 +585,17 @@ >> __u16 vesapm_off; /* 48 2 */ >> __u16 pages; /* 50 2 */ >> __u16 vesa_attributes; /* 52 2 */ >> - __u32 capabilities; /* 54 4 */ >> - __u32 ext_lfb_base; /* 58 4 */ >> + __u32 capabilities; /* 52 4 */ >> + __u32 ext_lfb_base; /* 56 4 */ >> + >> + /* XXX 2 bytes hole, try to pack */ >> + >> __u8 _reserved[2]; /* 62 2 */ >> >> Were these 4 extra bytes came from? The original struct is: >> >> /* >> * These are set up by the setup-routine at boot-time: >> */ >> >> struct screen_info { >> __u8 orig_x; /* 0x00 */ >> __u8 orig_y; /* 0x01 */ >> __u16 ext_mem_k; /* 0x02 */ >> __u16 orig_video_page; /* 0x04 */ >> __u8 orig_video_mode; /* 0x06 */ >> __u8 orig_video_cols; /* 0x07 */ >> __u8 flags; /* 0x08 */ >> __u8 unused2; /* 0x09 */ >> __u16 orig_video_ega_bx;/* 0x0a */ >> __u16 unused3; /* 0x0c */ >> __u8 orig_video_lines; /* 0x0e */ >> __u8 orig_video_isVGA; /* 0x0f */ >> __u16 orig_video_points;/* 0x10 */ >> >> /* VESA graphic mode -- linear frame buffer */ >> __u16 lfb_width; /* 0x12 */ >> __u16 lfb_height; /* 0x14 */ >> __u16 lfb_depth; /* 0x16 */ >> __u32 lfb_base; /* 0x18 */ >> __u32 lfb_size; /* 0x1c */ >> __u16 cl_magic, cl_offset; /* 0x20 */ >> __u16 lfb_linelength; /* 0x24 */ >> __u8 red_size; /* 0x26 */ >> __u8 red_pos; /* 0x27 */ >> __u8 green_size; /* 0x28 */ >> __u8 green_pos; /* 0x29 */ >> __u8 blue_size; /* 0x2a */ >> __u8 blue_pos; /* 0x2b */ >> __u8 rsvd_size; /* 0x2c */ >> __u8 rsvd_pos; /* 0x2d */ >> __u16 vesapm_seg; /* 0x2e */ >> __u16 vesapm_off; /* 0x30 */ >> __u16 pages; /* 0x32 */ >> __u16 vesa_attributes; /* 0x34 */ >> __u32 capabilities; /* 0x36 */ >> __u32 ext_lfb_base; /* 0x3a */ >> __u8 _reserved[2]; /* 0x3e */ >> } __attribute__((packed)); >> >> $ pahole --hex -F dwarf -C screen_info ~/git/build/v4.20-rc5/net/ipv4/tcp.o >> struct screen_info { >> __u8 orig_x; /* 0 0x1 */ >> __u8 orig_y; /* 0x1 0x1 */ >> __u16 ext_mem_k; /* 0x2 0x2 */ >> __u16 orig_video_page; /* 0x4 0x2 */ >> __u8 orig_video_mode; /* 0x6 0x1 */ >> __u8 orig_video_cols; /* 0x7 0x1 */ >> __u8 flags; /* 0x8 0x1 */ >> __u8 unused2; /* 0x9 0x1 */ >> __u16 orig_video_ega_bx; /* 0xa 0x2 */ >> __u16 unused3; /* 0xc 0x2 */ >> __u8 orig_video_lines; /* 0xe 0x1 */ >> __u8 orig_video_isVGA; /* 0xf 0x1 */ >> __u16 orig_video_points; /* 0x10 0x2 */ >> __u16 lfb_width; /* 0x12 0x2 */ >> __u16 lfb_height; /* 0x14 0x2 */ >> __u16 lfb_depth; /* 0x16 0x2 */ >> __u32 lfb_base; /* 0x18 0x4 */ >> __u32 lfb_size; /* 0x1c 0x4 */ >> __u16 cl_magic; /* 0x20 0x2 */ >> __u16 cl_offset; /* 0x22 0x2 */ >> __u16 lfb_linelength; /* 0x24 0x2 */ >> __u8 red_size; /* 0x26 0x1 */ >> __u8 red_pos; /* 0x27 0x1 */ >> __u8 green_size; /* 0x28 0x1 */ >> __u8 green_pos; /* 0x29 0x1 */ >> __u8 blue_size; /* 0x2a 0x1 */ >> __u8 blue_pos; /* 0x2b 0x1 */ >> __u8 rsvd_size; /* 0x2c 0x1 */ >> __u8 rsvd_pos; /* 0x2d 0x1 */ >> __u16 vesapm_seg; /* 0x2e 0x2 */ >> __u16 vesapm_off; /* 0x30 0x2 */ >> __u16 pages; /* 0x32 0x2 */ >> __u16 vesa_attributes; /* 0x34 0x2 */ >> __u32 capabilities; /* 0x36 0x4 */ >> __u32 ext_lfb_base; /* 0x3a 0x4 */ >> __u8 _reserved[2]; /* 0x3e 0x2 */ >> >> /* size: 64, cachelines: 1, members: 36 */ >> }; >> >> the dwarf loader/pahole printer seem to be doing the right thing, the >> BTF encoder, it seems, has issues with attribute((packed)) structs, see >> below. >> >> [acme@quaco pahole]$ pahole --hex -F dwarf -C screen_info ~/git/build/v4.20-rc5/net/ipv4/tcp.o > /tmp/dwarf >> [acme@quaco pahole]$ pahole --hex -F btf -C screen_info ~/git/build/v4.20-rc5/net/ipv4/tcp.o > /tmp/btf >> [acme@quaco pahole]$ diff -u /tmp/dwarf /tmp/btf >> --- /tmp/dwarf 2019-01-10 15:00:14.946723885 -0300 >> +++ /tmp/btf 2019-01-10 15:00:18.669757798 -0300 >> @@ -32,9 +32,15 @@ >> __u16 vesapm_off; /* 0x30 0x2 */ >> __u16 pages; /* 0x32 0x2 */ >> __u16 vesa_attributes; /* 0x34 0x2 */ >> - __u32 capabilities; /* 0x36 0x4 */ >> + __u32 capabilities; /* 0x34 0x4 */ >> - __u32 ext_lfb_base; /* 0x3a 0x4 */ >> + __u32 ext_lfb_base; /* 0x38 0x4 */ >> + >> + /* XXX 2 bytes hole, try to pack */ >> + >> __u8 _reserved[2]; /* 0x3e 0x2 */ >> >> /* size: 64, cachelines: 1, members: 36 */ >> + /* sum members: 60, holes: 1, sum holes: 2 */ >> + >> + /* BRAIN FART ALERT! 64 != 60 + 2(holes), diff = 2 */ >> }; >> [acme@quaco pahole]$ >> >> Why the BTF encoder or loader is getting this in the wrong place... >> Debugging... >> >> /* size: 64, cachelines: 1, members: 36 */ >> + /* sum members: 60, holes: 1, sum holes: 2 */ >> + >> + /* BRAIN FART ALERT! 64 != 60 + 2(holes), diff = 2 */ >> }; >> struct apm_bios_info { >> __u16 version; /* 0 2 */ >> @@ -630,7 +636,10 @@ >> __u32 sectors_per_track; /* 12 4 */ >> __u64 number_of_sectors; /* 16 8 */ >> __u16 bytes_per_sector; /* 24 2 */ >> - __u32 dpte_ptr; /* 26 4 */ >> + __u32 dpte_ptr; /* 24 4 */ >> >> >> Looks like the same problem as with screen_info, 'dpte_ptr' got >> "unionized" with 'bytes_per_sector' >> >> + >> + /* XXX 2 bytes hole, try to pack */ >> + >> __u16 key; /* 30 2 */ >> __u8 device_path_info_length; /* 32 1 */ >> __u8 reserved2; /* 33 1 */ >> @@ -683,7 +692,10 @@ >> } atapi; /* 56 16 */ >> struct { >> __u16 id; /* 56 2 */ >> - __u64 lun; /* 58 8 */ >> + __u64 lun; /* 56 8 */ >> >> Ditto, here it could be trying to put 'lun' in its natural alignment, >> probably 'struct edd_device_params' is packed like screen_info? Yes... >> include/uapi/linux/edd.h line 72 >> >> + >> + /* XXX 2 bytes hole, try to pack */ >> + >> /* --- cacheline 1 boundary (64 bytes) was 2 bytes ago --- */ >> __u16 reserved1; /* 66 2 */ >> __u32 reserved2; /* 68 4 */ >> @@ -733,7 +745,10 @@ >> __u8 checksum; /* 73 1 */ >> >> /* size: 74, cachelines: 2, members: 18 */ >> + /* sum members: 70, holes: 1, sum holes: 2 */ >> /* last cacheline: 10 bytes */ >> + >> + /* BRAIN FART ALERT! 74 != 70 + 2(holes), diff = 2 */ >> }; >> struct edd_info { >> __u8 device; /* 0 1 */ >> @@ -849,10 +864,13 @@ >> }; >> struct desc_ptr { >> short unsigned int size; /* 0 2 */ >> - long unsigned int address; /* 2 8 */ >> + long unsigned int address; /* 0 8 */ >> >> One more packed struct: >> >> struct desc_ptr { >> unsigned short size; >> unsigned long address; >> } __attribute__((packed)) ; >> >> /* size: 10, cachelines: 1, members: 2 */ >> + /* padding: 2 */ >> /* last cacheline: 10 bytes */ >> + >> + /* BRAIN FART ALERT! 10 != 2 + 0(holes), diff = 8 */ >> }; >> struct pgprot { >> pgprotval_t pgprot; /* 0 8 */ >> @@ -1467,10 +1485,13 @@ >> }; >> struct x86_hw_tss { >> >> Also a packed struct >> >> u32 reserved1; /* 0 4 */ >> - u64 sp0; /* 4 8 */ >> - u64 sp1; /* 12 8 */ >> - u64 sp2; /* 20 8 */ >> - u64 reserved2; /* 28 8 */ >> + u64 sp0; /* 0 8 */ >> + u64 sp1; /* 8 8 */ >> + u64 sp2; /* 16 8 */ >> + u64 reserved2; /* 24 8 */ >> + >> + /* XXX 4 bytes hole, try to pack */ >> + >> u64 ist[7]; /* 36 56 */ >> /* --- cacheline 1 boundary (64 bytes) was 28 bytes ago --- */ >> u32 reserved3; /* 92 4 */ >> @@ -1479,7 +1500,10 @@ >> u16 io_bitmap_base; /* 102 2 */ >> >> /* size: 104, cachelines: 2, members: 10 */ >> + /* sum members: 96, holes: 1, sum holes: 4 */ >> /* last cacheline: 40 bytes */ >> + >> + /* BRAIN FART ALERT! 104 != 96 + 4(holes), diff = 4 */ >> }; >> struct seq_operations { >> void * (*start)(struct seq_file *, loff_t *); /* 0 8 */ >> >> >> This last one is interesting, was this done on purpose? I think one >> level of indirection more and we would keep the original expressiveness. >> >> @@ -8473,7 +8497,7 @@ >> struct lruvec_stat * lruvec_stat_cpu; /* 136 8 */ >> atomic_long_t lruvec_stat[30]; /* 144 240 */ >> /* --- cacheline 6 boundary (384 bytes) --- */ >> - long unsigned int lru_zone_size[5][5]; /* 384 200 */ >> + long unsigned int lru_zone_size[25]; /* 384 200 */ >> /* --- cacheline 9 boundary (576 bytes) was 8 bytes ago --- */ >> struct mem_cgroup_reclaim_iter iter[13]; /* 584 208 */ >> /* --- cacheline 12 boundary (768 bytes) was 24 bytes ago --- */ >> >> - Arnaldo > > There appears to be problems with bitfields in both DWARF data and > pahole's conversion from DWARF to BTF (see test file source code and > pahole output at the bottom). > > 1. pahole believes unpacked.y1 has offset 7, which is incorrect > (should be 32?). > unpacked.y2 has offset 32, which also doesn't make sense. > DWARF info for unpacked.y1 and unpacked.y2 also seem wrong (how > do we get offset 18 for y1?). > > <2><63>: Abbrev Number: 3 (DW_TAG_member) > <64> DW_AT_name : y1 > <67> DW_AT_decl_file : 1 > <68> DW_AT_decl_line : 5 > <69> DW_AT_type : <0x87> > <6d> DW_AT_byte_size : 4 > <6e> DW_AT_bit_size : 7 > <6f> DW_AT_bit_offset : 18 > <70> DW_AT_data_member_location: 0 > <2><71>: Abbrev Number: 3 (DW_TAG_member) > <72> DW_AT_name : y2 > <75> DW_AT_decl_file : 1 > <76> DW_AT_decl_line : 6 > <77> DW_AT_type : <0x87> > <7b> DW_AT_byte_size : 4 > <7c> DW_AT_bit_size : 20 > <7d> DW_AT_bit_offset : 12 > <7e> DW_AT_data_member_location: 4 > > > 2. Similar problems with packed.y1 and packed.y2. packed.y2 offset > is negative value even in DWARF: > > <2><d2>: Abbrev Number: 6 (DW_TAG_member) > <d3> DW_AT_name : y2 > <d6> DW_AT_decl_file : 1 > <d7> DW_AT_decl_line : 14 > <d8> DW_AT_type : <0x87> > <dc> DW_AT_byte_size : 4 > <dd> DW_AT_bit_size : 20 > <de> DW_AT_bit_offset : -2 > <df> DW_AT_data_member_location: 0 Did some research on why we got -2 as bit_offset with the above packed data structure. First, the compiler generates correct thing. the negative DW_AT_bit_offset is allowed for dwarf2, but not dwarf4. The "-2" here means (1). the member is crossing two adjacent "4" bytes boundary (DW_AT_byte_size = 4), and (2). the -2 means the the starting bit is at the "4" byte boundary - 2 bits. So basically it covers: | | V V [0 1 .....30 31][0 .. 17 18 ... 31] totally 20 bits. The following patch fixed the issue: -bash-4.4$ git diff diff --git a/dwarves.h b/dwarves.h index e6bffe8..e5f8347 100644 --- a/dwarves.h +++ b/dwarves.h @@ -831,7 +831,7 @@ struct class_member { uint32_t bit_size; uint32_t byte_offset; size_t byte_size; - uint8_t bitfield_offset; + int8_t bitfield_offset; uint8_t bitfield_size; uint8_t bit_hole; uint8_t bitfield_end:1; -bash-4.4$ With the above change: -bash-4.4$ ~/work/github/pahole/build/pahole -JV bitfields.o File bitfields.o: [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED [2] STRUCT packed kind_flag=1 size=5 vlen=5 x1 type_id=3 bitfield_size=1 bits_offset=0 x2 type_id=3 bitfield_size=3 bits_offset=1 x3 type_id=3 bitfield_size=3 bits_offset=4 y1 type_id=1 bitfield_size=7 bits_offset=7 y2 type_id=1 bitfield_size=20 bits_offset=14 [3] INT char size=1 bit_offset=0 nr_bits=8 encoding=(none) [4] STRUCT unpacked kind_flag=1 size=8 vlen=5 x1 type_id=3 bitfield_size=1 bits_offset=0 x2 type_id=3 bitfield_size=3 bits_offset=1 x3 type_id=3 bitfield_size=3 bits_offset=4 y1 type_id=1 bitfield_size=7 bits_offset=7 y2 type_id=1 bitfield_size=20 bits_offset=32 [5] UNION packed_union kind_flag=0 size=8 vlen=4 x1 type_id=3 bits_offset=0 y2 type_id=1 bits_offset=0 s1 type_id=2 bits_offset=0 s2 type_id=4 bits_offset=0 [6] UNION unpacked_union kind_flag=0 size=8 vlen=3 x1 type_id=3 bits_offset=0 y2 type_id=1 bits_offset=0 s type_id=4 bits_offset=0 -bash-4.4$ Will send out a patch shortly. > > andriin@devvm$ cat bitfields.c > struct unpacked{ > char x1: 1; > char x2: 3; > char x3: 3; > int y1: 7; > int y2: 20; > }; > > struct packed{ > char x1: 1; > char x2: 3; > char x3: 3; > int y1: 7; > int y2: 20; > } __attribute__((packed)); > > union packed_union { > char x1; > int y2; > struct packed s1; > struct unpacked s2; > }; > > union unpacked_union { > char x1; > int y2; > struct unpacked s; > } __attribute__((packed)); > > int main() { > struct packed s1; > struct unpacked s2; > union packed_union u1; > union unpacked_union u2; > return 0; > } > andriin@devvm$ cc -g bitfields.c -o bitfields > andriin@devvm$ LLVM_OBJCOPY=objcopy ~/local/pahole/build/pahole -JV bitfields > File bitfields: > [1] STRUCT unpacked kind_flag=1 size=8 vlen=5 > x1 type_id=2 bitfield_size=1 bits_offset=0 > x2 type_id=2 bitfield_size=3 bits_offset=1 > x3 type_id=2 bitfield_size=3 bits_offset=4 > y1 type_id=3 bitfield_size=7 bits_offset=7 > y2 type_id=3 bitfield_size=20 bits_offset=32 > [2] INT char size=1 bit_offset=0 nr_bits=8 encoding=(none) > [3] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED > [4] STRUCT packed kind_flag=1 size=5 vlen=5 > x1 type_id=2 bitfield_size=1 bits_offset=0 > x2 type_id=2 bitfield_size=3 bits_offset=1 > x3 type_id=2 bitfield_size=3 bits_offset=4 > y1 type_id=3 bitfield_size=7 bits_offset=7 > y2 type_id=3 bitfield_size=255 bits_offset=16776974 > [5] UNION packed_union kind_flag=0 size=8 vlen=4 > x1 type_id=2 bits_offset=0 > y2 type_id=3 bits_offset=0 > s1 type_id=4 bits_offset=0 > s2 type_id=1 bits_offset=0 > [6] UNION unpacked_union kind_flag=0 size=8 vlen=3 > x1 type_id=2 bits_offset=0 > y2 type_id=3 bits_offset=0 > s type_id=1 bits_offset=0 > andriin@devvm$ readelf -wi bitfields > Contents of the .debug_info section: > > ... > > <1><2d>: Abbrev Number: 2 (DW_TAG_structure_type) > <2e> DW_AT_name : (indirect string, offset: 0x4e): unpacked > <32> DW_AT_byte_size : 8 > <33> DW_AT_decl_file : 1 > <34> DW_AT_decl_line : 1 > <35> DW_AT_sibling : <0x80> > <2><39>: Abbrev Number: 3 (DW_TAG_member) > <3a> DW_AT_name : x1 > <3d> DW_AT_decl_file : 1 > <3e> DW_AT_decl_line : 2 > <3f> DW_AT_type : <0x80> > <43> DW_AT_byte_size : 1 > <44> DW_AT_bit_size : 1 > <45> DW_AT_bit_offset : 7 > <46> DW_AT_data_member_location: 0 > <2><47>: Abbrev Number: 3 (DW_TAG_member) > <48> DW_AT_name : x2 > <4b> DW_AT_decl_file : 1 > <4c> DW_AT_decl_line : 3 > <4d> DW_AT_type : <0x80> > <51> DW_AT_byte_size : 1 > <52> DW_AT_bit_size : 3 > <53> DW_AT_bit_offset : 4 > <54> DW_AT_data_member_location: 0 > <2><55>: Abbrev Number: 3 (DW_TAG_member) > <56> DW_AT_name : x3 > <59> DW_AT_decl_file : 1 > <5a> DW_AT_decl_line : 4 > <5b> DW_AT_type : <0x80> > <5f> DW_AT_byte_size : 1 > <60> DW_AT_bit_size : 3 > <61> DW_AT_bit_offset : 1 > <62> DW_AT_data_member_location: 0 > <2><63>: Abbrev Number: 3 (DW_TAG_member) > <64> DW_AT_name : y1 > <67> DW_AT_decl_file : 1 > <68> DW_AT_decl_line : 5 > <69> DW_AT_type : <0x87> > <6d> DW_AT_byte_size : 4 > <6e> DW_AT_bit_size : 7 > <6f> DW_AT_bit_offset : 18 > <70> DW_AT_data_member_location: 0 > <2><71>: Abbrev Number: 3 (DW_TAG_member) > <72> DW_AT_name : y2 > <75> DW_AT_decl_file : 1 > <76> DW_AT_decl_line : 6 > <77> DW_AT_type : <0x87> > <7b> DW_AT_byte_size : 4 > <7c> DW_AT_bit_size : 20 > <7d> DW_AT_bit_offset : 12 > <7e> DW_AT_data_member_location: 4 > <2><7f>: Abbrev Number: 0 > <1><80>: Abbrev Number: 4 (DW_TAG_base_type) > <81> DW_AT_byte_size : 1 > <82> DW_AT_encoding : 6 (signed char) > <83> DW_AT_name : (indirect string, offset: 0xb3): char > <1><87>: Abbrev Number: 5 (DW_TAG_base_type) > <88> DW_AT_byte_size : 4 > <89> DW_AT_encoding : 5 (signed) > <8a> DW_AT_name : int > <1><8e>: Abbrev Number: 2 (DW_TAG_structure_type) > <8f> DW_AT_name : (indirect string, offset: 0x50): packed > <93> DW_AT_byte_size : 5 > <94> DW_AT_decl_file : 1 > <95> DW_AT_decl_line : 9 > <96> DW_AT_sibling : <0xe1> > <2><9a>: Abbrev Number: 3 (DW_TAG_member) > <9b> DW_AT_name : x1 > <9e> DW_AT_decl_file : 1 > <9f> DW_AT_decl_line : 10 > <a0> DW_AT_type : <0x80> > <a4> DW_AT_byte_size : 1 > <a5> DW_AT_bit_size : 1 > <a6> DW_AT_bit_offset : 7 > <a7> DW_AT_data_member_location: 0 > <2><a8>: Abbrev Number: 3 (DW_TAG_member) > <a9> DW_AT_name : x2 > <ac> DW_AT_decl_file : 1 > <ad> DW_AT_decl_line : 11 > <ae> DW_AT_type : <0x80> > <b2> DW_AT_byte_size : 1 > <b3> DW_AT_bit_size : 3 > <b4> DW_AT_bit_offset : 4 > <b5> DW_AT_data_member_location: 0 > <2><b6>: Abbrev Number: 3 (DW_TAG_member) > <b7> DW_AT_name : x3 > <ba> DW_AT_decl_file : 1 > <bb> DW_AT_decl_line : 12 > <bc> DW_AT_type : <0x80> > <c0> DW_AT_byte_size : 1 > <c1> DW_AT_bit_size : 3 > <c2> DW_AT_bit_offset : 1 > <c3> DW_AT_data_member_location: 0 > <2><c4>: Abbrev Number: 3 (DW_TAG_member) > <c5> DW_AT_name : y1 > <c8> DW_AT_decl_file : 1 > <c9> DW_AT_decl_line : 13 > <ca> DW_AT_type : <0x87> > <ce> DW_AT_byte_size : 4 > <cf> DW_AT_bit_size : 7 > <d0> DW_AT_bit_offset : 18 > <d1> DW_AT_data_member_location: 0 > <2><d2>: Abbrev Number: 6 (DW_TAG_member) > <d3> DW_AT_name : y2 > <d6> DW_AT_decl_file : 1 > <d7> DW_AT_decl_line : 14 > <d8> DW_AT_type : <0x87> > <dc> DW_AT_byte_size : 4 > <dd> DW_AT_bit_size : 20 > <de> DW_AT_bit_offset : -2 > <df> DW_AT_data_member_location: 0 > <2><e0>: Abbrev Number: 0 > > ... >