From 636f8aa4a742220819e5123647eff25172f36529 Mon Sep 17 00:00:00 2001
From: Ryan Houdek <Sonicadvance1@gmail.com>
Date: Thu, 7 Sep 2023 17:38:39 -0700
Subject: [PATCH] Arm64: Fix undefined behaviour in Push operation

Arm64 store with writeback when source register is the same register as
the address is undefined behaviour.
Depending on hardware details this can do a whole bunch of things.

This situation happens when the x86 code does `push rsp` which is quite
common for applications to do. We would then convert this to a `str x8, [x8, #-8]!`
Which results in undefined behaviour.

Now that redundant loads are optimized this showed up as an issue. Adds
a unit test to ensure we don't hit this again.
---
 .../Interface/Core/JIT/Arm64/MemoryOps.cpp    | 18 ++++++++++++++-
 unittests/ASM/FEX_bugs/Push.asm               | 22 +++++++++++++++++++
 2 files changed, 39 insertions(+), 1 deletion(-)
 create mode 100644 unittests/ASM/FEX_bugs/Push.asm

diff --git a/FEXCore/Source/Interface/Core/JIT/Arm64/MemoryOps.cpp b/FEXCore/Source/Interface/Core/JIT/Arm64/MemoryOps.cpp
index 199360d21..4a96fc502 100644
--- a/FEXCore/Source/Interface/Core/JIT/Arm64/MemoryOps.cpp
+++ b/FEXCore/Source/Interface/Core/JIT/Arm64/MemoryOps.cpp
@@ -1601,7 +1601,7 @@ DEF_OP(VBroadcastFromMem) {
 DEF_OP(Push) {
   const auto Op = IROp->C<IR::IROp_Push>();
   const auto ValueSize = Op->ValueSize;
-  const auto Src = GetReg(Op->Value.ID());
+  auto Src = GetReg(Op->Value.ID());
   const auto AddrSrc = GetReg(Op->Addr.ID());
   const auto Dst = GetReg(Node);
 
@@ -1617,6 +1617,22 @@ DEF_OP(Push) {
     }
   }
 
+  if (Src == AddrSrc) {
+    // If the data source is the address source then we need to do some additional work.
+    // This is because it is undefined behaviour to do a writeback on store operation where dest == src.
+    // In the case of writeback where the source is the address there are multiple behaviours.
+    // - SIGILL - Apple Silicon Behaviour
+    // - Stores original value - Cortex behaviour
+    // - Stores value after pre-index adjust adjust - Vixl simulator behaviour.
+    // - Undefined value stored
+    // - Undefined behaviour(!)
+
+    // In this path Src can end up overlapping both AddrSrc and Dst.
+    // Move the data to a temporary and store from there instead.
+    mov(TMP1, Src.X());
+    Src = TMP1;
+  }
+
   if (NeedsMoveAfterwards) {
     switch (ValueSize) {
       case 1: {
diff --git a/unittests/ASM/FEX_bugs/Push.asm b/unittests/ASM/FEX_bugs/Push.asm
new file mode 100644
index 000000000..5cd7e374c
--- /dev/null
+++ b/unittests/ASM/FEX_bugs/Push.asm
@@ -0,0 +1,22 @@
+%ifdef CONFIG
+{
+  "RegData": {
+    "RAX": "0xe0000010",
+    "RSP": "0xe0000008"
+  }
+}
+%endif
+
+; FEX had a bug where a `push rsp` would generate an Arm64 instruction with undefined behaviour.
+; `push rsp` -> `str x8, [x8, #-8]!`
+; This instruction has constrained undefined behaviour.
+; On Cortex it stores the original value.
+; On Apple Silicon it raises a SIGILL.
+; It can also store undefined data or have undefined behaviour.
+; Test to ensure we don't generate undefined behaviour.
+mov rsp, 0xe0000010
+push rsp
+
+mov rax, [rsp]
+
+hlt