| Message ID | 20201029005902.1706310-3-andrii@kernel.org |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | libbpf: split BTF support | expand |
| 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 |
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 >
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 > >
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 --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; }
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(-)