Skip to content

Commit 5a869e8

Browse files
mthommthom
authored
Merge pull request #2777 from adri326/fix-2772-rnd_i-clipping
Fix invalid casts in is/2
2 parents 28678e7 + 0cde8f9 commit 5a869e8

File tree

6 files changed

+820
-106
lines changed

6 files changed

+820
-106
lines changed

‎src/arithmetic.rs‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -354,36 +354,41 @@ impl<'a> ArithmeticEvaluator<'a> {
354354
}
355355

356356
// integer division rounding function -- 9.1.3.1.
357-
pub(crate) fn rnd_i(n: &'_ Number, arena: &mut Arena) -> Number {
357+
pub(crate) fn rnd_i(n: &'_ Number, arena: &mut Arena) -> Result<Number, EvalError> {
358358
match n {
359359
&Number::Integer(i) => {
360360
let result = (&*i).try_into();
361361
if let Ok(value) = result {
362-
fixnum!(Number, value, arena)
362+
Ok(fixnum!(Number, value, arena))
363363
} else {
364-
*n
364+
Ok(*n)
365365
}
366366
}
367-
Number::Fixnum(_) => *n,
367+
Number::Fixnum(_) => Ok(*n),
368368
&Number::Float(f) => {
369369
let f = f.floor();
370370

371371
const I64_MIN_TO_F: OrderedFloat<f64> = OrderedFloat(i64::MIN as f64);
372372
const I64_MAX_TO_F: OrderedFloat<f64> = OrderedFloat(i64::MAX as f64);
373373

374374
if I64_MIN_TO_F <= f && f <= I64_MAX_TO_F {
375-
fixnum!(Number, f.into_inner() as i64, arena)
375+
Ok(fixnum!(Number, f.into_inner() as i64, arena))
376376
} else {
377-
Number::Integer(arena_alloc!(Integer::from(f.0 as i64), arena))
377+
Ok(Number::Integer(arena_alloc!(
378+
Integer::try_from(classify_float(f.0)?).unwrap_or_else(|_| {
379+
unreachable!();
380+
}),
381+
arena
382+
)))
378383
}
379384
}
380385
Number::Rational(ref r) => {
381-
let (_, floor) = (r.fract(), r.floor());
386+
let floor = r.floor();
382387

383388
if let Ok(value) = (&floor).try_into() {
384-
fixnum!(Number, value, arena)
389+
Ok(fixnum!(Number, value, arena))
385390
} else {
386-
Number::Integer(arena_alloc!(floor, arena))
391+
Ok(Number::Integer(arena_alloc!(floor, arena)))
387392
}
388393
}
389394
}

‎src/machine/arithmetic_ops.rs‎

Lines changed: 120 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -455,13 +455,8 @@ pub(crate) fn max(n1: Number, n2: Number) -> Result<Number, MachineStubGen> {
455455
Ok(Number::Fixnum(n2))
456456
}
457457
}
458-
(Number::Integer(n1), Number::Integer(n2)) => {
459-
if n1 > n2 {
460-
Ok(Number::Integer(n1))
461-
} else {
462-
Ok(Number::Integer(n2))
463-
}
464-
}
458+
(Number::Integer(n1), Number::Integer(n2)) => Ok(Number::Integer(cmp::max(n1, n2))),
459+
(Number::Rational(r1), Number::Rational(r2)) => Ok(Number::Rational(cmp::max(r1, r2))),
465460
(n1, n2) => {
466461
let stub_gen = || {
467462
let max_atom = atom!("max");
@@ -471,7 +466,15 @@ pub(crate) fn max(n1: Number, n2: Number) -> Result<Number, MachineStubGen> {
471466
let f1 = try_numeric_result!(result_f(&n1), stub_gen)?;
472467
let f2 = try_numeric_result!(result_f(&n2), stub_gen)?;
473468

474-
Ok(Number::Float(cmp::max(OrderedFloat(f1), OrderedFloat(f2))))
469+
match OrderedFloat(f1).cmp(&OrderedFloat(f2)) {
470+
cmp::Ordering::Less => Ok(n2),
471+
cmp::Ordering::Equal => {
472+
// Note: n1 and n2 were compared as floats,
473+
// so we return the second argument as a floating point value.
474+
Ok(Number::Float(OrderedFloat(f2)))
475+
}
476+
cmp::Ordering::Greater => Ok(n1),
477+
}
475478
}
476479
}
477480
}
@@ -499,13 +502,8 @@ pub(crate) fn min(n1: Number, n2: Number) -> Result<Number, MachineStubGen> {
499502
Ok(Number::Fixnum(n2))
500503
}
501504
}
502-
(Number::Integer(n1), Number::Integer(n2)) => {
503-
if n1 < n2 {
504-
Ok(Number::Integer(n1))
505-
} else {
506-
Ok(Number::Integer(n2))
507-
}
508-
}
505+
(Number::Integer(n1), Number::Integer(n2)) => Ok(Number::Integer(cmp::min(n1, n2))),
506+
(Number::Rational(r1), Number::Rational(r2)) => Ok(Number::Rational(cmp::min(r1, r2))),
509507
(n1, n2) => {
510508
let stub_gen = || {
511509
let min_atom = atom!("min");
@@ -515,7 +513,15 @@ pub(crate) fn min(n1: Number, n2: Number) -> Result<Number, MachineStubGen> {
515513
let f1 = try_numeric_result!(result_f(&n1), stub_gen)?;
516514
let f2 = try_numeric_result!(result_f(&n2), stub_gen)?;
517515

518-
Ok(Number::Float(cmp::min(OrderedFloat(f1), OrderedFloat(f2))))
516+
match OrderedFloat(f1).cmp(&OrderedFloat(f2)) {
517+
cmp::Ordering::Less => Ok(n1),
518+
cmp::Ordering::Equal => {
519+
// Note: n1 and n2 were compared as floats,
520+
// so we return the first argument as a floating point value.
521+
Ok(Number::Float(OrderedFloat(f1)))
522+
}
523+
cmp::Ordering::Greater => Ok(n2),
524+
}
519525
}
520526
}
521527
}
@@ -623,103 +629,110 @@ pub(crate) fn int_floor_div(
623629
idiv(n1, n2, arena)
624630
}
625631

626-
pub(crate) fn shr(n1: Number, n2: Number, arena: &mut Arena) -> Result<Number, MachineStubGen> {
632+
pub(crate) fn shr(lhs: Number, rhs: Number, arena: &mut Arena) -> Result<Number, MachineStubGen> {
627633
let stub_gen = || {
628634
let shr_atom = atom!(">>");
629635
functor_stub(shr_atom, 2)
630636
};
631637

632-
if n2.is_integer() && n2.is_negative() {
633-
return shl(n1, neg(n2, arena), arena);
638+
if rhs.is_integer() && rhs.is_negative() {
639+
return shl(lhs, neg(rhs, arena), arena);
634640
}
635641

636-
match (n1, n2) {
637-
(Number::Fixnum(n1), Number::Fixnum(n2)) => {
638-
let n1_i = n1.get_num();
639-
let n2_i = n2.get_num();
640-
641-
// FIXME(arithmetic_overflow)
642-
// what should this do for too large n2,
643-
// - logical right shift should probably turn to 0
644-
// - arithmetic right shift should maybe differ for negative numbers
645-
//
646-
// note: negaitve n2 is already handled above
647-
#[allow(arithmetic_overflow)]
648-
if let Ok(n2) = usize::try_from(n2_i) {
649-
Ok(Number::arena_from(n1_i >> n2, arena))
650-
} else {
651-
Ok(Number::arena_from(n1_i >> usize::MAX, arena))
652-
}
653-
}
654-
(Number::Fixnum(n1), Number::Integer(n2)) => {
655-
let n1 = Integer::from(n1.get_num());
656-
657-
let result: Result<usize, _> = (&*n2).try_into();
642+
match lhs {
643+
Number::Fixnum(lhs) => {
644+
let rhs = match rhs {
645+
Number::Fixnum(fix) => fix.get_num().try_into().unwrap_or(u32::MAX),
646+
Number::Integer(int) => (&*int).try_into().unwrap_or(u32::MAX),
647+
other => {
648+
return Err(numerical_type_error(ValidType::Integer, other, stub_gen));
649+
}
650+
};
658651

659-
match result {
660-
Ok(n2) => Ok(Number::arena_from(n1 >> n2, arena)),
661-
Err(_) => Ok(Number::arena_from(n1 >> usize::MAX, arena)),
662-
}
663-
}
664-
(Number::Integer(n1), Number::Fixnum(n2)) => match usize::try_from(n2.get_num()) {
665-
Ok(n2) => Ok(Number::arena_from(Integer::from(&*n1 >> n2), arena)),
666-
_ => Ok(Number::arena_from(Integer::from(&*n1 >> usize::MAX), arena)),
667-
},
668-
(Number::Integer(n1), Number::Integer(n2)) => {
669-
let result: Result<usize, _> = (&*n2).try_into();
652+
let res = lhs.get_num().checked_shr(rhs).unwrap_or(0);
653+
Ok(Number::arena_from(res, arena))
654+
}
655+
Number::Integer(lhs) => {
656+
// Note: bigints require `log(n)` bits of space. If `rhs > usize::MAX`,
657+
// then this clamping only becomes an issue when `lhs < 2 ^ (usize::MAX)`:
658+
// - on 32-bit systems, `lhs` would need to be 512MiB big (1/8th of the addressable memory)
659+
// - on 64-bit systems, `lhs` would need to be 2EiB big (!!!)
660+
let rhs = match rhs {
661+
Number::Fixnum(fix) => fix.get_num().try_into().unwrap_or(usize::MAX),
662+
Number::Integer(int) => (&*int).try_into().unwrap_or(usize::MAX),
663+
other => {
664+
return Err(numerical_type_error(ValidType::Integer, other, stub_gen));
665+
}
666+
};
670667

671-
match result {
672-
Ok(n2) => Ok(Number::arena_from(Integer::from(&*n1 >> n2), arena)),
673-
Err(_) => Ok(Number::arena_from(Integer::from(&*n1 >> usize::MAX), arena)),
674-
}
668+
Ok(Number::arena_from(Integer::from(&*lhs >> rhs), arena))
675669
}
676-
(Number::Integer(_), n2) => Err(numerical_type_error(ValidType::Integer, n2, stub_gen)),
677-
(Number::Fixnum(_), n2) => Err(numerical_type_error(ValidType::Integer, n2, stub_gen)),
678-
(n1, _) => Err(numerical_type_error(ValidType::Integer, n1, stub_gen)),
670+
other => Err(numerical_type_error(ValidType::Integer, other, stub_gen)),
679671
}
680672
}
681673

682-
pub(crate) fn shl(n1: Number, n2: Number, arena: &mut Arena) -> Result<Number, MachineStubGen> {
674+
pub(crate) fn shl(lhs: Number, rhs: Number, arena: &mut Arena) -> Result<Number, MachineStubGen> {
683675
let stub_gen = || {
684676
let shl_atom = atom!("<<");
685677
functor_stub(shl_atom, 2)
686678
};
687679

688-
if n2.is_integer() && n2.is_negative() {
689-
return shr(n1, neg(n2, arena), arena);
680+
if rhs.is_integer() && rhs.is_negative() {
681+
return shr(lhs, neg(rhs, arena), arena);
690682
}
691683

692-
match (n1, n2) {
693-
(Number::Fixnum(n1), Number::Fixnum(n2)) => {
694-
let n1_i = n1.get_num();
695-
let n2_i = n2.get_num();
684+
let rhs = match rhs {
685+
Number::Fixnum(fix) => fix.get_num().try_into().unwrap_or(usize::MAX),
686+
Number::Integer(int) => (&*int).try_into().unwrap_or(usize::MAX),
687+
other => {
688+
return Err(numerical_type_error(ValidType::Integer, other, stub_gen));
689+
}
690+
};
691+
692+
match lhs {
693+
Number::Fixnum(lhs) => {
694+
let lhs = lhs.get_num();
696695

697-
if let Ok(n2) = usize::try_from(n2_i) {
698-
Ok(Number::arena_from(n1_i << n2, arena))
696+
if let Some(res) = checked_signed_shl(lhs, rhs) {
697+
Ok(Number::arena_from(res, arena))
699698
} else {
700-
let n1 = Integer::from(n1_i);
701-
Ok(Number::arena_from(n1 << usize::MAX, arena))
699+
let lhs = Integer::from(lhs);
700+
Ok(Number::arena_from(
701+
Integer::from(lhs << (rhs as usize)),
702+
arena,
703+
))
702704
}
703705
}
704-
(Number::Fixnum(n1), Number::Integer(n2)) => {
705-
let n1 = Integer::from(n1.get_num());
706+
Number::Integer(lhs) => Ok(Number::arena_from(
707+
Integer::from(&*lhs << (rhs as usize)),
708+
arena,
709+
)),
710+
other => Err(numerical_type_error(ValidType::Integer, other, stub_gen)),
711+
}
712+
}
706713

707-
match (&*n2).try_into() as Result<usize, _> {
708-
Ok(n2) => Ok(Number::arena_from(n1 << n2, arena)),
709-
_ => Ok(Number::arena_from(n1 << usize::MAX, arena)),
710-
}
714+
/// Returns `x << shift`, checking for overflow and for values of `shift` that are too big.
715+
#[inline]
716+
fn checked_signed_shl(x: i64, shift: usize) -> Option<i64> {
717+
if shift == 0 {
718+
return Some(x);
719+
}
720+
721+
if x >= 0 {
722+
// Note: for unsigned integers, the condition would usually be spelled
723+
// `shift <= x.leading_zeros()`, but since the MSB for signed integers
724+
// controls the sign, we need to make sure that `shift` is at most
725+
// `x.leading_zeros() - 1`.
726+
if shift < x.leading_zeros() as usize {
727+
Some(x << shift)
728+
} else {
729+
None
711730
}
712-
(Number::Integer(n1), Number::Fixnum(n2)) => match usize::try_from(n2.get_num()) {
713-
Ok(n2) => Ok(Number::arena_from(Integer::from(&*n1 << n2), arena)),
714-
_ => Ok(Number::arena_from(Integer::from(&*n1 << usize::MAX), arena)),
715-
},
716-
(Number::Integer(n1), Number::Integer(n2)) => match (&*n2).try_into() as Result<usize, _> {
717-
Ok(n2) => Ok(Number::arena_from(Integer::from(&*n1 << n2), arena)),
718-
_ => Ok(Number::arena_from(Integer::from(&*n1 << usize::MAX), arena)),
719-
},
720-
(Number::Integer(_), n2) => Err(numerical_type_error(ValidType::Integer, n2, stub_gen)),
721-
(Number::Fixnum(_), n2) => Err(numerical_type_error(ValidType::Integer, n2, stub_gen)),
722-
(n1, _) => Err(numerical_type_error(ValidType::Integer, n1, stub_gen)),
731+
} else {
732+
let y = x.checked_neg()?;
733+
// FIXME: incorrectly rejects `-2 ^ 62 << 1`. This is currently a non-issue,
734+
// since the bitshift is then done as a `Number::Integer`
735+
checked_signed_shl(y, shift).and_then(|res| res.checked_neg())
723736
}
724737
}
725738

@@ -947,8 +960,7 @@ pub(crate) fn gcd(n1: Number, n2: Number, arena: &mut Arena) -> Result<Number, M
947960
Ok(Number::arena_from(Integer::from(n2_clone.gcd(&n1)), arena))
948961
}
949962
(Number::Integer(n1), Number::Integer(n2)) => {
950-
let n2: isize = (&*n2).try_into().unwrap();
951-
let value: Integer = (&*n1).gcd(&Integer::from(n2)).into();
963+
let value: Integer = (&*n1).gcd(&*n2).into();
952964
Ok(Number::arena_from(value, arena))
953965
}
954966
(Number::Float(f), _) | (_, Number::Float(f)) => {
@@ -1044,7 +1056,12 @@ pub(crate) fn sqrt(n1: Number) -> Result<f64, MachineStubGen> {
10441056

10451057
#[inline]
10461058
pub(crate) fn floor(n1: Number, arena: &mut Arena) -> Number {
1047-
rnd_i(&n1, arena)
1059+
rnd_i(&n1, arena).unwrap_or_else(|_err| {
1060+
// FIXME: Currently floor/1 (and several call sites) are infallible,
1061+
// but the failing cases (when `n1` is `Number::Float(NAN)` or `Number::Float(INFINITY)`)
1062+
// are not reachable with standard is/2 operations.
1063+
todo!("Make floor/1 fallible");
1064+
})
10481065
}
10491066

10501067
#[inline]
@@ -1067,16 +1084,22 @@ pub(crate) fn truncate(n: Number, arena: &mut Arena) -> Number {
10671084
}
10681085
}
10691086

1070-
pub(crate) fn round(n: Number, arena: &mut Arena) -> Result<Number, MachineStubGen> {
1071-
let stub_gen = || {
1072-
let is_atom = atom!("is");
1073-
functor_stub(is_atom, 2)
1087+
pub(crate) fn round(num: Number, arena: &mut Arena) -> Result<Number, MachineStubGen> {
1088+
let res = match num {
1089+
Number::Fixnum(_) | Number::Integer(_) => num,
1090+
Number::Rational(rat) => Number::arena_from(rat.round(), arena),
1091+
Number::Float(f) => Number::Float(OrderedFloat((*f).round())),
10741092
};
10751093

1076-
let result = add(n, Number::Float(OrderedFloat(0.5f64)), arena);
1077-
let result = try_numeric_result!(result, stub_gen)?;
1094+
// FIXME: make round/1 return EvalError
1095+
rnd_i(&res, arena).map_err(|err| -> MachineStubGen {
1096+
Box::new(move |machine_st| {
1097+
let eval_error = machine_st.evaluation_error(err);
1098+
let stub = functor_stub(atom!("round"), 1);
10781099

1079-
Ok(floor(result, arena))
1100+
machine_st.error_form(eval_error, stub)
1101+
})
1102+
})
10801103
}
10811104

10821105
pub(crate) fn bitwise_complement(n1: Number, arena: &mut Arena) -> Result<Number, MachineStubGen> {

0 commit comments

Comments
 (0)