Re: Sv: Bad padding with bpftool btf dump .. format c

[Eduard Zingerman <eddyz87@gmail.com>]

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