[d-torrance]

Written by Claude (Claude Opus 5.5, via Claude Code), not by @d-torrance, whose account posted it -- please weigh it accordingly.

Fixes in the code behind the default F4 reducer: F4MatrixBuilder2, and the MonomialMap that the F4 matrix builders use for their columns.

A task comparator that runs std::sort off its vector (5734656)

F4MatrixBuilder2::initializeRowsToReduce sorts its row tasks so that plain rows come first and S-pairs follow in order of lead monomial. When a was an S-pair and b a plain row, the comparator compared their leads instead of returning false, so cmp(a, b) and cmp(b, a) could both be true. That is not a strict weak ordering, and std::sort (which oneTBB's parallel_sort uses below 500 elements, and which the non-TBB build always uses) relies on one: its unguarded insertion step walks off the front of the vector.

No caller has reached it, because F4Reducer fills a builder with only S-pairs or only plain rows, and the comparator was wrong only between the two kinds. But both add methods are public.

The new test, F4MatrixBuilder2.SPairsAndPlainRowsTogether, adds four tasks in an order that crashes the old comparator.

	Debug	Release
without the fix	segfault in std::__insertion_sort	segfault
with the fix	passes	passes

It is the first test to drive F4MatrixBuilder2 directly; until now it was only reached through whole Gröbner basis computations. It runs on one thread, because which half of an S-pair becomes the pivot row depends on which row reaches that column first, so with more threads there are several equally correct matrices.

A data race when MonomialMap grows (4333d4c)

When the map grows, the bigger FixedSizeMonomialMap takes over the old table's nodes and relinks each node's mNext into its new bucket. Those stores were relaxed, on the grounds that the constructor does not run concurrently. But a Reader made before the growth keeps its reference to the old table and can be partway along a chain when it is relinked. Its consume load can then follow the relaxed store to a node inserted after the Reader was made, with nothing ordering that node's construction before the read. That is a data race by the C++ memory model, on the node's mNext and its monomial. It is benign on x86 and very probably in practice elsewhere too, because inserts happen under a mutex.

The relinking stores are now release stores, which the reader's consume load pairs with. The bucket stores stay relaxed, since the new table is published afterwards by a seq_cst store. The loop runs only when the table grows.

ThreadSanitizer found it once oneTBB itself was built with -fsanitize=thread (the packaged oneTBB produces thousands of false reports), in GB.liu_0_1, GB.weispfennig97_0_5 and GB.gerdt93_0_1. A worker in findProduct or findTwoProducts read a node that F4MatrixBuilder::createColumn had just inserted.

TSan, instrumented oneTBB 2021.11	reports
GB.* repeated 10 times, before	29
GB.* repeated 10 times, after	0
whole suite repeated 15 times, after	0

There is no test that fails without this fix: only TSan sees the race, and CI does not run it.

Checked

• 256 tests pass in Debug and Release cmake builds with GCC 13.3 and oneTBB 2021.11.
• The -reducer 26 (default F4) output on cyclic5 and hyclic8 is byte-identical to -reducer 25.

Why this is a draft

Two fixes are in; three more for the same code are still to come, each as its own commit:

• A Debug assert that fails on the default F4 path. mgb gb cyclic7 -reducer 26 -threadCount 1 aborts in a Debug build at F4MatrixBuilder2.cpp:133, and Macaulay2's Debug builds take this path. The assert, and the one at :207, check a guarantee that the comment above them credits to S-pair elimination: that all S-pairs sharing an lcm share a generator. It does not hold. On cyclic7 the lcm bcef4g5 gets (276,145) (335,180) (335,216), where 276 and 335 are distinct generators whose own lcm bcef4g4 strictly divides bcef4g5. Release handles the leftover pair correctly as an ordinary two-sided row, so the only cost is a skipped optimisation, and a small one: 3 of 2224 S-pairs on cyclic7, and none on cyclic5, hyclic8 or yang1. The fix will remove the asserts and correct the comment.
• A leak in SparseMatrix::reserveFreeEntries. When a row being built outgrows a block that holds no finished rows, the old block is deleted without freeing the chain of earlier blocks it points to, or its own index and scalar arrays. SparseMatrix.RowsSpanningBlocks (Test rows that outgrow their SparseMatrix block while being built #97) reaches it, so the suite is not ASan-clean.
• A null pointer passed to fwrite for an empty row in SparseMatrix::write, which UBSan reports under -storeMatrices.

🤖 Generated with Claude Code