On Wed, Nov 30, 2022 at 3:06 PM Eduard Zingerman <eddyz87@xxxxxxxxx> 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@xxxxxxxxx> 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@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.
> >
> > 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
> > > > >
> > >
>
