On Sat, Feb 19, 2022 at 6:25 PM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
>
> There turned out to be a few problems with btfgen selftests.
>
> First, core_btfgen tests are failing in BPF CI due to the use of
> full-featured bpftool, which has extra dependencies on libbfd, libcap,
> etc, which are present in BPF CI's build environment, but those shared
> libraries are missing in QEMU image in which test_progs is running.
>
> To fix this problem, use minimal bootstrap version of bpftool instead.
> It only depend on libelf and libz, same as libbpf, so doesn't add any
> new requirements (and bootstrap bpftool still implementes entire
> `bpftool gen` functionality, which is quite convenient).
>
> Second problem is even more interesting. Both core_btfgen and core_reloc
> reuse the same set of struct core_reloc_test_case array of test case
> definitions. That in itself is not a problem, but btfgen test replaces
> test_case->btf_src_file property with the path to temporary file into
> which minimized BTF is output by bpftool. This interferes with original
> core_reloc tests, depending on order of tests execution (core_btfgen is
> run first in sequential mode and skrews up subsequent core_reloc run by
> pointing to already deleted temporary file, instead of the original BTF
> files) and whether those two runs share the same process (in parallel
> mode the chances are high for them to run in two separate processes and
> so not interfere with each other).
>
> To prevent this interference, create and use local copy of a test
> definition. Mark original array as constant to catch accidental
> modifcations. Note that setup_type_id_case_success() and
> setup_type_id_case_success() still modify common test_case->output
> memory area, but it is ok as each setup function has to re-initialize it
> completely anyways. In sequential mode it leads to deterministic and
> correct initialization. In parallel mode they will either each have
> their own process, or if core_reloc and core_btfgen happen to be run by
> the same worker process, they will still do that sequentially within the
> worker process. If they are sharded across multiple processes, they
> don't really share anything anyways.
>
> Also, rename core_btfgen into core_reloc_btfgen, as it is indeed just
> a "flavor" of core_reloc test, not an independent set of tests. So make
> it more obvious.
>
> Cc: Mauricio Vásquez <mauricio@xxxxxxxxxx>
> Fixes: 704c91e59fe0 ("selftests/bpf: Test "bpftool gen min_core_btf")
> Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> ---
> .../selftests/bpf/prog_tests/core_reloc.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/core_reloc.c b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
> index 8fbb40a832d5..5c5e5e72d9fe 100644
> --- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c
> +++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
> @@ -512,7 +512,7 @@ static int __trigger_module_test_read(const struct core_reloc_test_case *test)
> }
>
>
> -static struct core_reloc_test_case test_cases[] = {
> +static const struct core_reloc_test_case test_cases[] = {
> /* validate we can find kernel image and use its BTF for relocs */
> {
> .case_name = "kernel",
> @@ -843,7 +843,7 @@ static int run_btfgen(const char *src_btf, const char *dst_btf, const char *objp
> int n;
>
> n = snprintf(command, sizeof(command),
> - "./tools/build/bpftool/bpftool gen min_core_btf %s %s %s",
> + "./tools/build/bpftool/bootstrap/bpftool gen min_core_btf %s %s %s",
> src_btf, dst_btf, objpath);
> if (n < 0 || n >= sizeof(command))
> return -1;
> @@ -855,7 +855,7 @@ static void run_core_reloc_tests(bool use_btfgen)
> {
> const size_t mmap_sz = roundup_page(sizeof(struct data));
> DECLARE_LIBBPF_OPTS(bpf_object_open_opts, open_opts);
> - struct core_reloc_test_case *test_case;
> + struct core_reloc_test_case *test_case, test_case_copy;
> const char *tp_name, *probe_name;
> int err, i, equal, fd;
> struct bpf_link *link = NULL;
> @@ -870,7 +870,10 @@ static void run_core_reloc_tests(bool use_btfgen)
>
> for (i = 0; i < ARRAY_SIZE(test_cases); i++) {
> char btf_file[] = "/tmp/core_reloc.btf.XXXXXX";
> - test_case = &test_cases[i];
> +
> + test_case_copy = test_cases[i];
> + test_case = &test_case_copy;
> +
> if (!test__start_subtest(test_case->case_name))
> continue;
>
> @@ -881,6 +884,7 @@ static void run_core_reloc_tests(bool use_btfgen)
>
> /* generate a "minimal" BTF file and use it as source */
> if (use_btfgen) {
> +
argh, unnecessary leftover from my local changes. Hopefully it can be
cleaned up during applying so that I don't spam the mailing list
unnecessarily.
> if (!test_case->btf_src_file || test_case->fails) {
> test__skip();
> continue;
> @@ -989,7 +993,8 @@ static void run_core_reloc_tests(bool use_btfgen)
> CHECK_FAIL(munmap(mmap_data, mmap_sz));
> mmap_data = NULL;
> }
> - remove(btf_file);
> + if (use_btfgen)
> + remove(test_case->btf_src_file);
> bpf_link__destroy(link);
> link = NULL;
> bpf_object__close(obj);
> @@ -1001,7 +1006,7 @@ void test_core_reloc(void)
> run_core_reloc_tests(false);
> }
>
> -void test_core_btfgen(void)
> +void test_core_reloc_btfgen(void)
> {
> run_core_reloc_tests(true);
> }
> --
> 2.30.2
>
