Recompiler correctness: fail loud, fix delay slots, $ra, jump tables and overlaps

ps1Analyzer/include/ps1recomp/function_finder.h

@@ -76,8 +76,66 @@ inline bool isStackEpilogue(uint32_t instr) {
 /// Check if instruction is a NOP (0x00000000)
 inline bool isNOP(uint32_t instr) { return instr == 0; }
 
+/// Check if instruction is JR (any register)
+inline bool isJR(uint32_t instr) {
+  return getOpcode(instr) == OP_SPECIAL && getFunction(instr) == FUNC_JR;
+}
+
+/// Check if instruction is an unconditional J (not JAL)
+inline bool isJ(uint32_t instr) { return getOpcode(instr) == OP_J; }
+
+/// Check if instruction is a PC-relative conditional branch (BEQ/BNE/BLEZ/
+/// BGTZ or a REGIMM BLTZ/BGEZ family member).
+inline bool isBranch(uint32_t instr) {
+  const uint32_t op = getOpcode(instr);
+  return op == OP_BEQ || op == OP_BNE || op == OP_BLEZ || op == OP_BGTZ ||
+         op == OP_REGIMM;
+}
+
+/// Compute a PC-relative branch target: PC + 4 + (signed imm16 << 2)
+inline uint32_t branchTarget(uint32_t pc, uint32_t instr) {
+  return pc + 4 + (static_cast<uint32_t>(static_cast<int32_t>(getImm16(instr)))
+                   << 2);
+}
+
+/// Check if instruction loads from memory into a register (LB..LWR).
+inline bool isLoad(uint32_t instr) {
+  const uint32_t op = getOpcode(instr);
+  return op >= 0x20 && op <= 0x26; // LB, LH, LWL, LW, LBU, LHU, LWR
+}
+
+/// Does `instr` write general-purpose register `reg`?
+/// Conservative: covers R-type rd writes, loads, and the immediate ALU forms.
+bool writesRegister(uint32_t instr, uint32_t reg);
+
 } // namespace mips
 
+/// Find where a function actually ends.
+///
+/// A function does **not** end at the first `jr` it contains: hand-written MIPS
+/// routinely branches forward over its own epilogue, and jump tables reach
+/// labels far past it. Ending there splits one function in two, and the branch
+/// that crossed the cut then has to be emitted as a call into the middle of
+/// another function -- an address the recompiler never emits, which the
+/// dispatcher can only fail on.
+///
+/// So the scan tracks `reach`, the furthest forward target seen so far, and
+/// accepts a terminator only once it lies at or past `reach`. `jr $ra` always
+/// terminates; `jr $reg` terminates only when `$reg` was not loaded from memory
+/// (a loaded register means a jump table or computed jump, which stays inside
+/// the function).
+///
+/// @param words       Instruction words, starting at `startAddr`.
+/// @param startAddr   Virtual address of `words[0]`.
+/// @param maxEndAddr  Hard upper bound (the next known entry point, or the end
+///                    of the section). Never returns past this.
+/// @return Address one past the function's last instruction, delay slot
+///         included. Falls back to `maxEndAddr` when no terminator qualifies.
+uint32_t refineFunctionEnd(const std::vector<uint32_t> &words,
+                           uint32_t startAddr, uint32_t maxEndAddr);
+
+
+
 // Function Detection Source
 
 enum class FunctionSource {
@@ -101,6 +159,20 @@ struct FunctionInfo {
   }
 };
 
+/// Shrink any function that extends into the next one.
+///
+/// Sizes arrive from several places -- the ELF symbol table, the gap to the
+/// next detected entry, `--add-func` on the command line -- and the later
+/// sources do not revisit the earlier ones. So adding an entry point inside an
+/// already-sized function leaves that function's extent untouched, and the
+/// overlapping bytes get emitted twice, under two different function names and
+/// two sets of labels. Every `--add-func` used to work around weak detection
+/// created one of these.
+///
+/// Expects `funcs` sorted by address; sorts it if not. Leaves the last
+/// function alone (nothing follows it to overlap).
+void clampOverlappingSizes(std::vector<FunctionInfo> &funcs);
+
 // FunctionFinder
 
 class FunctionFinder {

ps1Analyzer/src/function_finder.cpp

@@ -8,6 +8,106 @@
 
 namespace ps1recomp {
 
+namespace mips {
+
+bool writesRegister(uint32_t instr, uint32_t reg) {
+  if (reg == 0)
+    return false; // $zero is never really written
+
+  const uint32_t op = getOpcode(instr);
+
+  if (op == OP_SPECIAL) {
+    const uint32_t fn = getFunction(instr);
+    // jr, jalr, mthi, mtlo, mult, multu, div, divu write no GPR rd we track
+    const bool noRdWrite = (fn == 0x08 || fn == 0x09 || fn == 0x11 ||
+                            fn == 0x13 || fn == 0x18 || fn == 0x19 ||
+                            fn == 0x1A || fn == 0x1B);
+    return !noRdWrite && getRd(instr) == reg;
+  }
+  if (isLoad(instr))
+    return getRt(instr) == reg;
+  // ADDI, ADDIU, SLTI, SLTIU, ANDI, ORI, XORI, LUI -- all write rt
+  if (op >= 0x08 && op <= 0x0F)
+    return getRt(instr) == reg;
+  return false;
+}
+
+} // namespace mips
+
+namespace {
+
+/// Does the instruction at `idx` terminate the function?
+///
+/// `jr $ra` always does. For `jr $reg` we walk back to whoever wrote `$reg`:
+/// a load means the target came out of memory -- a jump table or dispatch
+/// through a pointer, both of which stay inside the function -- while anything
+/// else (notably `move $reg, $ra`) means the register carries a return address.
+bool isFunctionEnd(const std::vector<uint32_t> &words, size_t idx) {
+  const uint32_t instr = words[idx];
+  if (!mips::isJR(instr))
+    return false;
+  const uint32_t reg = mips::getRs(instr);
+  if (reg == mips::REG_RA)
+    return true;
+
+  for (size_t k = idx; k-- > 0;) {
+    if (!mips::writesRegister(words[k], reg))
+      continue;
+    return !mips::isLoad(words[k]);
+  }
+  // Nothing in this range wrote it, so it came in from the caller.
+  return true;
+}
+
+} // namespace
+
+uint32_t refineFunctionEnd(const std::vector<uint32_t> &words,
+                           uint32_t startAddr, uint32_t maxEndAddr) {
+  if (words.empty() || maxEndAddr <= startAddr)
+    return maxEndAddr;
+
+  uint32_t reach = startAddr;
+
+  for (size_t i = 0; i < words.size(); ++i) {
+    const uint32_t addr = startAddr + static_cast<uint32_t>(i * 4);
+    if (addr >= maxEndAddr)
+      break;
+    const uint32_t instr = words[i];
+
+    // Track the furthest forward target: while any branch still jumps past
+    // this point, the body continues regardless of terminators in between.
+    if (mips::isBranch(instr) || mips::isJ(instr)) {
+      const uint32_t target = mips::isJ(instr)
+                                  ? mips::jalTarget(addr, instr)
+                                  : mips::branchTarget(addr, instr);
+      if (target > reach && target > startAddr && target < maxEndAddr)
+        reach = target;
+    }
+
+    if (isFunctionEnd(words, i) && addr >= reach) {
+      const uint32_t end = addr + 8; // terminator + its delay slot
+      return end > maxEndAddr ? maxEndAddr : end;
+    }
+  }
+
+  return maxEndAddr;
+}
+
+void clampOverlappingSizes(std::vector<FunctionInfo> &funcs) {
+  if (funcs.size() < 2)
+    return;
+  if (!std::is_sorted(funcs.begin(), funcs.end()))
+    std::sort(funcs.begin(), funcs.end());
+
+  for (size_t i = 0; i + 1 < funcs.size(); ++i) {
+    if (funcs[i + 1].address <= funcs[i].address)
+      continue; // duplicate address -- not this function's problem
+    const uint32_t room = funcs[i + 1].address - funcs[i].address;
+    if (funcs[i].size > room)
+      funcs[i].size = room;
+  }
+}
+
 // Main Entry Point
 
 void FunctionFinder::findFunctions(const ElfParser& elf) {
@@ -140,6 +240,12 @@ void FunctionFinder::computeBoundaries(const Section& text) {
         }
     }
 
+    // No function may reach into the next one. Sizes set by earlier passes
+    // (ELF symbols) and by `--add-func` after the fact never revisit each
+    // other, so a containing function keeps its old extent and the same
+    // bytes get emitted twice, under two names.
+    clampOverlappingSizes(m_functions);
+
     // Determine leaf functions: scan each function for JAL/JALR instructions
     for (auto& func : m_functions) {
         if (func.size == 0 || !text.containsAddress(func.address)) {

ps1Recomp/CMakeLists.txt

@@ -7,6 +7,7 @@ add_library(ps1Recomp_lib
     src/gte_emitter.cpp
     src/overlay_handler.cpp
     src/hle_emitter.cpp
+    src/dispatch_emitter.cpp
 )
 
 target_include_directories(ps1Recomp_lib

ps1Recomp/include/ps1recomp/dispatch_emitter.h

@@ -0,0 +1,33 @@
+#pragma once
+/**
+ * @file dispatch_emitter.h
+ * @brief Emits the body of the generated `recomp_dispatch` function.
+ *
+ * `recomp_dispatch` is the runtime resolver for every indirect call and jump
+ * the recompiler could not bind statically. Its body is fixed text, so it
+ * lives here rather than inline in the CLI: the policy it encodes -- above all
+ * what happens when an address was never emitted -- is behaviour worth pinning
+ * with tests.
+ */
+
+#include <string>
+
+namespace ps1recomp {
+
+/**
+ * @brief Generate the C++ source of `recomp_dispatch`.
+ *
+ * Resolution order: NULL guard, direct lookup, KSEG-normalised retry, BIOS
+ * entry points, BIOS table sentinels, JR-RA trampolines in RAM, and finally
+ * the unmapped-target path.
+ *
+ * **Unmapped targets abort by default.** An address that reaches the end is
+ * code the recompiler never emitted; continuing past it turns the miss into a
+ * silent no-op, so the game runs on with corrupted state and the symptom
+ * appears far from the cause. Aborting makes the failure point the cause.
+ * `PS1_DISPATCH_PERMISSIVE=1` restores rate-limited logging for when you
+ * deliberately want to see how far a knowingly-incomplete build gets.
+ */
+std::string emitDispatchBody();
+
+} // namespace ps1recomp

ps1Recomp/src/dispatch_emitter.cpp

@@ -0,0 +1,100 @@
+#include "ps1recomp/dispatch_emitter.h"
+
+namespace ps1recomp {
+
+std::string emitDispatchBody() {
+  return R"CPP(void recomp_dispatch(uint8_t* rdram, recomp_context* ctx, uint32_t addr) {
+    // Lazy-init on first call
+    if (!recomp_table_ready) recomp_init_dispatch_table();
+
+    // 1. NULL pointer guard
+    if (addr == 0) [[unlikely]] {
+        static bool nullDispatchWarned = false;
+        if (!nullDispatchWarned) {
+            nullDispatchWarned = true;
+            fmt::print("[DISPATCH] null addr suppressed (startup transient, RA=0x{:08X})\n", ctx->r[31]);
+        }
+        return;
+    }
+
+    // 2. Direct lookup in dispatch table
+    recomp_func_t fn = recomp_lookup(addr);
+    if (fn) { fn(rdram, ctx); return; }
+
+    // 3. Address normalization (KSEG mirrors) and retry
+    uint32_t phys = addr & 0x1FFFFFFFu;
+    uint32_t normalized = phys | 0x80000000u;
+    if (normalized != addr) {
+        fn = recomp_lookup(normalized);
+        if (fn) { fn(rdram, ctx); return; }
+    }
+
+    // 4. BIOS entry points (A0, B0, C0 -- any KSEG mirror)
+    if (ctx->bios) {
+        if (phys == 0xA0) { ctx->bios->executeA0(); return; }
+        if (phys == 0xB0) { ctx->bios->executeB0(); return; }
+        if (phys == 0xC0) { ctx->bios->executeC0(); return; }
+    }
+
+    // 5. BIOS table sentinel dispatch (B0:xx / C0:xx)
+    if (ctx->bios) {
+        if (phys >= 0xB000 && phys < 0xB100) {
+            ctx->r[9] = phys & 0xFF; // set $t1 = function index
+            ctx->bios->executeB0();
+            return;
+        }
+        if (phys >= 0xC000 && phys < 0xC100) {
+            ctx->r[9] = phys & 0xFF;
+            ctx->bios->executeC0();
+            return;
+        }
+        if (phys >= 0xA000 && phys < 0xA100) {
+            ctx->r[9] = phys & 0xFF;
+            ctx->bios->executeA0();
+            return;
+        }
+    }
+
+    // 6. JR RA trampoline detection in RAM
+    if (phys < 0x200000u) { // Within 2MB main RAM
+        uint32_t instr = (uint32_t)rdram[phys] | ((uint32_t)rdram[phys+1] << 8) |
+                         ((uint32_t)rdram[phys+2] << 16) | ((uint32_t)rdram[phys+3] << 24);
+        if (instr == 0x03E00008u) {
+            return; // JR RA trampoline -- no-op return
+        }
+    }
+
+    // 7. Unmapped target -- fatal unless PS1_DISPATCH_PERMISSIVE
+    //
+    // Anything reaching here is code the recompiler never emitted. Logging and
+    // continuing turns the miss into a silent no-op: execution proceeds with
+    // corrupted state and the symptom surfaces far from its cause. Aborting
+    // makes the failure point at the cause, which is what makes the fix loop
+    // converge -- run, read the address and caller, emit it, repeat.
+    static const bool s_permissive = []() {
+        const char* e = std::getenv("PS1_DISPATCH_PERMISSIVE");
+        return e && e[0] != '\0' && std::strcmp(e, "0") != 0;
+    }();
+    if (!s_permissive) {
+        fmt::print(stderr,
+                   "[DISPATCH] FATAL: unmapped call to 0x{:08X} (RA=0x{:08X}, phys=0x{:08X})\n"
+                   "           This address was never emitted by the recompiler.\n"
+                   "           Set PS1_DISPATCH_PERMISSIVE=1 to log and continue instead.\n",
+                   addr, ctx->r[31], phys);
+        std::fflush(stderr);
+        std::abort();
+    }
+    static std::unordered_map<uint32_t, uint32_t> s_unknownHits;
+    auto& hitCount = s_unknownHits[addr];
+    if (hitCount < 5) {
+        fmt::print(stderr, "[DISPATCH] Unknown target: 0x{:08X} (RA=0x{:08X}, phys=0x{:08X})\n",
+                   addr, ctx->r[31], phys);
+    } else if (hitCount == 5) {
+        fmt::print(stderr, "[DISPATCH] Unknown target: 0x{:08X} -- suppressing further logs\n", addr);
+    }
+    hitCount++;
+}
+)CPP";
+}
+
+} // namespace ps1recomp