Re: [PATCH bpf-next v2 1/3] selftests/bpf: Refactor the failed assertion to another subtest

[Yonghong Song <yonghong.song@linux.dev>]

On 6/24/25 12:48 PM, Andrii Nakryiko wrote:
> On Tue, Jun 24, 2025 at 9:15 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>>
>> On 6/24/25 8:36 AM, Andrii Nakryiko wrote:
>>> On Tue, Jun 17, 2025 at 9:36 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>>>
>>>> On 6/16/25 3:00 PM, Andrii Nakryiko wrote:
>>>>> On Sun, Jun 15, 2025 at 11:54 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>>>>>> When building the selftest with arm64/clang20, the following test failed:
>>>>>>        ...
>>>>>>        ubtest_multispec_usdt:PASS:usdt_100_called 0 nsec
>>>>>>        subtest_multispec_usdt:PASS:usdt_100_sum 0 nsec
>>>>>>        subtest_multispec_usdt:FAIL:usdt_300_bad_attach unexpected pointer: 0xaaaad82a2a80
>>>>>>        #469/2   usdt/multispec:FAIL
>>>>>>        #469     usdt:FAIL
>>>>>>
>>>>>> The failed assertion
>>>>>>        subtest_multispec_usdt:FAIL:usdt_300_bad_attach unexpected pointer: 0xaaaad82a2a80
>>>>>> is caused by bpf_program__attach_usdt() which is expected to fail. But
>>>>>> with arm64/clang20 bpf_program__attach_usdt() actually succeeded.
>>>>> I think I missed that it's unexpected *success* that is causing
>>>>> issues. If that's so, then I think it might be more straightforward to
>>>>> just ensure that test is expectedly failing regardless of compiler
>>>>> code generation logic. Maybe something along the following lines:
>>>>>
>>>>> diff --git a/tools/testing/selftests/bpf/prog_tests/usdt.c
>>>>> b/tools/testing/selftests/bpf/prog_tests/usdt.c
>>>>> index 495d66414b57..fdd8642cfdff 100644
>>>>> --- a/tools/testing/selftests/bpf/prog_tests/usdt.c
>>>>> +++ b/tools/testing/selftests/bpf/prog_tests/usdt.c
>>>>> @@ -190,11 +190,21 @@ static void __always_inline f300(int x)
>>>>>            STAP_PROBE1(test, usdt_300, x);
>>>>>     }
>>>>>
>>>>> +#define RP10(F, X)  F(*(X+0)); F(*(X+1));F(*(X+2)); F(*(X+3)); F(*(X+4)); \
>>>>> +                   F(*(X+5)); F(*(X+6)); F(*(X+7)); F(*(X+8)); F(*(X+9));
>>>>> +#define RP100(F, X) RP10(F,X+
>>>>> 0);RP10(F,X+10);RP10(F,X+20);RP10(F,X+30);RP10(F,X+40); \
>>>>> +
>>>>> RP10(F,X+50);RP10(F,X+60);RP10(F,X+70);RP10(F,X+80);RP10(F,X+90);
>>>>> +
>>>>>     __weak void trigger_300_usdts(void)
>>>>>     {
>>>>> -       R100(f300, 0);
>>>>> -       R100(f300, 100);
>>>>> -       R100(f300, 200);
>>>>> +       volatile int arr[300], i;
>>>>> +
>>>>> +       for (i = 0; i < 300; i++)
>>>>> +               arr[i] = 300;
>>>>> +
>>>>> +       RP100(f300, arr + 0);
>>>>> +       RP100(f300, arr + 100);
>>>>> +       RP100(f300, arr + 200);
>>>>>     }
>>>>>
>>>>>
>>>>> So basically force the compiler to use 300 different locations for
>>>>> each of 300 USDT instantiations? I didn't check how that will look
>>>>> like on arm64, but on x86 gcc it seems to generate what is expected of
>>>>> it.
>>>>>
>>>>> Can you please try it on arm64 and see if that works?
>>>> I tried the above on arm64 and it does not work. It has the same usdt arguments
>>>> as without this patch:
>>>>
>>>>      stapsdt              0x0000002e       NT_STAPSDT (SystemTap probe descriptors)
>>>>        Provider: test
>>>>        Name: usdt_300
>>>>        Location: 0x00000000000009e0, Base: 0x0000000000000000, Semaphore: 0x0000000000000008
>>>>        Arguments: -4@[x9]
>>>>      stapsdt              0x0000002e       NT_STAPSDT (SystemTap probe descriptors)
>>>>        Provider: test
>>>>        Name: usdt_300
>>>>        Location: 0x00000000000009f8, Base: 0x0000000000000000, Semaphore: 0x0000000000000008
>>>>        Arguments: -4@[x9]
>>>>      ...
>>>>
>>>> But I found if we build usdt.c file with -O2 (RELEASE=1) on arm64, the test will be successful:
>>>>
>>>>      stapsdt              0x0000002b       NT_STAPSDT (SystemTap probe descriptors)
>>>>        Provider: test
>>>>        Name: usdt_300
>>>>        Location: 0x00000000000001a4, Base: 0x0000000000000000, Semaphore: 0x0000000000000008
>>>>        Arguments: -4@0
>>>>      stapsdt              0x0000002b       NT_STAPSDT (SystemTap probe descriptors)
>>>>        Provider: test
>>>>        Name: usdt_300
>>>>        Location: 0x00000000000001a8, Base: 0x0000000000000000, Semaphore: 0x0000000000000008
>>>>        Arguments: -4@1
>>>>      ...
>>>>
>>>> But usdt.c with -O2 will have a problem with gcc14 on x86:
>>>>
>>>>      stapsdt              0x00000087       NT_STAPSDT (SystemTap probe descriptors)
>>>>        Provider: test
>>>>        Name: usdt12
>>>>        Location: 0x000000000000258f, Base: 0x0000000000000000, Semaphore: 0x0000000000000006
>>>>        Arguments: -4@$2 -4@$3 -8@$42 -8@$44 -4@$5 -8@$6 8@%rdx 8@%rsi -4@$-9 -2@%cx -2@nums(%rax,%rax) -1@t1+4(%rip)
>>>>      ...
>>>>
>>>> You can see the above last two arguments which are not supported by libbpf.
>>>>
>>>> So let us say usdt.c is compiled with -O2:
>>>>       x86:
>>>>         gcc14 built kernel/selftests: failed, see the above
>>>>         clang built kernel/selftests: good
>>>>       arm64:
>>>>         both gcc14/clang built kernel/selftrests: good
>>>>
>>>> arm64 has more reigsters so it is likely to have better argument representation, e.g.,
>>>> for arm64/gcc with -O2, we have
>>>>
>>>>      stapsdt              0x00000071       NT_STAPSDT (SystemTap probe descriptors)
>>>>        Provider: test
>>>>        Name: usdt12
>>>>        Location: 0x0000000000002e74, Base: 0x0000000000000000, Semaphore: 0x000000000000000a
>>>>        Arguments: -4@2 -4@3 -8@42 -8@44 -4@5 -8@6 8@x1 8@x3 -4@-9 -2@x2 -2@[x0, 8] -1@[x3, 28]
>>>>
>>>> Eduard helped me to figure out how to compile prog_tests/usdt.c with -O2 alone.
>>>> The following patch resolved the issue and usdt test will be happy for both x86 and arm64:
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
>>>> index 97013c49920b..05fc9149bc4f 100644
>>>> --- a/tools/testing/selftests/bpf/Makefile
>>>> +++ b/tools/testing/selftests/bpf/Makefile
>>>> @@ -760,6 +760,14 @@ TRUNNER_BPF_BUILD_RULE := $$(error no BPF objects should be built)
>>>>     TRUNNER_BPF_CFLAGS :=
>>>>     $(eval $(call DEFINE_TEST_RUNNER,test_maps))
>>>>
>>>> +# Compiler prog_tests/usdt.c with -O2 with clang compiler.
>>>> +# Otherwise, with -O0 on arm64, the usdt test will fail.
>>>> +ifneq ($(LLVM),)
>>>> +$(OUTPUT)/usdt.test.o: CFLAGS:=$(subst O0,O2,$(CFLAGS))
>>>> +$(OUTPUT)/cpuv4/usdt.test.o: CFLAGS:=$(subst O0,O2,$(CFLAGS))
>>>> +$(OUTPUT)/no_alu32/usdt.test.o: CFLAGS:=$(subst O0,O2,$(CFLAGS))
>>>> +endif
>>>> +
>>>>     # Define test_verifier test runner.
>>>>     # It is much simpler than test_maps/test_progs and sufficiently different from
>>>>     # them (e.g., test.h is using completely pattern), that it's worth just
>>>>
>>>> Another choice is to support argument like `-2@nums(%rax,%rax)` and `-1@t1+4(%rip)`.
>>>> But I am not sure whether we should do it or not as typically a usdt probe
>>>> probably won't have lots of diverse arguments.
>>>>
>>>> WDYT?
>>> Can we just make that part of the test x86-64 specific for now? All
>>> other alternatives seem worse, tbh.
>> So something like below?
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/usdt.c b/tools/testing/selftests/bpf/prog_tests/usdt.c
>> index 495d66414b57..1e7e222034f7 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/usdt.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/usdt.c
>> @@ -270,8 +270,16 @@ static void subtest_multispec_usdt(void)
>>            */
>>           trigger_300_usdts();
>>
>> -       /* we'll reuse usdt_100 BPF program for usdt_300 test */
>>           bpf_link__destroy(skel->links.usdt_100);
>> +
>> +       /* If built with clang with arm64 target, there will be much less
>> +        * number of specs for usdt_300 call sites.
>> +        */
>> +#if defined(__clang__) && defined(__aarch64__)
>> +       bss->usdt_100_called = 0;
>> +       bss->usdt_100_sum = 0;
> I'd add this right before usdt_400 attachment unconditionally and
> avoid #if/#else/#endif branching. But other than that, yeah, something
> like that.

Sounds good to me!

>
>> +#else
>> +       /* we'll reuse usdt_100 BPF program for usdt_300 test */
>>           skel->links.usdt_100 = bpf_program__attach_usdt(skel->progs.usdt_100, -1, "/proc/self/exe",
>>                                                           "test", "usdt_300", NULL);
>>           err = -errno;
>> @@ -289,6 +297,7 @@ static void subtest_multispec_usdt(void)
>>
>>           ASSERT_EQ(bss->usdt_100_called, 0, "usdt_301_called");
>>           ASSERT_EQ(bss->usdt_100_sum, 0, "usdt_301_sum");
>> +#endif
>>
>>           /* This time we have USDT with 400 inlined invocations, but arg specs
>>            * should be the same across all sites, so libbpf will only need to
>>