Skip to content

fix: avoid u64 overflow in solve_itp for large nmax values - #588

Open
greysquirr3l wants to merge 1 commit into
linebender:mainfrom
greysquirr3l:fix/solve-itp-nmax-overflow
Open

fix: avoid u64 overflow in solve_itp for large nmax values#588
greysquirr3l wants to merge 1 commit into
linebender:mainfrom
greysquirr3l:fix/solve-itp-nmax-overflow

Conversation

@greysquirr3l

Copy link
Copy Markdown

Problem

When epsilon is very small (e.g. 1e-24), nmax in solve_itp_fallible can exceed 63. The current expression:

let mut scaled_epsilon = epsilon * (1u64 << nmax) as f64;

overflows u64 when nmax >= 64:

  • Debug builds: panic with "attempt to shift left with overflow"
  • Release builds: silent wrong result — x86 masks shift amount to 6 bits, so << 64 wraps to << 0, producing scaled_epsilon = epsilon * 1 instead of epsilon * 2^nmax

How nmax gets large

let n1_2 = (((b - a) / epsilon).log2().ceil() - 1.0).max(0.0) as usize;
let nmax = n0 + n1_2;

For b - a = 1.0 and epsilon = 1e-24, log2(1e24) ≈ 79.7, giving nmax ≈ 79.

Fix

Replace the integer shift with f64::exp2():

let mut scaled_epsilon = epsilon * (nmax as f64).exp2();

This is mathematically identical (2^nmax) but uses floating-point arithmetic. f64 handles nmax up to ~1023 before saturating to +inf — well beyond any epsilon that makes physical sense.

Test

Added test_solve_itp_large_nmax_does_not_overflow with epsilon = 1e-24, which forces nmax ≈ 79. This panics on the original code in debug mode and returns the wrong root in release mode.

Copilot AI review requested due to automatic review settings May 19, 2026 13:49
@greysquirr3l

Copy link
Copy Markdown
Author

Ope, investigating CI issue.

@greysquirr3l
greysquirr3l force-pushed the fix/solve-itp-nmax-overflow branch from da6b2e7 to c64fec2 Compare May 19, 2026 13:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes an overflow bug in the ITP root solver when epsilon is extremely small and the derived nmax exceeds 63, preventing debug-mode panics and release-mode misbehavior.

Changes:

  • Replaces a u64 left-shift (1u64 << nmax) with a floating-point exp2 computation when scaling epsilon.
  • Adds a regression test that exercises a case where nmax becomes large (e.g. epsilon = 1e-24).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread kurbo/src/common.rs Outdated
let n1_2 = (((b - a) / epsilon).log2().ceil() - 1.0).max(0.0) as usize;
let nmax = n0 + n1_2;
let mut scaled_epsilon = epsilon * (1u64 << nmax) as f64;
let mut scaled_epsilon = epsilon * 2.0_f64.powi(nmax as i32);
Comment thread kurbo/src/common.rs Outdated
Comment on lines +1102 to +1103
let f = |x: f64| x - 0.5;
let x = solve_itp(f, 0.0, 1.0, 1e-24, 0, 0.2, f(0.0), f(1.0));
@greysquirr3l
greysquirr3l force-pushed the fix/solve-itp-nmax-overflow branch 2 times, most recently from 9ecb1d3 to a7c7385 Compare May 19, 2026 13:59
When epsilon is very small (e.g. 1e-24), nmax can exceed 63.
The expression `(1u64 << nmax) as f64` then overflows: debug
builds panic, release builds silently produce the wrong value
because x86 masks the shift amount to 6 bits (so << 64 wraps
to << 0 = 1).

Replace with `(nmax as f64).exp2()` which is mathematically
identical (2^nmax) but uses f64 arithmetic, handling nmax up
to ~1023 before saturating to +inf — well beyond any practical
epsilon.

Add a regression test with epsilon=1e-24 (nmax≈79) that panics
on the original code in debug mode.
@greysquirr3l
greysquirr3l force-pushed the fix/solve-itp-nmax-overflow branch from a7c7385 to 12ea6bb Compare May 19, 2026 14:01
@mlwilkerson

Copy link
Copy Markdown
Contributor

Oh, I think this may be the fix for issue #602 I submitted recently.

@raphlinus

Copy link
Copy Markdown
Contributor

I'm going to dig deeply into this set of problems soon. My immediate question is whether nmax > 63 is ever valid. A back-of-the-envelope analysis suggests that with bisection you're never going to get more than the number of mantissa bits (52 or 53 bits), so I'm thinking of just clamping nmax to that. That may well mean not being able to satisfy the precision request implied by epsilon.

Clearly the existing code is wrong and needs a fix. I just need to find out where these too-small epsilons are coming from and what the best behavior for the solver should be in those cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants