On Sat, 24 May 2025 at 02:06, Yonghong Song <yonghong.song@xxxxxxxxx> 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@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.
> > 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.
