diff options
Diffstat (limited to 'src/CLAUDE.md')
| -rw-r--r-- | src/CLAUDE.md | 105 |
1 files changed, 105 insertions, 0 deletions
diff --git a/src/CLAUDE.md b/src/CLAUDE.md index 52a6877..b545edd 100644 --- a/src/CLAUDE.md +++ b/src/CLAUDE.md @@ -34,6 +34,111 @@ This uses `clang`/`clang++` (already the GNUmakefile default) and produces a touched by many translation units, `gmake clean` first to avoid 32/64-bit object mismatches at link time. +## Pre-commit testing + +Two cheap, complementary gates exist and should both be run before +committing a search/eval/move-generation change (not needed for pure +doc/comment-only changes). Neither replaces the curated-suite/`sd`-`sn` +comparison workflow below -- these catch outright bugs (crashes, broken +invariants) fast; the suite workflow measures whether a change is actually +*better*, which is a slower, separate question. + +- **`gmake TEST=1` self-test suite.** A from-scratch internal unit-test + harness (`test*.c`, gated behind `GNUmakefile`'s `ifdef TEST` block -- + not built by the normal `GENETIC=1 PERF_COUNTERS=1 MP=1 SIXTYFOUR=1` + profile) exercising bitboard ops, SAN/ICS parsing, move generation + (pseudo-legal and legal), FEN translation, `ExposesCheck`/`IsAttacked`, + and a `Search()` smoke exercise. Runs automatically at startup before + dropping to the normal prompt -- `./typhoon --batch --command ""` runs + the self-tests then exits cleanly (`Exhausted input in batch mode`) with + no other input needed, making it scriptable. Most of these files are + original-2004-era and untouched since, but `testeval.c`/`testsearch.c`/ + `testsee.c` have been actively edited by recent sessions -- this is a + live, working harness, not dead code, despite living outside the normal + build profile. +- **`debug_smoke_test.sh [binary] [depth] [count]`** (defaults: `./_typhoon`, + `sd 4`, 10 positions). Builds/runs a `DEBUG=1` binary (asserts compiled + in) against a fresh random sample of positions drawn from the full + 881-problem `tests/ecm.ep_`, not a fixed curated suite -- variety across + many distinct positions catches more distinct code paths per second than + a deep run on one or a few. Prints the chosen FENs and saves a + timestamped, reproducible `.ep_` sample + log under + `/tmp/typhoon/smoke_test/` on *every* run (not just failures), so a + crash's exact position is never lost to a later run overwriting the same + filename (a real bug in the first version of this script). A normal + batch-exhaustion exit is `0` (see the `command.c` fix below) so exit + status alone reliably distinguishes a crash from a clean run -- no need + to grep for "Breakpoint"/"assert" text, though `_assert()` (`util.c`) + does print `"Assertion failure in %s, line %u."` via `Bug()` (which + flushes the logfile every call) before triggering the `int3` breakpoint, + so that text survives in the logfile even if stdout capture is lost to + an abrupt `SIGTRAP` death. +- **`precommit_check.sh`** runs both gates back-to-back and restores a + normal release build afterward (`gmake clean` + the standard + `GENETIC=1 PERF_COUNTERS=1 MP=1 SIXTYFOUR=1` profile), so the working + directory isn't left with a `TEST`/`DEBUG`-flavored binary as a side + effect of running it. ~20s end to end on this box. + +**Two real bugs found and fixed by exercising this session's changes +through the smoke test, both pre-existing and unrelated to the session's +actual move-ordering work:** +1. **A killer-table backfill collision** (`dynamic.c`'s `_NewKillerMove`, + from a previous session's `917bf12`): when slot `[1]` was empty after + the shift, it backfilled unconditionally from + `mvNullmoveQuietRefutations[uPly]` with no check that the backfilled + move differed from the move just placed in slot `[0]` -- if they + coincided, both slots ended up holding the identical move, tripping + `_NewKillerMove`'s own `!IS_SAME_MOVE(mvKiller[uPly][0], mvKiller[uPly][1])` + invariant. Silent (a wasted killer slot, not a crash) in a release + build, since `ASSERT` compiles to nothing there -- only visible once a + `DEBUG` build actually got exercised. Fixed by skipping the backfill + when it would collide. +2. **Two `ASSERT(iBestScore > -NMATE)` calls in `QSearch()`** (`search.c`) + encoded an invariant that isn't actually guaranteed: at a very early + (full-width) root iteration, or after aspiration-window widening + following repeated fail-highs, an ancestor frame's `iAlpha`/`iBeta` can + itself already be more extreme than `-NMATE` (e.g. `-32767`) with no + mate anywhere in the line. Once that's true, a legitimately-computed + fail-low placeholder (`iBestScore = iAlpha`, used to deny a stand-pat + credit when material looks unavoidably in trouble) or a genuine + fail-high (`iScore >= iBeta`) can land in mate-magnitude territory + purely as a window artifact -- there's no value that can simultaneously + satisfy `iBestScore <= iAlpha` (required to preserve fail-low semantics) + and `iBestScore > -NMATE` once `iAlpha` itself is past that threshold, + so the assert was demanding something structurally impossible, not + catching a real bug. Removed both. This exposure (a non-mate score + numerically resembling one) already existed in every release build + before this, since `ASSERT` is a no-op there -- checked `hash.c`'s + storage path specifically and confirmed it already treats any value + `<= -NMATE` as a sound, if loose, upper bound regardless of origin + (clamped to exactly `-NMATE` on store, never marked `HASH_FLAG_EXACT`, + so the `ADJUST_MATE_IN_N` ply-distance readjustment -- which only fires + for `HASH_FLAG_EXACT` entries -- never misapplies to it); a full audit + of every other `NMATE`/`MATED_SCORE` consumer (root.c's mate-in-N + display being the obvious remaining one) hasn't been done. + +Also fixed while chasing these: batch-mode's exit code was `exit(-1)` +(`command.c`, truncates to `255` as an 8-bit status), indistinguishable +from a real crash's nonzero exit -- changed to `exit(0)` so a clean batch +run and a crash are actually distinguishable by exit status, which is what +`debug_smoke_test.sh` relies on. + +**On not making this an automatic git pre-commit hook**: deliberately not +wired up that way. Two concrns pushed toward manual-by-default: (1) each +full gate is a `gmake clean` + rebuild, real time on every commit even for +trivial changes, easy to end up bypassed with `--no-verify` out of +impatience if it's ever in the way of a quick commit; (2) it overwrites +`./typhoon`/`./_typhoon` in the working directory, and this session +directly hit exactly this class of problem earlier (concurrent background +processes racing on the same binary filename) -- an automatic hook firing +underneath an in-progress interactive session or another script's +background run risks clobbering it. Run `./precommit_check.sh` by hand +before committing a search/eval/move-generation change instead. If full +automation is ever wanted, `git config core.hooksPath .githooks` (with a +tracked `.githooks/pre-commit` calling this script) is the standard way to +get a *version-controlled* hook -- `.git/hooks/` itself isn't tracked by +git -- but that's an opt-in setup step, not a default. + ## Running / xboard protocol basics No command-line flags are required for interactive use; the binary drops into |
