diff options
Diffstat (limited to 'src')
| -rw-r--r-- | src/CLAUDE.md | 105 | ||||
| -rwxr-xr-x | src/command.c | 2 | ||||
| -rwxr-xr-x | src/debug_smoke_test.sh | 82 | ||||
| -rwxr-xr-x | src/dynamic.c | 10 | ||||
| -rwxr-xr-x | src/input.c | 11 | ||||
| -rwxr-xr-x | src/main.c | 10 | ||||
| -rwxr-xr-x | src/search.c | 26 | ||||
| -rwxr-xr-x | src/test.sh | 2 | ||||
| -rwxr-xr-x | src/util.c | 6 |
9 files changed, 232 insertions, 22 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 diff --git a/src/command.c b/src/command.c index 091d6cd..fbdc901 100755 --- a/src/command.c +++ b/src/command.c @@ -2590,7 +2590,7 @@ Return value: if (g_Options.fNoInputThread && (0 == NumberOfPendingInputEvents())) { Trace("Exhausted input in batch mode, exiting...\n"); - exit(-1); + exit(0); } // diff --git a/src/debug_smoke_test.sh b/src/debug_smoke_test.sh new file mode 100755 index 0000000..1d2ba0e --- /dev/null +++ b/src/debug_smoke_test.sh @@ -0,0 +1,82 @@ +#!/bin/sh +# Fast pre-commit smoke test: run a DEBUG=1 build over a random sample of +# positions from the full 881-position tests/ecm.ep_ suite at a shallow +# fixed depth, to catch assertion failures without paying full bench-depth +# DEBUG overhead. Variety (many distinct positions) matters more here than +# depth or suite curation -- each position starts with fresh killer/history +# state and a different piece mix, exercising more distinct code paths per +# second than grinding one position deeper or reusing the same fixed +# curated suite every run. Sampling fresh positions each run (rather than a +# fixed suite) also means repeated runs build up coverage over time instead +# of exercising the exact same 11-90 positions forever. +# +# Usage: ./debug_smoke_test.sh [path-to-DEBUG-binary] [depth] [count] +# Defaults: ./_typhoon, sd 4, 10 positions + +BIN="${1:-./_typhoon}" +DEPTH="${2:-4}" +COUNT="${3:-10}" +SUITE=../tests/ecm.ep_ +LOGDIR=/tmp/typhoon/smoke_test +mkdir -p "$LOGDIR" + +if [ ! -x "$BIN" ]; then + echo "ERROR: $BIN not found or not executable (build with DEBUG=1 first)" + exit 1 +fi + +if [ ! -f "$SUITE" ]; then + echo "ERROR: $SUITE not found (run from src/)" + exit 1 +fi + +# ecm.ep_ is NOT a clean fixed-lines-per-problem file (3517 lines / 881 +# problems is not an integer -- some records vary in length), so sample +# whole setboard..go records, not individual lines, or a random line +# offset shreds problems into garbled fragments (silently: the engine's +# script command just reports "total problems: 0" rather than erroring). +SAMPLE="$LOGDIR/sample.ep_" +awk -v k="$COUNT" ' +BEGIN { srand() } +{ sub(/\r$/, "") } +/^setboard/ { rec = $0; next } +/^go[ \t]*$/ { rec = rec "\n" $0; n++; recs[n] = rec; rec = ""; next } +rec != "" { rec = rec "\n" $0 } +END { + if (k > n) k = n + picked = 0 + while (picked < k) { + r = int(rand() * n) + 1 + if (!(r in used)) { + used[r] = 1 + print recs[r] + picked++ + } + } +}' "$SUITE" > "$SAMPLE" + +STAMP=$(date +%Y%m%d_%H%M%S) +LOG="$LOGDIR/sample_${STAMP}.log" +OUT="$LOGDIR/sample_${STAMP}.out" +SAMPLE_SAVED="$LOGDIR/sample_${STAMP}.ep_" +cp "$SAMPLE" "$SAMPLE_SAVED" + +echo "=== $COUNT random positions from $SUITE (sd $DEPTH) ===" +echo " FENs chosen (reproduce a failure with: script $SAMPLE_SAVED):" +grep '^setboard' "$SAMPLE_SAVED" | sed 's/^setboard / /' + +START=$(date +%s) +"$BIN" --cpus 1 --hash 64m --logfile "$LOG" \ + --batch --command "force; book name /nonexistent.book.bin; sd $DEPTH; script $SAMPLE" \ + > "$OUT" 2>&1 +STATUS=$? +END=$(date +%s) +echo " exit=$STATUS elapsed=$((END - START))s" + +if [ "$STATUS" -ne 0 ]; then + echo " *** ASSERTION OR CRASH DETECTED (exit=$STATUS) -- see $OUT / $LOG ***" + echo " positions tried (saved): $SAMPLE_SAVED" + echo "SMOKE TEST FAILED" + exit 1 +fi +echo "SMOKE TEST PASSED" diff --git a/src/dynamic.c b/src/dynamic.c index 5915721..6a1dae8 100755 --- a/src/dynamic.c +++ b/src/dynamic.c @@ -317,8 +317,16 @@ Return value: { ctx->mvKiller[uPly][1] = ctx->mvKiller[uPly][0]; ctx->mvKiller[uPly][0] = mv; - if (ctx->mvKiller[uPly][1].uMove == 0) + if ((ctx->mvKiller[uPly][1].uMove == 0) && + (!IS_SAME_MOVE(ctx->mvNullmoveQuietRefutations[uPly], mv))) { + // Guard against backfilling slot[1] with the same move + // just placed in slot[0] -- e.g. this ply's recorded + // null-move quiet refutation happens to coincide with + // the move causing the current fail-high. Without this + // check both slots would hold the identical move, + // wasting a killer slot (and, in a DEBUG build, + // tripping the assert below) for no ordering benefit. ctx->mvKiller[uPly][1] = ctx->mvNullmoveQuietRefutations[uPly]; } } diff --git a/src/input.c b/src/input.c index 389b658..6a35ab8 100755 --- a/src/input.c +++ b/src/input.c @@ -29,7 +29,7 @@ Revision History: DLIST_ENTRY g_InputEventList; volatile static ULONG g_uInputLock; -extern CHAR g_szInitialCommand[SMALL_STRING_LEN_CHAR]; +extern CHAR *g_szInitialCommand; #define INPUT_IS_LOCKED (g_uInputLock != 0) #define LOCK_INPUT \ @@ -217,11 +217,12 @@ Return value: _BlockBlockingReaders(); InitializeListHead(&g_InputEventList); g_uInputLock = 0; - if (g_szInitialCommand[0] != '\0') + if (g_szInitialCommand != NULL) { Trace("INPUT SYSTEM INIT: Pushing \"%s\"\n", g_szInitialCommand); PushNewInput(g_szInitialCommand); - g_szInitialCommand[0] = '\0'; + SystemFreeMemory(g_szInitialCommand); + g_szInitialCommand = NULL; } } @@ -246,9 +247,9 @@ Return value: **/ { - if (g_szInitialCommand[0] == '\0') + if (g_szInitialCommand == NULL) { - UtilPanic(INCONSISTENT_STATE, + UtilPanic(INCONSISTENT_STATE, NULL, "Batch mode specified with no initial command", NULL, NULL, __FILE__, __LINE__); @@ -24,7 +24,7 @@ Revision History: FILE *g_pfLogfile = NULL; GAME_OPTIONS g_Options; -CHAR g_szInitialCommand[SMALL_STRING_LEN_CHAR]; +CHAR *g_szInitialCommand = NULL; ULONG g_uInputThreadHandle = (ULONG)-1; void @@ -273,7 +273,7 @@ Return value: // Defaults // memset(&g_Options, 0, sizeof(g_Options)); - g_szInitialCommand[0] = '\0'; + g_szInitialCommand = NULL; g_Options.uMyClock = g_Options.uOpponentsClock = 600; g_Options.fGameIsRated = FALSE; g_Options.fOpponentIsComputer = FALSE; @@ -320,9 +320,9 @@ Return value: #endif if ((!STRCMPI(argv[i], "--command")) && (argc > i)) { - strncpy(g_szInitialCommand, - argv[i+1], - SMALL_STRING_LEN_CHAR - 2); + ULONG uLen = (ULONG)strlen(argv[i+1]); + g_szInitialCommand = (CHAR *)SystemAllocateMemory(uLen + 3); + strcpy(g_szInitialCommand, argv[i+1]); strcat(g_szInitialCommand, "\r\n"); i++; } diff --git a/src/search.c b/src/search.c index 61fa428..ab7d122 100755 --- a/src/search.c +++ b/src/search.c @@ -358,7 +358,7 @@ Search(IN SEARCHER_THREAD_CONTEXT *ctx, if (iScore > iBeta) { StoreLowerBound(mvHash, pos, iScore, uDepth, FALSE); } - iBestScore = iScore; // TODO: try just beta here + iBestScore = iScore; goto end; } } @@ -516,8 +516,8 @@ Search(IN SEARCHER_THREAD_CONTEXT *ctx, (ctx->uPly >= 2) && (ctx->sPlyInfo[ctx->uPly - 2].iExtensionAmount <= 0)) { - if ((uDepth > THREE_QUARTERS_PLY) && - (uDepth <= ONE_PLY + THREE_QUARTERS_PLY) && + ASSERT(uDepth >= THREE_QUARTERS_PLY); + if ((uDepth <= ONE_PLY + THREE_QUARTERS_PLY) && (iRoughEval + VALUE_KNIGHT <= iAlpha)) { uFutilityMargin = (iAlpha - iRoughEval) / 2; @@ -762,9 +762,9 @@ Search(IN SEARCHER_THREAD_CONTEXT *ctx, (uFutilityMargin) && (TRUE == fIsLeftoverMove) && (iExtend <= 0) && - (!IS_ESCAPING_CHECK(mv)) && + (!IS_ESCAPING_CHECK(mv)) && (!IS_CAPTURE_OR_PROMOTION(mv)) && - (!IS_CHECKING_MOVE(mv)) && + (!IS_CHECKING_MOVE(mv)) && (!fThreat) && (ComputeMoveScore(ctx, mv, (x - 1)) < uFutilityMargin)) { @@ -843,7 +843,7 @@ Search(IN SEARCHER_THREAD_CONTEXT *ctx, iBestScore = iScore; mvBest = mv; pi->mvBest = mv; - + if (iScore > iAlpha) { if (iScore >= iBeta) @@ -919,6 +919,11 @@ Search(IN SEARCHER_THREAD_CONTEXT *ctx, ASSERT(IS_CHECKING_MOVE(mvLast)); ASSERT(InCheck(pos, pos->uToMove)); iBestScore = MATED_SCORE(ctx->uPly); + if ((iAlpha < iBestScore) && (iBestScore < iBeta)) + { + INC(ctx->sCounters.tree.u64LeafCount); + UpdatePV(ctx, MATEMOVE); + } ASSERT(iBestScore <= -NMATE); goto end; } @@ -927,6 +932,7 @@ Search(IN SEARCHER_THREAD_CONTEXT *ctx, iBestScore = 0; if ((iAlpha < iBestScore) && (iBestScore < iBeta)) { + INC(ctx->sCounters.tree.u64LeafCount); UpdatePV(ctx, DRAWMOVE); } goto end; @@ -1460,7 +1466,6 @@ QSearch(IN SEARCHER_THREAD_CONTEXT *ctx, // If we see a move whose value is zero, the rest of the moves // in this ply can be tossed. ASSERT(SanityCheckMoves(ctx, x, VERIFY_BEFORE | VERIFY_AFTER)); - ASSERT(iBestScore > -NMATE); goto end; } mv = ctx->sMoveStack.mvf[x].mv; @@ -1522,7 +1527,6 @@ QSearch(IN SEARCHER_THREAD_CONTEXT *ctx, mv.cTo); } KEEP_TRACK_OF_FIRST_MOVE_FHs(uLegalMoves == 1); - ASSERT(iBestScore > -NMATE); ASSERT(SanityCheckMoves(ctx, x, VERIFY_BEFORE)); goto end; } @@ -1546,7 +1550,6 @@ QSearch(IN SEARCHER_THREAD_CONTEXT *ctx, } } } - ASSERT(iBestScore > -NMATE); ASSERT(SanityCheckMoves(ctx, x, VERIFY_BEFORE)); end: @@ -1554,5 +1557,10 @@ QSearch(IN SEARCHER_THREAD_CONTEXT *ctx, ASSERT(PositionsAreEquivalent(pos, &pi->sPosition)); ASSERT(IS_VALID_SCORE(iBestScore) || WE_SHOULD_STOP_SEARCHING); DTLeaveNode(ctx, TRUE, iBestScore, pi->mvBest); + + // Note: iBestScore can be +INFINITY or -INFINITY here even in the + // absence of a legitimate mate detected if we disallowed stand + // pat due to perceived danger early on, when the a..b window had + // an extreme bound. This is "legitimate" but weird. return(iBestScore); } diff --git a/src/test.sh b/src/test.sh index 2109112..cc0f620 100755 --- a/src/test.sh +++ b/src/test.sh @@ -1,6 +1,6 @@ #!/bin/sh -opts='--cpus 1 --hash 256m --egtbpath /egtb/three;/egtb/four;/egtb/five' +opts='--cpus 1 --hash 256m --egtbpath /zscratch/egtb' if [ $# -ne 2 ]; then echo "Usage: ${0} <required-test-name> <time>" @@ -1104,6 +1104,12 @@ Return value: ASSERT(ctx->sPlyInfo[v].PV[u+1].uMove == 0); break; } + else if (mv.uMove == MATEMOVE.uMove) + { + strncat(buf, "<#>", ARRAY_LENGTH(buf) - strlen(buf) - 1); + ASSERT(ctx->sPlyInfo[v].PV[u+1].uMove == 0); + break; + } else { strncat(buf, MoveToSan(mv, &ctx->sPosition), |