Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()

Christophe Leroy
2021-12-07T05:45:13Z

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