Fix FastReducer.SubMod infinite loop on 32-bit platforms#126261
Open
stephentoub wants to merge 2 commits intodotnet:mainfrom
Open
Fix FastReducer.SubMod infinite loop on 32-bit platforms#126261stephentoub wants to merge 2 commits intodotnet:mainfrom
stephentoub wants to merge 2 commits intodotnet:mainfrom
Conversation
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-numerics |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a 32-bit hang in BigInteger Barrett reduction by removing an unbounded compensation loop in FastReducer.SubMod and restoring the previously-disabled 32-bit regression test to ensure coverage of the affected ModPow path.
Changes:
- Replace
FastReducer.SubMod’sAddSelfcompensation loop with an inline subtraction that tolerates unsigned underflow (wrap-around). - Re-enable
FastReducer_AssertFailure_RegressionTeston 32-bit by removing the[ActiveIssue]skip.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.FastReducer.cs |
Removes potentially unbounded correction loop in Barrett reduction; performs wrap-safe subtraction and then canonicalizes by subtracting modulus as needed. |
src/libraries/System.Runtime.Numerics/tests/BigInteger/modpow.cs |
Re-enables the regression test that previously hung on 32-bit, ensuring the fix is exercised. |
Comments suppressed due to low confidence (1)
src/libraries/System.Runtime.Numerics/tests/BigInteger/modpow.cs:376
- Now that this test runs on 32-bit again, its expected-value computation is potentially very expensive: the body uses an O(exponent) loop (and the input data includes exponent=65537). Consider switching the expected calculation to exponentiation-by-squaring (O(log exponent)) to keep the test runtime predictable on slower 32-bit/low-end machines and reduce the risk of future timeouts.
public static void FastReducer_AssertFailure_RegressionTest(BigInteger value, int exponent, BigInteger modulus)
{
// Regression test exercising the code path that was resulting in
// assertion failures per https://github.com/dotnet/runtime/issues/97780.
// The failures seemingly have no impact on functional correctness,
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.FastReducer.cs
Show resolved
Hide resolved
jkotas
reviewed
Mar 28, 2026
The compensating AddSelf loop added in PR dotnet#125799 hangs when the Barrett reduction's truncated q2 exceeds the truncated value. Each iteration adds modulus (~2^1025) but needs to cover a gap of ~2^1088, requiring ~2^64 iterations on 32-bit. Barrett reduction guarantees the true residual x - q*m is in [0, 2*modulus]. After k-limb truncation, right can appear larger than left, but unsigned subtraction underflow naturally wraps to the correct residual (which fits in k limbs since 2*modulus < b^k). Replace the AddSelf loop with an inline subtraction that tolerates underflow, matching the original (pre-dotnet#125799) algorithm's behavior. Also re-enable the test on 32-bit platforms. Fixes dotnet#126212 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove stale comments about disabled assertions from ModPow test - Rename FastReducer_AssertFailure_RegressionTest to ModPow_BarrettReduction and AssertFailureRegressionTest_InputData to ModPow_BarrettReduction_Data to describe what the test validates rather than why it was written - Remove Act/Assert section comments - Add bounds-check hoist in SubMod inline subtraction to match SubtractSelf pattern and help JIT elide bounds checks Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
8f37f31 to
de7fe34
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
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.
PR #125799 rewrote BigInteger internals to use native-width (
nuint) limbs. ItsFastReducer.SubModadded a compensating loop:This hangs on 32-bit platforms. After truncating both operands to
klimbs, the truncatedright(q̂·m mod b^k) can appear much larger thanleft(x mod b^k). Each loop iteration addsmodulus(~2^1025), but the gap can be ~2^1088, requiring ~2^64 iterations on 32-bit — effectively infinite.Barrett reduction guarantees the true residual
x − q̂·mis in[0, 2·modulus]. After k-limb truncation, unsigned subtraction underflow naturally wraps to the correct residual, which fits in k limbs since2·modulus < b^k. The original (pre-#125799) code relied on this property — it just calledSubtractSelfdirectly (and hit a debug assertion on borrow, which was separately suppressed in #98212).This PR:
AddSelfloop with an inline subtraction that tolerates unsigned underflowFastReducer_AssertFailure_RegressionTeston 32-bit platforms (disabled in Disable FastReducer_AssertFailure_RegressionTest on 32-bit platforms #126216)All 3,031 System.Runtime.Numerics tests pass on x64; the previously-hanging test completes in ~10 seconds.
Fixes #126212