[PATCH v1 1/2] LoongArch: Define barrier_before_unreachable() as empty

Tiezhu Yang posted 2 patches 1 year, 5 months ago
[PATCH v1 1/2] LoongArch: Define barrier_before_unreachable() as empty
Posted by Tiezhu Yang 1 year, 5 months ago
When building kernel with -Os (CONFIG_CC_OPTIMIZE_FOR_SIZE=y) rather
than the default -O2 (CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE is not set),
there exist many objtool warnings "unreachable instruction", this is
because objtool couldn't find a code path to reach the instruction,
objdump shows that the unreachable instruction is related with BUG()
or unreachable().

In include/linux/compiler-gcc.h, there is an empty inline assembly

  #define barrier_before_unreachable() asm volatile("")

in the definition of unreachable() to work around GCC 7.0 after the
commit 173a3efd3edb ("bug.h: work around GCC PR82365 in BUG()") in
the year of 2018, the unreachable instruction is generated by it.

As far as I can see, this workaround is to fix warnings of frame sizes,
the default CONFIG_FRAME_WARN is 2048 on 64 bit system now, it seems
that there are no -Wframe-larger-than warnings in the current kernel
without this workaround.

I am not sure whether the GCC bug has been fixed, I can not find the
fixup in the link https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
and in the GCC repo. So I am not sure whether it is time and proper
to remove this workaround in the common header totally, just remove
it in the arch specified header when compiling kernel with a newer
GCC version (for example GCC 12.1 or higher on LoongArch) at least.

For example:
arch/loongarch/kernel/cpu-probe.o: warning: objtool: cpu_probe+0x664: unreachable instruction

objdump -M no-aliases -D arch/loongarch/kernel/cpu-probe.o

Without this patch:

0000000000000000 <cpu_probe>:
 ...
 65c:   44000d80        bnez            $t0, 12 # 668 <.L345^B1+0x4>

0000000000000660 <.L10001^B2>:
 660:   002a0001        break           0x1

0000000000000664 <.L345^B1>:
 664:   53ff63ff        b               -160    # 5c4 <.L344^B1>
 668:   0280040c        addi.w          $t0, $zero, 1
 66c:   00006d8d        cpucfg          $t1, $t0

With this patch:

0000000000000000 <cpu_probe>:
 ...
 65c:   44000980        bnez            $t0, 8  # 664 <.L345^B1>

0000000000000660 <.L10001^B2>:
 660:   002a0001        break           0x1

0000000000000664 <.L345^B1>:
 664:   0280040c        addi.w          $t0, $zero, 1
 668:   00006d8d        cpucfg          $t1, $t0

Cc: stable@vger.kernel.org # 6.9+
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202407221208.6SSBeN9H-lkp@intel.com/
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/loongarch/Kconfig                |  1 +
 arch/loongarch/include/asm/compiler.h | 13 +++++++++++++
 2 files changed, 14 insertions(+)
 create mode 100644 arch/loongarch/include/asm/compiler.h

diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
index 70f169210b52..3af0da76d103 100644
--- a/arch/loongarch/Kconfig
+++ b/arch/loongarch/Kconfig
@@ -102,6 +102,7 @@ config LOONGARCH
 	select GPIOLIB
 	select HAS_IOPORT
 	select HAVE_ARCH_AUDITSYSCALL
+	select HAVE_ARCH_COMPILER_H
 	select HAVE_ARCH_JUMP_LABEL
 	select HAVE_ARCH_JUMP_LABEL_RELATIVE
 	select HAVE_ARCH_KASAN
diff --git a/arch/loongarch/include/asm/compiler.h b/arch/loongarch/include/asm/compiler.h
new file mode 100644
index 000000000000..4f6adb8d533e
--- /dev/null
+++ b/arch/loongarch/include/asm/compiler.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2024 Loongson Technology Corporation Limited
+ */
+#ifndef _ASM_COMPILER_H
+#define _ASM_COMPILER_H
+
+#ifdef barrier_before_unreachable
+#undef barrier_before_unreachable
+#define barrier_before_unreachable() do { } while (0)
+#endif
+
+#endif /* _ASM_COMPILER_H */
-- 
2.42.0
Re: [PATCH v1 1/2] LoongArch: Define barrier_before_unreachable() as empty
Posted by Huacai Chen 1 year, 5 months ago
Hi, Arnd and Jinyang,

On Tue, Aug 20, 2024 at 8:37 PM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>
> When building kernel with -Os (CONFIG_CC_OPTIMIZE_FOR_SIZE=y) rather
> than the default -O2 (CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE is not set),
> there exist many objtool warnings "unreachable instruction", this is
> because objtool couldn't find a code path to reach the instruction,
> objdump shows that the unreachable instruction is related with BUG()
> or unreachable().
>
> In include/linux/compiler-gcc.h, there is an empty inline assembly
>
>   #define barrier_before_unreachable() asm volatile("")
>
> in the definition of unreachable() to work around GCC 7.0 after the
> commit 173a3efd3edb ("bug.h: work around GCC PR82365 in BUG()") in
> the year of 2018, the unreachable instruction is generated by it.
>
> As far as I can see, this workaround is to fix warnings of frame sizes,
> the default CONFIG_FRAME_WARN is 2048 on 64 bit system now, it seems
> that there are no -Wframe-larger-than warnings in the current kernel
> without this workaround.
>
> I am not sure whether the GCC bug has been fixed, I can not find the
> fixup in the link https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
> and in the GCC repo. So I am not sure whether it is time and proper
> to remove this workaround in the common header totally, just remove
> it in the arch specified header when compiling kernel with a newer
> GCC version (for example GCC 12.1 or higher on LoongArch) at least.
What's your opinion? From my point of view, this GCC bug hasn't been
fixed. So there may still be potential problems.

Huacai

>
> For example:
> arch/loongarch/kernel/cpu-probe.o: warning: objtool: cpu_probe+0x664: unreachable instruction
>
> objdump -M no-aliases -D arch/loongarch/kernel/cpu-probe.o
>
> Without this patch:
>
> 0000000000000000 <cpu_probe>:
>  ...
>  65c:   44000d80        bnez            $t0, 12 # 668 <.L345^B1+0x4>
>
> 0000000000000660 <.L10001^B2>:
>  660:   002a0001        break           0x1
>
> 0000000000000664 <.L345^B1>:
>  664:   53ff63ff        b               -160    # 5c4 <.L344^B1>
>  668:   0280040c        addi.w          $t0, $zero, 1
>  66c:   00006d8d        cpucfg          $t1, $t0
>
> With this patch:
>
> 0000000000000000 <cpu_probe>:
>  ...
>  65c:   44000980        bnez            $t0, 8  # 664 <.L345^B1>
>
> 0000000000000660 <.L10001^B2>:
>  660:   002a0001        break           0x1
>
> 0000000000000664 <.L345^B1>:
>  664:   0280040c        addi.w          $t0, $zero, 1
>  668:   00006d8d        cpucfg          $t1, $t0
>
> Cc: stable@vger.kernel.org # 6.9+
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202407221208.6SSBeN9H-lkp@intel.com/
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>  arch/loongarch/Kconfig                |  1 +
>  arch/loongarch/include/asm/compiler.h | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
>  create mode 100644 arch/loongarch/include/asm/compiler.h
>
> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
> index 70f169210b52..3af0da76d103 100644
> --- a/arch/loongarch/Kconfig
> +++ b/arch/loongarch/Kconfig
> @@ -102,6 +102,7 @@ config LOONGARCH
>         select GPIOLIB
>         select HAS_IOPORT
>         select HAVE_ARCH_AUDITSYSCALL
> +       select HAVE_ARCH_COMPILER_H
>         select HAVE_ARCH_JUMP_LABEL
>         select HAVE_ARCH_JUMP_LABEL_RELATIVE
>         select HAVE_ARCH_KASAN
> diff --git a/arch/loongarch/include/asm/compiler.h b/arch/loongarch/include/asm/compiler.h
> new file mode 100644
> index 000000000000..4f6adb8d533e
> --- /dev/null
> +++ b/arch/loongarch/include/asm/compiler.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2024 Loongson Technology Corporation Limited
> + */
> +#ifndef _ASM_COMPILER_H
> +#define _ASM_COMPILER_H
> +
> +#ifdef barrier_before_unreachable
> +#undef barrier_before_unreachable
> +#define barrier_before_unreachable() do { } while (0)
> +#endif
> +
> +#endif /* _ASM_COMPILER_H */
> --
> 2.42.0
>
>
Re: [PATCH v1 1/2] LoongArch: Define barrier_before_unreachable() as empty
Posted by Xi Ruoyao 1 year, 5 months ago
On Wed, 2024-08-21 at 15:37 +0800, Huacai Chen wrote:
> > I am not sure whether the GCC bug has been fixed, I can not find the
> > fixup in the link https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
> > and in the GCC repo. So I am not sure whether it is time and proper
> > to remove this workaround in the common header totally, just remove
> > it in the arch specified header when compiling kernel with a newer
> > GCC version (for example GCC 12.1 or higher on LoongArch) at least.

> What's your opinion? From my point of view, this GCC bug hasn't been
> fixed. So there may still be potential problems.

I'm pretty sure it isn't fixed.  Using the test case from the bug
report:

struct i2c_board_info {
        char type[20];
        char pad[100];
};

#ifdef NONORETURN
void fortify_panic();
#else
void fortify_panic() __attribute__((noreturn));
#endif


int f(int a)
{
  if (a)
    fortify_panic();
}


void i2c_new_device(struct i2c_board_info *);
int em28xx_dvb_init(int model, int a, int b, int c, int d)
{
        switch (model) {
        case 1:{
                        struct i2c_board_info info = {};
                        f(a);
                        i2c_new_device(&info);
                        break;
                }
        case 2:{
                        struct i2c_board_info info = {};
                        f(b);
                        i2c_new_device(&info);
                        break;
                }
        case 3:{
                        struct i2c_board_info info = { };
                        f(c);
                        i2c_new_device(&info);
                        break;
                }
        case 4:{
                        struct i2c_board_info info = { };
                        f(d);
                        i2c_new_device(&info);
                        break;
                }
        }
        return 0;
}

$ cc -v
Using built-in specs.
COLLECT_GCC=cc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/loongarch64-unknown-linux-gnu/14.2.0/lto-wrapper
Target: loongarch64-unknown-linux-gnu
Configured with: ../configure --prefix=/usr LD=ld --enable-languages=c,c++ --enable-default-pie --enable-default-ssp --disable-multilib --with-build-config=bootstrap-lto --disable-fixincludes --with-system-zlib --enable-host-pie
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 14.2.0 (GCC) 
$ cc t.c -S -Wframe-larger-than=1 -DNONORETURN -O2
t.c: In function 'em28xx_dvb_init':
t.c:50:1: warning: the frame size of 144 bytes is larger than 1 bytes [-Wframe-larger-than=]
   50 | }
      | ^
$ cc t.c -S -Wframe-larger-than=1 -O2            
t.c: In function 'em28xx_dvb_init':
t.c:50:1: warning: the frame size of 512 bytes is larger than 1 bytes [-Wframe-larger-than=]
   50 | }
      | ^

And I'm puzzled why "unreachable instruction" is not a problem on x86?

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University
Re: [PATCH v1 1/2] LoongArch: Define barrier_before_unreachable() as empty
Posted by Jinyang He 1 year, 5 months ago
On 2024-08-21 15:52, Xi Ruoyao wrote:

> On Wed, 2024-08-21 at 15:37 +0800, Huacai Chen wrote:
>>> I am not sure whether the GCC bug has been fixed, I can not find the
>>> fixup in the link https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
>>> and in the GCC repo. So I am not sure whether it is time and proper
>>> to remove this workaround in the common header totally, just remove
>>> it in the arch specified header when compiling kernel with a newer
>>> GCC version (for example GCC 12.1 or higher on LoongArch) at least.
>> What's your opinion? From my point of view, this GCC bug hasn't been
>> fixed. So there may still be potential problems.
> I'm pretty sure it isn't fixed.  Using the test case from the bug
> report:
>
> struct i2c_board_info {
>          char type[20];
>          char pad[100];
> };
>
> #ifdef NONORETURN
> void fortify_panic();
> #else
> void fortify_panic() __attribute__((noreturn));
> #endif
>
>
> int f(int a)
> {
>    if (a)
>      fortify_panic();
> }
>
>
> void i2c_new_device(struct i2c_board_info *);
> int em28xx_dvb_init(int model, int a, int b, int c, int d)
> {
>          switch (model) {
>          case 1:{
>                          struct i2c_board_info info = {};
>                          f(a);
>                          i2c_new_device(&info);
>                          break;
>                  }
>          case 2:{
>                          struct i2c_board_info info = {};
>                          f(b);
>                          i2c_new_device(&info);
>                          break;
>                  }
>          case 3:{
>                          struct i2c_board_info info = { };
>                          f(c);
>                          i2c_new_device(&info);
>                          break;
>                  }
>          case 4:{
>                          struct i2c_board_info info = { };
>                          f(d);
>                          i2c_new_device(&info);
>                          break;
>                  }
>          }
>          return 0;
> }
>
> $ cc -v
> Using built-in specs.
> COLLECT_GCC=cc
> COLLECT_LTO_WRAPPER=/usr/libexec/gcc/loongarch64-unknown-linux-gnu/14.2.0/lto-wrapper
> Target: loongarch64-unknown-linux-gnu
> Configured with: ../configure --prefix=/usr LD=ld --enable-languages=c,c++ --enable-default-pie --enable-default-ssp --disable-multilib --with-build-config=bootstrap-lto --disable-fixincludes --with-system-zlib --enable-host-pie
> Thread model: posix
> Supported LTO compression algorithms: zlib zstd
> gcc version 14.2.0 (GCC)
> $ cc t.c -S -Wframe-larger-than=1 -DNONORETURN -O2
> t.c: In function 'em28xx_dvb_init':
> t.c:50:1: warning: the frame size of 144 bytes is larger than 1 bytes [-Wframe-larger-than=]
>     50 | }
>        | ^
> $ cc t.c -S -Wframe-larger-than=1 -O2
> t.c: In function 'em28xx_dvb_init':
> t.c:50:1: warning: the frame size of 512 bytes is larger than 1 bytes [-Wframe-larger-than=]
>     50 | }
>        | ^
>
> And I'm puzzled why "unreachable instruction" is not a problem on x86?
>
Hi, Ruoyao,

I looked the gcc:machine_kexec.c:rtl:jump2+:kexec_reboot, it seems
the gcc thought the block `asm volatile ("")` is same. So for -Os,
if 2+ these blocks appears, it create "b" to the same block.

I tried "-fno-crossjumping" to disable the jump2 pass, it works.
Could we change this block different to solve this? (e.g. by __COUNTER__).
But it seems break the original trick. :-(

Jinyang