Test that a NaN pivot is reported by the packed symmetric-indefinite factorizations - #1393
Open
rmlarsen wants to merge 1 commit into
Open
Test that a NaN pivot is reported by the packed symmetric-indefinite factorizations#1393rmlarsen wants to merge 1 commit into
rmlarsen wants to merge 1 commit into
Conversation
…factorizations xSPTRF and xHPTRF rejected a column that cannot be pivoted on with IF( MAX( ABSAKK, COLMAX ).EQ.ZERO ) THEN alone until f96546f added the DISNAN( ABSAKK ) term their dense twins xSYTF2 and xHETF2 have carried since 2006. Nothing in the test suite covered it: no matrix type contains a NaN, so the routines were only ever asked about a pivot MAX can classify. Add that question to xCHKSP and xCHKHP, after the matrix-type loops. For each N in NVAL and each UPLO, factor a matrix whose only NaN sits on the diagonal of the last column the loop visits -- column N for UPLO = 'L', column 1 for UPLO = 'U'. That column has no off-diagonal candidate, so COLMAX is zero and IMAX is never assigned; unless the pivot itself is tested for a NaN, every later comparison is false, the code reaches the 2-by-2 branch, and it writes IPIV( K+1 ) or IPIV( K-1 ), one element past an end of IPIV, with INFO left at zero. The NaN column is otherwise zero and the rest of the matrix is I + ones, so it is factored without an interchange and INFO must come back as the index of the NaN column. IPIV is passed as IWORK( 2 ) so that the write below its first element lands in IWORK( 1 ), where a sentinel catches it; a second sentinel in IWORK( N+2 ) catches the write above its last. With the six routines restored to their state before f96546f, every one of the six paths dies of SIGSEGV on the first case (N = 1, where the uninitialized IMAX is dereferenced immediately). Running the cases one process each, gfortran 13.3 at -O2 returns INFO = 0 instead of the column index for all ten cases with N >= 2, and overwrites IPIV( 0 ) with -1 for UPLO = 'U' and IPIV( N+1 ) with -N for UPLO = 'L'; both checks fire. With the routines as they are, INFO is the column index and both sentinels survive in all 24 cases of each path. The full test suite passes: 5442045 LAPACK and 315872 BLAS tests, 0 errors, 215 of 215 ctest cases, against 5441901 LAPACK tests on the parent -- the 144 new tests are 24 in each of SSP, DSP, CSP, ZSP, CHP and ZHP. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
❌ 2 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 77 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
Contributor
Author
|
Verified on an Apple M4 (macOS, Homebrew gfortran 16.2, Release build with the CI flags). With this branch merged onto current master, the full test suite passes. With the six factorizations restored to their state before #1378, the new tests fail in all six paths: the double and complex test binaries crash, and the single-precision one reports the wrong INFO and the write outside IPIV. |
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.
Adds 144 tests — 24 in each of the
SSP,DSP,CSP,ZSP,CHPandZHPpaths — for the NaN pivot guard merged in #1378 (f96546f). With the six factorizations restored to their state before that commit, every one of those six paths now dies ofSIGSEGV, so what the suite previously could not see at all is a hard failure.xSPTRFandxHPTRFrejected a column that cannot be pivoted on withalone until #1378 added the
DISNAN( ABSAKK )term that their dense twinsxSYTF2andxHETF2have carried since 2006. Nothing in the test suite covered the difference: no matrix type contains a NaN, so the routines were only ever asked about a pivot thatMAXcan classify.What the test does
After the matrix-type loops of
xCHKSPandxCHKHP, for eachNinNVALand eachUPLO, factor a matrix whose only NaN sits on the diagonal of the last column the pivot loop visits — columnNforUPLO = 'L', column 1 forUPLO = 'U'. That column has no off-diagonal candidate, soCOLMAXis zero andIMAXis never assigned; unless the pivot itself is tested for a NaN, every later comparison is false, the code reaches the 2-by-2 branch, and it writesIPIV( K+1 )orIPIV( K-1 ), one element past an end ofIPIV, withINFOleft at zero.The NaN column is otherwise zero and the rest of the matrix is$I + \mathbf{1}\mathbf{1}^T$ , which is positive definite, so the leading part is factored without an interchange and
INFOmust come back as the index of the NaN column. Two things are checked per case:INFOequals that index;IPIVwas written only inside its own bounds.IPIVis passed asIWORK( 2 ), so the write below its first element lands inIWORK( 1 ), where a sentinel catches it; a second sentinel inIWORK( N+2 )catches the write above its last.Evidence
With
{s,d,c,z}sptrf.fand{c,z}hptrf.frestored to f96546f^ and everything else at master, each of the six paths crashes on its first case,N = 1, where the uninitializedIMAXis dereferenced immediately. Running the cases one process each shows what the remaining ones do — gfortran 13.3,-O2:DSPTRF, before and after #1378, one process per case-9999is the sentinel;IPIV( 0 ) = -1forUPLO = 'U'andIPIV( N+1 ) = -NforUPLO = 'L'are the out-of-bounds writes. Both of the new checks fire in all ten non-crashing cases.Whether
MAX( ABSAKK, COLMAX ).EQ.ZEROcatches a NaN is compiler- and optimization-dependent, so a build on which the old code happens to classify the pivot correctly would see this test pass. That is the same dependence #1378 removed; the test asserts the behavior the guard now makes unconditional.xCHKSPandxCHKHPwere added to the NAG-Onopropagatelist beside thexERRCXXfiles, for the same reason: they construct a NaN.Validation
Test-only change; nothing under
SRC/is touched. The full suite passes — 5442045 LAPACK and 315872 BLAS tests, 0 errors, 215 of 215 ctest cases, against 5441901 LAPACK tests on the parent. The 144 new tests are the difference.