[v6,4/4] MIPS: Workaround GCC __builtin_unreachable reordering bug
diff mbox series

Message ID 20180809174444.31705-5-paul.burton@mips.com
State Not Applicable
Headers show
Series
  • Untitled series #60129
Related show

Commit Message

Paul Burton Aug. 9, 2018, 5:44 p.m. UTC
Older versions of GCC for the MIPS architecture suffer from a bug which
can lead to instructions from beyond an unreachable statement being
incorrectly reordered into earlier branch delay slots if the unreachable
statement is the only content of a case in a switch statement. This can
lead to seemingly random behaviour, such as invalid memory accesses from
incorrectly reordered loads or stores, and link failures on microMIPS
builds.

See this potential GCC fix for details:

    https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00360.html

This bug can be worked around by placing a volatile asm statement, which
GCC is prevented from reordering past, prior to the
__builtin_unreachable call.

This was actually done already for other reasons by commit 173a3efd3edb
("bug.h: work around GCC PR82365 in BUG()"), but without the MIPS
specific .insn, which broke microMIPS builds on newer GCC 7.2 toolchains
with errors like the following:

    arch/mips/mm/dma-default.s:3265: Error: branch to a symbol in another ISA mode
    arch/mips/mm/dma-default.s:5027: Error: branch to a symbol in another ISA mode

Add a MIPS-specific definition of barrier_before_unreachable() which
includes the .insn directive in order to satisfy the assembler that
branch targets are in fact code.

The original bug affects at least a maltasmvp_defconfig kernel built
from the v4.4 tag using GCC 4.9.2 (from a Codescape SDK 2015.06-05
toolchain), with the result being an address exception taken after log
messages about the L1 caches (during probe of the L2 cache):

    Initmem setup node 0 [mem 0x0000000080000000-0x000000009fffffff]
    VPE topology {2,2} total 4
    Primary instruction cache 64kB, VIPT, 4-way, linesize 32 bytes.
    Primary data cache 64kB, 4-way, PIPT, no aliases, linesize 32 bytes
    <AdEL exception here>

This is early enough that the kernel exception vectors are not in use,
so any further output depends upon the bootloader. This is reproducible
in QEMU where no further output occurs - ie. the system hangs here.
Given the nature of the bug it may potentially be hit with differing
symptoms.

Fixes: 173a3efd3edb ("bug.h: work around GCC PR82365 in BUG()")
Signed-off-by: Paul Burton <paul.burton@mips.com>
[jhogan@kernel.org: Forward port and use asm/compiler.h instead of
 asm/compiler-gcc.h]
Signed-off-by: James Hogan <jhogan@kernel.org>
Reviewed-by: Paul Burton <paul.burton@mips.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-mips@linux-mips.org

---

Changes in v6: None
Changes in v5:
- Comment & commit message tweaks.

Changes in v4: None
Changes in v3:
- Forward port to v4.17-rc and update commit message.
- Drop stable tag for now.

Changes in v2:
- Remove generic-y entry.

 arch/mips/include/asm/compiler.h | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

Comments

Arnd Bergmann Aug. 9, 2018, 6:12 p.m. UTC | #1
On Thu, Aug 9, 2018 at 7:45 PM Paul Burton <paul.burton@mips.com> wrote:

> +/*
> + * With GCC v4.5 onwards can use __builtin_unreachable to indicate to the
> + * compiler that a particular code path will never be hit. This allows it to be
> + * optimised out of the generated binary.
> + *
> + * Unfortunately GCC from at least v4.9.2 to current head of tree as of May
> + * 2016 suffer from a bug that can lead to instructions from beyond an

Has anything happened to address this in gcc in the meantime?
Could you update this text to reflect whatever is in current gcc-9?

      Arnd
Paul Burton Aug. 9, 2018, 6:56 p.m. UTC | #2
Hi Arnd,

On Thu, Aug 09, 2018 at 08:12:27PM +0200, Arnd Bergmann wrote:
> On Thu, Aug 9, 2018 at 7:45 PM Paul Burton <paul.burton@mips.com> wrote:
> 
> > +/*
> > + * With GCC v4.5 onwards can use __builtin_unreachable to indicate to the
> > + * compiler that a particular code path will never be hit. This allows it to be
> > + * optimised out of the generated binary.
> > + *
> > + * Unfortunately GCC from at least v4.9.2 to current head of tree as of May
> > + * 2016 suffer from a bug that can lead to instructions from beyond an
> 
> Has anything happened to address this in gcc in the meantime?
> Could you update this text to reflect whatever is in current gcc-9?

Good question. I can reproduce the problem using the test case from [1]
using both GCC 6.4.0 & 7.3.0, but 8.1.0 generates wildly different code
which looks good.

Nothing relevant is listed in the release notes for GCC 8.x though, and
I can't see anything obvious in gcc's commit logs. It doesn't looks like
the fix Robert suggested went in. So I don't know whether current GCC's
have resolved the problem or just get lucky enough not to hit it with
the existing testcase.

I've copied Matthew (GCC MIPS maintainer) in case he has any relevant
information.

Thanks,
    Paul

[1] https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00360.html

Patch
diff mbox series

diff --git a/arch/mips/include/asm/compiler.h b/arch/mips/include/asm/compiler.h
index e081a265f422..1e9548faf9c7 100644
--- a/arch/mips/include/asm/compiler.h
+++ b/arch/mips/include/asm/compiler.h
@@ -8,6 +8,36 @@ 
 #ifndef _ASM_COMPILER_H
 #define _ASM_COMPILER_H
 
+/*
+ * With GCC v4.5 onwards can use __builtin_unreachable to indicate to the
+ * compiler that a particular code path will never be hit. This allows it to be
+ * optimised out of the generated binary.
+ *
+ * Unfortunately GCC from at least v4.9.2 to current head of tree as of May
+ * 2016 suffer from a bug that can lead to instructions from beyond an
+ * unreachable statement being incorrectly reordered into earlier delay slots
+ * if the unreachable statement is the only content of a case in a switch
+ * statement. This can lead to seemingly random behaviour, such as invalid
+ * memory accesses from incorrectly reordered loads or stores. See this
+ * potential GCC fix for details:
+ *
+ *   https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00360.html
+ *
+ * GCC also handles stack allocation suboptimally when calling noreturn
+ * functions or calling __builtin_unreachable():
+ *
+ *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
+ *
+ * We work around both of these issues by placing a volatile asm statement,
+ * which GCC is prevented from reordering past, prior to __builtin_unreachable
+ * calls.
+ *
+ * The .insn statement is required to ensure that any branches to the
+ * statement, which sadly must be kept due to the asm statement, are known to
+ * be branches to code and satisfy linker requirements for microMIPS kernels.
+ */
+#define barrier_before_unreachable() asm volatile(".insn")
+
 #if __GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 4)
 #define GCC_IMM_ASM() "n"
 #define GCC_REG_ACCUM "$0"