Repository navigation
feat(accuracysnes): the ares host runs the battery — A2.10 is settled 3-vs-1 #304
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,66 +1,84 @@ | ||
| # A headless ares host for AccuracySNES — the third opinion | ||
|
|
||
| **Working.** It runs the battery and reports the results block: | ||
|
|
||
| ``` | ||
| $ REF_PROJ=$PWD/ref-proj bash scripts/accuracysnes/ares_host/build.sh | ||
| $ /tmp/ares_host tests/roms/AccuracySNES/build/accuracysnes.sfc 900 | ||
| ACCURACYSNES-BEGIN | ||
| magic ACSN | ||
| done a5 | ||
| count 338 | ||
| passed 299 | ||
| failed 5 | ||
| skipped 1 | ||
| golden 33 | ||
| status 0 01 | ||
| ... | ||
| ``` | ||
|
|
||
| Reproducible: two runs give the identical tally. | ||
|
|
||
| ## Why it exists | ||
|
|
||
| Several findings sat at **2 versus 1** with no way to break the tie, because this project's | ||
| provenance rule counts ares and bsnes as **one** reference — so "RustySNES and snes9x against | ||
| Mesen2" is only 2-vs-1 if ares is not already on RustySNES's side, and nobody could check. | ||
|
|
||
| **The first thing it settled: `A2.10` ("PEI does not page-wrap").** ares **passes** it (catalogue | ||
| index 11, status `$01`). With RustySNES and snes9x also passing, that is **3 against 1 with Mesen2 | ||
| the outlier**, and the row comes off the "unexplained, needs a fourth opinion" list. | ||
|
|
||
| ## The five rows ares disagrees with the cart about | ||
|
|
||
| New information, and **not yet adjudicated** — the cart, snes9x and Mesen2 all pass these: | ||
|
|
||
| | idx | row | code | | ||
| |---:|---|---:| | ||
| | 116 | `C7.05` | 1 | | ||
| | 120 | `C7.10` | 1 | | ||
| | 265 | `E8.02` | 3 | | ||
| | 276 | `E3.06` | 2 | | ||
| | 288 | `F1.10` | 2 | | ||
|
|
||
| **Treat `F1.10` as suspect-of-this-host first.** It is a `PAD2_CONTRACT` row, and this host's port | ||
| detection (`port->name().find("2")` on the button's grandparent) is *assumed* to work, not verified. | ||
| Check that before concluding anything about ares. | ||
|
Comment on lines
+44
to
+46
|
||
|
|
||
| The standing heuristic — three implementations agreeing usually means a broken test, one disagreeing | ||
| usually means a real bug — cuts an unfamiliar way here: it is **ares** alone, on rows the other three | ||
| pass. | ||
|
|
||
| ## Three setup steps that are not optional, each of which cost a debugging round | ||
|
|
||
| All three fail as a **segfault inside `System::load`**, with a backtrace pointing at memory setup | ||
| rather than at what is actually missing. | ||
|
|
||
| 1. **`ares::Memory::FixedAllocator::get()` before anything touches a core.** `Bus::reset()` | ||
| allocates its page tables from that bump allocator. desktop-ui does this on its first line. | ||
| 2. **`ares::SuperFamicom::option("Pixel Accuracy", "true")` before `load`.** `PPUBase::implementation` | ||
| is null until `setAccurate` picks one of the two PPUs, and `Bus::reset()` calls `ppu.map()` → | ||
| `implementation->map()`. `"true"` selects the **accurate** PPU, the only one worth | ||
| cross-validating against. | ||
| 3. **`nall/main.hpp` included by this translation unit.** It emits `::main` only when | ||
| `NALL_MAIN_IMPL` is undefined, and nall's own `main.cpp.o` defines that. Omit it and the *link* | ||
| fails with a bare `undefined reference to 'main'` from `crt1.o`, which reads like a missing object | ||
| file rather than a missing shim. | ||
|
Comment on lines
+52
to
+66
There was a problem hiding this comment. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Correct the repeated setup-failure description. Both documents group the allocator, PPU, and
As per path instructions, documentation is the specification and must match the implementation and build recipe. 📍 Affects 2 files
🤖 Prompt for AI AgentsSource: Path instructions |
||
|
|
||
| Plus one build fact: **`hiro` is not optional even headless** — `mia/mia.hpp` includes it, and its | ||
| generated `resource/resource.hpp` does not exist until `ninja hiro` has run once. | ||
|
|
||
| `ptrace` is denied in this sandbox, so gdb cannot attach. The host installs its own `SIGSEGV` | ||
| handler and prints a backtrace; resolve the frames with `addr2line -Cfe /tmp/ares_host 0x…`. | ||
|
|
||
| ## Results-block offsets | ||
|
|
||
| From `asm/runtime.inc`, and easy to get wrong by one field: `R_COUNT` is `+$06`, `R_PASSED` is | ||
| `+$0A`. Reading the latter as the former reported "count 299" for a 338-test battery, which looks | ||
| like a truncated run rather than a misread field. | ||
|
|
||
| ## Not yet wired into `crossval.sh` | ||
|
|
||
| Deliberately. Adding a third reference means an `ARES_KNOWN_FAILURES` constant, and that constant | ||
| must not be written until the five rows above are adjudicated — a known-failure count that encodes | ||
| unexamined disagreements is worse than no third reference at all. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,25 @@ | |
| #include <cstdio> | ||
| #include <cstring> | ||
| #include <cstdlib> | ||
| #include <csignal> | ||
| #include <execinfo.h> | ||
| #include <unistd.h> | ||
|
|
||
| namespace { | ||
|
|
||
| // A crash here is a setup mistake in this file, not an ares bug, and this sandbox cannot run a | ||
| // debugger (`ptrace` is denied), so the backtrace has to come from inside the process. Build with | ||
| // `-rdynamic` — `build.sh` does — or the frames come back as bare addresses that `addr2line -Cfe` | ||
| // still resolves. | ||
| auto crashHandler(int sig) -> void { | ||
| void* frames[40]; | ||
| int n = backtrace(frames, 40); | ||
| fprintf(stderr, "\nares_host: signal %d — backtrace follows\n", sig); | ||
| backtrace_symbols_fd(frames, n, 2); | ||
| _exit(9); | ||
|
Comment on lines
+22
to
+27
There was a problem hiding this comment. 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift Keep
Keep the handler limited to a fixed 🧰 Tools🪛 Clang (14.0.6)[warning] 23-23: do not declare C-style arrays, use std::array<> instead (modernize-avoid-c-arrays) [warning] 24-24: variable name 'n' is too short, expected at least 3 characters (readability-identifier-length) [warning] 25-25: do not call c-style vararg functions (cppcoreguidelines-pro-type-vararg) 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| } // namespace | ||
|
|
||
| namespace { | ||
|
|
||
|
|
@@ -78,9 +97,17 @@ auto main(Arguments arguments) -> void { | |
| printf("usage: ares_host <rom.sfc> <frames>\n"); | ||
| exit(2); | ||
| } | ||
| bool verbose = (bool)arguments.take("--verbose"); | ||
| string rom = arguments[0]; | ||
| u32 budget = (u32)toNatural(arguments[1]); | ||
|
Comment on lines
+100
to
102
There was a problem hiding this comment. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files 'scripts/accuracysnes/ares_host/*' | sed -n '1,120p'
printf '%s\n' '--- host source context ---'
host=$(git ls-files | grep -E '(^|/)scripts/accuracysnes/ares_host/ares_host\.cpp$' | head -n 1 || true)
if [ -n "$host" ]; then
cat -n "$host" | sed -n '80,125p'
fi
printf '%s\n' '--- Arguments definitions and uses ---'
rg -n --glob '!snesdev_wiki/**' \
'class Arguments|struct Arguments|Arguments::|arguments\.take|take\([^)]*--verbose|arguments\.size\(\)|operator\[\]' \
. | sed -n '1,240p'Repository: doublegate/RustySNES Length of output: 2447 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- build script ---'
cat -n scripts/accuracysnes/ares_host/build.sh
printf '%s\n' '--- source header and dependency references ---'
cat -n scripts/accuracysnes/ares_host/ares_host.cpp | sed -n '1,45p'
rg -n --glob '!snesdev_wiki/**' \
'ares|nall|Arguments|github.com|commit|checkout|submodule' \
scripts/accuracysnes/ares_host README.md .github 2>/dev/null | sed -n '1,240p'Repository: doublegate/RustySNES Length of output: 22865 🌐 Web query:
💡 Result: In the ares-emulator/ares source code, the nall::Arguments class (found in nall/nall/arguments.hpp) provides a wrapper around a std::vector to manage command-line arguments [1]. The semantics for the requested methods are as follows: size The size method returns the number of arguments currently held in the internal collection as a u32 type [1]: auto size const -> u32 { return arguments.size; } [1] operator[] The subscript operator provides access to the individual argument strings at a given index. It includes both mutable and const overloads, returning a reference to the string [1]: auto operator[](u32 index) -> string& { return arguments[index]; } [1] auto operator[](u32 index) const -> const string& { return arguments[index]; } [1] take The take method is used to consume and remove specific arguments from the internal collection, which is useful for parsing command-line flags and their values [1][2]. It is overloaded to support different search patterns: 1. take -> string: Removes and returns the first element in the arguments list. 2. take(string_view name) -> bool: Searches for an argument matching the given name; if found, it removes it from the list and returns true [1]. 3. take(string_view name, bool& argument) -> bool: Searches for a flag. If found, it removes it, sets the referenced bool to true, and returns true [1]. 4. take(string_view name, string& argument) -> bool: Searches for an argument by name. If found, it removes both the name and the subsequent value from the list, assigns the value to the referenced string, and returns true [1]. These methods facilitate typical CLI patterns, such as checking for flags (e.g., arguments.take("--fullscreen")) or extracting values associated with flags (e.g., arguments.take("--system", system)) [2]. Citations:
🏁 Script executed: #!/bin/bash
set -eu
url='https://raw.githubusercontent.com/ares-emulator/ares/6f6786e0/nall/nall/arguments.hpp'
src=$(curl --fail --silent --show-error --location "$url")
printf '%s\n' '--- relevant declarations and methods ---'
printf '%s\n' "$src" | rg -n -A14 -B5 \
'class Arguments|auto size|auto take|operator\[\]|arguments\.remove|arguments\.size' | sed -n '1,260p'
printf '%s\n' '--- source length and commit URL ---'
printf 'lines=%s\n' "$(printf '%s\n' "$src" | wc -l)"
printf '%s\n' "$url"Repository: doublegate/RustySNES Length of output: 4256 🏁 Script executed: #!/bin/bash
set -eu
python3 - <<'PY'
args = ["--verbose", "rom.sfc"]
initial_size = len(args)
try:
verbose = "--verbose" in args
if verbose:
args.remove("--verbose")
final_size = len(args)
print(f"input=ares_host --verbose rom.sfc")
print(f"initial_size={initial_size}")
print(f"take_verbose={verbose}")
print(f"remaining={args}")
print(f"final_size={final_size}")
print(f"arity_check_before_take={'passes' if initial_size >= 2 else 'fails'}")
print(f"arity_check_after_take={'passes' if final_size >= 2 else 'fails'}")
if final_size < 2:
raise IndexError("arguments[1] is out of bounds")
except IndexError as error:
print(f"access_result=out_of_bounds: {error}")
PYRepository: doublegate/RustySNES Length of output: 379 Move the arity check after option removal. With 🧰 Tools🪛 Clang (14.0.6)[warning] 101-101: variable 'rom' is not initialized (cppcoreguidelines-init-variables) [warning] 102-102: variable 'budget' is not initialized (cppcoreguidelines-init-variables) 🤖 Prompt for AI Agents |
||
|
|
||
| // MUST come before anything touches a core. `Bus::reset()` allocates its page tables from this | ||
| // bump allocator, and without the first `get()` to construct it the very first load segfaults | ||
| // inside `System::load` with no hint that memory setup was the problem. desktop-ui does the same | ||
| // thing on its first line for the same reason. | ||
| signal(SIGSEGV, crashHandler); | ||
| signal(SIGABRT, crashHandler); | ||
| ares::Memory::FixedAllocator::get(); | ||
| ares::platform = &platform_; | ||
| mia::setHomeLocation([]() -> string { return {Path::userData(), "ares/"}; }); | ||
|
|
||
|
|
@@ -95,11 +122,21 @@ auto main(Arguments arguments) -> void { | |
| exit(3); | ||
| } | ||
|
|
||
| if(verbose) fprintf(stderr, "ares_host: loaded the game and system paks\n"); | ||
|
|
||
| // MUST come before `load`. `PPUBase::implementation` is null until `setAccurate` picks one of the | ||
| // two PPU implementations, and `Bus::reset()` calls `ppu.map()` -> `implementation->map()` during | ||
| // `System::load`. Without this the first load segfaults in `Bus::reset` with a backtrace that | ||
| // points at memory setup rather than at an unselected PPU. desktop-ui passes its own setting here. | ||
| // "true" selects the ACCURATE PPU, which is the only one worth cross-validating against. | ||
| ares::SuperFamicom::option("Pixel Accuracy", "true"); | ||
|
|
||
| ares::Node::System root; | ||
| if(!ares::SuperFamicom::load(root, "[Nintendo] Super Famicom (NTSC)")) { | ||
| fprintf(stderr, "ares_host: ares::SuperFamicom::load failed\n"); | ||
| exit(3); | ||
| } | ||
| if(verbose) fprintf(stderr, "ares_host: ares::SuperFamicom::load returned\n"); | ||
| if(auto port = root->find<ares::Node::Port>("Cartridge Slot")) { | ||
| port->allocate(); | ||
| port->connect(); | ||
|
|
@@ -110,18 +147,28 @@ auto main(Arguments arguments) -> void { | |
| port->connect(); | ||
| } | ||
| } | ||
| if(verbose) fprintf(stderr, "ares_host: ports connected\n"); | ||
| root->power(); | ||
|
|
||
| if(verbose) fprintf(stderr, "ares_host: powered on\n"); | ||
| while(platform_.frames < budget) root->run(); | ||
|
|
||
| // The results block, straight out of WRAM. $7E:F000 is WRAM offset $F000. | ||
| const auto& wram = ares::SuperFamicom::cpu.wram; | ||
| const u32 RESULTS = 0xF000; | ||
| // Offsets are `asm/runtime.inc`'s, and they are easy to get wrong by one field: R_COUNT is +$06 | ||
| // and R_PASSED is +$0A. Reading the latter as the former reported "count 299" for a 338-test | ||
| // battery, which looks like a truncated run rather than a misread field. | ||
| auto rd16 = [&](u32 off) -> u32 { return wram[RESULTS + off] | (wram[RESULTS + off + 1] << 8); }; | ||
| u32 count = rd16(0x06); | ||
| printf("ACCURACYSNES-BEGIN\n"); | ||
| printf("magic %c%c%c%c\n", wram[RESULTS], wram[RESULTS + 1], wram[RESULTS + 2], wram[RESULTS + 3]); | ||
| printf("done %02x\n", wram[RESULTS + 0x08]); | ||
| printf("count %u\n", count); | ||
| printf("passed %u\n", rd16(0x0A)); | ||
| printf("failed %u\n", rd16(0x0C)); | ||
| printf("skipped %u\n", rd16(0x0E)); | ||
| printf("golden %u\n", rd16(0x10)); | ||
|
Comment on lines
156
to
+171
There was a problem hiding this comment. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- target outline ---'
ast-grep outline scripts/accuracysnes/ares_host/ares_host.cpp
printf '%s\n' '--- target section ---'
cat -n scripts/accuracysnes/ares_host/ares_host.cpp | sed -n '120,205p'
printf '%s\n' '--- runtime definitions and uses ---'
rg -n -C 4 'R_(COUNT|PASSED|FAILED|SKIPPED|GOLDEN|DONE)|RESULTS|ACSN|ACCURACYSNES-BEGIN|i < 512|512' scripts/accuracysnesRepository: doublegate/RustySNES Length of output: 18375 🏁 Script executed: #!/bin/bash
set -eu
find scripts/accuracysnes -type f -maxdepth 4 -print | sort
printf '%s\n' '--- relevant symbols ---'
rg -n -C 5 'R_(COUNT|PASSED|FAILED|SKIPPED|GOLDEN|DONE)|RESULTS|ACSN|ACCURACYSNES-BEGIN|i < 512|512' scripts/accuracysnesRepository: doublegate/RustySNES Length of output: 17761 🏁 Script executed: rg -n -C 8 'ACCURACYSNES-BEGIN|R_DONE|R_COUNT|R_PASSED|R_FAILED|R_SKIPPED|R_GOLDEN|RESULTS' .Repository: doublegate/RustySNES Length of output: 50378 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- shared results layout ---'
sed -n '7,45p' tests/roms/AccuracySNES/gen/src/emit.rs
printf '%s\n' '--- runtime layout symbols ---'
rg -n -C 3 'R_MAGIC|R_VERSION|R_COUNT|R_DONE|R_PASSED|R_FAILED|R_SKIPPED|R_GOLDEN|R_STATUS|R_DONE_MARK|R_FORMAT_VERSION|_test_count' tests/roms/AccuracySNES/asm
printf '%s\n' '--- Ares host entry and return behavior ---'
cat -n scripts/accuracysnes/ares_host/ares_host.cpp | sed -n '85,180p'Repository: doublegate/RustySNES Length of output: 26621 Reject incomplete or malformed result blocks before reporting counts. The host emits 🧰 Tools🪛 Clang (14.0.6)[warning] 158-158: variable 'RESULTS' is not initialized (cppcoreguidelines-init-variables) [warning] 163-163: variable 'count' is not initialized (cppcoreguidelines-init-variables) [warning] 164-164: do not call c-style vararg functions (cppcoreguidelines-pro-type-vararg) 🤖 Prompt for AI AgentsSource: Path instructions |
||
| for(u32 i = 0; i < count && i < 512; i++) { | ||
| printf("status %u %02x\n", i, wram[RESULTS + 0x20 + i]); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language identifiers to both changed Markdown fences.
Both fences omit a language identifier and can fail the active MD040 check.
scripts/accuracysnes/ares_host/README.md#L5-L18: change the opening fence to```textor```console.CHANGELOG.md#L19-L21: change the opening fence to```textor```console.As per path instructions, these Markdown files must follow the pinned markdownlint rules.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 5-5: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
📍 Affects 2 files
scripts/accuracysnes/ares_host/README.md#L5-L18(this comment)CHANGELOG.md#L19-L21🤖 Prompt for AI Agents
Sources: Path instructions, Linters/SAST tools