[PATCH v2 bpf-next 0/6] BTF-to-C dumper fixes and improvements

[PATCH v2 bpf-next 0/6] BTF-to-C dumper fixes and improvements

[Andrii Nakryiko]

Fix few tricky issues in libbpf's BTF-to-C converter, discovered thanks to
Per's reports and his randomized testing script.

Most notably there is a much improved and correct padding handling.  But also
it turned out that some corner cases with enums weren't handled correctly
(mode(byte) attribute was a new discovery for me). See respective patches for
more details.

v1->v2:
  - fix bug in natural alignment detection (Eduard).

Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Per Sundström XP <per.xp.sundstrom@ericsson.com>

Andrii Nakryiko (6):
  libbpf: fix single-line struct definition output in btf_dump
  libbpf: handle non-standardly sized enums better in BTF-to-C dumper
  selftests/bpf: add non-standardly sized enum tests for btf_dump
  libbpf: fix btf__align_of() by taking into account field offsets
  libbpf: fix BTF-to-C converter's padding logic
  selftests/bpf: add few corner cases to test padding handling of
    btf_dump

 tools/lib/bpf/btf.c                           |  13 ++
 tools/lib/bpf/btf_dump.c                      | 214 ++++++++++++++----
 .../bpf/progs/btf_dump_test_case_bitfields.c  |   2 +-
 .../bpf/progs/btf_dump_test_case_packing.c    |  61 ++++-
 .../bpf/progs/btf_dump_test_case_padding.c    | 162 +++++++++++--
 .../bpf/progs/btf_dump_test_case_syntax.c     |  36 +++
 6 files changed, 420 insertions(+), 68 deletions(-)

-- 
2.30.2

[PATCH v2 bpf-next 1/6] libbpf: fix single-line struct definition output in btf_dump

[Andrii Nakryiko]

btf_dump APIs emit unnecessary tabs when emitting struct/union
definition that fits on the single line. Before this patch we'd get:

struct blah {<tab>};

This patch fixes this and makes sure that we get more natural:

struct blah {};

Fixes: 44a726c3f23c ("bpftool: Print newline before '}' for struct with padding only fields")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/btf_dump.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index deb2bc9a0a7b..69e80ee5f70e 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -959,9 +959,12 @@ static void btf_dump_emit_struct_def(struct btf_dump *d,
 	 * Keep `struct empty {}` on a single line,
 	 * only print newline when there are regular or padding fields.
 	 */
-	if (vlen || t->size)
+	if (vlen || t->size) {
 		btf_dump_printf(d, "\n");
-	btf_dump_printf(d, "%s}", pfx(lvl));
+		btf_dump_printf(d, "%s}", pfx(lvl));
+	} else {
+		btf_dump_printf(d, "}");
+	}
 	if (packed)
 		btf_dump_printf(d, " __attribute__((packed))");
 }
-- 
2.30.2

[PATCH v2 bpf-next 2/6] libbpf: handle non-standardly sized enums better in BTF-to-C dumper

[Andrii Nakryiko]

Turns out C allows to force enum to be 1-byte or 8-byte explicitly using
mode(byte) or mode(word), respecticely. Linux sources are using this in
some cases. This is imporant to handle correctly, as enum size
determines corresponding fields in a struct that use that enum type. And
if enum size is incorrect, this will lead to invalid struct layout. So
add mode(byte) and mode(word) attribute support to btf_dump APIs.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/btf_dump.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index 69e80ee5f70e..234e82334d56 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -13,6 +13,7 @@
 #include <ctype.h>
 #include <endian.h>
 #include <errno.h>
+#include <limits.h>
 #include <linux/err.h>
 #include <linux/btf.h>
 #include <linux/kernel.h>
@@ -1076,6 +1077,43 @@ static void btf_dump_emit_enum_def(struct btf_dump *d, __u32 id,
 	else
 		btf_dump_emit_enum64_val(d, t, lvl, vlen);
 	btf_dump_printf(d, "\n%s}", pfx(lvl));
+
+	/* special case enums with special sizes */
+	if (t->size == 1) {
+		/* one-byte enums can be forced with mode(byte) attribute */
+		btf_dump_printf(d, " __attribute__((mode(byte)))");
+	} else if (t->size == 8 && d->ptr_sz == 8) {
+		/* enum can be 8-byte sized if one of the enumerator values
+		 * doesn't fit in 32-bit integer, or by adding mode(word)
+		 * attribute (but probably only on 64-bit architectures); do
+		 * our best here to try to satisfy the contract without adding
+		 * unnecessary attributes
+		 */
+		bool needs_word_mode;
+
+		if (btf_is_enum(t)) {
+			/* enum can't represent 64-bit values, so we need word mode */
+			needs_word_mode = true;
+		} else {
+			/* enum64 needs mode(word) if none of its values has
+			 * non-zero upper 32-bits (which means that all values
+			 * fit in 32-bit integers and won't cause compiler to
+			 * bump enum to be 64-bit naturally
+			 */
+			int i;
+
+			needs_word_mode = true;
+			for (i = 0; i < vlen; i++) {
+				if (btf_enum64(t)[i].val_hi32 != 0) {
+					needs_word_mode = false;
+					break;
+				}
+			}
+		}
+		if (needs_word_mode)
+			btf_dump_printf(d, " __attribute__((mode(word)))");
+	}
+
 }
 
 static void btf_dump_emit_fwd_def(struct btf_dump *d, __u32 id,
-- 
2.30.2

[PATCH v2 bpf-next 3/6] selftests/bpf: add non-standardly sized enum tests for btf_dump

[Andrii Nakryiko]

Add few custom enum definitions testing mode(byte) and mode(word)
attributes.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 .../bpf/progs/btf_dump_test_case_syntax.c     | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c
index 4ee4748133fe..26fffb02ed10 100644
--- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c
+++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c
@@ -25,6 +25,39 @@ typedef enum {
 	H = 2,
 } e3_t;
 
+/* ----- START-EXPECTED-OUTPUT ----- */
+/*
+ *enum e_byte {
+ *	EBYTE_1 = 0,
+ *	EBYTE_2 = 1,
+ *} __attribute__((mode(byte)));
+ *
+ */
+/* ----- END-EXPECTED-OUTPUT ----- */
+enum e_byte {
+	EBYTE_1,
+	EBYTE_2,
+} __attribute__((mode(byte)));
+
+/* ----- START-EXPECTED-OUTPUT ----- */
+/*
+ *enum e_word {
+ *	EWORD_1 = 0LL,
+ *	EWORD_2 = 1LL,
+ *} __attribute__((mode(word)));
+ *
+ */
+/* ----- END-EXPECTED-OUTPUT ----- */
+enum e_word {
+	EWORD_1,
+	EWORD_2,
+} __attribute__((mode(word))); /* force to use 8-byte backing for this enum */
+
+/* ----- START-EXPECTED-OUTPUT ----- */
+enum e_big {
+	EBIG_1 = 1000000000000ULL,
+};
+
 typedef int int_t;
 
 typedef volatile const int * volatile const crazy_ptr_t;
@@ -224,6 +257,9 @@ struct root_struct {
 	enum e2 _2;
 	e2_t _2_1;
 	e3_t _2_2;
+	enum e_byte _100;
+	enum e_word _101;
+	enum e_big _102;
 	struct struct_w_typedefs _3;
 	anon_struct_t _7;
 	struct struct_fwd *_8;
-- 
2.30.2

[PATCH v2 bpf-next 4/6] libbpf: fix btf__align_of() by taking into account field offsets

[Andrii Nakryiko]

btf__align_of() is supposed to be return alignment requirement of
a requested BTF type. For STRUCT/UNION it doesn't always return correct
value, because it calculates alignment only based on field types. But
for packed structs this is not enough, we need to also check field
offsets and struct size. If field offset isn't aligned according to
field type's natural alignment, then struct must be packed. Similarly,
if struct size is not a multiple of struct's natural alignment, then
struct must be packed as well.

This patch fixes this issue precisely by additionally checking these
conditions.

Fixes: 3d208f4ca111 ("libbpf: Expose btf__align_of() API")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/btf.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 71e165b09ed5..8cbcef959456 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -688,8 +688,21 @@ int btf__align_of(const struct btf *btf, __u32 id)
 			if (align <= 0)
 				return libbpf_err(align);
 			max_align = max(max_align, align);
+
+			/* if field offset isn't aligned according to field
+			 * type's alignment, then struct must be packed
+			 */
+			if (btf_member_bitfield_size(t, i) == 0 &&
+			    (m->offset % (8 * align)) != 0)
+				return 1;
 		}
 
+		/* if struct/union size isn't a multiple of its alignment,
+		 * then struct must be packed
+		 */
+		if ((t->size % max_align) != 0)
+			return 1;
+
 		return max_align;
 	}
 	default:
-- 
2.30.2

[PATCH v2 bpf-next 5/6] libbpf: fix BTF-to-C converter's padding logic

[Andrii Nakryiko]

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@ericsson.com/

Reported-by: Per Sundström XP <per.xp.sundstrom@ericsson.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 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);
 		bit_sz = btf_member_bitfield_size(t, i);
 		if (align && bit_sz == 0 && m->offset % (8 * align) != 0)
 			return true;
@@ -859,44 +878,97 @@ static bool btf_is_struct_packed(const struct btf *btf, __u32 id,
 	return false;
 }
 
-static int chip_away_bits(int total, int at_most)
-{
-	return total % at_most ? : at_most;
-}
-
 static void btf_dump_emit_bit_padding(const struct btf_dump *d,
-				      int cur_off, int m_off, int m_bit_sz,
-				      int align, int lvl)
+				      int cur_off, int next_off, int next_align,
+				      bool in_bitfield, int lvl)
 {
-	int off_diff = m_off - cur_off;
-	int ptr_bits = d->ptr_sz * 8;
+	const struct {
+		const char *name;
+		int bits;
+	} pads[] = {
+		{"long", d->ptr_sz * 8}, {"int", 32}, {"short", 16}, {"char", 8}
+	};
+	int new_off, pad_bits, bits, i;
+	const char *pad_type;
+
+	if (cur_off >= next_off)
+		return; /* no gap */
+
+	/* For filling out padding we want to take advantage of
+	 * natural alignment rules to minimize unnecessary explicit
+	 * padding. First, we find the largest type (among long, int,
+	 * short, or char) that can be used to force naturally aligned
+	 * boundary. Once determined, we'll use such type to fill in
+	 * the remaining padding gap. In some cases we can rely on
+	 * compiler filling some gaps, but sometimes we need to force
+	 * alignment to close natural alignment with markers like
+	 * `long: 0` (this is always the case for bitfields).  Note
+	 * that even if struct itself has, let's say 4-byte alignment
+	 * (i.e., it only uses up to int-aligned types), using `long:
+	 * X;` explicit padding doesn't actually change struct's
+	 * overall alignment requirements, but compiler does take into
+	 * account that type's (long, in this example) natural
+	 * alignment requirements when adding implicit padding. We use
+	 * this fact heavily and don't worry about ruining correct
+	 * struct alignment requirement.
+	 */
+	for (i = 0; i < ARRAY_SIZE(pads); i++) {
+		pad_bits = pads[i].bits;
+		pad_type = pads[i].name;
 
-	if (off_diff <= 0)
-		/* no gap */
-		return;
-	if (m_bit_sz == 0 && off_diff < align * 8)
-		/* natural padding will take care of a gap */
-		return;
+		new_off = roundup(cur_off, pad_bits);
+		if (new_off <= next_off)
+			break;
+	}
 
-	while (off_diff > 0) {
-		const char *pad_type;
-		int pad_bits;
-
-		if (ptr_bits > 32 && off_diff > 32) {
-			pad_type = "long";
-			pad_bits = chip_away_bits(off_diff, ptr_bits);
-		} else if (off_diff > 16) {
-			pad_type = "int";
-			pad_bits = chip_away_bits(off_diff, 32);
-		} else if (off_diff > 8) {
-			pad_type = "short";
-			pad_bits = chip_away_bits(off_diff, 16);
-		} else {
-			pad_type = "char";
-			pad_bits = chip_away_bits(off_diff, 8);
+	if (new_off > cur_off && new_off <= next_off) {
+		/* We need explicit `<type>: 0` aligning mark if next
+		 * field is right on alignment offset and its
+		 * alignment requirement is less strict than <type>'s
+		 * alignment (so compiler won't naturally align to the
+		 * offset we expect), or if subsequent `<type>: X`,
+		 * will actually completely fit in the remaining hole,
+		 * making compiler basically ignore `<type>: X`
+		 * completely.
+		 */
+		if (in_bitfield ||
+		    (new_off == next_off && roundup(cur_off, next_align * 8) != new_off) ||
+		    (new_off != next_off && next_off - new_off <= new_off - cur_off))
+			/* but for bitfields we'll emit explicit bit count */
+			btf_dump_printf(d, "\n%s%s: %d;", pfx(lvl), pad_type,
+					in_bitfield ? new_off - cur_off : 0);
+		cur_off = new_off;
+	}
+
+	/* Now we know we start at naturally aligned offset for a chosen
+	 * padding type (long, int, short, or char), and so the rest is just
+	 * a straightforward filling of remaining padding gap with full
+	 * `<type>: sizeof(<type>);` markers, except for the last one, which
+	 * might need smaller than sizeof(<type>) padding.
+	 */
+	while (cur_off != next_off) {
+		bits = min(next_off - cur_off, pad_bits);
+		if (bits == pad_bits) {
+			btf_dump_printf(d, "\n%s%s: %d;", pfx(lvl), pad_type, pad_bits);
+			cur_off += bits;
+			continue;
+		}
+		/* For the remainder padding that doesn't cover entire
+		 * pad_type bit length, we pick the smallest necessary type.
+		 * This is pure aesthetics, we could have just used `long`,
+		 * but having smallest necessary one communicates better the
+		 * scale of the padding gap.
+		 */
+		for (i = ARRAY_SIZE(pads) - 1; i >= 0; i--) {
+			pad_type = pads[i].name;
+			pad_bits = pads[i].bits;
+			if (pad_bits < bits)
+				continue;
+
+			btf_dump_printf(d, "\n%s%s: %d;", pfx(lvl), pad_type, bits);
+			cur_off += bits;
+			break;
 		}
-		btf_dump_printf(d, "\n%s%s: %d;", pfx(lvl), pad_type, pad_bits);
-		off_diff -= pad_bits;
 	}
 }
 
@@ -916,9 +988,11 @@ static void btf_dump_emit_struct_def(struct btf_dump *d,
 {
 	const struct btf_member *m = btf_members(t);
 	bool is_struct = btf_is_struct(t);
-	int align, i, packed, off = 0;
+	bool packed, prev_bitfield = false;
+	int align, i, off = 0;
 	__u16 vlen = btf_vlen(t);
 
+	align = btf__align_of(d->btf, id);
 	packed = is_struct ? btf_is_struct_packed(d->btf, id, t) : 0;
 
 	btf_dump_printf(d, "%s%s%s {",
@@ -928,33 +1002,36 @@ static void btf_dump_emit_struct_def(struct btf_dump *d,
 
 	for (i = 0; i < vlen; i++, m++) {
 		const char *fname;
-		int m_off, m_sz;
+		int m_off, m_sz, m_align;
+		bool in_bitfield;
 
 		fname = btf_name_of(d, m->name_off);
 		m_sz = btf_member_bitfield_size(t, i);
 		m_off = btf_member_bit_offset(t, i);
-		align = packed ? 1 : btf__align_of(d->btf, m->type);
+		m_align = packed ? 1 : btf__align_of(d->btf, m->type);
+
+		in_bitfield = prev_bitfield && m_sz != 0;
 
-		btf_dump_emit_bit_padding(d, off, m_off, m_sz, align, lvl + 1);
+		btf_dump_emit_bit_padding(d, off, m_off, m_align, in_bitfield, lvl + 1);
 		btf_dump_printf(d, "\n%s", pfx(lvl + 1));
 		btf_dump_emit_type_decl(d, m->type, fname, lvl + 1);
 
 		if (m_sz) {
 			btf_dump_printf(d, ": %d", m_sz);
 			off = m_off + m_sz;
+			prev_bitfield = true;
 		} else {
 			m_sz = max((__s64)0, btf__resolve_size(d->btf, m->type));
 			off = m_off + m_sz * 8;
+			prev_bitfield = false;
 		}
+
 		btf_dump_printf(d, ";");
 	}
 
 	/* pad at the end, if necessary */
-	if (is_struct) {
-		align = packed ? 1 : btf__align_of(d->btf, id);
-		btf_dump_emit_bit_padding(d, off, t->size * 8, 0, align,
-					  lvl + 1);
-	}
+	if (is_struct)
+		btf_dump_emit_bit_padding(d, off, t->size * 8, align, false, lvl + 1);
 
 	/*
 	 * Keep `struct empty {}` on a single line,
diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c
index e5560a656030..e01690618e1e 100644
--- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c
+++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c
@@ -53,7 +53,7 @@ struct bitfields_only_mixed_types {
  */
 /* ------ END-EXPECTED-OUTPUT ------ */
 struct bitfield_mixed_with_others {
-	long: 4; /* char is enough as a backing field */
+	char: 4; /* char is enough as a backing field */
 	int a: 4;
 	/* 8-bit implicit padding */
 	short b; /* combined with previous bitfield */
diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c
index 7cb522d22a66..6f963d34c45b 100644
--- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c
+++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c
@@ -19,7 +19,7 @@ struct padded_implicitly {
 /*
  *struct padded_explicitly {
  *	int a;
- *	int: 32;
+ *	long: 0;
  *	int b;
  *};
  *
@@ -28,41 +28,28 @@ struct padded_implicitly {
 
 struct padded_explicitly {
 	int a;
-	int: 1; /* algo will explicitly pad with full 32 bits here */
+	int: 1; /* algo will emit aligning `long: 0;` here */
 	int b;
 };
 
 /* ----- START-EXPECTED-OUTPUT ----- */
-/*
- *struct padded_a_lot {
- *	int a;
- *	long: 32;
- *	long: 64;
- *	long: 64;
- *	int b;
- *};
- *
- */
-/* ------ END-EXPECTED-OUTPUT ------ */
-
 struct padded_a_lot {
 	int a;
-	/* 32 bit of implicit padding here, which algo will make explicit */
 	long: 64;
 	long: 64;
 	int b;
 };
 
+/* ------ END-EXPECTED-OUTPUT ------ */
+
 /* ----- START-EXPECTED-OUTPUT ----- */
 /*
  *struct padded_cache_line {
  *	int a;
- *	long: 32;
  *	long: 64;
  *	long: 64;
  *	long: 64;
  *	int b;
- *	long: 32;
  *	long: 64;
  *	long: 64;
  *	long: 64;
@@ -85,7 +72,7 @@ struct padded_cache_line {
  *struct zone {
  *	int a;
  *	short b;
- *	short: 16;
+ *	long: 0;
  *	struct zone_padding __pad__;
  *};
  *
@@ -108,6 +95,39 @@ struct padding_wo_named_members {
 	long: 64;
 };
 
+struct padding_weird_1 {
+	int a;
+	long: 64;
+	short: 16;
+	short b;
+};
+
+/* ------ END-EXPECTED-OUTPUT ------ */
+
+/* ----- START-EXPECTED-OUTPUT ----- */
+/*
+ *struct padding_weird_2 {
+ *	long: 56;
+ *	char a;
+ *	long: 56;
+ *	char b;
+ *	char: 8;
+ *};
+ *
+ */
+/* ------ END-EXPECTED-OUTPUT ------ */
+struct padding_weird_2 {
+	int: 32;	/* these paddings will be collapsed into `long: 56;` */
+	short: 16;
+	char: 8;
+	char a;
+	int: 32;	/* these paddings will be collapsed into `long: 56;` */
+	short: 16;
+	char: 8;
+	char b;
+	char: 8;
+};
+
 /* ------ END-EXPECTED-OUTPUT ------ */
 
 int f(struct {
@@ -117,6 +137,8 @@ int f(struct {
 	struct padded_cache_line _4;
 	struct zone _5;
 	struct padding_wo_named_members _6;
+	struct padding_weird_1 _7;
+	struct padding_weird_2 _8;
 } *_)
 {
 	return 0;
-- 
2.30.2

Re: [PATCH v2 bpf-next 5/6] libbpf: fix BTF-to-C converter's padding logic

[Eduard Zingerman]

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@ericsson.com/
> 
> Reported-by: Per Sundström XP <per.xp.sundstrom@ericsson.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  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.

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;
> @@ -859,44 +878,97 @@ static bool btf_is_struct_packed(const struct btf *btf, __u32 id,
>  	return false;
>  }
>  
> -static int chip_away_bits(int total, int at_most)
> -{
> -	return total % at_most ? : at_most;
> -}
> -
>  static void btf_dump_emit_bit_padding(const struct btf_dump *d,
> -				      int cur_off, int m_off, int m_bit_sz,
> -				      int align, int lvl)
> +				      int cur_off, int next_off, int next_align,
> +				      bool in_bitfield, int lvl)
>  {
> -	int off_diff = m_off - cur_off;
> -	int ptr_bits = d->ptr_sz * 8;
> +	const struct {
> +		const char *name;
> +		int bits;
> +	} pads[] = {
> +		{"long", d->ptr_sz * 8}, {"int", 32}, {"short", 16}, {"char", 8}
> +	};
> +	int new_off, pad_bits, bits, i;
> +	const char *pad_type;
> +
> +	if (cur_off >= next_off)
> +		return; /* no gap */
> +
> +	/* For filling out padding we want to take advantage of
> +	 * natural alignment rules to minimize unnecessary explicit
> +	 * padding. First, we find the largest type (among long, int,
> +	 * short, or char) that can be used to force naturally aligned
> +	 * boundary. Once determined, we'll use such type to fill in
> +	 * the remaining padding gap. In some cases we can rely on
> +	 * compiler filling some gaps, but sometimes we need to force
> +	 * alignment to close natural alignment with markers like
> +	 * `long: 0` (this is always the case for bitfields).  Note
> +	 * that even if struct itself has, let's say 4-byte alignment
> +	 * (i.e., it only uses up to int-aligned types), using `long:
> +	 * X;` explicit padding doesn't actually change struct's
> +	 * overall alignment requirements, but compiler does take into
> +	 * account that type's (long, in this example) natural
> +	 * alignment requirements when adding implicit padding. We use
> +	 * this fact heavily and don't worry about ruining correct
> +	 * struct alignment requirement.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(pads); i++) {
> +		pad_bits = pads[i].bits;
> +		pad_type = pads[i].name;
>  
> -	if (off_diff <= 0)
> -		/* no gap */
> -		return;
> -	if (m_bit_sz == 0 && off_diff < align * 8)
> -		/* natural padding will take care of a gap */
> -		return;
> +		new_off = roundup(cur_off, pad_bits);
> +		if (new_off <= next_off)
> +			break;
> +	}
>  
> -	while (off_diff > 0) {
> -		const char *pad_type;
> -		int pad_bits;
> -
> -		if (ptr_bits > 32 && off_diff > 32) {
> -			pad_type = "long";
> -			pad_bits = chip_away_bits(off_diff, ptr_bits);
> -		} else if (off_diff > 16) {
> -			pad_type = "int";
> -			pad_bits = chip_away_bits(off_diff, 32);
> -		} else if (off_diff > 8) {
> -			pad_type = "short";
> -			pad_bits = chip_away_bits(off_diff, 16);
> -		} else {
> -			pad_type = "char";
> -			pad_bits = chip_away_bits(off_diff, 8);
> +	if (new_off > cur_off && new_off <= next_off) {
> +		/* We need explicit `<type>: 0` aligning mark if next
> +		 * field is right on alignment offset and its
> +		 * alignment requirement is less strict than <type>'s
> +		 * alignment (so compiler won't naturally align to the
> +		 * offset we expect), or if subsequent `<type>: X`,
> +		 * will actually completely fit in the remaining hole,
> +		 * making compiler basically ignore `<type>: X`
> +		 * completely.
> +		 */
> +		if (in_bitfield ||
> +		    (new_off == next_off && roundup(cur_off, next_align * 8) != new_off) ||
> +		    (new_off != next_off && next_off - new_off <= new_off - cur_off))
> +			/* but for bitfields we'll emit explicit bit count */
> +			btf_dump_printf(d, "\n%s%s: %d;", pfx(lvl), pad_type,
> +					in_bitfield ? new_off - cur_off : 0);
> +		cur_off = new_off;
> +	}
> +
> +	/* Now we know we start at naturally aligned offset for a chosen
> +	 * padding type (long, int, short, or char), and so the rest is just
> +	 * a straightforward filling of remaining padding gap with full
> +	 * `<type>: sizeof(<type>);` markers, except for the last one, which
> +	 * might need smaller than sizeof(<type>) padding.
> +	 */
> +	while (cur_off != next_off) {
> +		bits = min(next_off - cur_off, pad_bits);
> +		if (bits == pad_bits) {
> +			btf_dump_printf(d, "\n%s%s: %d;", pfx(lvl), pad_type, pad_bits);
> +			cur_off += bits;
> +			continue;
> +		}
> +		/* For the remainder padding that doesn't cover entire
> +		 * pad_type bit length, we pick the smallest necessary type.
> +		 * This is pure aesthetics, we could have just used `long`,
> +		 * but having smallest necessary one communicates better the
> +		 * scale of the padding gap.
> +		 */
> +		for (i = ARRAY_SIZE(pads) - 1; i >= 0; i--) {
> +			pad_type = pads[i].name;
> +			pad_bits = pads[i].bits;
> +			if (pad_bits < bits)
> +				continue;
> +
> +			btf_dump_printf(d, "\n%s%s: %d;", pfx(lvl), pad_type, bits);
> +			cur_off += bits;
> +			break;
>  		}
> -		btf_dump_printf(d, "\n%s%s: %d;", pfx(lvl), pad_type, pad_bits);
> -		off_diff -= pad_bits;
>  	}
>  }
>  
> @@ -916,9 +988,11 @@ static void btf_dump_emit_struct_def(struct btf_dump *d,
>  {
>  	const struct btf_member *m = btf_members(t);
>  	bool is_struct = btf_is_struct(t);
> -	int align, i, packed, off = 0;
> +	bool packed, prev_bitfield = false;
> +	int align, i, off = 0;
>  	__u16 vlen = btf_vlen(t);
>  
> +	align = btf__align_of(d->btf, id);
>  	packed = is_struct ? btf_is_struct_packed(d->btf, id, t) : 0;
>  
>  	btf_dump_printf(d, "%s%s%s {",
> @@ -928,33 +1002,36 @@ static void btf_dump_emit_struct_def(struct btf_dump *d,
>  
>  	for (i = 0; i < vlen; i++, m++) {
>  		const char *fname;
> -		int m_off, m_sz;
> +		int m_off, m_sz, m_align;
> +		bool in_bitfield;
>  
>  		fname = btf_name_of(d, m->name_off);
>  		m_sz = btf_member_bitfield_size(t, i);
>  		m_off = btf_member_bit_offset(t, i);
> -		align = packed ? 1 : btf__align_of(d->btf, m->type);
> +		m_align = packed ? 1 : btf__align_of(d->btf, m->type);
> +
> +		in_bitfield = prev_bitfield && m_sz != 0;
>  
> -		btf_dump_emit_bit_padding(d, off, m_off, m_sz, align, lvl + 1);
> +		btf_dump_emit_bit_padding(d, off, m_off, m_align, in_bitfield, lvl + 1);
>  		btf_dump_printf(d, "\n%s", pfx(lvl + 1));
>  		btf_dump_emit_type_decl(d, m->type, fname, lvl + 1);
>  
>  		if (m_sz) {
>  			btf_dump_printf(d, ": %d", m_sz);
>  			off = m_off + m_sz;
> +			prev_bitfield = true;
>  		} else {
>  			m_sz = max((__s64)0, btf__resolve_size(d->btf, m->type));
>  			off = m_off + m_sz * 8;
> +			prev_bitfield = false;
>  		}
> +
>  		btf_dump_printf(d, ";");
>  	}
>  
>  	/* pad at the end, if necessary */
> -	if (is_struct) {
> -		align = packed ? 1 : btf__align_of(d->btf, id);
> -		btf_dump_emit_bit_padding(d, off, t->size * 8, 0, align,
> -					  lvl + 1);
> -	}
> +	if (is_struct)
> +		btf_dump_emit_bit_padding(d, off, t->size * 8, align, false, lvl + 1);
>  
>  	/*
>  	 * Keep `struct empty {}` on a single line,
> diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c
> index e5560a656030..e01690618e1e 100644
> --- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c
> +++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c
> @@ -53,7 +53,7 @@ struct bitfields_only_mixed_types {
>   */
>  /* ------ END-EXPECTED-OUTPUT ------ */
>  struct bitfield_mixed_with_others {
> -	long: 4; /* char is enough as a backing field */
> +	char: 4; /* char is enough as a backing field */
>  	int a: 4;
>  	/* 8-bit implicit padding */
>  	short b; /* combined with previous bitfield */
> diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c
> index 7cb522d22a66..6f963d34c45b 100644
> --- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c
> +++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c
> @@ -19,7 +19,7 @@ struct padded_implicitly {
>  /*
>   *struct padded_explicitly {
>   *	int a;
> - *	int: 32;
> + *	long: 0;
>   *	int b;
>   *};
>   *
> @@ -28,41 +28,28 @@ struct padded_implicitly {
>  
>  struct padded_explicitly {
>  	int a;
> -	int: 1; /* algo will explicitly pad with full 32 bits here */
> +	int: 1; /* algo will emit aligning `long: 0;` here */
>  	int b;
>  };
>  
>  /* ----- START-EXPECTED-OUTPUT ----- */
> -/*
> - *struct padded_a_lot {
> - *	int a;
> - *	long: 32;
> - *	long: 64;
> - *	long: 64;
> - *	int b;
> - *};
> - *
> - */
> -/* ------ END-EXPECTED-OUTPUT ------ */
> -
>  struct padded_a_lot {
>  	int a;
> -	/* 32 bit of implicit padding here, which algo will make explicit */
>  	long: 64;
>  	long: 64;
>  	int b;
>  };
>  
> +/* ------ END-EXPECTED-OUTPUT ------ */
> +
>  /* ----- START-EXPECTED-OUTPUT ----- */
>  /*
>   *struct padded_cache_line {
>   *	int a;
> - *	long: 32;
>   *	long: 64;
>   *	long: 64;
>   *	long: 64;
>   *	int b;
> - *	long: 32;
>   *	long: 64;
>   *	long: 64;
>   *	long: 64;
> @@ -85,7 +72,7 @@ struct padded_cache_line {
>   *struct zone {
>   *	int a;
>   *	short b;
> - *	short: 16;
> + *	long: 0;
>   *	struct zone_padding __pad__;
>   *};
>   *
> @@ -108,6 +95,39 @@ struct padding_wo_named_members {
>  	long: 64;
>  };
>  
> +struct padding_weird_1 {
> +	int a;
> +	long: 64;
> +	short: 16;
> +	short b;
> +};
> +
> +/* ------ END-EXPECTED-OUTPUT ------ */
> +
> +/* ----- START-EXPECTED-OUTPUT ----- */
> +/*
> + *struct padding_weird_2 {
> + *	long: 56;
> + *	char a;
> + *	long: 56;
> + *	char b;
> + *	char: 8;
> + *};
> + *
> + */
> +/* ------ END-EXPECTED-OUTPUT ------ */
> +struct padding_weird_2 {
> +	int: 32;	/* these paddings will be collapsed into `long: 56;` */
> +	short: 16;
> +	char: 8;
> +	char a;
> +	int: 32;	/* these paddings will be collapsed into `long: 56;` */
> +	short: 16;
> +	char: 8;
> +	char b;
> +	char: 8;
> +};
> +
>  /* ------ END-EXPECTED-OUTPUT ------ */
>  
>  int f(struct {
> @@ -117,6 +137,8 @@ int f(struct {
>  	struct padded_cache_line _4;
>  	struct zone _5;
>  	struct padding_wo_named_members _6;
> +	struct padding_weird_1 _7;
> +	struct padding_weird_2 _8;
>  } *_)
>  {
>  	return 0;

Re: [PATCH v2 bpf-next 5/6] libbpf: fix BTF-to-C converter's padding logic

[Andrii Nakryiko]

On Wed, Dec 14, 2022 at 4:19 PM Eduard Zingerman <eddyz87@gmail.com> 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@ericsson.com/
> >
> > Reported-by: Per Sundström XP <per.xp.sundstrom@ericsson.com>
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  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;

[...]

[PATCH v2 bpf-next 6/6] selftests/bpf: add few corner cases to test padding handling of btf_dump

[Andrii Nakryiko]

Add few hand-crafted cases and few randomized cases found using script
from [0] that tests btf_dump's padding logic.

  [0] https://lore.kernel.org/bpf/85f83c333f5355c8ac026f835b18d15060725fcb.camel@ericsson.com/

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 .../bpf/progs/btf_dump_test_case_packing.c    |  61 +++++++++-
 .../bpf/progs/btf_dump_test_case_padding.c    | 104 ++++++++++++++++++
 2 files changed, 164 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_packing.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_packing.c
index e304b6204bd9..5c6c62f7ed32 100644
--- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_packing.c
+++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_packing.c
@@ -58,7 +58,64 @@ union jump_code_union {
 	} __attribute__((packed));
 };
 
-/*------ END-EXPECTED-OUTPUT ------ */
+/* ----- START-EXPECTED-OUTPUT ----- */
+/*
+ *struct nested_packed_but_aligned_struct {
+ *	int x1;
+ *	int x2;
+ *};
+ *
+ *struct outer_implicitly_packed_struct {
+ *	char y1;
+ *	struct nested_packed_but_aligned_struct y2;
+ *} __attribute__((packed));
+ *
+ */
+/* ------ END-EXPECTED-OUTPUT ------ */
+
+struct nested_packed_but_aligned_struct {
+	int x1;
+	int x2;
+} __attribute__((packed));
+
+struct outer_implicitly_packed_struct {
+	char y1;
+	struct nested_packed_but_aligned_struct y2;
+};
+/* ----- START-EXPECTED-OUTPUT ----- */
+/*
+ *struct usb_ss_ep_comp_descriptor {
+ *	char: 8;
+ *	char bDescriptorType;
+ *	char bMaxBurst;
+ *	short wBytesPerInterval;
+ *};
+ *
+ *struct usb_host_endpoint {
+ *	long: 64;
+ *	char: 8;
+ *	struct usb_ss_ep_comp_descriptor ss_ep_comp;
+ *	long: 0;
+ *} __attribute__((packed));
+ *
+ */
+/* ------ END-EXPECTED-OUTPUT ------ */
+
+struct usb_ss_ep_comp_descriptor {
+	char: 8;
+	char bDescriptorType;
+	char bMaxBurst;
+	int: 0;
+	short wBytesPerInterval;
+} __attribute__((packed));
+
+struct usb_host_endpoint {
+	long: 64;
+	char: 8;
+	struct usb_ss_ep_comp_descriptor ss_ep_comp;
+	long: 0;
+};
+
 
 int f(struct {
 	struct packed_trailing_space _1;
@@ -69,6 +126,8 @@ int f(struct {
 	union union_is_never_packed _6;
 	union union_does_not_need_packing _7;
 	union jump_code_union _8;
+	struct outer_implicitly_packed_struct _9;
+	struct usb_host_endpoint _10;
 } *_)
 {
 	return 0;
diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c
index 6f963d34c45b..79276fbe454a 100644
--- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c
+++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c
@@ -128,6 +128,98 @@ struct padding_weird_2 {
 	char: 8;
 };
 
+/* ----- START-EXPECTED-OUTPUT ----- */
+struct exact_1byte {
+	char x;
+};
+
+struct padded_1byte {
+	char: 8;
+};
+
+struct exact_2bytes {
+	short x;
+};
+
+struct padded_2bytes {
+	short: 16;
+};
+
+struct exact_4bytes {
+	int x;
+};
+
+struct padded_4bytes {
+	int: 32;
+};
+
+struct exact_8bytes {
+	long x;
+};
+
+struct padded_8bytes {
+	long: 64;
+};
+
+struct ff_periodic_effect {
+	int: 32;
+	short magnitude;
+	long: 0;
+	short phase;
+	long: 0;
+	int: 32;
+	int custom_len;
+	short *custom_data;
+};
+
+struct ib_wc {
+	long: 64;
+	long: 64;
+	int: 32;
+	int byte_len;
+	void *qp;
+	union {} ex;
+	long: 64;
+	int slid;
+	int wc_flags;
+	long: 64;
+	char smac[6];
+	long: 0;
+	char network_hdr_type;
+};
+
+struct acpi_object_method {
+	long: 64;
+	char: 8;
+	char type;
+	short reference_count;
+	char flags;
+	short: 0;
+	char: 8;
+	char sync_level;
+	long: 64;
+	void *node;
+	void *aml_start;
+	union {} dispatch;
+	long: 64;
+	int aml_length;
+};
+
+struct nested_unpacked {
+	int x;
+};
+
+struct nested_packed {
+	struct nested_unpacked a;
+	char c;
+} __attribute__((packed));
+
+struct outer_mixed_but_unpacked {
+	struct nested_packed b1;
+	short a1;
+	struct nested_packed b2;
+};
+
 /* ------ END-EXPECTED-OUTPUT ------ */
 
 int f(struct {
@@ -139,6 +231,18 @@ int f(struct {
 	struct padding_wo_named_members _6;
 	struct padding_weird_1 _7;
 	struct padding_weird_2 _8;
+	struct exact_1byte _100;
+	struct padded_1byte _101;
+	struct exact_2bytes _102;
+	struct padded_2bytes _103;
+	struct exact_4bytes _104;
+	struct padded_4bytes _105;
+	struct exact_8bytes _106;
+	struct padded_8bytes _107;
+	struct ff_periodic_effect _200;
+	struct ib_wc _201;
+	struct acpi_object_method _202;
+	struct outer_mixed_but_unpacked _203;
 } *_)
 {
 	return 0;
-- 
2.30.2

Re: [PATCH v2 bpf-next 0/6] BTF-to-C dumper fixes and improvements

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Mon, 12 Dec 2022 13:14:59 -0800 you wrote:
> Fix few tricky issues in libbpf's BTF-to-C converter, discovered thanks to
> Per's reports and his randomized testing script.
> 
> Most notably there is a much improved and correct padding handling.  But also
> it turned out that some corner cases with enums weren't handled correctly
> (mode(byte) attribute was a new discovery for me). See respective patches for
> more details.
> 
> [...]

Here is the summary with links:
  - [v2,bpf-next,1/6] libbpf: fix single-line struct definition output in btf_dump
    https://git.kernel.org/bpf/bpf-next/c/872aec4b5f63
  - [v2,bpf-next,2/6] libbpf: handle non-standardly sized enums better in BTF-to-C dumper
    https://git.kernel.org/bpf/bpf-next/c/21a9a1bcccaa
  - [v2,bpf-next,3/6] selftests/bpf: add non-standardly sized enum tests for btf_dump
    https://git.kernel.org/bpf/bpf-next/c/9d2349740e43
  - [v2,bpf-next,4/6] libbpf: fix btf__align_of() by taking into account field offsets
    https://git.kernel.org/bpf/bpf-next/c/25a4481b4136
  - [v2,bpf-next,5/6] libbpf: fix BTF-to-C converter's padding logic
    https://git.kernel.org/bpf/bpf-next/c/ea2ce1ba99aa
  - [v2,bpf-next,6/6] selftests/bpf: add few corner cases to test padding handling of btf_dump
    https://git.kernel.org/bpf/bpf-next/c/b148c8b9b926

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html