diff mbox series

[bpf-next,02/11] selftest/bpf: relax btf_dedup test checks

Message ID 20201029005902.1706310-3-andrii@kernel.org
State Not Applicable
Delegated to: BPF Maintainers
Headers show
Series libbpf: split BTF support | expand

Checks

Context Check Description
jkicinski/cover_letter success Link
jkicinski/fixes_present success Link
jkicinski/patch_count success Link
jkicinski/tree_selection success Clearly marked for bpf-next
jkicinski/subject_prefix success Link
jkicinski/source_inline success Was 0 now: 0
jkicinski/verify_signedoff success Link
jkicinski/module_param success Was 0 now: 0
jkicinski/build_32bit fail Errors and warnings before: 4 this patch: 4
jkicinski/kdoc success Errors and warnings before: 0 this patch: 0
jkicinski/verify_fixes success Link
jkicinski/checkpatch fail Link
jkicinski/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
jkicinski/header_inline success Link
jkicinski/stable success Stable not CCed

Commit Message

Andrii Nakryiko Oct. 29, 2020, 12:58 a.m. UTC
Remove the requirement of a strictly exact string section contents. This used
to be true when string deduplication was done through sorting, but with string
dedup done through hash table, it's no longer true. So relax test harness to
relax strings checks and, consequently, type checks, which now don't have to
have exactly the same string offsets.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/prog_tests/btf.c | 34 +++++++++++---------
 1 file changed, 19 insertions(+), 15 deletions(-)

Comments

Song Liu Oct. 30, 2020, 4:43 p.m. UTC | #1
On Thu, Oct 29, 2020 at 1:40 AM Andrii Nakryiko <andrii@kernel.org> wrote:
>
> Remove the requirement of a strictly exact string section contents. This used
> to be true when string deduplication was done through sorting, but with string
> dedup done through hash table, it's no longer true. So relax test harness to
> relax strings checks and, consequently, type checks, which now don't have to
> have exactly the same string offsets.
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  tools/testing/selftests/bpf/prog_tests/btf.c | 34 +++++++++++---------
>  1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c
> index 93162484c2ca..2ccc23b2a36f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/btf.c
> +++ b/tools/testing/selftests/bpf/prog_tests/btf.c
> @@ -6652,7 +6652,7 @@ static void do_test_dedup(unsigned int test_num)
>         const void *test_btf_data, *expect_btf_data;
>         const char *ret_test_next_str, *ret_expect_next_str;
>         const char *test_strs, *expect_strs;
> -       const char *test_str_cur, *test_str_end;
> +       const char *test_str_cur;
>         const char *expect_str_cur, *expect_str_end;
>         unsigned int raw_btf_size;
>         void *raw_btf;
> @@ -6719,12 +6719,18 @@ static void do_test_dedup(unsigned int test_num)
>                 goto done;
>         }
>
> -       test_str_cur = test_strs;
> -       test_str_end = test_strs + test_hdr->str_len;
>         expect_str_cur = expect_strs;
>         expect_str_end = expect_strs + expect_hdr->str_len;
> -       while (test_str_cur < test_str_end && expect_str_cur < expect_str_end) {
> +       while (expect_str_cur < expect_str_end) {
>                 size_t test_len, expect_len;
> +               int off;
> +
> +               off = btf__find_str(test_btf, expect_str_cur);
> +               if (CHECK(off < 0, "exp str '%s' not found: %d\n", expect_str_cur, off)) {
> +                       err = -1;
> +                       goto done;
> +               }
> +               test_str_cur = btf__str_by_offset(test_btf, off);
>
>                 test_len = strlen(test_str_cur);
>                 expect_len = strlen(expect_str_cur);
> @@ -6741,15 +6747,8 @@ static void do_test_dedup(unsigned int test_num)
>                         err = -1;
>                         goto done;
>                 }
> -               test_str_cur += test_len + 1;
>                 expect_str_cur += expect_len + 1;
>         }
> -       if (CHECK(test_str_cur != test_str_end,
> -                 "test_str_cur:%p != test_str_end:%p",
> -                 test_str_cur, test_str_end)) {
> -               err = -1;
> -               goto done;
> -       }
>
>         test_nr_types = btf__get_nr_types(test_btf);
>         expect_nr_types = btf__get_nr_types(expect_btf);
> @@ -6775,10 +6774,15 @@ static void do_test_dedup(unsigned int test_num)
>                         err = -1;
>                         goto done;
>                 }
> -               if (CHECK(memcmp((void *)test_type,
> -                                (void *)expect_type,
> -                                test_size),
> -                         "type #%d: contents differ", i)) {

I guess test_size and expect_size are not needed anymore?

> +               if (CHECK(btf_kind(test_type) != btf_kind(expect_type),
> +                         "type %d kind: exp %d != got %u\n",
> +                         i, btf_kind(expect_type), btf_kind(test_type))) {
> +                       err = -1;
> +                       goto done;
> +               }
> +               if (CHECK(test_type->info != expect_type->info,
> +                         "type %d info: exp %d != got %u\n",
> +                         i, expect_type->info, test_type->info)) {

btf_kind() returns part of ->info, so we only need the second check, no?

IIUC, test_type and expect_type may have different name_off now. Shall
we check ->size matches?


>                         err = -1;
>                         goto done;
>                 }
> --
> 2.24.1
>
Andrii Nakryiko Oct. 30, 2020, 6:44 p.m. UTC | #2
On Fri, Oct 30, 2020 at 9:43 AM Song Liu <song@kernel.org> wrote:
>
> On Thu, Oct 29, 2020 at 1:40 AM Andrii Nakryiko <andrii@kernel.org> wrote:
> >
> > Remove the requirement of a strictly exact string section contents. This used
> > to be true when string deduplication was done through sorting, but with string
> > dedup done through hash table, it's no longer true. So relax test harness to
> > relax strings checks and, consequently, type checks, which now don't have to
> > have exactly the same string offsets.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  tools/testing/selftests/bpf/prog_tests/btf.c | 34 +++++++++++---------
> >  1 file changed, 19 insertions(+), 15 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c
> > index 93162484c2ca..2ccc23b2a36f 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/btf.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/btf.c
> > @@ -6652,7 +6652,7 @@ static void do_test_dedup(unsigned int test_num)
> >         const void *test_btf_data, *expect_btf_data;
> >         const char *ret_test_next_str, *ret_expect_next_str;
> >         const char *test_strs, *expect_strs;
> > -       const char *test_str_cur, *test_str_end;
> > +       const char *test_str_cur;
> >         const char *expect_str_cur, *expect_str_end;
> >         unsigned int raw_btf_size;
> >         void *raw_btf;
> > @@ -6719,12 +6719,18 @@ static void do_test_dedup(unsigned int test_num)
> >                 goto done;
> >         }
> >
> > -       test_str_cur = test_strs;
> > -       test_str_end = test_strs + test_hdr->str_len;
> >         expect_str_cur = expect_strs;
> >         expect_str_end = expect_strs + expect_hdr->str_len;
> > -       while (test_str_cur < test_str_end && expect_str_cur < expect_str_end) {
> > +       while (expect_str_cur < expect_str_end) {
> >                 size_t test_len, expect_len;
> > +               int off;
> > +
> > +               off = btf__find_str(test_btf, expect_str_cur);
> > +               if (CHECK(off < 0, "exp str '%s' not found: %d\n", expect_str_cur, off)) {
> > +                       err = -1;
> > +                       goto done;
> > +               }
> > +               test_str_cur = btf__str_by_offset(test_btf, off);
> >
> >                 test_len = strlen(test_str_cur);
> >                 expect_len = strlen(expect_str_cur);
> > @@ -6741,15 +6747,8 @@ static void do_test_dedup(unsigned int test_num)
> >                         err = -1;
> >                         goto done;
> >                 }
> > -               test_str_cur += test_len + 1;
> >                 expect_str_cur += expect_len + 1;
> >         }
> > -       if (CHECK(test_str_cur != test_str_end,
> > -                 "test_str_cur:%p != test_str_end:%p",
> > -                 test_str_cur, test_str_end)) {
> > -               err = -1;
> > -               goto done;
> > -       }
> >
> >         test_nr_types = btf__get_nr_types(test_btf);
> >         expect_nr_types = btf__get_nr_types(expect_btf);
> > @@ -6775,10 +6774,15 @@ static void do_test_dedup(unsigned int test_num)
> >                         err = -1;
> >                         goto done;
> >                 }
> > -               if (CHECK(memcmp((void *)test_type,
> > -                                (void *)expect_type,
> > -                                test_size),
> > -                         "type #%d: contents differ", i)) {
>
> I guess test_size and expect_size are not needed anymore?

hm.. they are used just one check above, still needed

>
> > +               if (CHECK(btf_kind(test_type) != btf_kind(expect_type),
> > +                         "type %d kind: exp %d != got %u\n",
> > +                         i, btf_kind(expect_type), btf_kind(test_type))) {
> > +                       err = -1;
> > +                       goto done;
> > +               }
> > +               if (CHECK(test_type->info != expect_type->info,
> > +                         "type %d info: exp %d != got %u\n",
> > +                         i, expect_type->info, test_type->info)) {
>
> btf_kind() returns part of ->info, so we only need the second check, no?

technically yes, but when kind mismatches, figuring that out from raw
info field is quite painful, so having a better, more targeted check
is still good.

>
> IIUC, test_type and expect_type may have different name_off now. Shall
> we check ->size matches?

yep, sure, I'll add

>
>
> >                         err = -1;
> >                         goto done;
> >                 }
> > --
> > 2.24.1
> >
Song Liu Oct. 30, 2020, 10:30 p.m. UTC | #3
On Fri, Oct 30, 2020 at 11:45 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
[...]
> > > @@ -6775,10 +6774,15 @@ static void do_test_dedup(unsigned int test_num)
> > >                         err = -1;
> > >                         goto done;
> > >                 }
> > > -               if (CHECK(memcmp((void *)test_type,
> > > -                                (void *)expect_type,
> > > -                                test_size),
> > > -                         "type #%d: contents differ", i)) {
> >
> > I guess test_size and expect_size are not needed anymore?
>
> hm.. they are used just one check above, still needed

Hmm... I don't know what happened to me back then.. Please ignore.

>
> >
> > > +               if (CHECK(btf_kind(test_type) != btf_kind(expect_type),
> > > +                         "type %d kind: exp %d != got %u\n",
> > > +                         i, btf_kind(expect_type), btf_kind(test_type))) {
> > > +                       err = -1;
> > > +                       goto done;
> > > +               }
> > > +               if (CHECK(test_type->info != expect_type->info,
> > > +                         "type %d info: exp %d != got %u\n",
> > > +                         i, expect_type->info, test_type->info)) {
> >
> > btf_kind() returns part of ->info, so we only need the second check, no?
>
> technically yes, but when kind mismatches, figuring that out from raw
> info field is quite painful, so having a better, more targeted check
> is still good.

Fair enough. We can have a more clear check.

Thanks,
Song
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c
index 93162484c2ca..2ccc23b2a36f 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf.c
@@ -6652,7 +6652,7 @@  static void do_test_dedup(unsigned int test_num)
 	const void *test_btf_data, *expect_btf_data;
 	const char *ret_test_next_str, *ret_expect_next_str;
 	const char *test_strs, *expect_strs;
-	const char *test_str_cur, *test_str_end;
+	const char *test_str_cur;
 	const char *expect_str_cur, *expect_str_end;
 	unsigned int raw_btf_size;
 	void *raw_btf;
@@ -6719,12 +6719,18 @@  static void do_test_dedup(unsigned int test_num)
 		goto done;
 	}
 
-	test_str_cur = test_strs;
-	test_str_end = test_strs + test_hdr->str_len;
 	expect_str_cur = expect_strs;
 	expect_str_end = expect_strs + expect_hdr->str_len;
-	while (test_str_cur < test_str_end && expect_str_cur < expect_str_end) {
+	while (expect_str_cur < expect_str_end) {
 		size_t test_len, expect_len;
+		int off;
+
+		off = btf__find_str(test_btf, expect_str_cur);
+		if (CHECK(off < 0, "exp str '%s' not found: %d\n", expect_str_cur, off)) {
+			err = -1;
+			goto done;
+		}
+		test_str_cur = btf__str_by_offset(test_btf, off);
 
 		test_len = strlen(test_str_cur);
 		expect_len = strlen(expect_str_cur);
@@ -6741,15 +6747,8 @@  static void do_test_dedup(unsigned int test_num)
 			err = -1;
 			goto done;
 		}
-		test_str_cur += test_len + 1;
 		expect_str_cur += expect_len + 1;
 	}
-	if (CHECK(test_str_cur != test_str_end,
-		  "test_str_cur:%p != test_str_end:%p",
-		  test_str_cur, test_str_end)) {
-		err = -1;
-		goto done;
-	}
 
 	test_nr_types = btf__get_nr_types(test_btf);
 	expect_nr_types = btf__get_nr_types(expect_btf);
@@ -6775,10 +6774,15 @@  static void do_test_dedup(unsigned int test_num)
 			err = -1;
 			goto done;
 		}
-		if (CHECK(memcmp((void *)test_type,
-				 (void *)expect_type,
-				 test_size),
-			  "type #%d: contents differ", i)) {
+		if (CHECK(btf_kind(test_type) != btf_kind(expect_type),
+			  "type %d kind: exp %d != got %u\n",
+			  i, btf_kind(expect_type), btf_kind(test_type))) {
+			err = -1;
+			goto done;
+		}
+		if (CHECK(test_type->info != expect_type->info,
+			  "type %d info: exp %d != got %u\n",
+			  i, expect_type->info, test_type->info)) {
 			err = -1;
 			goto done;
 		}