| Message ID | 20230712134552.534955-1-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b49e578b9314db051da0ad72bba24094193f9bd0 |
| Headers | show |
| Series | [v5] Revert "powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto" | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
Le 12/07/2023 à 15:45, Michael Ellerman a écrit : > From: Christophe Leroy <christophe.leroy@csgroup.eu> > > This partly reverts commit 1e688dd2a3d6759d416616ff07afc4bb836c4213. > > That commit aimed at optimising the code around generation of > WARN_ON/BUG_ON but this leads to a lot of dead code erroneously > generated by GCC. > > That dead code becomes a problem when we start using objtool validation > because objtool will abort validation with a warning as soon as it > detects unreachable code. This is because unreachable code might > be the indication that objtool doesn't properly decode object text. > > text data bss dec hex filename > 9551585 3627834 224376 13403795 cc8693 vmlinux.before > 9535281 3628358 224376 13388015 cc48ef vmlinux.after > > Once this change is reverted, in a standard configuration (pmac32 + > function tracer) the text is reduced by 16k which is around 1.7% > > We already had problem with it when starting to use objtool on powerpc > as a replacement for recordmcount, see commit 93e3f45a2631 ("powerpc: > Fix __WARN_FLAGS() for use with Objtool") > > There is also a problem with at least GCC 12, on ppc64_defconfig + > CONFIG_CC_OPTIMIZE_FOR_SIZE=y + CONFIG_DEBUG_SECTION_MISMATCH=y : > > LD .tmp_vmlinux.kallsyms1 > powerpc64-linux-ld: net/ipv4/tcp_input.o:(__ex_table+0xc4): undefined reference to `.L2136' > make[2]: *** [scripts/Makefile.vmlinux:36: vmlinux] Error 1 > make[1]: *** [/home/chleroy/linux-powerpc/Makefile:1238: vmlinux] Error 2 > > Taking into account that other problems are encountered with that > 'asm goto' in WARN_ON(), including build failures, keeping that > change is not worth it allthough it is primarily a compiler bug. > > Revert it for now. > > mpe: Retain EMIT_WARN_ENTRY as a synonym for EMIT_BUG_ENTRY to reduce > churn, as there are now nearly as many uses of EMIT_WARN_ENTRY as > EMIT_BUG_ENTRY. In that case, should we keep __EMIT_BUG_ENTRY and also keep the check that makes sure nobody uses EMIT_BUG_ENTRY with BUGFLAG_WARNING ? > -.macro EMIT_BUG_ENTRY addr,file,line,flags > - .if \flags & 1 /* BUGFLAG_WARNING */ > - .err /* Use EMIT_WARN_ENTRY for warnings */ > - .endif > - __EMIT_BUG_ENTRY \addr,\file,\line,\flags > -.endm > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > Acked-by: Naveen N Rao <naveen@kernel.org> > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > Link: https://msgid.link/a7d6d0c20deaccfcbc74c3149e782538461fd6fe.1689091394.git.christophe.leroy@csgroup.eu > --- > arch/powerpc/include/asm/bug.h | 69 +++++++--------------------------- > arch/powerpc/kernel/traps.c | 9 +---- > 2 files changed, 15 insertions(+), 63 deletions(-) > > v5: Keep EMIT_WARN_ENTRY. > I'll plan to take this as a fix. > > diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h > index ef42adb44aa3..00c6b0b4ede4 100644 > --- a/arch/powerpc/include/asm/bug.h > +++ b/arch/powerpc/include/asm/bug.h > @@ -4,14 +4,13 @@ > #ifdef __KERNEL__ > > #include <asm/asm-compat.h> > -#include <asm/extable.h> > > #ifdef CONFIG_BUG > > #ifdef __ASSEMBLY__ > #include <asm/asm-offsets.h> > #ifdef CONFIG_DEBUG_BUGVERBOSE > -.macro __EMIT_BUG_ENTRY addr,file,line,flags > +.macro EMIT_BUG_ENTRY addr,file,line,flags > .section __bug_table,"aw" > 5001: .4byte \addr - . > .4byte 5002f - . > @@ -23,7 +22,7 @@ > .previous > .endm > #else > -.macro __EMIT_BUG_ENTRY addr,file,line,flags > +.macro EMIT_BUG_ENTRY addr,file,line,flags > .section __bug_table,"aw" > 5001: .4byte \addr - . > .short \flags > @@ -32,18 +31,6 @@ > .endm > #endif /* verbose */ > > -.macro EMIT_WARN_ENTRY addr,file,line,flags > - EX_TABLE(\addr,\addr+4) > - __EMIT_BUG_ENTRY \addr,\file,\line,\flags > -.endm > - > -.macro EMIT_BUG_ENTRY addr,file,line,flags > - .if \flags & 1 /* BUGFLAG_WARNING */ > - .err /* Use EMIT_WARN_ENTRY for warnings */ > - .endif > - __EMIT_BUG_ENTRY \addr,\file,\line,\flags > -.endm > - > #else /* !__ASSEMBLY__ */ > /* _EMIT_BUG_ENTRY expects args %0,%1,%2,%3 to be FILE, LINE, flags and > sizeof(struct bug_entry), respectively */ > @@ -73,16 +60,6 @@ > "i" (sizeof(struct bug_entry)), \ > ##__VA_ARGS__) > > -#define WARN_ENTRY(insn, flags, label, ...) \ > - asm_volatile_goto( \ > - "1: " insn "\n" \ > - EX_TABLE(1b, %l[label]) \ > - _EMIT_BUG_ENTRY \ > - : : "i" (__FILE__), "i" (__LINE__), \ > - "i" (flags), \ > - "i" (sizeof(struct bug_entry)), \ > - ##__VA_ARGS__ : : label) > - > /* > * BUG_ON() and WARN_ON() do their best to cooperate with compile-time > * optimisations. However depending on the complexity of the condition > @@ -95,16 +72,7 @@ > } while (0) > #define HAVE_ARCH_BUG > > -#define __WARN_FLAGS(flags) do { \ > - __label__ __label_warn_on; \ > - \ > - WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), __label_warn_on); \ > - barrier_before_unreachable(); \ > - __builtin_unreachable(); \ > - \ > -__label_warn_on: \ > - break; \ > -} while (0) > +#define __WARN_FLAGS(flags) BUG_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags)) > > #ifdef CONFIG_PPC64 > #define BUG_ON(x) do { \ > @@ -117,25 +85,15 @@ __label_warn_on: \ > } while (0) > > #define WARN_ON(x) ({ \ > - bool __ret_warn_on = false; \ > - do { \ > - if (__builtin_constant_p((x))) { \ > - if (!(x)) \ > - break; \ > + int __ret_warn_on = !!(x); \ > + if (__builtin_constant_p(__ret_warn_on)) { \ > + if (__ret_warn_on) \ > __WARN(); \ > - __ret_warn_on = true; \ > - } else { \ > - __label__ __label_warn_on; \ > - \ > - WARN_ENTRY(PPC_TLNEI " %4, 0", \ > - BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN), \ > - __label_warn_on, \ > - "r" ((__force long)(x))); \ > - break; \ > -__label_warn_on: \ > - __ret_warn_on = true; \ > - } \ > - } while (0); \ > + } else { \ > + BUG_ENTRY(PPC_TLNEI " %4, 0", \ > + BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN), \ > + "r" (__ret_warn_on)); \ > + } \ > unlikely(__ret_warn_on); \ > }) > > @@ -148,14 +106,13 @@ __label_warn_on: \ > #ifdef __ASSEMBLY__ > .macro EMIT_BUG_ENTRY addr,file,line,flags > .endm > -.macro EMIT_WARN_ENTRY addr,file,line,flags > -.endm > #else /* !__ASSEMBLY__ */ > #define _EMIT_BUG_ENTRY > -#define _EMIT_WARN_ENTRY > #endif > #endif /* CONFIG_BUG */ > > +#define EMIT_WARN_ENTRY EMIT_BUG_ENTRY > + > #include <asm-generic/bug.h> > > #ifndef __ASSEMBLY__ > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c > index e59ec6d32d37..7ef147e2a20d 100644 > --- a/arch/powerpc/kernel/traps.c > +++ b/arch/powerpc/kernel/traps.c > @@ -1508,13 +1508,8 @@ static void do_program_check(struct pt_regs *regs) > > if (!(regs->msr & MSR_PR) && /* not user-mode */ > report_bug(bugaddr, regs) == BUG_TRAP_TYPE_WARN) { > - const struct exception_table_entry *entry; > - > - entry = search_exception_tables(bugaddr); > - if (entry) { > - regs_set_return_ip(regs, extable_fixup(entry) + regs->nip - bugaddr); > - return; > - } > + regs_add_return_ip(regs, 4); > + return; > } > > if (cpu_has_feature(CPU_FTR_DEXCR_NPHIE) && user_mode(regs)) {
Christophe Leroy <christophe.leroy@csgroup.eu> writes: > Le 12/07/2023 à 15:45, Michael Ellerman a écrit : >> From: Christophe Leroy <christophe.leroy@csgroup.eu> >> >> This partly reverts commit 1e688dd2a3d6759d416616ff07afc4bb836c4213. >> >> That commit aimed at optimising the code around generation of >> WARN_ON/BUG_ON but this leads to a lot of dead code erroneously >> generated by GCC. >> >> That dead code becomes a problem when we start using objtool validation >> because objtool will abort validation with a warning as soon as it >> detects unreachable code. This is because unreachable code might >> be the indication that objtool doesn't properly decode object text. >> >> text data bss dec hex filename >> 9551585 3627834 224376 13403795 cc8693 vmlinux.before >> 9535281 3628358 224376 13388015 cc48ef vmlinux.after >> >> Once this change is reverted, in a standard configuration (pmac32 + >> function tracer) the text is reduced by 16k which is around 1.7% >> >> We already had problem with it when starting to use objtool on powerpc >> as a replacement for recordmcount, see commit 93e3f45a2631 ("powerpc: >> Fix __WARN_FLAGS() for use with Objtool") >> >> There is also a problem with at least GCC 12, on ppc64_defconfig + >> CONFIG_CC_OPTIMIZE_FOR_SIZE=y + CONFIG_DEBUG_SECTION_MISMATCH=y : >> >> LD .tmp_vmlinux.kallsyms1 >> powerpc64-linux-ld: net/ipv4/tcp_input.o:(__ex_table+0xc4): undefined reference to `.L2136' >> make[2]: *** [scripts/Makefile.vmlinux:36: vmlinux] Error 1 >> make[1]: *** [/home/chleroy/linux-powerpc/Makefile:1238: vmlinux] Error 2 >> >> Taking into account that other problems are encountered with that >> 'asm goto' in WARN_ON(), including build failures, keeping that >> change is not worth it allthough it is primarily a compiler bug. >> >> Revert it for now. >> >> mpe: Retain EMIT_WARN_ENTRY as a synonym for EMIT_BUG_ENTRY to reduce >> churn, as there are now nearly as many uses of EMIT_WARN_ENTRY as >> EMIT_BUG_ENTRY. > > In that case, should we keep __EMIT_BUG_ENTRY and also keep the check > that makes sure nobody uses EMIT_BUG_ENTRY with BUGFLAG_WARNING ? I didn't think it was worth it, now that it's not a correctness issue. I think the better option would be to have EMIT_WARN_ENTRY add BUGFLAG_WARNING itself, rather than the caller having to pass it. cheers
Le 17/07/2023 à 07:01, Michael Ellerman a écrit : > Christophe Leroy <christophe.leroy@csgroup.eu> writes: >> Le 12/07/2023 à 15:45, Michael Ellerman a écrit : >>> From: Christophe Leroy <christophe.leroy@csgroup.eu> >>> >>> This partly reverts commit 1e688dd2a3d6759d416616ff07afc4bb836c4213. >>> >>> That commit aimed at optimising the code around generation of >>> WARN_ON/BUG_ON but this leads to a lot of dead code erroneously >>> generated by GCC. >>> >>> That dead code becomes a problem when we start using objtool validation >>> because objtool will abort validation with a warning as soon as it >>> detects unreachable code. This is because unreachable code might >>> be the indication that objtool doesn't properly decode object text. >>> >>> text data bss dec hex filename >>> 9551585 3627834 224376 13403795 cc8693 vmlinux.before >>> 9535281 3628358 224376 13388015 cc48ef vmlinux.after >>> >>> Once this change is reverted, in a standard configuration (pmac32 + >>> function tracer) the text is reduced by 16k which is around 1.7% >>> >>> We already had problem with it when starting to use objtool on powerpc >>> as a replacement for recordmcount, see commit 93e3f45a2631 ("powerpc: >>> Fix __WARN_FLAGS() for use with Objtool") >>> >>> There is also a problem with at least GCC 12, on ppc64_defconfig + >>> CONFIG_CC_OPTIMIZE_FOR_SIZE=y + CONFIG_DEBUG_SECTION_MISMATCH=y : >>> >>> LD .tmp_vmlinux.kallsyms1 >>> powerpc64-linux-ld: net/ipv4/tcp_input.o:(__ex_table+0xc4): undefined reference to `.L2136' >>> make[2]: *** [scripts/Makefile.vmlinux:36: vmlinux] Error 1 >>> make[1]: *** [/home/chleroy/linux-powerpc/Makefile:1238: vmlinux] Error 2 >>> >>> Taking into account that other problems are encountered with that >>> 'asm goto' in WARN_ON(), including build failures, keeping that >>> change is not worth it allthough it is primarily a compiler bug. >>> >>> Revert it for now. >>> >>> mpe: Retain EMIT_WARN_ENTRY as a synonym for EMIT_BUG_ENTRY to reduce >>> churn, as there are now nearly as many uses of EMIT_WARN_ENTRY as >>> EMIT_BUG_ENTRY. >> >> In that case, should we keep __EMIT_BUG_ENTRY and also keep the check >> that makes sure nobody uses EMIT_BUG_ENTRY with BUGFLAG_WARNING ? > > I didn't think it was worth it, now that it's not a correctness issue. > > I think the better option would be to have EMIT_WARN_ENTRY add > BUGFLAG_WARNING itself, rather than the caller having to pass it. > Ok that's fine for me. I'll do that in a follow-up patch one day. Christophe
On Wed, 12 Jul 2023 23:45:02 +1000, Michael Ellerman wrote: > This partly reverts commit 1e688dd2a3d6759d416616ff07afc4bb836c4213. > > That commit aimed at optimising the code around generation of > WARN_ON/BUG_ON but this leads to a lot of dead code erroneously > generated by GCC. > > That dead code becomes a problem when we start using objtool validation > because objtool will abort validation with a warning as soon as it > detects unreachable code. This is because unreachable code might > be the indication that objtool doesn't properly decode object text. > > [...] Applied to powerpc/fixes. [1/1] Revert "powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto" https://git.kernel.org/powerpc/c/b49e578b9314db051da0ad72bba24094193f9bd0 cheers
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h index ef42adb44aa3..00c6b0b4ede4 100644 --- a/arch/powerpc/include/asm/bug.h +++ b/arch/powerpc/include/asm/bug.h @@ -4,14 +4,13 @@ #ifdef __KERNEL__ #include <asm/asm-compat.h> -#include <asm/extable.h> #ifdef CONFIG_BUG #ifdef __ASSEMBLY__ #include <asm/asm-offsets.h> #ifdef CONFIG_DEBUG_BUGVERBOSE -.macro __EMIT_BUG_ENTRY addr,file,line,flags +.macro EMIT_BUG_ENTRY addr,file,line,flags .section __bug_table,"aw" 5001: .4byte \addr - . .4byte 5002f - . @@ -23,7 +22,7 @@ .previous .endm #else -.macro __EMIT_BUG_ENTRY addr,file,line,flags +.macro EMIT_BUG_ENTRY addr,file,line,flags .section __bug_table,"aw" 5001: .4byte \addr - . .short \flags @@ -32,18 +31,6 @@ .endm #endif /* verbose */ -.macro EMIT_WARN_ENTRY addr,file,line,flags - EX_TABLE(\addr,\addr+4) - __EMIT_BUG_ENTRY \addr,\file,\line,\flags -.endm - -.macro EMIT_BUG_ENTRY addr,file,line,flags - .if \flags & 1 /* BUGFLAG_WARNING */ - .err /* Use EMIT_WARN_ENTRY for warnings */ - .endif - __EMIT_BUG_ENTRY \addr,\file,\line,\flags -.endm - #else /* !__ASSEMBLY__ */ /* _EMIT_BUG_ENTRY expects args %0,%1,%2,%3 to be FILE, LINE, flags and sizeof(struct bug_entry), respectively */ @@ -73,16 +60,6 @@ "i" (sizeof(struct bug_entry)), \ ##__VA_ARGS__) -#define WARN_ENTRY(insn, flags, label, ...) \ - asm_volatile_goto( \ - "1: " insn "\n" \ - EX_TABLE(1b, %l[label]) \ - _EMIT_BUG_ENTRY \ - : : "i" (__FILE__), "i" (__LINE__), \ - "i" (flags), \ - "i" (sizeof(struct bug_entry)), \ - ##__VA_ARGS__ : : label) - /* * BUG_ON() and WARN_ON() do their best to cooperate with compile-time * optimisations. However depending on the complexity of the condition @@ -95,16 +72,7 @@ } while (0) #define HAVE_ARCH_BUG -#define __WARN_FLAGS(flags) do { \ - __label__ __label_warn_on; \ - \ - WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), __label_warn_on); \ - barrier_before_unreachable(); \ - __builtin_unreachable(); \ - \ -__label_warn_on: \ - break; \ -} while (0) +#define __WARN_FLAGS(flags) BUG_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags)) #ifdef CONFIG_PPC64 #define BUG_ON(x) do { \ @@ -117,25 +85,15 @@ __label_warn_on: \ } while (0) #define WARN_ON(x) ({ \ - bool __ret_warn_on = false; \ - do { \ - if (__builtin_constant_p((x))) { \ - if (!(x)) \ - break; \ + int __ret_warn_on = !!(x); \ + if (__builtin_constant_p(__ret_warn_on)) { \ + if (__ret_warn_on) \ __WARN(); \ - __ret_warn_on = true; \ - } else { \ - __label__ __label_warn_on; \ - \ - WARN_ENTRY(PPC_TLNEI " %4, 0", \ - BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN), \ - __label_warn_on, \ - "r" ((__force long)(x))); \ - break; \ -__label_warn_on: \ - __ret_warn_on = true; \ - } \ - } while (0); \ + } else { \ + BUG_ENTRY(PPC_TLNEI " %4, 0", \ + BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN), \ + "r" (__ret_warn_on)); \ + } \ unlikely(__ret_warn_on); \ }) @@ -148,14 +106,13 @@ __label_warn_on: \ #ifdef __ASSEMBLY__ .macro EMIT_BUG_ENTRY addr,file,line,flags .endm -.macro EMIT_WARN_ENTRY addr,file,line,flags -.endm #else /* !__ASSEMBLY__ */ #define _EMIT_BUG_ENTRY -#define _EMIT_WARN_ENTRY #endif #endif /* CONFIG_BUG */ +#define EMIT_WARN_ENTRY EMIT_BUG_ENTRY + #include <asm-generic/bug.h> #ifndef __ASSEMBLY__ diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c index e59ec6d32d37..7ef147e2a20d 100644 --- a/arch/powerpc/kernel/traps.c +++ b/arch/powerpc/kernel/traps.c @@ -1508,13 +1508,8 @@ static void do_program_check(struct pt_regs *regs) if (!(regs->msr & MSR_PR) && /* not user-mode */ report_bug(bugaddr, regs) == BUG_TRAP_TYPE_WARN) { - const struct exception_table_entry *entry; - - entry = search_exception_tables(bugaddr); - if (entry) { - regs_set_return_ip(regs, extable_fixup(entry) + regs->nip - bugaddr); - return; - } + regs_add_return_ip(regs, 4); + return; } if (cpu_has_feature(CPU_FTR_DEXCR_NPHIE) && user_mode(regs)) {