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

[Ilya Leoshkevich <iii@linux.ibm.com>]

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).