On Tue, 2022-11-29 at 16:27 -0800, Andrii Nakryiko wrote:
> On Tue, Nov 29, 2022 at 9:38 AM Eduard Zingerman <eddyz87@xxxxxxxxx> 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@xxxxxxxxxxxx> 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
> >
