On Mon, 2022-12-12 at 10:44 -0800, Andrii Nakryiko wrote: > 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? No additional subtle points. It's a consequence of the non-recursiveness of "packed" and I wanted to find a direct confirmation of such behaviour in the doc as it seemed odd. > > > 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 > > > > [...]
