[PATCH bpf-next v1 0/4] bpftool, selftests: Add tests for C dump and fix a dropped type

[PATCH bpf-next v1 0/4] bpftool, selftests: Add tests for C dump and fix a dropped type

[Ihor Solodrai]

"bpftool btf dump format c" generates the vmlinux.h that BPF programs
are built against, and it has no direct test coverage. Building
against the header is not a substitute for testing it, nothing a
compiler accepts would break the build.

Patch #3 is an example of a bug that might have been caught by a
dedicated "format c" test.

---

Ihor Solodrai (4):
  selftests/bpf: NUL-terminate bpftool command output
  selftests/bpf: Add tests for bpftool btf dump format c
  bpftool: Don't drop a type in the sorted C dump
  selftests/bpf: Check that sorting preserves types in bpftool dump

 tools/bpf/bpftool/btf.c                       |   6 +-
 tools/testing/selftests/bpf/bpftool_helpers.c |   7 +-
 .../bpf/prog_tests/bpftool_btf_dump.c         | 223 ++++++++++++++++++
 3 files changed, 233 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/bpftool_btf_dump.c

-- 
2.55.0

[PATCH bpf-next v1 1/4] selftests/bpf: NUL-terminate bpftool command output

[Ihor Solodrai]

run_command() writes a bpftool command's output into a caller-supplied
buffer, and every caller treats that buffer as a C string.

However fread() reports a byte count and doesn't terminate the
string. The helper does not terminate either, so callers have to zero
the buffer first. prog_tests/bpftool_metadata.c does not, for example.

Read one byte less and terminate in the helper.

Fixes: f21fae577446 ("selftests/bpf: Add a few helpers for bpftool testing")
Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
---
 tools/testing/selftests/bpf/bpftool_helpers.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpftool_helpers.c b/tools/testing/selftests/bpf/bpftool_helpers.c
index 0a2a4f0a2794..c49fdd90eb03 100644
--- a/tools/testing/selftests/bpf/bpftool_helpers.c
+++ b/tools/testing/selftests/bpf/bpftool_helpers.c
@@ -53,6 +53,7 @@ static int run_command(char *args, char *output_buf, size_t output_max_len)
 	static char bpftool_path[PATH_MAX] = {};
 	bool suppress_output = !(output_buf && output_max_len);
 	char command[BPFTOOL_FULL_CMD_MAX_LEN];
+	size_t n;
 	FILE *f;
 	int ret;
 
@@ -68,8 +69,10 @@ static int run_command(char *args, char *output_buf, size_t output_max_len)
 	if (!f)
 		return 1;
 
-	if (!suppress_output)
-		fread(output_buf, 1, output_max_len, f);
+	if (!suppress_output) {
+		n = fread(output_buf, 1, output_max_len - 1, f);
+		output_buf[n] = '\0';
+	}
 	ret = pclose(f);
 
 	return ret;
-- 
2.55.0

[PATCH bpf-next v1 2/4] selftests/bpf: Add tests for bpftool btf dump format c

[Ihor Solodrai]

"bpftool btf dump format c" generates the vmlinux.h that BPF programs
are built against, and it has no test coverage at all. The only
in-tree consumers are build systems, and none of them diff the result.

Building against the header only catches what a compiler rejects.
Missing macros, a missing preserve_access_index pragma and the wrong
padding width all compile.

Add a test for those three, over both sort orderings.

Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
---
 .../bpf/prog_tests/bpftool_btf_dump.c         | 151 ++++++++++++++++++
 1 file changed, 151 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/bpftool_btf_dump.c

diff --git a/tools/testing/selftests/bpf/prog_tests/bpftool_btf_dump.c b/tools/testing/selftests/bpf/prog_tests/bpftool_btf_dump.c
new file mode 100644
index 000000000000..53bb7065b3ca
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/bpftool_btf_dump.c
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include <bpftool_helpers.h>
+#include <bpf/btf.h>
+#include <unistd.h>
+
+#define DUMP_BUF_SZ	(16 * 1024)
+
+static int btf_to_tmpfile(struct btf *btf, char *path, size_t path_sz)
+{
+	const void *raw;
+	ssize_t written;
+	__u32 sz;
+	int fd;
+
+	raw = btf__raw_data(btf, &sz);
+	if (!ASSERT_OK_PTR(raw, "raw_data"))
+		return -1;
+
+	snprintf(path, path_sz, "/tmp/bpftool_btf_dump.XXXXXX");
+	fd = mkstemp(path);
+	if (!ASSERT_OK_FD(fd, "mkstemp"))
+		return -1;
+
+	written = write(fd, raw, sz);
+	close(fd);
+	if (!ASSERT_EQ(written, sz, "write_btf")) {
+		unlink(path);
+		return -1;
+	}
+	return 0;
+}
+
+static char *dump_c(const char *path, bool sorted)
+{
+	char args[MAX_BPFTOOL_CMD_LEN];
+	char *buf;
+	int err;
+
+	buf = malloc(DUMP_BUF_SZ);
+	if (!ASSERT_OK_PTR(buf, "alloc"))
+		return NULL;
+
+	snprintf(args, sizeof(args), "btf dump file %s format c%s",
+		 path, sorted ? "" : " unsorted");
+	err = get_bpftool_command_output(args, buf, DUMP_BUF_SZ);
+	if (!ASSERT_OK(err, "dump")) {
+		free(buf);
+		return NULL;
+	}
+	return buf;
+}
+
+/*
+ * struct holey { int c; <32 bit hole> int tail; };
+ *
+ * One record with a hole, and a 4-byte long to pad it with. How records
+ * themselves are rendered is already covered by the build, so the fixture does
+ * not need to be more elaborate than that.
+ */
+static struct btf *mk_btf(void)
+{
+	struct btf *btf;
+	int id, err;
+
+	btf = btf__new_empty();
+	if (!ASSERT_OK_PTR(btf, "new_empty"))
+		return NULL;
+
+	id = btf__add_int(btf, "int", 4, BTF_INT_SIGNED);
+	if (!ASSERT_EQ(id, 1, "int"))
+		goto err_out;
+
+	id = btf__add_int(btf, "long int", 4, BTF_INT_SIGNED);
+	if (!ASSERT_GT(id, 0, "long"))
+		goto err_out;
+
+	id = btf__add_struct(btf, "holey", 16);
+	if (!ASSERT_GT(id, 0, "struct_holey"))
+		goto err_out;
+
+	err = btf__add_field(btf, "c", 1, 0, 0);
+	if (!ASSERT_OK(err, "holey_c"))
+		goto err_out;
+
+	err = btf__add_field(btf, "tail", 1, 96, 0);
+	if (!ASSERT_OK(err, "holey_tail"))
+		goto err_out;
+
+	btf__set_pointer_size(btf, 4);
+
+	return btf;
+err_out:
+	btf__free(btf);
+	return NULL;
+}
+
+/*
+ * Check only what the selftests build cannot:
+ *   - bpf_helpers.h defines __ksym and __weak as well, and no program uses
+ *     __bpf_fastcall, so losing the macro block changes nothing;
+ *   - building without the preserve_access_index pragma is a supported mode
+ *     (BPF_NO_PRESERVE_ACCESS_INDEX), so losing it only costs CO-RE;
+ *   - the padding width comes from the BTF's pointer size, and a native build
+ *     never runs the host bpftool over a differently sized target's BTF.
+ */
+static void test_dump(const char *path, bool sorted)
+{
+	char *buf;
+
+	buf = dump_c(path, sorted);
+	if (!buf)
+		return;
+
+	ASSERT_HAS_SUBSTR(buf, "#define __ksym __attribute__((section(\".ksyms\")))",
+			  "ksym");
+	ASSERT_HAS_SUBSTR(buf, "#define __weak __attribute__((weak))", "weak");
+	ASSERT_HAS_SUBSTR(buf, "#define __bpf_fastcall __attribute__((bpf_fastcall))",
+			  "bpf_fastcall");
+
+	ASSERT_HAS_SUBSTR(buf, "#ifndef BPF_NO_PRESERVE_ACCESS_INDEX", "pai_guard");
+	ASSERT_HAS_SUBSTR(buf,
+			  "#pragma clang attribute push (__attribute__((preserve_access_index)), apply_to = record)",
+			  "pai_push");
+	ASSERT_HAS_SUBSTR(buf, "#pragma clang attribute pop", "pai_pop");
+
+	ASSERT_HAS_SUBSTR(buf, "long: 32;", "target_ptr_size");
+
+	free(buf);
+}
+
+void test_bpftool_btf_dump(void)
+{
+	char path[PATH_MAX];
+	struct btf *btf;
+
+	btf = mk_btf();
+	if (!btf)
+		return;
+	if (btf_to_tmpfile(btf, path, sizeof(path)))
+		goto out;
+
+	if (test__start_subtest("c_sorted"))
+		test_dump(path, true);
+	if (test__start_subtest("c_unsorted"))
+		test_dump(path, false);
+
+	unlink(path);
+out:
+	btf__free(btf);
+}
-- 
2.55.0

Re: [PATCH bpf-next v1 2/4] selftests/bpf: Add tests for bpftool btf dump format c

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The comment describing the `holey` struct layout contradicts the actual code, creating a 64-bit hole instead of the documented 32-bit hole.
--

--- Patch [2]: [PATCH bpf-next v1 2/4] selftests/bpf: Add tests for bpftool btf dump format c ---
commit 6c049c9e731fda95ed237645c2367be42ed67768
Author: Ihor Solodrai <ihor.solodrai@linux.dev>

selftests/bpf: Add tests for bpftool btf dump format c

This commit adds a selftest for "bpftool btf dump format c" to ensure that
the generated vmlinux.h header contains the correct macros, pragmas, and
padding widths. It verifies these features across both sorted and unsorted
BTF output formats.

> diff --git a/tools/testing/selftests/bpf/prog_tests/bpftool_btf_dump.c b/tools/testing/selftests/bpf/prog_tests/bpftool_btf_dump.c
> new file mode 100644
> index 0000000000000..53bb7065b3cae
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/bpftool_btf_dump.c

[ ... ]

> +/*
> + * struct holey { int c; <32 bit hole> int tail; };
> + *
> + * One record with a hole, and a 4-byte long to pad it with. How records
> + * themselves are rendered is already covered by the build, so the fixture does
> + * not need to be more elaborate than that.
> + */
> +static struct btf *mk_btf(void)
> +{

[ ... ]

> +	id = btf__add_struct(btf, "holey", 16);
> +	if (!ASSERT_GT(id, 0, "struct_holey"))
> +		goto err_out;
> +
> +	err = btf__add_field(btf, "c", 1, 0, 0);
> +	if (!ASSERT_OK(err, "holey_c"))
> +		goto err_out;
> +
> +	err = btf__add_field(btf, "tail", 1, 96, 0);

[Severity: Low]
This isn't a bug, but does this create a 64-bit hole instead of the 32-bit hole
mentioned in the comment above?

Since "c" is a 32-bit integer ending at bit 32 and "tail" starts at bit offset
96, the gap between them appears to be 64 bits.

Could the comment be updated to accurately reflect the actual struct layout
generated here?

> +	if (!ASSERT_OK(err, "holey_tail"))
> +		goto err_out;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820000627.3826188-1-ihor.solodrai@linux.dev?part=2

Re: [PATCH bpf-next v1 2/4] selftests/bpf: Add tests for bpftool btf dump format c

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 1793 bytes --]

> diff --git a/tools/testing/selftests/bpf/prog_tests/bpftool_btf_dump.c b/tools/testing/selftests/bpf/prog_tests/bpftool_btf_dump.c
> new file mode 100644
> index 000000000000..53bb7065b3ca
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/bpftool_btf_dump.c
> @@ -0,0 +1,151 @@

[ ... ]

> +static struct btf *mk_btf(void)
> +{
> +	struct btf *btf;
> +	int id, err;
> +
> +	btf = btf__new_empty();
> +	if (!ASSERT_OK_PTR(btf, "new_empty"))
> +		return NULL;
> +
> +	id = btf__add_int(btf, "int", 4, BTF_INT_SIGNED);
> +	if (!ASSERT_EQ(id, 1, "int"))
> +		goto err_out;
> +
> +	id = btf__add_int(btf, "long int", 4, BTF_INT_SIGNED);
> +	if (!ASSERT_GT(id, 0, "long"))
> +		goto err_out;
> +
> +	id = btf__add_struct(btf, "holey", 16);
> +	if (!ASSERT_GT(id, 0, "struct_holey"))
> +		goto err_out;
> +
> +	err = btf__add_field(btf, "c", 1, 0, 0);
> +	if (!ASSERT_OK(err, "holey_c"))
> +		goto err_out;
> +
> +	err = btf__add_field(btf, "tail", 1, 96, 0);
> +	if (!ASSERT_OK(err, "holey_tail"))
> +		goto err_out;
> +
> +	btf__set_pointer_size(btf, 4);

This isn't a bug, but since the pointer size isn't part of the serialized
BTF and bpftool re-derives it from the 4-byte "long int" type, does the
btf__set_pointer_size() call still do anything here, or could it go?

The BTF wire format has no pointer-size field, so btf__raw_data() doesn't
carry it into the temp file. When bpftool re-parses the file it recomputes
the value with determine_ptr_size().

> +
> +	return btf;
> +err_out:
> +	btf__free(btf);
> +	return NULL;
> +}

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32317197542

[PATCH bpf-next v1 3/4] bpftool: Don't drop a type in the sorted C dump

[Ihor Solodrai]

The C dump sorts types by default, so that generated headers are
diffable. The sorted dump emits one type fewer than the unsorted dump
of the same BTF.

dump_btf_c() starts its loop at index 1 to skip the void type at BTF
type ID 0. That holds for the unsorted dump, where the array index is
the type ID, but not after qsort(): position 0 is then the lowest ranked
type, and btf_type_rank() ranks an anonymous enum 0 while void takes the
default rank of 10. So the enum is skipped, and void is emitted instead
as a no-op.

Skip by type ID rather than by position.

Fixes: 94133cf24bb3 ("bpftool: Introduce btf c dump sorting")
Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
---
 tools/bpf/bpftool/btf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index c9589026da8d..345d49a9a22c 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -805,9 +805,13 @@ static int dump_btf_c(const struct btf *btf,
 
 		if (sort_dump)
 			datums = sort_btf_c(btf);
-		for (i = 1; i < cnt; i++) {
+		for (i = 0; i < cnt; i++) {
 			int idx = datums ? datums[i].index : i;
 
+			/* type ID 0 is void, skip it */
+			if (!idx)
+				continue;
+
 			err = btf_dump__dump_type(d, idx);
 			if (err)
 				goto done;
-- 
2.55.0

[PATCH bpf-next v1 4/4] selftests/bpf: Check that sorting preserves types in bpftool dump

[Ihor Solodrai]

Sorting the C dump is meant to make generated headers diffable. The
sorted and unsorted dumps of the same BTF have to emit the same types.

Extend the fixture with a few more top-level definitions, including
anonymous enums, and add a subtest comparing how many types are
emitted with or without sort.

Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
---
 .../bpf/prog_tests/bpftool_btf_dump.c         | 78 ++++++++++++++++++-
 1 file changed, 75 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/bpftool_btf_dump.c b/tools/testing/selftests/bpf/prog_tests/bpftool_btf_dump.c
index 53bb7065b3ca..28059820ea0b 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpftool_btf_dump.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpftool_btf_dump.c
@@ -53,15 +53,18 @@ static char *dump_c(const char *path, bool sorted)
 
 /*
  * struct holey { int c; <32 bit hole> int tail; };
+ * enum { E0 = 1 }; enum { E1 = 2 }; enum { E2 = 3 };
+ * struct s { int f; };
+ * union u { int f; };
  *
  * One record with a hole, and a 4-byte long to pad it with. How records
- * themselves are rendered is already covered by the build, so the fixture does
- * not need to be more elaborate than that.
+ * themselves are rendered is already covered by the build.
+ * The anonymous enums matter to the sorting subtest.
  */
 static struct btf *mk_btf(void)
 {
 	struct btf *btf;
-	int id, err;
+	int id, err, i;
 
 	btf = btf__new_empty();
 	if (!ASSERT_OK_PTR(btf, "new_empty"))
@@ -87,6 +90,36 @@ static struct btf *mk_btf(void)
 	if (!ASSERT_OK(err, "holey_tail"))
 		goto err_out;
 
+	for (i = 0; i < 3; i++) {
+		char name[16];
+
+		snprintf(name, sizeof(name), "E%d", i);
+
+		id = btf__add_enum(btf, NULL, 4);
+		if (!ASSERT_GT(id, 0, "anon_enum"))
+			goto err_out;
+
+		err = btf__add_enum_value(btf, name, i + 1);
+		if (!ASSERT_OK(err, "enum_val"))
+			goto err_out;
+	}
+
+	id = btf__add_struct(btf, "s", 4);
+	if (!ASSERT_GT(id, 0, "struct_s"))
+		goto err_out;
+
+	err = btf__add_field(btf, "f", 1, 0, 0);
+	if (!ASSERT_OK(err, "s_field"))
+		goto err_out;
+
+	id = btf__add_union(btf, "u", 4);
+	if (!ASSERT_GT(id, 0, "union_u"))
+		goto err_out;
+
+	err = btf__add_field(btf, "f", 1, 0, 0);
+	if (!ASSERT_OK(err, "u_field"))
+		goto err_out;
+
 	btf__set_pointer_size(btf, 4);
 
 	return btf;
@@ -95,6 +128,42 @@ static struct btf *mk_btf(void)
 	return NULL;
 }
 
+static int count_substr(const char *haystack, const char *needle)
+{
+	const char *p = haystack;
+	int n = 0;
+
+	for (; (p = strstr(p, needle)); p++)
+		n++;
+
+	return n;
+}
+
+static void test_sort_preserves_types(const char *path)
+{
+	char *sorted = NULL, *unsorted = NULL;
+	int n_sorted, n_unsorted;
+
+	sorted = dump_c(path, true);
+	unsorted = dump_c(path, false);
+	if (!sorted || !unsorted)
+		goto out;
+
+	/*
+	 * Count definitions by the closing brace in their first column. Counting
+	 * "struct"/"union"/"enum" openers instead would also count forward
+	 * declarations, and which types need one depends on emission order.
+	 */
+	n_unsorted = count_substr(unsorted, "\n}");
+	n_sorted = count_substr(sorted, "\n}");
+	ASSERT_GT(n_unsorted, 0, "types_emitted");
+	ASSERT_EQ(n_sorted, n_unsorted, "same_type_count");
+
+out:
+	free(sorted);
+	free(unsorted);
+}
+
 /*
  * Check only what the selftests build cannot:
  *   - bpf_helpers.h defines __ksym and __weak as well, and no program uses
@@ -145,6 +214,9 @@ void test_bpftool_btf_dump(void)
 	if (test__start_subtest("c_unsorted"))
 		test_dump(path, false);
 
+	if (test__start_subtest("c_sort_preserves_types"))
+		test_sort_preserves_types(path);
+
 	unlink(path);
 out:
 	btf__free(btf);
-- 
2.55.0