From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) (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 AD0AA2C80 for ; Tue, 7 Dec 2021 11:19:10 +0000 (UTC) Received: from authenticated.ozlabs.org (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mail.ozlabs.org (Postfix) with ESMTPSA id 4J7d964jVBz4xgY; Tue, 7 Dec 2021 22:19:06 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ellerman.id.au; s=201909; t=1638875948; bh=0XMPFg1uyLXcByQa57fLF55p79RM8131PNd2ix+ygg8=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=bllKWJBbZ5SepP/QI5nePCofpeapO/yXsQh3GdA7T6GRyQ3YwRoz3HU4W+hOxciuP L/KZUi1UGwNjzHgWKO/BZi+TrLa+fHALGKEhRquvGlNUCcmf3V8JcVWzJseXUOtpne Dl0rzR7bbVJQ9LqxFSSFUjMfCv2xVYZ2c0GmWBfNhxShRobl9qNl/CyahVxIm58HOU cseXlKGcij6ojOLG1BvahU7GSonXbaxGvS45H5X/huhIjlwYGllxSHR2lVddIGoTz+ JXbKM6QBH1BBDnsgpLMY9yrwkLRey1sHjMGJ+TdeGCehP1D3ZhOJDEC9ylUEQP+rB7 tb+WksYYkS3yg== From: Michael Ellerman To: Nathan Chancellor Cc: Bill Wendling , Christophe Leroy , kernel test robot , Benjamin Herrenschmidt , Paul Mackerras , Nick Desaulniers , llvm@lists.linux.dev, kbuild-all@lists.01.org, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault() In-Reply-To: References: <0d5b12183d5176dd702d29ad94c39c384e51c78f.1638208156.git.christophe.leroy@csgroup.eu> <202111300652.0yDBNvyJ-lkp@intel.com> <87a6hlq408.fsf@mpe.ellerman.id.au> <87o85tnkzt.fsf@mpe.ellerman.id.au> Date: Tue, 07 Dec 2021 22:19:05 +1100 Message-ID: <87lf0woe6u.fsf@mpe.ellerman.id.au> Precedence: bulk X-Mailing-List: llvm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Nathan Chancellor writes: > On Tue, Dec 07, 2021 at 02:37:26PM +1100, Michael Ellerman wrote: >> Bill Wendling writes: >> > On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling wrot= e: >> >> On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor wrote: >> >> > On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote: >> >> > > Christophe Leroy writes: >> >> > > > Le 29/11/2021 =C3=A0 23:55, kernel test robot a =C3=A9crit : >> ... >> >> > > >> All warnings (new ones prefixed by >>): >> >> > > >> >> >> > > >> In file included from arch/powerpc/kernel/asm-offsets.c:71: >> >> > > >> In file included from arch/powerpc/kernel/../xmon/xmon_bpt= s.h:7: >> >> > > >>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'v= al' is uninitialized when used here [-Wuninitialized] >> >> > > >> *inst =3D ppc_inst(val); >> >> > > >> ^~~ >> >> > > >> arch/powerpc/include/asm/inst.h:53:22: note: expanded from= macro 'ppc_inst' >> >> > > >> #define ppc_inst(x) (x) >> >> > > >> ^ >> >> > > >> arch/powerpc/include/asm/inst.h:155:18: note: initialize t= he variable 'val' to silence this warning >> >> > > >> unsigned int val, suffix; >> >> > > >> ^ >> >> > > >> =3D 0 >> >> > > > >> >> > > > I can't understand what's wrong here. >> ... >> >> > > > >> >> > > > I see no possibility, no alternative path where val wouldn't be= set. The >> >> > > > asm clearly has *addr as an output param so it is always set. >> >> > > >> >> > > I guess clang can't convince itself of that? >> ... >> >> > >> >> > It certainly looks like there is something wrong with how clang is >> >> > tracking the initialization of the variable because it looks to me = like >> >> > val is only used in the fallthrough path, which happens after it is >> >> > initialized via lwz. Perhaps something is wrong with the logic of >> >> > https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issue= s are >> >> > being migrated from Bugzilla to GitHub Issues right now so I cannot= file >> >> > this upstream at the moment). >> >> > >> >> If I remove the casts of "val" the warning doesn't appear. I suspect >> >> that when I wrote that patch I forgot to remove those when checking. >> >> #include "Captain_Picard_facepalm.h" >> >> >> >> I'll look into it. >> >> >> > Small retraction. It's the "*()&val" that's the issue. (I.e. the= "*&") >>=20 >> I guess for now I'll just squash this in as a workaround? >>=20 >>=20 >> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/= inst.h >> index 631436f3f5c3..5b591c51fec9 100644 >> --- a/arch/powerpc/include/asm/inst.h >> +++ b/arch/powerpc/include/asm/inst.h >> @@ -157,6 +157,9 @@ static inline int copy_inst_from_kernel_nofault(ppc_= inst_t *inst, u32 *src) >> if (unlikely(!is_kernel_addr((unsigned long)src))) >> return -ERANGE; > > Could we add a version check to this and a link to our bug tracker: > > /* https://github.com/ClangBuiltLinux/linux/issues/1521 */ > #if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION < 140000 Yep, thanks I'll use that. cheers From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 30256C433FE for ; Tue, 7 Dec 2021 11:19:49 +0000 (UTC) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4J7d9v41PRz3bjP for ; Tue, 7 Dec 2021 22:19:47 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=ellerman.id.au header.i=@ellerman.id.au header.a=rsa-sha256 header.s=201909 header.b=bllKWJBb; dkim-atps=neutral Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [IPv6:2404:9400:2:0:216:3eff:fee2:21ea]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4J7d9B2mjyz2yN1 for ; Tue, 7 Dec 2021 22:19:10 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=ellerman.id.au header.i=@ellerman.id.au header.a=rsa-sha256 header.s=201909 header.b=bllKWJBb; dkim-atps=neutral Received: from authenticated.ozlabs.org (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mail.ozlabs.org (Postfix) with ESMTPSA id 4J7d964jVBz4xgY; Tue, 7 Dec 2021 22:19:06 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ellerman.id.au; s=201909; t=1638875948; bh=0XMPFg1uyLXcByQa57fLF55p79RM8131PNd2ix+ygg8=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=bllKWJBbZ5SepP/QI5nePCofpeapO/yXsQh3GdA7T6GRyQ3YwRoz3HU4W+hOxciuP L/KZUi1UGwNjzHgWKO/BZi+TrLa+fHALGKEhRquvGlNUCcmf3V8JcVWzJseXUOtpne Dl0rzR7bbVJQ9LqxFSSFUjMfCv2xVYZ2c0GmWBfNhxShRobl9qNl/CyahVxIm58HOU cseXlKGcij6ojOLG1BvahU7GSonXbaxGvS45H5X/huhIjlwYGllxSHR2lVddIGoTz+ JXbKM6QBH1BBDnsgpLMY9yrwkLRey1sHjMGJ+TdeGCehP1D3ZhOJDEC9ylUEQP+rB7 tb+WksYYkS3yg== From: Michael Ellerman To: Nathan Chancellor Subject: Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault() In-Reply-To: References: <0d5b12183d5176dd702d29ad94c39c384e51c78f.1638208156.git.christophe.leroy@csgroup.eu> <202111300652.0yDBNvyJ-lkp@intel.com> <87a6hlq408.fsf@mpe.ellerman.id.au> <87o85tnkzt.fsf@mpe.ellerman.id.au> Date: Tue, 07 Dec 2021 22:19:05 +1100 Message-ID: <87lf0woe6u.fsf@mpe.ellerman.id.au> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kbuild-all@lists.01.org, kernel test robot , llvm@lists.linux.dev, Nick Desaulniers , linux-kernel@vger.kernel.org, Paul Mackerras , Bill Wendling , linuxppc-dev@lists.ozlabs.org Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" Nathan Chancellor writes: > On Tue, Dec 07, 2021 at 02:37:26PM +1100, Michael Ellerman wrote: >> Bill Wendling writes: >> > On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling wrot= e: >> >> On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor wrote: >> >> > On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote: >> >> > > Christophe Leroy writes: >> >> > > > Le 29/11/2021 =C3=A0 23:55, kernel test robot a =C3=A9crit : >> ... >> >> > > >> All warnings (new ones prefixed by >>): >> >> > > >> >> >> > > >> In file included from arch/powerpc/kernel/asm-offsets.c:71: >> >> > > >> In file included from arch/powerpc/kernel/../xmon/xmon_bpt= s.h:7: >> >> > > >>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'v= al' is uninitialized when used here [-Wuninitialized] >> >> > > >> *inst =3D ppc_inst(val); >> >> > > >> ^~~ >> >> > > >> arch/powerpc/include/asm/inst.h:53:22: note: expanded from= macro 'ppc_inst' >> >> > > >> #define ppc_inst(x) (x) >> >> > > >> ^ >> >> > > >> arch/powerpc/include/asm/inst.h:155:18: note: initialize t= he variable 'val' to silence this warning >> >> > > >> unsigned int val, suffix; >> >> > > >> ^ >> >> > > >> =3D 0 >> >> > > > >> >> > > > I can't understand what's wrong here. >> ... >> >> > > > >> >> > > > I see no possibility, no alternative path where val wouldn't be= set. The >> >> > > > asm clearly has *addr as an output param so it is always set. >> >> > > >> >> > > I guess clang can't convince itself of that? >> ... >> >> > >> >> > It certainly looks like there is something wrong with how clang is >> >> > tracking the initialization of the variable because it looks to me = like >> >> > val is only used in the fallthrough path, which happens after it is >> >> > initialized via lwz. Perhaps something is wrong with the logic of >> >> > https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issue= s are >> >> > being migrated from Bugzilla to GitHub Issues right now so I cannot= file >> >> > this upstream at the moment). >> >> > >> >> If I remove the casts of "val" the warning doesn't appear. I suspect >> >> that when I wrote that patch I forgot to remove those when checking. >> >> #include "Captain_Picard_facepalm.h" >> >> >> >> I'll look into it. >> >> >> > Small retraction. It's the "*()&val" that's the issue. (I.e. the= "*&") >>=20 >> I guess for now I'll just squash this in as a workaround? >>=20 >>=20 >> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/= inst.h >> index 631436f3f5c3..5b591c51fec9 100644 >> --- a/arch/powerpc/include/asm/inst.h >> +++ b/arch/powerpc/include/asm/inst.h >> @@ -157,6 +157,9 @@ static inline int copy_inst_from_kernel_nofault(ppc_= inst_t *inst, u32 *src) >> if (unlikely(!is_kernel_addr((unsigned long)src))) >> return -ERANGE; > > Could we add a version check to this and a link to our bug tracker: > > /* https://github.com/ClangBuiltLinux/linux/issues/1521 */ > #if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION < 140000 Yep, thanks I'll use that. cheers From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============6199626668175946261==" MIME-Version: 1.0 From: Michael Ellerman To: kbuild-all@lists.01.org Subject: Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault() Date: Tue, 07 Dec 2021 22:19:05 +1100 Message-ID: <87lf0woe6u.fsf@mpe.ellerman.id.au> In-Reply-To: List-Id: --===============6199626668175946261== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Nathan Chancellor writes: > On Tue, Dec 07, 2021 at 02:37:26PM +1100, Michael Ellerman wrote: >> Bill Wendling writes: >> > On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling wr= ote: >> >> On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor wrote: >> >> > On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote: >> >> > > Christophe Leroy writes: >> >> > > > Le 29/11/2021 =C3=A0 23:55, kernel test robot a =C3=A9crit : >> ... >> >> > > >> All warnings (new ones prefixed by >>): >> >> > > >> >> >> > > >> In file included from arch/powerpc/kernel/asm-offsets.c:71: >> >> > > >> In file included from arch/powerpc/kernel/../xmon/xmon_bpt= s.h:7: >> >> > > >>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'v= al' is uninitialized when used here [-Wuninitialized] >> >> > > >> *inst =3D ppc_inst(val); >> >> > > >> ^~~ >> >> > > >> arch/powerpc/include/asm/inst.h:53:22: note: expanded from= macro 'ppc_inst' >> >> > > >> #define ppc_inst(x) (x) >> >> > > >> ^ >> >> > > >> arch/powerpc/include/asm/inst.h:155:18: note: initialize t= he variable 'val' to silence this warning >> >> > > >> unsigned int val, suffix; >> >> > > >> ^ >> >> > > >> =3D 0 >> >> > > > >> >> > > > I can't understand what's wrong here. >> ... >> >> > > > >> >> > > > I see no possibility, no alternative path where val wouldn't be= set. The >> >> > > > asm clearly has *addr as an output param so it is always set. >> >> > > >> >> > > I guess clang can't convince itself of that? >> ... >> >> > >> >> > It certainly looks like there is something wrong with how clang is >> >> > tracking the initialization of the variable because it looks to me = like >> >> > val is only used in the fallthrough path, which happens after it is >> >> > initialized via lwz. Perhaps something is wrong with the logic of >> >> > https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issue= s are >> >> > being migrated from Bugzilla to GitHub Issues right now so I cannot= file >> >> > this upstream at the moment). >> >> > >> >> If I remove the casts of "val" the warning doesn't appear. I suspect >> >> that when I wrote that patch I forgot to remove those when checking. >> >> #include "Captain_Picard_facepalm.h" >> >> >> >> I'll look into it. >> >> >> > Small retraction. It's the "*()&val" that's the issue. (I.e. the= "*&") >> = >> I guess for now I'll just squash this in as a workaround? >> = >> = >> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/= inst.h >> index 631436f3f5c3..5b591c51fec9 100644 >> --- a/arch/powerpc/include/asm/inst.h >> +++ b/arch/powerpc/include/asm/inst.h >> @@ -157,6 +157,9 @@ static inline int copy_inst_from_kernel_nofault(ppc_= inst_t *inst, u32 *src) >> if (unlikely(!is_kernel_addr((unsigned long)src))) >> return -ERANGE; > > Could we add a version check to this and a link to our bug tracker: > > /* https://github.com/ClangBuiltLinux/linux/issues/1521 */ > #if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION < 140000 Yep, thanks I'll use that. cheers --===============6199626668175946261==--