On Mon, 12 May 2025 at 12:41, Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Mon, May 12, 2025 at 5:22 AM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > 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@xxxxxxxxxxxxx>
> > > > > 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.
