[PATCH bpf-next 0/3] Fix BTF-to-C converter's padding generation

[PATCH bpf-next 0/3] Fix BTF-to-C converter's padding generation

[Andrii Nakryiko]

Fix BTF-to-C logic of handling padding at the end of a struct. Fix existing
test that should have captured this. Also move test_btf_dump into a test_progs
test to leverage common infrastructure.

Andrii Nakryiko (3):
  libbpf: fix struct end padding in btf_dump
  selftests/bpf: convert test_btf_dump into test_progs test
  selftests/bpf: fix btf_dump padding test case

 tools/lib/bpf/btf_dump.c                      |  8 +-
 tools/testing/selftests/bpf/Makefile          |  2 +-
 .../btf_dump.c}                               | 88 +++++++------------
 .../bpf/progs/btf_dump_test_case_padding.c    |  5 +-
 4 files changed, 46 insertions(+), 57 deletions(-)
 rename tools/testing/selftests/bpf/{test_btf_dump.c => prog_tests/btf_dump.c} (51%)

-- 
2.17.1

[PATCH bpf-next 1/3] libbpf: fix struct end padding in btf_dump

[Andrii Nakryiko]

Fix a case where explicit padding at the end of a struct is necessary
due to non-standart alignment requirements of fields (which BTF doesn't
capture explicitly).

Fixes: 351131b51c7a ("libbpf: add btf_dump API for BTF-to-C conversion")
Reported-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/btf_dump.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index ede55fec3618..87f27e2664c5 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -876,7 +876,6 @@ static void btf_dump_emit_struct_def(struct btf_dump *d,
 	__u16 vlen = btf_vlen(t);
 
 	packed = is_struct ? btf_is_struct_packed(d->btf, id, t) : 0;
-	align = packed ? 1 : btf_align_of(d->btf, id);
 
 	btf_dump_printf(d, "%s%s%s {",
 			is_struct ? "struct" : "union",
@@ -906,6 +905,13 @@ static void btf_dump_emit_struct_def(struct btf_dump *d,
 		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 (vlen)
 		btf_dump_printf(d, "\n");
 	btf_dump_printf(d, "%s}", pfx(lvl));
-- 
2.17.1

[PATCH bpf-next 2/3] selftests/bpf: convert test_btf_dump into test_progs test

[Andrii Nakryiko]

Convert test_btf_dump into a part of test_progs, instead of
a stand-alone test binary.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/testing/selftests/bpf/Makefile          |  2 +-
 .../btf_dump.c}                               | 88 +++++++------------
 2 files changed, 35 insertions(+), 55 deletions(-)
 rename tools/testing/selftests/bpf/{test_btf_dump.c => prog_tests/btf_dump.c} (51%)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 771a4e82128b..0de5d6eb62de 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -29,7 +29,7 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
 	test_sock test_btf test_sockmap get_cgroup_id_user test_socket_cookie \
 	test_cgroup_storage test_select_reuseport test_section_names \
 	test_netcnt test_tcpnotify_user test_sock_fields test_sysctl test_hashmap \
-	test_btf_dump test_cgroup_attach xdping
+	test_cgroup_attach xdping
 
 BPF_OBJ_FILES = $(patsubst %.c,%.o, $(notdir $(wildcard progs/*.c)))
 TEST_GEN_FILES = $(BPF_OBJ_FILES)
diff --git a/tools/testing/selftests/bpf/test_btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
similarity index 51%
rename from tools/testing/selftests/bpf/test_btf_dump.c
rename to tools/testing/selftests/bpf/prog_tests/btf_dump.c
index 6e75dd3cb14f..7390d3061065 100644
--- a/tools/testing/selftests/bpf/test_btf_dump.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
@@ -1,36 +1,26 @@
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <unistd.h>
-#include <errno.h>
-#include <linux/err.h>
-#include <btf.h>
-
-#define CHECK(condition, format...) ({					\
-	int __ret = !!(condition);					\
-	if (__ret) {							\
-		fprintf(stderr, "%s:%d:FAIL ", __func__, __LINE__);	\
-		fprintf(stderr, format);				\
-	}								\
-	__ret;								\
-})
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include <bpf/btf.h>
+
+static int duration = 0;
 
 void btf_dump_printf(void *ctx, const char *fmt, va_list args)
 {
 	vfprintf(ctx, fmt, args);
 }
 
-struct btf_dump_test_case {
+static struct btf_dump_test_case {
 	const char *name;
+	const char *file;
 	struct btf_dump_opts opts;
 } btf_dump_test_cases[] = {
-	{.name = "btf_dump_test_case_syntax", .opts = {}},
-	{.name = "btf_dump_test_case_ordering", .opts = {}},
-	{.name = "btf_dump_test_case_padding", .opts = {}},
-	{.name = "btf_dump_test_case_packing", .opts = {}},
-	{.name = "btf_dump_test_case_bitfields", .opts = {}},
-	{.name = "btf_dump_test_case_multidim", .opts = {}},
-	{.name = "btf_dump_test_case_namespacing", .opts = {}},
+	{"btf_dump: syntax", "btf_dump_test_case_syntax", {}},
+	{"btf_dump: ordering", "btf_dump_test_case_ordering", {}},
+	{"btf_dump: padding", "btf_dump_test_case_padding", {}},
+	{"btf_dump: packing", "btf_dump_test_case_packing", {}},
+	{"btf_dump: bitfields", "btf_dump_test_case_bitfields", {}},
+	{"btf_dump: multidim", "btf_dump_test_case_multidim", {}},
+	{"btf_dump: namespacing", "btf_dump_test_case_namespacing", {}},
 };
 
 static int btf_dump_all_types(const struct btf *btf,
@@ -55,55 +45,51 @@ static int btf_dump_all_types(const struct btf *btf,
 	return err;
 }
 
-int test_btf_dump_case(int n, struct btf_dump_test_case *test_case)
+static int test_btf_dump_case(int n, struct btf_dump_test_case *t)
 {
 	char test_file[256], out_file[256], diff_cmd[1024];
 	struct btf *btf = NULL;
 	int err = 0, fd = -1;
 	FILE *f = NULL;
 
-	fprintf(stderr, "Test case #%d (%s): ", n, test_case->name);
-
-	snprintf(test_file, sizeof(test_file), "%s.o", test_case->name);
+	snprintf(test_file, sizeof(test_file), "%s.o", t->file);
 
 	btf = btf__parse_elf(test_file, NULL);
-	if (CHECK(IS_ERR(btf),
+	if (CHECK(IS_ERR(btf), "btf_parse_elf",
 	    "failed to load test BTF: %ld\n", PTR_ERR(btf))) {
 		err = -PTR_ERR(btf);
 		btf = NULL;
 		goto done;
 	}
 
-	snprintf(out_file, sizeof(out_file),
-		 "/tmp/%s.output.XXXXXX", test_case->name);
+	snprintf(out_file, sizeof(out_file), "/tmp/%s.output.XXXXXX", t->file);
 	fd = mkstemp(out_file);
-	if (CHECK(fd < 0, "failed to create temp output file: %d\n", fd)) {
+	if (CHECK(fd < 0, "create_tmp", "failed to create file: %d\n", fd)) {
 		err = fd;
 		goto done;
 	}
 	f = fdopen(fd, "w");
-	if (CHECK(f == NULL, "failed to open temp output file: %s(%d)\n",
+	if (CHECK(f == NULL, "open_tmp",  "failed to open file: %s(%d)\n",
 		  strerror(errno), errno)) {
 		close(fd);
 		goto done;
 	}
 
-	test_case->opts.ctx = f;
-	err = btf_dump_all_types(btf, &test_case->opts);
+	t->opts.ctx = f;
+	err = btf_dump_all_types(btf, &t->opts);
 	fclose(f);
 	close(fd);
-	if (CHECK(err, "failure during C dumping: %d\n", err)) {
+	if (CHECK(err, "btf_dump", "failure during C dumping: %d\n", err)) {
 		goto done;
 	}
 
-	snprintf(test_file, sizeof(test_file), "progs/%s.c", test_case->name);
+	snprintf(test_file, sizeof(test_file), "progs/%s.c", t->file);
 	if (access(test_file, R_OK) == -1)
 		/*
 		 * When the test is run with O=, kselftest copies TEST_FILES
 		 * without preserving the directory structure.
 		 */
-		snprintf(test_file, sizeof(test_file), "%s.c",
-			test_case->name);
+		snprintf(test_file, sizeof(test_file), "%s.c", t->file);
 	/*
 	 * Diff test output and expected test output, contained between
 	 * START-EXPECTED-OUTPUT and END-EXPECTED-OUTPUT lines in test case.
@@ -118,33 +104,27 @@ int test_btf_dump_case(int n, struct btf_dump_test_case *test_case)
 		 "out {sub(/^[ \\t]*\\*/, \"\"); print}' '%s' | diff -u - '%s'",
 		 test_file, out_file);
 	err = system(diff_cmd);
-	if (CHECK(err,
+	if (CHECK(err, "diff",
 		  "differing test output, output=%s, err=%d, diff cmd:\n%s\n",
 		  out_file, err, diff_cmd))
 		goto done;
 
 	remove(out_file);
-	fprintf(stderr, "OK\n");
 
 done:
 	btf__free(btf);
 	return err;
 }
 
-int main() {
-	int test_case_cnt, i, err, failed = 0;
-
-	test_case_cnt = sizeof(btf_dump_test_cases) /
-			sizeof(btf_dump_test_cases[0]);
+void test_btf_dump() {
+	int i;
 
-	for (i = 0; i < test_case_cnt; i++) {
-		err = test_btf_dump_case(i, &btf_dump_test_cases[i]);
-		if (err)
-			failed++;
-	}
+	for (i = 0; i < ARRAY_SIZE(btf_dump_test_cases); i++) {
+		struct btf_dump_test_case *t = &btf_dump_test_cases[i];
 
-	fprintf(stderr, "%d tests succeeded, %d tests failed.\n",
-		test_case_cnt - failed, failed);
+		if (!test__start_subtest(t->name))
+			continue;
 
-	return failed;
+		 test_btf_dump_case(i, &btf_dump_test_cases[i]);
+	}
 }
-- 
2.17.1

[PATCH bpf-next 3/3] selftests/bpf: fix btf_dump padding test case

[Andrii Nakryiko]

Existing padding test case for btf_dump has a good test that was
supposed to test padding generation at the end of a struct, but its
expected output was specified incorrectly. Fix this.

Fixes: 2d2a3ad872f8 ("selftests/bpf: add btf_dump BTF-to-C conversion tests")
Reported-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 .../testing/selftests/bpf/progs/btf_dump_test_case_padding.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

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 3a62119c7498..35c512818a56 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
@@ -62,6 +62,10 @@ struct padded_a_lot {
  *	long: 64;
  *	long: 64;
  *	int b;
+ *	long: 32;
+ *	long: 64;
+ *	long: 64;
+ *	long: 64;
  *};
  *
  */
@@ -95,7 +99,6 @@ struct zone_padding {
 struct zone {
 	int a;
 	short b;
-	short: 16;
 	struct zone_padding __pad__;
 };
 
-- 
2.17.1

RE: [PATCH bpf-next 1/3] libbpf: fix struct end padding in btf_dump

[John Fastabend]

Andrii Nakryiko wrote:
> Fix a case where explicit padding at the end of a struct is necessary
> due to non-standart alignment requirements of fields (which BTF doesn't
> capture explicitly).
> 
> Fixes: 351131b51c7a ("libbpf: add btf_dump API for BTF-to-C conversion")
> Reported-by: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---

Tested a few different kernels and seems to have resolved the issue
I was seeing.

Tested-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH bpf-next 0/3] Fix BTF-to-C converter's padding generation

[Alexei Starovoitov]

On Tue, Oct 8, 2019 at 4:12 PM Andrii Nakryiko <andriin@fb.com> wrote:
>
> Fix BTF-to-C logic of handling padding at the end of a struct. Fix existing
> test that should have captured this. Also move test_btf_dump into a test_progs
> test to leverage common infrastructure.

Applied.
But I see some build weirdness.
Probably due to some previous patches.
Parallel build in selftests/bpf/ is not always succeeding.
For this particular set. New progs/*.c files failed to build the first time.

Re: [PATCH bpf-next 0/3] Fix BTF-to-C converter's padding generation

[Andrii Nakryiko]

On Wed, Oct 9, 2019 at 3:49 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Oct 8, 2019 at 4:12 PM Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > Fix BTF-to-C logic of handling padding at the end of a struct. Fix existing
> > test that should have captured this. Also move test_btf_dump into a test_progs
> > test to leverage common infrastructure.
>
> Applied.
> But I see some build weirdness.
> Probably due to some previous patches.
> Parallel build in selftests/bpf/ is not always succeeding.
> For this particular set. New progs/*.c files failed to build the first time.

I think it's another problem with test_attach_probe.c - it depends on
libbpf's auto-generated bpf_helper_defs.h, which is not enforced to
happen before progs/*.c are built. I think it's time to do some
selftests/bpf/Makefile overhaul, honestly. But I'll try to come up
with a quick fix for this particular issue as well.