From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-188.mta0.migadu.com (out-188.mta0.migadu.com [91.218.175.188]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 128B31FDA for ; Tue, 27 May 2025 21:26:50 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.188 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1748381214; cv=none; b=owVsKJEcoMgUeM+xjj+o5gJgY8pD0vtG99VpfBBe8fvl9YSLtnsa44frplUFtiNiTHdyAapHNxTcfA8F7uh/QEQb2W0OuYK30Hb54lGtI8WKExVZ/gjAIewlTdkI6QamPQy5PHxlJTuYSeRc1E6WXx3jPPs0rndU/XrTFWUPNP0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1748381214; c=relaxed/simple; bh=EqJo7YCoQTBzDJz04ILn7SArEdQUMsXHOZnmy782KuM=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=rmTuXUm1k+j2vXwRwFMDV+pXxUpShQvvxgwt9jIHP9BsvFqPIUS3A57q43SU9R6TWMS4qS1DlZRE87VINO4wm5tGe5cTB5z/fcv4QDnlXRPj7boNbwwggjts6MmGnlsUkq54EC002QUfxRm6kVkUbyvdD8BsNQ3RXH9LGLkvLW8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=HV88pdD/; arc=none smtp.client-ip=91.218.175.188 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="HV88pdD/" Message-ID: <92c810bf-64f0-4f84-80d5-65e27bbe9a3e@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1748381208; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=csYR+SQ8ocIYay0ZE9cKGwa1y8NKac3caRYihbvoGFY=; b=HV88pdD/2am+l+MDOjqTarZ6MB671Xrr966srI1PlSWWVKd5mo9KxnNwrb6BEIoSb3Muy1 0mLFH3yYf2mnFeeBeWcDWK3BD4rG6abk0avGlLqpje8k6o/Au6Deg4mBEtNHNlB81igs/Z 4JInsZw5pXvX1rp003gf0msXSk0Aa84= Date: Tue, 27 May 2025 14:26:42 -0700 Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Subject: Re: [PATCH bpf-next] selftests/bpf: Fix "expression result unused" warnings Content-Language: en-GB To: Ilya Leoshkevich , Kumar Kartikeya Dwivedi Cc: Alexei Starovoitov , Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , bpf , Heiko Carstens , Vasily Gorbik , Alexander Gordeev References: <20250508113804.304665-1-iii@linux.ibm.com> <15bf9a71b8185006c8d19a3aefb331a2765629c5.camel@linux.ibm.com> <7a242102eecdd17b4d35c1e4f7d01ea15cb8066a.camel@linux.ibm.com> <195a1fd78ebf029eba204982f5bbe0ec6ef025fb.camel@linux.ibm.com> <6192c51c-e800-4a89-a0b2-52abab33010a@linux.dev> <15f2b0cb9fd8c106d1daac1c7e0c156c97e0ee04.camel@linux.ibm.com> X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Yonghong Song In-Reply-To: <15f2b0cb9fd8c106d1daac1c7e0c156c97e0ee04.camel@linux.ibm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT 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 >>>> >>>> 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 >>>>>>> wrote: >>>>>>>> On Mon, May 12, 2025 at 5:22 AM Ilya Leoshkevich >>>>>>>> wrote: >>>>>>>>> On Fri, 2025-05-09 at 09:51 -0700, Alexei Starovoitov >>>>>>>>> wrote: >>>>>>>>>> On Thu, May 8, 2025 at 12:21 PM Ilya Leoshkevich >>>>>>>>>> >>>>>>>>>> wrote: >>>>>>>>>>> On Thu, 2025-05-08 at 11:38 -0700, Alexei >>>>>>>>>>> Starovoitov >>>>>>>>>>> wrote: >>>>>>>>>>>> On Thu, May 8, 2025 at 4:38 AM Ilya Leoshkevich >>>>>>>>>>>> >>>>>>>>>>>> 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 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).