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

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

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.