Skip to content

Commit e564caa

Browse files
doublegateclaude
andcommitted
docs(libretro): record the maintainer's two decisions from #556 review
Review on #556 left two policy questions open. The maintainer settled both on 2026-09-25, and both confirm what the code already does, so this records the decisions and changes no behaviour. 1. A direct `cargo build --release` of the libretro core stays a WARNING, not an error. build.rs warns when CARGO_PROFILE_RELEASE_PANIC=unwind is absent, and on_load_game logs cfg!(panic = "abort") to the frontend. A hard error was not chosen because the documented `cargo build --release --workspace` builds this crate too, and would fail the desktop build over a core that command does not ship. (Copilot's finding.) 2. Battery RAM is still written after a contained panic. A poisoned core refuses further emulation calls but keeps the console allocated and its memory readable, so RetroArch writes the save as usual. A panic interrupts between host operations and cannot tear a byte, so what is written is the cartridge RAM at one instant, the same as a power cut on hardware. Withholding it (reporting size 0 for SAVE_RAM while poisoned) would lose every save since the game was loaded. (agy's round-3 "blocking" item.) Recorded in the L-1.1 row of docs/audits/libretro-disposition.md, whose PR column now cites #556, and in to-dos/plans/v2.8.0-bulkhead-plan.md. markdownlint clean. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
1 parent 075a410 commit e564caa

2 files changed

Lines changed: 5 additions & 1 deletion

File tree

‎docs/audits/libretro-disposition.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ as it has been since the MiSTer core became the priority.
1010

1111
| Id | Finding (report §) | Verdict | Evidence | Release | PR |
1212
| --- | --- | --- | --- | --- | --- |
13-
| L-1.1 | Panics unwind across `extern "C"`; `.expect` on the run path; no `catch_unwind` (§1.1, Fix 9) | FIXED | Two halves; the unwind build costs ~0.1% of retired instructions per frame (`docs/performance.md` §v2.8.0). The shipped core aborted on panic (`[profile.release] panic = "abort"`, built `--release` on every path), so `catch_unwind` would have been dead code: the libretro builds now set `CARGO_PROFILE_RELEASE_PANIC=unwind` (crate `Makefile`, `.gitlab-ci.yml` `.core-defs`, the `libretro-cross` gate). A direct `cargo build --release` bypasses all three (and the crate is the workspace's only `default-members` entry), so `build.rs` warns when the override is absent and `on_load_game` logs `cfg!(panic = "abort")` to the frontend: verified by building both ways, the log string is in the aborting `.so` and absent from the unwinding one (Copilot, #556). Then `RustyNesLibretro::contained` wraps every emulation callback (run, serialize, unserialize, reset, load, the disk and cheat mutators) in `catch_unwind`; a caught panic POISONS the core (later calls inert) and keeps the console allocated, because the frontend holds pointers into its memory: the audit's Fix 9 (`self.nes = None`) would have been the use-after-free of L-1.3. The seven `.expect`s are `let-else` returns. Test `abi_tests::a_panic_inside_a_frame_is_contained_and_the_memory_stays_valid`. Mutations: without `catch_unwind` the test binary dies (`non-unwinding panic`, SIGABRT), exactly the frontend's fate; without poisoning, and with the audit's drop-on-panic, the test fails. The `panic!` sites inside `rust-libretro` itself are "core not initialized" guards (and two null-callback checks); they fire only if a frontend calls in before `retro_get_system_info` creates the instance. That RetroArch does not is inferred from the core running in it, not traced through RetroArch's source | v2.8.0 | |
13+
| L-1.1 | Panics unwind across `extern "C"`; `.expect` on the run path; no `catch_unwind` (§1.1, Fix 9) | FIXED | Two halves; the unwind build costs ~0.1% of retired instructions per frame (`docs/performance.md` §v2.8.0). The shipped core aborted on panic (`[profile.release] panic = "abort"`, built `--release` on every path), so `catch_unwind` would have been dead code: the libretro builds now set `CARGO_PROFILE_RELEASE_PANIC=unwind` (crate `Makefile`, `.gitlab-ci.yml` `.core-defs`, the `libretro-cross` gate). A direct `cargo build --release` bypasses all three (and the crate is the workspace's only `default-members` entry), so `build.rs` warns when the override is absent and `on_load_game` logs `cfg!(panic = "abort")` to the frontend: verified by building both ways, the log string is in the aborting `.so` and absent from the unwinding one (Copilot, #556). Then `RustyNesLibretro::contained` wraps every emulation callback (run, serialize, unserialize, reset, load, the disk and cheat mutators) in `catch_unwind`; a caught panic POISONS the core (later calls inert) and keeps the console allocated, because the frontend holds pointers into its memory: the audit's Fix 9 (`self.nes = None`) would have been the use-after-free of L-1.3. The seven `.expect`s are `let-else` returns. Test `abi_tests::a_panic_inside_a_frame_is_contained_and_the_memory_stays_valid`. Mutations: without `catch_unwind` the test binary dies (`non-unwinding panic`, SIGABRT), exactly the frontend's fate; without poisoning, and with the audit's drop-on-panic, the test fails. The `panic!` sites inside `rust-libretro` itself are "core not initialized" guards (and two null-callback checks); they fire only if a frontend calls in before `retro_get_system_info` creates the instance. That RetroArch does not is inferred from the core running in it, not traced through RetroArch's source. **Maintainer decisions (2026-09-25, review on #556):** (1) a direct `cargo build --release` of the core stays a build-time WARNING (plus the load-time log), not an error, because the documented `cargo build --release --workspace` builds this crate too; (2) a poisoned core keeps its memory readable and RetroArch still writes battery RAM after a contained panic -- a panic cannot tear a byte, so what is written is the cartridge's RAM at one instant, as after a power cut, and withholding it would lose every save since the load | v2.8.0 | #556 |
1414
| L-1.2 | `on_load_game` ignores `_game` and requires `GET_GAME_INFO_EXT` (§1.2, Fix 10) | FIXED | `lib.rs:855` took `_game` and `:899` refused a frontend without `GET_GAME_INFO_EXT`. The cause was the binding, not only the core: `rust-libretro-sys` 0.3.2's bindgen output reduced `retro_game_info` to `{ _address: u8 }` (libretro.h forward-declares it at line 2879), so `retro_load_game`'s `Some(*game)` carried one byte; already recorded in `docs/libretro/WALKTHROUGH.md:26` as why the core used `GET_GAME_INFO_EXT`. Fixed by vendoring `rust-libretro-sys` with the generated struct blocklisted and defined by hand (`vendor/rust-libretro-sys/VENDORED.md`; maintainer decision 2026-09-25), and a fallback in `on_load_game` (data, else read `path`; FDS by extension or magic). Red first: `abi_tests::a_frontend_without_game_info_ext_still_loads_the_game`. Mutations: removing the fallback turns it red; restoring the unpatched binding fails to compile (no fields `path`/`data`/`size`/`meta`) | v2.8.0 | |
1515
| L-1.3 | Memory maps not deregistered on unload (§1.3, Fix 5) | FIXED | RetroArch keeps a core's memory maps until the core itself is torn down (`runloop.c` `uninit_libretro_symbols` -> `runloop_system_info_free`), not on content unload, so after `retro_unload_game` its descriptors point at freed WRAM/SRAM/CIRAM. An empty map clears them: the `SET_MEMORY_MAPS` handler frees the old descriptors before reading the new count (`runloop.c:3058-3079`). Red test `abi_tests::unloading_withdraws_the_memory_maps` (last map had 3 descriptors after unload); `on_unload_game` now sends an empty map before dropping the console, and removing that call turns the test red | v2.8.0 | |
1616
| L-1.4 | `static mut` instance, no teardown, repeated-init panic in `rust-libretro` (§1.4) | PARTIAL | `static mut RETRO_INSTANCE` / `SYS_INFO` and the never-freed instance are `rust-libretro` design, unchanged. The leak that was ours is fixed: `on_deinit` now releases the core's buffers and consoles. The repeated-init panic is REFUTED: `set_core` runs only from `retro_get_system_info` while `SYS_INFO` is unset, once per process, and `retro_init` never calls it; `abi_tests::the_core_works_again_after_deinit_and_init` shows deinit + init + load working. The data-race claim needs a frontend calling the core from several threads, which the libretro API does not permit | v2.8.0 | |

‎to-dos/plans/v2.8.0-bulkhead-plan.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,10 @@ v2.7.6's `main`.
120120
caught: no deregistration, no headroom, tail not zeroed, the device maximum
121121
at 13.
122122
- **L-1.2 fixed** (maintainer decision 2026-09-25: vendor the patched binding).
123+
- **Two review questions settled by the maintainer (2026-09-25, #556):** a
124+
direct `cargo build --release` of the core keeps the build-time warning
125+
rather than failing the build, and battery RAM is still written after a
126+
contained panic.
123127
`vendor/rust-libretro-sys` blocklists the generated `retro_game_info` and
124128
defines it by hand; `on_load_game` falls back to it. Red first; removing the
125129
fallback turns the test red, and the unpatched binding does not compile.

0 commit comments

Comments
 (0)