Make the test suite honest: fix 16 misregistrations + lint + docs#11
Merged
Conversation
The munit suite registered 16 tests whose display-name mapped to a
*different* function, so "140 passed" overstated coverage:
- 4x *swap_stride slots actually ran *swap_two
- 12x idamax/ihamax/iqamax slots ran the isamax functions
The correctly-named functions existed but were never wired in. Repoint
every registration so the function pointer matches the name stem.
Wiring the real test_qswap_stride in surfaced a latent out-of-bounds in
that never-run test: its RY expected-array has 9 elements but the qvec
size argument was 5, so the assertion loop read RY[5..8] past the
allocation. qswap itself is correct (verified by direct probe); fix the
size to 9.
Add tools/check_test_registration.sh and a CI step asserting every
{"/test_X", fn} has name-stem == fn, so this class can't recur.
Docs: rounding is now a per-call rndMode argument, not the removed
softblas_roundingMode global; 'a' is round-to-nearest-ties-away (not
unconditional away-from-zero); note the exit(-1) -> return reversion.
All 140 tests pass; the swap-stride and d/h/q-amax routines are now
genuinely exercised.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
B1 (critical): *swap/csrot declared incX/incY as uint64_t but tested `if (incX < 0)` — always false, so a negative stride wrapped to a huge positive stride and read/wrote out of bounds. Change incX/incY (and the index accumulators) to int64_t, matching the already-correct saxpy family. Test: test_sswap_negstride (X reversed, Y forward). B2 (high): qaxpy canonicalized only QY[0]; non-canonical NaNs in QY[1..] escaped (SoftFloat propagates the quieted payload), breaking determinism for length>1. Canonicalize every written element. Test: test_qaxpy_nan_unify (non-canonical NaN at index 1 -> QUADNAN). B3 (high): FNEG used `1 << (bits-1)` — UB for 32-bit (1<<31), wrong for 64-bit (1<<63 exceeds int), and c128_conj didn't compile (xor on the uint64_t[2] array). Use a uint64_t mask; define c128_conj separately to flip only the high-word sign bit. Verified compiling + correct under -fsanitize=undefined. B4 (medium): i*amax returned a hardcoded 1 for N==1 while being 0-based for N>1. Return 0. Test: test_isamax_one. B5 (low): *nrm2 loop bound `1 + (N-1)*incX` could overflow -> early exit or OOB. Iterate by count (k < N, ix += incX). Covered by the existing nrm2 stride tests. 143/143 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fix five correctness bugs from the audit (B1–B5)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The munit suite registered 16 tests whose display-name mapped to a different function, so "140 passed" overstated real coverage:
*swap_strideslots actually ran*swap_twoidamax/ihamax/iqamaxslots ran theisamaxfunctionsThe correctly-named functions existed but were never wired in. This PR repoints every registration so the function pointer matches the name stem.
What the honest wiring surfaced
Running the real
test_qswap_strideexposed a latent out-of-bounds in that never-run test: itsRYexpected-array has 9 elements but theqvecsize argument was5, so the assertion loop readRY[5..8]past the allocation.qswapitself is correct (verified by direct probe); fixed the size to9. The*swap_strideandd/h/q-amax routines are now genuinely exercised, all green.Guard against recurrence
tools/check_test_registration.sh+ a CI step assert every{"/test_X", fn}has name-stem ==fn. Would have caught all 16.Docs
rndModeargument, not the removedsoftblas_roundingModeglobal.'a'is round-to-nearest-ties-away, not unconditional away-from-zero (matches thesoftfloat_round_near_maxMagmapping).exit(-1)→returnreversion.140/140 pass. First of several PRs working through the audit backlog; this one is the low-risk foundation that makes the suite trustworthy for the behavior fixes to follow.
🤖 Generated with Claude Code