* Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()
[not found] <0d5b12183d5176dd702d29ad94c39c384e51c78f.1638208156.git.christophe.leroy@csgroup.eu>
@ 2021-11-29 22:55 ` kernel test robot
2021-11-30 5:58 ` Christophe Leroy
2021-12-01 6:45 ` kernel test robot
1 sibling, 1 reply; 13+ messages in thread
From: kernel test robot @ 2021-11-29 22:55 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: llvm, kbuild-all, linuxppc-dev, linux-kernel
Hi Christophe,
I love your patch! Perhaps something to improve:
[auto build test WARNING on powerpc/next]
[also build test WARNING on v5.16-rc3 next-20211129]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-randconfig-r023-20211129 (https://download.01.org/0day-ci/archive/20211130/202111300652.0yDBNvyJ-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
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
1 warning generated.
<stdin>:1559:2: warning: syscall futex_waitv not implemented [-W#warnings]
#warning syscall futex_waitv not implemented
^
1 warning generated.
arch/powerpc/kernel/vdso32/gettimeofday.S:72:8: error: unsupported directive '.stabs'
.stabs "_restgpr_31_x:F-1",36,0,0,_restgpr_31_x; .globl _restgpr_31_x; _restgpr_31_x:
^
arch/powerpc/kernel/vdso32/gettimeofday.S:73:8: error: unsupported directive '.stabs'
.stabs "_rest32gpr_31_x:F-1",36,0,0,_rest32gpr_31_x; .globl _rest32gpr_31_x; _rest32gpr_31_x:
^
make[2]: *** [arch/powerpc/kernel/vdso32/Makefile:55: arch/powerpc/kernel/vdso32/gettimeofday.o] Error 1
make[2]: Target 'include/generated/vdso32-offsets.h' not remade because of errors.
make[1]: *** [arch/powerpc/Makefile:421: vdso_prepare] Error 2
make[1]: Target 'prepare' not remade because of errors.
make: *** [Makefile:219: __sub-make] Error 2
make: Target 'prepare' not remade because of errors.
vim +/val +165 arch/powerpc/include/asm/inst.h
152
153 static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
154 {
155 unsigned int val, suffix;
156
157 if (unlikely(!is_kernel_addr((unsigned long)src)))
158 return -ERANGE;
159
160 __get_kernel_nofault(&val, src, u32, Efault);
161 if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
162 __get_kernel_nofault(&suffix, src + 1, u32, Efault);
163 *inst = ppc_inst_prefix(val, suffix);
164 } else {
> 165 *inst = ppc_inst(val);
166 }
167 return 0;
168 Efault:
169 return -EFAULT;
170 }
171
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()
2021-11-29 22:55 ` [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault() kernel test robot
@ 2021-11-30 5:58 ` Christophe Leroy
2021-11-30 11:25 ` Michael Ellerman
0 siblings, 1 reply; 13+ messages in thread
From: Christophe Leroy @ 2021-11-30 5:58 UTC (permalink / raw)
To: kernel test robot, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Nick Desaulniers
Cc: llvm, kbuild-all, linuxppc-dev, linux-kernel
Le 29/11/2021 à 23:55, kernel test robot a écrit :
> Hi Christophe,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on powerpc/next]
> [also build test WARNING on v5.16-rc3 next-20211129]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> config: powerpc-randconfig-r023-20211129 (https://download.01.org/0day-ci/archive/20211130/202111300652.0yDBNvyJ-lkp@intel.com/config)
> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # install powerpc cross compiling tool for clang build
> # apt-get install binutils-powerpc-linux-gnu
> # https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826
> git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
> git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826
> # save the config file to linux build tree
> mkdir build_dir
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
> 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.
We have
__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);
*inst = ppc_inst_prefix(val, suffix);
} else {
*inst = ppc_inst(val);
}
With
#define __get_kernel_nofault(dst, src, type, err_label) \
__get_user_size_goto(*((type *)(dst)), \
(__force type __user *)(src), sizeof(type), err_label)
And
#define __get_user_size_goto(x, ptr, size, label) \
do { \
BUILD_BUG_ON(size > sizeof(x)); \
switch (size) { \
case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break; \
case 2: __get_user_asm_goto(x, (u16 __user *)ptr, label, "lhz"); break; \
case 4: __get_user_asm_goto(x, (u32 __user *)ptr, label, "lwz"); break; \
case 8: __get_user_asm2_goto(x, (u64 __user *)ptr, label); break; \
default: x = 0; BUILD_BUG(); \
} \
} while (0)
And
#define __get_user_asm_goto(x, addr, label, op) \
asm_volatile_goto( \
"1: "op"%U1%X1 %0, %1 # get_user\n" \
EX_TABLE(1b, %l2) \
: "=r" (x) \
: "m<>" (*addr) \
: \
: label)
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.
> 1 warning generated.
> <stdin>:1559:2: warning: syscall futex_waitv not implemented [-W#warnings]
> #warning syscall futex_waitv not implemented
> ^
> 1 warning generated.
> arch/powerpc/kernel/vdso32/gettimeofday.S:72:8: error: unsupported directive '.stabs'
> .stabs "_restgpr_31_x:F-1",36,0,0,_restgpr_31_x; .globl _restgpr_31_x; _restgpr_31_x:
> ^
> arch/powerpc/kernel/vdso32/gettimeofday.S:73:8: error: unsupported directive '.stabs'
> .stabs "_rest32gpr_31_x:F-1",36,0,0,_rest32gpr_31_x; .globl _rest32gpr_31_x; _rest32gpr_31_x:
How should we fix that ? Will clang support .stabs in the future ?
> ^
> make[2]: *** [arch/powerpc/kernel/vdso32/Makefile:55: arch/powerpc/kernel/vdso32/gettimeofday.o] Error 1
> make[2]: Target 'include/generated/vdso32-offsets.h' not remade because of errors.
> make[1]: *** [arch/powerpc/Makefile:421: vdso_prepare] Error 2
> make[1]: Target 'prepare' not remade because of errors.
> make: *** [Makefile:219: __sub-make] Error 2
> make: Target 'prepare' not remade because of errors.
>
>
> vim +/val +165 arch/powerpc/include/asm/inst.h
>
> 152
> 153 static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
> 154 {
> 155 unsigned int val, suffix;
> 156
> 157 if (unlikely(!is_kernel_addr((unsigned long)src)))
> 158 return -ERANGE;
> 159
> 160 __get_kernel_nofault(&val, src, u32, Efault);
> 161 if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
> 162 __get_kernel_nofault(&suffix, src + 1, u32, Efault);
> 163 *inst = ppc_inst_prefix(val, suffix);
> 164 } else {
> > 165 *inst = ppc_inst(val);
> 166 }
> 167 return 0;
> 168 Efault:
> 169 return -EFAULT;
> 170 }
> 171
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()
2021-11-30 5:58 ` Christophe Leroy
@ 2021-11-30 11:25 ` Michael Ellerman
2021-11-30 18:17 ` Nathan Chancellor
0 siblings, 1 reply; 13+ messages in thread
From: Michael Ellerman @ 2021-11-30 11:25 UTC (permalink / raw)
To: Christophe Leroy, kernel test robot, Benjamin Herrenschmidt,
Paul Mackerras, Nick Desaulniers
Cc: llvm, kbuild-all, linuxppc-dev, linux-kernel
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 29/11/2021 à 23:55, kernel test robot a écrit :
>> Hi Christophe,
>>
>> I love your patch! Perhaps something to improve:
>>
>> [auto build test WARNING on powerpc/next]
>> [also build test WARNING on v5.16-rc3 next-20211129]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch]
>>
>> url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
>> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
>> config: powerpc-randconfig-r023-20211129 (https://download.01.org/0day-ci/archive/20211130/202111300652.0yDBNvyJ-lkp@intel.com/config)
>> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
>> reproduce (this is a W=1 build):
>> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>> chmod +x ~/bin/make.cross
>> # install powerpc cross compiling tool for clang build
>> # apt-get install binutils-powerpc-linux-gnu
>> # https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826
>> git remote add linux-review https://github.com/0day-ci/linux
>> git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
>> git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826
>> # save the config file to linux build tree
>> mkdir build_dir
>> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <lkp@intel.com>
>>
>> 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.
>
> We have
>
> __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);
> *inst = ppc_inst_prefix(val, suffix);
> } else {
> *inst = ppc_inst(val);
> }
>
> With
>
> #define __get_kernel_nofault(dst, src, type, err_label) \
> __get_user_size_goto(*((type *)(dst)), \
> (__force type __user *)(src), sizeof(type), err_label)
>
>
> And
>
> #define __get_user_size_goto(x, ptr, size, label) \
> do { \
> BUILD_BUG_ON(size > sizeof(x)); \
> switch (size) { \
> case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break; \
> case 2: __get_user_asm_goto(x, (u16 __user *)ptr, label, "lhz"); break; \
> case 4: __get_user_asm_goto(x, (u32 __user *)ptr, label, "lwz"); break; \
> case 8: __get_user_asm2_goto(x, (u64 __user *)ptr, label); break; \
> default: x = 0; BUILD_BUG(); \
> } \
> } while (0)
>
> And
>
> #define __get_user_asm_goto(x, addr, label, op) \
> asm_volatile_goto( \
> "1: "op"%U1%X1 %0, %1 # get_user\n" \
> EX_TABLE(1b, %l2) \
> : "=r" (x) \
> : "m<>" (*addr) \
> : \
> : label)
>
>
> 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?
>> 1 warning generated.
>> <stdin>:1559:2: warning: syscall futex_waitv not implemented [-W#warnings]
>> #warning syscall futex_waitv not implemented
>> ^
>> 1 warning generated.
>> arch/powerpc/kernel/vdso32/gettimeofday.S:72:8: error: unsupported directive '.stabs'
>> .stabs "_restgpr_31_x:F-1",36,0,0,_restgpr_31_x; .globl _restgpr_31_x; _restgpr_31_x:
>> ^
>> arch/powerpc/kernel/vdso32/gettimeofday.S:73:8: error: unsupported directive '.stabs'
>> .stabs "_rest32gpr_31_x:F-1",36,0,0,_rest32gpr_31_x; .globl _rest32gpr_31_x; _rest32gpr_31_x:
>
> How should we fix that ? Will clang support .stabs in the future ?
I think we should drop any stabs annotations / support. AFAICT none of
the toolchains we support produce it anymore.
cheers
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()
2021-11-30 11:25 ` Michael Ellerman
@ 2021-11-30 18:17 ` Nathan Chancellor
2021-11-30 18:38 ` Bill Wendling
0 siblings, 1 reply; 13+ messages in thread
From: Nathan Chancellor @ 2021-11-30 18:17 UTC (permalink / raw)
To: Michael Ellerman
Cc: Christophe Leroy, kernel test robot, Benjamin Herrenschmidt,
Paul Mackerras, Nick Desaulniers, llvm, kbuild-all, linuxppc-dev,
linux-kernel, Bill Wendling
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 :
> >> Hi Christophe,
> >>
> >> I love your patch! Perhaps something to improve:
> >>
> >> [auto build test WARNING on powerpc/next]
> >> [also build test WARNING on v5.16-rc3 next-20211129]
> >> [If your patch is applied to the wrong git tree, kindly drop us a note.
> >> And when submitting patch, we suggest to use '--base' as documented in
> >> https://git-scm.com/docs/git-format-patch]
> >>
> >> url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
> >> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> >> config: powerpc-randconfig-r023-20211129 (https://download.01.org/0day-ci/archive/20211130/202111300652.0yDBNvyJ-lkp@intel.com/config)
> >> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
> >> reproduce (this is a W=1 build):
> >> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >> chmod +x ~/bin/make.cross
> >> # install powerpc cross compiling tool for clang build
> >> # apt-get install binutils-powerpc-linux-gnu
> >> # https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826
> >> git remote add linux-review https://github.com/0day-ci/linux
> >> git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
> >> git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826
> >> # save the config file to linux build tree
> >> mkdir build_dir
> >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare
> >>
> >> If you fix the issue, kindly add following tag as appropriate
> >> Reported-by: kernel test robot <lkp@intel.com>
> >>
> >> 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.
> >
> > We have
> >
> > __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);
> > *inst = ppc_inst_prefix(val, suffix);
> > } else {
> > *inst = ppc_inst(val);
> > }
> >
> > With
> >
> > #define __get_kernel_nofault(dst, src, type, err_label) \
> > __get_user_size_goto(*((type *)(dst)), \
> > (__force type __user *)(src), sizeof(type), err_label)
> >
> >
> > And
> >
> > #define __get_user_size_goto(x, ptr, size, label) \
> > do { \
> > BUILD_BUG_ON(size > sizeof(x)); \
> > switch (size) { \
> > case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break; \
> > case 2: __get_user_asm_goto(x, (u16 __user *)ptr, label, "lhz"); break; \
> > case 4: __get_user_asm_goto(x, (u32 __user *)ptr, label, "lwz"); break; \
> > case 8: __get_user_asm2_goto(x, (u64 __user *)ptr, label); break; \
> > default: x = 0; BUILD_BUG(); \
> > } \
> > } while (0)
> >
> > And
> >
> > #define __get_user_asm_goto(x, addr, label, op) \
> > asm_volatile_goto( \
> > "1: "op"%U1%X1 %0, %1 # get_user\n" \
> > EX_TABLE(1b, %l2) \
> > : "=r" (x) \
> > : "m<>" (*addr) \
> > : \
> > : label)
> >
> >
> > 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?
A simplified reproducer:
$ cat test.c
static inline int copy_inst_from_kernel_nofault(unsigned int *inst,
unsigned int *src)
{
unsigned int val;
asm goto("1: lwz %U1%X1 %0, %1 # get_user\n"
".section __ex_table,\"a\";"
".balign 4;"
".long (1b) - . ;"
".long (%l2) - . ;"
".previous"
: "=r" (*(unsigned int *)(&val))
: "m<>" (*(unsigned int *)(src))
:
: Efault);
*inst = val;
return 0;
Efault:
return -14; /* -EFAULT */
}
$ clang --target=powerpc-linux-gnu -Wuninitialized -fsyntax-only test.c
test.c:17:10: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
*inst = val;
^~~
test.c:4:18: note: initialize the variable 'val' to silence this warning
unsigned int val;
^
= 0
1 warning generated.
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).
Cheers,
Nathan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()
2021-11-30 18:17 ` Nathan Chancellor
@ 2021-11-30 18:38 ` Bill Wendling
2021-11-30 18:44 ` Bill Wendling
0 siblings, 1 reply; 13+ messages in thread
From: Bill Wendling @ 2021-11-30 18:38 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Michael Ellerman, Christophe Leroy, kernel test robot,
Benjamin Herrenschmidt, Paul Mackerras, Nick Desaulniers, llvm,
kbuild-all, linuxppc-dev, linux-kernel
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 :
> > >> Hi Christophe,
> > >>
> > >> I love your patch! Perhaps something to improve:
> > >>
> > >> [auto build test WARNING on powerpc/next]
> > >> [also build test WARNING on v5.16-rc3 next-20211129]
> > >> [If your patch is applied to the wrong git tree, kindly drop us a note.
> > >> And when submitting patch, we suggest to use '--base' as documented in
> > >> https://git-scm.com/docs/git-format-patch]
> > >>
> > >> url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
> > >> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> > >> config: powerpc-randconfig-r023-20211129 (https://download.01.org/0day-ci/archive/20211130/202111300652.0yDBNvyJ-lkp@intel.com/config)
> > >> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
> > >> reproduce (this is a W=1 build):
> > >> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > >> chmod +x ~/bin/make.cross
> > >> # install powerpc cross compiling tool for clang build
> > >> # apt-get install binutils-powerpc-linux-gnu
> > >> # https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826
> > >> git remote add linux-review https://github.com/0day-ci/linux
> > >> git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
> > >> git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826
> > >> # save the config file to linux build tree
> > >> mkdir build_dir
> > >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare
> > >>
> > >> If you fix the issue, kindly add following tag as appropriate
> > >> Reported-by: kernel test robot <lkp@intel.com>
> > >>
> > >> 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.
> > >
> > > We have
> > >
> > > __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);
> > > *inst = ppc_inst_prefix(val, suffix);
> > > } else {
> > > *inst = ppc_inst(val);
> > > }
> > >
> > > With
> > >
> > > #define __get_kernel_nofault(dst, src, type, err_label) \
> > > __get_user_size_goto(*((type *)(dst)), \
> > > (__force type __user *)(src), sizeof(type), err_label)
> > >
> > >
> > > And
> > >
> > > #define __get_user_size_goto(x, ptr, size, label) \
> > > do { \
> > > BUILD_BUG_ON(size > sizeof(x)); \
> > > switch (size) { \
> > > case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break; \
> > > case 2: __get_user_asm_goto(x, (u16 __user *)ptr, label, "lhz"); break; \
> > > case 4: __get_user_asm_goto(x, (u32 __user *)ptr, label, "lwz"); break; \
> > > case 8: __get_user_asm2_goto(x, (u64 __user *)ptr, label); break; \
> > > default: x = 0; BUILD_BUG(); \
> > > } \
> > > } while (0)
> > >
> > > And
> > >
> > > #define __get_user_asm_goto(x, addr, label, op) \
> > > asm_volatile_goto( \
> > > "1: "op"%U1%X1 %0, %1 # get_user\n" \
> > > EX_TABLE(1b, %l2) \
> > > : "=r" (x) \
> > > : "m<>" (*addr) \
> > > : \
> > > : label)
> > >
> > >
> > > 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?
>
> A simplified reproducer:
>
> $ cat test.c
> static inline int copy_inst_from_kernel_nofault(unsigned int *inst,
> unsigned int *src)
> {
> unsigned int val;
>
> asm goto("1: lwz %U1%X1 %0, %1 # get_user\n"
> ".section __ex_table,\"a\";"
> ".balign 4;"
> ".long (1b) - . ;"
> ".long (%l2) - . ;"
> ".previous"
> : "=r" (*(unsigned int *)(&val))
> : "m<>" (*(unsigned int *)(src))
> :
> : Efault);
>
> *inst = val;
> return 0;
>
> Efault:
> return -14; /* -EFAULT */
> }
>
> $ clang --target=powerpc-linux-gnu -Wuninitialized -fsyntax-only test.c
> test.c:17:10: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
> *inst = val;
> ^~~
> test.c:4:18: note: initialize the variable 'val' to silence this warning
> unsigned int val;
> ^
> = 0
> 1 warning generated.
>
> 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.
-bw
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()
2021-11-30 18:38 ` Bill Wendling
@ 2021-11-30 18:44 ` Bill Wendling
2021-12-07 3:37 ` Michael Ellerman
0 siblings, 1 reply; 13+ messages in thread
From: Bill Wendling @ 2021-11-30 18:44 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Michael Ellerman, Christophe Leroy, kernel test robot,
Benjamin Herrenschmidt, Paul Mackerras, Nick Desaulniers, llvm,
kbuild-all, linuxppc-dev, linux-kernel
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 :
> > > >> Hi Christophe,
> > > >>
> > > >> I love your patch! Perhaps something to improve:
> > > >>
> > > >> [auto build test WARNING on powerpc/next]
> > > >> [also build test WARNING on v5.16-rc3 next-20211129]
> > > >> [If your patch is applied to the wrong git tree, kindly drop us a note.
> > > >> And when submitting patch, we suggest to use '--base' as documented in
> > > >> https://git-scm.com/docs/git-format-patch]
> > > >>
> > > >> url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
> > > >> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> > > >> config: powerpc-randconfig-r023-20211129 (https://download.01.org/0day-ci/archive/20211130/202111300652.0yDBNvyJ-lkp@intel.com/config)
> > > >> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
> > > >> reproduce (this is a W=1 build):
> > > >> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > > >> chmod +x ~/bin/make.cross
> > > >> # install powerpc cross compiling tool for clang build
> > > >> # apt-get install binutils-powerpc-linux-gnu
> > > >> # https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826
> > > >> git remote add linux-review https://github.com/0day-ci/linux
> > > >> git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
> > > >> git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826
> > > >> # save the config file to linux build tree
> > > >> mkdir build_dir
> > > >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare
> > > >>
> > > >> If you fix the issue, kindly add following tag as appropriate
> > > >> Reported-by: kernel test robot <lkp@intel.com>
> > > >>
> > > >> 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.
> > > >
> > > > We have
> > > >
> > > > __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);
> > > > *inst = ppc_inst_prefix(val, suffix);
> > > > } else {
> > > > *inst = ppc_inst(val);
> > > > }
> > > >
> > > > With
> > > >
> > > > #define __get_kernel_nofault(dst, src, type, err_label) \
> > > > __get_user_size_goto(*((type *)(dst)), \
> > > > (__force type __user *)(src), sizeof(type), err_label)
> > > >
> > > >
> > > > And
> > > >
> > > > #define __get_user_size_goto(x, ptr, size, label) \
> > > > do { \
> > > > BUILD_BUG_ON(size > sizeof(x)); \
> > > > switch (size) { \
> > > > case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break; \
> > > > case 2: __get_user_asm_goto(x, (u16 __user *)ptr, label, "lhz"); break; \
> > > > case 4: __get_user_asm_goto(x, (u32 __user *)ptr, label, "lwz"); break; \
> > > > case 8: __get_user_asm2_goto(x, (u64 __user *)ptr, label); break; \
> > > > default: x = 0; BUILD_BUG(); \
> > > > } \
> > > > } while (0)
> > > >
> > > > And
> > > >
> > > > #define __get_user_asm_goto(x, addr, label, op) \
> > > > asm_volatile_goto( \
> > > > "1: "op"%U1%X1 %0, %1 # get_user\n" \
> > > > EX_TABLE(1b, %l2) \
> > > > : "=r" (x) \
> > > > : "m<>" (*addr) \
> > > > : \
> > > > : label)
> > > >
> > > >
> > > > 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?
> >
> > A simplified reproducer:
> >
> > $ cat test.c
> > static inline int copy_inst_from_kernel_nofault(unsigned int *inst,
> > unsigned int *src)
> > {
> > unsigned int val;
> >
> > asm goto("1: lwz %U1%X1 %0, %1 # get_user\n"
> > ".section __ex_table,\"a\";"
> > ".balign 4;"
> > ".long (1b) - . ;"
> > ".long (%l2) - . ;"
> > ".previous"
> > : "=r" (*(unsigned int *)(&val))
> > : "m<>" (*(unsigned int *)(src))
> > :
> > : Efault);
> >
> > *inst = val;
> > return 0;
> >
> > Efault:
> > return -14; /* -EFAULT */
> > }
> >
> > $ clang --target=powerpc-linux-gnu -Wuninitialized -fsyntax-only test.c
> > test.c:17:10: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
> > *inst = val;
> > ^~~
> > test.c:4:18: note: initialize the variable 'val' to silence this warning
> > unsigned int val;
> > ^
> > = 0
> > 1 warning generated.
> >
> > 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 "*&")
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()
[not found] <0d5b12183d5176dd702d29ad94c39c384e51c78f.1638208156.git.christophe.leroy@csgroup.eu>
2021-11-29 22:55 ` [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault() kernel test robot
@ 2021-12-01 6:45 ` kernel test robot
1 sibling, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-12-01 6:45 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: llvm, kbuild-all, linuxppc-dev, linux-kernel
Hi Christophe,
I love your patch! Perhaps something to improve:
[auto build test WARNING on powerpc/next]
[also build test WARNING on v5.16-rc3 next-20211201]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc64-randconfig-r021-20211201 (https://download.01.org/0day-ci/archive/20211201/202112011435.ttoYYtbC-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 25eb7fa01d7ebbe67648ea03841cda55b4239ab2)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc64 cross compiling tool for clang build
# apt-get install binutils-powerpc64-linux-gnu
# https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:143:1: note: expanded from here
__do_insl
^
arch/powerpc/include/asm/io.h:558:56: note: expanded from macro '__do_insl'
#define __do_insl(p, b, n) readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/asm-offsets.c:21:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:145:1: note: expanded from here
__do_outsb
^
arch/powerpc/include/asm/io.h:559:58: note: expanded from macro '__do_outsb'
#define __do_outsb(p, b, n) writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/asm-offsets.c:21:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:147:1: note: expanded from here
__do_outsw
^
arch/powerpc/include/asm/io.h:560:58: note: expanded from macro '__do_outsw'
#define __do_outsw(p, b, n) writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/asm-offsets.c:21:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:149:1: note: expanded from here
__do_outsl
^
arch/powerpc/include/asm/io.h:561:58: note: expanded from macro '__do_outsl'
#define __do_outsl(p, b, n) writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
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:161:41: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
^~~
arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning
unsigned int val, suffix;
^
= 0
>> arch/powerpc/include/asm/inst.h:163:32: warning: variable 'suffix' is uninitialized when used here [-Wuninitialized]
*inst = ppc_inst_prefix(val, suffix);
^~~~~~
arch/powerpc/include/asm/inst.h:62:69: note: expanded from macro 'ppc_inst_prefix'
#define ppc_inst_prefix(x, y) ((ppc_inst_t){ .val = (x), .suffix = (y) })
^
arch/powerpc/include/asm/inst.h:155:26: note: initialize the variable 'suffix' to silence this warning
unsigned int val, suffix;
^
= 0
14 warnings generated.
<stdin>:1559:2: warning: syscall futex_waitv not implemented [-W#warnings]
#warning syscall futex_waitv not implemented
^
1 warning generated.
/usr/bin/ld: unrecognised emulation mode: elf64ppc
Supported emulations: elf_x86_64 elf32_x86_64 elf_i386 elf_iamcu elf_l1om elf_k1om i386pep i386pe
clang-14: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [arch/powerpc/kernel/vdso64/Makefile:44: arch/powerpc/kernel/vdso64/vdso64.so.dbg] Error 1
make[2]: Target 'include/generated/vdso64-offsets.h' not remade because of errors.
make[1]: *** [arch/powerpc/Makefile:422: vdso_prepare] Error 2
make[1]: Target 'prepare' not remade because of errors.
make: *** [Makefile:219: __sub-make] Error 2
make: Target 'prepare' not remade because of errors.
vim +/suffix +163 arch/powerpc/include/asm/inst.h
152
153 static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
154 {
155 unsigned int val, suffix;
156
157 if (unlikely(!is_kernel_addr((unsigned long)src)))
158 return -ERANGE;
159
160 __get_kernel_nofault(&val, src, u32, Efault);
161 if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
162 __get_kernel_nofault(&suffix, src + 1, u32, Efault);
> 163 *inst = ppc_inst_prefix(val, suffix);
164 } else {
165 *inst = ppc_inst(val);
166 }
167 return 0;
168 Efault:
169 return -EFAULT;
170 }
171
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()
2021-11-30 18:44 ` Bill Wendling
@ 2021-12-07 3:37 ` Michael Ellerman
2021-12-07 4:48 ` Nathan Chancellor
0 siblings, 1 reply; 13+ messages in thread
From: Michael Ellerman @ 2021-12-07 3:37 UTC (permalink / raw)
To: Bill Wendling, Nathan Chancellor
Cc: Christophe Leroy, kernel test robot, Benjamin Herrenschmidt,
Paul Mackerras, Nick Desaulniers, llvm, kbuild-all, linuxppc-dev,
linux-kernel
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;
+#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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()
2021-12-07 3:37 ` Michael Ellerman
@ 2021-12-07 4:48 ` Nathan Chancellor
2021-12-07 5:45 ` Christophe Leroy
2021-12-07 11:19 ` Michael Ellerman
0 siblings, 2 replies; 13+ messages in thread
From: Nathan Chancellor @ 2021-12-07 4:48 UTC (permalink / raw)
To: Michael Ellerman
Cc: Bill Wendling, Christophe Leroy, kernel test robot,
Benjamin Herrenschmidt, Paul Mackerras, Nick Desaulniers, llvm,
kbuild-all, linuxppc-dev, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()
2021-12-07 4:48 ` Nathan Chancellor
@ 2021-12-07 5:45 ` Christophe Leroy
2021-12-07 6:41 ` Nathan Chancellor
2021-12-07 11:19 ` Michael Ellerman
1 sibling, 1 reply; 13+ messages in thread
From: Christophe Leroy @ 2021-12-07 5:45 UTC (permalink / raw)
To: Nathan Chancellor, Michael Ellerman
Cc: Bill Wendling, 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
Le 07/12/2021 à 05:48, Nathan Chancellor a écrit :
> 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
The robot reported the problem on:
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project
df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
Should it be 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);
>>
Christophe
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()
2021-12-07 5:45 ` Christophe Leroy
@ 2021-12-07 6:41 ` Nathan Chancellor
2021-12-07 7:55 ` Christophe Leroy
0 siblings, 1 reply; 13+ messages in thread
From: Nathan Chancellor @ 2021-12-07 6:41 UTC (permalink / raw)
To: Christophe Leroy
Cc: Michael Ellerman, Bill Wendling, 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
On Tue, Dec 07, 2021 at 05:45:08AM +0000, Christophe Leroy wrote:
>
>
> Le 07/12/2021 à 05:48, Nathan Chancellor a écrit :
> > 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
>
> The robot reported the problem on:
>
> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project
> df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
>
> Should it be CONFIG_CLANG_VERSION <= 140000 ?
The robot tests clang from tip of tree, rebuilding every week or so. The
fix is getting ready to land so it will be released in 14.0.0 final. We
have always written tip of tree version checks with the expectation that
if people are testing tip of tree clang, they are frequently rebuilding.
If that is not true, they need to be using released/stable versions,
otherwise the model is broken.
If that is too problematic, we could add a version check to Kconfig
(cannot think of a great name for the config off the top of my head)
that checks for this issue and ifdef on that. That might be nice in
case another instance of this crops up in the future.
Cheers,
Nathan
> >
> >> +#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);
> >>
>
> Christophe
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()
2021-12-07 6:41 ` Nathan Chancellor
@ 2021-12-07 7:55 ` Christophe Leroy
0 siblings, 0 replies; 13+ messages in thread
From: Christophe Leroy @ 2021-12-07 7:55 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Michael Ellerman, Bill Wendling, 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
Le 07/12/2021 à 07:41, Nathan Chancellor a écrit :
> On Tue, Dec 07, 2021 at 05:45:08AM +0000, Christophe Leroy wrote:
>>
>>
>> Le 07/12/2021 à 05:48, Nathan Chancellor a écrit :
>>> 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
>>
>> The robot reported the problem on:
>>
>> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project
>> df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
>>
>> Should it be CONFIG_CLANG_VERSION <= 140000 ?
>
> The robot tests clang from tip of tree, rebuilding every week or so. The
> fix is getting ready to land so it will be released in 14.0.0 final. We
> have always written tip of tree version checks with the expectation that
> if people are testing tip of tree clang, they are frequently rebuilding.
> If that is not true, they need to be using released/stable versions,
> otherwise the model is broken.
>
> If that is too problematic, we could add a version check to Kconfig
> (cannot think of a great name for the config off the top of my head)
> that checks for this issue and ifdef on that. That might be nice in
> case another instance of this crops up in the future.
>
It's fine for me. I didn't know robot was using prereleases with the
same name as the future release.
Christophe
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()
2021-12-07 4:48 ` Nathan Chancellor
2021-12-07 5:45 ` Christophe Leroy
@ 2021-12-07 11:19 ` Michael Ellerman
1 sibling, 0 replies; 13+ messages in thread
From: Michael Ellerman @ 2021-12-07 11:19 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Bill Wendling, Christophe Leroy, kernel test robot,
Benjamin Herrenschmidt, Paul Mackerras, Nick Desaulniers, llvm,
kbuild-all, linuxppc-dev, linux-kernel
Nathan Chancellor <nathan@kernel.org> writes:
> 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
Yep, thanks I'll use that.
cheers
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-12-07 11:19 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <0d5b12183d5176dd702d29ad94c39c384e51c78f.1638208156.git.christophe.leroy@csgroup.eu>
2021-11-29 22:55 ` [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault() 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
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox