* 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
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
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
> 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, andrii.nakryiko; +Cc: bpf
> > ============ 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, bpf
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, bpf
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, bpf
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, bpf
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, bpf
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, bpf
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, bpf
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, andrii.nakryiko; +Cc: olsajiri, bpf
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, olsajiri, bpf
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 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.