On Wed, Dec 14, 2022 at 4:19 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > On Mon, 2022-12-12 at 13:15 -0800, Andrii Nakryiko wrote: > > Turns out that btf_dump API doesn't handle a bunch of tricky corner > > cases, as reported by Per, and further discovered using his testing > > Python script ([0]). > > > > This patch revamps btf_dump's padding logic significantly, making it > > more correct and also avoiding unnecessary explicit padding, where > > compiler would pad naturally. This overall topic turned out to be very > > tricky and subtle, there are lots of subtle corner cases. The comments > > in the code tries to give some clues, but comments themselves are > > supposed to be paired with good understanding of C alignment and padding > > rules. Plus some experimentation to figure out subtle things like > > whether `long :0;` means that struct is now forced to be long-aligned > > (no, it's not, turns out). > > > > Anyways, Per's script, while not completely correct in some known > > situations, doesn't show any obvious cases where this logic breaks, so > > this is a nice improvement over the previous state of this logic. > > > > Some selftests had to be adjusted to accommodate better use of natural > > alignment rules, eliminating some unnecessary padding, or changing it to > > `type: 0;` alignment markers. > > > > Note also that for when we are in between bitfields, we emit explicit > > bit size, while otherwise we use `: 0`, this feels much more natural in > > practice. > > > > Next patch will add few more test cases, found through randomized Per's > > script. > > > > [0] https://lore.kernel.org/bpf/85f83c333f5355c8ac026f835b18d15060725fcb.camel@xxxxxxxxxxxx/ > > > > Reported-by: Per Sundström XP <per.xp.sundstrom@xxxxxxxxxxxx> > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx> > > --- > > tools/lib/bpf/btf_dump.c | 169 +++++++++++++----- > > .../bpf/progs/btf_dump_test_case_bitfields.c | 2 +- > > .../bpf/progs/btf_dump_test_case_padding.c | 58 ++++-- > > 3 files changed, 164 insertions(+), 65 deletions(-) > > > > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c > > index 234e82334d56..d6fd93a57f11 100644 > > --- a/tools/lib/bpf/btf_dump.c > > +++ b/tools/lib/bpf/btf_dump.c > > @@ -830,6 +830,25 @@ static void btf_dump_emit_type(struct btf_dump *d, __u32 id, __u32 cont_id) > > } > > } > > > > +static int btf_natural_align_of(const struct btf *btf, __u32 id) > > +{ > > + const struct btf_type *t = btf__type_by_id(btf, id); > > + int i, align, vlen; > > + const struct btf_member *m; > > + > > + if (!btf_is_composite(t)) > > + return btf__align_of(btf, id); > > + > > + align = 1; > > + m = btf_members(t); > > + vlen = btf_vlen(t); > > + for (i = 0; i < vlen; i++, m++) { > > + align = max(align, btf__align_of(btf, m->type)); > > + } > > + > > + return align; > > +} > > + > > static bool btf_is_struct_packed(const struct btf *btf, __u32 id, > > const struct btf_type *t) > > { > > @@ -837,16 +856,16 @@ static bool btf_is_struct_packed(const struct btf *btf, __u32 id, > > int align, i, bit_sz; > > __u16 vlen; > > > > - align = btf__align_of(btf, id); > > - /* size of a non-packed struct has to be a multiple of its alignment*/ > > - if (align && t->size % align) > > + align = btf_natural_align_of(btf, id); > > + /* size of a non-packed struct has to be a multiple of its alignment */ > > + if (align && (t->size % align) != 0) > > return true; > > > > m = btf_members(t); > > vlen = btf_vlen(t); > > /* all non-bitfield fields have to be naturally aligned */ > > for (i = 0; i < vlen; i++, m++) { > > - align = btf__align_of(btf, m->type); > > + align = btf_natural_align_of(btf, m->type); > > Sorry, I'm late to the party... > > I think that this call to btf__align_of() should remain as is, > otherwise the packed-ness of the m->type is ignored, which leads to > the example below generating unnecessary packed annotation, which in > turn leads to wrong result of sizeof() if asked. > Reverting this hunk makes the test below work as expected and > no other tests break. Yep, I don't think I actually need btf_natural_align_of(), I'll just adjust btf_is_struct_packed() instead. Thanks for another great catch! I'll send a follow up fix. > > 02:02:44 tmp$ cat test.c > #ifndef __BPF__ > > #include <stddef.h> > #include <stdio.h> > > #endif /* __BPF__ */ > > struct a { > int x; > char y; > } __attribute__((packed)); > > struct b { > short x; > struct a a; > }; > > #ifdef __BPF__ > > int root(struct b *b) { return 7; } > > #else /* __BPF__ */ > > struct b1 { > short x; > struct a a; > } __attribute__((packed)); > > int main() { > printf("sizeof(struct b ): %ld\n", sizeof(struct b)); > printf("sizeof(struct b1): %ld\n", sizeof(struct b1)); > } > > #endif /* __BPF__ */ > > 02:04:24 tmp$ clang test.c && ./a.out > sizeof(struct b ): 8 > sizeof(struct b1): 7 > > 02:04:30 tmp$ clang -target bpf -g -c test.c -o test.o && bpftool btf dump file ./test.o format c > #ifndef __VMLINUX_H__ > #define __VMLINUX_H__ > > #ifndef BPF_NO_PRESERVE_ACCESS_INDEX > #pragma clang attribute push (__attribute__((preserve_access_index)), apply_to = record) > #endif > > struct a { > int x; > char y; > } __attribute__((packed)); > > struct b { > short x; > struct a a; > } __attribute__((packed)); > > #ifndef BPF_NO_PRESERVE_ACCESS_INDEX > #pragma clang attribute pop > #endif > > #endif /* __VMLINUX_H__ */ > > > bit_sz = btf_member_bitfield_size(t, i); > > if (align && bit_sz == 0 && m->offset % (8 * align) != 0) > > return true; [...]
