Skip to content

func_override tier: replace or wrap any guest function with hand-written C - #174

Open
rickguedes wants to merge 7 commits into
mstan:masterfrom
rickguedes:feat/func-override-tier
Open

func_override tier: replace or wrap any guest function with hand-written C#174
rickguedes wants to merge 7 commits into
mstan:masterfrom
rickguedes:feat/func-override-tier

Conversation

@rickguedes

Copy link
Copy Markdown
Contributor

Draft for discussion. The tier is running in production on a game repo today; opening as a draft so we can align on the design before it's final.

What this is

A recomp turns the game into C, but that C is a build artifact — regenerated from the player's own disc, never hand-edited. So there has been no supported way to say "this function should do something else", which is the single most important thing a port needs to grow beyond a faithful replay: progressive decompilation, native mods, QoL features.

This PR adds that mechanism: register hand-written C against a guest address and the dispatcher calls yours instead of the recompiled original.

int my_impl(CPUState *cpu) {
    uint32_t a0 = cpu->gpr[4], a1 = cpu->gpr[5];   /* guest args      */
    ...
    cpu->gpr[2] = result;                           /* $v0             */
    return 1;                                       /* handled         */
}
/* constructor, or a mod-package registration: */
func_override_add("game.my_function", 0x8001DCB0, my_impl);

Return 1 and the guest resumes at $ra exactly as if the original ran jr $ra. Return 0 and the original recompiled/interpreted code runs untouched — an override can decline case by case (conditional pre-hook: mutate state, return 0, original runs).

No size limit. This is not a ROM patch; there is no slot to fit inside, no code cave. The replacement is native code in a native binary — five instructions or five thousand lines. That freedom is the point of recompiling rather than hacking bytes.

Where it hooks — and why that placement is load-bearing

  • In psx_dispatch_impl after the BIOS tiers (an override can never shadow a kernel service vector) and before every game backend — one address-keyed hook covers the static EXE, runtime-loaded overlays, and dirty RAM alike. Overlay code that the recompiler classified observed-PC-only (never became a standalone C function) is overridable; content-hash shard revocation/rebuild cannot orphan an override because the hook sits upstream of the shard cache.
  • At the interpreter's JAL/JALR call-resolution tiers (the reserved CRES_OVERRIDE slot). This one is scar tissue: in the tier's first game-side deployment, the dirty-RAM interpreter resolved same-region calls itself (native shard or local pc-chain) and silently bypassed every registered hook — registration is not coverage. Both interpreter call sites consult the hook ahead of both resolution paths; handled calls resume via the same call contract as a native callee.
  • Byte-identical when unused: the hook pointer stays NULL with nothing registered, so a build without overrides dispatches exactly as before. Games that never adopt the tier are unaffected (after their next regen — see below).

API surface (runtime/include/func_override.h has the full contract)

Call What it does
func_override_add(id, addr, fn) Register. Duplicate address = error, never silent last-wins
func_override_add_guarded(..., words, n) + prologue-word residency guard: if the first N guest words at addr do not match, the override declines instead of corrupting — for overlay/dirty-RAM addresses other code may occupy
func_override_call_original(cpu) The wrap primitive: run the original from inside an override (one-shot bypass; recursion re-consults, matching a guest-level wrap)
func_override_guest_call(cpu, target, site_ra) Call guest code from native, with an authentic $ra so callees, fntrace, and crash forensics see real values
psx_mod_register_function_override(id, addr, fn, guard, n) Package-gated registration (below)
func_override TCP command Live inventory: id, addr, calls, guard_misses. calls counts consults (declines included) — a decline-only probe proves an address crosses a hooked path; calls: 0 means never reached

Security model: overrides compile with the recomp — .psxmod files carry no code

This deliberately follows the existing format-5 trusted-plugin rule (MOD_PACKAGES.md, "Trusted adapters and archive safety"): a package archive supplies no native code. Overrides are C source compiled into the runtime binary by the game repo's own build (EXTRAS_SOURCES / the game's globbed mod sources). A .psxmod can only select statically-registered plugin ids:

  • psx_mod_register_function_override("pkg.feature", addr, fn, ...) queues the override at constructor time; it is armed only when the resolved package plan selects that plugin id — the same gating as activation/VBlank callbacks. Not selected = the address is never hooked, dispatch identical to a build without the mod.
  • The plugin id is a registry key, not a library path or symbol name. A malicious or corrupted .psxmod cannot inject code, hook new addresses, or reach beyond the implementations the game shipped — resolution fails before launch if an enabled plugin has no registered implementation.
  • Armed override identities participate in the committed plan, and netplay's clear-mods path drops them like every other plugin kind.
  • Direct func_override_add (no package) is for the game repo's own faithful decomp reimplementations — always on, meant to be indistinguishable from the code they replace, not player-toggleable.

How a game uses it

1. Progressive decompilation (direct, always-on — the faithful-reimplementation idiom):

/* src/decomp/game_damage_calc.c — compiled into the runtime by the game repo */
#include "func_override.h"
#include "cpu_state.h"

static int damage_calc(CPUState *cpu) {
    /* faithful C reimplementation of the routine at 0x800224E0 ... */
    cpu->gpr[2] = result;
    return 1;
}

__attribute__((constructor))
static void register_damage_calc(void) {
    /* prologue guard: decline (do not corrupt) if other overlay code is resident */
    static const uint32_t prologue[2] = { 0x27BDFFE0u, 0xAFB20018u };
    func_override_add_guarded("decomp.damage_calc", 0x800224E0,
                              damage_calc, prologue, 2);
}

2. A player-toggleable mod, gated by a .psxmod — the C side registers under the plugin id; several overrides can share one manifest plugin using the optional :label suffix (gating matches the part before the :, the full id names each row in the TCP inventory):

psx_mod_register_function_override("pkg.gunfire:aim",  0x80023F08, aim_impl,  guard, 2);
psx_mod_register_function_override("pkg.gunfire:fire", 0x8002AB08, fire_impl, guard, 2);
# manifest.toml — the .psxmod carries only this selection, no code
format_version = 5

[[feature]]
id = "gunfire"
name = "Sub-weapon gunfire"

[[plugin]]
feature = "gunfire"
id = "pkg.gunfire"

3. Wrapping instead of replacing (post-hook; also the right shape on timing-sensitive paths):

static int battle_assemble_wrap(CPUState *cpu) {
    func_override_call_original(cpu);          /* original runs fully    */
    if (player_wears_custom_body(cpu))         /* then adjust the result */
        func_override_guest_call(cpu, LOAD_VOICE_PACK, cpu->gpr[31]);
    return 1;
}

Rules an override must obey (documented in the header): pointers handed back to the guest must be guest addresses (the caller performs guest loads — a host pointer reads garbage; write into guest RAM and return that address). Keep all mutable state in guest RAM — rollback/rewind/netplay snapshot guest state only, host statics desync replays.

Verification workflow (from live deployments)

func_override over TCP is the ground truth: an override whose calls stays 0 was never reached (wrong address, or that path never ran); guard_misses counts guard declines. Because calls counts consults, registering a decline-only probe (always return 0) is a safe way to prove an address crosses a hooked path before writing the real implementation.

What was tested

  • Bushido Blade 2 (SLUS-00663), MinGW RelWithDebInfo, GL, on this code: boots to gameplay; 10 overrides armed live — 3 always-on decomp reimplementations + 7 package-gated mod overrides (including a 4-override plugin), all prologue-guarded; package gating verified both ways (disabled feature = count drops, address unhooked). Battle-path overrides observed firing in play (the wrap + guest_call path drives a real voice-pack swap).
  • The tier's earlier, more primitive revision has been shipping in a second game repo (Azure Dreams) for weeks — overlay-resident overrides at 500+ calls, user-confirmed on screen. This PR is that lineage with the interpreter-tier fix designed in, plus the wrap/guest-call/guard/gating/label API.
  • recompiler ctest on this branch's own fresh tree: 45/50 pass. The 5 failures are pre-existing on master and unrelated (mod_load_acceleration asserts a gi.has_turbo_loads line removed by the turbo_loads retirement; launcher_vulkan_option is a cp1252 decode error in the test harness on Windows; aot_overlay_discovery / release_zip / vk_present_wait_stage fail identically without this change).
  • tools/gen_tcp_commands.py --check passes; note the regenerated index also picks up rows for commands that had drifted on master since its last regen (the checked-in index says 292, the servers register 304 before this PR).
  • Honest gaps: no netplay session was run with a package-gated override armed (the clear-mods path is exercised, an actual rollback session is not); no second maintainer-side game has regenerated with this emitter change yet.

Scope / downstream

  • Fully game-agnostic: no title checks, no magic addresses; the tier is address-keyed by whatever the consuming game registers.
  • Regen required downstream: full_function_emitter.cpp emits the dispatch consult, so consuming games pick the tier up on their next BIOS + game regen. Until then their builds are unchanged. The runtime side is inert without registrations either way.
  • Cycle accounting is deliberately deferred and documented: a handled override credits no guest cycles for skipped code; the header steers timing-sensitive paths to wrap (call_original) instead of replace, until the shared cycle core exposes a public credit API. Happy to discuss whether a cycles parameter should land now instead.
  • Caps are static and small (FO_MAX_OVERRIDES 128, FO_MAX_GUARD_WORDS 4) — sized for hand-written registries, trivially raisable if a game outgrows them.

Henrique Guedes and others added 4 commits August 22, 2026 19:35
An address-keyed replace-or-decline tier for hand-written native C:

- Core registry (func_override.{h,c}): register an implementation
  against a guest address; return 1 = handled (guest resumes at $ra),
  0 = decline (the original recompiled/interpreted code runs). The
  dispatch hook stays NULL with nothing registered, so a build without
  overrides dispatches byte-identically.
- Consulted in psx_dispatch_impl AFTER the BIOS tiers (an override can
  never shadow a kernel service vector) and BEFORE every game backend
  (emitted by full_function_emitter.cpp), and at the interpreter
  JAL/JALR call-resolution tiers (the reserved CRES_OVERRIDE slot) so
  locally-resolved calls cannot bypass a hook. One hook covers the
  static EXE, runtime-loaded overlays, and dirty RAM alike.
- Ergonomics as API instead of copy-paste idioms:
  func_override_guest_call (call guest code from native, authentic $ra),
  func_override_call_original (one-shot bypass = real wrap semantics),
  func_override_add_guarded (prologue-word residency guard for overlay
  addresses other code may occupy).
- Mod-system integration: psx_mod_register_function_override queues
  under a plugin id; overrides ARM only when the resolved package plan
  selects that plugin -- same gating as vblank/activation callbacks.
- func_override TCP command: per-override id/addr/calls/guard_misses;
  calls counts consults (declines included) so a decline-only probe
  proves an address crosses a hooked path.

Cycle crediting is documented as deferred until the shared cycle core
exposes a public credit API; overrides should wrap (call_original) on
timing-sensitive paths. Rollback constraint documented: all mutable
override state belongs in guest RAM.
An id whose only registration is a queued function override was
invisible to mod_plugin_registered, so a manifest gating an
override-only plugin failed resolution with 'trusted plugin is
unavailable'. Registration now marks the id in the plugin registry
(same map as activation/vblank). MOD_PACKAGES.md documents the
function-override plugin kind and the direct-registration
(progressive-decomp) idiom.
…TCP command

The func_override TCP inventory doubles as the live list of what native
mods hooked, but a plugin that registers several overrides showed one
indistinguishable id per row (only the address told them apart). Adopt
the registry convention from the tier's original game-side deployment:
an optional ":label" suffix on the registered id ("pkg.feature:aim")
names the override in diagnostics, while gating and resolver
availability match only the part before the ':' — several overrides sit
under one manifest [[plugin]] entry and still read apart.

Also add the missing TCP_COMMANDS.md entry for `func_override` (the
command existed, the doc row didn't), spelling out the decline-probe
workflow: `calls` counts consults, declines included, so a decline-only
probe proves an address crosses a hooked path. Index regenerated.
…he guard

Review fixes for the func_override tier (PR mstan#174), tracked as beads-eio.3.59.
All three were silent failures: nothing at runtime reports them.

1. Coverage. Both interpreter call sites consulted the hook AFTER
   interp_enter_compiled, which reaches psx_dispatch_game_compiled ->
   entry->fn(cpu) directly and never re-enters psx_dispatch_impl. Any
   override on a statically-compiled function called from interpreted
   (dirty-RAM / overlay) code therefore ran the ORIGINAL, while
   registration, the armed count and the func_override inventory all
   looked healthy. The consult now precedes enter-compiled at both sites,
   matching the placement psx_dispatch_impl already uses and the
   invariant the header documents. Tail-transfer sites are deliberately
   NOT hooked; the header now states that overrides are call-site keyed
   so a calls==0 on a tail-called address is explained rather than
   mysterious.

2. Teardown. func_override.c had no removal path, so armed overrides
   outlived a cleared mod plan. Clearing s.plan is enough for
   activation/vblank callbacks because those only run while something
   iterates the plan, but an armed override lives in this module's own
   table with the hook installed. On the rematch path (main.cpp jumps to
   session_reboot, past mod_runtime_activate_plugins and
   func_override_install) a modded session entering netplay printed the
   vanilla-session banner and kept running its overrides, diverging from
   a peer without the mod. Adds func_override_add_package /
   func_override_reset_package_armed, wired into
   mod_runtime_clear_for_netplay, dropping package-armed entries while
   keeping direct always-on registrations.

3. Observability. The residency guard read through psx_read_word, which
   is traced: it feeds ls_read_hook under lockstep, RETURNS the replayed
   value under lockstep replay, and calls ds_note_read under DuckStation
   recording. Guard words were therefore injected into the divergence
   streams as phantom guest reads, and compared replayed data instead of
   resident bytes during replay. Adds psx_peek_word_untraced (same
   address decode, no tracing) and uses it for the guard.

Also: reject an address normalising to phys 0 (it collides with the
not-inside-an-override sentinel and would silently break
call_original); give func_override_get/_get_ex an id buffer size instead
of an implicit FO_MAX_ID requirement; clamp the accumulated length in
handle_func_override so raising FO_MAX_ID or FO_MAX_OVERRIDES cannot
underflow the remaining-size argument; move XRES_OVERRIDE to the end of
its enum with a note that it is appended, NOT slotted into consult order
(that enum is dense, unlike CRES, so inserting would renumber the codes
above it and invalidate captured xprobe traces).

Corrects two claims in the header: the interpreter placement, and the
assertion that a cycle-credit parameter is blocked on a missing API.
psx_advance_cycles is public in psx_cycles.h and bios_hle.c already
charges per service with it. The zero-credit behaviour is unchanged here
and now documented as an unresolved POLICY question, not a technical
one.

Adds runtime/tests/test_func_override.c (registered in
runtime/CMakeLists.txt): install-is-NULL-when-empty, argument and
duplicate refusal, consult counting for declines, guard decline without
running the body, call_original one-shot semantics, bypass consumption
proven via a self-recursive original, package reset keeping direct
entries, and bounded id copy. Verified by mutation: no-op'ing the
package reset, removing the phys 0 check, and leaving the bypass armed
each fail the suite.

Not verified here: no full runtime link and no game run, so fix 1 is
confirmed by compile and reasoning only, not by observing an override
fire from interpreted code. That is acceptance criterion 1 on
beads-eio.3.59 and remains open.
@mstan

mstan commented Aug 23, 2026

Copy link
Copy Markdown
Owner

Thanks for opening this as a draft — the design reads well, and the hook-placement notes made it much easier to review.

I read the change against master, and I have pushed one commit onto this branch (8105d937) with fixes for three things I found. To be clear about that: it is a fast-forward on top of your three commits, nothing of yours was rewritten. Please assess it as a proposal, not a decision. If you disagree with any part, say so and we roll it back or split it out — I pushed it rather than describing it because it is easier to judge code than prose.

There is also one thing I would like to decide together, and I have deliberately not touched it.

The thing I want to discuss: cycle accounting

The header says a cycle-credit parameter is deferred "until the shared cycle core exposes a public credit API." I do not think that blocker exists. psx_advance_cycles() is already public in runtime/include/psx_cycles.h, and the BIOS HLE tier right next to this one already charges per service with it (bios_hle.c:184, 193, 206, 218, 262) under its own TIMING NOTE.

So the mechanism is there. What is left is a policy choice, and that is what I would like your view on.

My concern: with zero credit, a handled override collapses the replaced function to 0 guest cycles and shifts IRQ phase. For a player-toggled mod that is fine. For the always-on faithful-decomp use case — the one the header describes as "indistinguishable from the code they replace" — a rewrite that takes zero time is not indistinguishable. Interrupt-phase drift is the class of bug this project treats as foundation work, so I am wary of a tier that defaults the whole thing to free.

My suggestion is that the credit becomes a required argument at registration, even when the honest answer is 0, so every call site has to state its timing intent instead of inheriting a silent default.

Two questions for you, since you have actually shipped this and I have only read it:

  1. Does a required credit argument sound right, or does it get in the way in practice?
  2. For a wrap via func_override_call_original, what should be charged? The original runs and accrues its own cycles, so I would expect the wrapper to charge only its own added work — but tell me if that is wrong.

In the commit I only corrected the header's factual claim about the missing API and relabelled this as an open policy question. The zero-credit behaviour itself is unchanged.

What the commit fixes

1. The override is skipped for interp → statically-compiled callees.

At both interpreter call sites (dirty_ram_interp.c:1586 JALR, :1797 JAL) the consult sat after interp_enter_compiled. That path goes interp_enter_compiledpsx_dispatch_game_compiledentry->fn(cpu) (emitted at main_psx.cpp:1632) and never re-enters psx_dispatch_impl, so the hook is never reached. An override on a static-EXE function called from interpreted overlay/dirty-RAM code runs the original — while registration, the armed count, and the func_override inventory all still look healthy.

It is the same "registration is not coverage" shape you fixed for the overlay-native and pc-chain paths. The commit moves the consult above that block at both sites, which also restores the invariant the header states ("BEFORE every game code backend").

I also documented the tail-transfer sites (:3114, :3163) as deliberately not hooked — overrides are call-site keyed, so a j-style tail call into an overridden address will show calls: 0. If you would rather those fire too, that is a real design choice and I am happy either way; I only wanted the answer written down instead of implied.

2. There is no teardown, so clear-mods cannot disarm an override.

func_override.c only ever adds; s_count never shrinks. Clearing s.plan works for activation/vblank callbacks because those only run while something iterates the plan, but armed overrides live in the module's own table with the hook installed.

On the rematch path this is reachable: main.cpp:13824 clears, then goto session_reboot lands at main.cpp:12275 — past mod_runtime_activate_plugins and func_override_install. So a modded session entering netplay prints the vanilla-session banner and keeps running its overrides, diverging from a peer without the mod.

The commit adds func_override_add_package / func_override_reset_package_armed, called from mod_runtime_clear_for_netplay. It drops package-armed entries and keeps direct ones, on the reasoning that direct registrations are the game's own always-on reimplementations — not player-selectable, and identical in both peers' builds. Tell me if you would rather it dropped everything.

3. The residency guard read through a traced accessor.

psx_read_word is not an inert peek (memory.c:1436): under lockstep it feeds ls_read_hook, under lockstep replay it returns the replayed value instead of RAM, and under DuckStation recording it calls ds_note_read. So guard words showed up in the divergence streams as reads the guest never made, and under replay the guard compared replayed data rather than resident bytes.

The commit adds psx_peek_word_untraced (same address decode, no tracing) and uses it for the guard. It is also cheaper than the traced path.

Smaller items in the same commit: reject an address normalising to phys 0 (it collides with the "not inside an override" sentinel and would quietly break call_original); give func_override_get/_get_ex an id buffer size instead of an implicit FO_MAX_ID requirement; clamp the accumulated length in handle_func_override so raising FO_MAX_ID or FO_MAX_OVERRIDES cannot underflow the remaining-size argument.

I moved XRES_OVERRIDE to the end of its enum but deliberately did not renumber it into consult order — that enum is dense, unlike CRES, so inserting would shift the codes above it and invalidate previously captured xprobe traces.

Tests

The tier had no tests, and every failure mode here is silent, so I would rather pin the behaviour than rely on field reports. The commit adds runtime/tests/test_func_override.c (registered as func_override_test): install-is-NULL-when-empty, argument and duplicate refusal, declines still counting as consults, guard declining without running the body, call_original one-shot semantics, package reset keeping direct entries, and bounded id copy.

I checked the test is not vacuous by breaking the code on purpose three times. Two failures were caught immediately. The third — leaving the one-shot bypass armed — passed, which was a hole in my test, not in your code: call_original restores the saved bypass value on return, so a non-recursive wrap looks correct even when hook() never clears the flag. The clear only matters when the original recursively calls itself, which is exactly what the header promises. I added a self-recursive-original case and it now catches it.

Two things I checked that are fine

So you do not have to worry about them:

  • full_function_emitter.cpp is in runtime/codegen_hash_sources.cmake:27, so the cg hash moves and overlay shard caches self-invalidate. No stale-shard risk from the emitter change.
  • CRES_OVERRIDE = 6 really was pre-reserved in consult order on master (dirty_ram_interp.c:425), exactly as you described.

I also want to withdraw something before you read it anywhere else: I initially thought the TCP_COMMANDS.md regen had introduced CRLF damage. I measured it properly afterwards — master is 540 CRLF lines plus 27 stray LF lines, and your regen normalised those 27 to match. It cleaned up an existing inconsistency. My mistake.

What is NOT verified

I want to be straight about the limits of this review. Everything above came from reading the code, plus compiling the new test. Specifically:

  • I have not run a game and watched an override fire from interpreted code, so fix 1 is confirmed by compile and reasoning only. Given that its failure mode is invisible by construction, that is the one I would most like to see measured before this leaves draft.
  • I did not do a full runtime link or a cmake configure, only per-file syntax checks on the files I touched plus a standalone build of the new test.

If any of the three fixes conflicts with something you learned running this in Bushido Blade 2 or Azure Dreams, your field experience beats my reading — say so and we will change it.

@mstan

mstan commented Aug 23, 2026

Copy link
Copy Markdown
Owner

Follow-up: I said the coverage fix (defect 1) was reasoned about but not measured. It is measured now, in a real game. Reproduced on the PR head, confirmed fixed by the commit.

Setup

Single-variable A/B, headless, Ape Escape (SCUS-94423):

base arm fix arm
PSXRECOMP_ROOT _wt-pr174-base @ 6ae886e9 (your head) _wt-pr174-fixes @ 8105d937
build RelWithDebInfo / Ninja identical
disc, game.toml, flags, probe string identical identical
build dir, debug port, exe name separate (4621) separate (4622)

git diff 6ae886e9..8105d937 touches zero recompiler/ files, so both arms share an identical cg hash and identical generated code. The one variable is where the interpreter consults the hook.

The instrument was a temporary decline-only probe mod driven by an env var (PSX_FO_PROBE=<addrs>), registered from a constructor. Every probe returns 0, so the original always runs and the only observable effect is the calls counter — the decline-only probe idiom your header documents. It has since been deleted from the title repo.

Result

Probes on the two statically-compiled targets, after ~290M / 231M interpreted instructions:

address base fix
0x8002C0E4 0 29,249
0x8002C14C 0 12,645

41,894 overridden calls that silently ran the original on the PR head.

Controls, same run — six probes on interp→overlay targets, which reach the consult in both arms because interp_enter_compiled declines for them:

address base fix
0x8013AE80 6,992,463 6,992,463
0x80136E00 2,220 1,676
0x8013ADA0 6 6
0x80136E68 4 4
0x80136454 2 2
0x801369E0 1 1

6/6 fire in both arms, one of them bit-identical at 6,992,463. So the base arm's probe machinery is fully functional and a zero on the compiled targets is a genuine miss, not a dead instrument. The base arm also executed more interpreted work than the fix arm (288M vs 231M instructions) and still recorded zero, so it is not a workload artefact.

No regression. Both arms render the title screen correctly, zero abort/fatal/wedge markers in either log. Worth calling out specifically: the fix arm processed 41,894 declining consults on compiled functions and every one fell through to the compiled original correctly — so moving the consult above interp_enter_compiled did not break the fall-through.

How to find these targets again

callret_watch with a nonzero lo (lo=0 silently disables the ring — callret_begin tests !g_callret_lo), sampled repeatedly, then histogram path & 0xFF against the CRES enum. Codes 2–5 are CRES_EC_*, i.e. the interpreter resolved into compiled code — exactly the bypassed route. Ape showed 186 of 1143 sampled JALR resolutions on EC paths.

A negative result worth recording

Tomba 2 cannot demonstrate this bug, and I want to save you the run. 2290 sampled JALR resolutions there produced zero EC paths — only NL_RET and PCCHAIN. Its whole text region is dirty, so psx_game_text_native_ok always fails, interp_enter_compiled always declines, and every call therefore reaches the hook in both arms. Probes on Tomba 2 targets read 10/10 and 1/1, identical across arms, for that reason and not because the fix is inert.

Practical consequence for the tier: whether this gap bites a given title depends on whether its static text stays clean. A game that self-modifies its text is accidentally immune; one that does not is exposed. That is not a property anyone would guess from the registration API, which is part of why I wanted it measured rather than argued.

Still not measured

Being straight about what this run does and does not cover:

  • Defect 2 (netplay teardown) — unit test plus mutation testing only. No actual rollback session was run, the same gap you flagged in your own testing notes.
  • Defect 3 (untraced guard read) — compile and reasoning only.
  • The cycle-credit question is untouched and still yours to weigh in on.

@mstan

mstan commented Aug 23, 2026

Copy link
Copy Markdown
Owner

Housekeeping: this branch is now up to date with master, and some observability tooling it depends on has landed there.

Branch updated. I merged current master into this branch (aa924200) rather than rebasing, so none of your commits were rewritten and you do not need to reset anything locally — a plain git pull will do. Only TCP_COMMANDS.md conflicted; I resolved it by regenerating (it is a generated file), and verified all three pieces survived: your func_override row, the new xprobe_watch row, and the callret_watch prose. The index now reports 306 commands and --check passes. The merged tree builds clean and the func_override unit test still passes.

Tooling that landed in master (#176), all found while measuring the coverage fix on this branch:

  • callret_watch armed on lo != 0, so address 0 was a silent off switch — lo=0 hi=0x200000 replied ok and recorded nothing. Measured on one build: lo=0 gave total 0 where lo=4 gave 468,732. Now armed on a non-empty window, reports armed, and refuses lo without hi instead of inheriting the previous ceiling. The documented {"lo":"0"} disarm still works and now says so.
  • s3_smear_watch had the identical gate. Same fix.
  • The xprobe JAL/JALR watched-target list was six guest addresses from MMX6 and Tomba compiled into the shared runtime, so every other title watched nothing, silently. It is now set via an xprobe_watch command or PSX_XPROBE_WATCH, default empty.

That last one is why the earlier measurement had to use the JALR callret ring rather than the JAL site: on Ape Escape the JAL watched dump was structurally incapable of recording. With it fixed, the same targets now produce 41 and 21 JALR records with full context.

Nothing in #176 changes the func_override tier itself — it is observability only. But if you want to reproduce the coverage measurement yourself, the tools now work on any title rather than just the two that were hardcoded.

Still open from my side and unchanged: the cycle-credit policy question. I have not touched the zero-credit behaviour, and that one genuinely needs your judgement.

Resolves the open policy question: a handled override replaced code that
took guest time on hardware, and every device schedules against that
clock, so zero-credit-by-default silently shifts IRQ phase for the rest
of the session. Every registration now states its timing intent — there
is no default to inherit. Two forms:

  credit >= 0       charged via psx_advance_cycles on every HANDLED
                    call, after the body runs (declines charge nothing:
                    the original runs and accrues its own cycles). 0 is
                    legal and visible — right for a player-toggled mod
                    whose behavior has no hardware analog.

  FO_CREDIT_SELF    the tier charges nothing; the body owns its timing
                    and calls psx_advance_cycles itself. Required for
                    data-dependent costs (base + per_iteration * n, the
                    bios_hle.c TIMING NOTE pattern) and for wraps: the
                    original re-dispatched by call_original accrues its
                    exact cycles by executing, so a fixed nonzero credit
                    on a wrap would double-count.

Any other negative credit is FO_ERR_ARGS (a typo, not a policy). The
declared credit is surfaced per entry by func_override_get_ex and the
func_override TCP command ("self" or the number), so timing intent is
inspectable, not just documented. Credits are approximations of the
original's dynamic instruction count unless measured; LLE remains the
timing oracle (decline the override and sample cycle deltas across the
call boundary via the callret ring to measure a real distribution).

Tests: credit below FO_CREDIT_SELF refused; a handled call charges
exactly the declared credit; a decline charges nothing; SELF and 0
charge nothing from the tier; get_ex reports both forms. Verified by
mutation (suppressing the charge fails the suite). The suite gains
cycle-core doubles so the charge is observable through psx_cycle_count.

Also repairs mod_runtime_test, which has been unbuildable since the
tier landed: it links mod_runtime.cpp, which references
func_override_add_package/_install, but never linked func_override.c.
The target now links the tier with inert doubles, so the package
arming/reset paths in mod_runtime.cpp are exercised for real.

Restores the func_override row in the curated TCP_COMMANDS.md inventory
(lost in the aa92420 merge resolution, which regenerated only the
autogenerated index) and documents the credit field; index regenerated,
gen_tcp_commands.py --check passes.
@rickguedes

Copy link
Copy Markdown
Contributor Author

Cycle credit: agreed, decided, and implemented — b35d9464 on this branch. You were right that the blocker never existed; the header's claim was wrong and the deferral with it. Answers to your two questions first, then what landed.

Q1 — required credit argument: yes, but not as a bare constant

A required argument is right: silent-default-to-free is exactly the failure mode this tier must not have. But a registration-time constant can't express most real functions — original costs are data-dependent (loops, branches, the 36-cycle div). Force a constant everywhere and decomp authors will register a misleading number, which is worse than an honest zero because it looks accounted for.

So the required argument takes two forms:

  • credit >= 0 — the tier charges exactly that via psx_advance_cycles on every handled call, after the body runs. Declines charge nothing: the original runs and accrues its own cycles, so a tier-side charge would double. 0 stays legal but is now a visible statement at the registration line — right for a player-toggled mod whose behavior has no hardware analog, and indefensible-by-inspection for an always-on reimplementation.
  • FO_CREDIT_SELF — the tier charges nothing; the body owns its timing and calls psx_advance_cycles itself, which is how a data-dependent cost gets charged honestly (base + per_iteration * n, your bios_hle TIMING NOTE pattern, per path taken).

Any other negative value is FO_ERR_ARGS — a typo, not a policy.

Q2 — wraps: exactly as you expected

The original re-dispatched by func_override_call_original accrues its exact cycles by executing, and so does anything invoked via func_override_guest_call. So a wrap charges only its own added work — and since a mod's added logic has no hardware analog, that is usually nothing. Which means: a wrap must register FO_CREDIT_SELF — a fixed nonzero credit on a wrap double-counts. That rule is now stated in the header's CYCLE ACCOUNTING section and at psx_mod_register_function_override, and the credit is only ever charged by the tier, never by call_original itself, so the invariant is structural, not just documented.

What the commit contains

  • Required credit on all three registration paths (func_override_add, _add_guarded, _add_package) and on psx_mod_register_function_override. Charge site: in the hook, handled-only, after the body.
  • The declared policy is inspectable: func_override_get_ex and the TCP command report credit per entry ("self" or the number), so the inventory now reads as a timing-intent ledger, not just a coverage ledger.
  • Tests: invalid credit refused; a handled call charges exactly the declared credit; a decline charges nothing; SELF and 0 charge nothing from the tier; both forms surfaced via get_ex. Mutation-checked (suppressing the charge fails the suite). The test gains cycle-core doubles so charges are observable through psx_cycle_count directly.
  • Found while wiring the tests: mod_runtime_test has been unbuildable since the tier landed — it links mod_runtime.cpp, which references func_override_add_package/_install, but never linked func_override.c. Fixed (the target now links the tier with inert doubles), so your teardown paths in mod_runtime.cpp are exercised by an actually-running test. Also restored the curated func_override row in TCP_COMMANDS.md — the aa924200 conflict resolution kept the generated index row but dropped the prose row — and documented the new field; gen_tcp_commands.py --check passes.

Field verification (Bushido Blade 2, SLUS-00663)

Merged this branch into the game integration branch, regenerated (your main_cli/strict_translator/emitter-hardening changes included), rebuilt MinGW RelWithDebInfo, and updated all ten shipping registrations to state their credits:

  • 7 mod overrides are wraps → FO_CREDIT_SELF (their originals self-charge; the added logic bills nothing).
  • 1 decomp with a path-dependent cost → FO_CREDIT_SELF + per-path charges in the body (12 on the miss path, 58 on the skip path — the R3000 div alone stalls 36).
  • 2 decomps with bounded flat costs → fixed 52 / 64, commented as approximations of the original's dynamic count. The 64 case runs its heavy callees through func_override_guest_call, which self-charge, so the constant covers only the loop shell.

Boots to gameplay; the TCP inventory shows all ten entries with their credit column. The approximations are labeled as approximations in the source, with measurement planned — which brings me to:

Follow-up proposal: measured credits, using the #176 tooling

Since LLE stays the timing oracle, the decline-only probe that proves coverage can also measure the credit: run with the override declining, sample the cycle-counter delta across the original's call boundary (the callret ring, now armed correctly on any title after #176), and you get the original's empirical cost distribution per address. That upgrades "approximation of the dynamic count" to "measured on this title" without this PR growing any further. I'd like to build that as a follow-up once this lands.

Your three fixes

Accepted as-is, and thank you — the Ape Escape A/B on the coverage gap is exactly the measurement this needed, and the teardown policy (drop package-armed, keep direct) is what we'd have chosen: direct registrations are identical in both peers' builds by construction. The untraced guard read also explains phantom reads we'd have chased eventually. The tail-call decision (call-site keyed, documented) is right for both shipping games — every registered override there is call-reached.

One unrelated observation from running the full runtime/ test project on this branch: gte_register_access_test fails to link on current master — gte.cpp references gpu_ws_precise_nclip_enabled (introduced by 200b5bf0) and the standalone test target doesn't provide it. Pre-existing, untouched here; flagging rather than fixing so this PR stays on topic.

@mstan
mstan requested review from TechnicallyComputers and mstan and removed request for TechnicallyComputers August 31, 2026 18:45
@rickguedes
rickguedes marked this pull request as ready for review August 31, 2026 20:09
@Alexbeav

Alexbeav commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Additional validation and remaining gates

I completed a separate validation pass against PR head 6524ded0db50537bf0feb68a37f68150ae4fedf1.
The proposed hardening and source-owned tests are in fork review PR #19.

The final reviewed head is ec7fccb13e9d0611faa3161dcaf489662404ac7e.
The fork review found problems in the initial fixes. All valid findings are corrected in this head.

Source-owned results

  • The default runtime build passed all 62 enabled tests. Two unrelated tests stayed disabled.
  • The rewind and netplay build passed all 62 enabled tests. The same two tests stayed disabled.
  • Seven focused reset, rollback, rewind, and function-override routes passed.
  • The real overlay loader passed its lifecycle route and five dynamic publication routes.
  • The recompiler passed 54 of 57 enabled tests. The three errors match the qualified baseline.
  • python tools/gen_tcp_commands.py --check passed with 311 commands.

The three baseline errors are a stale source guard, an absent hard-coded compiler path, and a Windows newline assumption.

Timing evidence

A Silent Hill differential covered 10,007 architectural seeds at -O0 and -O2.
It also covered 20,014 cold and warm cycle cases.

The original function produced cycle deltas of 24, 27, 31, 43, and 47.
One fixed credit cannot represent all calls.

A FO_CREDIT_SELF replacement matched the current psxrecomp model in all compared cases.
This result proves psxrecomp-model parity for this function. It does not prove hardware timing.

A source-owned cycle ROM also recorded 8 of 8 hits at all 15 anchors on native and Beetle PSX.
Twelve stable loop classes matched.
Native was one cycle higher on DIV and MULT loops, and three cycles higher on the spaced-DIV loop.

Beetle warned that the available firmware identity was unsupported.
Therefore, this comparison is independent-emulator evidence, not physical-hardware proof.

Current-source title routes

Current-source Silent Hill and Vagrant Story resident builds regenerated and linked.
Both hidden LLE runs reached their first dynamic-code boundary.

The runs then used interpreter fallback because no title-owned override package was installed.
These results prove current-source build closure and dynamic-boundary entry.
They do not prove title-level function-override acceptance.

Remaining work

  • A maintainer must review and decide whether to accept the hardening changes.
  • The three cycle-model differences need a correction or an explicit acceptance decision.
  • Title-level acceptance needs a title-owned function-override package.
  • A physical-hardware timing claim needs physical-hardware evidence.

Developed with AI assistance; validated as described (test evidence in PR body). AI writes the code and the PR, but I always test before I send something up. Happy to iterate on this process with your feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants