diff mbox series

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

Message ID 20201029005902.1706310-3-andrii@kernel.org (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series libbpf: split BTF support | expand

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;
 		}