[PATCH bpf-next v3 0/2] Fix btf dump error caused by declaration

[PATCH bpf-next v3 0/2] Fix btf dump error caused by declaration

[Xu Kuohai]

This series fixes a btf dump error caused by forward declaration.

Currently if a declaration appears in the BTF before the definition,
the definition is dumped as a conflicting name, eg:

    $ bpftool btf dump file vmlinux format raw | grep "'unix_sock'"
    [81287] FWD 'unix_sock' fwd_kind=struct
    [89336] STRUCT 'unix_sock' size=1024 vlen=14

    $ bpftool btf dump file vmlinux format c | grep "struct unix_sock"
    struct unix_sock;
    struct unix_sock___2 {	<--- conflict, the "___2" is unexpected
		    struct unix_sock___2 *unix_sk;

This causes a "definition not found" compilation error if the dump output
is used as a header file.

Xu Kuohai (2):
  libbpf: Skip forward declaration when counting duplicated type names
  selftests/bpf: Update btf_dump case for conflicting names

 tools/lib/bpf/btf_dump.c                      |  5 ++
 .../selftests/bpf/prog_tests/btf_dump.c       | 54 ++++++++++++++-----
 2 files changed, 46 insertions(+), 13 deletions(-)

-- 
2.30.2

[PATCH bpf-next v3 1/2] libbpf: Skip forward declaration when counting duplicated type names

[Xu Kuohai]

Currently if a declaration appears in the BTF before the definition, the
definition is dumped as a conflicting name, eg:

    $ bpftool btf dump file vmlinux format raw | grep "'unix_sock'"
    [81287] FWD 'unix_sock' fwd_kind=struct
    [89336] STRUCT 'unix_sock' size=1024 vlen=14

    $ bpftool btf dump file vmlinux format c | grep "struct unix_sock"
    struct unix_sock;
    struct unix_sock___2 {	<--- conflict, the "___2" is unexpected
		    struct unix_sock___2 *unix_sk;

This causes a compilation error if the dump output is used as a header
file.

Fix it by skipping declaration when counting duplicated type names.

Fixes: 351131b51c7a ("libbpf: add btf_dump API for BTF-to-C conversion")
Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
Acked-by: Song Liu <songliubraving@fb.com>
---
 tools/lib/bpf/btf_dump.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index 07ebe70d3a30..6b1bc1f43728 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -1505,6 +1505,11 @@ static const char *btf_dump_resolve_name(struct btf_dump *d, __u32 id,
 	if (s->name_resolved)
 		return *cached_name ? *cached_name : orig_name;
 
+	if (btf_is_fwd(t) || (btf_is_enum(t) && btf_vlen(t) == 0)) {
+		s->name_resolved = 1;
+		return orig_name;
+	}
+
 	dup_cnt = btf_dump_name_dups(d, name_map, orig_name);
 	if (dup_cnt > 1) {
 		const size_t max_len = 256;
-- 
2.30.2

[PATCH bpf-next v3 2/2] selftests/bpf: Update btf_dump case for conflicting names

[Xu Kuohai]

Update btf_dump case for conflicting names caused by forward
declaration.

Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
Acked-by: Song Liu <songliubraving@fb.com>
---
 .../selftests/bpf/prog_tests/btf_dump.c       | 54 ++++++++++++++-----
 1 file changed, 41 insertions(+), 13 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
index 9e26903f9170..a224207cdcc4 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
@@ -148,22 +148,38 @@ static void test_btf_dump_incremental(void)
 
 	/* First, generate BTF corresponding to the following C code:
 	 *
-	 * enum { VAL = 1 };
+	 * enum x;
+	 *
+	 * enum x { X = 1 };
+	 *
+	 * enum { Y = 1 };
+	 *
+	 * struct s;
 	 *
 	 * struct s { int x; };
 	 *
 	 */
+	id = btf__add_enum(btf, "x", 4);
+	ASSERT_EQ(id, 1, "enum_declaration_id");
+	id = btf__add_enum(btf, "x", 4);
+	ASSERT_EQ(id, 2, "named_enum_id");
+	err = btf__add_enum_value(btf, "X", 1);
+	ASSERT_OK(err, "named_enum_val_ok");
+
 	id = btf__add_enum(btf, NULL, 4);
-	ASSERT_EQ(id, 1, "enum_id");
-	err = btf__add_enum_value(btf, "VAL", 1);
-	ASSERT_OK(err, "enum_val_ok");
+	ASSERT_EQ(id, 3, "anon_enum_id");
+	err = btf__add_enum_value(btf, "Y", 1);
+	ASSERT_OK(err, "anon_enum_val_ok");
 
 	id = btf__add_int(btf, "int", 4, BTF_INT_SIGNED);
-	ASSERT_EQ(id, 2, "int_id");
+	ASSERT_EQ(id, 4, "int_id");
+
+	id = btf__add_fwd(btf, "s", BTF_FWD_STRUCT);
+	ASSERT_EQ(id, 5, "fwd_id");
 
 	id = btf__add_struct(btf, "s", 4);
-	ASSERT_EQ(id, 3, "struct_id");
-	err = btf__add_field(btf, "x", 2, 0, 0);
+	ASSERT_EQ(id, 6, "struct_id");
+	err = btf__add_field(btf, "x", 4, 0, 0);
 	ASSERT_OK(err, "field_ok");
 
 	for (i = 1; i < btf__type_cnt(btf); i++) {
@@ -173,11 +189,20 @@ static void test_btf_dump_incremental(void)
 
 	fflush(dump_buf_file);
 	dump_buf[dump_buf_sz] = 0; /* some libc implementations don't do this */
+
 	ASSERT_STREQ(dump_buf,
+"enum x;\n"
+"\n"
+"enum x {\n"
+"	X = 1,\n"
+"};\n"
+"\n"
 "enum {\n"
-"	VAL = 1,\n"
+"	Y = 1,\n"
 "};\n"
 "\n"
+"struct s;\n"
+"\n"
 "struct s {\n"
 "	int x;\n"
 "};\n\n", "c_dump1");
@@ -199,10 +224,12 @@ static void test_btf_dump_incremental(void)
 	fseek(dump_buf_file, 0, SEEK_SET);
 
 	id = btf__add_struct(btf, "s", 4);
-	ASSERT_EQ(id, 4, "struct_id");
-	err = btf__add_field(btf, "x", 1, 0, 0);
+	ASSERT_EQ(id, 5, "struct_id");
+	err = btf__add_field(btf, "x", 2, 0, 0);
+	ASSERT_OK(err, "field_ok");
+	err = btf__add_field(btf, "y", 3, 32, 0);
 	ASSERT_OK(err, "field_ok");
-	err = btf__add_field(btf, "s", 3, 32, 0);
+	err = btf__add_field(btf, "s", 6, 64, 0);
 	ASSERT_OK(err, "field_ok");
 
 	for (i = 1; i < btf__type_cnt(btf); i++) {
@@ -214,9 +241,10 @@ static void test_btf_dump_incremental(void)
 	dump_buf[dump_buf_sz] = 0; /* some libc implementations don't do this */
 	ASSERT_STREQ(dump_buf,
 "struct s___2 {\n"
+"	enum x x;\n"
 "	enum {\n"
-"		VAL___2 = 1,\n"
-"	} x;\n"
+"		Y___2 = 1,\n"
+"	} y;\n"
 "	struct s s;\n"
 "};\n\n" , "c_dump1");
 
-- 
2.30.2

Re: [PATCH bpf-next v3 0/2] Fix btf dump error caused by declaration

[Andrii Nakryiko]

On Thu, Feb 24, 2022 at 3:59 AM Xu Kuohai <xukuohai@huawei.com> wrote:
>
> This series fixes a btf dump error caused by forward declaration.
>
> Currently if a declaration appears in the BTF before the definition,
> the definition is dumped as a conflicting name, eg:
>
>     $ bpftool btf dump file vmlinux format raw | grep "'unix_sock'"
>     [81287] FWD 'unix_sock' fwd_kind=struct
>     [89336] STRUCT 'unix_sock' size=1024 vlen=14
>
>     $ bpftool btf dump file vmlinux format c | grep "struct unix_sock"
>     struct unix_sock;
>     struct unix_sock___2 {      <--- conflict, the "___2" is unexpected
>                     struct unix_sock___2 *unix_sk;
>
> This causes a "definition not found" compilation error if the dump output
> is used as a header file.
>

seems like I replied about test failures on v2 instead of v3 (there
are test failures for v3, though), please check the link in that
reply. But I wanted to also mention that it would be great to keep a
succinct version log in the cover letter with what was changed or
fixed between versions (see other submissions on the mailing list).

> Xu Kuohai (2):
>   libbpf: Skip forward declaration when counting duplicated type names
>   selftests/bpf: Update btf_dump case for conflicting names
>
>  tools/lib/bpf/btf_dump.c                      |  5 ++
>  .../selftests/bpf/prog_tests/btf_dump.c       | 54 ++++++++++++++-----
>  2 files changed, 46 insertions(+), 13 deletions(-)
>
> --
> 2.30.2
>

Re: [PATCH bpf-next v3 0/2] Fix btf dump error caused by declaration

[Xu Kuohai]

On 2022/3/1 9:31, Andrii Nakryiko wrote:
> On Thu, Feb 24, 2022 at 3:59 AM Xu Kuohai <xukuohai@huawei.com> wrote:
>>
>> This series fixes a btf dump error caused by forward declaration.
>>
>> Currently if a declaration appears in the BTF before the definition,
>> the definition is dumped as a conflicting name, eg:
>>
>>      $ bpftool btf dump file vmlinux format raw | grep "'unix_sock'"
>>      [81287] FWD 'unix_sock' fwd_kind=struct
>>      [89336] STRUCT 'unix_sock' size=1024 vlen=14
>>
>>      $ bpftool btf dump file vmlinux format c | grep "struct unix_sock"
>>      struct unix_sock;
>>      struct unix_sock___2 {      <--- conflict, the "___2" is unexpected
>>                      struct unix_sock___2 *unix_sk;
>>
>> This causes a "definition not found" compilation error if the dump output
>> is used as a header file.
>>
> 
> seems like I replied about test failures on v2 instead of v3 (there
> are test failures for v3, though), please check the link in that
> reply. But I wanted to also mention that it would be great to keep a
> succinct version log in the cover letter with what was changed or
> fixed between versions (see other submissions on the mailing list).
> 

Sorry for not describing the change log clearly, will add it in the v4.