Re: [PATCH v2 2/2] powerpc/math-emu: Remove -w build flag and fix warnings
Hi! On Fri, Sep 02, 2022 at 09:11:48AM -0700, Nathan Chancellor wrote: > On Fri, Sep 02, 2022 at 10:59:54AM -0500, Segher Boessenkool wrote: > > Maybe add -Wno-implicit-fallthrough? This code is a copy from outside > > the kernel, no one has ever wanted to maintain it, if nothing else (the > > more politically correct formulation is "we cannot as easily pick up > > improvements from upstream if we modify stuff"). > > Sure, we could do something like this if you preferred: > > diff --git a/arch/powerpc/math-emu/Makefile b/arch/powerpc/math-emu/Makefile > index 26fef2e5672e..ed775747a2a5 100644 > --- a/arch/powerpc/math-emu/Makefile > +++ b/arch/powerpc/math-emu/Makefile > @@ -16,3 +16,7 @@ obj-$(CONFIG_SPE) += math_efp.o > > CFLAGS_fabs.o = -fno-builtin-fabs > CFLAGS_math.o = -fno-builtin-fabs > + > +ifdef CONFIG_CC_IS_CLANG > +ccflags-remove-y := $(CONFIG_CC_IMPLICIT_FALLTHROUGH) > +endif That is a GCC warning as well. It needs some $(call cc-option ...) thing then, though (GCC versions of more than two or so years ago are supported as well). > At the same time, I see other modifications to these files that appear > to be for the kernel only so I suspect that this is already in the "we > cannot as easily pick up improvements from upstream" category, > regardless of that diff. So maybe someone should really maintain this stuff, bring it up to some reasonably modern state? :-) Segher
Re: [PATCH v2 2/2] powerpc/math-emu: Remove -w build flag and fix warnings
On Fri, Sep 02, 2022 at 10:59:54AM -0500, Segher Boessenkool wrote: > On Fri, Sep 02, 2022 at 08:37:23AM -0700, Nathan Chancellor wrote: > > On Fri, Sep 02, 2022 at 12:08:55PM +0200, Christophe Leroy wrote: > > > This should have been detected by gcc at build time, but due to > > > '-w' flag it went undetected. > > > > > > Removing that flag leads to many warnings hence errors. > > > Thanks for figuring out what was going on here! I took this patch for a > > spin with clang and it has a few more errors around > > -Wimplicit-fallthrough: > > Maybe add -Wno-implicit-fallthrough? This code is a copy from outside > the kernel, no one has ever wanted to maintain it, if nothing else (the > more politically correct formulation is "we cannot as easily pick up > improvements from upstream if we modify stuff"). Sure, we could do something like this if you preferred: diff --git a/arch/powerpc/math-emu/Makefile b/arch/powerpc/math-emu/Makefile index 26fef2e5672e..ed775747a2a5 100644 --- a/arch/powerpc/math-emu/Makefile +++ b/arch/powerpc/math-emu/Makefile @@ -16,3 +16,7 @@ obj-$(CONFIG_SPE) += math_efp.o CFLAGS_fabs.o = -fno-builtin-fabs CFLAGS_math.o = -fno-builtin-fabs + +ifdef CONFIG_CC_IS_CLANG +ccflags-remove-y := $(CONFIG_CC_IMPLICIT_FALLTHROUGH) +endif At the same time, I see other modifications to these files that appear to be for the kernel only so I suspect that this is already in the "we cannot as easily pick up improvements from upstream" category, regardless of that diff. No strong opinion from me, although I see Christophe already included my suggestion in the most recent series: https://lore.kernel.org/2663961738a46073713786d4efeb53100ca156e7.1662134272.git.christophe.le...@csgroup.eu/ Cheers, Nathan
Re: [PATCH v2 2/2] powerpc/math-emu: Remove -w build flag and fix warnings
Le 02/09/2022 à 17:59, Segher Boessenkool a écrit :
> On Fri, Sep 02, 2022 at 08:37:23AM -0700, Nathan Chancellor wrote:
>> On Fri, Sep 02, 2022 at 12:08:55PM +0200, Christophe Leroy wrote:
>>> This should have been detected by gcc at build time, but due to
>>> '-w' flag it went undetected.
>>>
>>> Removing that flag leads to many warnings hence errors.
>
>> Thanks for figuring out what was going on here! I took this patch for a
>> spin with clang and it has a few more errors around
>> -Wimplicit-fallthrough:
>
> Maybe add -Wno-implicit-fallthrough? This code is a copy from outside
> the kernel, no one has ever wanted to maintain it, if nothing else (the
> more politically correct formulation is "we cannot as easily pick up
> improvements from upstream if we modify stuff").
>
There are already such changes in that common file, see for instance
commit f336a009f8e3 ("math-emu: Fix fall-through warning"), was in July
2021.
Christophe
Re: [PATCH v2 2/2] powerpc/math-emu: Remove -w build flag and fix warnings
On Fri, Sep 02, 2022 at 08:37:23AM -0700, Nathan Chancellor wrote: > On Fri, Sep 02, 2022 at 12:08:55PM +0200, Christophe Leroy wrote: > > This should have been detected by gcc at build time, but due to > > '-w' flag it went undetected. > > > > Removing that flag leads to many warnings hence errors. > Thanks for figuring out what was going on here! I took this patch for a > spin with clang and it has a few more errors around > -Wimplicit-fallthrough: Maybe add -Wno-implicit-fallthrough? This code is a copy from outside the kernel, no one has ever wanted to maintain it, if nothing else (the more politically correct formulation is "we cannot as easily pick up improvements from upstream if we modify stuff"). Segher
[PATCH v2 2/2] powerpc/math-emu: Remove -w build flag and fix warnings
As reported by Nathan, the module_init() macro was not taken into
account because the header was missing. That means spe_mathemu_init()
was never called.
This should have been detected by gcc at build time, but due to
'-w' flag it went undetected.
Removing that flag leads to many warnings hence errors.
Fix those warnings then remove the -w flag.
Reported-by: Nathan Chancellor
Signed-off-by: Christophe Leroy
Reviewed-by: Nathan Chancellor
---
v2: Added 3 fallthrough; in include/math-emu/op-common.h for the sake of CLANG.
---
arch/powerpc/math-emu/Makefile | 2 --
arch/powerpc/math-emu/math.c | 18 +-
arch/powerpc/math-emu/math_efp.c | 57 +---
include/math-emu/op-common.h | 3 ++
4 files changed, 42 insertions(+), 38 deletions(-)
diff --git a/arch/powerpc/math-emu/Makefile b/arch/powerpc/math-emu/Makefile
index a8794032f15f..26fef2e5672e 100644
--- a/arch/powerpc/math-emu/Makefile
+++ b/arch/powerpc/math-emu/Makefile
@@ -16,5 +16,3 @@ obj-$(CONFIG_SPE) += math_efp.o
CFLAGS_fabs.o = -fno-builtin-fabs
CFLAGS_math.o = -fno-builtin-fabs
-
-ccflags-y = -w
diff --git a/arch/powerpc/math-emu/math.c b/arch/powerpc/math-emu/math.c
index 36761bd00f38..936a9a149037 100644
--- a/arch/powerpc/math-emu/math.c
+++ b/arch/powerpc/math-emu/math.c
@@ -24,9 +24,9 @@ FLOATFUNC(mtfsf);
FLOATFUNC(mtfsfi);
#ifdef CONFIG_MATH_EMULATION_HW_UNIMPLEMENTED
-#undef FLOATFUNC(x)
+#undef FLOATFUNC
#define FLOATFUNC(x) static inline int x(void *op1, void *op2, void *op3, \
-void *op4) { }
+void *op4) { return 0; }
#endif
FLOATFUNC(fadd);
@@ -396,28 +396,28 @@ do_mathemu(struct pt_regs *regs)
case XCR:
op0 = (void *)>ccr;
- op1 = (void *)((insn >> 23) & 0x7);
+ op1 = (void *)(long)((insn >> 23) & 0x7);
op2 = (void *)>thread.TS_FPR((insn >> 16) & 0x1f);
op3 = (void *)>thread.TS_FPR((insn >> 11) & 0x1f);
break;
case XCRL:
op0 = (void *)>ccr;
- op1 = (void *)((insn >> 23) & 0x7);
- op2 = (void *)((insn >> 18) & 0x7);
+ op1 = (void *)(long)((insn >> 23) & 0x7);
+ op2 = (void *)(long)((insn >> 18) & 0x7);
break;
case XCRB:
- op0 = (void *)((insn >> 21) & 0x1f);
+ op0 = (void *)(long)((insn >> 21) & 0x1f);
break;
case XCRI:
- op0 = (void *)((insn >> 23) & 0x7);
- op1 = (void *)((insn >> 12) & 0xf);
+ op0 = (void *)(long)((insn >> 23) & 0x7);
+ op1 = (void *)(long)((insn >> 12) & 0xf);
break;
case XFLB:
- op0 = (void *)((insn >> 17) & 0xff);
+ op0 = (void *)(long)((insn >> 17) & 0xff);
op1 = (void *)>thread.TS_FPR((insn >> 11) & 0x1f);
break;
diff --git a/arch/powerpc/math-emu/math_efp.c b/arch/powerpc/math-emu/math_efp.c
index aa3bb8da1cb9..f01e3475f689 100644
--- a/arch/powerpc/math-emu/math_efp.c
+++ b/arch/powerpc/math-emu/math_efp.c
@@ -219,6 +219,7 @@ int do_spe_mathemu(struct pt_regs *regs)
case AB:
case XCR:
FP_UNPACK_SP(SA, va.wp + 1);
+ fallthrough;
case XB:
FP_UNPACK_SP(SB, vb.wp + 1);
break;
@@ -227,8 +228,8 @@ int do_spe_mathemu(struct pt_regs *regs)
break;
}
- pr_debug("SA: %ld %08lx %ld (%ld)\n", SA_s, SA_f, SA_e, SA_c);
- pr_debug("SB: %ld %08lx %ld (%ld)\n", SB_s, SB_f, SB_e, SB_c);
+ pr_debug("SA: %d %08x %d (%d)\n", SA_s, SA_f, SA_e, SA_c);
+ pr_debug("SB: %d %08x %d (%d)\n", SB_s, SB_f, SB_e, SB_c);
switch (func) {
case EFSABS:
@@ -279,7 +280,7 @@ int do_spe_mathemu(struct pt_regs *regs)
} else {
SB_e += (func == EFSCTSF ? 31 : 32);
FP_TO_INT_ROUND_S(vc.wp[1], SB, 32,
- (func == EFSCTSF));
+ (func == EFSCTSF) ? 1 : 0);
}
goto update_regs;
@@ -288,7 +289,7 @@ int do_spe_mathemu(struct pt_regs *regs)
FP_CLEAR_EXCEPTIONS;
FP_UNPACK_DP(DB, vb.dp);
- pr_debug("DB: %ld %08lx %08lx %ld (%ld)\n",
+ pr_debug("DB: %d %08x %08x %d (%d)\n",
DB_s, DB_f1, DB_f0, DB_e, DB_c);
FP_CONV(S, D, 1, 2, SR, DB);
@@ -302,7 +303,7 @@ int do_spe_mathemu(struct pt_regs *regs)
Re: [PATCH v2 2/2] powerpc/math-emu: Remove -w build flag and fix warnings
Hi Christophe, On Fri, Sep 02, 2022 at 12:08:55PM +0200, Christophe Leroy wrote: > As reported by Nathan, the module_init() macro was not taken into > account because the header was missing. That means spe_mathemu_init() > was never called. > > This should have been detected by gcc at build time, but due to > '-w' flag it went undetected. > > Removing that flag leads to many warnings hence errors. > > Fix those warnings then remove the -w flag. > > Reported-by: Nathan Chancellor > Signed-off-by: Christophe Leroy Thanks for figuring out what was going on here! I took this patch for a spin with clang and it has a few more errors around -Wimplicit-fallthrough: arch/powerpc/math-emu/fctiw.c:18:2: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough] FP_TO_INT_D(r, B, 32, 1); ^ ./include/math-emu/double.h:120:34: note: expanded from macro 'FP_TO_INT_D' #define FP_TO_INT_D(r,X,rsz,rsg)_FP_TO_INT(D,2,r,X,rsz,rsg) ^ ./include/math-emu/op-common.h:665:4: note: expanded from macro '_FP_TO_INT' case FP_CLS_ZERO: \ ^ arch/powerpc/math-emu/fctiw.c:18:2: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough] ./include/math-emu/double.h:120:34: note: expanded from macro 'FP_TO_INT_D' #define FP_TO_INT_D(r,X,rsz,rsg)_FP_TO_INT(D,2,r,X,rsz,rsg) ^ ./include/math-emu/op-common.h:671:4: note: expanded from macro '_FP_TO_INT' case FP_CLS_NAN: \ ^ 2 errors generated. arch/powerpc/math-emu/fctiwz.c:23:2: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough] FP_TO_INT_D(r, B, 32, 1); ^ ./include/math-emu/double.h:120:34: note: expanded from macro 'FP_TO_INT_D' #define FP_TO_INT_D(r,X,rsz,rsg)_FP_TO_INT(D,2,r,X,rsz,rsg) ^ ./include/math-emu/op-common.h:665:4: note: expanded from macro '_FP_TO_INT' case FP_CLS_ZERO: \ ^ arch/powerpc/math-emu/fctiwz.c:23:2: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough] ./include/math-emu/double.h:120:34: note: expanded from macro 'FP_TO_INT_D' #define FP_TO_INT_D(r,X,rsz,rsg)_FP_TO_INT(D,2,r,X,rsz,rsg) ^ ./include/math-emu/op-common.h:671:4: note: expanded from macro '_FP_TO_INT' case FP_CLS_NAN: \ ^ 2 errors generated. make[3]: *** [scripts/Makefile.build:249: arch/powerpc/math-emu/fctiw.o] Error 1 make[3]: *** [scripts/Makefile.build:249: arch/powerpc/math-emu/fctiwz.o] Error 1 arch/powerpc/math-emu/math_efp.c:282:5: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough] FP_TO_INT_ROUND_S(vc.wp[1], SB, 32, ^ ./include/math-emu/single.h:110:40: note: expanded from macro 'FP_TO_INT_ROUND_S' #define FP_TO_INT_ROUND_S(r,X,rsz,rsg) _FP_TO_INT_ROUND(S,1,r,X,rsz,rsg) ^ ./include/math-emu/op-common.h:770:4: note: expanded from macro '_FP_TO_INT_ROUND' case FP_CLS_NAN: \ ^ arch/powerpc/math-emu/math_efp.c:305:5: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough] FP_TO_INT_ROUND_S(vc.wp[1], SB, 32, ^ ./include/math-emu/single.h:110:40: note: expanded from macro 'FP_TO_INT_ROUND_S' #define FP_TO_INT_ROUND_S(r,X,rsz,rsg) _FP_TO_INT_ROUND(S,1,r,X,rsz,rsg) ^ ./include/math-emu/op-common.h:770:4: note: expanded from macro '_FP_TO_INT_ROUND' case FP_CLS_NAN: \ ^ arch/powerpc/math-emu/math_efp.c:316:5: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough] FP_TO_INT_S(vc.wp[1], SB, 32, ^ ./include/math-emu/single.h:109:34: note: expanded from macro 'FP_TO_INT_S' #define FP_TO_INT_S(r,X,rsz,rsg)_FP_TO_INT(S,1,r,X,rsz,rsg) ^ ./include/math-emu/op-common.h:665:4: note: expanded from macro '_FP_TO_INT' case FP_CLS_ZERO: \ ^ arch/powerpc/math-emu/math_efp.c:316:5: error:
[PATCH v2 2/2] powerpc/math-emu: Remove -w build flag and fix warnings
As reported by Nathan, the module_init() macro was not taken into
account because the header was missing. That means spe_mathemu_init()
was never called.
This should have been detected by gcc at build time, but due to
'-w' flag it went undetected.
Removing that flag leads to many warnings hence errors.
Fix those warnings then remove the -w flag.
Reported-by: Nathan Chancellor
Signed-off-by: Christophe Leroy
---
arch/powerpc/math-emu/Makefile | 2 --
arch/powerpc/math-emu/math.c | 18 +-
arch/powerpc/math-emu/math_efp.c | 57 +---
3 files changed, 39 insertions(+), 38 deletions(-)
diff --git a/arch/powerpc/math-emu/Makefile b/arch/powerpc/math-emu/Makefile
index a8794032f15f..26fef2e5672e 100644
--- a/arch/powerpc/math-emu/Makefile
+++ b/arch/powerpc/math-emu/Makefile
@@ -16,5 +16,3 @@ obj-$(CONFIG_SPE) += math_efp.o
CFLAGS_fabs.o = -fno-builtin-fabs
CFLAGS_math.o = -fno-builtin-fabs
-
-ccflags-y = -w
diff --git a/arch/powerpc/math-emu/math.c b/arch/powerpc/math-emu/math.c
index 36761bd00f38..936a9a149037 100644
--- a/arch/powerpc/math-emu/math.c
+++ b/arch/powerpc/math-emu/math.c
@@ -24,9 +24,9 @@ FLOATFUNC(mtfsf);
FLOATFUNC(mtfsfi);
#ifdef CONFIG_MATH_EMULATION_HW_UNIMPLEMENTED
-#undef FLOATFUNC(x)
+#undef FLOATFUNC
#define FLOATFUNC(x) static inline int x(void *op1, void *op2, void *op3, \
-void *op4) { }
+void *op4) { return 0; }
#endif
FLOATFUNC(fadd);
@@ -396,28 +396,28 @@ do_mathemu(struct pt_regs *regs)
case XCR:
op0 = (void *)>ccr;
- op1 = (void *)((insn >> 23) & 0x7);
+ op1 = (void *)(long)((insn >> 23) & 0x7);
op2 = (void *)>thread.TS_FPR((insn >> 16) & 0x1f);
op3 = (void *)>thread.TS_FPR((insn >> 11) & 0x1f);
break;
case XCRL:
op0 = (void *)>ccr;
- op1 = (void *)((insn >> 23) & 0x7);
- op2 = (void *)((insn >> 18) & 0x7);
+ op1 = (void *)(long)((insn >> 23) & 0x7);
+ op2 = (void *)(long)((insn >> 18) & 0x7);
break;
case XCRB:
- op0 = (void *)((insn >> 21) & 0x1f);
+ op0 = (void *)(long)((insn >> 21) & 0x1f);
break;
case XCRI:
- op0 = (void *)((insn >> 23) & 0x7);
- op1 = (void *)((insn >> 12) & 0xf);
+ op0 = (void *)(long)((insn >> 23) & 0x7);
+ op1 = (void *)(long)((insn >> 12) & 0xf);
break;
case XFLB:
- op0 = (void *)((insn >> 17) & 0xff);
+ op0 = (void *)(long)((insn >> 17) & 0xff);
op1 = (void *)>thread.TS_FPR((insn >> 11) & 0x1f);
break;
diff --git a/arch/powerpc/math-emu/math_efp.c b/arch/powerpc/math-emu/math_efp.c
index aa3bb8da1cb9..f01e3475f689 100644
--- a/arch/powerpc/math-emu/math_efp.c
+++ b/arch/powerpc/math-emu/math_efp.c
@@ -219,6 +219,7 @@ int do_spe_mathemu(struct pt_regs *regs)
case AB:
case XCR:
FP_UNPACK_SP(SA, va.wp + 1);
+ fallthrough;
case XB:
FP_UNPACK_SP(SB, vb.wp + 1);
break;
@@ -227,8 +228,8 @@ int do_spe_mathemu(struct pt_regs *regs)
break;
}
- pr_debug("SA: %ld %08lx %ld (%ld)\n", SA_s, SA_f, SA_e, SA_c);
- pr_debug("SB: %ld %08lx %ld (%ld)\n", SB_s, SB_f, SB_e, SB_c);
+ pr_debug("SA: %d %08x %d (%d)\n", SA_s, SA_f, SA_e, SA_c);
+ pr_debug("SB: %d %08x %d (%d)\n", SB_s, SB_f, SB_e, SB_c);
switch (func) {
case EFSABS:
@@ -279,7 +280,7 @@ int do_spe_mathemu(struct pt_regs *regs)
} else {
SB_e += (func == EFSCTSF ? 31 : 32);
FP_TO_INT_ROUND_S(vc.wp[1], SB, 32,
- (func == EFSCTSF));
+ (func == EFSCTSF) ? 1 : 0);
}
goto update_regs;
@@ -288,7 +289,7 @@ int do_spe_mathemu(struct pt_regs *regs)
FP_CLEAR_EXCEPTIONS;
FP_UNPACK_DP(DB, vb.dp);
- pr_debug("DB: %ld %08lx %08lx %ld (%ld)\n",
+ pr_debug("DB: %d %08x %08x %d (%d)\n",
DB_s, DB_f1, DB_f0, DB_e, DB_c);
FP_CONV(S, D, 1, 2, SR, DB);
@@ -302,7 +303,7 @@ int do_spe_mathemu(struct pt_regs *regs)
FP_SET_EXCEPTION(FP_EX_INVALID);
} else {
FP_TO_INT_ROUND_S(vc.wp[1], SB, 32,
-
