* Bad padding with bpftool btf dump .. format c
@ 2022-11-18 10:30 Per Sundström XP
2022-11-18 12:42 ` Jiri Olsa
0 siblings, 1 reply; 13+ messages in thread
From: Per Sundström XP @ 2022-11-18 10:30 UTC (permalink / raw)
To: bpf@vger.kernel.org
Hi,
I don't know if this is the channel for reporting issues with the
"bpftool dump .. format c" function.
If this is not the one, please help me find the correct one.
This bash script illustrates a problem where 'bpftool btf dump <file>
format c': produces an incorrect 'h' file.
I looked into it a bit, and the problem seem to be in the
"libbpf/btfdump.c : btf_dump_emit_bit_padding()" function.
I can dig into it more if you like, but first I want to report it as a
bug.
Regards,
/Per
---- bad_padding bash script ---
----------------------------------------------------
#
# Reproduction bash script for wrong offsets
#
cat >foo.h <<EOF
#pragma clang attribute push (__attribute__((preserve_access_index)),
apply_to = record)
struct foo {
struct {
int aa;
char ab;
} a;
long :64;
int :4;
char b;
short c;
};
#pragma clang attribute pop
EOF
cat >foo.c <<EOF
#include "foo.h"
#define offsetof(TYPE, MEMBER) ((long) &((TYPE*)0)->MEMBER)
long foo()
{
long ret = 0;
//ret += ((struct foo*)0)->a.ab;
ret += ((struct foo*)0)->b;
ret += ((struct foo*)0)->c;
return ret;
}
EOF
cat >main.c <<EOF
#include <stdio.h>
#include "foo.h"
#define offsetof(TYPE, MEMBER) ((long) &((TYPE*)0)->MEMBER)
void main(){
printf("offsetof(struct foo, c)=%ld\n", offsetof(struct foo, c));
}
EOF
# Vanilla header case
printf "============ Vanilla ==========\n"
cat foo.h | awk '/^struct foo/,/^}/'
gcc -O0 -g -I. -o main main.c; ./main
# Proudce a custom [minimized] header
CFLAGS="-I. -ggdb -gdwarf -O2 -Wall -fpie -target bpf
-D__TARGET_ARCH_x86"
clang $CFLAGS -DBOOTSTRAP -c foo.c -o foo.o
pahole --btf_encode_detached full.btf foo.o
bpftool gen min_core_btf full.btf custom.btf foo.o
bpftool btf dump file custom.btf format c > foo.h
printf "\n============ Custom ==========\n"
cat foo.h | awk '/^struct foo/,/^}/'
gcc -O0 -g -I. -o main main.c; ./main
printf "\n=== BTF offsets ===\n"
printf "full : "
/usr/sbin/bpftool btf dump file full.btf | grep "'c'"
printf "custom : "
/usr/sbin/bpftool btf dump file custom.btf | grep "'c'"
#---------------------end of script -------------------------------
Output of ./bad_padding.sh:
---
============ Vanilla ==========
struct foo {
struct {
int aa;
char ab;
} a;
long :64;
int :4;
char b;
short c;
};
offsetof(struct foo, c)=18
============ Custom ==========
struct foo {
long: 8;
long: 64;
long: 64;
char b;
short c;
};
offsetof(struct foo, c)=26
=== BTF offsets ===
full : 'c' type_id=6 bits_offset=144
custom : 'c' type_id=3 bits_offset=144
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: Bad padding with bpftool btf dump .. format c 2022-11-18 10:30 Bad padding with bpftool btf dump .. format c Per Sundström XP @ 2022-11-18 12:42 ` Jiri Olsa 2022-11-18 15:10 ` Sv: " Per Sundström XP 0 siblings, 1 reply; 13+ messages in thread From: Jiri Olsa @ 2022-11-18 12:42 UTC (permalink / raw) To: Per Sundström XP, Andrii Nakryiko; +Cc: bpf@vger.kernel.org On Fri, Nov 18, 2022 at 10:30:50AM +0000, Per Sundström XP wrote: > Hi, > > I don't know if this is the channel for reporting issues with the > "bpftool dump .. format c" function. > If this is not the one, please help me find the correct one. > > This bash script illustrates a problem where 'bpftool btf dump <file> > format c': produces an incorrect 'h' file. > I looked into it a bit, and the problem seem to be in the > "libbpf/btfdump.c : btf_dump_emit_bit_padding()" function. > > I can dig into it more if you like, but first I want to report it as a > bug. > > Regards, > /Per > > ---- bad_padding bash script --- > ---------------------------------------------------- > # > # Reproduction bash script for wrong offsets > # > cat >foo.h <<EOF > #pragma clang attribute push (__attribute__((preserve_access_index)), > apply_to = record) > struct foo { > struct { > int aa; > char ab; > } a; > long :64; > int :4; > char b; > short c; > }; > #pragma clang attribute pop > EOF > > cat >foo.c <<EOF > #include "foo.h" > > #define offsetof(TYPE, MEMBER) ((long) &((TYPE*)0)->MEMBER) > > long foo() > { > long ret = 0; > //ret += ((struct foo*)0)->a.ab; > ret += ((struct foo*)0)->b; > ret += ((struct foo*)0)->c; > return ret; > } > EOF > > cat >main.c <<EOF > #include <stdio.h> > #include "foo.h" > > #define offsetof(TYPE, MEMBER) ((long) &((TYPE*)0)->MEMBER) > > void main(){ > printf("offsetof(struct foo, c)=%ld\n", offsetof(struct foo, c)); > } > EOF > > # Vanilla header case > printf "============ Vanilla ==========\n" > cat foo.h | awk '/^struct foo/,/^}/' > gcc -O0 -g -I. -o main main.c; ./main > > # Proudce a custom [minimized] header > CFLAGS="-I. -ggdb -gdwarf -O2 -Wall -fpie -target bpf > -D__TARGET_ARCH_x86" > clang $CFLAGS -DBOOTSTRAP -c foo.c -o foo.o > pahole --btf_encode_detached full.btf foo.o > bpftool gen min_core_btf full.btf custom.btf foo.o > bpftool btf dump file custom.btf format c > foo.h > > printf "\n============ Custom ==========\n" > cat foo.h | awk '/^struct foo/,/^}/' > gcc -O0 -g -I. -o main main.c; ./main > > printf "\n=== BTF offsets ===\n" > printf "full : " > /usr/sbin/bpftool btf dump file full.btf | grep "'c'" > printf "custom : " > /usr/sbin/bpftool btf dump file custom.btf | grep "'c'" > > #---------------------end of script ------------------------------- > > > Output of ./bad_padding.sh: > --- > ============ Vanilla ========== > struct foo { > struct { > int aa; > char ab; > } a; > long :64; > int :4; > char b; > short c; > }; > offsetof(struct foo, c)=18 > > ============ Custom ========== > struct foo { > long: 8; > long: 64; > long: 64; > char b; > short c; > }; so I guess the issue is that the first 'long: 8' is padded to full long: 64 ? looks like btf_dump_emit_bit_padding did not take into accout the gap on the begining of the struct on the other hand you generated that header file from 'min_core_btf' btf data, which takes away all the unused fields.. it might not beeen considered as a use case before jirka > offsetof(struct foo, c)=26 > > === BTF offsets === > full : 'c' type_id=6 bits_offset=144 > custom : 'c' type_id=3 bits_offset=144 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Sv: Bad padding with bpftool btf dump .. format c 2022-11-18 12:42 ` Jiri Olsa @ 2022-11-18 15:10 ` Per Sundström XP 2022-11-18 17:26 ` Per Sundström XP 0 siblings, 1 reply; 13+ messages in thread From: Per Sundström XP @ 2022-11-18 15:10 UTC (permalink / raw) To: Jiri Olsa, Andrii Nakryiko; +Cc: bpf@vger.kernel.org > On Fri, Nov 18, 2022 at 10:30:50AM +0000, Per Sundström XP wrote: >> Hi, >> >> I don't know if this is the channel for reporting issues with the >> "bpftool dump .. format c" function. >> If this is not the one, please help me find the correct one. >> >> This bash script illustrates a problem where 'bpftool btf dump <file> >> format c': produces an incorrect 'h' file. >> I looked into it a bit, and the problem seem to be in the >> "libbpf/btfdump.c : btf_dump_emit_bit_padding()" function. >> >> I can dig into it more if you like, but first I want to report it as a >> bug. >> >> Regards, >> /Per >> >> ---- bad_padding bash script --- >> ---------------------------------------------------- >> # >> # Reproduction bash script for wrong offsets >> # >> cat >foo.h <<EOF >> #pragma clang attribute push (__attribute__((preserve_access_index)), >> apply_to = record) >> struct foo { >> struct { >> int aa; >> char ab; >> } a; >> long :64; >> int :4; >> char b; >> short c; >> }; >> #pragma clang attribute pop >> EOF >> >> cat >foo.c <<EOF >> #include "foo.h" >> >> #define offsetof(TYPE, MEMBER) ((long) &((TYPE*)0)->MEMBER) >> >> long foo() >> { >> long ret = 0; >> //ret += ((struct foo*)0)->a.ab; >> ret += ((struct foo*)0)->b; >> ret += ((struct foo*)0)->c; >> return ret; >> } >> EOF >> >> cat >main.c <<EOF >> #include <stdio.h> >> #include "foo.h" >> >> #define offsetof(TYPE, MEMBER) ((long) &((TYPE*)0)->MEMBER) >> >> void main(){ >> printf("offsetof(struct foo, c)=%ld\n", offsetof(struct foo, c)); >> } >> EOF >> >> # Vanilla header case >> printf "============ Vanilla ==========\n" >> cat foo.h | awk '/^struct foo/,/^}/' >> gcc -O0 -g -I. -o main main.c; ./main >> >> # Proudce a custom [minimized] header >> CFLAGS="-I. -ggdb -gdwarf -O2 -Wall -fpie -target bpf >> -D__TARGET_ARCH_x86" >> clang $CFLAGS -DBOOTSTRAP -c foo.c -o foo.o >> pahole --btf_encode_detached full.btf foo.o >> bpftool gen min_core_btf full.btf custom.btf foo.o >> bpftool btf dump file custom.btf format c > foo.h >> >> printf "\n============ Custom ==========\n" >> cat foo.h | awk '/^struct foo/,/^}/' >> gcc -O0 -g -I. -o main main.c; ./main >> >> printf "\n=== BTF offsets ===\n" >> printf "full : " >> /usr/sbin/bpftool btf dump file full.btf | grep "'c'" >> printf "custom : " >> /usr/sbin/bpftool btf dump file custom.btf | grep "'c'" >> >> #---------------------end of script ------------------------------- >> >> >> Output of ./bad_padding.sh: >> --- >> ============ Vanilla ========== >> struct foo { >> struct { >> int aa; >> char ab; >> } a; >> long :64; >> int :4; >> char b; >> short c; >> }; >> offsetof(struct foo, c)=18 >> >> ============ Custom ========== >> struct foo { >> long: 8; >> long: 64; >> long: 64; >> char b; >> short c; >> }; > > so I guess the issue is that the first 'long: 8' is padded to full long: 64 ? > > looks like btf_dump_emit_bit_padding did not take into accout the gap on the > begining of the struct > > on the other hand you generated that header file from 'min_core_btf' btf data, > which takes away all the unused fields.. it might not beeen considered as a > use case before > > jirka > That could be the case, but I think the 'emit_bit_padding()' will not really have a lot to do for the non sparse headers .. /Per > >> offsetof(struct foo, c)=26 >> >> === BTF offsets === >> full : 'c' type_id=6 bits_offset=144 >> custom : 'c' type_id=3 bits_offset=144 >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Sv: Bad padding with bpftool btf dump .. format c 2022-11-18 15:10 ` Sv: " Per Sundström XP @ 2022-11-18 17:26 ` Per Sundström XP 2022-11-24 2:37 ` Andrii Nakryiko 0 siblings, 1 reply; 13+ messages in thread From: Per Sundström XP @ 2022-11-18 17:26 UTC (permalink / raw) To: olsajiri@gmail.com, andrii.nakryiko@gmail.com; +Cc: bpf@vger.kernel.org > > ============ Vanilla ========== > > struct foo { > > struct { > > int aa; > > char ab; > > } a; > > long :64; > > int :4; > > char b; > > short c; > > }; > > offsetof(struct foo, c)=18 > > > > ============ Custom ========== > > struct foo { > > long: 8; > > long: 64; > > long: 64; > > char b; > > short c; > > }; > > so I guess the issue is that the first 'long: 8' is padded to full > long: 64 ? > > looks like btf_dump_emit_bit_padding did not take into accout the gap > on the > begining of the struct > > on the other hand you generated that header file from 'min_core_btf' > btf data, > which takes away all the unused fields.. it might not beeen > considered as a > use case before > > jirka > > That could be the case, but I think the 'emit_bit_padding()' will not > really have a > lot to do for the non sparse headers .. > /Per Looks like something like this makes tings a lot better: diff --git a/src/btf_dump.c b/src/btf_dump.c index 12f7039..a8bd52a 100644 --- a/src/btf_dump.c +++ b/src/btf_dump.c @@ -881,13 +881,13 @@ static void btf_dump_emit_bit_padding(const struct btf_dump *d, const char *pad_type; int pad_bits; - if (ptr_bits > 32 && off_diff > 32) { + if (align > 4 && ptr_bits > 32 && off_diff > 32) { pad_type = "long"; pad_bits = chip_away_bits(off_diff, ptr_bits); - } else if (off_diff > 16) { + } else if (align > 2 && off_diff > 16) { pad_type = "int"; pad_bits = chip_away_bits(off_diff, 32); - } else if (off_diff > 8) { + } else if (align > 1 && off_diff > 8) { pad_type = "short"; pad_bits = chip_away_bits(off_diff, 16); } else { /Per ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: Sv: Bad padding with bpftool btf dump .. format c 2022-11-18 17:26 ` Per Sundström XP @ 2022-11-24 2:37 ` Andrii Nakryiko 2022-11-29 17:38 ` Eduard Zingerman 0 siblings, 1 reply; 13+ messages in thread From: Andrii Nakryiko @ 2022-11-24 2:37 UTC (permalink / raw) To: Per Sundström XP; +Cc: olsajiri@gmail.com, bpf@vger.kernel.org On Fri, Nov 18, 2022 at 9:26 AM Per Sundström XP <per.xp.sundstrom@ericsson.com> wrote: > > > > > ============ Vanilla ========== > > > struct foo { > > > struct { > > > int aa; > > > char ab; > > > } a; > > > long :64; > > > int :4; > > > char b; > > > short c; > > > }; > > > offsetof(struct foo, c)=18 > > > > > > ============ Custom ========== > > > struct foo { > > > long: 8; > > > long: 64; > > > long: 64; > > > char b; > > > short c; > > > }; > > > > so I guess the issue is that the first 'long: 8' is padded to full > > long: 64 ? > > > > looks like btf_dump_emit_bit_padding did not take into accout the gap > > on the > > begining of the struct > > > > on the other hand you generated that header file from 'min_core_btf' > > btf data, > > which takes away all the unused fields.. it might not beeen > > considered as a > > use case before > > > > jirka > > > > > That could be the case, but I think the 'emit_bit_padding()' will not > > really have a > > lot to do for the non sparse headers .. > > /Per > > > Looks like something like this makes tings a lot better: yep, this helps, though changes output with padding to more verbose version, quite often unnecessarily. I need to thing a bit more on this, but the way we currently calculate alignment of a type is not always going to be correct. E.g., just because there is an int field, doesn't mean that struct actually has 4-byte alignment. We must take into account natural alignment, but also actual alignment, which might be different due to __attribute__((packed)). Either way, thanks for reporting! > > diff --git a/src/btf_dump.c b/src/btf_dump.c > index 12f7039..a8bd52a 100644 > --- a/src/btf_dump.c > +++ b/src/btf_dump.c > @@ -881,13 +881,13 @@ static void btf_dump_emit_bit_padding(const > struct btf_dump *d, > const char *pad_type; > int pad_bits; > > - if (ptr_bits > 32 && off_diff > 32) { > + if (align > 4 && ptr_bits > 32 && off_diff > 32) { > pad_type = "long"; > pad_bits = chip_away_bits(off_diff, ptr_bits); > - } else if (off_diff > 16) { > + } else if (align > 2 && off_diff > 16) { > pad_type = "int"; > pad_bits = chip_away_bits(off_diff, 32); > - } else if (off_diff > 8) { > + } else if (align > 1 && off_diff > 8) { > pad_type = "short"; > pad_bits = chip_away_bits(off_diff, 16); > } else { > /Per ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Sv: Bad padding with bpftool btf dump .. format c 2022-11-24 2:37 ` Andrii Nakryiko @ 2022-11-29 17:38 ` Eduard Zingerman 2022-11-30 0:27 ` Andrii Nakryiko 0 siblings, 1 reply; 13+ messages in thread From: Eduard Zingerman @ 2022-11-29 17:38 UTC (permalink / raw) To: Andrii Nakryiko, Per Sundström XP Cc: olsajiri@gmail.com, bpf@vger.kernel.org On Wed, 2022-11-23 at 18:37 -0800, Andrii Nakryiko wrote: > On Fri, Nov 18, 2022 at 9:26 AM Per Sundström XP > <per.xp.sundstrom@ericsson.com> wrote: > > > > > > > > ============ Vanilla ========== > > > > struct foo { > > > > struct { > > > > int aa; > > > > char ab; > > > > } a; > > > > long :64; > > > > int :4; > > > > char b; > > > > short c; > > > > }; > > > > offsetof(struct foo, c)=18 > > > > > > > > ============ Custom ========== > > > > struct foo { > > > > long: 8; > > > > long: 64; > > > > long: 64; > > > > char b; > > > > short c; > > > > }; > > > > > > so I guess the issue is that the first 'long: 8' is padded to full > > > long: 64 ? > > > > > > looks like btf_dump_emit_bit_padding did not take into accout the gap > > > on the > > > begining of the struct > > > > > > on the other hand you generated that header file from 'min_core_btf' > > > btf data, > > > which takes away all the unused fields.. it might not beeen > > > considered as a > > > use case before > > > > > > jirka > > > > > > > > That could be the case, but I think the 'emit_bit_padding()' will not > > > really have a > > > lot to do for the non sparse headers .. > > > /Per > > > > > > Looks like something like this makes tings a lot better: > > yep, this helps, though changes output with padding to more verbose > version, quite often unnecessarily. I need to thing a bit more on > this, but the way we currently calculate alignment of a type is not > always going to be correct. E.g., just because there is an int field, > doesn't mean that struct actually has 4-byte alignment. > > We must take into account natural alignment, but also actual > alignment, which might be different due to __attribute__((packed)). > > Either way, thanks for reporting! Hi everyone, I think the fix is simpler: diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c index deb2bc9a0a7b..23a00818854b 100644 --- a/tools/lib/bpf/btf_dump.c +++ b/tools/lib/bpf/btf_dump.c @@ -860,7 +860,7 @@ static bool btf_is_struct_packed(const struct btf *btf, __u32 id, static int chip_away_bits(int total, int at_most) { - return total % at_most ? : at_most; + return total > at_most ? at_most : total; } It changes the order in which btf_dump_emit_bit_padding() prints field sizes. Right now it returns the division remainder on a first call and full 'at_most' values at subsequent calls. For this particular example the bit offset of 'b' is 136, so the output looks as follows: struct foo { long: 8; // first call pad_bits = 136 % 64 ? : 64; off_diff -= 8; long: 64; // second call pad_bits = 128 % 64 ? : 64; off_diff -= 64; ... long: 64; char b; short c; }; This is incorrect, because compiler would always add padding between the first and second members to account for the second member alignment. However, my change inverts the order, which avoids the accidental padding and gets the desired output: ============ Custom ========== struct foo { long: 64; long: 64; char: 8; char b; short c; }; offsetof(struct foo, c)=18 === BTF offsets === full : 'c' type_id=6 bits_offset=144 custom : 'c' type_id=3 bits_offset=144 wdyt? > > > > > diff --git a/src/btf_dump.c b/src/btf_dump.c > > index 12f7039..a8bd52a 100644 > > --- a/src/btf_dump.c > > +++ b/src/btf_dump.c > > @@ -881,13 +881,13 @@ static void btf_dump_emit_bit_padding(const > > struct btf_dump *d, > > const char *pad_type; > > int pad_bits; > > > > - if (ptr_bits > 32 && off_diff > 32) { > > + if (align > 4 && ptr_bits > 32 && off_diff > 32) { > > pad_type = "long"; > > pad_bits = chip_away_bits(off_diff, ptr_bits); > > - } else if (off_diff > 16) { > > + } else if (align > 2 && off_diff > 16) { > > pad_type = "int"; > > pad_bits = chip_away_bits(off_diff, 32); > > - } else if (off_diff > 8) { > > + } else if (align > 1 && off_diff > 8) { > > pad_type = "short"; > > pad_bits = chip_away_bits(off_diff, 16); > > } else { > > /Per ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: Sv: Bad padding with bpftool btf dump .. format c 2022-11-29 17:38 ` Eduard Zingerman @ 2022-11-30 0:27 ` Andrii Nakryiko 2022-11-30 2:29 ` Eduard Zingerman 0 siblings, 1 reply; 13+ messages in thread From: Andrii Nakryiko @ 2022-11-30 0:27 UTC (permalink / raw) To: Eduard Zingerman Cc: Per Sundström XP, olsajiri@gmail.com, bpf@vger.kernel.org On Tue, Nov 29, 2022 at 9:38 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Wed, 2022-11-23 at 18:37 -0800, Andrii Nakryiko wrote: > > On Fri, Nov 18, 2022 at 9:26 AM Per Sundström XP > > <per.xp.sundstrom@ericsson.com> wrote: > > > > > > > > > > > ============ Vanilla ========== > > > > > struct foo { > > > > > struct { > > > > > int aa; > > > > > char ab; > > > > > } a; > > > > > long :64; > > > > > int :4; > > > > > char b; > > > > > short c; > > > > > }; > > > > > offsetof(struct foo, c)=18 > > > > > > > > > > ============ Custom ========== > > > > > struct foo { > > > > > long: 8; > > > > > long: 64; > > > > > long: 64; > > > > > char b; > > > > > short c; > > > > > }; > > > > > > > > so I guess the issue is that the first 'long: 8' is padded to full > > > > long: 64 ? > > > > > > > > looks like btf_dump_emit_bit_padding did not take into accout the gap > > > > on the > > > > begining of the struct > > > > > > > > on the other hand you generated that header file from 'min_core_btf' > > > > btf data, > > > > which takes away all the unused fields.. it might not beeen > > > > considered as a > > > > use case before > > > > > > > > jirka > > > > > > > > > > > That could be the case, but I think the 'emit_bit_padding()' will not > > > > really have a > > > > lot to do for the non sparse headers .. > > > > /Per > > > > > > > > > Looks like something like this makes tings a lot better: > > > > yep, this helps, though changes output with padding to more verbose > > version, quite often unnecessarily. I need to thing a bit more on > > this, but the way we currently calculate alignment of a type is not > > always going to be correct. E.g., just because there is an int field, > > doesn't mean that struct actually has 4-byte alignment. > > > > We must take into account natural alignment, but also actual > > alignment, which might be different due to __attribute__((packed)). > > > > Either way, thanks for reporting! > > Hi everyone, > > I think the fix is simpler: > > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c > index deb2bc9a0a7b..23a00818854b 100644 > --- a/tools/lib/bpf/btf_dump.c > +++ b/tools/lib/bpf/btf_dump.c > @@ -860,7 +860,7 @@ static bool btf_is_struct_packed(const struct btf *btf, __u32 id, > > static int chip_away_bits(int total, int at_most) > { > - return total % at_most ? : at_most; > + return total > at_most ? at_most : total; > } > > It changes the order in which btf_dump_emit_bit_padding() prints field > sizes. Right now it returns the division remainder on a first call and > full 'at_most' values at subsequent calls. For this particular example > the bit offset of 'b' is 136, so the output looks as follows: > > struct foo { > long: 8; // first call pad_bits = 136 % 64 ? : 64; off_diff -= 8; > long: 64; // second call pad_bits = 128 % 64 ? : 64; off_diff -= 64; ... > long: 64; > char b; > short c; > }; > > This is incorrect, because compiler would always add padding between > the first and second members to account for the second member alignment. > > However, my change inverts the order, which avoids the accidental > padding and gets the desired output: > > ============ Custom ========== > struct foo { > long: 64; > long: 64; > char: 8; > char b; > short c; > }; > offsetof(struct foo, c)=18 > > === BTF offsets === > full : 'c' type_id=6 bits_offset=144 > custom : 'c' type_id=3 bits_offset=144 > > wdyt? There were at least two issues I realized when I was thinking about fixing this, and I think you are missing at least one of them. 1. Adding `long :xxx` as padding makes struct at least 8-byte aligned. If the struct originally had a smaller alignment requirement, you are now potentially breaking the struct layout by changing its layout. 2. The way btf__align_of() is calculating alignment doesn't work correctly for __attribute__((packed)) structs. So we need to fix btf__align_of() first. What btf__align_of() is calculating right now is a natural alignment requirement if we ignore actual field offsets. This might be useful (at the very least to determine if the struct is packed or not), so maybe we should have a separate btf__natural_align_of() or something along those lines? And then we need to fix btf_dump_emit_bit_padding() to better handle alignment and padding rules. This is what Per Sundström is trying to do, I believe, but I haven't carefully thought about his latest code suggestion. In general, the most obvious solution would be to pad with `char :8;` everywhere, but that's very ugly and I'd prefer us to have as "natural" output as possible. That is, only emit strictly necessary padding fields and rely on natural alignment otherwise. > > > > > > > > > > diff --git a/src/btf_dump.c b/src/btf_dump.c > > > index 12f7039..a8bd52a 100644 > > > --- a/src/btf_dump.c > > > +++ b/src/btf_dump.c > > > @@ -881,13 +881,13 @@ static void btf_dump_emit_bit_padding(const > > > struct btf_dump *d, > > > const char *pad_type; > > > int pad_bits; > > > > > > - if (ptr_bits > 32 && off_diff > 32) { > > > + if (align > 4 && ptr_bits > 32 && off_diff > 32) { > > > pad_type = "long"; > > > pad_bits = chip_away_bits(off_diff, ptr_bits); > > > - } else if (off_diff > 16) { > > > + } else if (align > 2 && off_diff > 16) { > > > pad_type = "int"; > > > pad_bits = chip_away_bits(off_diff, 32); > > > - } else if (off_diff > 8) { > > > + } else if (align > 1 && off_diff > 8) { > > > pad_type = "short"; > > > pad_bits = chip_away_bits(off_diff, 16); > > > } else { > > > /Per > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Sv: Bad padding with bpftool btf dump .. format c 2022-11-30 0:27 ` Andrii Nakryiko @ 2022-11-30 2:29 ` Eduard Zingerman 2022-11-30 22:49 ` Andrii Nakryiko 0 siblings, 1 reply; 13+ messages in thread From: Eduard Zingerman @ 2022-11-30 2:29 UTC (permalink / raw) To: Andrii Nakryiko Cc: Per Sundström XP, olsajiri@gmail.com, bpf@vger.kernel.org On Tue, 2022-11-29 at 16:27 -0800, Andrii Nakryiko wrote: > On Tue, Nov 29, 2022 at 9:38 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > On Wed, 2022-11-23 at 18:37 -0800, Andrii Nakryiko wrote: > > > On Fri, Nov 18, 2022 at 9:26 AM Per Sundström XP > > > <per.xp.sundstrom@ericsson.com> wrote: > > > > > > > > > > > > > > ============ Vanilla ========== > > > > > > struct foo { > > > > > > struct { > > > > > > int aa; > > > > > > char ab; > > > > > > } a; > > > > > > long :64; > > > > > > int :4; > > > > > > char b; > > > > > > short c; > > > > > > }; > > > > > > offsetof(struct foo, c)=18 > > > > > > > > > > > > ============ Custom ========== > > > > > > struct foo { > > > > > > long: 8; > > > > > > long: 64; > > > > > > long: 64; > > > > > > char b; > > > > > > short c; > > > > > > }; > > > > > > > > > > so I guess the issue is that the first 'long: 8' is padded to full > > > > > long: 64 ? > > > > > > > > > > looks like btf_dump_emit_bit_padding did not take into accout the gap > > > > > on the > > > > > begining of the struct > > > > > > > > > > on the other hand you generated that header file from 'min_core_btf' > > > > > btf data, > > > > > which takes away all the unused fields.. it might not beeen > > > > > considered as a > > > > > use case before > > > > > > > > > > jirka > > > > > > > > > > > > > > That could be the case, but I think the 'emit_bit_padding()' will not > > > > > really have a > > > > > lot to do for the non sparse headers .. > > > > > /Per > > > > > > > > > > > > Looks like something like this makes tings a lot better: > > > > > > yep, this helps, though changes output with padding to more verbose > > > version, quite often unnecessarily. I need to thing a bit more on > > > this, but the way we currently calculate alignment of a type is not > > > always going to be correct. E.g., just because there is an int field, > > > doesn't mean that struct actually has 4-byte alignment. > > > > > > We must take into account natural alignment, but also actual > > > alignment, which might be different due to __attribute__((packed)). > > > > > > Either way, thanks for reporting! > > > > Hi everyone, > > > > I think the fix is simpler: > > > > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c > > index deb2bc9a0a7b..23a00818854b 100644 > > --- a/tools/lib/bpf/btf_dump.c > > +++ b/tools/lib/bpf/btf_dump.c > > @@ -860,7 +860,7 @@ static bool btf_is_struct_packed(const struct btf *btf, __u32 id, > > > > static int chip_away_bits(int total, int at_most) > > { > > - return total % at_most ? : at_most; > > + return total > at_most ? at_most : total; > > } > > > > It changes the order in which btf_dump_emit_bit_padding() prints field > > sizes. Right now it returns the division remainder on a first call and > > full 'at_most' values at subsequent calls. For this particular example > > the bit offset of 'b' is 136, so the output looks as follows: > > > > struct foo { > > long: 8; // first call pad_bits = 136 % 64 ? : 64; off_diff -= 8; > > long: 64; // second call pad_bits = 128 % 64 ? : 64; off_diff -= 64; ... > > long: 64; > > char b; > > short c; > > }; > > > > This is incorrect, because compiler would always add padding between > > the first and second members to account for the second member alignment. > > > > However, my change inverts the order, which avoids the accidental > > padding and gets the desired output: > > > > ============ Custom ========== > > struct foo { > > long: 64; > > long: 64; > > char: 8; > > char b; > > short c; > > }; > > offsetof(struct foo, c)=18 > > > > === BTF offsets === > > full : 'c' type_id=6 bits_offset=144 > > custom : 'c' type_id=3 bits_offset=144 > > > > wdyt? > > There were at least two issues I realized when I was thinking about > fixing this, and I think you are missing at least one of them. > > 1. Adding `long :xxx` as padding makes struct at least 8-byte aligned. > If the struct originally had a smaller alignment requirement, you are > now potentially breaking the struct layout by changing its layout. > > 2. The way btf__align_of() is calculating alignment doesn't work > correctly for __attribute__((packed)) structs. Missed these point, sorry. On the other hand isn't this information lost in the custom.btf? $ bpftool btf dump file custom.btf [1] STRUCT 'foo' size=20 vlen=2 'b' type_id=2 bits_offset=136 'c' type_id=3 bits_offset=144 [2] INT 'char' size=1 bits_offset=0 nr_bits=8 encoding=SIGNED [3] INT 'short' size=2 bits_offset=0 nr_bits=16 encoding=SIGNED This has no info that 'foo' had fields of size 'long'. It does not matter for structs used inside BTF because 'bits_offset' is specified everywhere, but would matter if STRUCT 'foo' is used as a member of a non-BTF struct. > > So we need to fix btf__align_of() first. What btf__align_of() is > calculating right now is a natural alignment requirement if we ignore > actual field offsets. This might be useful (at the very least to > determine if the struct is packed or not), so maybe we should have a > separate btf__natural_align_of() or something along those lines? > > And then we need to fix btf_dump_emit_bit_padding() to better handle > alignment and padding rules. This is what Per Sundström is trying to > do, I believe, but I haven't carefully thought about his latest code > suggestion. > > In general, the most obvious solution would be to pad with `char :8;` > everywhere, but that's very ugly and I'd prefer us to have as > "natural" output as possible. That is, only emit strictly necessary > padding fields and rely on natural alignment otherwise. > > > > > > > > > > > > > > > > diff --git a/src/btf_dump.c b/src/btf_dump.c > > > > index 12f7039..a8bd52a 100644 > > > > --- a/src/btf_dump.c > > > > +++ b/src/btf_dump.c > > > > @@ -881,13 +881,13 @@ static void btf_dump_emit_bit_padding(const > > > > struct btf_dump *d, > > > > const char *pad_type; > > > > int pad_bits; > > > > > > > > - if (ptr_bits > 32 && off_diff > 32) { > > > > + if (align > 4 && ptr_bits > 32 && off_diff > 32) { > > > > pad_type = "long"; > > > > pad_bits = chip_away_bits(off_diff, ptr_bits); > > > > - } else if (off_diff > 16) { > > > > + } else if (align > 2 && off_diff > 16) { > > > > pad_type = "int"; > > > > pad_bits = chip_away_bits(off_diff, 32); > > > > - } else if (off_diff > 8) { > > > > + } else if (align > 1 && off_diff > 8) { > > > > pad_type = "short"; > > > > pad_bits = chip_away_bits(off_diff, 16); > > > > } else { > > > > /Per > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Sv: Bad padding with bpftool btf dump .. format c 2022-11-30 2:29 ` Eduard Zingerman @ 2022-11-30 22:49 ` Andrii Nakryiko 2022-11-30 23:06 ` Eduard Zingerman 0 siblings, 1 reply; 13+ messages in thread From: Andrii Nakryiko @ 2022-11-30 22:49 UTC (permalink / raw) To: Eduard Zingerman Cc: Per Sundström XP, olsajiri@gmail.com, bpf@vger.kernel.org On Tue, Nov 29, 2022 at 6:29 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Tue, 2022-11-29 at 16:27 -0800, Andrii Nakryiko wrote: > > On Tue, Nov 29, 2022 at 9:38 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > > > On Wed, 2022-11-23 at 18:37 -0800, Andrii Nakryiko wrote: > > > > On Fri, Nov 18, 2022 at 9:26 AM Per Sundström XP > > > > <per.xp.sundstrom@ericsson.com> wrote: > > > > > > > > > > > > > > > > > ============ Vanilla ========== > > > > > > > struct foo { > > > > > > > struct { > > > > > > > int aa; > > > > > > > char ab; > > > > > > > } a; > > > > > > > long :64; > > > > > > > int :4; > > > > > > > char b; > > > > > > > short c; > > > > > > > }; > > > > > > > offsetof(struct foo, c)=18 > > > > > > > > > > > > > > ============ Custom ========== > > > > > > > struct foo { > > > > > > > long: 8; > > > > > > > long: 64; > > > > > > > long: 64; > > > > > > > char b; > > > > > > > short c; > > > > > > > }; > > > > > > > > > > > > so I guess the issue is that the first 'long: 8' is padded to full > > > > > > long: 64 ? > > > > > > > > > > > > looks like btf_dump_emit_bit_padding did not take into accout the gap > > > > > > on the > > > > > > begining of the struct > > > > > > > > > > > > on the other hand you generated that header file from 'min_core_btf' > > > > > > btf data, > > > > > > which takes away all the unused fields.. it might not beeen > > > > > > considered as a > > > > > > use case before > > > > > > > > > > > > jirka > > > > > > > > > > > > > > > > > That could be the case, but I think the 'emit_bit_padding()' will not > > > > > > really have a > > > > > > lot to do for the non sparse headers .. > > > > > > /Per > > > > > > > > > > > > > > > Looks like something like this makes tings a lot better: > > > > > > > > yep, this helps, though changes output with padding to more verbose > > > > version, quite often unnecessarily. I need to thing a bit more on > > > > this, but the way we currently calculate alignment of a type is not > > > > always going to be correct. E.g., just because there is an int field, > > > > doesn't mean that struct actually has 4-byte alignment. > > > > > > > > We must take into account natural alignment, but also actual > > > > alignment, which might be different due to __attribute__((packed)). > > > > > > > > Either way, thanks for reporting! > > > > > > Hi everyone, > > > > > > I think the fix is simpler: > > > > > > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c > > > index deb2bc9a0a7b..23a00818854b 100644 > > > --- a/tools/lib/bpf/btf_dump.c > > > +++ b/tools/lib/bpf/btf_dump.c > > > @@ -860,7 +860,7 @@ static bool btf_is_struct_packed(const struct btf *btf, __u32 id, > > > > > > static int chip_away_bits(int total, int at_most) > > > { > > > - return total % at_most ? : at_most; > > > + return total > at_most ? at_most : total; > > > } > > > > > > It changes the order in which btf_dump_emit_bit_padding() prints field > > > sizes. Right now it returns the division remainder on a first call and > > > full 'at_most' values at subsequent calls. For this particular example > > > the bit offset of 'b' is 136, so the output looks as follows: > > > > > > struct foo { > > > long: 8; // first call pad_bits = 136 % 64 ? : 64; off_diff -= 8; > > > long: 64; // second call pad_bits = 128 % 64 ? : 64; off_diff -= 64; ... > > > long: 64; > > > char b; > > > short c; > > > }; > > > > > > This is incorrect, because compiler would always add padding between > > > the first and second members to account for the second member alignment. > > > > > > However, my change inverts the order, which avoids the accidental > > > padding and gets the desired output: > > > > > > ============ Custom ========== > > > struct foo { > > > long: 64; > > > long: 64; > > > char: 8; > > > char b; > > > short c; > > > }; > > > offsetof(struct foo, c)=18 > > > > > > === BTF offsets === > > > full : 'c' type_id=6 bits_offset=144 > > > custom : 'c' type_id=3 bits_offset=144 > > > > > > wdyt? > > > > There were at least two issues I realized when I was thinking about > > fixing this, and I think you are missing at least one of them. > > > > 1. Adding `long :xxx` as padding makes struct at least 8-byte aligned. > > If the struct originally had a smaller alignment requirement, you are > > now potentially breaking the struct layout by changing its layout. > > > > 2. The way btf__align_of() is calculating alignment doesn't work > > correctly for __attribute__((packed)) structs. > > Missed these point, sorry. > On the other hand isn't this information lost in the custom.btf? > > $ bpftool btf dump file custom.btf > [1] STRUCT 'foo' size=20 vlen=2 > 'b' type_id=2 bits_offset=136 > 'c' type_id=3 bits_offset=144 > [2] INT 'char' size=1 bits_offset=0 nr_bits=8 encoding=SIGNED > [3] INT 'short' size=2 bits_offset=0 nr_bits=16 encoding=SIGNED > > This has no info that 'foo' had fields of size 'long'. It does not > matter for structs used inside BTF because 'bits_offset' is specified > everywhere, but would matter if STRUCT 'foo' is used as a member of a > non-BTF struct. Yes, the latter is important, though, right? So I think ideally we determine "maximum allowable alignment" and use that to determine what's the allowable set of padding types is. WDYT? > > > > > So we need to fix btf__align_of() first. What btf__align_of() is > > calculating right now is a natural alignment requirement if we ignore > > actual field offsets. This might be useful (at the very least to > > determine if the struct is packed or not), so maybe we should have a > > separate btf__natural_align_of() or something along those lines? > > > > And then we need to fix btf_dump_emit_bit_padding() to better handle > > alignment and padding rules. This is what Per Sundström is trying to > > do, I believe, but I haven't carefully thought about his latest code > > suggestion. > > > > In general, the most obvious solution would be to pad with `char :8;` > > everywhere, but that's very ugly and I'd prefer us to have as > > "natural" output as possible. That is, only emit strictly necessary > > padding fields and rely on natural alignment otherwise. > > > > > > > > > > > > > > > > > > > > > > diff --git a/src/btf_dump.c b/src/btf_dump.c > > > > > index 12f7039..a8bd52a 100644 > > > > > --- a/src/btf_dump.c > > > > > +++ b/src/btf_dump.c > > > > > @@ -881,13 +881,13 @@ static void btf_dump_emit_bit_padding(const > > > > > struct btf_dump *d, > > > > > const char *pad_type; > > > > > int pad_bits; > > > > > > > > > > - if (ptr_bits > 32 && off_diff > 32) { > > > > > + if (align > 4 && ptr_bits > 32 && off_diff > 32) { > > > > > pad_type = "long"; > > > > > pad_bits = chip_away_bits(off_diff, ptr_bits); > > > > > - } else if (off_diff > 16) { > > > > > + } else if (align > 2 && off_diff > 16) { > > > > > pad_type = "int"; > > > > > pad_bits = chip_away_bits(off_diff, 32); > > > > > - } else if (off_diff > 8) { > > > > > + } else if (align > 1 && off_diff > 8) { > > > > > pad_type = "short"; > > > > > pad_bits = chip_away_bits(off_diff, 16); > > > > > } else { > > > > > /Per > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Sv: Bad padding with bpftool btf dump .. format c 2022-11-30 22:49 ` Andrii Nakryiko @ 2022-11-30 23:06 ` Eduard Zingerman 2022-11-30 23:11 ` Andrii Nakryiko 0 siblings, 1 reply; 13+ messages in thread From: Eduard Zingerman @ 2022-11-30 23:06 UTC (permalink / raw) To: Andrii Nakryiko Cc: Per Sundström XP, olsajiri@gmail.com, bpf@vger.kernel.org On Wed, 2022-11-30 at 14:49 -0800, Andrii Nakryiko wrote: > On Tue, Nov 29, 2022 at 6:29 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > On Tue, 2022-11-29 at 16:27 -0800, Andrii Nakryiko wrote: > > > On Tue, Nov 29, 2022 at 9:38 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > > > > > On Wed, 2022-11-23 at 18:37 -0800, Andrii Nakryiko wrote: > > > > > On Fri, Nov 18, 2022 at 9:26 AM Per Sundström XP > > > > > <per.xp.sundstrom@ericsson.com> wrote: > > > > > > > > > > > > > > > > > > > > ============ Vanilla ========== > > > > > > > > struct foo { > > > > > > > > struct { > > > > > > > > int aa; > > > > > > > > char ab; > > > > > > > > } a; > > > > > > > > long :64; > > > > > > > > int :4; > > > > > > > > char b; > > > > > > > > short c; > > > > > > > > }; > > > > > > > > offsetof(struct foo, c)=18 > > > > > > > > > > > > > > > > ============ Custom ========== > > > > > > > > struct foo { > > > > > > > > long: 8; > > > > > > > > long: 64; > > > > > > > > long: 64; > > > > > > > > char b; > > > > > > > > short c; > > > > > > > > }; > > > > > > > > > > > > > > so I guess the issue is that the first 'long: 8' is padded to full > > > > > > > long: 64 ? > > > > > > > > > > > > > > looks like btf_dump_emit_bit_padding did not take into accout the gap > > > > > > > on the > > > > > > > begining of the struct > > > > > > > > > > > > > > on the other hand you generated that header file from 'min_core_btf' > > > > > > > btf data, > > > > > > > which takes away all the unused fields.. it might not beeen > > > > > > > considered as a > > > > > > > use case before > > > > > > > > > > > > > > jirka > > > > > > > > > > > > > > > > > > > > That could be the case, but I think the 'emit_bit_padding()' will not > > > > > > > really have a > > > > > > > lot to do for the non sparse headers .. > > > > > > > /Per > > > > > > > > > > > > > > > > > > Looks like something like this makes tings a lot better: > > > > > > > > > > yep, this helps, though changes output with padding to more verbose > > > > > version, quite often unnecessarily. I need to thing a bit more on > > > > > this, but the way we currently calculate alignment of a type is not > > > > > always going to be correct. E.g., just because there is an int field, > > > > > doesn't mean that struct actually has 4-byte alignment. > > > > > > > > > > We must take into account natural alignment, but also actual > > > > > alignment, which might be different due to __attribute__((packed)). > > > > > > > > > > Either way, thanks for reporting! > > > > > > > > Hi everyone, > > > > > > > > I think the fix is simpler: > > > > > > > > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c > > > > index deb2bc9a0a7b..23a00818854b 100644 > > > > --- a/tools/lib/bpf/btf_dump.c > > > > +++ b/tools/lib/bpf/btf_dump.c > > > > @@ -860,7 +860,7 @@ static bool btf_is_struct_packed(const struct btf *btf, __u32 id, > > > > > > > > static int chip_away_bits(int total, int at_most) > > > > { > > > > - return total % at_most ? : at_most; > > > > + return total > at_most ? at_most : total; > > > > } > > > > > > > > It changes the order in which btf_dump_emit_bit_padding() prints field > > > > sizes. Right now it returns the division remainder on a first call and > > > > full 'at_most' values at subsequent calls. For this particular example > > > > the bit offset of 'b' is 136, so the output looks as follows: > > > > > > > > struct foo { > > > > long: 8; // first call pad_bits = 136 % 64 ? : 64; off_diff -= 8; > > > > long: 64; // second call pad_bits = 128 % 64 ? : 64; off_diff -= 64; ... > > > > long: 64; > > > > char b; > > > > short c; > > > > }; > > > > > > > > This is incorrect, because compiler would always add padding between > > > > the first and second members to account for the second member alignment. > > > > > > > > However, my change inverts the order, which avoids the accidental > > > > padding and gets the desired output: > > > > > > > > ============ Custom ========== > > > > struct foo { > > > > long: 64; > > > > long: 64; > > > > char: 8; > > > > char b; > > > > short c; > > > > }; > > > > offsetof(struct foo, c)=18 > > > > > > > > === BTF offsets === > > > > full : 'c' type_id=6 bits_offset=144 > > > > custom : 'c' type_id=3 bits_offset=144 > > > > > > > > wdyt? > > > > > > There were at least two issues I realized when I was thinking about > > > fixing this, and I think you are missing at least one of them. > > > > > > 1. Adding `long :xxx` as padding makes struct at least 8-byte aligned. > > > If the struct originally had a smaller alignment requirement, you are > > > now potentially breaking the struct layout by changing its layout. > > > > > > 2. The way btf__align_of() is calculating alignment doesn't work > > > correctly for __attribute__((packed)) structs. > > > > Missed these point, sorry. > > On the other hand isn't this information lost in the custom.btf? > > > > $ bpftool btf dump file custom.btf > > [1] STRUCT 'foo' size=20 vlen=2 > > 'b' type_id=2 bits_offset=136 > > 'c' type_id=3 bits_offset=144 > > [2] INT 'char' size=1 bits_offset=0 nr_bits=8 encoding=SIGNED > > [3] INT 'short' size=2 bits_offset=0 nr_bits=16 encoding=SIGNED > > > > This has no info that 'foo' had fields of size 'long'. It does not > > matter for structs used inside BTF because 'bits_offset' is specified > > everywhere, but would matter if STRUCT 'foo' is used as a member of a > > non-BTF struct. > > Yes, the latter is important, though, right? Do you want to do anything about this at the custom BTF creation stage? E.g. leave one real member / create a synthetic member to force a specific struct alignment in the minimized version. > So I think ideally we determine "maximum allowable alignment" and use > that to determine what's the allowable set of padding types is. WDYT? Yes, I agree. I think that a change in the btf__align_of() should be minimal, just check if structure is packed and if so return 1, otherwise logic should remain unchanged, this would match what LLVM does ([1]). Also the flip of the order of chip_away_bits() should remain. [1] https://github.com/eddyz87/llvm-project/blob/main/llvm/lib/IR/DataLayout.cpp#L764 > > > > > > > > > So we need to fix btf__align_of() first. What btf__align_of() is > > > calculating right now is a natural alignment requirement if we ignore > > > actual field offsets. This might be useful (at the very least to > > > determine if the struct is packed or not), so maybe we should have a > > > separate btf__natural_align_of() or something along those lines? > > > > > > And then we need to fix btf_dump_emit_bit_padding() to better handle > > > alignment and padding rules. This is what Per Sundström is trying to > > > do, I believe, but I haven't carefully thought about his latest code > > > suggestion. > > > > > > In general, the most obvious solution would be to pad with `char :8;` > > > everywhere, but that's very ugly and I'd prefer us to have as > > > "natural" output as possible. That is, only emit strictly necessary > > > padding fields and rely on natural alignment otherwise. > > > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/src/btf_dump.c b/src/btf_dump.c > > > > > > index 12f7039..a8bd52a 100644 > > > > > > --- a/src/btf_dump.c > > > > > > +++ b/src/btf_dump.c > > > > > > @@ -881,13 +881,13 @@ static void btf_dump_emit_bit_padding(const > > > > > > struct btf_dump *d, > > > > > > const char *pad_type; > > > > > > int pad_bits; > > > > > > > > > > > > - if (ptr_bits > 32 && off_diff > 32) { > > > > > > + if (align > 4 && ptr_bits > 32 && off_diff > 32) { > > > > > > pad_type = "long"; > > > > > > pad_bits = chip_away_bits(off_diff, ptr_bits); > > > > > > - } else if (off_diff > 16) { > > > > > > + } else if (align > 2 && off_diff > 16) { > > > > > > pad_type = "int"; > > > > > > pad_bits = chip_away_bits(off_diff, 32); > > > > > > - } else if (off_diff > 8) { > > > > > > + } else if (align > 1 && off_diff > 8) { > > > > > > pad_type = "short"; > > > > > > pad_bits = chip_away_bits(off_diff, 16); > > > > > > } else { > > > > > > /Per > > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Sv: Bad padding with bpftool btf dump .. format c 2022-11-30 23:06 ` Eduard Zingerman @ 2022-11-30 23:11 ` Andrii Nakryiko 2022-12-05 13:54 ` Per Sundström XP 0 siblings, 1 reply; 13+ messages in thread From: Andrii Nakryiko @ 2022-11-30 23:11 UTC (permalink / raw) To: Eduard Zingerman Cc: Per Sundström XP, olsajiri@gmail.com, bpf@vger.kernel.org On Wed, Nov 30, 2022 at 3:06 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Wed, 2022-11-30 at 14:49 -0800, Andrii Nakryiko wrote: > > On Tue, Nov 29, 2022 at 6:29 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > > > On Tue, 2022-11-29 at 16:27 -0800, Andrii Nakryiko wrote: > > > > On Tue, Nov 29, 2022 at 9:38 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > > > > > > > On Wed, 2022-11-23 at 18:37 -0800, Andrii Nakryiko wrote: > > > > > > On Fri, Nov 18, 2022 at 9:26 AM Per Sundström XP > > > > > > <per.xp.sundstrom@ericsson.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > ============ Vanilla ========== > > > > > > > > > struct foo { > > > > > > > > > struct { > > > > > > > > > int aa; > > > > > > > > > char ab; > > > > > > > > > } a; > > > > > > > > > long :64; > > > > > > > > > int :4; > > > > > > > > > char b; > > > > > > > > > short c; > > > > > > > > > }; > > > > > > > > > offsetof(struct foo, c)=18 > > > > > > > > > > > > > > > > > > ============ Custom ========== > > > > > > > > > struct foo { > > > > > > > > > long: 8; > > > > > > > > > long: 64; > > > > > > > > > long: 64; > > > > > > > > > char b; > > > > > > > > > short c; > > > > > > > > > }; > > > > > > > > > > > > > > > > so I guess the issue is that the first 'long: 8' is padded to full > > > > > > > > long: 64 ? > > > > > > > > > > > > > > > > looks like btf_dump_emit_bit_padding did not take into accout the gap > > > > > > > > on the > > > > > > > > begining of the struct > > > > > > > > > > > > > > > > on the other hand you generated that header file from 'min_core_btf' > > > > > > > > btf data, > > > > > > > > which takes away all the unused fields.. it might not beeen > > > > > > > > considered as a > > > > > > > > use case before > > > > > > > > > > > > > > > > jirka > > > > > > > > > > > > > > > > > > > > > > > That could be the case, but I think the 'emit_bit_padding()' will not > > > > > > > > really have a > > > > > > > > lot to do for the non sparse headers .. > > > > > > > > /Per > > > > > > > > > > > > > > > > > > > > > Looks like something like this makes tings a lot better: > > > > > > > > > > > > yep, this helps, though changes output with padding to more verbose > > > > > > version, quite often unnecessarily. I need to thing a bit more on > > > > > > this, but the way we currently calculate alignment of a type is not > > > > > > always going to be correct. E.g., just because there is an int field, > > > > > > doesn't mean that struct actually has 4-byte alignment. > > > > > > > > > > > > We must take into account natural alignment, but also actual > > > > > > alignment, which might be different due to __attribute__((packed)). > > > > > > > > > > > > Either way, thanks for reporting! > > > > > > > > > > Hi everyone, > > > > > > > > > > I think the fix is simpler: > > > > > > > > > > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c > > > > > index deb2bc9a0a7b..23a00818854b 100644 > > > > > --- a/tools/lib/bpf/btf_dump.c > > > > > +++ b/tools/lib/bpf/btf_dump.c > > > > > @@ -860,7 +860,7 @@ static bool btf_is_struct_packed(const struct btf *btf, __u32 id, > > > > > > > > > > static int chip_away_bits(int total, int at_most) > > > > > { > > > > > - return total % at_most ? : at_most; > > > > > + return total > at_most ? at_most : total; > > > > > } > > > > > > > > > > It changes the order in which btf_dump_emit_bit_padding() prints field > > > > > sizes. Right now it returns the division remainder on a first call and > > > > > full 'at_most' values at subsequent calls. For this particular example > > > > > the bit offset of 'b' is 136, so the output looks as follows: > > > > > > > > > > struct foo { > > > > > long: 8; // first call pad_bits = 136 % 64 ? : 64; off_diff -= 8; > > > > > long: 64; // second call pad_bits = 128 % 64 ? : 64; off_diff -= 64; ... > > > > > long: 64; > > > > > char b; > > > > > short c; > > > > > }; > > > > > > > > > > This is incorrect, because compiler would always add padding between > > > > > the first and second members to account for the second member alignment. > > > > > > > > > > However, my change inverts the order, which avoids the accidental > > > > > padding and gets the desired output: > > > > > > > > > > ============ Custom ========== > > > > > struct foo { > > > > > long: 64; > > > > > long: 64; > > > > > char: 8; > > > > > char b; > > > > > short c; > > > > > }; > > > > > offsetof(struct foo, c)=18 > > > > > > > > > > === BTF offsets === > > > > > full : 'c' type_id=6 bits_offset=144 > > > > > custom : 'c' type_id=3 bits_offset=144 > > > > > > > > > > wdyt? > > > > > > > > There were at least two issues I realized when I was thinking about > > > > fixing this, and I think you are missing at least one of them. > > > > > > > > 1. Adding `long :xxx` as padding makes struct at least 8-byte aligned. > > > > If the struct originally had a smaller alignment requirement, you are > > > > now potentially breaking the struct layout by changing its layout. > > > > > > > > 2. The way btf__align_of() is calculating alignment doesn't work > > > > correctly for __attribute__((packed)) structs. > > > > > > Missed these point, sorry. > > > On the other hand isn't this information lost in the custom.btf? > > > > > > $ bpftool btf dump file custom.btf > > > [1] STRUCT 'foo' size=20 vlen=2 > > > 'b' type_id=2 bits_offset=136 > > > 'c' type_id=3 bits_offset=144 > > > [2] INT 'char' size=1 bits_offset=0 nr_bits=8 encoding=SIGNED > > > [3] INT 'short' size=2 bits_offset=0 nr_bits=16 encoding=SIGNED > > > > > > This has no info that 'foo' had fields of size 'long'. It does not > > > matter for structs used inside BTF because 'bits_offset' is specified > > > everywhere, but would matter if STRUCT 'foo' is used as a member of a > > > non-BTF struct. > > > > Yes, the latter is important, though, right? > > Do you want to do anything about this at the custom BTF creation stage? No, absolutely not. We just need to teach btf_dump.c to not introduce any new alignment requirements while taking advantage of existing ones. We can derive enough information from BTF to achieve this. > E.g. leave one real member / create a synthetic member to force a specific > struct alignment in the minimized version. > > > So I think ideally we determine "maximum allowable alignment" and use > > that to determine what's the allowable set of padding types is. WDYT? > > Yes, I agree. > I think that a change in the btf__align_of() should be minimal, just check > if structure is packed and if so return 1, otherwise logic should remain > unchanged, this would match what LLVM does ([1]). > Also the flip of the order of chip_away_bits() should remain. Let's come up with a few tricky examples trying to break existing logic and then fix it. I suspect just chip_away_bits() changes are not sufficient. > > [1] https://github.com/eddyz87/llvm-project/blob/main/llvm/lib/IR/DataLayout.cpp#L764 > > > > > > > > > > > > > So we need to fix btf__align_of() first. What btf__align_of() is > > > > calculating right now is a natural alignment requirement if we ignore > > > > actual field offsets. This might be useful (at the very least to > > > > determine if the struct is packed or not), so maybe we should have a > > > > separate btf__natural_align_of() or something along those lines? > > > > > > > > And then we need to fix btf_dump_emit_bit_padding() to better handle > > > > alignment and padding rules. This is what Per Sundström is trying to > > > > do, I believe, but I haven't carefully thought about his latest code > > > > suggestion. > > > > > > > > In general, the most obvious solution would be to pad with `char :8;` > > > > everywhere, but that's very ugly and I'd prefer us to have as > > > > "natural" output as possible. That is, only emit strictly necessary > > > > padding fields and rely on natural alignment otherwise. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/src/btf_dump.c b/src/btf_dump.c > > > > > > > index 12f7039..a8bd52a 100644 > > > > > > > --- a/src/btf_dump.c > > > > > > > +++ b/src/btf_dump.c > > > > > > > @@ -881,13 +881,13 @@ static void btf_dump_emit_bit_padding(const > > > > > > > struct btf_dump *d, > > > > > > > const char *pad_type; > > > > > > > int pad_bits; > > > > > > > > > > > > > > - if (ptr_bits > 32 && off_diff > 32) { > > > > > > > + if (align > 4 && ptr_bits > 32 && off_diff > 32) { > > > > > > > pad_type = "long"; > > > > > > > pad_bits = chip_away_bits(off_diff, ptr_bits); > > > > > > > - } else if (off_diff > 16) { > > > > > > > + } else if (align > 2 && off_diff > 16) { > > > > > > > pad_type = "int"; > > > > > > > pad_bits = chip_away_bits(off_diff, 32); > > > > > > > - } else if (off_diff > 8) { > > > > > > > + } else if (align > 1 && off_diff > 8) { > > > > > > > pad_type = "short"; > > > > > > > pad_bits = chip_away_bits(off_diff, 16); > > > > > > > } else { > > > > > > > /Per > > > > > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Sv: Bad padding with bpftool btf dump .. format c 2022-11-30 23:11 ` Andrii Nakryiko @ 2022-12-05 13:54 ` Per Sundström XP 2022-12-08 19:04 ` Andrii Nakryiko 0 siblings, 1 reply; 13+ messages in thread From: Per Sundström XP @ 2022-12-05 13:54 UTC (permalink / raw) To: eddyz87@gmail.com, andrii.nakryiko@gmail.com Cc: olsajiri@gmail.com, bpf@vger.kernel.org On Wed, 2022-11-30 at 15:11 -0800, Andrii Nakryiko wrote: > On Wed, Nov 30, 2022 at 3:06 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Wed, 2022-11-30 at 14:49 -0800, Andrii Nakryiko wrote: > > > On Tue, Nov 29, 2022 at 6:29 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > On Tue, 2022-11-29 at 16:27 -0800, Andrii Nakryiko wrote: > > > > > On Tue, Nov 29, 2022 at 9:38 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > > > On Wed, 2022-11-23 at 18:37 -0800, Andrii Nakryiko wrote: > > > > > > > On Fri, Nov 18, 2022 at 9:26 AM Per Sundström XP > > > > > > > <per.xp.sundstrom@ericsson.com> wrote: > > > > > > > > > > > > > > > > > > ============ Vanilla ========== > > > > > > > > > > struct foo { > > > > > > > > > > struct { > > > > > > > > > > int aa; > > > > > > > > > > char ab; > > > > > > > > > > } a; > > > > > > > > > > long :64; > > > > > > > > > > int :4; > > > > > > > > > > char b; > > > > > > > > > > short c; > > > > > > > > > > }; > > > > > > > > > > offsetof(struct foo, c)=18 > > > > > > > > > > > > > > > > > > > > ============ Custom ========== > > > > > > > > > > struct foo { > > > > > > > > > > long: 8; > > > > > > > > > > long: 64; > > > > > > > > > > long: 64; > > > > > > > > > > char b; > > > > > > > > > > short c; > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > so I guess the issue is that the first 'long: 8' is padded to full > > > > > > > > > long: 64 ? > > > > > > > > > > > > > > > > > > looks like btf_dump_emit_bit_padding did not take into accout the gap > > > > > > > > > on the > > > > > > > > > begining of the struct > > > > > > > > > > > > > > > > > > on the other hand you generated that header file from 'min_core_btf' > > > > > > > > > btf data, > > > > > > > > > which takes away all the unused fields.. it might not beeen > > > > > > > > > considered as a > > > > > > > > > use case before > > > > > > > > > > > > > > > > > > jirka > > > > > > > > > > > > > > > > > > That could be the case, but I think the 'emit_bit_padding()' will not > > > > > > > > > really have a > > > > > > > > > lot to do for the non sparse headers .. > > > > > > > > > /Per > > > > > > > > > > > > > > > > Looks like something like this makes tings a lot better: > > > > > > > > > > > > > > yep, this helps, though changes output with padding to more verbose > > > > > > > version, quite often unnecessarily. I need to thing a bit more on > > > > > > > this, but the way we currently calculate alignment of a type is not > > > > > > > always going to be correct. E.g., just because there is an int field, > > > > > > > doesn't mean that struct actually has 4-byte alignment. > > > > > > > > > > > > > > We must take into account natural alignment, but also actual > > > > > > > alignment, which might be different due to __attribute__((packed)). > > > > > > > > > > > > > > Either way, thanks for reporting! > > > > > > > > > > > > Hi everyone, > > > > > > > > > > > > I think the fix is simpler: > > > > > > > > > > > > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c > > > > > > index deb2bc9a0a7b..23a00818854b 100644 > > > > > > --- a/tools/lib/bpf/btf_dump.c > > > > > > +++ b/tools/lib/bpf/btf_dump.c > > > > > > @@ -860,7 +860,7 @@ static bool btf_is_struct_packed(const struct btf *btf, __u32 id, > > > > > > > > > > > > static int chip_away_bits(int total, int at_most) > > > > > > { > > > > > > - return total % at_most ? : at_most; > > > > > > + return total > at_most ? at_most : total; > > > > > > } > > > > > > > > > > > > It changes the order in which btf_dump_emit_bit_padding() prints field > > > > > > sizes. Right now it returns the division remainder on a first call and > > > > > > full 'at_most' values at subsequent calls. For this particular example > > > > > > the bit offset of 'b' is 136, so the output looks as follows: > > > > > > > > > > > > struct foo { > > > > > > long: 8; // first call pad_bits = 136 % 64 ? : 64; off_diff -= 8; > > > > > > long: 64; // second call pad_bits = 128 % 64 ? : 64; off_diff -= 64; ... > > > > > > long: 64; > > > > > > char b; > > > > > > short c; > > > > > > }; > > > > > > > > > > > > This is incorrect, because compiler would always add padding between > > > > > > the first and second members to account for the second member alignment. > > > > > > > > > > > > However, my change inverts the order, which avoids the accidental > > > > > > padding and gets the desired output: > > > > > > > > > > > > ============ Custom ========== > > > > > > struct foo { > > > > > > long: 64; > > > > > > long: 64; > > > > > > char: 8; > > > > > > char b; > > > > > > short c; > > > > > > }; > > > > > > offsetof(struct foo, c)=18 > > > > > > > > > > > > === BTF offsets === > > > > > > full : 'c' type_id=6 bits_offset=144 > > > > > > custom : 'c' type_id=3 bits_offset=144 > > > > > > > > > > > > wdyt? > > > > > > > > > > There were at least two issues I realized when I was thinking about > > > > > fixing this, and I think you are missing at least one of them. > > > > > > > > > > 1. Adding `long :xxx` as padding makes struct at least 8-byte aligned. > > > > > If the struct originally had a smaller alignment requirement, you are > > > > > now potentially breaking the struct layout by changing its layout. > > > > > > > > > > 2. The way btf__align_of() is calculating alignment doesn't work > > > > > correctly for __attribute__((packed)) structs. > > > > > > > > Missed these point, sorry. > > > > On the other hand isn't this information lost in the custom.btf? > > > > > > > > $ bpftool btf dump file custom.btf > > > > [1] STRUCT 'foo' size=20 vlen=2 > > > > 'b' type_id=2 bits_offset=136 > > > > 'c' type_id=3 bits_offset=144 > > > > [2] INT 'char' size=1 bits_offset=0 nr_bits=8 encoding=SIGNED > > > > [3] INT 'short' size=2 bits_offset=0 nr_bits=16 encoding=SIGNED > > > > > > > > This has no info that 'foo' had fields of size 'long'. It does not > > > > matter for structs used inside BTF because 'bits_offset' is specified > > > > everywhere, but would matter if STRUCT 'foo' is used as a member of a > > > > non-BTF struct. > > > > > > Yes, the latter is important, though, right? > > > > Do you want to do anything about this at the custom BTF creation stage? > > No, absolutely not. We just need to teach btf_dump.c to not introduce > any new alignment requirements while taking advantage of existing > ones. We can derive enough information from BTF to achieve this. > > > E.g. leave one real member / create a synthetic member to force a specific > > struct alignment in the minimized version. > > > > > So I think ideally we determine "maximum allowable alignment" and use > > > that to determine what's the allowable set of padding types is. WDYT? > > > > Yes, I agree. > > I think that a change in the btf__align_of() should be minimal, just check > > if structure is packed and if so return 1, otherwise logic should remain > > unchanged, this would match what LLVM does ([1]). > > Also the flip of the order of chip_away_bits() should remain. > > Let's come up with a few tricky examples trying to break existing > logic and then fix it. I suspect just chip_away_bits() changes are not > sufficient. I have been using this python script to produce code that verifies offsets for struct members for some various kernel 'btf's. It compares the offsets from 'bits_offset' generated with 'bpftool btf dump <file>' (without 'format c') and the offsets computed by 'gcc' from header files generated with 'format c'. Use as: './verify_header_offsets.py <path to btf>' It will by default skip 50% of the members to make it harder for bpftool to produce correct offsets (can be changed with environment variable "RANDOM_SKIP_MEMBERS=<value between 0.0 and 1.0>" 'clang' does not play well with these big files, so I need to divide the generated files into batches. Default is 1000 structs. (can be controlled with environment variable 'MAX_STRUCTS') /Per ---------- verify_header_offsets.py ------------ #!/usr/bin/env python3 import os import sys import time import random import tempfile import subprocess as sp structs_count = {} n_files = 0 done = False class AppError(Exception): """ Class used for application generated exceptions """ pass def shell_cmd(command, **kwargs): print(f"COMMAND: {command}") res = sp.run(["bash", "-c", command], **kwargs) if res.returncode != 0: raise AppError(f'shell command "{command} failed') return res def find_struct_members(btf_file): struct_list = [] members = [] found = False name = "" res = shell_cmd(f"bpftool btf dump file {btf_file}", universal_newlines=True, stdout=sp.PIPE) for line in res.stdout.splitlines(): #print(line, flush=True) if found and line.startswith("\t"): member = line.split()[0].replace("'", "") # Get "Error: error recording relocations for <file>.o: Invalid argument" in 'bpftool' # for some structs. Skip for now if member in ['(anon)', 'context', 'inflate_state', 'dma_fence_array', 'net_generic']: continue bit_offset = int(line.split()[2].replace("bits_offset=", "")) bitfield_size = int(line.split()[-1].replace("bitfield_size=", "")) if line.find("bitfield_size") > 0 else 0 if random.random() > float(os.environ.get('RANDOM_SKIP_MEMBERS', '0.5')): members.append((member, bitfield_size, bit_offset)) else: # flag skipped members with (0,0) so we can log them later members.append((member, 0, 0)) elif found: found = False struct_list.append((name, members)) name = "" members = [] if line.find(" STRUCT ") > 0: name = line.split()[2].replace("'", "") if name in ["(anon)"]: continue structs_count[name] = structs_count.get(name, 0) + 1 found = True # Due to limitations in "clang" we need to split the # verification into batches split_n_structs = int(os.environ.get('MAX_STRUCTS', 1000)) structs = [] batches = [] for n, struct in enumerate(struct_list): if n and n % split_n_structs == 0: batches.append(structs) structs = [] structs.append(struct) batches.append(structs) return batches def generate_header(dir, btf_file): shell_cmd(f"bpftool btf dump file {btf_file} format c > {dir}/test.h") def generate_verification_code(dir, btf_file, struct_batch): code = "" main_body = "" code += f'#include "test_{n_files}.h"\n' code += 'int printf(const char *format, ...);\n' code += 'int sprintf(char *str, const char *format, ...);\n' code += '#define offsetof(TYPE, MEMBER) ((long) &((TYPE*)0)->MEMBER)\n' for name, members in struct_batch: if structs_count[name] > 1: # structs seen more than one time will be called 'struct foo___<n>' in # the generated header file. Only that the '<n>' seems arbitrary, so skip # them for now continue if name in ['context']: # for some reason, there are missing structs in the generated header file # skip them continue code += f"int __ref_func_struct_{name}() {{\n" code += f" int ret = 0;\n" code += f" char data[100];\n" for member, bitfield_size, bit_offset in members: if bitfield_size: code += f' ret += ((struct {name}*)&data)->{member}; /* bit_offset={bit_offset}, bitfield_size={bitfield_size} */\n' else: if bitfield_size == 0 and bit_offset == 0: # Skip verifying non bitfield member at offset 0, will always be correct code += f' /* ret += offsetof(struct {name}, {member}); Skipped */\n' continue code += f' ret += offsetof(struct {name}, {member});\n' main_body += f' offset = offsetof(struct {name}, {member});\n' main_body += f' sprintf(line, "offsetof(struct {name}, {member}) = %d", offset);\n' main_body += f' printf("%-80.80s %s\\n", line, offset == {int(bit_offset/8)} ? "OK" : "Not OK (should be {int(bit_offset/8)})");\n' code += " return ret;\n" code += "}\n" code += 'int main() {\n' code += '#ifdef VERIFY\n' code += ' char line[200];\n' code += ' int offset = 0;\n' code += ' int dummy = 0;\n' code += main_body code += '#endif\n' code += ' return 0;\n' code += '}\n' with open(f"{dir}/test_{n_files}.c", "w") as f: f.write(code) def compile_btf_object(dir, btf_file): shell_cmd(f"cp {dir}/test.h {dir}/test_{n_files}.h") shell_cmd(f"clang -c -I{dir} -ggdb -gdwarf -fpie -target bpf -D__TARGET_ARCH_x86 -o {dir}/test_{n_files}.o {dir}/test_{n_files}.c") shell_cmd(f"bpftool gen min_core_btf {btf_file} {dir}/test.btf {dir}/test_{n_files}.o") shell_cmd(f"bpftool btf dump file {dir}/test.btf format c > {dir}/test_{n_files}.h") def compile_and_run_verification(dir): shell_cmd(f"gcc -DVERIFY -I{dir} -o {dir}/test_{n_files} {dir}/test_{n_files}.c") shell_cmd(f"{dir}/test_{n_files}") def main(): global n_files global done if len(sys.argv) > 1 and os.path.exists(sys.argv[1]): btf_file = sys.argv[1] print(f"Verifying btf file {btf_file}", flush=True) #with tempfile.TemporaryDirectory() as dir: dir="/tmp" try: generate_header(dir, btf_file) for batch in find_struct_members(btf_file): generate_verification_code(dir, btf_file, batch) try: compile_btf_object(dir, btf_file) except AppError as fault: print(f"Error: {fault}", file=sys.stderr) print(f".. ignore ..", file=sys.stderr) continue compile_and_run_verification(dir) if done: break n_files += 1 except AppError as fault: print(f"Error: {fault}", file=sys.stderr) sys.exit(1) if __name__ == "__main__": main() > > > [1] > > https://protect2.fireeye.com/v1/url?k=31323334-501cfaf3-313273af-454445554331-e6381a6a39d24e8d&q=1&e=50f6402e-fdb7-4512-8c16-8ce450e943f7&u=https%3A%2F%2Fgithub.com%2Feddyz87%2Fllvm-project%2Fblob%2Fmain%2Fllvm%2Flib%2FIR%2FDataLayout.cpp%23L764 > > > > > So we need to fix btf__align_of() first. What btf__align_of() is > > > > > calculating right now is a natural alignment requirement if we ignore > > > > > actual field offsets. This might be useful (at the very least to > > > > > determine if the struct is packed or not), so maybe we should have a > > > > > separate btf__natural_align_of() or something along those lines? > > > > > > > > > > And then we need to fix btf_dump_emit_bit_padding() to better handle > > > > > alignment and padding rules. This is what Per Sundström is trying to > > > > > do, I believe, but I haven't carefully thought about his latest code > > > > > suggestion. > > > > > > > > > > In general, the most obvious solution would be to pad with `char :8;` > > > > > everywhere, but that's very ugly and I'd prefer us to have as > > > > > "natural" output as possible. That is, only emit strictly necessary > > > > > padding fields and rely on natural alignment otherwise. > > > > > > > > > > > > > > > > > > > diff --git a/src/btf_dump.c b/src/btf_dump.c > > > > > > > > index 12f7039..a8bd52a 100644 > > > > > > > > --- a/src/btf_dump.c > > > > > > > > +++ b/src/btf_dump.c > > > > > > > > @@ -881,13 +881,13 @@ static void btf_dump_emit_bit_padding(const > > > > > > > > struct btf_dump *d, > > > > > > > > const char *pad_type; > > > > > > > > int pad_bits; > > > > > > > > > > > > > > > > - if (ptr_bits > 32 && off_diff > 32) { > > > > > > > > + if (align > 4 && ptr_bits > 32 && off_diff > 32) { > > > > > > > > pad_type = "long"; > > > > > > > > pad_bits = chip_away_bits(off_diff, ptr_bits); > > > > > > > > - } else if (off_diff > 16) { > > > > > > > > + } else if (align > 2 && off_diff > 16) { > > > > > > > > pad_type = "int"; > > > > > > > > pad_bits = chip_away_bits(off_diff, 32); > > > > > > > > - } else if (off_diff > 8) { > > > > > > > > + } else if (align > 1 && off_diff > 8) { > > > > > > > > pad_type = "short"; > > > > > > > > pad_bits = chip_away_bits(off_diff, 16); > > > > > > > > } else { > > > > > > > > /Per ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Sv: Bad padding with bpftool btf dump .. format c 2022-12-05 13:54 ` Per Sundström XP @ 2022-12-08 19:04 ` Andrii Nakryiko 0 siblings, 0 replies; 13+ messages in thread From: Andrii Nakryiko @ 2022-12-08 19:04 UTC (permalink / raw) To: Per Sundström XP Cc: eddyz87@gmail.com, olsajiri@gmail.com, bpf@vger.kernel.org On Mon, Dec 5, 2022 at 5:54 AM Per Sundström XP <per.xp.sundstrom@ericsson.com> wrote: > > On Wed, 2022-11-30 at 15:11 -0800, Andrii Nakryiko wrote: > > On Wed, Nov 30, 2022 at 3:06 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > On Wed, 2022-11-30 at 14:49 -0800, Andrii Nakryiko wrote: > > > > On Tue, Nov 29, 2022 at 6:29 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > > On Tue, 2022-11-29 at 16:27 -0800, Andrii Nakryiko wrote: > > > > > > On Tue, Nov 29, 2022 at 9:38 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > > > > On Wed, 2022-11-23 at 18:37 -0800, Andrii Nakryiko wrote: > > > > > > > > On Fri, Nov 18, 2022 at 9:26 AM Per Sundström XP > > > > > > > > <per.xp.sundstrom@ericsson.com> wrote: > > > > > > > > > > > > > > > > > > > > ============ Vanilla ========== > > > > > > > > > > > struct foo { > > > > > > > > > > > struct { > > > > > > > > > > > int aa; > > > > > > > > > > > char ab; > > > > > > > > > > > } a; > > > > > > > > > > > long :64; > > > > > > > > > > > int :4; > > > > > > > > > > > char b; > > > > > > > > > > > short c; > > > > > > > > > > > }; > > > > > > > > > > > offsetof(struct foo, c)=18 > > > > > > > > > > > > > > > > > > > > > > ============ Custom ========== > > > > > > > > > > > struct foo { > > > > > > > > > > > long: 8; > > > > > > > > > > > long: 64; > > > > > > > > > > > long: 64; > > > > > > > > > > > char b; > > > > > > > > > > > short c; > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > so I guess the issue is that the first 'long: 8' is padded to full > > > > > > > > > > long: 64 ? > > > > > > > > > > > > > > > > > > > > looks like btf_dump_emit_bit_padding did not take into accout the gap > > > > > > > > > > on the > > > > > > > > > > begining of the struct > > > > > > > > > > > > > > > > > > > > on the other hand you generated that header file from 'min_core_btf' > > > > > > > > > > btf data, > > > > > > > > > > which takes away all the unused fields.. it might not beeen > > > > > > > > > > considered as a > > > > > > > > > > use case before > > > > > > > > > > > > > > > > > > > > jirka > > > > > > > > > > > > > > > > > > > > That could be the case, but I think the 'emit_bit_padding()' will not > > > > > > > > > > really have a > > > > > > > > > > lot to do for the non sparse headers .. > > > > > > > > > > /Per > > > > > > > > > > > > > > > > > > Looks like something like this makes tings a lot better: > > > > > > > > > > > > > > > > yep, this helps, though changes output with padding to more verbose > > > > > > > > version, quite often unnecessarily. I need to thing a bit more on > > > > > > > > this, but the way we currently calculate alignment of a type is not > > > > > > > > always going to be correct. E.g., just because there is an int field, > > > > > > > > doesn't mean that struct actually has 4-byte alignment. > > > > > > > > > > > > > > > > We must take into account natural alignment, but also actual > > > > > > > > alignment, which might be different due to __attribute__((packed)). > > > > > > > > > > > > > > > > Either way, thanks for reporting! > > > > > > > > > > > > > > Hi everyone, > > > > > > > > > > > > > > I think the fix is simpler: > > > > > > > > > > > > > > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c > > > > > > > index deb2bc9a0a7b..23a00818854b 100644 > > > > > > > --- a/tools/lib/bpf/btf_dump.c > > > > > > > +++ b/tools/lib/bpf/btf_dump.c > > > > > > > @@ -860,7 +860,7 @@ static bool btf_is_struct_packed(const struct btf *btf, __u32 id, > > > > > > > > > > > > > > static int chip_away_bits(int total, int at_most) > > > > > > > { > > > > > > > - return total % at_most ? : at_most; > > > > > > > + return total > at_most ? at_most : total; > > > > > > > } > > > > > > > > > > > > > > It changes the order in which btf_dump_emit_bit_padding() prints field > > > > > > > sizes. Right now it returns the division remainder on a first call and > > > > > > > full 'at_most' values at subsequent calls. For this particular example > > > > > > > the bit offset of 'b' is 136, so the output looks as follows: > > > > > > > > > > > > > > struct foo { > > > > > > > long: 8; // first call pad_bits = 136 % 64 ? : 64; off_diff -= 8; > > > > > > > long: 64; // second call pad_bits = 128 % 64 ? : 64; off_diff -= 64; ... > > > > > > > long: 64; > > > > > > > char b; > > > > > > > short c; > > > > > > > }; > > > > > > > > > > > > > > This is incorrect, because compiler would always add padding between > > > > > > > the first and second members to account for the second member alignment. > > > > > > > > > > > > > > However, my change inverts the order, which avoids the accidental > > > > > > > padding and gets the desired output: > > > > > > > > > > > > > > ============ Custom ========== > > > > > > > struct foo { > > > > > > > long: 64; > > > > > > > long: 64; > > > > > > > char: 8; > > > > > > > char b; > > > > > > > short c; > > > > > > > }; > > > > > > > offsetof(struct foo, c)=18 > > > > > > > > > > > > > > === BTF offsets === > > > > > > > full : 'c' type_id=6 bits_offset=144 > > > > > > > custom : 'c' type_id=3 bits_offset=144 > > > > > > > > > > > > > > wdyt? > > > > > > > > > > > > There were at least two issues I realized when I was thinking about > > > > > > fixing this, and I think you are missing at least one of them. > > > > > > > > > > > > 1. Adding `long :xxx` as padding makes struct at least 8-byte aligned. > > > > > > If the struct originally had a smaller alignment requirement, you are > > > > > > now potentially breaking the struct layout by changing its layout. > > > > > > > > > > > > 2. The way btf__align_of() is calculating alignment doesn't work > > > > > > correctly for __attribute__((packed)) structs. > > > > > > > > > > Missed these point, sorry. > > > > > On the other hand isn't this information lost in the custom.btf? > > > > > > > > > > $ bpftool btf dump file custom.btf > > > > > [1] STRUCT 'foo' size=20 vlen=2 > > > > > 'b' type_id=2 bits_offset=136 > > > > > 'c' type_id=3 bits_offset=144 > > > > > [2] INT 'char' size=1 bits_offset=0 nr_bits=8 encoding=SIGNED > > > > > [3] INT 'short' size=2 bits_offset=0 nr_bits=16 encoding=SIGNED > > > > > > > > > > This has no info that 'foo' had fields of size 'long'. It does not > > > > > matter for structs used inside BTF because 'bits_offset' is specified > > > > > everywhere, but would matter if STRUCT 'foo' is used as a member of a > > > > > non-BTF struct. > > > > > > > > Yes, the latter is important, though, right? > > > > > > Do you want to do anything about this at the custom BTF creation stage? > > > > No, absolutely not. We just need to teach btf_dump.c to not introduce > > any new alignment requirements while taking advantage of existing > > ones. We can derive enough information from BTF to achieve this. > > > > > E.g. leave one real member / create a synthetic member to force a specific > > > struct alignment in the minimized version. > > > > > > > So I think ideally we determine "maximum allowable alignment" and use > > > > that to determine what's the allowable set of padding types is. WDYT? > > > > > > Yes, I agree. > > > I think that a change in the btf__align_of() should be minimal, just check > > > if structure is packed and if so return 1, otherwise logic should remain > > > unchanged, this would match what LLVM does ([1]). > > > Also the flip of the order of chip_away_bits() should remain. > > > > Let's come up with a few tricky examples trying to break existing > > logic and then fix it. I suspect just chip_away_bits() changes are not > > sufficient. > > I have been using this python script to produce code that verifies > offsets for struct members for some various kernel 'btf's. > It compares the offsets from 'bits_offset' generated with 'bpftool > btf dump <file>' (without 'format c') and the offsets computed by > 'gcc' from header files generated with 'format c'. > > Use as: './verify_header_offsets.py <path to btf>' > > It will by default skip 50% of the members to make it harder for > bpftool to produce correct offsets (can be changed with environment > variable "RANDOM_SKIP_MEMBERS=<value between 0.0 and 1.0>" > > 'clang' does not play well with these big files, so I need to divide > the generated files into batches. Default is 1000 structs. > (can be controlled with environment variable 'MAX_STRUCTS') > Hi Per, Your script was very useful and instrumental to get to the final state of the fixes I just submitted ([0]). Thanks to it I also discovered for myself mode(byte) and mode(word) attributes, which influences sizing of enums. All the issues I found were fixed in my patch set. That said, your script isn't always correct, it seems. I've noticed that often when there are two structs involved, one of which is embedded in another as a field, offset assumptions will be incorrect. I suspect it has something to do with your script modifying nested struct definition in a way that actually changes its size, but offset checks don't take this into the account. I haven't spent time trying to understand or fix it, though. The script itself is great, and it would be nice to have it in selftests/bpf available at least for manual testing. So if you can, please improve it and submit it for inclusion. Oh, another issue is that sometimes clang will crash due to some condition violation when related to bitfields. It would be nice to either avoid this, or at least not spam the output. Please check that as well. But overall, I've found multiple issues as I worked on this, so it definitely was very useful. Thank you! [0] https://patchwork.kernel.org/project/netdevbpf/list/?series=703071&state=* > /Per > > > ---------- verify_header_offsets.py ------------ > #!/usr/bin/env python3 > > import os > import sys > import time > import random > import tempfile > import subprocess as sp > > structs_count = {} > n_files = 0 > done = False > > class AppError(Exception): > """ > Class used for application generated exceptions > """ > pass > > > def shell_cmd(command, **kwargs): > print(f"COMMAND: {command}") > res = sp.run(["bash", "-c", command], **kwargs) > if res.returncode != 0: > raise AppError(f'shell command "{command} failed') > return res > > > def find_struct_members(btf_file): > struct_list = [] > members = [] > found = False > name = "" > > res = shell_cmd(f"bpftool btf dump file {btf_file}", universal_newlines=True, stdout=sp.PIPE) > > for line in res.stdout.splitlines(): > #print(line, flush=True) > if found and line.startswith("\t"): > member = line.split()[0].replace("'", "") > # Get "Error: error recording relocations for <file>.o: Invalid argument" in 'bpftool' > # for some structs. Skip for now > if member in ['(anon)', 'context', 'inflate_state', 'dma_fence_array', 'net_generic']: > continue > bit_offset = int(line.split()[2].replace("bits_offset=", "")) > bitfield_size = int(line.split()[-1].replace("bitfield_size=", "")) if line.find("bitfield_size") > 0 else 0 > if random.random() > float(os.environ.get('RANDOM_SKIP_MEMBERS', '0.5')): > members.append((member, bitfield_size, bit_offset)) > else: > # flag skipped members with (0,0) so we can log them later > members.append((member, 0, 0)) > elif found: > found = False > struct_list.append((name, members)) > name = "" > members = [] > if line.find(" STRUCT ") > 0: > name = line.split()[2].replace("'", "") > if name in ["(anon)"]: > continue > structs_count[name] = structs_count.get(name, 0) + 1 > found = True > > # Due to limitations in "clang" we need to split the > # verification into batches > split_n_structs = int(os.environ.get('MAX_STRUCTS', 1000)) > structs = [] > batches = [] > for n, struct in enumerate(struct_list): > if n and n % split_n_structs == 0: > batches.append(structs) > structs = [] > structs.append(struct) > > batches.append(structs) > return batches > > def generate_header(dir, btf_file): > shell_cmd(f"bpftool btf dump file {btf_file} format c > {dir}/test.h") > > def generate_verification_code(dir, btf_file, struct_batch): > code = "" > main_body = "" > > code += f'#include "test_{n_files}.h"\n' > code += 'int printf(const char *format, ...);\n' > code += 'int sprintf(char *str, const char *format, ...);\n' > code += '#define offsetof(TYPE, MEMBER) ((long) &((TYPE*)0)->MEMBER)\n' > for name, members in struct_batch: > if structs_count[name] > 1: > # structs seen more than one time will be called 'struct foo___<n>' in > # the generated header file. Only that the '<n>' seems arbitrary, so skip > # them for now > continue > if name in ['context']: > # for some reason, there are missing structs in the generated header file > # skip them > continue > > code += f"int __ref_func_struct_{name}() {{\n" > code += f" int ret = 0;\n" > code += f" char data[100];\n" > for member, bitfield_size, bit_offset in members: > if bitfield_size: > code += f' ret += ((struct {name}*)&data)->{member}; /* bit_offset={bit_offset}, bitfield_size={bitfield_size} */\n' > else: > if bitfield_size == 0 and bit_offset == 0: > # Skip verifying non bitfield member at offset 0, will always be correct > code += f' /* ret += offsetof(struct {name}, {member}); Skipped */\n' > continue > code += f' ret += offsetof(struct {name}, {member});\n' > main_body += f' offset = offsetof(struct {name}, {member});\n' > main_body += f' sprintf(line, "offsetof(struct {name}, {member}) = %d", offset);\n' > main_body += f' printf("%-80.80s %s\\n", line, offset == {int(bit_offset/8)} ? "OK" : "Not OK (should be {int(bit_offset/8)})");\n' > code += " return ret;\n" > code += "}\n" > code += 'int main() {\n' > code += '#ifdef VERIFY\n' > code += ' char line[200];\n' > code += ' int offset = 0;\n' > code += ' int dummy = 0;\n' > code += main_body > code += '#endif\n' > code += ' return 0;\n' > code += '}\n' > with open(f"{dir}/test_{n_files}.c", "w") as f: > f.write(code) > > def compile_btf_object(dir, btf_file): > shell_cmd(f"cp {dir}/test.h {dir}/test_{n_files}.h") > shell_cmd(f"clang -c -I{dir} -ggdb -gdwarf -fpie -target bpf -D__TARGET_ARCH_x86 -o {dir}/test_{n_files}.o {dir}/test_{n_files}.c") > shell_cmd(f"bpftool gen min_core_btf {btf_file} {dir}/test.btf {dir}/test_{n_files}.o") > shell_cmd(f"bpftool btf dump file {dir}/test.btf format c > {dir}/test_{n_files}.h") > > def compile_and_run_verification(dir): > shell_cmd(f"gcc -DVERIFY -I{dir} -o {dir}/test_{n_files} {dir}/test_{n_files}.c") > shell_cmd(f"{dir}/test_{n_files}") > > def main(): > global n_files > global done > if len(sys.argv) > 1 and os.path.exists(sys.argv[1]): > btf_file = sys.argv[1] > print(f"Verifying btf file {btf_file}", flush=True) > #with tempfile.TemporaryDirectory() as dir: > dir="/tmp" > try: > generate_header(dir, btf_file) > for batch in find_struct_members(btf_file): > generate_verification_code(dir, btf_file, batch) > try: > compile_btf_object(dir, btf_file) > except AppError as fault: > print(f"Error: {fault}", file=sys.stderr) > print(f".. ignore ..", file=sys.stderr) > continue > compile_and_run_verification(dir) > if done: > break > n_files += 1 > except AppError as fault: > print(f"Error: {fault}", file=sys.stderr) > sys.exit(1) > > if __name__ == "__main__": > main() > > > > > > [1] > > > https://protect2.fireeye.com/v1/url?k=31323334-501cfaf3-313273af-454445554331-e6381a6a39d24e8d&q=1&e=50f6402e-fdb7-4512-8c16-8ce450e943f7&u=https%3A%2F%2Fgithub.com%2Feddyz87%2Fllvm-project%2Fblob%2Fmain%2Fllvm%2Flib%2FIR%2FDataLayout.cpp%23L764 > > > > > > So we need to fix btf__align_of() first. What btf__align_of() is > > > > > > calculating right now is a natural alignment requirement if we ignore > > > > > > actual field offsets. This might be useful (at the very least to > > > > > > determine if the struct is packed or not), so maybe we should have a > > > > > > separate btf__natural_align_of() or something along those lines? > > > > > > > > > > > > And then we need to fix btf_dump_emit_bit_padding() to better handle > > > > > > alignment and padding rules. This is what Per Sundström is trying to > > > > > > do, I believe, but I haven't carefully thought about his latest code > > > > > > suggestion. > > > > > > > > > > > > In general, the most obvious solution would be to pad with `char :8;` > > > > > > everywhere, but that's very ugly and I'd prefer us to have as > > > > > > "natural" output as possible. That is, only emit strictly necessary > > > > > > padding fields and rely on natural alignment otherwise. > > > > > > > > > > > > > > > > > > > > > > diff --git a/src/btf_dump.c b/src/btf_dump.c > > > > > > > > > index 12f7039..a8bd52a 100644 > > > > > > > > > --- a/src/btf_dump.c > > > > > > > > > +++ b/src/btf_dump.c > > > > > > > > > @@ -881,13 +881,13 @@ static void btf_dump_emit_bit_padding(const > > > > > > > > > struct btf_dump *d, > > > > > > > > > const char *pad_type; > > > > > > > > > int pad_bits; > > > > > > > > > > > > > > > > > > - if (ptr_bits > 32 && off_diff > 32) { > > > > > > > > > + if (align > 4 && ptr_bits > 32 && off_diff > 32) { > > > > > > > > > pad_type = "long"; > > > > > > > > > pad_bits = chip_away_bits(off_diff, ptr_bits); > > > > > > > > > - } else if (off_diff > 16) { > > > > > > > > > + } else if (align > 2 && off_diff > 16) { > > > > > > > > > pad_type = "int"; > > > > > > > > > pad_bits = chip_away_bits(off_diff, 32); > > > > > > > > > - } else if (off_diff > 8) { > > > > > > > > > + } else if (align > 1 && off_diff > 8) { > > > > > > > > > pad_type = "short"; > > > > > > > > > pad_bits = chip_away_bits(off_diff, 16); > > > > > > > > > } else { > > > > > > > > > /Per ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-12-08 19:05 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-11-18 10:30 Bad padding with bpftool btf dump .. format c Per Sundström XP 2022-11-18 12:42 ` Jiri Olsa 2022-11-18 15:10 ` Sv: " Per Sundström XP 2022-11-18 17:26 ` Per Sundström XP 2022-11-24 2:37 ` Andrii Nakryiko 2022-11-29 17:38 ` Eduard Zingerman 2022-11-30 0:27 ` Andrii Nakryiko 2022-11-30 2:29 ` Eduard Zingerman 2022-11-30 22:49 ` Andrii Nakryiko 2022-11-30 23:06 ` Eduard Zingerman 2022-11-30 23:11 ` Andrii Nakryiko 2022-12-05 13:54 ` Per Sundström XP 2022-12-08 19:04 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox