On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling wrote: > > 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. > Small retraction. It's the "*()&val" that's the issue. (I.e. the "*&")