Repository navigation
radix60: fix the Fermat-mod AVX/AVX2 carry reading past its subchain tables (SIGSEGV in short/hiacc chain mode) - #369
Conversation
The AVX/AVX2 Fermat-mod carry loop in radix60_main_carry_loop.h runs 1 << nfold subchains (nfold = USE_SHORT_CY_CHAIN) and reads one inc_arr entry per subchain. radix60's nexec_short and nexec_hiacc tables had 3 and 5 entries, so the short and hiacc chain modes read 1 and 3 ints past the end of the array. Whatever was on the stack became the inner-loop count and the root-table offset; when it was nonzero the carry macro walked off the end of the carry arrays and segfaulted (ASan: stack-buffer-overflow on nexec_hiacc). The run hits this whenever the chain length is raised to short or hiacc: by the FFT-length reversion at a checkpoint (e.g. F18 at 15K in worktodo mode, which reverts toward 13K at every checkpoint and switches to hiacc), or by the ROE retry in fermat_mod_square. Split RADIX/4 = 15 into 4 and 8 pieces, as radix224/240/960 already do, in both the single-threaded and the cy60_process_chunk copies. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Update nex partition counts src/radix60_ditN_cy_dif1.c |
The setup and worker now use four nex entries for the short-chain case and eight for HIACC. Each partition sums to 15. The comments state the 2^nfold entry-count and sum requirement. The long and med partitions are unchanged. |
Priority:
Estimated code review effort: 2 (Simple) | ~5 minutes
Change: Bug fix
Suggested labels: bug
Suggested reviewers: tdulcet
Merge Risk: 
f75d4
-
No actionable merge-blocking risk is identified from the supplied change context.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly identifies the radix60 Fermat-mod AVX/AVX2 carry-table bug and the resulting SIGSEGV. It accurately summarizes the main change, although it is somewhat long. Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
@coderabbitai review |
✅ Action performed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
What crashes
An AVX2 build doing Pepin tests can segfault in
cy60_process_chunk, atradix60_main_carry_loop.h:664(SSE2_fermat_carry_norm_errcheck_X4_loacc), as soon as the carry chain length is raised toshortorhiaccon a leading-radix-60 FFT.Reproduction (current main, f05d3f0, avx2, gcc 16.2)
worktodo.txt:
run with
./Mlucas -shift 0 -cpu 0:3. fermat.cfg has 7K28 8 16and 15K60 8 16entries from-f 17 -fft 7/-f 18 -fft 15, plus 6K and 13K entries that lose on timing. On current main worktodo mode needs those 6K/13K entries to get going at all: the default lengths for F17/F18 (6K/13K) have no Fermat-capable radix set, and without an entry for them the run loops on self-tests forever. #174 fixes that part.F18 starts at 15K (
60 8 16), above its 13K default. At the first checkpoint the FFT-length reversion steps back toward 13K (goto SETUP_FFT), picks 15K again, raisesUSE_SHORT_CY_CHAINto hiacc and reloads the savefile in-process. The next carry pass segfaults. Run alone in worktodo, F18 survives all 26 checkpoints on the optimized build. That is only luck: under ASan the same F18-alone run stops at the first hiacc pass with the overflow below.ASan (avx2, -O1):
Root cause
The Fermat-mod AVX/AVX2 carry loop runs
1 << nfoldsubchains, wherenfold = USE_SHORT_CY_CHAIN(0..3). For each one it reads one entry ofinc_arr, which is that subchain's length (the entries must sum to RADIX/4 = 15). radix60's tables wereso
shortreads 1 int pastnexec_short(4 subchains, 3 entries) andhiaccreads 3 pastnexec_hiacc(8 subchains, 5 entries). Whatever is on the stack there becomes both the inner-loop count and the offset into the roots table. When it is 0 the extra subchains do nothing and the residue is even right. Otherwisetm0/tm1run past the carry arrays, and the result depends on the value: a SIGSEGV, a thread-local memcheck abort, or a silently wrong residue. To see which, I gave main's tables an explicit trailing value standing in for that stack word and forced the chain mode on F18 at 15K (60,8,16), 1000 iterations. With 1 in short mode the run finished normally with a wrong Res64 (1511AE8A19230CAC threaded, 580BF0ECE62C9213 single-threaded, against 8193BD41931E9DE8), with no error reported; with 2 or 7, or in hi-acc mode, it aborted on the memcheck at radix60_ditN_cy_dif1.c:1916/1921. The single-threaded copy of the tables (line 232) has the same defect. radix224, radix240 and radix960 are the only other users of this loop, and their tables all have exactly 2^nfold entries. radix60 is the only one whose RADIX/4 is odd, and it fell back to equal pieces with the wrong count.Two paths raise the chain length mid-run: the per-checkpoint FFT-length reversion (#21, which #74 gates), and the roundoff handling in
fermat_mod_square, which steps the chain down one notch on a 0.4375 fractional part before it gives up on the FFT length. #74 and #174 only change how often this code is reached. The ROE-retry path reaches it either way.The fix
Split 15 into 4 and 8 pieces, as the sibling radices do with their odd quotients:
in both copies, plus a comment saying the tables need exactly 2^nfold entries. AVX-512 is untouched: radix60 has no AVX-512 Fermat carry and keeps its
{0}placeholders.Tests
nexec_hiacc. After, no report, and the same residues.-f 18 -fft 15 -iters 1000(60,8,16), with a throwaway env override ofUSE_SHORT_CY_CHAINin a test build. Threaded (4 threads,cy60_process_chunkcopy), before, under ASan: long and medium clean, short and hiacc stack-buffer-overflow. After: all four give Res64 8193BD41931E9DE8, with AvgMaxErr falling from long to hiacc (0.00334 / 0.00215 / 0.00173 / 0.00151), on both the ASan and the -O3 build. Unthreaded ASan build (the line-232 copy): before, short overflowsnexec_shortand hiacc overflowsnexec_hiacc. After, both are clean with the same Res64.-s tt -cpu 0:3(avx2): noRes64 Error. The 1K (0 of 2) and 2K (1 of 2) radix-set lines are the same as on main.-f 18 -fft 15,-f 19 -fft 30,-f 20 -fft 60) at 100/1000/10000 iterations, single-threaded and-cpu 0:3: every radix set passes (60,8,16 at 15K; 60,8,32 and 60,16,16 at 30K; 60,16,32 and 60,32,16 at 60K). The Res64 agrees between radix sets and thread counts: F18 7C6B681485EB86DB / 8193BD41931E9DE8 / 9D6A242467D28700, F19 529E54642A813995 / E24EAE4B153EE86B / 787FABD98DD5FEC0, F20 64629CED6E218018 / A380121F6FD26B2A / 352CB92DABA82A8B. These runs use the default long chain, so they check that nothing else changed. The forced-mode runs above are the ones that exercise the fix.Overlap with other open PRs
No open PR touches these tables or this loop. #74 (reversion gate) and #174 (Fermat-aware default FFT length, no self-test retry loop) change how often the chain length gets raised, so either one hides this particular reproduction. Neither fixes the table. #364 changes radix60's AVX-512 Mersenne HIACC weights, which are separate code. #316 and #325 also edit radix60_ditN_cy_dif1.c, but git merges them with this change cleanly.
🤖 Generated with Claude Code
Summary by CodeRabbit