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

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

On 5/27/25 1:27 AM, Ilya Leoshkevich wrote:
> On Mon, 2025-05-26 at 22:15 -0700, Yonghong Song wrote:
>>
>> On 5/24/25 2:05 PM, Ilya Leoshkevich wrote:
>>> 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.
>> Thanks, Ilya. It could be great if you can find out the
>> cmake command lines which eventually builds your clang-21.
>> Once cmake command lines are available, I can build
>> the compiler on x86_64 host and do some checking for it.
> Hi Yonghong, I don't build it, I take it from apt.llvm.org.
> It's surprising we don't see this in CI, because it also takes
> clang from there. If you think this is a compiler and not a code
> bug, I can debug this myself, because maybe it's reproducible only on
> s390x.

I don't think this is a compiler bug. As mentioned by Alexei, __must_check

   linux/compiler_attributes.h:#define __must_check __attribute__((__warn_unused_result__))

is needed for the compiler to issue an error for unused func return value.

I did some further checking on clang source code with a simple example on x86_64 machine:

$ cat t.c
int bar(void) __attribute__((warn_unused_result));
// int bar(void);
int foo(int a) {
   bar();
   return a;
}

and command line

clang -Wall -Werror -g -O2 -c t.c

The key related code is at
   https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaStmt.cpp#L230-L257

// Diagnoses unused expressions that call functions marked [[nodiscard]],
// [[gnu::warn_unused_result]] and similar.
// Additionally, a DiagID can be provided to emit a warning in additional
// contexts (such as for an unused LHS of a comma expression)
void DiagnoseUnused(Sema &S, const Expr *E, std::optional<unsigned> DiagID) {
   bool NoDiscardOnly = !DiagID.has_value();
......


The following two lines of code is the key:

   if (!E->isUnusedResultAWarning(WarnExpr, Loc, R1, R2, S.Context))
     return;
   ...

With 'int bar(void) __attribute__((warn_unused_result));' the above
if stmt will fall through.
With 'int bar(void);' the above if stmt will return from DiagnozeUnused() func.

For 'return true' case, eventually it emits an error.

So we don't have issues with x86.

But if s390x emits an error even without __attribute__((warn_unused_result)),
I suspect that there is a bug in clang21 frontend with s390x.
I assume clang20 will be okay?
It is possible that in clang21, s390x clang frontend target specific things
may cause clang emit error even without __must_check attribute.

If clang20 is okay for s390x, I suggest to file a bug to llvm-project
(clang21 frontend).