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

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

On 5/27/25 2:26 PM, Yonghong Song wrote:
>
>
> On 5/27/25 1:27 AM, Ilya Leoshkevich wrote:
>> On Mon, 2025-05-26 at 22:15 -0700, Yonghong Song wrote:
>>>
>>> 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.
>> Hi Yonghong, I don't build it, I take it from apt.llvm.org.
>> It's surprising we don't see this in CI, because it also takes
>> clang from there. If you think this is a compiler and not a code
>> bug, I can debug this myself, because maybe it's reproducible only on
>> s390x.
>
> I don't think this is a compiler bug. As mentioned by Alexei, 
> __must_check

Ignore this 'I don't think this is a compiler bug'. This is my early thinking
but eventually I think it is likely to be a clang bug with s390x target.

>
>   linux/compiler_attributes.h:#define __must_check 
> __attribute__((__warn_unused_result__))
>
> is needed for the compiler to issue an error for unused func return 
> value.
>
[...]

>
> But if s390x emits an error even without 
> __attribute__((warn_unused_result)),
> I suspect that there is a bug in clang21 frontend with s390x.
> I assume clang20 will be okay?
> It is possible that in clang21, s390x clang frontend target specific 
> things
> may cause clang emit error even without __must_check attribute.
>
> If clang20 is okay for s390x, I suggest to file a bug to llvm-project
> (clang21 frontend).
>
>