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

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

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?