[PATCH v5] Revert "powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto"
Christophe Leroy
christophe.leroy at csgroup.eu
Wed Jul 12 23:52:59 AEST 2023
Le 12/07/2023 à 15:45, Michael Ellerman a écrit :
> From: Christophe Leroy <christophe.leroy at 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 at csgroup.eu>
> Acked-by: Naveen N Rao <naveen at kernel.org>
> Signed-off-by: Michael Ellerman <mpe at 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)) {
More information about the Linuxppc-dev
mailing list