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