[LinearAlgebra] CompressedRowSparseMatrix family: Fix problematic cases - #6270
Open
fredroy wants to merge 8 commits into
Open
[LinearAlgebra] CompressedRowSparseMatrix family: Fix problematic cases#6270fredroy wants to merge 8 commits into
fredroy wants to merge 8 commits into
Conversation
fredroy
added
pr: fix
block() short-circuits on "is j the first or the last column of this row?" without first checking that the row range is non-empty. A registered row may hold no block at all -- fullRows() and fullDiagonal() both create such rows -- and then colsIndex[rowRange.first] reads the next row's first entry while colsIndex[rowRange.second - 1] reads the previous row's last one, which is index -1 for the first row. Reading any empty row therefore returned another row's value, or read out of bounds. On a 6x6 matrix holding only (4,4) = 7, block(0,4) returned 7 and block(0,0) returned uninitialised memory. The same row lookup and column lookup are open-coded at a dozen sites, so extract them here, in two variants: - searchRow() / searchColInRange() are the interpolated binary search on its own, with the guards the copies were missing -- the nBlockRow == 0 and nBlockCol == 0 division guards, and sortedFind()'s own "empty range means not found" behaviour; - findRow() / findColInRange() add the first/last-row and first/last-column checks on top, and are the ones block() uses. Keeping the two apart matters for performance: those end checks pay for themselves when the queried entry is usually an end one, as it is here, but on the insertion paths they almost never hit and cost two extra loads and branches per call. Later commits pick whichever variant matches the call site. Fixes CompressedRowSparseMatrixGeneric.BlockOnEmptyRowAfterFullRows, CompressedRowSparseMatrixGeneric.BlockOnEmptyRowDoesNotBorrowNeighbourValue and CompressedRowSparseMatrixMechanical.ElementAfterFullRows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
clearRowBlock() called rowIndex.back() and rowIndex.front() as fast paths before checking that rowIndex was non-empty, so clearing a row of a matrix with no registered row dereferenced past a null pointer and segfaulted. findRow() already reports an empty matrix as "row not found", so use it here instead of the open-coded lookup. Fixes CompressedRowSparseMatrixGenericDeathTest.ClearRowBlockOnEmptyMatrix, CompressedRowSparseMatrixGenericDeathTest.ClearRowBlockOnDefaultConstructedMatrix and CompressedRowSparseMatrixConstraintDeathTest.ClearRowBlockOnEmptyMatrix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 14d101ebcb2fa13c352e2ec298e54c8137c0b40b)
clearRowColBlock() computed foundRowId but never acted on it. When the row lookup failed, rowId kept whatever value the failed binary search left behind, and that value was then used to build rowRange -- reading out of bounds -- and passed straight to deleteRow(). On a matrix where row i is absent but column i exists, this dropped a completely unrelated row: clearing row/column 0 of a matrix holding (1,0), (1,1) and (2,2) deleted row 1, losing (1,1). An absent row is normal in a sparse matrix and column i may still hold blocks in other rows, so it is not an error either. Reserve the diagnostic for an index that is genuinely outside the matrix, delete the row only when findRow() locates it, and always clear the column. clearColBlock() carried its own copy of the unguarded column lookup, so route it through findColInRange() as well since clearRowColBlock() delegates to it. Fixes CompressedRowSparseMatrixGeneric.ClearRowColBlockWithAbsentRow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 3275851fb0e1c693bb1f4828221867d26aa44ece)
Two defects in the symmetric half of clearRowCol(). First, having read the block column j out of colsIndex, it built the symmetric block's range as rowBegin[j] .. rowBegin[j + 1]. rowBegin is indexed by the position of a row inside rowIndex, not by the row number; the two coincide only when every row is present. On a sparse matrix this searched the wrong row -- reading out of bounds when j exceeded the number of registered rows -- and so failed to clear entries that are in column i. Clearing row/column 2 of an 8x8 matrix holding rows 2..5 searched row 5's range for the symmetric block and left (3,2) untouched. Second, when the symmetric block (j,i) was not found, the local pointer was never reset, so the second loop zeroed a column of block (i,j) instead -- entries that lie in neither row i nor column i. Only observable for NL > 1; with 3x3 blocks, clearing row/column 0 wiped (1,3) and (2,3). Look row j up with searchRow() before taking its range, locate the symmetric block with searchColInRange(), and skip the second clear entirely when it does not exist. Handle the diagonal block explicitly, since it is its own symmetric counterpart. This also drops an unguarded division by nBlockRow. The lean lookup variants are the right ones here: this code walks a row's entries and the queried row is not usually the first or last registered one. Correctness still costs something -- finding the symmetric block is now a real row lookup per entry, where the buggy version used a wrong index and found nothing. Measured at about 75 ns per clearRowCol() call on a 5000-block-row matrix, and it is called once per constrained scalar DOF per step. Fixes CompressedRowSparseMatrixMechanical.ClearRowColMissesSymmetricBlockWhenRowsAreSparse and CompressedRowSparseMatrixMechanical.ClearRowColAsymmetricPatternMat3. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
filterValues() only pushed a destination row when that row had produced at least one value, so the keepEmptyRows flag threaded through all eight copy*() wrappers did nothing. The compensating pop_back() meant to undo the push could never fire either: rows are only pushed once oldVid != vid, so rowBegin.back() is always strictly less than vid at that point. Push the row when the caller asked to keep empty ones, and drop the dead pop_back(). The flag concerns destination rows emptied by the filter itself, not source rows that were never registered: filterValues() only iterates srcMatrix.rowIndex. No caller in the tree passes keepEmptyRows; every call uses the default false. Fixes CompressedRowSparseMatrixMechanical.CopyNonZerosKeepEmptyRows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RowConstIterator's defaulted default constructor left m_internal and m_matrix without initialisers. A default-constructed iterator therefore held an indeterminate row position and a dangling matrix pointer, and its own isInvalid() reported false -- so it read as a valid iterator onto row 0 of an unspecified matrix. Give both members a default member initialiser so the default-constructed state is the invalid one the class already knows how to describe. Fixes CompressedRowSparseMatrixConstraint.DefaultConstructedRowIteratorIsInvalid. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…arded helpers The "find row i" and "find column j inside a row" idioms were open-coded at a dozen more sites. Each copy carried the same two hazards the previous commits fixed at individual call sites: an unguarded division by nBlockRow / nBlockCol, undefined on a default-constructed matrix, and the first/last-column fast paths dereferencing colsIndex without checking the row range for emptiness. The division silently yields 0 on ARM, which is why the accompanying guard tests pass there, but integer division by zero raises SIGFPE on x86_64. Confirmed with UBSan at Mechanical.h:343 (clearRow), :748 (blockGet) and :841 (bRowBegin) before this change, clean after. Converted: clearRow, blockGet, blockGetW, blockCreate, bRowBegin, bRowEnd and bRowRange in the mechanical matrix; both wblock() overloads -- including the hinted one, whose divisions had no guard at all -- and getMaxColIndex in the generic one. The ordered-insertion append path now treats an empty range on the last row as "append here" rather than reading colsIndex[-1]. All of these use the lean searchRow() / searchColInRange() variants, matching what they did before the series: none of them had the first/last-entry checks, and adding them here would slow assembly down measurably. wblock() in particular was already safe -- it guarded both divisions, and sortedFind() reports an empty range as not found -- so it gains nothing from those checks and would only pay for them. Measured on a 5000-block-row FEM-shaped matrix, giving wblock() the end checks cost 28% on add(); with the lean variants it is within noise of 503ce3a, while element() misses improve 19% and block() 9%. No behaviour change on well-formed input; this is the undefined behaviour left over once the six defects with failing tests were fixed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fredroy
force-pushed
the
fix_crs_problematic_unittests
branch
from
August 28, 2026 06:07
93ba262 to
78d7ae5
Compare
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.
Based on
Fix the several issues highlighted in the previous PR,
7ed8a64be4block()returned a neighbouring row's value, or readcolsIndex[-1], because its first/last-column fast paths never checked the row range for emptiness; extracts the row and column lookups into guarded helpers, in a fast-path and a lean variant7bcac9b50fclearRowBlock()segfaulted on a matrix with no registered row, callingrowIndex.back()before testing for emptiness70ad9e0981clearRowColBlock()ignored its ownfoundRowIdflag and passed a stale index todeleteRow(), dropping an unrelated rowfc04456781clearRowCol()indexedrowBeginwith a block-column number instead of a row position, so it missed entries in the cleared column and zeroed unrelated ones when the symmetric block was absent98c264ed95filterValues()never honouredkeepEmptyRows, making the flag a no-op across all eightcopy*()wrappers0c3925adadRowConstIterator's members had no initialisers, so a default-constructed iterator read as a valid iterator onto row 078d7ae5210nBlockRow/nBlockCol(UB; SIGFPE on x86_64), using the lean variant so assembly is not slowed down--> fc04456 is the one with real production exposure: clearRowCol is called by FixedProjectiveConstraint, AttachProjectiveConstraint, PointProjectiveConstraint and friends every time step.
[with-all-tests]
By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).
Reviewers will merge this pull-request only if