On Fri, Dec 9, 2022 at 9:21 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > On Thu, 2022-12-08 at 10:57 -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..d708452c9952 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_natural_align_of(btf, m->type)); > > + } > > + > > + return align; > > +} > > + > > The btf_natural_align_of() recursively visits nested structures. > However, the "packed" relation is non-recursive (see entry for > "packed" in [1]). Such mismatch leads to the following example being > printed incorrectly: > > struct a { > int x; > }; > > struct b { > struct a a; > char c; > } __attribute__((packed)); > > struct c { > struct b b1; > short a1; > struct b b2; > }; > > The bpftool output looks as follows: > > struct a { > int x; > }; > > struct b { > struct a a; > char c; > } __attribute__((packed)); > > struct c { > struct b b1; > short: 0; > short a1; > struct b b2; > } __attribute__((packed)); Nice find, thank you! The fix is very simple: diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c index d708452c9952..d6fd93a57f11 100644 --- a/tools/lib/bpf/btf_dump.c +++ b/tools/lib/bpf/btf_dump.c @@ -843,7 +843,7 @@ static int btf_natural_align_of(const struct btf *btf, __u32 id) m = btf_members(t); vlen = btf_vlen(t); for (i = 0; i < vlen; i++, m++) { - align = max(align, btf_natural_align_of(btf, m->type)); + align = max(align, btf__align_of(btf, m->type)); } I'll also add your example to selftests to make sure. [...] > > Also the following comment in [2] is interesting: > > > If the member is a structure, then the structure has an alignment > > of 1-byte, but the members of that structure continue to have their > > natural alignment. > If I read it correctly, it just means that within that nested struct all the members are aligned naturally (unless nested struct itself is packed), which makes sense and is already handled correctly. Or did I miss some subtle point you are making? > Which leads to unaligned access in the following case: > > int foo(struct a *p) { return p->x; } > > int main() { > struct b b[2] = {{ {1}, 2 }, { {3}, 4 }}; > printf("%i, %i\n", foo(&b[0].a), foo(&b[1].a)); > } > > $ gcc -Wall test.c > test.c: In function ‘main’: > test.c:38:26: warning: taking address of packed member of ‘struct b’ may result > in an unaligned pointer value [-Waddress-of-packed-member] > 38 | printf("%i, %i\n", foo(&b[0].a), foo(&b[1].a)); > > (This works fine on my x86 machine, but would be an issue on arm as > far as I understand). > > [1] https://gcc.gnu.org/onlinedocs/gcc-12.2.0/gcc/Common-Type-Attributes.html#Common-Type-Attributes > [2] https://developer.arm.com/documentation/100748/0607/writing-optimized-code/packing-data-structures > [...]
