[m-rev.] for review: fix a bug in the string to int and uint overflow checks

[Julien Fischer]

On Fri, 17 Jul 2026 at 22:41, Zoltan Somogyi <zoltan.somogyi at runbox.com> wrote:
>
>
> On Fri, 17 Jul 2026 17:07:47 +1000, Julien Fischer <jfischer at opturion.com> wrote:
> > We can almost certainly do better for the common bases performance-wise (and
> > likely the generic case as well). I will look into doing that separately.
>
> The obvious way to do it would be to check the base being in 2..36 not using
> comparisons, but by using a switch, and having that switch also return
> the precomputed value of maxint/base (for both 32 and 64 bits) as well.

Looking at how such overflow checking is implemented in strtol,
there's a better approach
we can probably use that hoists the division out of the loop and only
performs comparisons
within the loop. (It should avoid the need for a switch entirely.).

> > For example, with 32-bit ints consider the input string "5368709120" (in the
> > base 10 case). At the last step of the conversion loop, we have N0 = 536870912
> > and the final digit '0'. The true value of (10 * 536870912) + 0 is 5368709120,
> > which does not fit in 32 bits. The multiplication wraps around, giving N =
> > 1073741824.  Since 536870912 =< 1073741824, the overflow check succeeds, and
> > the conversion incorrectly returns 1073741824 instead of failing.
>
> Such examples would be MUCH more understandable if you added a comma
> every third decimal place, to allow distinguishing three billion from thirty billion
> without tedious digit counting.

Done.

> > --- a/library/string.m
> > +++ b/library/string.m
> > @@ -6035,10 +6035,10 @@ base_positive_int_accumulator(Base) = Pred :-
> >
> >  accumulate_int(Base, Char, N0, N) :-
>
> I would rename this to either accumulate_positive_int
> or accumulate_non_negative_int.

Renamed to the former, which is more consistent with the surrounding predicates.

> >      char.unsafe_base_digit_to_int(Base, Char, M),
> > -    N = (Base * N0) + M,
> > -    % Fail on overflow.
> > -    % XXX depends on undefined behaviour
> > -    N0 =< N.
> > +    % Fail if Base * N0 + M would exceed max_int.
> > +    % The division is safe since our caller sets Base to be in 2..36.
> > +    N0 =< (max_int - M) `unchecked_quotient` Base,
> > +    N = (Base * N0) + M.
> >
> >  :- func base_negative_int_accumulator(int) = pred(char, int, int).
> >  :- mode base_negative_int_accumulator(in) = out(pred(in, in, out) is semidet)
> > @@ -6067,10 +6067,13 @@ base_negative_int_accumulator(Base) = Pred :-
> >
> >  accumulate_negative_int(Base, Char, N0, N) :-
> >      char.unsafe_base_digit_to_int(Base, Char, M),
> > -    N = (Base * N0) - M,
> > -    % Fail on overflow.
> > -    % XXX depends on undefined behaviour
> > -    N =< N0.
> > +    % Fail if Base * N0 - M would be less than min_int.
> > +    % We must use truncating division in the following check.
> > +    % Flooring division (i.e. div) causes the test to succeed
> > +    % for values of N0 for which the multiplication overflows.
> > +    % The division is safe since our caller sets Base to be in 2..36.
> > +    N0 >= (min_int + M) `unchecked_quotient` Base,
> > +    N = (Base * N0) - M.
>
> What guarantees do our target languages offer about truncating
> vs flooring?

Java provides both, C and C# provide only truncating division.
The Mercury standard library implements flooring division (div)
in Mercury for all of our signed integer types. We only require
that the target languages provide truncating division.

> If you know, it would help to mention them here.

How is what the target languages provide relevant here?

> I did not check the test cases, partly because the absence of commas
> in large numbers makes it too hard :-(
>
> However, the rest of the diff is fine.

Thanks.

Julien.