Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use #[rustc_inherit_overflow_checks] instead of Add::add etc. #81732

Merged
merged 1 commit into from
Feb 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 12 additions & 11 deletions library/core/src/iter/adapters/enumerate.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::iter::adapters::{zip::try_get_unchecked, SourceIter, TrustedRandomAccess};
use crate::iter::{FusedIterator, InPlaceIterable, TrustedLen};
use crate::ops::{Add, AddAssign, Try};
use crate::ops::Try;

/// An iterator that yields the current count and the element during iteration.
///
Expand Down Expand Up @@ -39,11 +39,11 @@ where
///
/// Might panic if the index of the element overflows a `usize`.
#[inline]
#[rustc_inherit_overflow_checks]
fn next(&mut self) -> Option<(usize, <I as Iterator>::Item)> {
let a = self.iter.next()?;
let i = self.count;
// Possible undefined overflow.
AddAssign::add_assign(&mut self.count, 1);
self.count += 1;
Some((i, a))
}

Expand All @@ -53,11 +53,11 @@ where
}

#[inline]
#[rustc_inherit_overflow_checks]
fn nth(&mut self, n: usize) -> Option<(usize, I::Item)> {
let a = self.iter.nth(n)?;
// Possible undefined overflow.
let i = Add::add(self.count, n);
self.count = Add::add(i, 1);
let i = self.count + n;
self.count = i + 1;
Some((i, a))
}

Expand All @@ -78,10 +78,10 @@ where
count: &'a mut usize,
mut fold: impl FnMut(Acc, (usize, T)) -> R + 'a,
) -> impl FnMut(Acc, T) -> R + 'a {
#[rustc_inherit_overflow_checks]
move |acc, item| {
let acc = fold(acc, (*count, item));
// Possible undefined overflow.
AddAssign::add_assign(count, 1);
*count += 1;
acc
}
}
Expand All @@ -99,25 +99,26 @@ where
mut count: usize,
mut fold: impl FnMut(Acc, (usize, T)) -> Acc,
) -> impl FnMut(Acc, T) -> Acc {
#[rustc_inherit_overflow_checks]
move |acc, item| {
let acc = fold(acc, (count, item));
// Possible undefined overflow.
AddAssign::add_assign(&mut count, 1);
count += 1;
acc
}
}

self.iter.fold(init, enumerate(self.count, fold))
}

#[rustc_inherit_overflow_checks]
unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> <Self as Iterator>::Item
where
Self: TrustedRandomAccess,
{
// SAFETY: the caller must uphold the contract for
// `Iterator::__iterator_get_unchecked`.
let value = unsafe { try_get_unchecked(&mut self.iter, idx) };
(Add::add(self.count, idx), value)
(self.count + idx, value)
}
}

Expand Down
8 changes: 5 additions & 3 deletions library/core/src/iter/range.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::char;
use crate::convert::TryFrom;
use crate::mem;
use crate::ops::{self, Add, Sub, Try};
use crate::ops::{self, Try};

use super::{FusedIterator, TrustedLen};

Expand Down Expand Up @@ -201,23 +201,25 @@ macro_rules! step_identical_methods {

#[inline]
#[allow(arithmetic_overflow)]
#[rustc_inherit_overflow_checks]
fn forward(start: Self, n: usize) -> Self {
// In debug builds, trigger a panic on overflow.
// This should optimize completely out in release builds.
if Self::forward_checked(start, n).is_none() {
let _ = Add::add(Self::MAX, 1);
let _ = Self::MAX + 1;
}
// Do wrapping math to allow e.g. `Step::forward(-128i8, 255)`.
start.wrapping_add(n as Self)
}

#[inline]
#[allow(arithmetic_overflow)]
#[rustc_inherit_overflow_checks]
fn backward(start: Self, n: usize) -> Self {
// In debug builds, trigger a panic on overflow.
// This should optimize completely out in release builds.
if Self::backward_checked(start, n).is_none() {
let _ = Sub::sub(Self::MIN, 1);
let _ = Self::MIN - 1;
}
// Do wrapping math to allow e.g. `Step::backward(127i8, 255)`.
start.wrapping_sub(n as Self)
Expand Down
50 changes: 40 additions & 10 deletions library/core/src/iter/traits/accum.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use crate::iter;
use crate::num::Wrapping;
use crate::ops::{Add, Mul};

/// Trait to represent types that can be created by summing up an iterator.
///
Expand Down Expand Up @@ -37,34 +36,49 @@ pub trait Product<A = Self>: Sized {
fn product<I: Iterator<Item = A>>(iter: I) -> Self;
}

// N.B., explicitly use Add and Mul here to inherit overflow checks
macro_rules! integer_sum_product {
(@impls $zero:expr, $one:expr, #[$attr:meta], $($a:ty)*) => ($(
#[$attr]
impl Sum for $a {
fn sum<I: Iterator<Item=Self>>(iter: I) -> Self {
iter.fold($zero, Add::add)
iter.fold(
$zero,
#[rustc_inherit_overflow_checks]
Copy link
Member

Choose a reason for hiding this comment

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

Hm, so I initially thought "this seems wrong, don't we need it on the fn sum too? But it looks like in practice inherit is a bit wrong - this attribute just forces the MIR to contain overflow checks in all cases, and then when we codegen (potentially in a downstream crate) they'll get removed. So I think we're good -- not sure what a better name for the attribute is, but inherit definitely feels a bit off.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the inherit here is about crates rather than functions, in some sense.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, "inherit" refers to the crate it's codegen'd in. So it's more "defer"? Or we can go more explicit really:

#[rustc_use_overflow_checks_settings_from_user_crate]

We could also make the attribute warn/error if building the MIR doesn't end up making a decision based on it, because that's what usually happens if it's applied to the wrong thing (e.g. a function doing iter.fold(...) instead of a closure).

|a, b| a + b,
Comment on lines +46 to +47
Copy link
Member

Choose a reason for hiding this comment

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

Hah, does this work? I half-expected it not to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha, this was your suggestion.. But yeah, it compiles. I think it works too, but I should check if it actually gets tested for this behaviour. :)

Copy link
Member

@eddyb eddyb Feb 5, 2021

Choose a reason for hiding this comment

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

Yeah, I was going off having seen #[inline(never)], IIRC, on closures, but I wasn't sure it would work out of the box.

)
}
}

#[$attr]
impl Product for $a {
fn product<I: Iterator<Item=Self>>(iter: I) -> Self {
iter.fold($one, Mul::mul)
iter.fold(
$one,
#[rustc_inherit_overflow_checks]
|a, b| a * b,
)
}
}

#[$attr]
impl<'a> Sum<&'a $a> for $a {
fn sum<I: Iterator<Item=&'a Self>>(iter: I) -> Self {
iter.fold($zero, Add::add)
iter.fold(
$zero,
#[rustc_inherit_overflow_checks]
|a, b| a + b,
)
}
}

#[$attr]
impl<'a> Product<&'a $a> for $a {
fn product<I: Iterator<Item=&'a Self>>(iter: I) -> Self {
iter.fold($one, Mul::mul)
iter.fold(
$one,
#[rustc_inherit_overflow_checks]
|a, b| a * b,
)
}
}
)*);
Expand All @@ -83,28 +97,44 @@ macro_rules! float_sum_product {
#[stable(feature = "iter_arith_traits", since = "1.12.0")]
impl Sum for $a {
fn sum<I: Iterator<Item=Self>>(iter: I) -> Self {
iter.fold(0.0, Add::add)
iter.fold(
0.0,
#[rustc_inherit_overflow_checks]
|a, b| a + b,
)
}
}

#[stable(feature = "iter_arith_traits", since = "1.12.0")]
impl Product for $a {
fn product<I: Iterator<Item=Self>>(iter: I) -> Self {
iter.fold(1.0, Mul::mul)
iter.fold(
1.0,
#[rustc_inherit_overflow_checks]
|a, b| a * b,
)
}
}

#[stable(feature = "iter_arith_traits", since = "1.12.0")]
impl<'a> Sum<&'a $a> for $a {
fn sum<I: Iterator<Item=&'a Self>>(iter: I) -> Self {
iter.fold(0.0, Add::add)
iter.fold(
0.0,
#[rustc_inherit_overflow_checks]
|a, b| a + b,
)
}
}

#[stable(feature = "iter_arith_traits", since = "1.12.0")]
impl<'a> Product<&'a $a> for $a {
fn product<I: Iterator<Item=&'a Self>>(iter: I) -> Self {
iter.fold(1.0, Mul::mul)
iter.fold(
1.0,
#[rustc_inherit_overflow_checks]
|a, b| a * b,
)
}
}
)*)
Expand Down
22 changes: 8 additions & 14 deletions library/core/src/iter/traits/iterator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// can't split that into multiple files.

use crate::cmp::{self, Ordering};
use crate::ops::{Add, ControlFlow, Try};
use crate::ops::{ControlFlow, Try};

use super::super::TrustedRandomAccess;
use super::super::{Chain, Cloned, Copied, Cycle, Enumerate, Filter, FilterMap, Fuse};
Expand Down Expand Up @@ -242,13 +242,11 @@ pub trait Iterator {
where
Self: Sized,
{
#[inline]
fn add1<T>(count: usize, _: T) -> usize {
// Might overflow.
Add::add(count, 1)
}

self.fold(0, add1)
self.fold(
0,
#[rustc_inherit_overflow_checks]
|count, _| count + 1,
)
}

/// Consumes the iterator, returning the last element.
Expand Down Expand Up @@ -2475,13 +2473,9 @@ pub trait Iterator {
fn check<T>(
mut predicate: impl FnMut(T) -> bool,
) -> impl FnMut(usize, T) -> ControlFlow<usize, usize> {
// The addition might panic on overflow
#[rustc_inherit_overflow_checks]
move |i, x| {
if predicate(x) {
ControlFlow::Break(i)
} else {
ControlFlow::Continue(Add::add(i, 1))
}
if predicate(x) { ControlFlow::Break(i) } else { ControlFlow::Continue(i + 1) }
}
}

Expand Down