From a1ec8ff493879c680ac92952d6c5ea50ea650314 Mon Sep 17 00:00:00 2001 From: ilammy Date: Tue, 24 Mar 2015 01:17:49 +0200 Subject: [PATCH 1/2] Avoid overflow when doing sexp_fx_abs() Naturally, fixed-width integer arithmetics can overflow. Chibi handles it pretty well in general, but one case was missing: it is negation of the minimal negative number that can be represented as a fixnum. That is, sexp_fx_neg() must not be applied to sexp_make_fixnum(SEXP_MIN_FIXNUM) because it overflows and returns an identical fixnum back. sexp_fx_neg() itself seems to be used right in the current code, but sexp_fx_abs()--which is defined in terms of sexp_fx_neg()--could be applied to the forbidden number when used to retrieve an unboxed value via the sexp_unbox_fixnum(sexp_fx_abs(x)) pattern. So I have added a separate macro that safely calculates unboxed absolute value of a fixnum, and replaced sexp_unbox_fixnum(sexp_fx_abs(x)) usages with it. Current implementation uses two-bit tag for fixnums, plus we need one bit for the sign, so fixnums have (machine word - 3) significant bits. Regression tests cover word sizes of 16, 32, 64, and 128 bits (for the sake of past- and future-proofness). sexp_bignum_expt() does not have a regression test because we need to check it with negative exponents like -2^29, so the base must be over at least 2^(2^29) for the differences to be visible. Fun fact: bignum representation of such number takes around 1/32 of the available user- space memory, which makes testing on anything except 32-bit systems unreasonable (4 TB of RAM anyone?) --- bignum.c | 12 ++++---- include/chibi/sexp.h | 2 ++ tests/numeric-tests.scm | 62 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 6 deletions(-) diff --git a/bignum.c b/bignum.c index da24dff4..b20fb7c1 100644 --- a/bignum.c +++ b/bignum.c @@ -29,7 +29,7 @@ sexp sexp_make_bignum (sexp ctx, sexp_uint_t len) { sexp sexp_fixnum_to_bignum (sexp ctx, sexp a) { sexp res = sexp_make_bignum(ctx, 1); if (!sexp_exceptionp(res)) { - sexp_bignum_data(res)[0] = sexp_unbox_fixnum(sexp_fx_abs(a)); + sexp_bignum_data(res)[0] = sexp_unbox_fx_abs(a); sexp_bignum_sign(res) = sexp_fx_sign(a); } return res; @@ -326,9 +326,9 @@ sexp sexp_bignum_add_fixnum (sexp ctx, sexp a, sexp b) { sexp_gc_preserve1(ctx, c); c = sexp_copy_bignum(ctx, NULL, a, 0); if (sexp_bignum_sign(c) == sexp_fx_sign(b)) - c = sexp_bignum_fxadd(ctx, c, sexp_unbox_fixnum(sexp_fx_abs(b))); + c = sexp_bignum_fxadd(ctx, c, sexp_unbox_fx_abs(b)); else - c = sexp_bignum_fxsub(ctx, c, sexp_unbox_fixnum(sexp_fx_abs(b))); + c = sexp_bignum_fxsub(ctx, c, sexp_unbox_fx_abs(b)); sexp_gc_release1(ctx); return c; } @@ -599,7 +599,7 @@ sexp sexp_bignum_remainder (sexp ctx, sexp a, sexp b) { } sexp sexp_bignum_expt (sexp ctx, sexp a, sexp b) { - sexp_sint_t e = sexp_unbox_fixnum(sexp_fx_abs(b)); + sexp_sint_t e = sexp_unbox_fx_abs(b); sexp_gc_var2(res, acc); sexp_gc_preserve2(ctx, res, acc); res = sexp_fixnum_to_bignum(ctx, SEXP_ONE); @@ -1390,7 +1390,7 @@ sexp sexp_mul (sexp ctx, sexp a, sexp b) { r = (a==SEXP_ZERO ? a : sexp_make_flonum(ctx, sexp_fixnum_to_double(a)*sexp_flonum_value(b))); break; case SEXP_NUM_FIX_BIG: - r = sexp_bignum_fxmul(ctx, NULL, b, sexp_unbox_fixnum(sexp_fx_abs(a)), 0); + r = sexp_bignum_fxmul(ctx, NULL, b, sexp_unbox_fx_abs(a), 0); sexp_bignum_sign(r) = sexp_fx_sign(a) * sexp_bignum_sign(b); r = sexp_bignum_normalize(r); break; @@ -1473,7 +1473,7 @@ sexp sexp_div (sexp ctx, sexp a, sexp b) { tmp = sexp_make_ratio(ctx, a, b); r = sexp_ratio_normalize(ctx, tmp, SEXP_FALSE); #else - r = sexp_make_flonum(ctx, sexp_fixnum_to_double(a)/sexp_bignum_to_double(b)); + r = sexp_make_flonum(ctx, sexp_fixnum_to_double(a)/sexp_bignum_to_double(b)); #endif break; case SEXP_NUM_FLO_FIX: diff --git a/include/chibi/sexp.h b/include/chibi/sexp.h index 78cbde36..be094ad6 100755 --- a/include/chibi/sexp.h +++ b/include/chibi/sexp.h @@ -1182,6 +1182,8 @@ SEXP_API sexp sexp_symbol_table[SEXP_SYMBOL_TABLE_SIZE]; #define sexp_fx_neg(a) (sexp_make_fixnum(-(sexp_unbox_fixnum(a)))) #define sexp_fx_abs(a) ((((sexp_sint_t)a) < 0) ? sexp_fx_neg(a) : a) +#define sexp_unbox_fx_abs(a) ((((sexp_sint_t)a) < 0) ? -sexp_unbox_fixnum(a) : sexp_unbox_fixnum(a)) + #define sexp_fp_add(x,a,b) (sexp_make_flonum(x, sexp_flonum_value(a) + sexp_flonum_value(b))) #define sexp_fp_sub(x,a,b) (sexp_make_flonum(x, sexp_flonum_value(a) - sexp_flonum_value(b))) #define sexp_fp_mul(x,a,b) (sexp_make_flonum(x, sexp_flonum_value(a) * sexp_flonum_value(b))) diff --git a/tests/numeric-tests.scm b/tests/numeric-tests.scm index 6fc7c06e..526810b5 100644 --- a/tests/numeric-tests.scm +++ b/tests/numeric-tests.scm @@ -143,6 +143,68 @@ -9223372036854775808)) (sign-combinations M7 (+ 1 (expt 2 64)))) +;; fixnum-bignum boundaries (machine word - 1 bit for sign - 2 bits for tag) + +(test 8191 (- -8191)) +(test 8192 (- -8192)) +(test 8193 (- -8193)) + +(test 536870911 (- -536870911)) +(test 536870912 (- -536870912)) +(test 536870913 (- -536870913)) + +(test 2305843009213693951 (- -2305843009213693951)) +(test 2305843009213693952 (- -2305843009213693952)) +(test 2305843009213693953 (- -2305843009213693953)) + +(test 42535295865117307932921825928971026431 (- -42535295865117307932921825928971026431)) +(test 42535295865117307932921825928971026432 (- -42535295865117307932921825928971026432)) +(test 42535295865117307932921825928971026433 (- -42535295865117307932921825928971026433)) + +(test '((536879104 -536862720 4398046511104 0 8192) + (536862720 -536879104 -4398046511104 0 -8192) + (-536862720 536879104 -4398046511104 0 8192) + (-536879104 536862720 4398046511104 0 -8192)) + (sign-combinations (expt 2 13) (expt 2 29))) + +(test '((536879104 536862720 4398046511104 65536 0) + (-536862720 -536879104 -4398046511104 -65536 0) + (536862720 536879104 -4398046511104 -65536 0) + (-536879104 -536862720 4398046511104 65536 0)) + (sign-combinations (expt 2 29) (expt 2 13))) + +(test '((2305843009750564864 -2305843008676823040 1237940039285380274899124224 0 536870912) + (2305843008676823040 -2305843009750564864 -1237940039285380274899124224 0 -536870912) + (-2305843008676823040 2305843009750564864 -1237940039285380274899124224 0 536870912) + (-2305843009750564864 2305843008676823040 1237940039285380274899124224 0 -536870912)) + (sign-combinations (expt 2 29) (expt 2 61))) + +(test '((2305843009750564864 2305843008676823040 1237940039285380274899124224 4294967296 0) + (-2305843008676823040 -2305843009750564864 -1237940039285380274899124224 -4294967296 0) + (2305843008676823040 2305843009750564864 -1237940039285380274899124224 -4294967296 0) + (-2305843009750564864 -2305843008676823040 1237940039285380274899124224 4294967296 0)) + (sign-combinations (expt 2 61) (expt 2 29))) + +(test '((42535295865117307935227668938184720384 -42535295865117307930615982919757332480 + 98079714615416886934934209737619787751599303819750539264 0 2305843009213693952) + (42535295865117307930615982919757332480 -42535295865117307935227668938184720384 + -98079714615416886934934209737619787751599303819750539264 0 -2305843009213693952) + (-42535295865117307930615982919757332480 42535295865117307935227668938184720384 + -98079714615416886934934209737619787751599303819750539264 0 2305843009213693952) + (-42535295865117307935227668938184720384 42535295865117307930615982919757332480 + 98079714615416886934934209737619787751599303819750539264 0 -2305843009213693952)) + (sign-combinations (expt 2 61) (expt 2 125))) + +(test '((42535295865117307935227668938184720384 42535295865117307930615982919757332480 + 98079714615416886934934209737619787751599303819750539264 18446744073709551616 0) + (-42535295865117307930615982919757332480 -42535295865117307935227668938184720384 + -98079714615416886934934209737619787751599303819750539264 -18446744073709551616 0) + (42535295865117307930615982919757332480 42535295865117307935227668938184720384 + -98079714615416886934934209737619787751599303819750539264 -18446744073709551616 0) + (-42535295865117307935227668938184720384 -42535295865117307930615982919757332480 + 98079714615416886934934209737619787751599303819750539264 18446744073709551616 0)) + (sign-combinations (expt 2 125) (expt 2 61))) + (test #f (< +nan.0 +nan.0)) (test #f (<= +nan.0 +nan.0)) (test #f (= +nan.0 +nan.0)) From 8329ee9fd6a7b007de8d0a381a6b0eaf5965559b Mon Sep 17 00:00:00 2001 From: ilammy Date: Wed, 25 Mar 2015 03:29:12 +0200 Subject: [PATCH 2/2] Do not lose carry bit in addition edge case Previous code was losing the carry bit in 'all ones' case, when adata[i] = bdata[i] = SEXP_UINT_T_MAX, and carry = 1 too. In this case expression (SEXP_UINT_T_MAX - bdata[i] - carry) overflows and yields an incorrect value SEXP_UINT_T_MAX which results into carry being incorrectly set to 0 after addition. We need to avoid the second overflow when calculating the new value of the carry bit. One way to do this is at first check for the overflow in (adata[i] + bdata[i]), and then throw in the (previous) carry bit. I have also given "n" more expressive name and added a comment about the reason why we need that temporary variable. --- bignum.c | 10 ++++++---- tests/numeric-tests.scm | 11 +++++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/bignum.c b/bignum.c index b20fb7c1..63859ddb 100644 --- a/bignum.c +++ b/bignum.c @@ -367,7 +367,7 @@ sexp sexp_bignum_sub_digits (sexp ctx, sexp dst, sexp a, sexp b) { sexp sexp_bignum_add_digits (sexp ctx, sexp dst, sexp a, sexp b) { sexp_uint_t alen=sexp_bignum_hi(a), blen=sexp_bignum_hi(b), - carry=0, i, n, *adata, *bdata, *cdata; + carry=0, i, old_a, p_sum, *adata, *bdata, *cdata; sexp_gc_var1(c); if (alen < blen) return sexp_bignum_add_digits(ctx, dst, b, a); sexp_gc_preserve1(ctx, c); @@ -377,9 +377,11 @@ sexp sexp_bignum_add_digits (sexp ctx, sexp dst, sexp a, sexp b) { bdata = sexp_bignum_data(b); cdata = sexp_bignum_data(c); for (i=0; i (SEXP_UINT_T_MAX - bdata[i] - carry) ? 1 : 0); + old_a = adata[i]; /* adata may alias cdata */ + p_sum = adata[i] + bdata[i]; + cdata[i] = p_sum + carry; + carry = (old_a > (SEXP_UINT_T_MAX - bdata[i]) ? 1 : 0) + + (p_sum > (SEXP_UINT_T_MAX - carry) ? 1 : 0); } for ( ; carry && (i