Re: [PATCH bpf-next] selftests/bpf: Fix "expression result unused" warnings

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

On 5/24/25 2:05 PM, Ilya Leoshkevich wrote:
> On Sat, 2025-05-24 at 03:01 +0200, Kumar Kartikeya Dwivedi wrote:
>> On Sat, 24 May 2025 at 02:06, Yonghong Song <yonghong.song@linux.dev>
>> wrote:
>>>
>>>
>>> On 5/23/25 4:25 AM, Ilya Leoshkevich wrote:
>>>> On Mon, 2025-05-12 at 15:29 -0400, Kumar Kartikeya Dwivedi wrote:
>>>>> On Mon, 12 May 2025 at 12:41, Alexei Starovoitov
>>>>> <alexei.starovoitov@gmail.com> wrote:
>>>>>> On Mon, May 12, 2025 at 5:22 AM Ilya Leoshkevich
>>>>>> <iii@linux.ibm.com> wrote:
>>>>>>> On Fri, 2025-05-09 at 09:51 -0700, Alexei Starovoitov
>>>>>>> wrote:
>>>>>>>> On Thu, May 8, 2025 at 12:21 PM Ilya Leoshkevich
>>>>>>>> <iii@linux.ibm.com>
>>>>>>>> wrote:
>>>>>>>>> On Thu, 2025-05-08 at 11:38 -0700, Alexei Starovoitov
>>>>>>>>> wrote:
>>>>>>>>>> On Thu, May 8, 2025 at 4:38 AM Ilya Leoshkevich
>>>>>>>>>> <iii@linux.ibm.com>
>>>>>>>>>> wrote:
>>>>>>>>>>> clang-21 complains about unused expressions in a
>>>>>>>>>>> few
>>>>>>>>>>> progs.
>>>>>>>>>>> Fix by explicitly casting the respective
>>>>>>>>>>> expressions to
>>>>>>>>>>> void.
>>>>>>>>>> ...
>>>>>>>>>>>           if (val & _Q_LOCKED_MASK)
>>>>>>>>>>> -               smp_cond_load_acquire_label(&lock-
>>>>>>>>>>>> locked,
>>>>>>>>>>> !VAL,
>>>>>>>>>>> release_err);
>>>>>>>>>>> +
>>>>>>>>>>> (void)smp_cond_load_acquire_label(&lock-
>>>>>>>>>>>> locked,
>>>>>>>>>>> !VAL, release_err);
>>>>>>>>>> Hmm. I'm on clang-21 too and I don't see them.
>>>>>>>>>> What warnings do you see ?
>>>>>>>>> In file included from progs/arena_spin_lock.c:7:
>>>>>>>>> progs/bpf_arena_spin_lock.h:305:1756: error: expression
>>>>>>>>> result
>>>>>>>>> unused
>>>>>>>>> [-Werror,-Wunused-value]
>>>>>>>>>     305 |   ({ typeof(_Generic((*&lock->locked), char:
>>>>>>>>> (char)0,
>>>>>>>>> unsigned
>>>>>>>>> char : (unsigned char)0, signed char : (signed char)0,
>>>>>>>>> unsigned
>>>>>>>>> short :
>>>>>>>>> (unsigned short)0, signed short : (signed short)0,
>>>>>>>>> unsigned
>>>>>>>>> int :
>>>>>>>>> (unsigned int)0, signed int : (signed int)0, unsigned
>>>>>>>>> long :
>>>>>>>>> (unsigned
>>>>>>>>> long)0, signed long : (signed long)0, unsigned long
>>>>>>>>> long :
>>>>>>>>> (unsigned
>>>>>>>>> long long)0, signed long long : (signed long long)0,
>>>>>>>>> default:
>>>>>>>>> (typeof(*&lock->locked))0)) __val = ({ typeof(&lock-
>>>>>>>>>> locked)
>>>>>>>>> __ptr
>>>>>>>>> =
>>>>>>>>> (&lock->locked); typeof(_Generic((*(&lock->locked)),
>>>>>>>>> char:
>>>>>>>>> (char)0,
>>>>>>>>> unsigned char : (unsigned char)0, signed char : (signed
>>>>>>>>> char)0,
>>>>>>>>> unsigned short : (unsigned short)0, signed short :
>>>>>>>>> (signed
>>>>>>>>> short)0,
>>>>>>>>> unsigned int : (unsigned int)0, signed int : (signed
>>>>>>>>> int)0,
>>>>>>>>> unsigned
>>>>>>>>> long : (unsigned long)0, signed long : (signed long)0,
>>>>>>>>> unsigned
>>>>>>>>> long
>>>>>>>>> long : (unsigned long long)0, signed long long :
>>>>>>>>> (signed long
>>>>>>>>> long)0,
>>>>>>>>> default: (typeof(*(&lock->locked)))0)) VAL; for (;;) {
>>>>>>>>> VAL =
>>>>>>>>> (typeof(_Generic((*(&lock->locked)), char: (char)0,
>>>>>>>>> unsigned
>>>>>>>>> char :
>>>>>>>>> (unsigned char)0, signed char : (signed char)0,
>>>>>>>>> unsigned
>>>>>>>>> short :
>>>>>>>>> (unsigned short)0, signed short : (signed short)0,
>>>>>>>>> unsigned
>>>>>>>>> int :
>>>>>>>>> (unsigned int)0, signed int : (signed int)0, unsigned
>>>>>>>>> long :
>>>>>>>>> (unsigned
>>>>>>>>> long)0, signed long : (signed long)0, unsigned long
>>>>>>>>> long :
>>>>>>>>> (unsigned
>>>>>>>>> long long)0, signed long long : (signed long long)0,
>>>>>>>>> default:
>>>>>>>>> (typeof(*(&lock->locked)))0)))(*(volatile
>>>>>>>>> typeof(*__ptr)
>>>>>>>>> *)&(*__ptr));
>>>>>>>>> if (!VAL) break; ({ __label__ l_break, l_continue; asm
>>>>>>>>> volatile
>>>>>>>>> goto("may_goto %l[l_break]" :::: l_break); goto
>>>>>>>>> l_continue;
>>>>>>>>> l_break:
>>>>>>>>> goto release_err; l_continue:; }); ({}); }
>>>>>>>>> (typeof(*(&lock-
>>>>>>>>>> locked)))VAL; }); ({ ({ if (!CONFIG_X86_64) ({
>>>>>>>>>> unsigned
>>>>>>>>>> long
>>>>>>>>>> __val;
>>>>>>>>> __sync_fetch_and_add(&__val, 0); }); else asm
>>>>>>>>> volatile("" :::
>>>>>>>>> "memory"); }); }); (typeof(*(&lock->locked)))__val; });
>>>>>>>>>         |
>>>>>>>>> ^                         ~~~~~
>>>>>>>>> 1 error generated.
>>>>>>>> hmm. The error is impossible to read.
>>>>>>>>
>>>>>>>> Kumar,
>>>>>>>>
>>>>>>>> Do you see a way to silence it differently ?
>>>>>>>>
>>>>>>>> Without adding (void)...
>>>>>>>>
>>>>>>>> Things like:
>>>>>>>> -       bpf_obj_new(..
>>>>>>>> +       (void)bpf_obj_new(..
>>>>>>>>
>>>>>>>> are good to fix, and if we could annotate
>>>>>>>> bpf_obj_new_impl kfunc with __must_check we would have
>>>>>>>> done it,
>>>>>>>>
>>>>>>>> but
>>>>>>>> -               arch_mcs_spin_lock...
>>>>>>>> +               (void)arch_mcs_spin_lock...
>>>>>>>>
>>>>>>>> is odd.
>>>>>>> What do you think about moving (void) to the definition of
>>>>>>> arch_mcs_spin_lock_contended_label()? I can send a v2 if
>>>>>>> this is
>>>>>>> better.
>>>>>> Kumar,
>>>>>>
>>>>>> thoughts?
>>>>> Sorry for the delay, I was afk.
>>>>>
>>>>> The warning seems a bit aggressive, in the kernel we have users
>>>>> which
>>>>> do and do not use the value and it's fine.
>>>>> I think moving (void) inside the macro is a problem since at
>>>>> least
>>>>> rqspinlock like algorithm would want to inspect the result of
>>>>> the
>>>>> locked bit.
>>>>> No such users exist for now, of course. So maybe we can silence
>>>>> it
>>>>> until we do end up depending on the value.
>>>>>
>>>>> I will give a try with clang-21, but I think probably (void) in
>>>>> the
>>>>> source is better if we do need to silence it.
>>>> Gentle ping.
>>>>
>>>> This is still an issue with clang version 21.0.0
>>>> (++20250522112647+491619a25003-1~exp1~20250522112819.1465).
>>>>
>>> I cannot reproduce the "unused expressions" error. What is the
>>> llvm cmake command line you are using?
>>>
>> Sorry for the delay. I tried just now with clang built from the
>> latest
>> git checkout but I don't see it either.
>> I built it following the steps at
>> https://www.kernel.org/doc/Documentation/bpf/bpf_devel_QA.rst.
> I use the following make invocation:
>
> make CC="ccache gcc" LD=ld.lld-21 O="$PWD/../linux-build-s390x"
> CLANG="ccache clang-21" LLVM_STRIP=llvm-strip-21 LLC=llc-21 LLD=lld-21
> -j128 -C tools/testing/selftests/bpf BPF_GCC= V=1
>
> which results in the following clang invocation:
>
> ccache clang-21  -g -Wall -Werror -D__TARGET_ARCH_s390 -mbig-endian -
> I"$PWD/../../../../.."/linux-build-s390x//tools/include -
> I"$PWD/../../../../.."/linux/tools/testing/selftests/bpf -
> I"$PWD/../../../../.."/linux/tools/include/uapi -
> I"$PWD/../../../../.."/usr/include -std=gnu11 -fno-strict-aliasing -
> Wno-compare-distinct-pointer-types -idirafter /usr/lib/llvm-
> 21/lib/clang/21/include -idirafter /usr/local/include -idirafter
> /usr/include/s390x-linux-gnu -idirafter /usr/include    -
> DENABLE_ATOMICS_TESTS   -O2 --target=bpfeb -c progs/arena_spin_lock.c -
> mcpu=v3 -o "$PWD/../../../../.."/linux-build-
> s390x//arena_spin_lock.bpf.o
>
> I tried dropping ccache, but it did not help.

Thanks, Ilya. It could be great if you can find out the
cmake command lines which eventually builds your clang-21.
Once cmake command lines are available, I can build
the compiler on x86_64 host and do some checking for it.