From: Nathan Chancellor <nathan@kernel.org>
To: Michael Ellerman <mpe@ellerman.id.au>
Cc: Bill Wendling <morbo@google.com>,
Christophe Leroy <christophe.leroy@csgroup.eu>,
kernel test robot <lkp@intel.com>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>,
Nick Desaulniers <ndesaulniers@google.com>,
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()
Date: Mon, 6 Dec 2021 21:48:52 -0700 [thread overview]
Message-ID: <Ya7ntJ0ehZPy6HKA@archlinux-ax161> (raw)
In-Reply-To: <87o85tnkzt.fsf@mpe.ellerman.id.au>
On Tue, Dec 07, 2021 at 02:37:26PM +1100, Michael Ellerman wrote:
> Bill Wendling <morbo@google.com> writes:
> > On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling <morbo@google.com> wrote:
> >> On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor <nathan@kernel.org> wrote:
> >> > On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote:
> >> > > Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> >> > > > Le 29/11/2021 à 23:55, kernel test robot a écrit :
> ...
> >> > > >> 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_bpts.h:7:
> >> > > >>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
> >> > > >> *inst = 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 the variable 'val' to silence this warning
> >> > > >> unsigned int val, suffix;
> >> > > >> ^
> >> > > >> = 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 issues 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 "*(<cast>)&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
> +#ifdef CONFIG_CC_IS_CLANG
> + val = suffix = 0;
> +#endif
> __get_kernel_nofault(&val, src, u32, Efault);
> if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
> __get_kernel_nofault(&suffix, src + 1, u32, Efault);
>
>
>
> cheers
next prev parent reply other threads:[~2021-12-07 4:49 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-29 17:49 [PATCH v5 1/5] powerpc/inst: Refactor ___get_user_instr() Christophe Leroy
2021-11-29 17:49 ` [PATCH v5 2/5] powerpc/inst: Define ppc_inst_t Christophe Leroy
2021-11-29 17:49 ` [PATCH v5 3/5] powerpc/inst: Define ppc_inst_t as u32 on PPC32 Christophe Leroy
2021-11-29 17:49 ` [PATCH v5 4/5] powerpc/inst: Move ppc_inst_t definition in asm/reg.h Christophe Leroy
2021-11-29 17:49 ` [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault() Christophe Leroy
2021-11-29 22:55 ` kernel test robot
2021-11-30 5:58 ` Christophe Leroy
2021-11-30 11:25 ` Michael Ellerman
2021-11-30 18:17 ` Nathan Chancellor
2021-11-30 18:38 ` Bill Wendling
2021-11-30 18:44 ` Bill Wendling
2021-12-07 3:37 ` Michael Ellerman
2021-12-07 4:48 ` Nathan Chancellor [this message]
2021-12-07 5:45 ` Christophe Leroy
2021-12-07 6:41 ` Nathan Chancellor
2021-12-07 7:55 ` Christophe Leroy
2021-12-07 11:19 ` Michael Ellerman
2021-12-01 6:45 ` kernel test robot
2021-12-15 0:24 ` [PATCH v5 1/5] powerpc/inst: Refactor ___get_user_instr() Michael Ellerman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Ya7ntJ0ehZPy6HKA@archlinux-ax161 \
--to=nathan@kernel.org \
--cc=benh@kernel.crashing.org \
--cc=christophe.leroy@csgroup.eu \
--cc=kbuild-all@lists.01.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=lkp@intel.com \
--cc=llvm@lists.linux.dev \
--cc=morbo@google.com \
--cc=mpe@ellerman.id.au \
--cc=ndesaulniers@google.com \
--cc=paulus@samba.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).