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