On Mon, Nov 27, 2023 at 11:49 AM Yonghong Song <yonghong.song@xxxxxxxxx> wrote: > > > On 11/27/23 1:49 PM, Andrii Nakryiko wrote: > > On Sun, Nov 26, 2023 at 9:04 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote: > >> With latest upstream llvm18, the following test cases failed: > >> $ ./test_progs -j > >> #13/2 bpf_cookie/multi_kprobe_link_api:FAIL > >> #13/3 bpf_cookie/multi_kprobe_attach_api:FAIL > >> #13 bpf_cookie:FAIL > >> #77 fentry_fexit:FAIL > >> #78/1 fentry_test/fentry:FAIL > >> #78 fentry_test:FAIL > >> #82/1 fexit_test/fexit:FAIL > >> #82 fexit_test:FAIL > >> #112/1 kprobe_multi_test/skel_api:FAIL > >> #112/2 kprobe_multi_test/link_api_addrs:FAIL > >> ... > >> #112 kprobe_multi_test:FAIL > >> #356/17 test_global_funcs/global_func17:FAIL > >> #356 test_global_funcs:FAIL > >> > >> Further analysis shows llvm upstream patch [1] is responsible > >> for the above failures. For example, for function bpf_fentry_test7() > >> in net/bpf/test_run.c, without [1], the asm code is: > >> 0000000000000400 <bpf_fentry_test7>: > >> 400: f3 0f 1e fa endbr64 > >> 404: e8 00 00 00 00 callq 0x409 <bpf_fentry_test7+0x9> > >> 409: 48 89 f8 movq %rdi, %rax > >> 40c: c3 retq > >> 40d: 0f 1f 00 nopl (%rax) > >> and with [1], the asm code is: > >> 0000000000005d20 <bpf_fentry_test7.specialized.1>: > >> 5d20: e8 00 00 00 00 callq 0x5d25 <bpf_fentry_test7.specialized.1+0x5> > >> 5d25: c3 retq > >> and <bpf_fentry_test7.specialized.1> is called instead of <bpf_fentry_test7> > >> and this caused test failures for #13/#77 etc. except #356. > >> > >> For test case #356/17, with [1] (progs/test_global_func17.c)), > >> the main prog looks like: > >> 0000000000000000 <global_func17>: > >> 0: b4 00 00 00 2a 00 00 00 w0 = 0x2a > >> 1: 95 00 00 00 00 00 00 00 exit > >> which passed verification while the test itself expects a verification > >> failure. > >> > >> Let us add 'barrier_var' style asm code in both places to prevent > >> function specialization which caused selftests failure. > >> > >> [1] https://github.com/llvm/llvm-project/pull/72903 > >> > >> Signed-off-by: Yonghong Song <yonghong.song@xxxxxxxxx> > >> --- > >> net/bpf/test_run.c | 2 +- > >> tools/testing/selftests/bpf/progs/test_global_func17.c | 1 + > >> 2 files changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > >> index c9fdcc5cdce1..711cf5d59816 100644 > >> --- a/net/bpf/test_run.c > >> +++ b/net/bpf/test_run.c > >> @@ -542,7 +542,7 @@ struct bpf_fentry_test_t { > >> > >> int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg) > >> { > >> - asm volatile (""); > >> + asm volatile ("": "+r"(arg)); > >> return (long)arg; > >> } > >> > >> diff --git a/tools/testing/selftests/bpf/progs/test_global_func17.c b/tools/testing/selftests/bpf/progs/test_global_func17.c > >> index a32e11c7d933..5de44b09e8ec 100644 > >> --- a/tools/testing/selftests/bpf/progs/test_global_func17.c > >> +++ b/tools/testing/selftests/bpf/progs/test_global_func17.c > >> @@ -5,6 +5,7 @@ > >> > >> __noinline int foo(int *p) > >> { > >> + barrier_var(p); > >> return p ? (*p = 42) : 0; > >> } > >> > > I recently stumbled upon no_clone ([0]) and no_ipa ([1]) attributes. > > Should we consider using those here instead? > > > > [0] https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-noclone-function-attribute > > [1] https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-noipa-function-attribute > > noipa attribute might help here. But sadly, noclone and noipa are gcc specific > and clang does not support either of them. I see, that's too bad, I assumed Clang also supports something like that. Maybe someday. > > > > > > >> -- > >> 2.34.1 > >>
