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 à 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 > > >> > > >> 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