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

Nathan Chancellor [off-list ref] writes:

On Tue, Dec 07, 2021 at 02:37:26PM +1100, Michael Ellerman wrote:

quoted

Bill Wendling [off-list ref] writes:

quoted

On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling [off-list ref] wrote:

quoted

On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor [off-list ref] wrote:

quoted

On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote:

quoted

Christophe Leroy [off-list ref] writes:

quoted

Le 29/11/2021 à 23:55, kernel test robot a écrit :

...

quoted

quoted

quoted

quoted

quoted

quoted

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:

quoted

quoted

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.

...

quoted

quoted

quoted

quoted

quoted

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?

...

quoted

quoted

quoted

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