* [PATCH pahole 0/3] fix handling of bitfields in btf_loader
@ 2019-02-20 20:57 Andrii Nakryiko
2019-02-20 20:57 ` [PATCH pahole 1/3] btfdiff: support specifying custom pahole location Andrii Nakryiko
` (3 more replies)
0 siblings, 4 replies; 23+ messages in thread
From: Andrii Nakryiko @ 2019-02-20 20:57 UTC (permalink / raw)
To: andrii.nakryiko, acme, dwarves, bpf, ast, yhs; +Cc: Andrii Nakryiko
This patchset fixes the logic of determining bitfield_offsets and
initial bit_offset when using BTF type information. It eliminates all
the remaining discrepancies, when doing btfdiff on vmlinux image, module
two instances of incorrectly reporting struct member type name when
bitfield is the very first field in a struct, which is only happening
when using DWARF data.
Patch #1 makes btfdiff script easier to use during local development.
Patch #2 fixes list of known base type names to handle clang-generated
type descriptions better.
Patch #3 fixes bitfield handling logic in btf_loader.
Andrii Nakryiko (3):
btfdiff: support specifying custom pahole location
pahole: complete list of base type names
btf_loader: fix bitfield fixup code
btf_loader.c | 21 ++++++++++++++-------
btfdiff | 5 +++--
dwarves.c | 3 +++
3 files changed, 20 insertions(+), 9 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH pahole 1/3] btfdiff: support specifying custom pahole location 2019-02-20 20:57 [PATCH pahole 0/3] fix handling of bitfields in btf_loader Andrii Nakryiko @ 2019-02-20 20:57 ` Andrii Nakryiko 2019-02-20 20:57 ` [PATCH pahole 2/3] pahole: complete list of base type names Andrii Nakryiko ` (2 subsequent siblings) 3 siblings, 0 replies; 23+ messages in thread From: Andrii Nakryiko @ 2019-02-20 20:57 UTC (permalink / raw) To: andrii.nakryiko, acme, dwarves, bpf, ast, yhs; +Cc: Andrii Nakryiko During development it's convenient to be able to specify custom location of pahole binary, built locally. E.g.: $ PAHOLE=~/local/pahole/build/pahole ./btfdiff /tmp/vmlinux4 Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- btfdiff | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/btfdiff b/btfdiff index 2b7ae33..6289a42 100755 --- a/btfdiff +++ b/btfdiff @@ -14,9 +14,10 @@ fi file=$1 btf_output=$(mktemp /tmp/btfdiff.btf.XXXXXX) dwarf_output=$(mktemp /tmp/btfdiff.dwarf.XXXXXX) +pahole_bin=${PAHOLE-"pahole"} -pahole -F dwarf --flat_arrays --show_private_classes $file > $dwarf_output -pahole -F btf $file > $btf_output +${pahole_bin} -F dwarf --flat_arrays --show_private_classes $file > $dwarf_output +${pahole_bin} -F btf $file > $btf_output diff -up $dwarf_output $btf_output -- 2.17.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH pahole 2/3] pahole: complete list of base type names 2019-02-20 20:57 [PATCH pahole 0/3] fix handling of bitfields in btf_loader Andrii Nakryiko 2019-02-20 20:57 ` [PATCH pahole 1/3] btfdiff: support specifying custom pahole location Andrii Nakryiko @ 2019-02-20 20:57 ` Andrii Nakryiko 2019-02-20 20:57 ` [PATCH pahole 3/3] btf_loader: fix bitfield fixup code Andrii Nakryiko 2019-02-22 22:02 ` [PATCH pahole 0/3] fix handling of bitfields in btf_loader Arnaldo Carvalho de Melo 3 siblings, 0 replies; 23+ messages in thread From: Andrii Nakryiko @ 2019-02-20 20:57 UTC (permalink / raw) To: andrii.nakryiko, acme, dwarves, bpf, ast, yhs; +Cc: Andrii Nakryiko Clang seems to generate base type with name "short", instead of "short in", but it also isn't inconceivable to imagine other compilers generating just "long" and/or "long long". This patch adds all those short forms to a list of base type names. $ cat type_test.c struct s { short x1; long x2; long long x3; }; int main() { struct s s; return 0; } $ clang -g type_test.c -o type_test && ~/local/pahole/build/pahole -JV type_test File type_test: [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED [2] STRUCT s kind_flag=0 size=24 vlen=3 x1 type_id=3 bits_offset=0 x2 type_id=4 bits_offset=64 x3 type_id=5 bits_offset=128 [3] INT short size=2 bit_offset=0 nr_bits=16 encoding=SIGNED [4] INT long int size=8 bit_offset=0 nr_bits=64 encoding=SIGNED [5] INT long long int size=8 bit_offset=0 nr_bits=64 encoding=SIGNED Before: $ ~/local/pahole/build/pahole -F btf type_test base_type__name_to_size: base_type short class__fixup_btf_bitfields: unknown base type name "short"! struct s { short x1; /* 0 0 */ /* XXX 8 bytes hole, try to pack */ long int x2; /* 8 8 */ long long int x3; /* 16 8 */ /* size: 24, cachelines: 1, members: 3 */ /* sum members: 16, holes: 1, sum holes: 8 */ /* last cacheline: 24 bytes */ }; After: $ ~/local/pahole/build/pahole -F btf type_test struct s { short x1; /* 0 2 */ /* XXX 6 bytes hole, try to pack */ long int x2; /* 8 8 */ long long int x3; /* 16 8 */ /* size: 24, cachelines: 1, members: 3 */ /* sum members: 18, holes: 1, sum holes: 6 */ /* last cacheline: 24 bytes */ }; Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- dwarves.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dwarves.c b/dwarves.c index 093de11..adc411a 100644 --- a/dwarves.c +++ b/dwarves.c @@ -147,11 +147,13 @@ static struct base_type_name_to_size { { .name = "signed short", .size = 16, }, { .name = "unsigned short", .size = 16, }, { .name = "short int", .size = 16, }, + { .name = "short", .size = 16, }, { .name = "char", .size = 8, }, { .name = "signed char", .size = 8, }, { .name = "unsigned char", .size = 8, }, { .name = "signed long", .size = 0, }, { .name = "long int", .size = 0, }, + { .name = "long", .size = 0, }, { .name = "signed long", .size = 0, }, { .name = "unsigned long", .size = 0, }, { .name = "long unsigned int", .size = 0, }, @@ -159,6 +161,7 @@ static struct base_type_name_to_size { { .name = "_Bool", .size = 8, }, { .name = "long long unsigned int", .size = 64, }, { .name = "long long int", .size = 64, }, + { .name = "long long", .size = 64, }, { .name = "signed long long", .size = 64, }, { .name = "unsigned long long", .size = 64, }, { .name = "double", .size = 64, }, -- 2.17.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH pahole 3/3] btf_loader: fix bitfield fixup code 2019-02-20 20:57 [PATCH pahole 0/3] fix handling of bitfields in btf_loader Andrii Nakryiko 2019-02-20 20:57 ` [PATCH pahole 1/3] btfdiff: support specifying custom pahole location Andrii Nakryiko 2019-02-20 20:57 ` [PATCH pahole 2/3] pahole: complete list of base type names Andrii Nakryiko @ 2019-02-20 20:57 ` Andrii Nakryiko 2019-02-22 22:02 ` [PATCH pahole 0/3] fix handling of bitfields in btf_loader Arnaldo Carvalho de Melo 3 siblings, 0 replies; 23+ messages in thread From: Andrii Nakryiko @ 2019-02-20 20:57 UTC (permalink / raw) To: andrii.nakryiko, acme, dwarves, bpf, ast, yhs; +Cc: Andrii Nakryiko Existing code assumes alignment of any integer type, which breaks for packed structs. This patch fixes all the current discrepanies between dwarf and btf loader, when compared using btfdiff. It preserves bit_offset of non-bitfield members, while for bitfield ones it re-calculates initial byte/bit offset using natural alignment of the underlying integer type, which seems to be always the case for bitfields. I've tested this on toy examples for both x86-64 and arm targets, there were no differences reported by btfdiff. Testing on vmlinux on x86-64 shows only these discrepancies, which are unrelated to bit offsets: $ ./btfdiff /tmp/vmlinux4 --- /tmp/btfdiff.dwarf.GIVfpr 2019-02-20 12:18:29.138788970 -0800 +++ /tmp/btfdiff.btf.c3x2KY 2019-02-20 12:18:29.351786365 -0800 @@ -16884,7 +16884,7 @@ struct pebs_record_nhm { }; union hsw_tsx_tuning { struct { - unsigned int cycles_last_block:32; /* 0: 0 4 */ + u32 cycles_last_block:32; /* 0: 0 4 */ u32 hle_abort:1; /* 4:31 4 */ u32 rtm_abort:1; /* 4:30 4 */ u32 instruction_abort:1; /* 4:29 4 */ @@ -26154,7 +26154,7 @@ struct acpi_device_power { /* last cacheline: 40 bytes */ }; struct acpi_device_perf_flags { - unsigned char reserved:8; /* 0: 0 1 */ + u8 reserved:8; /* 0: 0 1 */ /* size: 1, cachelines: 1, members: 1 */ /* last cacheline: 1 bytes */ Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- btf_loader.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/btf_loader.c b/btf_loader.c index fc28884..62e7e30 100644 --- a/btf_loader.c +++ b/btf_loader.c @@ -502,19 +502,26 @@ static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu, struct btf * such unlikely thing happens. */ pos->byte_size = integral_bit_size / 8; + pos->bit_size = type_bit_size; if (integral_bit_size == 0) { - pos->bit_size = integral_bit_size; + pos->bit_size = 0; continue; } - pos->bitfield_offset = pos->bit_offset % integral_bit_size; - if (!btfe->is_big_endian) - pos->bitfield_offset = integral_bit_size - pos->bitfield_offset - pos->bitfield_size; + if (pos->bitfield_size) { + /* bitfields seem to be always aligned, no matter the packing */ + pos->byte_offset = pos->bit_offset / integral_bit_size * integral_bit_size / 8; + } else { + pos->byte_offset = pos->bit_offset / 8; + } + - pos->bit_size = type_bit_size; - pos->byte_offset = (((pos->bit_offset / integral_bit_size) * - integral_bit_size) / 8); + if (pos->bitfield_size) { + pos->bitfield_offset = pos->bit_offset - pos->byte_offset * 8; + if (!btfe->is_big_endian) + pos->bitfield_offset = integral_bit_size - pos->bitfield_offset - pos->bitfield_size; + } } return 0; -- 2.17.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader 2019-02-20 20:57 [PATCH pahole 0/3] fix handling of bitfields in btf_loader Andrii Nakryiko ` (2 preceding siblings ...) 2019-02-20 20:57 ` [PATCH pahole 3/3] btf_loader: fix bitfield fixup code Andrii Nakryiko @ 2019-02-22 22:02 ` Arnaldo Carvalho de Melo 2019-02-22 23:23 ` Andrii Nakryiko 3 siblings, 1 reply; 23+ messages in thread From: Arnaldo Carvalho de Melo @ 2019-02-22 22:02 UTC (permalink / raw) To: Andrii Nakryiko; +Cc: andrii.nakryiko, dwarves, bpf, ast, yhs Em Wed, Feb 20, 2019 at 12:57:29PM -0800, Andrii Nakryiko escreveu: > This patchset fixes the logic of determining bitfield_offsets and > initial bit_offset when using BTF type information. It eliminates all > the remaining discrepancies, when doing btfdiff on vmlinux image, module > two instances of incorrectly reporting struct member type name when > bitfield is the very first field in a struct, which is only happening > when using DWARF data. > > Patch #1 makes btfdiff script easier to use during local development. > > Patch #2 fixes list of known base type names to handle clang-generated > type descriptions better. > > Patch #3 fixes bitfield handling logic in btf_loader. Thanks a lot! Applied. And here I see no differences in my vmlinux: [acme@quaco pahole]$ btfdiff vmlinux [acme@quaco pahole]$ [acme@quaco pahole]$ perf stat pahole -F dwarf --flat_arrays --show_private_classes vmlinux > /tmp/dwarf Performance counter stats for 'pahole -F dwarf --flat_arrays --show_private_classes vmlinux': 36,140.58 msec task-clock:u # 0.998 CPUs utilized 0 context-switches:u # 0.000 K/sec 0 cpu-migrations:u # 0.000 K/sec 359,238 page-faults:u # 0.010 M/sec 83,741,861,962 cycles:u # 2.317 GHz 99,851,434,338 instructions:u # 1.19 insn per cycle 20,974,798,749 branches:u # 580.367 M/sec 288,410,045 branch-misses:u # 1.38% of all branches 36.230455825 seconds time elapsed 35.037146000 seconds user 0.906593000 seconds sys [acme@quaco pahole]$ perf stat pahole -F btf vmlinux > /tmp/btf Performance counter stats for 'pahole -F btf vmlinux': 215.21 msec task-clock:u # 0.994 CPUs utilized 0 context-switches:u # 0.000 K/sec 0 cpu-migrations:u # 0.000 K/sec 3,195 page-faults:u # 0.015 M/sec 460,363,235 cycles:u # 2.139 GHz 546,688,575 instructions:u # 1.19 insn per cycle 104,488,443 branches:u # 485.522 M/sec 2,012,815 branch-misses:u # 1.93% of all branches 0.216609739 seconds time elapsed 0.200431000 seconds user 0.014834000 seconds sys [acme@quaco pahole]$ [acme@quaco pahole]$ wc -l /tmp/dwarf 106101 /tmp/dwarf [acme@quaco pahole]$ wc -l /tmp/btf 106101 /tmp/btf [acme@quaco pahole]$ [acme@quaco pahole]$ pahole -F btf -C sk_buff_head vmlinux struct sk_buff_head { struct sk_buff * next; /* 0 8 */ struct sk_buff * prev; /* 8 8 */ __u32 qlen; /* 16 4 */ spinlock_t lock; /* 20 4 */ /* size: 24, cachelines: 1, members: 4 */ /* last cacheline: 24 bytes */ }; [acme@quaco pahole]$ pahole --expand_types -F btf -C sk_buff_head vmlinux struct sk_buff_head { struct sk_buff * next; /* 0 8 */ struct sk_buff * prev; /* 8 8 */ /* typedef __u32 */ unsigned int qlen; /* 16 4 */ /* typedef spinlock_t */ struct spinlock { union { struct raw_spinlock { /* typedef arch_spinlock_t */ struct qspinlock { union { /* typedef atomic_t */ struct { int counter; /* 20 4 */ } val; /* 20 4 */ struct { /* typedef u8 -> __u8 */ unsigned char locked; /* 20 1 */ /* typedef u8 -> __u8 */ unsigned char pending; /* 21 1 */ }; /* 20 2 */ struct { /* typedef u16 -> __u16 */ short unsigned int locked_pending; /* 20 2 */ /* typedef u16 -> __u16 */ short unsigned int tail; /* 22 2 */ }; /* 20 4 */ }; /* 20 4 */ } raw_lock; /* 20 4 */ } rlock; /* 20 4 */ }; /* 20 4 */ } lock; /* 20 4 */ /* size: 24, cachelines: 1, members: 4 */ /* last cacheline: 24 bytes */ }; [acme@quaco pahole]$ The problem with the packed structs is gone: [acme@quaco pahole]$ pahole -F dwarf -C screen_info vmlinux > /tmp/dwarf [acme@quaco pahole]$ pahole -F btf -C screen_info vmlinux > /tmp/btf [acme@quaco pahole]$ diff -u /tmp/btf /tmp/dwarf [acme@quaco pahole]$ pahole --hex -F btf -C screen_info vmlinux 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 */ }; [acme@quaco pahole]$ And the definition in include/uapi/linux/screen_info.h 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)); Which ones where these: "module two instances of incorrectly reporting struct member type name when bitfield is the very first field in a struct, which is only happening when using DWARF data." ? - Arnaldo ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader 2019-02-22 22:02 ` [PATCH pahole 0/3] fix handling of bitfields in btf_loader Arnaldo Carvalho de Melo @ 2019-02-22 23:23 ` Andrii Nakryiko 2019-02-25 16:02 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 23+ messages in thread From: Andrii Nakryiko @ 2019-02-22 23:23 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Andrii Nakryiko, dwarves, bpf, Alexei Starovoitov, Yonghong Song On Fri, Feb 22, 2019 at 2:02 PM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > > Em Wed, Feb 20, 2019 at 12:57:29PM -0800, Andrii Nakryiko escreveu: > > This patchset fixes the logic of determining bitfield_offsets and > > initial bit_offset when using BTF type information. It eliminates all > > the remaining discrepancies, when doing btfdiff on vmlinux image, module > > two instances of incorrectly reporting struct member type name when > > bitfield is the very first field in a struct, which is only happening > > when using DWARF data. > > > > Patch #1 makes btfdiff script easier to use during local development. > > > > Patch #2 fixes list of known base type names to handle clang-generated > > type descriptions better. > > > > Patch #3 fixes bitfield handling logic in btf_loader. > > Thanks a lot! Applied. > > And here I see no differences in my vmlinux: > <SNIP> > > Which ones where these: "module two instances of incorrectly reporting struct > member type name when bitfield is the very first field in a struct, which is > only happening when using DWARF data." ? $ ./btfdiff /tmp/vmlinux4 --- /tmp/btfdiff.dwarf.GIVfpr 2019-02-20 12:18:29.138788970 -0800 +++ /tmp/btfdiff.btf.c3x2KY 2019-02-20 12:18:29.351786365 -0800 @@ -16884,7 +16884,7 @@ struct pebs_record_nhm { }; union hsw_tsx_tuning { struct { - unsigned int cycles_last_block:32; /* 0: 0 4 */ + u32 cycles_last_block:32; /* 0: 0 4 */ u32 hle_abort:1; /* 4:31 4 */ u32 rtm_abort:1; /* 4:30 4 */ u32 instruction_abort:1; /* 4:29 4 */ @@ -26154,7 +26154,7 @@ struct acpi_device_power { /* last cacheline: 40 bytes */ }; struct acpi_device_perf_flags { - unsigned char reserved:8; /* 0: 0 1 */ + u8 reserved:8; /* 0: 0 1 */ /* size: 1, cachelines: 1, members: 1 */ /* last cacheline: 1 bytes */ For hsw_tsx_tuning, it is defined in sources like this: $ cat hsw_tsx_tuning.c typedef unsigned long long u64; typedef unsigned int u32; union hsw_tsx_tuning { struct { u32 cycles_last_block : 32, hle_abort : 1, rtm_abort : 1, instruction_abort : 1, non_instruction_abort : 1, retry : 1, data_conflict : 1, capacity_writes : 1, capacity_reads : 1; }; u64 value; }; int main() { union hsw_tsx_tuning t; return 0; } Building same defition with gcc and clang reveals some more info. $ cc -g -c hsw_tsx_tuning.c -o hsw_tsx_tuning-gcc && ~/local/pahole/build/pahole -J hsw_tsx_tuning-gcc $ clang -g -c hsw_tsx_tuning.c -o hsw_tsx_tuning-clang && ~/local/pahole/build/pahole -J hsw_tsx_tuning-clang GCC-generated DWARF/BTF exposes this bug: $ PAHOLE=~/local/pahole/build/pahole ~/local/pahole/btfdiff hsw_tsx_tuning-gcc --- /tmp/btfdiff.dwarf.khRiFg 2019-02-22 15:18:46.768858141 -0800 +++ /tmp/btfdiff.btf.jqdEsR 2019-02-22 15:18:46.770858140 -0800 @@ -1,6 +1,6 @@ union hsw_tsx_tuning { struct { - unsigned int cycles_last_block:32; /* 0: 0 4 */ + u32 cycles_last_block:32; /* 0: 0 4 */ u32 hle_abort:1; /* 4:31 4 */ u32 rtm_abort:1; /* 4:30 4 */ u32 instruction_abort:1; /* 4:29 4 */ But the one generated by clang doesn't: $ PAHOLE=~/local/pahole/build/pahole ~/local/pahole/btfdiff hsw_tsx_tuning-clang The only difference is that GCC correctly marks cycles_last_block as bitfield, while clang optimizes it to stand-alone u32. $ ~/local/pahole/build/pahole -F btf hsw_tsx_tuning-gcc union hsw_tsx_tuning { struct { u32 cycles_last_block:32; /* 0: 0 4 */ u32 hle_abort:1; /* 4:31 4 */ u32 rtm_abort:1; /* 4:30 4 */ u32 instruction_abort:1; /* 4:29 4 */ u32 non_instruction_abort:1; /* 4:28 4 */ u32 retry:1; /* 4:27 4 */ u32 data_conflict:1; /* 4:26 4 */ u32 capacity_writes:1; /* 4:25 4 */ u32 capacity_reads:1; /* 4:24 4 */ }; /* 0 8 */ u64 value; /* 0 8 */ }; $ ~/local/pahole/build/pahole -F btf hsw_tsx_tuning-clang union hsw_tsx_tuning { struct { u32 cycles_last_block; /* 0 4 */ u32 hle_abort:1; /* 4:31 4 */ u32 rtm_abort:1; /* 4:30 4 */ u32 instruction_abort:1; /* 4:29 4 */ u32 non_instruction_abort:1; /* 4:28 4 */ u32 retry:1; /* 4:27 4 */ u32 data_conflict:1; /* 4:26 4 */ u32 capacity_writes:1; /* 4:25 4 */ u32 capacity_reads:1; /* 4:24 4 */ }; /* 0 8 */ u64 value; /* 0 8 */ }; I've spent a bit of time debugging this, but didn't get far, as I'm pretty unfamiliar with overall flow of decoders, I was hoping this can give you some idea where to look, though. > > - Arnaldo ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader 2019-02-22 23:23 ` Andrii Nakryiko @ 2019-02-25 16:02 ` Arnaldo Carvalho de Melo 2019-02-25 18:59 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 23+ messages in thread From: Arnaldo Carvalho de Melo @ 2019-02-25 16:02 UTC (permalink / raw) To: Andrii Nakryiko Cc: Arnaldo Carvalho de Melo, Andrii Nakryiko, dwarves, bpf, Alexei Starovoitov, Yonghong Song Em Fri, Feb 22, 2019 at 03:23:52PM -0800, Andrii Nakryiko escreveu: > On Fri, Feb 22, 2019 at 2:02 PM Arnaldo Carvalho de Melo > <arnaldo.melo@gmail.com> wrote: > > > > Em Wed, Feb 20, 2019 at 12:57:29PM -0800, Andrii Nakryiko escreveu: > > > This patchset fixes the logic of determining bitfield_offsets and > > > initial bit_offset when using BTF type information. It eliminates all > > > the remaining discrepancies, when doing btfdiff on vmlinux image, module > > > two instances of incorrectly reporting struct member type name when > > > bitfield is the very first field in a struct, which is only happening > > > when using DWARF data. > > > > > > Patch #1 makes btfdiff script easier to use during local development. > > > > > > Patch #2 fixes list of known base type names to handle clang-generated > > > type descriptions better. > > > > > > Patch #3 fixes bitfield handling logic in btf_loader. > > > > Thanks a lot! Applied. > > > > And here I see no differences in my vmlinux: > > > > <SNIP> > > > > > Which ones where these: "module two instances of incorrectly reporting struct > > member type name when bitfield is the very first field in a struct, which is > > only happening when using DWARF data." ? > > $ ./btfdiff /tmp/vmlinux4 > --- /tmp/btfdiff.dwarf.GIVfpr 2019-02-20 12:18:29.138788970 -0800 > +++ /tmp/btfdiff.btf.c3x2KY 2019-02-20 12:18:29.351786365 -0800 > @@ -16884,7 +16884,7 @@ struct pebs_record_nhm { > }; > union hsw_tsx_tuning { > struct { > - unsigned int cycles_last_block:32; /* 0: 0 4 */ > + u32 cycles_last_block:32; /* 0: 0 4 */ $ vim hsw_tsx_tuning.c $ gcc -g -c hsw_tsx_tuning.c -o hsw_tsx_tuning-gcc $ pahole -J hsw_tsx_tuning pahole: hsw_tsx_tuning: No such file or directory $ pahole -J hsw_tsx_tuning-gcc $ $ btfdiff hsw_tsx_tuning-gcc --- /tmp/btfdiff.dwarf.ErXffk 2019-02-25 10:26:56.863625697 -0300 +++ /tmp/btfdiff.btf.Y5EDdM 2019-02-25 10:26:56.864625706 -0300 @@ -1,6 +1,6 @@ union hsw_tsx_tuning { struct { - unsigned int cycles_last_block:32; /* 0: 0 4 */ + u32 cycles_last_block:32; /* 0: 0 4 */ u32 hle_abort:1; /* 4:31 4 */ u32 rtm_abort:1; /* 4:30 4 */ u32 instruction_abort:1; /* 4:29 4 */ $ Reproduced. <2><5c>: Abbrev Number: 5 (DW_TAG_member) <5d> DW_AT_name : (indirect string, offset: 0x23): cycles_last_block <61> DW_AT_decl_file : 1 <62> DW_AT_decl_line : 8 <63> DW_AT_decl_column : 13 <64> DW_AT_type : <0x40> <68> DW_AT_byte_size : 4 <69> DW_AT_bit_size : 32 <6a> DW_AT_bit_offset : 0 <6b> DW_AT_data_member_location: 0 <1><40>: Abbrev Number: 2 (DW_TAG_typedef) <41> DW_AT_name : u32 <45> DW_AT_decl_file : 1 <46> DW_AT_decl_line : 4 <47> DW_AT_decl_column : 22 <48> DW_AT_type : <0x4c> <1><4c>: Abbrev Number: 3 (DW_TAG_base_type) <4d> DW_AT_byte_size : 4 <4e> DW_AT_encoding : 7 (unsigned) <4f> DW_AT_name : (indirect string, offset: 0x16): unsigned int So yeah, the BTF encoder/decoder is working just fine, the problem is in pahole's DWARF code, lemme see... > u32 hle_abort:1; /* 4:31 4 */ > u32 rtm_abort:1; /* 4:30 4 */ > u32 instruction_abort:1; /* 4:29 4 */ > @@ -26154,7 +26154,7 @@ struct acpi_device_power { > /* last cacheline: 40 bytes */ > }; > struct acpi_device_perf_flags { > - unsigned char reserved:8; /* 0: 0 1 */ > + u8 reserved:8; /* 0: 0 1 */ > > /* size: 1, cachelines: 1, members: 1 */ > /* last cacheline: 1 bytes */ > > For hsw_tsx_tuning, it is defined in sources like this: > > $ cat hsw_tsx_tuning.c > typedef unsigned long long u64; > typedef unsigned int u32; > > union hsw_tsx_tuning { > struct { > u32 cycles_last_block : 32, > hle_abort : 1, > rtm_abort : 1, > instruction_abort : 1, > non_instruction_abort : 1, > retry : 1, > data_conflict : 1, > capacity_writes : 1, > capacity_reads : 1; > }; > u64 value; > }; > > int main() { > union hsw_tsx_tuning t; > return 0; > } > > Building same defition with gcc and clang reveals some more info. > > $ cc -g -c hsw_tsx_tuning.c -o hsw_tsx_tuning-gcc && > ~/local/pahole/build/pahole -J hsw_tsx_tuning-gcc > $ clang -g -c hsw_tsx_tuning.c -o hsw_tsx_tuning-clang && > ~/local/pahole/build/pahole -J hsw_tsx_tuning-clang > > GCC-generated DWARF/BTF exposes this bug: > > $ PAHOLE=~/local/pahole/build/pahole ~/local/pahole/btfdiff hsw_tsx_tuning-gcc > --- /tmp/btfdiff.dwarf.khRiFg 2019-02-22 15:18:46.768858141 -0800 > +++ /tmp/btfdiff.btf.jqdEsR 2019-02-22 15:18:46.770858140 -0800 > @@ -1,6 +1,6 @@ > union hsw_tsx_tuning { > struct { > - unsigned int cycles_last_block:32; /* 0: 0 4 */ > + u32 cycles_last_block:32; /* 0: 0 4 */ > u32 hle_abort:1; /* 4:31 4 */ > u32 rtm_abort:1; /* 4:30 4 */ > u32 instruction_abort:1; /* 4:29 4 */ > > But the one generated by clang doesn't: > > $ PAHOLE=~/local/pahole/build/pahole ~/local/pahole/btfdiff hsw_tsx_tuning-clang > > The only difference is that GCC correctly marks cycles_last_block as > bitfield, while clang optimizes it to stand-alone u32. > > $ ~/local/pahole/build/pahole -F btf hsw_tsx_tuning-gcc > union hsw_tsx_tuning { > struct { > u32 cycles_last_block:32; /* 0: 0 4 */ > u32 hle_abort:1; /* 4:31 4 */ > u32 rtm_abort:1; /* 4:30 4 */ > u32 instruction_abort:1; /* 4:29 4 */ > u32 non_instruction_abort:1; /* 4:28 4 */ > u32 retry:1; /* 4:27 4 */ > u32 data_conflict:1; /* 4:26 4 */ > u32 capacity_writes:1; /* 4:25 4 */ > u32 capacity_reads:1; /* 4:24 4 */ > }; /* 0 8 */ > u64 value; /* 0 8 */ > }; > > $ ~/local/pahole/build/pahole -F btf hsw_tsx_tuning-clang > union hsw_tsx_tuning { > struct { > u32 cycles_last_block; /* 0 4 */ > u32 hle_abort:1; /* 4:31 4 */ > u32 rtm_abort:1; /* 4:30 4 */ > u32 instruction_abort:1; /* 4:29 4 */ > u32 non_instruction_abort:1; /* 4:28 4 */ > u32 retry:1; /* 4:27 4 */ > u32 data_conflict:1; /* 4:26 4 */ > u32 capacity_writes:1; /* 4:25 4 */ > u32 capacity_reads:1; /* 4:24 4 */ > }; /* 0 8 */ > u64 value; /* 0 8 */ > }; > > I've spent a bit of time debugging this, but didn't get far, as I'm > pretty unfamiliar with overall flow of decoders, I was hoping this can > give you some idea where to look, though. > > > > > - Arnaldo -- - Arnaldo ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader 2019-02-25 16:02 ` Arnaldo Carvalho de Melo @ 2019-02-25 18:59 ` Arnaldo Carvalho de Melo 2019-02-25 19:42 ` Andrii Nakryiko 0 siblings, 1 reply; 23+ messages in thread From: Arnaldo Carvalho de Melo @ 2019-02-25 18:59 UTC (permalink / raw) To: Andrii Nakryiko Cc: Arnaldo Carvalho de Melo, Andrii Nakryiko, dwarves, bpf, Alexei Starovoitov, Yonghong Song Em Mon, Feb 25, 2019 at 01:02:39PM -0300, Arnaldo Carvalho de Melo escreveu: > So yeah, the BTF encoder/decoder is working just fine, the problem is in > pahole's DWARF code, lemme see... Please try the patch below, for me btfdiff continues to show no diff for all types in my vmlinux and now it also produces the same output for when the first element of a bitfield has its bit_size equal to its byte_size * 8: [acme@quaco pahole]$ btfdiff vmlinux [acme@quaco pahole]$ btfdiff examples/andrii/hsw_tsx_tuning-gcc [acme@quaco pahole]$ btfdiff examples/andrii/hsw_tsx_tuning-clang [acme@quaco pahole]$ pahole examples/andrii/hsw_tsx_tuning-clang union hsw_tsx_tuning { struct { u32 cycles_last_block; /* 0 4 */ u32 hle_abort:1; /* 4:31 4 */ u32 rtm_abort:1; /* 4:30 4 */ u32 instruction_abort:1; /* 4:29 4 */ u32 non_instruction_abort:1; /* 4:28 4 */ u32 retry:1; /* 4:27 4 */ u32 data_conflict:1; /* 4:26 4 */ u32 capacity_writes:1; /* 4:25 4 */ u32 capacity_reads:1; /* 4:24 4 */ }; /* 0 8 */ u64 value; /* 0 8 */ }; [acme@quaco pahole]$ pahole -F btf examples/andrii/hsw_tsx_tuning-clang union hsw_tsx_tuning { struct { u32 cycles_last_block; /* 0 4 */ u32 hle_abort:1; /* 4:31 4 */ u32 rtm_abort:1; /* 4:30 4 */ u32 instruction_abort:1; /* 4:29 4 */ u32 non_instruction_abort:1; /* 4:28 4 */ u32 retry:1; /* 4:27 4 */ u32 data_conflict:1; /* 4:26 4 */ u32 capacity_writes:1; /* 4:25 4 */ u32 capacity_reads:1; /* 4:24 4 */ }; /* 0 8 */ u64 value; /* 0 8 */ }; [acme@quaco pahole]$ diff --git a/dwarf_loader.c b/dwarf_loader.c index adef437109f6..37477fb23be3 100644 --- a/dwarf_loader.c +++ b/dwarf_loader.c @@ -607,7 +607,8 @@ static struct variable *variable__new(Dwarf_Die *die, struct cu *cu) int tag__recode_dwarf_bitfield(struct tag *tag, struct cu *cu, uint16_t bit_size) { - uint16_t id; + int id; + uint16_t short_id; struct tag *recoded; /* in all the cases the name is at the same offset */ strings_t name = tag__namespace(tag)->name; @@ -620,7 +621,7 @@ int tag__recode_dwarf_bitfield(struct tag *tag, struct cu *cu, uint16_t bit_size struct tag *type = dtype->tag; id = tag__recode_dwarf_bitfield(type, cu, bit_size); - if (id == tag->type) + if (id < 0) return id; struct type *new_typedef = obstack_zalloc(&cu->obstack, @@ -660,10 +661,9 @@ int tag__recode_dwarf_bitfield(struct tag *tag, struct cu *cu, uint16_t bit_size * the dwarf_cu as in dwarf there are no such things * as base_types of less than 8 bits, etc. */ - recoded = cu__find_base_type_by_sname_and_size(cu, name, bit_size, &id); + recoded = cu__find_base_type_by_sname_and_size(cu, name, bit_size, &short_id); if (recoded != NULL) - return id; - + return short_id; struct base_type *new_bt = obstack_zalloc(&cu->obstack, sizeof(*new_bt)); @@ -683,10 +683,9 @@ int tag__recode_dwarf_bitfield(struct tag *tag, struct cu *cu, uint16_t bit_size * the dwarf_cu as in dwarf there are no such things * as enumeration_types of less than 8 bits, etc. */ - recoded = cu__find_enumeration_by_sname_and_size(cu, name, - bit_size, &id); + recoded = cu__find_enumeration_by_sname_and_size(cu, name, bit_size, &short_id); if (recoded != NULL) - return id; + return short_id; struct type *alias = tag__type(tag); struct type *new_enum = obstack_zalloc(&cu->obstack, sizeof(*new_enum)); ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader 2019-02-25 18:59 ` Arnaldo Carvalho de Melo @ 2019-02-25 19:42 ` Andrii Nakryiko 2019-02-25 20:08 ` encoding BTF on glibc was: " Arnaldo Carvalho de Melo 0 siblings, 1 reply; 23+ messages in thread From: Andrii Nakryiko @ 2019-02-25 19:42 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Andrii Nakryiko, dwarves, bpf, Alexei Starovoitov, Yonghong Song On Mon, Feb 25, 2019 at 11:00 AM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > > Em Mon, Feb 25, 2019 at 01:02:39PM -0300, Arnaldo Carvalho de Melo escreveu: > > So yeah, the BTF encoder/decoder is working just fine, the problem is in > > pahole's DWARF code, lemme see... > > Please try the patch below, for me btfdiff continues to show no diff for > all types in my vmlinux and now it also produces the same output for > when the first element of a bitfield has its bit_size equal to its > byte_size * 8: Yes, this fixes all the issues I've seen. btfdiff output is now empty for my kernel image. Thanks for quick fix! Reviewed-by: Andrii Nakryiko <andriin@fb.com> > > [acme@quaco pahole]$ btfdiff vmlinux > [acme@quaco pahole]$ btfdiff examples/andrii/hsw_tsx_tuning-gcc > [acme@quaco pahole]$ btfdiff examples/andrii/hsw_tsx_tuning-clang > [acme@quaco pahole]$ pahole examples/andrii/hsw_tsx_tuning-clang > union hsw_tsx_tuning { > struct { > u32 cycles_last_block; /* 0 4 */ > u32 hle_abort:1; /* 4:31 4 */ > u32 rtm_abort:1; /* 4:30 4 */ > u32 instruction_abort:1; /* 4:29 4 */ > u32 non_instruction_abort:1; /* 4:28 4 */ > u32 retry:1; /* 4:27 4 */ > u32 data_conflict:1; /* 4:26 4 */ > u32 capacity_writes:1; /* 4:25 4 */ > u32 capacity_reads:1; /* 4:24 4 */ > }; /* 0 8 */ > u64 value; /* 0 8 */ > }; > [acme@quaco pahole]$ pahole -F btf examples/andrii/hsw_tsx_tuning-clang > union hsw_tsx_tuning { > struct { > u32 cycles_last_block; /* 0 4 */ > u32 hle_abort:1; /* 4:31 4 */ > u32 rtm_abort:1; /* 4:30 4 */ > u32 instruction_abort:1; /* 4:29 4 */ > u32 non_instruction_abort:1; /* 4:28 4 */ > u32 retry:1; /* 4:27 4 */ > u32 data_conflict:1; /* 4:26 4 */ > u32 capacity_writes:1; /* 4:25 4 */ > u32 capacity_reads:1; /* 4:24 4 */ > }; /* 0 8 */ > u64 value; /* 0 8 */ > }; > [acme@quaco pahole]$ > > diff --git a/dwarf_loader.c b/dwarf_loader.c > index adef437109f6..37477fb23be3 100644 > --- a/dwarf_loader.c > +++ b/dwarf_loader.c > @@ -607,7 +607,8 @@ static struct variable *variable__new(Dwarf_Die *die, struct cu *cu) > > int tag__recode_dwarf_bitfield(struct tag *tag, struct cu *cu, uint16_t bit_size) > { > - uint16_t id; > + int id; > + uint16_t short_id; > struct tag *recoded; > /* in all the cases the name is at the same offset */ > strings_t name = tag__namespace(tag)->name; > @@ -620,7 +621,7 @@ int tag__recode_dwarf_bitfield(struct tag *tag, struct cu *cu, uint16_t bit_size > struct tag *type = dtype->tag; > > id = tag__recode_dwarf_bitfield(type, cu, bit_size); > - if (id == tag->type) > + if (id < 0) > return id; > > struct type *new_typedef = obstack_zalloc(&cu->obstack, > @@ -660,10 +661,9 @@ int tag__recode_dwarf_bitfield(struct tag *tag, struct cu *cu, uint16_t bit_size > * the dwarf_cu as in dwarf there are no such things > * as base_types of less than 8 bits, etc. > */ > - recoded = cu__find_base_type_by_sname_and_size(cu, name, bit_size, &id); > + recoded = cu__find_base_type_by_sname_and_size(cu, name, bit_size, &short_id); > if (recoded != NULL) > - return id; > - > + return short_id; > > struct base_type *new_bt = obstack_zalloc(&cu->obstack, > sizeof(*new_bt)); > @@ -683,10 +683,9 @@ int tag__recode_dwarf_bitfield(struct tag *tag, struct cu *cu, uint16_t bit_size > * the dwarf_cu as in dwarf there are no such things > * as enumeration_types of less than 8 bits, etc. > */ > - recoded = cu__find_enumeration_by_sname_and_size(cu, name, > - bit_size, &id); > + recoded = cu__find_enumeration_by_sname_and_size(cu, name, bit_size, &short_id); > if (recoded != NULL) > - return id; > + return short_id; > > struct type *alias = tag__type(tag); > struct type *new_enum = obstack_zalloc(&cu->obstack, sizeof(*new_enum)); ^ permalink raw reply [flat|nested] 23+ messages in thread
* encoding BTF on glibc was: Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader 2019-02-25 19:42 ` Andrii Nakryiko @ 2019-02-25 20:08 ` Arnaldo Carvalho de Melo 2019-02-25 20:51 ` Andrii Nakryiko 0 siblings, 1 reply; 23+ messages in thread From: Arnaldo Carvalho de Melo @ 2019-02-25 20:08 UTC (permalink / raw) To: Andrii Nakryiko Cc: Arnaldo Carvalho de Melo, Andrii Nakryiko, dwarves, bpf, Alexei Starovoitov, Yonghong Song Em Mon, Feb 25, 2019 at 11:42:29AM -0800, Andrii Nakryiko escreveu: > On Mon, Feb 25, 2019 at 11:00 AM Arnaldo Carvalho de Melo> <arnaldo.melo@gmail.com> wrote: > > Em Mon, Feb 25, 2019 at 01:02:39PM -0300, Arnaldo Carvalho de Melo escreveu: > > > So yeah, the BTF encoder/decoder is working just fine, the problem is in > > > pahole's DWARF code, lemme see... > > > > Please try the patch below, for me btfdiff continues to show no diff for > > all types in my vmlinux and now it also produces the same output for > > when the first element of a bitfield has its bit_size equal to its > > byte_size * 8: > > Yes, this fixes all the issues I've seen. btfdiff output is now empty > for my kernel image. Thanks for quick fix! > > Reviewed-by: Andrii Nakryiko <andriin@fb.com> Thanks, just for some extra testing I tried encoding BTF for glibc 2.28: [acme@quaco pahole]$ pahole -JV libc-2.28.so.debug <BIG snip> [386033] STRUCT dl_scope_free_list kind_flag=0 size=408 vlen=2 count type_id=385556 bits_offset=0 list type_id=386034 bits_offset=64 [386034] ARRAY (anon) type_id=385511 index_type_id=13 nr_elems=50 [386035] STRUCT rtld_global kind_flag=0 size=3992 vlen=31 _dl_ns type_id=386036 bits_offset=0 _dl_nns type_id=385556 bits_offset=18432 _dl_load_lock type_id=385917 bits_offset=18496 _dl_load_write_lock type_id=385917 bits_offset=18816 _dl_load_adds type_id=385567 bits_offset=19136 _dl_initfirst type_id=385922 bits_offset=19200 _dl_cpuclock_offset type_id=385662 bits_offset=19264 _dl_profile_map type_id=385922 bits_offset=19328 _dl_num_relocations type_id=385496 bits_offset=19392 _dl_num_cache_relocations type_id=385496 bits_offset=19456 _dl_all_dirs type_id=385926 bits_offset=19520 _dl_rtld_map type_id=385919 bits_offset=19584 audit_data type_id=386037 bits_offset=28736 _dl_rtld_lock_recursive type_id=385680 bits_offset=30784 _dl_rtld_unlock_recursive type_id=385680 bits_offset=30848 _dl_x86_feature_1 type_id=385571 bits_offset=30912 _dl_x86_legacy_bitmap type_id=385653 bits_offset=30976 _dl_make_stack_executable_hook type_id=386039 bits_offset=31104 _dl_stack_flags type_id=385525 bits_offset=31168 _dl_tls_dtv_gaps type_id=385825 bits_offset=31200 _dl_tls_max_dtv_idx type_id=385556 bits_offset=31232 _dl_tls_dtv_slotinfo_list type_id=386031 bits_offset=31296 _dl_tls_static_nelem type_id=385556 bits_offset=31360 _dl_tls_static_size type_id=385556 bits_offset=31424 _dl_tls_static_used type_id=385556 bits_offset=31488 _dl_tls_static_align type_id=385556 bits_offset=31552 _dl_initial_dtv type_id=385511 bits_offset=31616 _dl_tls_generation type_id=385556 bits_offset=31680 _dl_init_static_tls type_id=386041 bits_offset=31744 _dl_wait_lookup_done type_id=385665 bits_offset=31808 _dl_scope_free_list type_id=386042 bits_offset=31872 [386036] ARRAY (anon) type_id=386028 index_type_id=13 nr_elems=16 [386037] ARRAY (anon) type_id=385938 index_type_id=13 nr_elems=16 [386038] FUNC_PROTO (anon) return=385501 args=(385970 (anon)) [386039] PTR (anon) type_id=386038 [386040] FUNC_PROTO (anon) return=0 args=(385922 (anon)) [386041] PTR (anon) type_id=386040 [386042] PTR (anon) type_id=386033 [386043] STRUCT rtld_global_ro kind_flag=0 size=432 vlen=40 _dl_debug_mask type_id=385501 bits_offset=0 _dl_osversion type_id=385495 bits_offset=32 _dl_platform type_id=385595 bits_offset=64 _dl_platformlen type_id=385556 bits_offset=128 _dl_pagesize type_id=385556 bits_offset=192 _dl_inhibit_cache type_id=385501 bits_offset=256 _dl_initial_searchlist type_id=385918 bits_offset=320 _dl_clktck type_id=385501 bits_offset=448 _dl_verbose type_id=385501 bits_offset=480 _dl_debug_fd type_id=385501 bits_offset=512 _dl_lazy type_id=385501 bits_offset=544 _dl_bind_not type_id=385501 bits_offset=576 _dl_dynamic_weak type_id=385501 bits_offset=608 _dl_fpu_control type_id=385977 bits_offset=640 _dl_correct_cache_id type_id=385501 bits_offset=672 _dl_hwcap type_id=385520 bits_offset=704 _dl_auxv type_id=386045 bits_offset=768 _dl_x86_cpu_features type_id=385599 bits_offset=832 _dl_x86_hwcap_flags type_id=386047 bits_offset=1664 _dl_x86_platforms type_id=386049 bits_offset=1880 _dl_inhibit_rpath type_id=385595 bits_offset=2176 _dl_origin_path type_id=385595 bits_offset=2240 _dl_use_load_bias type_id=385529 bits_offset=2304 _dl_profile type_id=385595 bits_offset=2368 _dl_profile_output type_id=385595 bits_offset=2432 _dl_trace_prelink type_id=385595 bits_offset=2496 _dl_trace_prelink_map type_id=385922 bits_offset=2560 _dl_init_all_dirs type_id=385926 bits_offset=2624 _dl_sysinfo_dso type_id=386050 bits_offset=2688 _dl_sysinfo_map type_id=385922 bits_offset=2752 _dl_hwcap2 type_id=385520 bits_offset=2816 _dl_debug_printf type_id=386052 bits_offset=2880 _dl_mcount type_id=386054 bits_offset=2944 _dl_lookup_symbol_x type_id=386058 bits_offset=3008 _dl_open type_id=386060 bits_offset=3072 _dl_close type_id=385680 bits_offset=3136 _dl_tls_get_addr_soft type_id=386062 bits_offset=3200 _dl_discover_osversion type_id=386064 bits_offset=3264 _dl_audit type_id=386023 bits_offset=3328 _dl_naudit type_id=385495 bits_offset=3392 [386044] CONST (anon) type_id=386043 [386045] PTR (anon) type_id=385552 [386046] ARRAY (anon) type_id=385515 index_type_id=13 nr_elems=27 [386047] CONST (anon) type_id=386046 [386048] ARRAY (anon) type_id=385515 index_type_id=13 nr_elems=36 [386049] CONST (anon) type_id=386048 [386050] PTR (anon) type_id=385538 [386051] FUNC_PROTO (anon) return=0 args=(385595 (anon), vararg) [386052] PTR (anon) type_id=386051 [386053] FUNC_PROTO (anon) return=0 args=(385529 (anon), 385529 (anon)) [386054] PTR (anon) type_id=386053 [386055] FUNC_PROTO (anon) return=385978 args=(385595 (anon), 385922 (anon), 386056 (anon), 385950 (anon), 386057 (anon), 385501 (anon), 385501 (anon), 385922 (anon)) [386056] PTR (anon) type_id=385937 [386057] PTR (anon) type_id=385943 [386058] PTR (anon) type_id=386055 [386059] FUNC_PROTO (anon) return=385511 args=(385595 (anon), 385501 (anon), 385640 (anon), 385602 (anon), 385501 (anon), 385954 (anon), 385954 (anon)) [386060] PTR (anon) type_id=386059 [386061] FUNC_PROTO (anon) return=385511 args=(385922 (anon)) [386062] PTR (anon) type_id=386061 [386063] FUNC_PROTO (anon) return=385501 args=(void) [386064] PTR (anon) type_id=386063 [386065] ARRAY (anon) type_id=386069 index_type_id=13 nr_elems=28 [386066] CONST (anon) type_id=386065 [386067] INT long unsigned int size=8 bit_offset=0 nr_bits=64 encoding=(none) [386068] INT char size=1 bit_offset=0 nr_bits=8 encoding=(none) [386069] CONST (anon) type_id=386068 [386070] TYPEDEF ui32 type_id=386072 [386071] CONST (anon) type_id=386070 [386072] INT unsigned int size=4 bit_offset=0 nr_bits=32 encoding=(none) [386073] ARRAY (anon) type_id=386071 index_type_id=13 nr_elems=1 [386074] CONST (anon) type_id=386073 [386075] INT long unsigned int size=8 bit_offset=0 nr_bits=64 encoding=(none) Cannot open libc-2.28.so.debug Failed to encode BTF [acme@quaco pahole]$ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: encoding BTF on glibc was: Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader 2019-02-25 20:08 ` encoding BTF on glibc was: " Arnaldo Carvalho de Melo @ 2019-02-25 20:51 ` Andrii Nakryiko 2019-02-25 21:36 ` Andrii Nakryiko 0 siblings, 1 reply; 23+ messages in thread From: Andrii Nakryiko @ 2019-02-25 20:51 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Andrii Nakryiko, dwarves, bpf, Alexei Starovoitov, Yonghong Song On Mon, Feb 25, 2019 at 12:08 PM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > > Em Mon, Feb 25, 2019 at 11:42:29AM -0800, Andrii Nakryiko escreveu: > > On Mon, Feb 25, 2019 at 11:00 AM Arnaldo Carvalho de Melo> <arnaldo.melo@gmail.com> wrote: > > > > Em Mon, Feb 25, 2019 at 01:02:39PM -0300, Arnaldo Carvalho de Melo escreveu: > > > > So yeah, the BTF encoder/decoder is working just fine, the problem is in > > > > pahole's DWARF code, lemme see... > > > > > > Please try the patch below, for me btfdiff continues to show no diff for > > > all types in my vmlinux and now it also produces the same output for > > > when the first element of a bitfield has its bit_size equal to its > > > byte_size * 8: > > > > Yes, this fixes all the issues I've seen. btfdiff output is now empty > > for my kernel image. Thanks for quick fix! > > > > Reviewed-by: Andrii Nakryiko <andriin@fb.com> > > Thanks, just for some extra testing I tried encoding BTF for glibc 2.28: > > [acme@quaco pahole]$ pahole -JV libc-2.28.so.debug > <BIG snip> <snip> > [386075] INT long unsigned int size=8 bit_offset=0 nr_bits=64 encoding=(none) > Cannot open libc-2.28.so.debug > Failed to encode BTF I've tried the same on libc-2.26.so.debug and it worked. It failes after BTF encoding and deduping is done, exactly when it tries to re-open libc-2.28.so.debug to write out new contents. Not sure why it would fail, as it seems to be just plain open() call, but the fact that we both can repro this means there is something there. It also seems that libc-2.28.so.debug is corrupted: $ readelf -n /home/andriin/local/btf/libc-2.26.so.debug | grep 'Unknown note type' | wc -l 0 $ readelf -n /home/andriin/local/btf/libc-2.28.so.debug | grep 'Unknown note type' | wc -l 17953 > [acme@quaco pahole]$ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: encoding BTF on glibc was: Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader 2019-02-25 20:51 ` Andrii Nakryiko @ 2019-02-25 21:36 ` Andrii Nakryiko 2019-02-26 0:34 ` Andrii Nakryiko ` (2 more replies) 0 siblings, 3 replies; 23+ messages in thread From: Andrii Nakryiko @ 2019-02-25 21:36 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Andrii Nakryiko, dwarves, bpf, Alexei Starovoitov, Yonghong Song On Mon, Feb 25, 2019 at 12:51 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Mon, Feb 25, 2019 at 12:08 PM Arnaldo Carvalho de Melo > <arnaldo.melo@gmail.com> wrote: > > > > Em Mon, Feb 25, 2019 at 11:42:29AM -0800, Andrii Nakryiko escreveu: > > > On Mon, Feb 25, 2019 at 11:00 AM Arnaldo Carvalho de Melo> <arnaldo.melo@gmail.com> wrote: > > > > > > Em Mon, Feb 25, 2019 at 01:02:39PM -0300, Arnaldo Carvalho de Melo escreveu: > > > > > So yeah, the BTF encoder/decoder is working just fine, the problem is in > > > > > pahole's DWARF code, lemme see... > > > > > > > > Please try the patch below, for me btfdiff continues to show no diff for > > > > all types in my vmlinux and now it also produces the same output for > > > > when the first element of a bitfield has its bit_size equal to its > > > > byte_size * 8: > > > > > > Yes, this fixes all the issues I've seen. btfdiff output is now empty > > > for my kernel image. Thanks for quick fix! > > > > > > Reviewed-by: Andrii Nakryiko <andriin@fb.com> > > > > Thanks, just for some extra testing I tried encoding BTF for glibc 2.28: > > > > [acme@quaco pahole]$ pahole -JV libc-2.28.so.debug > > <BIG snip> > > <snip> > > > [386075] INT long unsigned int size=8 bit_offset=0 nr_bits=64 encoding=(none) > > Cannot open libc-2.28.so.debug > > Failed to encode BTF > > I've tried the same on libc-2.26.so.debug and it worked. > > It failes after BTF encoding and deduping is done, exactly when it > tries to re-open libc-2.28.so.debug to write out new contents. Not > sure why it would fail, as it seems to be just plain open() call, but > the fact that we both can repro this means there is something there. > > It also seems that libc-2.28.so.debug is corrupted: > > $ readelf -n /home/andriin/local/btf/libc-2.26.so.debug | grep > 'Unknown note type' | wc -l > 0 > $ readelf -n /home/andriin/local/btf/libc-2.28.so.debug | grep > 'Unknown note type' | wc -l > 17953 > Ignore everything I've said (though 'Unknown note type' issue is strange), for me it was just permissions issues. $ chmod +rw /home/andriin/local/btf/libc-2.28.so.debug $ ~/local/pahole/build/pahole -J ~/local/btf/libc-2.28.so.debug $ PAHOLE=~/local/pahole/build/pahole ./btfdiff ~/local/btf/libc-2.28.so.debug base_type__name_to_size: base_type _Float128 class__fixup_btf_bitfields: unknown base type name "_Float128"! base_type__name_to_size: base_type _Float128 class__fixup_btf_bitfields: unknown base type name "_Float128"! base_type__name_to_size: base_type _Float128 class__fixup_btf_bitfields: unknown base type name "_Float128"! base_type__name_to_size: base_type _Float128 class__fixup_btf_bitfields: unknown base type name "_Float128"! base_type__name_to_size: base_type _Float128 class__fixup_btf_bitfields: unknown base type name "_Float128"! base_type__name_to_size: base_type _Float128 class__fixup_btf_bitfields: unknown base type name "_Float128"! --- /tmp/btfdiff.dwarf.aV4wSL 2019-02-25 13:31:54.787923673 -0800 +++ /tmp/btfdiff.btf.22NQmJ 2019-02-25 13:31:54.802923668 -0800 @@ -461,9 +461,15 @@ struct La_x86_64_retval { uint64_t lrv_rdx; /* 8 8 */ La_x86_64_xmm lrv_xmm0; /* 16 16 */ La_x86_64_xmm lrv_xmm1; /* 32 16 */ - long double lrv_st0; /* 48 16 */ + long double lrv_st0; /* 48 8 */ + + /* XXX 8 bytes hole, try to pack */ + /* --- cacheline 1 boundary (64 bytes) --- */ - long double lrv_st1; /* 64 16 */ + long double lrv_st1; /* 64 8 */ + + /* XXX 8 bytes hole, try to pack */ + La_x86_64_vector lrv_vector0; /* 80 64 */ /* --- cacheline 2 boundary (128 bytes) was 16 bytes ago --- */ La_x86_64_vector lrv_vector1; /* 144 64 */ @@ -472,6 +478,7 @@ struct La_x86_64_retval { __int128 lrv_bnd1; /* 224 16 */ /* size: 240, cachelines: 4, members: 10 */ + /* sum members: 224, holes: 2, sum holes: 16 */ /* last cacheline: 48 bytes */ }; struct r_debug { @@ -2044,7 +2051,7 @@ union ieee754_float { } ieee_nan; /* 0 4 */ }; union ieee854_long_double { - long double d; /* 0 16 */ + long double d; /* 0 8 */ struct { unsigned int mantissa1:32; /* 0: 0 4 */ unsigned int mantissa0:32; /* 4: 0 4 */ @@ -2141,7 +2148,7 @@ struct ucontext_t { /* last cacheline: 8 bytes */ }; union ieee854_float128 { - _Float128 d; /* 0 16 */ + _Float128 d; /* 0 0 */ struct { unsigned int mantissa3:32; /* 0: 0 4 */ unsigned int mantissa2:32; /* 4: 0 4 */ @@ -2219,7 +2226,7 @@ union printf_arg { long unsigned int pa_u_long_int; /* 0 8 */ long long unsigned int pa_u_long_long_int; /* 0 8 */ double pa_double; /* 0 8 */ - long double pa_long_double; /* 0 16 */ + long double pa_long_double; /* 0 8 */ const char * pa_string; /* 0 8 */ const wchar_t * pa_wstring; /* 0 8 */ void * pa_pointer; /* 0 8 */ Seems like "long double" size is invalid for BTF. And we should probably add "_Float128" as another "well known" base type name? Though for the latter, it probably would be better to resolve base type size using BTF data itself. > > [acme@quaco pahole]$ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: encoding BTF on glibc was: Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader 2019-02-25 21:36 ` Andrii Nakryiko @ 2019-02-26 0:34 ` Andrii Nakryiko 2019-02-26 5:37 ` Yonghong Song 2019-02-26 13:11 ` Arnaldo Carvalho de Melo 2 siblings, 0 replies; 23+ messages in thread From: Andrii Nakryiko @ 2019-02-26 0:34 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Andrii Nakryiko, dwarves, bpf, Alexei Starovoitov, Yonghong Song One more datapoint. I've compiled latest redis sources and tested pahole/BTF on it: $ ~/local/pahole/build/pahole -J ~/local/btf/redis-server.debug $ PAHOLE=~/local/pahole/build/pahole ./btfdiff ~/local/btf/redis-server.debug $ readelf -S ~/local/btf/redis-server.debug | rg -A1 'BTF|debug_str|debug_info' readelf: /home/andriin/local/btf/redis-server.debug: Warning: the .dynamic section is not contained within the dynamic segment [30] .debug_info PROGBITS 0000000000000000 0019738d 00000000002529cf 0000000000000000 0 0 1 -- [33] .debug_str PROGBITS 0000000000000000 004574a0 000000000004fba1 0000000000000001 MS 0 0 1 -- [40] .BTF PROGBITS 0000000000000000 007dcc92 000000000001553c 0000000000000000 0 0 1 $ printf "dwarf data: %d, dwarf strs: %d, btf: %d (ratio: %.3lf)\n" 0x2529cf 0x4fba1 0x1553c '(0x2529cf+0x4fba1+1.0)/0x1553c' dwarf data: 2435535, dwarf strs: 326561, btf: 87356 (ratio: 31.619) Even for relatively small DWARF in redis (2.6MB, including strings), BTF deduplication gives 31.5x compression. -- Andrii On Mon, Feb 25, 2019 at 1:36 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Mon, Feb 25, 2019 at 12:51 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Mon, Feb 25, 2019 at 12:08 PM Arnaldo Carvalho de Melo > > <arnaldo.melo@gmail.com> wrote: > > > > > > Em Mon, Feb 25, 2019 at 11:42:29AM -0800, Andrii Nakryiko escreveu: > > > > On Mon, Feb 25, 2019 at 11:00 AM Arnaldo Carvalho de Melo> <arnaldo.melo@gmail.com> wrote: > > > > > > > > Em Mon, Feb 25, 2019 at 01:02:39PM -0300, Arnaldo Carvalho de Melo escreveu: > > > > > > So yeah, the BTF encoder/decoder is working just fine, the problem is in > > > > > > pahole's DWARF code, lemme see... > > > > > > > > > > Please try the patch below, for me btfdiff continues to show no diff for > > > > > all types in my vmlinux and now it also produces the same output for > > > > > when the first element of a bitfield has its bit_size equal to its > > > > > byte_size * 8: > > > > > > > > Yes, this fixes all the issues I've seen. btfdiff output is now empty > > > > for my kernel image. Thanks for quick fix! > > > > > > > > Reviewed-by: Andrii Nakryiko <andriin@fb.com> > > > > > > Thanks, just for some extra testing I tried encoding BTF for glibc 2.28: > > > > > > [acme@quaco pahole]$ pahole -JV libc-2.28.so.debug > > > <BIG snip> > > > > <snip> > > > > > [386075] INT long unsigned int size=8 bit_offset=0 nr_bits=64 encoding=(none) > > > Cannot open libc-2.28.so.debug > > > Failed to encode BTF > > > > I've tried the same on libc-2.26.so.debug and it worked. > > > > It failes after BTF encoding and deduping is done, exactly when it > > tries to re-open libc-2.28.so.debug to write out new contents. Not > > sure why it would fail, as it seems to be just plain open() call, but > > the fact that we both can repro this means there is something there. > > > > It also seems that libc-2.28.so.debug is corrupted: > > > > $ readelf -n /home/andriin/local/btf/libc-2.26.so.debug | grep > > 'Unknown note type' | wc -l > > 0 > > $ readelf -n /home/andriin/local/btf/libc-2.28.so.debug | grep > > 'Unknown note type' | wc -l > > 17953 > > > > Ignore everything I've said (though 'Unknown note type' issue is > strange), for me it was just permissions issues. > > $ chmod +rw /home/andriin/local/btf/libc-2.28.so.debug > > $ ~/local/pahole/build/pahole -J ~/local/btf/libc-2.28.so.debug > > $ PAHOLE=~/local/pahole/build/pahole ./btfdiff ~/local/btf/libc-2.28.so.debug > base_type__name_to_size: base_type _Float128 > class__fixup_btf_bitfields: unknown base type name "_Float128"! > base_type__name_to_size: base_type _Float128 > class__fixup_btf_bitfields: unknown base type name "_Float128"! > base_type__name_to_size: base_type _Float128 > class__fixup_btf_bitfields: unknown base type name "_Float128"! > base_type__name_to_size: base_type _Float128 > class__fixup_btf_bitfields: unknown base type name "_Float128"! > base_type__name_to_size: base_type _Float128 > class__fixup_btf_bitfields: unknown base type name "_Float128"! > base_type__name_to_size: base_type _Float128 > class__fixup_btf_bitfields: unknown base type name "_Float128"! > --- /tmp/btfdiff.dwarf.aV4wSL 2019-02-25 13:31:54.787923673 -0800 > +++ /tmp/btfdiff.btf.22NQmJ 2019-02-25 13:31:54.802923668 -0800 > @@ -461,9 +461,15 @@ struct La_x86_64_retval { > uint64_t lrv_rdx; /* 8 8 */ > La_x86_64_xmm lrv_xmm0; /* 16 16 */ > La_x86_64_xmm lrv_xmm1; /* 32 16 */ > - long double lrv_st0; /* 48 16 */ > + long double lrv_st0; /* 48 8 */ > + > + /* XXX 8 bytes hole, try to pack */ > + > /* --- cacheline 1 boundary (64 bytes) --- */ > - long double lrv_st1; /* 64 16 */ > + long double lrv_st1; /* 64 8 */ > + > + /* XXX 8 bytes hole, try to pack */ > + > La_x86_64_vector lrv_vector0; /* 80 64 */ > /* --- cacheline 2 boundary (128 bytes) was 16 bytes ago --- */ > La_x86_64_vector lrv_vector1; /* 144 64 */ > @@ -472,6 +478,7 @@ struct La_x86_64_retval { > __int128 lrv_bnd1; /* 224 16 */ > > /* size: 240, cachelines: 4, members: 10 */ > + /* sum members: 224, holes: 2, sum holes: 16 */ > /* last cacheline: 48 bytes */ > }; > struct r_debug { > @@ -2044,7 +2051,7 @@ union ieee754_float { > } ieee_nan; /* 0 4 */ > }; > union ieee854_long_double { > - long double d; /* 0 16 */ > + long double d; /* 0 8 */ > struct { > unsigned int mantissa1:32; /* 0: 0 4 */ > unsigned int mantissa0:32; /* 4: 0 4 */ > @@ -2141,7 +2148,7 @@ struct ucontext_t { > /* last cacheline: 8 bytes */ > }; > union ieee854_float128 { > - _Float128 d; /* 0 16 */ > + _Float128 d; /* 0 0 */ > struct { > unsigned int mantissa3:32; /* 0: 0 4 */ > unsigned int mantissa2:32; /* 4: 0 4 */ > @@ -2219,7 +2226,7 @@ union printf_arg { > long unsigned int pa_u_long_int; /* 0 8 */ > long long unsigned int pa_u_long_long_int; /* 0 8 */ > double pa_double; /* 0 8 */ > - long double pa_long_double; /* 0 16 */ > + long double pa_long_double; /* 0 8 */ > const char * pa_string; /* 0 8 */ > const wchar_t * pa_wstring; /* 0 8 */ > void * pa_pointer; /* 0 8 */ > > Seems like "long double" size is invalid for BTF. And we should > probably add "_Float128" as another "well known" base type name? > > Though for the latter, it probably would be better to resolve base > type size using BTF data itself. > > > > [acme@quaco pahole]$ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: encoding BTF on glibc was: Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader 2019-02-25 21:36 ` Andrii Nakryiko 2019-02-26 0:34 ` Andrii Nakryiko @ 2019-02-26 5:37 ` Yonghong Song 2019-02-26 5:44 ` Alexei Starovoitov 2019-02-26 5:45 ` Andrii Nakryiko 2019-02-26 13:11 ` Arnaldo Carvalho de Melo 2 siblings, 2 replies; 23+ messages in thread From: Yonghong Song @ 2019-02-26 5:37 UTC (permalink / raw) To: Andrii Nakryiko, Arnaldo Carvalho de Melo Cc: Andrii Nakryiko, dwarves, bpf, Alexei Starovoitov On 2/25/19 1:36 PM, Andrii Nakryiko wrote: > On Mon, Feb 25, 2019 at 12:51 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: >> >> On Mon, Feb 25, 2019 at 12:08 PM Arnaldo Carvalho de Melo >> <arnaldo.melo@gmail.com> wrote: >>> >>> Em Mon, Feb 25, 2019 at 11:42:29AM -0800, Andrii Nakryiko escreveu: >>>> On Mon, Feb 25, 2019 at 11:00 AM Arnaldo Carvalho de Melo> <arnaldo.melo@gmail.com> wrote: >>> >>>>> Em Mon, Feb 25, 2019 at 01:02:39PM -0300, Arnaldo Carvalho de Melo escreveu: >>>>>> So yeah, the BTF encoder/decoder is working just fine, the problem is in >>>>>> pahole's DWARF code, lemme see... >>>>> >>>>> Please try the patch below, for me btfdiff continues to show no diff for >>>>> all types in my vmlinux and now it also produces the same output for >>>>> when the first element of a bitfield has its bit_size equal to its >>>>> byte_size * 8: >>>> >>>> Yes, this fixes all the issues I've seen. btfdiff output is now empty >>>> for my kernel image. Thanks for quick fix! >>>> >>>> Reviewed-by: Andrii Nakryiko <andriin@fb.com> >>> >>> Thanks, just for some extra testing I tried encoding BTF for glibc 2.28: >>> >>> [acme@quaco pahole]$ pahole -JV libc-2.28.so.debug >>> <BIG snip> >> >> <snip> >> >>> [386075] INT long unsigned int size=8 bit_offset=0 nr_bits=64 encoding=(none) >>> Cannot open libc-2.28.so.debug >>> Failed to encode BTF >> >> I've tried the same on libc-2.26.so.debug and it worked. >> >> It failes after BTF encoding and deduping is done, exactly when it >> tries to re-open libc-2.28.so.debug to write out new contents. Not >> sure why it would fail, as it seems to be just plain open() call, but >> the fact that we both can repro this means there is something there. >> >> It also seems that libc-2.28.so.debug is corrupted: >> >> $ readelf -n /home/andriin/local/btf/libc-2.26.so.debug | grep >> 'Unknown note type' | wc -l >> 0 >> $ readelf -n /home/andriin/local/btf/libc-2.28.so.debug | grep >> 'Unknown note type' | wc -l >> 17953 >> > > Ignore everything I've said (though 'Unknown note type' issue is > strange), for me it was just permissions issues. > > $ chmod +rw /home/andriin/local/btf/libc-2.28.so.debug > > $ ~/local/pahole/build/pahole -J ~/local/btf/libc-2.28.so.debug > > $ PAHOLE=~/local/pahole/build/pahole ./btfdiff ~/local/btf/libc-2.28.so.debug > base_type__name_to_size: base_type _Float128 > class__fixup_btf_bitfields: unknown base type name "_Float128"! > base_type__name_to_size: base_type _Float128 > class__fixup_btf_bitfields: unknown base type name "_Float128"! > base_type__name_to_size: base_type _Float128 > class__fixup_btf_bitfields: unknown base type name "_Float128"! > base_type__name_to_size: base_type _Float128 > class__fixup_btf_bitfields: unknown base type name "_Float128"! > base_type__name_to_size: base_type _Float128 > class__fixup_btf_bitfields: unknown base type name "_Float128"! > base_type__name_to_size: base_type _Float128 > class__fixup_btf_bitfields: unknown base type name "_Float128"! > --- /tmp/btfdiff.dwarf.aV4wSL 2019-02-25 13:31:54.787923673 -0800 > +++ /tmp/btfdiff.btf.22NQmJ 2019-02-25 13:31:54.802923668 -0800 > @@ -461,9 +461,15 @@ struct La_x86_64_retval { > uint64_t lrv_rdx; /* 8 8 */ > La_x86_64_xmm lrv_xmm0; /* 16 16 */ > La_x86_64_xmm lrv_xmm1; /* 32 16 */ > - long double lrv_st0; /* 48 16 */ > + long double lrv_st0; /* 48 8 */ > + > + /* XXX 8 bytes hole, try to pack */ > + > /* --- cacheline 1 boundary (64 bytes) --- */ > - long double lrv_st1; /* 64 16 */ > + long double lrv_st1; /* 64 8 */ > + > + /* XXX 8 bytes hole, try to pack */ > + > La_x86_64_vector lrv_vector0; /* 80 64 */ > /* --- cacheline 2 boundary (128 bytes) was 16 bytes ago --- */ > La_x86_64_vector lrv_vector1; /* 144 64 */ > @@ -472,6 +478,7 @@ struct La_x86_64_retval { > __int128 lrv_bnd1; /* 224 16 */ > > /* size: 240, cachelines: 4, members: 10 */ > + /* sum members: 224, holes: 2, sum holes: 16 */ > /* last cacheline: 48 bytes */ > }; > struct r_debug { > @@ -2044,7 +2051,7 @@ union ieee754_float { > } ieee_nan; /* 0 4 */ > }; > union ieee854_long_double { > - long double d; /* 0 16 */ > + long double d; /* 0 8 */ > struct { > unsigned int mantissa1:32; /* 0: 0 4 */ > unsigned int mantissa0:32; /* 4: 0 4 */ > @@ -2141,7 +2148,7 @@ struct ucontext_t { > /* last cacheline: 8 bytes */ > }; > union ieee854_float128 { > - _Float128 d; /* 0 16 */ > + _Float128 d; /* 0 0 */ > struct { > unsigned int mantissa3:32; /* 0: 0 4 */ > unsigned int mantissa2:32; /* 4: 0 4 */ > @@ -2219,7 +2226,7 @@ union printf_arg { > long unsigned int pa_u_long_int; /* 0 8 */ > long long unsigned int pa_u_long_long_int; /* 0 8 */ > double pa_double; /* 0 8 */ > - long double pa_long_double; /* 0 16 */ > + long double pa_long_double; /* 0 8 */ > const char * pa_string; /* 0 8 */ > const wchar_t * pa_wstring; /* 0 8 */ > void * pa_pointer; /* 0 8 */ > > Seems like "long double" size is invalid for BTF. And we should > probably add "_Float128" as another "well known" base type name? BPF does not support floating point instructions. As the result, BTF design does not contain floating point types. Looks like kernel also uses float types (in certain drivers and arch's), but types referred by bpf program mostly will not contain float types. Currently, both llvm and pahole will ignore floating types, so from type comparison point of view, we will be fine. The issue may come up with people using vmlinux BTF inside the kernel for different purpose where a float type needs to be specified, e.g., for a floating point function argument in ftrace. But this is rare too. I am not 100% sure whether pahole dwarf/btf parity is considered as a strong reason to add floating point type support in BTF or not. > > Though for the latter, it probably would be better to resolve base > type size using BTF data itself. > >>> [acme@quaco pahole]$ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: encoding BTF on glibc was: Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader 2019-02-26 5:37 ` Yonghong Song @ 2019-02-26 5:44 ` Alexei Starovoitov 2019-02-26 5:45 ` Andrii Nakryiko 1 sibling, 0 replies; 23+ messages in thread From: Alexei Starovoitov @ 2019-02-26 5:44 UTC (permalink / raw) To: Yonghong Song, Andrii Nakryiko, Arnaldo Carvalho de Melo Cc: Andrii Nakryiko, dwarves, bpf On 2/25/19 9:37 PM, Yonghong Song wrote: > I am not 100% sure whether pahole dwarf/btf parity is considered > as a strong reason to add floating point type support in BTF or not. I wouldn't add floating point to BTF yet. BTF is not trying to replace dwarf. Let's make it working well for BPF use cases first. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: encoding BTF on glibc was: Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader 2019-02-26 5:37 ` Yonghong Song 2019-02-26 5:44 ` Alexei Starovoitov @ 2019-02-26 5:45 ` Andrii Nakryiko 2019-02-26 6:03 ` Yonghong Song 1 sibling, 1 reply; 23+ messages in thread From: Andrii Nakryiko @ 2019-02-26 5:45 UTC (permalink / raw) To: Yonghong Song Cc: Arnaldo Carvalho de Melo, Andrii Nakryiko, dwarves, bpf, Alexei Starovoitov On Mon, Feb 25, 2019 at 9:38 PM Yonghong Song <yhs@fb.com> wrote: > > > > On 2/25/19 1:36 PM, Andrii Nakryiko wrote: > > On Mon, Feb 25, 2019 at 12:51 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > >> > >> On Mon, Feb 25, 2019 at 12:08 PM Arnaldo Carvalho de Melo > >> <arnaldo.melo@gmail.com> wrote: > >>> > >>> Em Mon, Feb 25, 2019 at 11:42:29AM -0800, Andrii Nakryiko escreveu: > >>>> On Mon, Feb 25, 2019 at 11:00 AM Arnaldo Carvalho de Melo> <arnaldo.melo@gmail.com> wrote: > >>> > >>>>> Em Mon, Feb 25, 2019 at 01:02:39PM -0300, Arnaldo Carvalho de Melo escreveu: > >>>>>> So yeah, the BTF encoder/decoder is working just fine, the problem is in > >>>>>> pahole's DWARF code, lemme see... > >>>>> > >>>>> Please try the patch below, for me btfdiff continues to show no diff for > >>>>> all types in my vmlinux and now it also produces the same output for > >>>>> when the first element of a bitfield has its bit_size equal to its > >>>>> byte_size * 8: > >>>> > >>>> Yes, this fixes all the issues I've seen. btfdiff output is now empty > >>>> for my kernel image. Thanks for quick fix! > >>>> > >>>> Reviewed-by: Andrii Nakryiko <andriin@fb.com> > >>> > >>> Thanks, just for some extra testing I tried encoding BTF for glibc 2.28: > >>> > >>> [acme@quaco pahole]$ pahole -JV libc-2.28.so.debug > >>> <BIG snip> > >> > >> <snip> > >> > >>> [386075] INT long unsigned int size=8 bit_offset=0 nr_bits=64 encoding=(none) > >>> Cannot open libc-2.28.so.debug > >>> Failed to encode BTF > >> > >> I've tried the same on libc-2.26.so.debug and it worked. > >> > >> It failes after BTF encoding and deduping is done, exactly when it > >> tries to re-open libc-2.28.so.debug to write out new contents. Not > >> sure why it would fail, as it seems to be just plain open() call, but > >> the fact that we both can repro this means there is something there. > >> > >> It also seems that libc-2.28.so.debug is corrupted: > >> > >> $ readelf -n /home/andriin/local/btf/libc-2.26.so.debug | grep > >> 'Unknown note type' | wc -l > >> 0 > >> $ readelf -n /home/andriin/local/btf/libc-2.28.so.debug | grep > >> 'Unknown note type' | wc -l > >> 17953 > >> > > > > Ignore everything I've said (though 'Unknown note type' issue is > > strange), for me it was just permissions issues. > > > > $ chmod +rw /home/andriin/local/btf/libc-2.28.so.debug > > > > $ ~/local/pahole/build/pahole -J ~/local/btf/libc-2.28.so.debug > > > > $ PAHOLE=~/local/pahole/build/pahole ./btfdiff ~/local/btf/libc-2.28.so.debug > > base_type__name_to_size: base_type _Float128 > > class__fixup_btf_bitfields: unknown base type name "_Float128"! > > base_type__name_to_size: base_type _Float128 > > class__fixup_btf_bitfields: unknown base type name "_Float128"! > > base_type__name_to_size: base_type _Float128 > > class__fixup_btf_bitfields: unknown base type name "_Float128"! > > base_type__name_to_size: base_type _Float128 > > class__fixup_btf_bitfields: unknown base type name "_Float128"! > > base_type__name_to_size: base_type _Float128 > > class__fixup_btf_bitfields: unknown base type name "_Float128"! > > base_type__name_to_size: base_type _Float128 > > class__fixup_btf_bitfields: unknown base type name "_Float128"! > > --- /tmp/btfdiff.dwarf.aV4wSL 2019-02-25 13:31:54.787923673 -0800 > > +++ /tmp/btfdiff.btf.22NQmJ 2019-02-25 13:31:54.802923668 -0800 > > @@ -461,9 +461,15 @@ struct La_x86_64_retval { > > uint64_t lrv_rdx; /* 8 8 */ > > La_x86_64_xmm lrv_xmm0; /* 16 16 */ > > La_x86_64_xmm lrv_xmm1; /* 32 16 */ > > - long double lrv_st0; /* 48 16 */ > > + long double lrv_st0; /* 48 8 */ > > + > > + /* XXX 8 bytes hole, try to pack */ > > + > > /* --- cacheline 1 boundary (64 bytes) --- */ > > - long double lrv_st1; /* 64 16 */ > > + long double lrv_st1; /* 64 8 */ > > + > > + /* XXX 8 bytes hole, try to pack */ > > + > > La_x86_64_vector lrv_vector0; /* 80 64 */ > > /* --- cacheline 2 boundary (128 bytes) was 16 bytes ago --- */ > > La_x86_64_vector lrv_vector1; /* 144 64 */ > > @@ -472,6 +478,7 @@ struct La_x86_64_retval { > > __int128 lrv_bnd1; /* 224 16 */ > > > > /* size: 240, cachelines: 4, members: 10 */ > > + /* sum members: 224, holes: 2, sum holes: 16 */ > > /* last cacheline: 48 bytes */ > > }; > > struct r_debug { > > @@ -2044,7 +2051,7 @@ union ieee754_float { > > } ieee_nan; /* 0 4 */ > > }; > > union ieee854_long_double { > > - long double d; /* 0 16 */ > > + long double d; /* 0 8 */ > > struct { > > unsigned int mantissa1:32; /* 0: 0 4 */ > > unsigned int mantissa0:32; /* 4: 0 4 */ > > @@ -2141,7 +2148,7 @@ struct ucontext_t { > > /* last cacheline: 8 bytes */ > > }; > > union ieee854_float128 { > > - _Float128 d; /* 0 16 */ > > + _Float128 d; /* 0 0 */ > > struct { > > unsigned int mantissa3:32; /* 0: 0 4 */ > > unsigned int mantissa2:32; /* 4: 0 4 */ > > @@ -2219,7 +2226,7 @@ union printf_arg { > > long unsigned int pa_u_long_int; /* 0 8 */ > > long long unsigned int pa_u_long_long_int; /* 0 8 */ > > double pa_double; /* 0 8 */ > > - long double pa_long_double; /* 0 16 */ > > + long double pa_long_double; /* 0 8 */ > > const char * pa_string; /* 0 8 */ > > const wchar_t * pa_wstring; /* 0 8 */ > > void * pa_pointer; /* 0 8 */ > > > > Seems like "long double" size is invalid for BTF. And we should > > probably add "_Float128" as another "well known" base type name? > > BPF does not support floating point instructions. As the result, > BTF design does not contain floating point types. > > Looks like kernel also uses float types (in certain drivers and arch's), > but types referred by bpf program mostly will not contain float types. > > Currently, both llvm and pahole will ignore floating types, so > from type comparison point of view, we will be fine. > The issue may come up with people using vmlinux BTF inside the > kernel for different purpose where a float type needs to be > specified, e.g., for a floating point function argument in ftrace. > But this is rare too. > > I am not 100% sure whether pahole dwarf/btf parity is considered > as a strong reason to add floating point type support in BTF or not. Isn't float and double just another base type with corresponding name? Fundamentally, how is it different from int or long? It's still just a collection of bits in 32-bit, 64-bit, etc number, except interpreted differently. Do you mean that for BTF to properly support floats, we'll need to extends set of possible encodings in INT type (in addition to today's CHAR, SIGNED and UNSIGNED)? > > > > > Though for the latter, it probably would be better to resolve base > > type size using BTF data itself. > > > >>> [acme@quaco pahole]$ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: encoding BTF on glibc was: Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader 2019-02-26 5:45 ` Andrii Nakryiko @ 2019-02-26 6:03 ` Yonghong Song 2019-02-26 17:22 ` Andrii Nakryiko 0 siblings, 1 reply; 23+ messages in thread From: Yonghong Song @ 2019-02-26 6:03 UTC (permalink / raw) To: Andrii Nakryiko Cc: Arnaldo Carvalho de Melo, Andrii Nakryiko, dwarves, bpf, Alexei Starovoitov On 2/25/19 9:45 PM, Andrii Nakryiko wrote: > On Mon, Feb 25, 2019 at 9:38 PM Yonghong Song <yhs@fb.com> wrote: >> >> >> >> On 2/25/19 1:36 PM, Andrii Nakryiko wrote: >>> On Mon, Feb 25, 2019 at 12:51 PM Andrii Nakryiko >>> <andrii.nakryiko@gmail.com> wrote: >>>> >>>> On Mon, Feb 25, 2019 at 12:08 PM Arnaldo Carvalho de Melo >>>> <arnaldo.melo@gmail.com> wrote: >>>>> >>>>> Em Mon, Feb 25, 2019 at 11:42:29AM -0800, Andrii Nakryiko escreveu: >>>>>> On Mon, Feb 25, 2019 at 11:00 AM Arnaldo Carvalho de Melo> <arnaldo.melo@gmail.com> wrote: >>>>> >>>>>>> Em Mon, Feb 25, 2019 at 01:02:39PM -0300, Arnaldo Carvalho de Melo escreveu: >>>>>>>> So yeah, the BTF encoder/decoder is working just fine, the problem is in >>>>>>>> pahole's DWARF code, lemme see... >>>>>>> >>>>>>> Please try the patch below, for me btfdiff continues to show no diff for >>>>>>> all types in my vmlinux and now it also produces the same output for >>>>>>> when the first element of a bitfield has its bit_size equal to its >>>>>>> byte_size * 8: >>>>>> >>>>>> Yes, this fixes all the issues I've seen. btfdiff output is now empty >>>>>> for my kernel image. Thanks for quick fix! >>>>>> >>>>>> Reviewed-by: Andrii Nakryiko <andriin@fb.com> >>>>> >>>>> Thanks, just for some extra testing I tried encoding BTF for glibc 2.28: >>>>> >>>>> [acme@quaco pahole]$ pahole -JV libc-2.28.so.debug >>>>> <BIG snip> >>>> >>>> <snip> >>>> >>>>> [386075] INT long unsigned int size=8 bit_offset=0 nr_bits=64 encoding=(none) >>>>> Cannot open libc-2.28.so.debug >>>>> Failed to encode BTF >>>> >>>> I've tried the same on libc-2.26.so.debug and it worked. >>>> >>>> It failes after BTF encoding and deduping is done, exactly when it >>>> tries to re-open libc-2.28.so.debug to write out new contents. Not >>>> sure why it would fail, as it seems to be just plain open() call, but >>>> the fact that we both can repro this means there is something there. >>>> >>>> It also seems that libc-2.28.so.debug is corrupted: >>>> >>>> $ readelf -n /home/andriin/local/btf/libc-2.26.so.debug | grep >>>> 'Unknown note type' | wc -l >>>> 0 >>>> $ readelf -n /home/andriin/local/btf/libc-2.28.so.debug | grep >>>> 'Unknown note type' | wc -l >>>> 17953 >>>> >>> >>> Ignore everything I've said (though 'Unknown note type' issue is >>> strange), for me it was just permissions issues. >>> >>> $ chmod +rw /home/andriin/local/btf/libc-2.28.so.debug >>> >>> $ ~/local/pahole/build/pahole -J ~/local/btf/libc-2.28.so.debug >>> >>> $ PAHOLE=~/local/pahole/build/pahole ./btfdiff ~/local/btf/libc-2.28.so.debug >>> base_type__name_to_size: base_type _Float128 >>> class__fixup_btf_bitfields: unknown base type name "_Float128"! >>> base_type__name_to_size: base_type _Float128 >>> class__fixup_btf_bitfields: unknown base type name "_Float128"! >>> base_type__name_to_size: base_type _Float128 >>> class__fixup_btf_bitfields: unknown base type name "_Float128"! >>> base_type__name_to_size: base_type _Float128 >>> class__fixup_btf_bitfields: unknown base type name "_Float128"! >>> base_type__name_to_size: base_type _Float128 >>> class__fixup_btf_bitfields: unknown base type name "_Float128"! >>> base_type__name_to_size: base_type _Float128 >>> class__fixup_btf_bitfields: unknown base type name "_Float128"! >>> --- /tmp/btfdiff.dwarf.aV4wSL 2019-02-25 13:31:54.787923673 -0800 >>> +++ /tmp/btfdiff.btf.22NQmJ 2019-02-25 13:31:54.802923668 -0800 >>> @@ -461,9 +461,15 @@ struct La_x86_64_retval { >>> uint64_t lrv_rdx; /* 8 8 */ >>> La_x86_64_xmm lrv_xmm0; /* 16 16 */ >>> La_x86_64_xmm lrv_xmm1; /* 32 16 */ >>> - long double lrv_st0; /* 48 16 */ >>> + long double lrv_st0; /* 48 8 */ >>> + >>> + /* XXX 8 bytes hole, try to pack */ >>> + >>> /* --- cacheline 1 boundary (64 bytes) --- */ >>> - long double lrv_st1; /* 64 16 */ >>> + long double lrv_st1; /* 64 8 */ >>> + >>> + /* XXX 8 bytes hole, try to pack */ >>> + >>> La_x86_64_vector lrv_vector0; /* 80 64 */ >>> /* --- cacheline 2 boundary (128 bytes) was 16 bytes ago --- */ >>> La_x86_64_vector lrv_vector1; /* 144 64 */ >>> @@ -472,6 +478,7 @@ struct La_x86_64_retval { >>> __int128 lrv_bnd1; /* 224 16 */ >>> >>> /* size: 240, cachelines: 4, members: 10 */ >>> + /* sum members: 224, holes: 2, sum holes: 16 */ >>> /* last cacheline: 48 bytes */ >>> }; >>> struct r_debug { >>> @@ -2044,7 +2051,7 @@ union ieee754_float { >>> } ieee_nan; /* 0 4 */ >>> }; >>> union ieee854_long_double { >>> - long double d; /* 0 16 */ >>> + long double d; /* 0 8 */ >>> struct { >>> unsigned int mantissa1:32; /* 0: 0 4 */ >>> unsigned int mantissa0:32; /* 4: 0 4 */ >>> @@ -2141,7 +2148,7 @@ struct ucontext_t { >>> /* last cacheline: 8 bytes */ >>> }; >>> union ieee854_float128 { >>> - _Float128 d; /* 0 16 */ >>> + _Float128 d; /* 0 0 */ >>> struct { >>> unsigned int mantissa3:32; /* 0: 0 4 */ >>> unsigned int mantissa2:32; /* 4: 0 4 */ >>> @@ -2219,7 +2226,7 @@ union printf_arg { >>> long unsigned int pa_u_long_int; /* 0 8 */ >>> long long unsigned int pa_u_long_long_int; /* 0 8 */ >>> double pa_double; /* 0 8 */ >>> - long double pa_long_double; /* 0 16 */ >>> + long double pa_long_double; /* 0 8 */ >>> const char * pa_string; /* 0 8 */ >>> const wchar_t * pa_wstring; /* 0 8 */ >>> void * pa_pointer; /* 0 8 */ >>> >>> Seems like "long double" size is invalid for BTF. And we should >>> probably add "_Float128" as another "well known" base type name? >> >> BPF does not support floating point instructions. As the result, >> BTF design does not contain floating point types. >> >> Looks like kernel also uses float types (in certain drivers and arch's), >> but types referred by bpf program mostly will not contain float types. >> >> Currently, both llvm and pahole will ignore floating types, so >> from type comparison point of view, we will be fine. >> The issue may come up with people using vmlinux BTF inside the >> kernel for different purpose where a float type needs to be >> specified, e.g., for a floating point function argument in ftrace. >> But this is rare too. >> >> I am not 100% sure whether pahole dwarf/btf parity is considered >> as a strong reason to add floating point type support in BTF or not. > > Isn't float and double just another base type with corresponding name? > Fundamentally, how is it different from int or long? It's still just a > collection of bits in 32-bit, 64-bit, etc number, except interpreted > differently. Do you mean that for BTF to properly support floats, > we'll need to extends set of possible encodings in INT type (in > addition to today's CHAR, SIGNED and UNSIGNED)? Probably another kind like BTF_KIND_FLOAT with additional u32 (in addition to btf_type) indicating the size (4 bytes, 8 bytes and 16 bytes). > >> >>> >>> Though for the latter, it probably would be better to resolve base >>> type size using BTF data itself. >>> >>>>> [acme@quaco pahole]$ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: encoding BTF on glibc was: Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader 2019-02-26 6:03 ` Yonghong Song @ 2019-02-26 17:22 ` Andrii Nakryiko 2019-02-26 17:24 ` Yonghong Song 0 siblings, 1 reply; 23+ messages in thread From: Andrii Nakryiko @ 2019-02-26 17:22 UTC (permalink / raw) To: Yonghong Song Cc: Arnaldo Carvalho de Melo, Andrii Nakryiko, dwarves, bpf, Alexei Starovoitov On Mon, Feb 25, 2019 at 10:03 PM Yonghong Song <yhs@fb.com> wrote: > > > > On 2/25/19 9:45 PM, Andrii Nakryiko wrote: > > On Mon, Feb 25, 2019 at 9:38 PM Yonghong Song <yhs@fb.com> wrote: > >> > >> > >> > >> On 2/25/19 1:36 PM, Andrii Nakryiko wrote: > >>> On Mon, Feb 25, 2019 at 12:51 PM Andrii Nakryiko > >>> <andrii.nakryiko@gmail.com> wrote: > >>>> > >>>> On Mon, Feb 25, 2019 at 12:08 PM Arnaldo Carvalho de Melo > >>>> <arnaldo.melo@gmail.com> wrote: > >>>>> > >>>>> Em Mon, Feb 25, 2019 at 11:42:29AM -0800, Andrii Nakryiko escreveu: > >>>>>> On Mon, Feb 25, 2019 at 11:00 AM Arnaldo Carvalho de Melo> <arnaldo.melo@gmail.com> wrote: > >>>>> > >>>>>>> Em Mon, Feb 25, 2019 at 01:02:39PM -0300, Arnaldo Carvalho de Melo escreveu: > >>>>>>>> So yeah, the BTF encoder/decoder is working just fine, the problem is in > >>>>>>>> pahole's DWARF code, lemme see... > >>>>>>> > >>>>>>> Please try the patch below, for me btfdiff continues to show no diff for > >>>>>>> all types in my vmlinux and now it also produces the same output for > >>>>>>> when the first element of a bitfield has its bit_size equal to its > >>>>>>> byte_size * 8: > >>>>>> > >>>>>> Yes, this fixes all the issues I've seen. btfdiff output is now empty > >>>>>> for my kernel image. Thanks for quick fix! > >>>>>> > >>>>>> Reviewed-by: Andrii Nakryiko <andriin@fb.com> > >>>>> > >>>>> Thanks, just for some extra testing I tried encoding BTF for glibc 2.28: > >>>>> > >>>>> [acme@quaco pahole]$ pahole -JV libc-2.28.so.debug > >>>>> <BIG snip> > >>>> > >>>> <snip> > >>>> <snip> > >>> > >>> Seems like "long double" size is invalid for BTF. And we should > >>> probably add "_Float128" as another "well known" base type name? > >> > >> BPF does not support floating point instructions. As the result, > >> BTF design does not contain floating point types. > >> > >> Looks like kernel also uses float types (in certain drivers and arch's), > >> but types referred by bpf program mostly will not contain float types. > >> > >> Currently, both llvm and pahole will ignore floating types, so > >> from type comparison point of view, we will be fine. > >> The issue may come up with people using vmlinux BTF inside the > >> kernel for different purpose where a float type needs to be > >> specified, e.g., for a floating point function argument in ftrace. > >> But this is rare too. > >> > >> I am not 100% sure whether pahole dwarf/btf parity is considered > >> as a strong reason to add floating point type support in BTF or not. > > > > Isn't float and double just another base type with corresponding name? > > Fundamentally, how is it different from int or long? It's still just a > > collection of bits in 32-bit, 64-bit, etc number, except interpreted > > differently. Do you mean that for BTF to properly support floats, > > we'll need to extends set of possible encodings in INT type (in > > addition to today's CHAR, SIGNED and UNSIGNED)? > > Probably another kind like BTF_KIND_FLOAT with additional u32 (in > addition to btf_type) indicating the size (4 bytes, 8 bytes and 16 bytes). This sounds exactly like current format of BTF_KIND_INT. What BTF_KIND_FLOAT will have that can't be represented by BTF_KIND_INT? Wouldn't it be equivalent to just adding BTF_INT_FLOAT in addition to existing BTF_INT_SIGNED, BTF_INT_CHAR, BTF_INT_BOOL to BTF_INT_ENCODING? Btw, while BTF_INT_SIGNED seems like it deserves separate bit, because it's mostly orthogonal to CHAR vs INT vs FLOAT vs (not so much) BOOL, BTF_INT_CHAR and BTF_INT_BOOL take two independent bits seems excessive. You can't have BOOL and CHAR, right? So why don't we just repurpose bits 1 and 2 to be a small 4-value enum: enum btf_int_encoding { INT = 0, CHAR = 1, BOOL = 2, FLOAT = 3, }; This is backwards compatible and covers all floats (long double vs double vs float is just a question of bitness). Am I missing something here? > > > > >> > >>> > >>> Though for the latter, it probably would be better to resolve base > >>> type size using BTF data itself. > >>> > >>>>> [acme@quaco pahole]$ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: encoding BTF on glibc was: Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader 2019-02-26 17:22 ` Andrii Nakryiko @ 2019-02-26 17:24 ` Yonghong Song 2019-02-26 17:34 ` Andrii Nakryiko 0 siblings, 1 reply; 23+ messages in thread From: Yonghong Song @ 2019-02-26 17:24 UTC (permalink / raw) To: Andrii Nakryiko Cc: Arnaldo Carvalho de Melo, Andrii Nakryiko, dwarves, bpf, Alexei Starovoitov On 2/26/19 9:22 AM, Andrii Nakryiko wrote: > On Mon, Feb 25, 2019 at 10:03 PM Yonghong Song <yhs@fb.com> wrote: >> >> >> >> On 2/25/19 9:45 PM, Andrii Nakryiko wrote: >>> On Mon, Feb 25, 2019 at 9:38 PM Yonghong Song <yhs@fb.com> wrote: >>>> >>>> >>>> >>>> On 2/25/19 1:36 PM, Andrii Nakryiko wrote: >>>>> On Mon, Feb 25, 2019 at 12:51 PM Andrii Nakryiko >>>>> <andrii.nakryiko@gmail.com> wrote: >>>>>> >>>>>> On Mon, Feb 25, 2019 at 12:08 PM Arnaldo Carvalho de Melo >>>>>> <arnaldo.melo@gmail.com> wrote: >>>>>>> >>>>>>> Em Mon, Feb 25, 2019 at 11:42:29AM -0800, Andrii Nakryiko escreveu: >>>>>>>> On Mon, Feb 25, 2019 at 11:00 AM Arnaldo Carvalho de Melo> <arnaldo.melo@gmail.com> wrote: >>>>>>> >>>>>>>>> Em Mon, Feb 25, 2019 at 01:02:39PM -0300, Arnaldo Carvalho de Melo escreveu: >>>>>>>>>> So yeah, the BTF encoder/decoder is working just fine, the problem is in >>>>>>>>>> pahole's DWARF code, lemme see... >>>>>>>>> >>>>>>>>> Please try the patch below, for me btfdiff continues to show no diff for >>>>>>>>> all types in my vmlinux and now it also produces the same output for >>>>>>>>> when the first element of a bitfield has its bit_size equal to its >>>>>>>>> byte_size * 8: >>>>>>>> >>>>>>>> Yes, this fixes all the issues I've seen. btfdiff output is now empty >>>>>>>> for my kernel image. Thanks for quick fix! >>>>>>>> >>>>>>>> Reviewed-by: Andrii Nakryiko <andriin@fb.com> >>>>>>> >>>>>>> Thanks, just for some extra testing I tried encoding BTF for glibc 2.28: >>>>>>> >>>>>>> [acme@quaco pahole]$ pahole -JV libc-2.28.so.debug >>>>>>> <BIG snip> >>>>>> >>>>>> <snip> >>>>>> > > <snip> > >>>>> >>>>> Seems like "long double" size is invalid for BTF. And we should >>>>> probably add "_Float128" as another "well known" base type name? >>>> >>>> BPF does not support floating point instructions. As the result, >>>> BTF design does not contain floating point types. >>>> >>>> Looks like kernel also uses float types (in certain drivers and arch's), >>>> but types referred by bpf program mostly will not contain float types. >>>> >>>> Currently, both llvm and pahole will ignore floating types, so >>>> from type comparison point of view, we will be fine. >>>> The issue may come up with people using vmlinux BTF inside the >>>> kernel for different purpose where a float type needs to be >>>> specified, e.g., for a floating point function argument in ftrace. >>>> But this is rare too. >>>> >>>> I am not 100% sure whether pahole dwarf/btf parity is considered >>>> as a strong reason to add floating point type support in BTF or not. >>> >>> Isn't float and double just another base type with corresponding name? >>> Fundamentally, how is it different from int or long? It's still just a >>> collection of bits in 32-bit, 64-bit, etc number, except interpreted >>> differently. Do you mean that for BTF to properly support floats, >>> we'll need to extends set of possible encodings in INT type (in >>> addition to today's CHAR, SIGNED and UNSIGNED)? >> >> Probably another kind like BTF_KIND_FLOAT with additional u32 (in >> addition to btf_type) indicating the size (4 bytes, 8 bytes and 16 bytes). > > This sounds exactly like current format of BTF_KIND_INT. What > BTF_KIND_FLOAT will have that can't be represented by BTF_KIND_INT? > Wouldn't it be equivalent to just adding BTF_INT_FLOAT in addition to > existing BTF_INT_SIGNED, BTF_INT_CHAR, BTF_INT_BOOL to > BTF_INT_ENCODING? Btw, while BTF_INT_SIGNED seems like it deserves > separate bit, because it's mostly orthogonal to CHAR vs INT vs FLOAT > vs (not so much) BOOL, BTF_INT_CHAR and BTF_INT_BOOL take two > independent bits seems excessive. You can't have BOOL and CHAR, right? > So why don't we just repurpose bits 1 and 2 to be a small 4-value > enum: > > enum btf_int_encoding { > INT = 0, > CHAR = 1, > BOOL = 2, > FLOAT = 3, > }; > > This is backwards compatible and covers all floats (long double vs > double vs float is just a question of bitness). > > Am I missing something here? float is not an "int". Encoding "float" in INT_KIND does not sound right in my opinion. > >> >>> >>>> >>>>> >>>>> Though for the latter, it probably would be better to resolve base >>>>> type size using BTF data itself. >>>>> >>>>>>> [acme@quaco pahole]$ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: encoding BTF on glibc was: Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader 2019-02-26 17:24 ` Yonghong Song @ 2019-02-26 17:34 ` Andrii Nakryiko 0 siblings, 0 replies; 23+ messages in thread From: Andrii Nakryiko @ 2019-02-26 17:34 UTC (permalink / raw) To: Yonghong Song Cc: Arnaldo Carvalho de Melo, Andrii Nakryiko, dwarves, bpf, Alexei Starovoitov On Tue, Feb 26, 2019 at 9:25 AM Yonghong Song <yhs@fb.com> wrote: > > > > On 2/26/19 9:22 AM, Andrii Nakryiko wrote: > > On Mon, Feb 25, 2019 at 10:03 PM Yonghong Song <yhs@fb.com> wrote: > >> > >> > >> > >> On 2/25/19 9:45 PM, Andrii Nakryiko wrote: > >>> On Mon, Feb 25, 2019 at 9:38 PM Yonghong Song <yhs@fb.com> wrote: > >>>> > >>>> > >>>> > >>>> On 2/25/19 1:36 PM, Andrii Nakryiko wrote: > >>>>> On Mon, Feb 25, 2019 at 12:51 PM Andrii Nakryiko > >>>>> <andrii.nakryiko@gmail.com> wrote: > >>>>>> > >>>>>> On Mon, Feb 25, 2019 at 12:08 PM Arnaldo Carvalho de Melo > >>>>>> <arnaldo.melo@gmail.com> wrote: > >>>>>>> > >>>>>>> Em Mon, Feb 25, 2019 at 11:42:29AM -0800, Andrii Nakryiko escreveu: > >>>>>>>> On Mon, Feb 25, 2019 at 11:00 AM Arnaldo Carvalho de Melo> <arnaldo.melo@gmail.com> wrote: > >>>>>>> > >>>>>>>>> Em Mon, Feb 25, 2019 at 01:02:39PM -0300, Arnaldo Carvalho de Melo escreveu: > >>>>>>>>>> So yeah, the BTF encoder/decoder is working just fine, the problem is in > >>>>>>>>>> pahole's DWARF code, lemme see... > >>>>>>>>> > >>>>>>>>> Please try the patch below, for me btfdiff continues to show no diff for > >>>>>>>>> all types in my vmlinux and now it also produces the same output for > >>>>>>>>> when the first element of a bitfield has its bit_size equal to its > >>>>>>>>> byte_size * 8: > >>>>>>>> > >>>>>>>> Yes, this fixes all the issues I've seen. btfdiff output is now empty > >>>>>>>> for my kernel image. Thanks for quick fix! > >>>>>>>> > >>>>>>>> Reviewed-by: Andrii Nakryiko <andriin@fb.com> > >>>>>>> > >>>>>>> Thanks, just for some extra testing I tried encoding BTF for glibc 2.28: > >>>>>>> > >>>>>>> [acme@quaco pahole]$ pahole -JV libc-2.28.so.debug > >>>>>>> <BIG snip> > >>>>>> > >>>>>> <snip> > >>>>>> > > > > <snip> > > > >>>>> > >>>>> Seems like "long double" size is invalid for BTF. And we should > >>>>> probably add "_Float128" as another "well known" base type name? > >>>> > >>>> BPF does not support floating point instructions. As the result, > >>>> BTF design does not contain floating point types. > >>>> > >>>> Looks like kernel also uses float types (in certain drivers and arch's), > >>>> but types referred by bpf program mostly will not contain float types. > >>>> > >>>> Currently, both llvm and pahole will ignore floating types, so > >>>> from type comparison point of view, we will be fine. > >>>> The issue may come up with people using vmlinux BTF inside the > >>>> kernel for different purpose where a float type needs to be > >>>> specified, e.g., for a floating point function argument in ftrace. > >>>> But this is rare too. > >>>> > >>>> I am not 100% sure whether pahole dwarf/btf parity is considered > >>>> as a strong reason to add floating point type support in BTF or not. > >>> > >>> Isn't float and double just another base type with corresponding name? > >>> Fundamentally, how is it different from int or long? It's still just a > >>> collection of bits in 32-bit, 64-bit, etc number, except interpreted > >>> differently. Do you mean that for BTF to properly support floats, > >>> we'll need to extends set of possible encodings in INT type (in > >>> addition to today's CHAR, SIGNED and UNSIGNED)? > >> > >> Probably another kind like BTF_KIND_FLOAT with additional u32 (in > >> addition to btf_type) indicating the size (4 bytes, 8 bytes and 16 bytes). > > > > This sounds exactly like current format of BTF_KIND_INT. What > > BTF_KIND_FLOAT will have that can't be represented by BTF_KIND_INT? > > Wouldn't it be equivalent to just adding BTF_INT_FLOAT in addition to > > existing BTF_INT_SIGNED, BTF_INT_CHAR, BTF_INT_BOOL to > > BTF_INT_ENCODING? Btw, while BTF_INT_SIGNED seems like it deserves > > separate bit, because it's mostly orthogonal to CHAR vs INT vs FLOAT > > vs (not so much) BOOL, BTF_INT_CHAR and BTF_INT_BOOL take two > > independent bits seems excessive. You can't have BOOL and CHAR, right? > > So why don't we just repurpose bits 1 and 2 to be a small 4-value > > enum: > > > > enum btf_int_encoding { > > INT = 0, > > CHAR = 1, > > BOOL = 2, > > FLOAT = 3, > > }; > > > > This is backwards compatible and covers all floats (long double vs > > double vs float is just a question of bitness). > > > > Am I missing something here? > > float is not an "int". Encoding "float" in INT_KIND does not sound right > in my opinion. So isn't "bool". But in the end, it's just a collection of bits that are interpreted according to some encoding based on convention. From application perspective, none of float, int or bool has any more structure, those are "opaque" values. So maybe name BTF_KIND_INT is a bit unfortunate, it should be more like BTF_KIND_PRIMITIVE or something (or in pahole's lingo: base type). > > > > >> > >>> > >>>> > >>>>> > >>>>> Though for the latter, it probably would be better to resolve base > >>>>> type size using BTF data itself. > >>>>> > >>>>>>> [acme@quaco pahole]$ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: encoding BTF on glibc was: Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader 2019-02-25 21:36 ` Andrii Nakryiko 2019-02-26 0:34 ` Andrii Nakryiko 2019-02-26 5:37 ` Yonghong Song @ 2019-02-26 13:11 ` Arnaldo Carvalho de Melo 2019-02-26 17:14 ` Andrii Nakryiko 2 siblings, 1 reply; 23+ messages in thread From: Arnaldo Carvalho de Melo @ 2019-02-26 13:11 UTC (permalink / raw) To: Andrii Nakryiko Cc: Arnaldo Carvalho de Melo, Andrii Nakryiko, dwarves, bpf, Alexei Starovoitov, Yonghong Song Em Mon, Feb 25, 2019 at 01:36:57PM -0800, Andrii Nakryiko escreveu: > On Mon, Feb 25, 2019 at 12:51 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Mon, Feb 25, 2019 at 12:08 PM Arnaldo Carvalho de Melo > > <arnaldo.melo@gmail.com> wrote: > > > > > > Em Mon, Feb 25, 2019 at 11:42:29AM -0800, Andrii Nakryiko escreveu: > > > > On Mon, Feb 25, 2019 at 11:00 AM Arnaldo Carvalho de Melo> <arnaldo.melo@gmail.com> wrote: > > > > > > > > Em Mon, Feb 25, 2019 at 01:02:39PM -0300, Arnaldo Carvalho de Melo escreveu: > > > > > > So yeah, the BTF encoder/decoder is working just fine, the problem is in > > > > > > pahole's DWARF code, lemme see... > > > > > > > > > > Please try the patch below, for me btfdiff continues to show no diff for > > > > > all types in my vmlinux and now it also produces the same output for > > > > > when the first element of a bitfield has its bit_size equal to its > > > > > byte_size * 8: > > > > > > > > Yes, this fixes all the issues I've seen. btfdiff output is now empty > > > > for my kernel image. Thanks for quick fix! > > > > > > > > Reviewed-by: Andrii Nakryiko <andriin@fb.com> > > > > > > Thanks, just for some extra testing I tried encoding BTF for glibc 2.28: > > > > > > [acme@quaco pahole]$ pahole -JV libc-2.28.so.debug > > > <BIG snip> > > > > <snip> > > > > > [386075] INT long unsigned int size=8 bit_offset=0 nr_bits=64 encoding=(none) > > > Cannot open libc-2.28.so.debug > > > Failed to encode BTF > > > > I've tried the same on libc-2.26.so.debug and it worked. > > > > It failes after BTF encoding and deduping is done, exactly when it > > tries to re-open libc-2.28.so.debug to write out new contents. Not > > sure why it would fail, as it seems to be just plain open() call, but > > the fact that we both can repro this means there is something there. > > > > It also seems that libc-2.28.so.debug is corrupted: > > > > $ readelf -n /home/andriin/local/btf/libc-2.26.so.debug | grep > > 'Unknown note type' | wc -l > > 0 > > $ readelf -n /home/andriin/local/btf/libc-2.28.so.debug | grep > > 'Unknown note type' | wc -l > > 17953 > > > > Ignore everything I've said (though 'Unknown note type' issue is > strange), for me it was just permissions issues. yeah, we need to fix up these messages, I did the chmod here and got further. [acme@quaco pahole]$ ls -la libc-2.28.so.debug -r--r--r--. 1 acme acme 17189368 Feb 25 17:04 libc-2.28.so.debug [acme@quaco pahole]$ pahole -J libc-2.28.so.debug Cannot open libc-2.28.so.debug Failed to encode BTF [acme@quaco pahole]$ chmod +w libc-2.28.so.debug [acme@quaco pahole]$ ls -la libc-2.28.so.debug -rw-rw-r--. 1 acme acme 17189368 Feb 25 17:04 libc-2.28.so.debug [acme@quaco pahole]$ pahole -J libc-2.28.so.debug [acme@quaco pahole]$ And I get the same warnings. base_type__name_to_size: base_type _Float128 If we just add _Float128 to the base_type__name_to_size arrays I think its good enough for pretty printing, i.e. we won't know from encoding that this is a float or double, as CTF and DWARF can, but for printing purposes, even regenerating the type for consumption by other tools, it should just work, no? So, two bugs, the bit_size for "long double" is 128, not 64, and if we add an entry for _Float128, then btfdiff is clean for libc, without any further work on properly supporting float types as in CTF or DWARF: [acme@quaco pahole]$ btfdiff libc-2.28.so.debug [acme@quaco pahole]$ git diff diff --git a/dwarves.c b/dwarves.c index adc411a8279d..48a018a9c354 100644 --- a/dwarves.c +++ b/dwarves.c @@ -168,11 +168,12 @@ static struct base_type_name_to_size { { .name = "double double", .size = 64, }, { .name = "single float", .size = 32, }, { .name = "float", .size = 32, }, - { .name = "long double", .size = 64, }, - { .name = "long double long double", .size = 64, }, + { .name = "long double", .size = sizeof(long double) * 8, }, + { .name = "long double long double", .size = sizeof(long double) * 8, }, { .name = "__int128", .size = 128, }, { .name = "unsigned __int128", .size = 128, }, { .name = "__int128 unsigned", .size = 128, }, + { .name = "_Float128", .size = 128, }, { .name = NULL }, }; [acme@quaco pahole]$ I'll add a series of patches fixing up the types that don't have encoded in its name the size in bits, so that it works on all arches, one more for _Float128, should be enough for now, right? - Arnaldo > $ chmod +rw /home/andriin/local/btf/libc-2.28.so.debug > > $ ~/local/pahole/build/pahole -J ~/local/btf/libc-2.28.so.debug > > $ PAHOLE=~/local/pahole/build/pahole ./btfdiff ~/local/btf/libc-2.28.so.debug > base_type__name_to_size: base_type _Float128 > class__fixup_btf_bitfields: unknown base type name "_Float128"! > base_type__name_to_size: base_type _Float128 > class__fixup_btf_bitfields: unknown base type name "_Float128"! > base_type__name_to_size: base_type _Float128 > class__fixup_btf_bitfields: unknown base type name "_Float128"! > base_type__name_to_size: base_type _Float128 > class__fixup_btf_bitfields: unknown base type name "_Float128"! > base_type__name_to_size: base_type _Float128 > class__fixup_btf_bitfields: unknown base type name "_Float128"! > base_type__name_to_size: base_type _Float128 > class__fixup_btf_bitfields: unknown base type name "_Float128"! > --- /tmp/btfdiff.dwarf.aV4wSL 2019-02-25 13:31:54.787923673 -0800 > +++ /tmp/btfdiff.btf.22NQmJ 2019-02-25 13:31:54.802923668 -0800 > @@ -461,9 +461,15 @@ struct La_x86_64_retval { > uint64_t lrv_rdx; /* 8 8 */ > La_x86_64_xmm lrv_xmm0; /* 16 16 */ > La_x86_64_xmm lrv_xmm1; /* 32 16 */ > - long double lrv_st0; /* 48 16 */ > + long double lrv_st0; /* 48 8 */ > + > + /* XXX 8 bytes hole, try to pack */ > + > /* --- cacheline 1 boundary (64 bytes) --- */ > - long double lrv_st1; /* 64 16 */ > + long double lrv_st1; /* 64 8 */ > + > + /* XXX 8 bytes hole, try to pack */ > + > La_x86_64_vector lrv_vector0; /* 80 64 */ > /* --- cacheline 2 boundary (128 bytes) was 16 bytes ago --- */ > La_x86_64_vector lrv_vector1; /* 144 64 */ > @@ -472,6 +478,7 @@ struct La_x86_64_retval { > __int128 lrv_bnd1; /* 224 16 */ > > /* size: 240, cachelines: 4, members: 10 */ > + /* sum members: 224, holes: 2, sum holes: 16 */ > /* last cacheline: 48 bytes */ > }; > struct r_debug { > @@ -2044,7 +2051,7 @@ union ieee754_float { > } ieee_nan; /* 0 4 */ > }; > union ieee854_long_double { > - long double d; /* 0 16 */ > + long double d; /* 0 8 */ > struct { > unsigned int mantissa1:32; /* 0: 0 4 */ > unsigned int mantissa0:32; /* 4: 0 4 */ > @@ -2141,7 +2148,7 @@ struct ucontext_t { > /* last cacheline: 8 bytes */ > }; > union ieee854_float128 { > - _Float128 d; /* 0 16 */ > + _Float128 d; /* 0 0 */ > struct { > unsigned int mantissa3:32; /* 0: 0 4 */ > unsigned int mantissa2:32; /* 4: 0 4 */ > @@ -2219,7 +2226,7 @@ union printf_arg { > long unsigned int pa_u_long_int; /* 0 8 */ > long long unsigned int pa_u_long_long_int; /* 0 8 */ > double pa_double; /* 0 8 */ > - long double pa_long_double; /* 0 16 */ > + long double pa_long_double; /* 0 8 */ > const char * pa_string; /* 0 8 */ > const wchar_t * pa_wstring; /* 0 8 */ > void * pa_pointer; /* 0 8 */ > > Seems like "long double" size is invalid for BTF. And we should > probably add "_Float128" as another "well known" base type name? > > Though for the latter, it probably would be better to resolve base > type size using BTF data itself. > > > > [acme@quaco pahole]$ -- - Arnaldo ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: encoding BTF on glibc was: Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader 2019-02-26 13:11 ` Arnaldo Carvalho de Melo @ 2019-02-26 17:14 ` Andrii Nakryiko 2019-02-26 17:45 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 23+ messages in thread From: Andrii Nakryiko @ 2019-02-26 17:14 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Andrii Nakryiko, dwarves, bpf, Alexei Starovoitov, Yonghong Song On Tue, Feb 26, 2019 at 5:12 AM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > > Em Mon, Feb 25, 2019 at 01:36:57PM -0800, Andrii Nakryiko escreveu: > > On Mon, Feb 25, 2019 at 12:51 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Mon, Feb 25, 2019 at 12:08 PM Arnaldo Carvalho de Melo > > > <arnaldo.melo@gmail.com> wrote: > > > > > > > > Em Mon, Feb 25, 2019 at 11:42:29AM -0800, Andrii Nakryiko escreveu: > > > > > On Mon, Feb 25, 2019 at 11:00 AM Arnaldo Carvalho de Melo> <arnaldo.melo@gmail.com> wrote: > > > > > > > > > > Em Mon, Feb 25, 2019 at 01:02:39PM -0300, Arnaldo Carvalho de Melo escreveu: > > > > > > > So yeah, the BTF encoder/decoder is working just fine, the problem is in > > > > > > > pahole's DWARF code, lemme see... > > > > > > > > > > > > Please try the patch below, for me btfdiff continues to show no diff for > > > > > > all types in my vmlinux and now it also produces the same output for > > > > > > when the first element of a bitfield has its bit_size equal to its > > > > > > byte_size * 8: > > > > > > > > > > Yes, this fixes all the issues I've seen. btfdiff output is now empty > > > > > for my kernel image. Thanks for quick fix! > > > > > > > > > > Reviewed-by: Andrii Nakryiko <andriin@fb.com> > > > > > > > > Thanks, just for some extra testing I tried encoding BTF for glibc 2.28: > > > > > > > > [acme@quaco pahole]$ pahole -JV libc-2.28.so.debug > > > > <BIG snip> > > > > > > <snip> > > > > > > > [386075] INT long unsigned int size=8 bit_offset=0 nr_bits=64 encoding=(none) > > > > Cannot open libc-2.28.so.debug > > > > Failed to encode BTF > > > > > > I've tried the same on libc-2.26.so.debug and it worked. > > > > > > It failes after BTF encoding and deduping is done, exactly when it > > > tries to re-open libc-2.28.so.debug to write out new contents. Not > > > sure why it would fail, as it seems to be just plain open() call, but > > > the fact that we both can repro this means there is something there. > > > > > > It also seems that libc-2.28.so.debug is corrupted: > > > > > > $ readelf -n /home/andriin/local/btf/libc-2.26.so.debug | grep > > > 'Unknown note type' | wc -l > > > 0 > > > $ readelf -n /home/andriin/local/btf/libc-2.28.so.debug | grep > > > 'Unknown note type' | wc -l > > > 17953 > > > > > > > Ignore everything I've said (though 'Unknown note type' issue is > > strange), for me it was just permissions issues. > > yeah, we need to fix up these messages, I did the chmod here and got > further. > > [acme@quaco pahole]$ ls -la libc-2.28.so.debug > -r--r--r--. 1 acme acme 17189368 Feb 25 17:04 libc-2.28.so.debug > [acme@quaco pahole]$ pahole -J libc-2.28.so.debug > Cannot open libc-2.28.so.debug > Failed to encode BTF > [acme@quaco pahole]$ chmod +w libc-2.28.so.debug > [acme@quaco pahole]$ ls -la libc-2.28.so.debug > -rw-rw-r--. 1 acme acme 17189368 Feb 25 17:04 libc-2.28.so.debug > [acme@quaco pahole]$ pahole -J libc-2.28.so.debug > [acme@quaco pahole]$ > > And I get the same warnings. > > base_type__name_to_size: base_type _Float128 > > If we just add _Float128 to the base_type__name_to_size arrays I think > its good enough for pretty printing, i.e. we won't know from encoding > that this is a float or double, as CTF and DWARF can, but for printing > purposes, even regenerating the type for consumption by other tools, it > should just work, no? With "btf_loader: Simplify fixup code by relying on BTF data more" patch, btf_loader doesn't rely on this mapping at all. So if there is any other base type we didn't foresee, BTF output should still be correct. > > So, two bugs, the bit_size for "long double" is 128, not 64, and if we > add an entry for _Float128, then btfdiff is clean for libc, without any > further work on properly supporting float types as in CTF or DWARF: My worry with this table (and one of the reasons for that patch to not use it at all) was that those sizeof() sizes might be correct for the architecture on which pahole was built, but not necessarily on the target arch for which ELF file was built. E.g., I don't know if it's true that long double is always 128-bit on all architectures, so relying on that table might still break output for CTF and DWARF (but not BTF, because it takes bitness from BTF data itself). But otherwise it's still good thing to complete that table. > > [acme@quaco pahole]$ btfdiff libc-2.28.so.debug > [acme@quaco pahole]$ git diff > diff --git a/dwarves.c b/dwarves.c > index adc411a8279d..48a018a9c354 100644 > --- a/dwarves.c > +++ b/dwarves.c > @@ -168,11 +168,12 @@ static struct base_type_name_to_size { > { .name = "double double", .size = 64, }, > { .name = "single float", .size = 32, }, > { .name = "float", .size = 32, }, > - { .name = "long double", .size = 64, }, > - { .name = "long double long double", .size = 64, }, > + { .name = "long double", .size = sizeof(long double) * 8, }, > + { .name = "long double long double", .size = sizeof(long double) * 8, }, > { .name = "__int128", .size = 128, }, > { .name = "unsigned __int128", .size = 128, }, > { .name = "__int128 unsigned", .size = 128, }, > + { .name = "_Float128", .size = 128, }, > { .name = NULL }, > }; > > [acme@quaco pahole]$ > > I'll add a series of patches fixing up the types that don't have encoded > in its name the size in bits, so that it works on all arches, one more > for _Float128, should be enough for now, right? > > - Arnaldo > > > $ chmod +rw /home/andriin/local/btf/libc-2.28.so.debug > > > > $ ~/local/pahole/build/pahole -J ~/local/btf/libc-2.28.so.debug > > > > $ PAHOLE=~/local/pahole/build/pahole ./btfdiff ~/local/btf/libc-2.28.so.debug > > base_type__name_to_size: base_type _Float128 > > class__fixup_btf_bitfields: unknown base type name "_Float128"! > > base_type__name_to_size: base_type _Float128 > > class__fixup_btf_bitfields: unknown base type name "_Float128"! > > base_type__name_to_size: base_type _Float128 > > class__fixup_btf_bitfields: unknown base type name "_Float128"! > > base_type__name_to_size: base_type _Float128 > > class__fixup_btf_bitfields: unknown base type name "_Float128"! > > base_type__name_to_size: base_type _Float128 > > class__fixup_btf_bitfields: unknown base type name "_Float128"! > > base_type__name_to_size: base_type _Float128 > > class__fixup_btf_bitfields: unknown base type name "_Float128"! > > --- /tmp/btfdiff.dwarf.aV4wSL 2019-02-25 13:31:54.787923673 -0800 > > +++ /tmp/btfdiff.btf.22NQmJ 2019-02-25 13:31:54.802923668 -0800 > > @@ -461,9 +461,15 @@ struct La_x86_64_retval { > > uint64_t lrv_rdx; /* 8 8 */ > > La_x86_64_xmm lrv_xmm0; /* 16 16 */ > > La_x86_64_xmm lrv_xmm1; /* 32 16 */ > > - long double lrv_st0; /* 48 16 */ > > + long double lrv_st0; /* 48 8 */ > > + > > + /* XXX 8 bytes hole, try to pack */ > > + > > /* --- cacheline 1 boundary (64 bytes) --- */ > > - long double lrv_st1; /* 64 16 */ > > + long double lrv_st1; /* 64 8 */ > > + > > + /* XXX 8 bytes hole, try to pack */ > > + > > La_x86_64_vector lrv_vector0; /* 80 64 */ > > /* --- cacheline 2 boundary (128 bytes) was 16 bytes ago --- */ > > La_x86_64_vector lrv_vector1; /* 144 64 */ > > @@ -472,6 +478,7 @@ struct La_x86_64_retval { > > __int128 lrv_bnd1; /* 224 16 */ > > > > /* size: 240, cachelines: 4, members: 10 */ > > + /* sum members: 224, holes: 2, sum holes: 16 */ > > /* last cacheline: 48 bytes */ > > }; > > struct r_debug { > > @@ -2044,7 +2051,7 @@ union ieee754_float { > > } ieee_nan; /* 0 4 */ > > }; > > union ieee854_long_double { > > - long double d; /* 0 16 */ > > + long double d; /* 0 8 */ > > struct { > > unsigned int mantissa1:32; /* 0: 0 4 */ > > unsigned int mantissa0:32; /* 4: 0 4 */ > > @@ -2141,7 +2148,7 @@ struct ucontext_t { > > /* last cacheline: 8 bytes */ > > }; > > union ieee854_float128 { > > - _Float128 d; /* 0 16 */ > > + _Float128 d; /* 0 0 */ > > struct { > > unsigned int mantissa3:32; /* 0: 0 4 */ > > unsigned int mantissa2:32; /* 4: 0 4 */ > > @@ -2219,7 +2226,7 @@ union printf_arg { > > long unsigned int pa_u_long_int; /* 0 8 */ > > long long unsigned int pa_u_long_long_int; /* 0 8 */ > > double pa_double; /* 0 8 */ > > - long double pa_long_double; /* 0 16 */ > > + long double pa_long_double; /* 0 8 */ > > const char * pa_string; /* 0 8 */ > > const wchar_t * pa_wstring; /* 0 8 */ > > void * pa_pointer; /* 0 8 */ > > > > Seems like "long double" size is invalid for BTF. And we should > > probably add "_Float128" as another "well known" base type name? > > > > Though for the latter, it probably would be better to resolve base > > type size using BTF data itself. > > > > > > [acme@quaco pahole]$ > > -- > > - Arnaldo ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: encoding BTF on glibc was: Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader 2019-02-26 17:14 ` Andrii Nakryiko @ 2019-02-26 17:45 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 23+ messages in thread From: Arnaldo Carvalho de Melo @ 2019-02-26 17:45 UTC (permalink / raw) To: Andrii Nakryiko Cc: Arnaldo Carvalho de Melo, Andrii Nakryiko, dwarves, bpf, Alexei Starovoitov, Yonghong Song Em Tue, Feb 26, 2019 at 09:14:57AM -0800, Andrii Nakryiko escreveu: > On Tue, Feb 26, 2019 at 5:12 AM Arnaldo Carvalho de Melo > <arnaldo.melo@gmail.com> wrote: > > > > Em Mon, Feb 25, 2019 at 01:36:57PM -0800, Andrii Nakryiko escreveu: > > > On Mon, Feb 25, 2019 at 12:51 PM Andrii Nakryiko > > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > On Mon, Feb 25, 2019 at 12:08 PM Arnaldo Carvalho de Melo > > > > <arnaldo.melo@gmail.com> wrote: > > > > > > > > > > Em Mon, Feb 25, 2019 at 11:42:29AM -0800, Andrii Nakryiko escreveu: > > > > > > On Mon, Feb 25, 2019 at 11:00 AM Arnaldo Carvalho de Melo> <arnaldo.melo@gmail.com> wrote: > > > > > > > > > > > > Em Mon, Feb 25, 2019 at 01:02:39PM -0300, Arnaldo Carvalho de Melo escreveu: > > > > > > > > So yeah, the BTF encoder/decoder is working just fine, the problem is in > > > > > > > > pahole's DWARF code, lemme see... > > > > > > > > > > > > > > Please try the patch below, for me btfdiff continues to show no diff for > > > > > > > all types in my vmlinux and now it also produces the same output for > > > > > > > when the first element of a bitfield has its bit_size equal to its > > > > > > > byte_size * 8: > > > > > > > > > > > > Yes, this fixes all the issues I've seen. btfdiff output is now empty > > > > > > for my kernel image. Thanks for quick fix! > > > > > > > > > > > > Reviewed-by: Andrii Nakryiko <andriin@fb.com> > > > > > > > > > > Thanks, just for some extra testing I tried encoding BTF for glibc 2.28: > > > > > > > > > > [acme@quaco pahole]$ pahole -JV libc-2.28.so.debug > > > > > <BIG snip> > > > > > > > > <snip> > > > > > > > > > [386075] INT long unsigned int size=8 bit_offset=0 nr_bits=64 encoding=(none) > > > > > Cannot open libc-2.28.so.debug > > > > > Failed to encode BTF > > > > > > > > I've tried the same on libc-2.26.so.debug and it worked. > > > > > > > > It failes after BTF encoding and deduping is done, exactly when it > > > > tries to re-open libc-2.28.so.debug to write out new contents. Not > > > > sure why it would fail, as it seems to be just plain open() call, but > > > > the fact that we both can repro this means there is something there. > > > > > > > > It also seems that libc-2.28.so.debug is corrupted: > > > > > > > > $ readelf -n /home/andriin/local/btf/libc-2.26.so.debug | grep > > > > 'Unknown note type' | wc -l > > > > 0 > > > > $ readelf -n /home/andriin/local/btf/libc-2.28.so.debug | grep > > > > 'Unknown note type' | wc -l > > > > 17953 > > > > > > > > > > Ignore everything I've said (though 'Unknown note type' issue is > > > strange), for me it was just permissions issues. > > > > yeah, we need to fix up these messages, I did the chmod here and got > > further. > > > > [acme@quaco pahole]$ ls -la libc-2.28.so.debug > > -r--r--r--. 1 acme acme 17189368 Feb 25 17:04 libc-2.28.so.debug > > [acme@quaco pahole]$ pahole -J libc-2.28.so.debug > > Cannot open libc-2.28.so.debug > > Failed to encode BTF > > [acme@quaco pahole]$ chmod +w libc-2.28.so.debug > > [acme@quaco pahole]$ ls -la libc-2.28.so.debug > > -rw-rw-r--. 1 acme acme 17189368 Feb 25 17:04 libc-2.28.so.debug > > [acme@quaco pahole]$ pahole -J libc-2.28.so.debug > > [acme@quaco pahole]$ > > > > And I get the same warnings. > > > > base_type__name_to_size: base_type _Float128 > > > > If we just add _Float128 to the base_type__name_to_size arrays I think > > its good enough for pretty printing, i.e. we won't know from encoding > > that this is a float or double, as CTF and DWARF can, but for printing > > purposes, even regenerating the type for consumption by other tools, it > > should just work, no? > > With "btf_loader: Simplify fixup code by relying on BTF data more" patch, > btf_loader doesn't rely on this mapping at all. So if there is any other > base type we didn't foresee, BTF output should still be correct. > > > So, two bugs, the bit_size for "long double" is 128, not 64, and if we > > add an entry for _Float128, then btfdiff is clean for libc, without any > > further work on properly supporting float types as in CTF or DWARF: > > My worry with this table (and one of the reasons for that patch to not use > it at all) was that those sizeof() sizes might be correct for the > architecture on which pahole was built, but not necessarily on the target > arch for which ELF file was built. E.g., I don't know if it's true that Right, that is why there are some entries with size zero, meaning they need to be calculated based on the size of a pointer in the arch where it was produced, not the one where it is beind processed. But yeah, I even made a note about that in the cset I added fixing the bit_size for "long double". As you said, I have to look and see if we can use the same logic in DWARF that you used for BTF and in the end just ditch this table completely. - Arnaldo > long double is always 128-bit on all architectures, so relying on that > table might still break output for CTF and DWARF (but not BTF, because it > takes bitness from BTF data itself). > > But otherwise it's still good thing to complete that table. > > > > > [acme@quaco pahole]$ btfdiff libc-2.28.so.debug > > [acme@quaco pahole]$ git diff > > diff --git a/dwarves.c b/dwarves.c > > index adc411a8279d..48a018a9c354 100644 > > --- a/dwarves.c > > +++ b/dwarves.c > > @@ -168,11 +168,12 @@ static struct base_type_name_to_size { > > { .name = "double double", .size = 64, }, > > { .name = "single float", .size = 32, }, > > { .name = "float", .size = 32, }, > > - { .name = "long double", .size = 64, }, > > - { .name = "long double long double", .size = 64, }, > > + { .name = "long double", .size = sizeof(long double) * 8, }, > > + { .name = "long double long double", .size = sizeof(long double) * 8, }, > > { .name = "__int128", .size = 128, }, > > { .name = "unsigned __int128", .size = 128, }, > > { .name = "__int128 unsigned", .size = 128, }, > > + { .name = "_Float128", .size = 128, }, > > { .name = NULL }, > > }; > > > > [acme@quaco pahole]$ > > > > I'll add a series of patches fixing up the types that don't have encoded > > in its name the size in bits, so that it works on all arches, one more > > for _Float128, should be enough for now, right? > > > > - Arnaldo > > > > > $ chmod +rw /home/andriin/local/btf/libc-2.28.so.debug > > > > > > $ ~/local/pahole/build/pahole -J ~/local/btf/libc-2.28.so.debug > > > > > > $ PAHOLE=~/local/pahole/build/pahole ./btfdiff ~/local/btf/libc-2.28.so.debug > > > base_type__name_to_size: base_type _Float128 > > > class__fixup_btf_bitfields: unknown base type name "_Float128"! > > > base_type__name_to_size: base_type _Float128 > > > class__fixup_btf_bitfields: unknown base type name "_Float128"! > > > base_type__name_to_size: base_type _Float128 > > > class__fixup_btf_bitfields: unknown base type name "_Float128"! > > > base_type__name_to_size: base_type _Float128 > > > class__fixup_btf_bitfields: unknown base type name "_Float128"! > > > base_type__name_to_size: base_type _Float128 > > > class__fixup_btf_bitfields: unknown base type name "_Float128"! > > > base_type__name_to_size: base_type _Float128 > > > class__fixup_btf_bitfields: unknown base type name "_Float128"! > > > --- /tmp/btfdiff.dwarf.aV4wSL 2019-02-25 13:31:54.787923673 -0800 > > > +++ /tmp/btfdiff.btf.22NQmJ 2019-02-25 13:31:54.802923668 -0800 > > > @@ -461,9 +461,15 @@ struct La_x86_64_retval { > > > uint64_t lrv_rdx; /* 8 8 */ > > > La_x86_64_xmm lrv_xmm0; /* 16 16 */ > > > La_x86_64_xmm lrv_xmm1; /* 32 16 */ > > > - long double lrv_st0; /* 48 16 */ > > > + long double lrv_st0; /* 48 8 */ > > > + > > > + /* XXX 8 bytes hole, try to pack */ > > > + > > > /* --- cacheline 1 boundary (64 bytes) --- */ > > > - long double lrv_st1; /* 64 16 */ > > > + long double lrv_st1; /* 64 8 */ > > > + > > > + /* XXX 8 bytes hole, try to pack */ > > > + > > > La_x86_64_vector lrv_vector0; /* 80 64 */ > > > /* --- cacheline 2 boundary (128 bytes) was 16 bytes ago --- */ > > > La_x86_64_vector lrv_vector1; /* 144 64 */ > > > @@ -472,6 +478,7 @@ struct La_x86_64_retval { > > > __int128 lrv_bnd1; /* 224 16 */ > > > > > > /* size: 240, cachelines: 4, members: 10 */ > > > + /* sum members: 224, holes: 2, sum holes: 16 */ > > > /* last cacheline: 48 bytes */ > > > }; > > > struct r_debug { > > > @@ -2044,7 +2051,7 @@ union ieee754_float { > > > } ieee_nan; /* 0 4 */ > > > }; > > > union ieee854_long_double { > > > - long double d; /* 0 16 */ > > > + long double d; /* 0 8 */ > > > struct { > > > unsigned int mantissa1:32; /* 0: 0 4 */ > > > unsigned int mantissa0:32; /* 4: 0 4 */ > > > @@ -2141,7 +2148,7 @@ struct ucontext_t { > > > /* last cacheline: 8 bytes */ > > > }; > > > union ieee854_float128 { > > > - _Float128 d; /* 0 16 */ > > > + _Float128 d; /* 0 0 */ > > > struct { > > > unsigned int mantissa3:32; /* 0: 0 4 */ > > > unsigned int mantissa2:32; /* 4: 0 4 */ > > > @@ -2219,7 +2226,7 @@ union printf_arg { > > > long unsigned int pa_u_long_int; /* 0 8 */ > > > long long unsigned int pa_u_long_long_int; /* 0 8 */ > > > double pa_double; /* 0 8 */ > > > - long double pa_long_double; /* 0 16 */ > > > + long double pa_long_double; /* 0 8 */ > > > const char * pa_string; /* 0 8 */ > > > const wchar_t * pa_wstring; /* 0 8 */ > > > void * pa_pointer; /* 0 8 */ > > > > > > Seems like "long double" size is invalid for BTF. And we should > > > probably add "_Float128" as another "well known" base type name? > > > > > > Though for the latter, it probably would be better to resolve base > > > type size using BTF data itself. > > > > > > > > [acme@quaco pahole]$ > > > > -- > > > > - Arnaldo -- - Arnaldo ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2019-02-26 17:46 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-20 20:57 [PATCH pahole 0/3] fix handling of bitfields in btf_loader Andrii Nakryiko 2019-02-20 20:57 ` [PATCH pahole 1/3] btfdiff: support specifying custom pahole location Andrii Nakryiko 2019-02-20 20:57 ` [PATCH pahole 2/3] pahole: complete list of base type names Andrii Nakryiko 2019-02-20 20:57 ` [PATCH pahole 3/3] btf_loader: fix bitfield fixup code Andrii Nakryiko 2019-02-22 22:02 ` [PATCH pahole 0/3] fix handling of bitfields in btf_loader Arnaldo Carvalho de Melo 2019-02-22 23:23 ` Andrii Nakryiko 2019-02-25 16:02 ` Arnaldo Carvalho de Melo 2019-02-25 18:59 ` Arnaldo Carvalho de Melo 2019-02-25 19:42 ` Andrii Nakryiko 2019-02-25 20:08 ` encoding BTF on glibc was: " Arnaldo Carvalho de Melo 2019-02-25 20:51 ` Andrii Nakryiko 2019-02-25 21:36 ` Andrii Nakryiko 2019-02-26 0:34 ` Andrii Nakryiko 2019-02-26 5:37 ` Yonghong Song 2019-02-26 5:44 ` Alexei Starovoitov 2019-02-26 5:45 ` Andrii Nakryiko 2019-02-26 6:03 ` Yonghong Song 2019-02-26 17:22 ` Andrii Nakryiko 2019-02-26 17:24 ` Yonghong Song 2019-02-26 17:34 ` Andrii Nakryiko 2019-02-26 13:11 ` Arnaldo Carvalho de Melo 2019-02-26 17:14 ` Andrii Nakryiko 2019-02-26 17:45 ` Arnaldo Carvalho de Melo
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.