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

[Eduard Zingerman <eddyz87@gmail.com>]

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
> > > > 
> >