From b1375cd23cf0fa3ffefa8ec82e6cba04d233b44c Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Fri, 10 Jul 2020 23:53:25 +0200 Subject: [PATCH 01/33] Deny unsafe op in unsafe fns without the unsafe keyword, first part for std/thread --- library/std/src/thread/local.rs | 99 +++++++++++++++++++++++---------- library/std/src/thread/mod.rs | 28 +++++++--- 2 files changed, 91 insertions(+), 36 deletions(-) diff --git a/library/std/src/thread/local.rs b/library/std/src/thread/local.rs index 66508f06b2884..13252baf731ef 100644 --- a/library/std/src/thread/local.rs +++ b/library/std/src/thread/local.rs @@ -284,7 +284,11 @@ mod lazy { } pub unsafe fn get(&self) -> Option<&'static T> { - (*self.inner.get()).as_ref() + // SAFETY: No reference is ever handed out to the inner cell nor + // mutable reference to the Option inside said cell. This make it + // safe to hand a reference, though the lifetime of 'static + // is itself unsafe, making the get method unsafe. + unsafe { (*self.inner.get()).as_ref() } } pub unsafe fn initialize T>(&self, init: F) -> &'static T { @@ -293,6 +297,8 @@ mod lazy { let value = init(); let ptr = self.inner.get(); + // SAFETY: + // // note that this can in theory just be `*ptr = Some(value)`, but due to // the compiler will currently codegen that pattern with something like: // @@ -305,22 +311,31 @@ mod lazy { // value (an aliasing violation). To avoid setting the "I'm running a // destructor" flag we just use `mem::replace` which should sequence the // operations a little differently and make this safe to call. - let _ = mem::replace(&mut *ptr, Some(value)); - - // After storing `Some` we want to get a reference to the contents of - // what we just stored. While we could use `unwrap` here and it should - // always work it empirically doesn't seem to always get optimized away, - // which means that using something like `try_with` can pull in - // panicking code and cause a large size bloat. - match *ptr { - Some(ref x) => x, - None => hint::unreachable_unchecked(), + unsafe { + let _ = mem::replace(&mut *ptr, Some(value)); + } + + // SAFETY: the *ptr operation is made safe by the `mem::replace` + // call above that made sure a valid value is present behind it. + unsafe { + // After storing `Some` we want to get a reference to the contents of + // what we just stored. While we could use `unwrap` here and it should + // always work it empirically doesn't seem to always get optimized away, + // which means that using something like `try_with` can pull in + // panicking code and cause a large size bloat. + match *ptr { + Some(ref x) => x, + None => hint::unreachable_unchecked(), + } } } #[allow(unused)] pub unsafe fn take(&mut self) -> Option { - (*self.inner.get()).take() + // SAFETY: The other methods hand out references while taking &self. + // As such, calling this method when such references are still alive + // will fail because it takes a &mut self, conflicting with them. + unsafe { (*self.inner.get()).take() } } } } @@ -409,9 +424,17 @@ pub mod fast { } pub unsafe fn get T>(&self, init: F) -> Option<&'static T> { - match self.inner.get() { - Some(val) => Some(val), - None => self.try_initialize(init), + // SAFETY: See the definitions of `LazyKeyInner::get` and + // `try_initialize` for more informations. + // + // The call to `get` is made safe because no mutable references are + // ever handed out and the `try_initialize` is dependant on the + // passed `init` function. + unsafe { + match self.inner.get() { + Some(val) => Some(val), + None => self.try_initialize(init), + } } } @@ -425,8 +448,10 @@ pub mod fast { // LLVM issue: https://bugs.llvm.org/show_bug.cgi?id=41722 #[cold] unsafe fn try_initialize T>(&self, init: F) -> Option<&'static T> { - if !mem::needs_drop::() || self.try_register_dtor() { - Some(self.inner.initialize(init)) + // SAFETY: See comment above. + if !mem::needs_drop::() || unsafe { self.try_register_dtor() } { + // SAFETY: See comment above. + Some(unsafe { self.inner.initialize(init) }) } else { None } @@ -438,8 +463,12 @@ pub mod fast { unsafe fn try_register_dtor(&self) -> bool { match self.dtor_state.get() { DtorState::Unregistered => { - // dtor registration happens before initialization. - register_dtor(self as *const _ as *mut u8, destroy_value::); + // SAFETY: dtor registration happens before initialization. + // Passing `self` as a pointer while using `destroy_value` + // is safe because the function will build a pointer to a + // Key, which is the type of self and so find the correct + // size. + unsafe { register_dtor(self as *const _ as *mut u8, destroy_value::) }; self.dtor_state.set(DtorState::Registered); true } @@ -455,13 +484,21 @@ pub mod fast { unsafe extern "C" fn destroy_value(ptr: *mut u8) { let ptr = ptr as *mut Key; + // SAFETY: + // + // The pointer `ptr` has been built just above and comes from + // `try_register_dtor` where it is originally a Key coming from `self`, + // making it non-NUL and of the correct type. + // // Right before we run the user destructor be sure to set the // `Option` to `None`, and `dtor_state` to `RunningOrHasRun`. This // causes future calls to `get` to run `try_initialize_drop` again, // which will now fail, and return `None`. - let value = (*ptr).inner.take(); - (*ptr).dtor_state.set(DtorState::RunningOrHasRun); - drop(value); + unsafe { + let value = (*ptr).inner.take(); + (*ptr).dtor_state.set(DtorState::RunningOrHasRun); + drop(value); + } } } @@ -530,11 +567,15 @@ pub mod os { ptr }; - Some((*ptr).inner.initialize(init)) + // SAFETY: ptr has been ensured as non-NUL just above an so can be + // dereferenced safely. + unsafe { Some((*ptr).inner.initialize(init)) } } } unsafe extern "C" fn destroy_value(ptr: *mut u8) { + // SAFETY: + // // The OS TLS ensures that this key contains a NULL value when this // destructor starts to run. We set it back to a sentinel value of 1 to // ensure that any future calls to `get` for this thread will return @@ -542,11 +583,13 @@ pub mod os { // // Note that to prevent an infinite loop we reset it back to null right // before we return from the destructor ourselves. - let ptr = Box::from_raw(ptr as *mut Value); - let key = ptr.key; - key.os.set(1 as *mut u8); - drop(ptr); - key.os.set(ptr::null_mut()); + unsafe { + let ptr = Box::from_raw(ptr as *mut Value); + let key = ptr.key; + key.os.set(1 as *mut u8); + drop(ptr); + key.os.set(ptr::null_mut()); + } } } diff --git a/library/std/src/thread/mod.rs b/library/std/src/thread/mod.rs index 202867258f1e4..90dc17de37704 100644 --- a/library/std/src/thread/mod.rs +++ b/library/std/src/thread/mod.rs @@ -155,6 +155,7 @@ //! [`with`]: struct.LocalKey.html#method.with #![stable(feature = "rust1", since = "1.0.0")] +#![deny(unsafe_op_in_unsafe_fn)] use crate::any::Any; use crate::cell::UnsafeCell; @@ -470,14 +471,23 @@ impl Builder { imp::Thread::set_name(name); } - thread_info::set(imp::guard::current(), their_thread); + // SAFETY: the stack guard passed is the one for the current thread. + // This means the current thread's stack and the new thread's stack + // are properly set and protected from each other. + thread_info::set(unsafe { imp::guard::current() }, their_thread); let try_result = panic::catch_unwind(panic::AssertUnwindSafe(|| { crate::sys_common::backtrace::__rust_begin_short_backtrace(f) })); - *their_packet.get() = Some(try_result); + // SAFETY: `their_packet` as been built just above and moved by the + // closure (it is an Arc<...>) and `my_packet` will be stored in the + // same `JoinInner` as this closure meaning the mutation will be + // safe (not modify it and affect a value far away). + unsafe { *their_packet.get() = Some(try_result) }; }; Ok(JoinHandle(JoinInner { + // SAFETY: + // // `imp::Thread::new` takes a closure with a `'static` lifetime, since it's passed // through FFI or otherwise used with low-level threading primitives that have no // notion of or way to enforce lifetimes. @@ -489,12 +499,14 @@ impl Builder { // Similarly, the `sys` implementation must guarantee that no references to the closure // exist after the thread has terminated, which is signaled by `Thread::join` // returning. - native: Some(imp::Thread::new( - stack_size, - mem::transmute::, Box>(Box::new( - main, - )), - )?), + native: unsafe { + Some(imp::Thread::new( + stack_size, + mem::transmute::, Box>( + Box::new(main), + ), + )?) + }, thread: my_thread, packet: Packet(my_packet), })) From 3a22b2181a0cd662be00e053a9e9c36d5330e444 Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Sat, 11 Jul 2020 00:15:24 +0200 Subject: [PATCH 02/33] Finished documenting all unsafe op inside unsafe fn --- library/std/src/thread/local.rs | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/library/std/src/thread/local.rs b/library/std/src/thread/local.rs index 13252baf731ef..bd0945c9a0704 100644 --- a/library/std/src/thread/local.rs +++ b/library/std/src/thread/local.rs @@ -536,20 +536,28 @@ pub mod os { } pub unsafe fn get(&'static self, init: fn() -> T) -> Option<&'static T> { - let ptr = self.os.get() as *mut Value; + // SAFETY: No mutable references are ever handed out meaning getting + // the value is ok. + let ptr = unsafe { self.os.get() as *mut Value }; if ptr as usize > 1 { - if let Some(ref value) = (*ptr).inner.get() { + // SAFETY: the check ensured the pointer is safe (its destructor + // is not running) + it is coming from a trusted source (self). + if let Some(ref value) = unsafe { (*ptr).inner.get() } { return Some(value); } } - self.try_initialize(init) + // SAFETY: At this point we are sure we have no value and so + // initializing (or trying to) is safe. + unsafe { self.try_initialize(init) } } // `try_initialize` is only called once per os thread local variable, // except in corner cases where thread_local dtors reference other // thread_local's, or it is being recursively initialized. unsafe fn try_initialize(&'static self, init: fn() -> T) -> Option<&'static T> { - let ptr = self.os.get() as *mut Value; + // SAFETY: No mutable references are ever handed out meaning getting + // the value is ok. + let ptr = unsafe { self.os.get() as *mut Value }; if ptr as usize == 1 { // destructor is running return None; @@ -560,7 +568,11 @@ pub mod os { // local copy, so do that now. let ptr: Box> = box Value { inner: LazyKeyInner::new(), key: self }; let ptr = Box::into_raw(ptr); - self.os.set(ptr as *mut u8); + // SAFETY: At this point we are sure there is no value inside + // ptr so setting it will not affect anyone else. + unsafe { + self.os.set(ptr as *mut u8); + } ptr } else { // recursive initialization From ee289d2c242a3513eec4f9af617d8b96517cadf7 Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Sat, 29 Aug 2020 20:10:05 +0200 Subject: [PATCH 03/33] Improve some SAFETY comments following suggestions --- library/std/src/thread/local.rs | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/library/std/src/thread/local.rs b/library/std/src/thread/local.rs index bd0945c9a0704..4ac1e523a97ea 100644 --- a/library/std/src/thread/local.rs +++ b/library/std/src/thread/local.rs @@ -311,12 +311,23 @@ mod lazy { // value (an aliasing violation). To avoid setting the "I'm running a // destructor" flag we just use `mem::replace` which should sequence the // operations a little differently and make this safe to call. + // + // `ptr` can be dereferenced safely since it was obtained from + // `UnsafeCell::get`, which should not return a non-aligned or NUL pointer. + // What's more a `LazyKeyInner` can only be created with `new`, which ensures + // `inner` is correctly initialized and all calls to methods on `LazyKeyInner` + // will leave `inner` initialized too. unsafe { let _ = mem::replace(&mut *ptr, Some(value)); } - // SAFETY: the *ptr operation is made safe by the `mem::replace` - // call above that made sure a valid value is present behind it. + // SAFETY: the `*ptr` operation is made safe by the `mem::replace` + // call above combined with `ptr` being correct from the beginning + // (see previous SAFETY: comment above). + // + // Plus, with the call to `mem::replace` it is guaranteed there is + // a `Some` behind `ptr`, not a `None` so `unreachable_unchecked` + // will never be reached. unsafe { // After storing `Some` we want to get a reference to the contents of // what we just stored. While we could use `unwrap` here and it should @@ -333,8 +344,8 @@ mod lazy { #[allow(unused)] pub unsafe fn take(&mut self) -> Option { // SAFETY: The other methods hand out references while taking &self. - // As such, calling this method when such references are still alive - // will fail because it takes a &mut self, conflicting with them. + // As such, callers of this method must ensure no `&` and `&mut` are + // available and used at the same time. unsafe { (*self.inner.get()).take() } } } @@ -448,9 +459,9 @@ pub mod fast { // LLVM issue: https://bugs.llvm.org/show_bug.cgi?id=41722 #[cold] unsafe fn try_initialize T>(&self, init: F) -> Option<&'static T> { - // SAFETY: See comment above. + // SAFETY: See comment above (this function doc). if !mem::needs_drop::() || unsafe { self.try_register_dtor() } { - // SAFETY: See comment above. + // SAFETY: See comment above (his function doc). Some(unsafe { self.inner.initialize(init) }) } else { None From a06edda3ad9abd4f07d07bbe46cb488efeebbbd0 Mon Sep 17 00:00:00 2001 From: Tavian Barnes Date: Wed, 9 Sep 2020 10:16:03 -0400 Subject: [PATCH 04/33] Fix segfault if pthread_getattr_np fails glibc destroys[1] the passed pthread_attr_t if pthread_getattr_np() fails. Destroying it again leads to a segfault. Fix it by only destroying it on success for glibc. [1]: https://sourceware.org/git/?p=glibc.git;a=blob;f=nptl/pthread_getattr_np.c;h=ce437205e41dc05653e435f6188768cccdd91c99;hb=HEAD#l205 --- library/std/src/sys/unix/thread.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/library/std/src/sys/unix/thread.rs b/library/std/src/sys/unix/thread.rs index 04da9812ddc45..569aedd741192 100644 --- a/library/std/src/sys/unix/thread.rs +++ b/library/std/src/sys/unix/thread.rs @@ -305,7 +305,9 @@ pub mod guard { assert_eq!(libc::pthread_attr_getstack(&attr, &mut stackaddr, &mut stacksize), 0); ret = Some(stackaddr); } - assert_eq!(libc::pthread_attr_destroy(&mut attr), 0); + if e == 0 || cfg!(not(target_env = "gnu")) { + assert_eq!(libc::pthread_attr_destroy(&mut attr), 0); + } ret } @@ -446,7 +448,9 @@ pub mod guard { Some(stackaddr..stackaddr + guardsize) }; } - assert_eq!(libc::pthread_attr_destroy(&mut attr), 0); + if e == 0 || cfg!(not(target_env = "gnu")) { + assert_eq!(libc::pthread_attr_destroy(&mut attr), 0); + } ret } } From a684153f2920729f9fc3ea27ddb77d7cc3543214 Mon Sep 17 00:00:00 2001 From: Tavian Barnes Date: Wed, 9 Sep 2020 11:10:43 -0400 Subject: [PATCH 05/33] Only call pthread_attr_destroy() after getattr_np() succeeds on all libcs The calling convention of pthread_getattr_np() is to initialize the pthread_attr_t, so _destroy() is only necessary on success (and _init() isn't necessary beforehand). On the other hand, FreeBSD wants the attr_t to be initialized before pthread_attr_get_np(), and therefore it should always be destroyed afterwards. --- library/std/src/sys/unix/thread.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/library/std/src/sys/unix/thread.rs b/library/std/src/sys/unix/thread.rs index 569aedd741192..652219e28f6e0 100644 --- a/library/std/src/sys/unix/thread.rs +++ b/library/std/src/sys/unix/thread.rs @@ -294,6 +294,7 @@ pub mod guard { unsafe fn get_stack_start() -> Option<*mut libc::c_void> { let mut ret = None; let mut attr: libc::pthread_attr_t = crate::mem::zeroed(); + #[cfg(target_os = "freebsd")] assert_eq!(libc::pthread_attr_init(&mut attr), 0); #[cfg(target_os = "freebsd")] let e = libc::pthread_attr_get_np(libc::pthread_self(), &mut attr); @@ -305,7 +306,7 @@ pub mod guard { assert_eq!(libc::pthread_attr_getstack(&attr, &mut stackaddr, &mut stacksize), 0); ret = Some(stackaddr); } - if e == 0 || cfg!(not(target_env = "gnu")) { + if e == 0 || cfg!(target_os = "freebsd") { assert_eq!(libc::pthread_attr_destroy(&mut attr), 0); } ret @@ -405,6 +406,7 @@ pub mod guard { pub unsafe fn current() -> Option { let mut ret = None; let mut attr: libc::pthread_attr_t = crate::mem::zeroed(); + #[cfg(target_os = "freebsd")] assert_eq!(libc::pthread_attr_init(&mut attr), 0); #[cfg(target_os = "freebsd")] let e = libc::pthread_attr_get_np(libc::pthread_self(), &mut attr); @@ -448,7 +450,7 @@ pub mod guard { Some(stackaddr..stackaddr + guardsize) }; } - if e == 0 || cfg!(not(target_env = "gnu")) { + if e == 0 || cfg!(target_os = "freebsd") { assert_eq!(libc::pthread_attr_destroy(&mut attr), 0); } ret From 8f27e3cb1b4140d9124d60df0850ef734e73b884 Mon Sep 17 00:00:00 2001 From: Christiaan Dirkx Date: Sun, 13 Sep 2020 01:55:34 +0200 Subject: [PATCH 06/33] Make some methods of `Pin` unstable const Make the following methods unstable const under the `const_pin` feature: - `new` - `new_unchecked` - `into_inner` - `into_inner_unchecked` - `get_ref` - `into_ref` Also adds tests for these methods in a const context. Tracking issue: #76654 --- library/core/src/lib.rs | 1 + library/core/src/pin.rs | 27 ++++++++++++++++----------- library/core/tests/lib.rs | 2 ++ library/core/tests/pin.rs | 21 +++++++++++++++++++++ 4 files changed, 40 insertions(+), 11 deletions(-) create mode 100644 library/core/tests/pin.rs diff --git a/library/core/src/lib.rs b/library/core/src/lib.rs index c3cadcbb01e31..696b6a64a9fec 100644 --- a/library/core/src/lib.rs +++ b/library/core/src/lib.rs @@ -80,6 +80,7 @@ #![feature(const_int_pow)] #![feature(constctlz)] #![feature(const_panic)] +#![feature(const_pin)] #![feature(const_fn_union)] #![feature(const_generics)] #![feature(const_option)] diff --git a/library/core/src/pin.rs b/library/core/src/pin.rs index 1cc1dfb014335..fa5b37edc36e6 100644 --- a/library/core/src/pin.rs +++ b/library/core/src/pin.rs @@ -471,9 +471,10 @@ impl> Pin

{ /// /// Unlike `Pin::new_unchecked`, this method is safe because the pointer /// `P` dereferences to an [`Unpin`] type, which cancels the pinning guarantees. - #[stable(feature = "pin", since = "1.33.0")] #[inline(always)] - pub fn new(pointer: P) -> Pin

{ + #[rustc_const_unstable(feature = "const_pin", issue = "76654")] + #[stable(feature = "pin", since = "1.33.0")] + pub const fn new(pointer: P) -> Pin

{ // SAFETY: the value pointed to is `Unpin`, and so has no requirements // around pinning. unsafe { Pin::new_unchecked(pointer) } @@ -483,9 +484,10 @@ impl> Pin

{ /// /// This requires that the data inside this `Pin` is [`Unpin`] so that we /// can ignore the pinning invariants when unwrapping it. - #[stable(feature = "pin_into_inner", since = "1.39.0")] #[inline(always)] - pub fn into_inner(pin: Pin

) -> P { + #[rustc_const_unstable(feature = "const_pin", issue = "76654")] + #[stable(feature = "pin_into_inner", since = "1.39.0")] + pub const fn into_inner(pin: Pin

) -> P { pin.pointer } } @@ -556,9 +558,10 @@ impl Pin

{ /// /// [`mem::swap`]: crate::mem::swap #[lang = "new_unchecked"] - #[stable(feature = "pin", since = "1.33.0")] #[inline(always)] - pub unsafe fn new_unchecked(pointer: P) -> Pin

{ + #[rustc_const_unstable(feature = "const_pin", issue = "76654")] + #[stable(feature = "pin", since = "1.33.0")] + pub const unsafe fn new_unchecked(pointer: P) -> Pin

{ Pin { pointer } } @@ -589,9 +592,10 @@ impl Pin

{ /// /// If the underlying data is [`Unpin`], [`Pin::into_inner`] should be used /// instead. - #[stable(feature = "pin_into_inner", since = "1.39.0")] #[inline(always)] - pub unsafe fn into_inner_unchecked(pin: Pin

) -> P { + #[rustc_const_unstable(feature = "const_pin", issue = "76654")] + #[stable(feature = "pin_into_inner", since = "1.39.0")] + pub const unsafe fn into_inner_unchecked(pin: Pin

) -> P { pin.pointer } } @@ -693,17 +697,18 @@ impl<'a, T: ?Sized> Pin<&'a T> { /// with the same lifetime as the original `Pin`. /// /// ["pinning projections"]: self#projections-and-structural-pinning - #[stable(feature = "pin", since = "1.33.0")] #[inline(always)] - pub fn get_ref(self) -> &'a T { + #[rustc_const_unstable(feature = "const_pin", issue = "76654")] + #[stable(feature = "pin", since = "1.33.0")] + pub const fn get_ref(self) -> &'a T { self.pointer } } impl<'a, T: ?Sized> Pin<&'a mut T> { /// Converts this `Pin<&mut T>` into a `Pin<&T>` with the same lifetime. - #[stable(feature = "pin", since = "1.33.0")] #[inline(always)] + #[stable(feature = "pin", since = "1.33.0")] pub fn into_ref(self) -> Pin<&'a T> { Pin { pointer: self.pointer } } diff --git a/library/core/tests/lib.rs b/library/core/tests/lib.rs index a2e294ace1860..b8d67d7266543 100644 --- a/library/core/tests/lib.rs +++ b/library/core/tests/lib.rs @@ -39,6 +39,7 @@ #![feature(iter_order_by)] #![feature(cmp_min_max_by)] #![feature(iter_map_while)] +#![feature(const_pin)] #![feature(const_slice_from_raw_parts)] #![feature(const_raw_ptr_deref)] #![feature(never_type)] @@ -74,6 +75,7 @@ mod num; mod ops; mod option; mod pattern; +mod pin; mod ptr; mod result; mod slice; diff --git a/library/core/tests/pin.rs b/library/core/tests/pin.rs new file mode 100644 index 0000000000000..1363353163829 --- /dev/null +++ b/library/core/tests/pin.rs @@ -0,0 +1,21 @@ +use core::pin::Pin; + +#[test] +fn pin_const() { + // test that the methods of `Pin` are usable in a const context + + const POINTER: &'static usize = &2; + + const PINNED: Pin<&'static usize> = Pin::new(POINTER); + const PINNED_UNCHECKED: Pin<&'static usize> = unsafe { Pin::new_unchecked(POINTER) }; + assert_eq!(PINNED_UNCHECKED, PINNED); + + const INNER: &'static usize = Pin::into_inner(PINNED); + assert_eq!(INNER, POINTER); + + const INNER_UNCHECKED: &'static usize = unsafe { Pin::into_inner_unchecked(PINNED) }; + assert_eq!(INNER_UNCHECKED, POINTER); + + const REF: &'static usize = PINNED.get_ref(); + assert_eq!(REF, POINTER) +} From e5447a22222ecc6a650e75282cb9931b910854b2 Mon Sep 17 00:00:00 2001 From: Simon Vandel Sillesen Date: Sun, 13 Sep 2020 10:47:20 +0200 Subject: [PATCH 07/33] Fix #76432 Only insert StorageDeads if we actually removed one. Fixes an issue where we added StorageDead to a place with no StorageLive --- .../transform/simplify_comparison_integral.rs | 43 +++---- src/test/mir-opt/issue_76432.rs | 16 +++ ...76432.test.SimplifyComparisonIntegral.diff | 116 ++++++++++++++++++ 3 files changed, 154 insertions(+), 21 deletions(-) create mode 100644 src/test/mir-opt/issue_76432.rs create mode 100644 src/test/mir-opt/issue_76432.test.SimplifyComparisonIntegral.diff diff --git a/compiler/rustc_mir/src/transform/simplify_comparison_integral.rs b/compiler/rustc_mir/src/transform/simplify_comparison_integral.rs index a450a75d091ef..9b460c9ecb1be 100644 --- a/compiler/rustc_mir/src/transform/simplify_comparison_integral.rs +++ b/compiler/rustc_mir/src/transform/simplify_comparison_integral.rs @@ -61,26 +61,6 @@ impl<'tcx> MirPass<'tcx> for SimplifyComparisonIntegral { _ => unreachable!(), } - let terminator = bb.terminator_mut(); - - // add StorageDead for the place switched on at the top of each target - for bb_idx in new_targets.iter() { - storage_deads_to_insert.push(( - *bb_idx, - Statement { - source_info: terminator.source_info, - kind: StatementKind::StorageDead(opt.to_switch_on.local), - }, - )); - } - - terminator.kind = TerminatorKind::SwitchInt { - discr: Operand::Move(opt.to_switch_on), - switch_ty: opt.branch_value_ty, - values: vec![new_value].into(), - targets: new_targets, - }; - // delete comparison statement if it the value being switched on was moved, which means it can not be user later on if opt.can_remove_bin_op_stmt { bb.statements[opt.bin_op_stmt_idx].make_nop(); @@ -106,14 +86,35 @@ impl<'tcx> MirPass<'tcx> for SimplifyComparisonIntegral { } } + let terminator = bb.terminator(); + // remove StorageDead (if it exists) being used in the assign of the comparison for (stmt_idx, stmt) in bb.statements.iter().enumerate() { if !matches!(stmt.kind, StatementKind::StorageDead(local) if local == opt.to_switch_on.local) { continue; } - storage_deads_to_remove.push((stmt_idx, opt.bb_idx)) + storage_deads_to_remove.push((stmt_idx, opt.bb_idx)); + // if we have StorageDeads to remove then make sure to insert them at the top of each target + for bb_idx in new_targets.iter() { + storage_deads_to_insert.push(( + *bb_idx, + Statement { + source_info: terminator.source_info, + kind: StatementKind::StorageDead(opt.to_switch_on.local), + }, + )); + } } + + let terminator = bb.terminator_mut(); + + terminator.kind = TerminatorKind::SwitchInt { + discr: Operand::Move(opt.to_switch_on), + switch_ty: opt.branch_value_ty, + values: vec![new_value].into(), + targets: new_targets, + }; } for (idx, bb_idx) in storage_deads_to_remove { diff --git a/src/test/mir-opt/issue_76432.rs b/src/test/mir-opt/issue_76432.rs new file mode 100644 index 0000000000000..c8b405ca8eaaf --- /dev/null +++ b/src/test/mir-opt/issue_76432.rs @@ -0,0 +1,16 @@ +// Check that we do not insert StorageDead at each target if StorageDead was never seen + +// EMIT_MIR issue_76432.test.SimplifyComparisonIntegral.diff +use std::fmt::Debug; + +fn test(x: T) { + let v: &[T] = &[x, x, x]; + match v { + [ref v1, ref v2, ref v3] => [v1 as *const _, v2 as *const _, v3 as *const _], + _ => unreachable!(), + }; +} + +fn main() { + test(0u32); +} diff --git a/src/test/mir-opt/issue_76432.test.SimplifyComparisonIntegral.diff b/src/test/mir-opt/issue_76432.test.SimplifyComparisonIntegral.diff new file mode 100644 index 0000000000000..499134b69919f --- /dev/null +++ b/src/test/mir-opt/issue_76432.test.SimplifyComparisonIntegral.diff @@ -0,0 +1,116 @@ +- // MIR for `test` before SimplifyComparisonIntegral ++ // MIR for `test` after SimplifyComparisonIntegral + + fn test(_1: T) -> () { + debug x => _1; // in scope 0 at $DIR/issue_76432.rs:6:38: 6:39 + let mut _0: (); // return place in scope 0 at $DIR/issue_76432.rs:6:44: 6:44 + let _2: &[T]; // in scope 0 at $DIR/issue_76432.rs:7:9: 7:10 + let mut _3: &[T; 3]; // in scope 0 at $DIR/issue_76432.rs:7:19: 7:29 + let _4: &[T; 3]; // in scope 0 at $DIR/issue_76432.rs:7:19: 7:29 + let _5: [T; 3]; // in scope 0 at $DIR/issue_76432.rs:7:20: 7:29 + let mut _6: T; // in scope 0 at $DIR/issue_76432.rs:7:21: 7:22 + let mut _7: T; // in scope 0 at $DIR/issue_76432.rs:7:24: 7:25 + let mut _8: T; // in scope 0 at $DIR/issue_76432.rs:7:27: 7:28 + let _9: [*const T; 3]; // in scope 0 at $DIR/issue_76432.rs:8:5: 11:6 + let mut _10: usize; // in scope 0 at $DIR/issue_76432.rs:9:9: 9:33 + let mut _11: usize; // in scope 0 at $DIR/issue_76432.rs:9:9: 9:33 + let mut _12: bool; // in scope 0 at $DIR/issue_76432.rs:9:9: 9:33 + let mut _16: *const T; // in scope 0 at $DIR/issue_76432.rs:9:38: 9:52 + let mut _17: *const T; // in scope 0 at $DIR/issue_76432.rs:9:38: 9:52 + let mut _18: *const T; // in scope 0 at $DIR/issue_76432.rs:9:54: 9:68 + let mut _19: *const T; // in scope 0 at $DIR/issue_76432.rs:9:54: 9:68 + let mut _20: *const T; // in scope 0 at $DIR/issue_76432.rs:9:70: 9:84 + let mut _21: *const T; // in scope 0 at $DIR/issue_76432.rs:9:70: 9:84 + let mut _22: !; // in scope 0 at $SRC_DIR/std/src/macros.rs:LL:COL + scope 1 { + debug v => _2; // in scope 1 at $DIR/issue_76432.rs:7:9: 7:10 + let _13: &T; // in scope 1 at $DIR/issue_76432.rs:9:10: 9:16 + let _14: &T; // in scope 1 at $DIR/issue_76432.rs:9:18: 9:24 + let _15: &T; // in scope 1 at $DIR/issue_76432.rs:9:26: 9:32 + scope 2 { + debug v1 => _13; // in scope 2 at $DIR/issue_76432.rs:9:10: 9:16 + debug v2 => _14; // in scope 2 at $DIR/issue_76432.rs:9:18: 9:24 + debug v3 => _15; // in scope 2 at $DIR/issue_76432.rs:9:26: 9:32 + } + } + + bb0: { + StorageLive(_2); // scope 0 at $DIR/issue_76432.rs:7:9: 7:10 + StorageLive(_3); // scope 0 at $DIR/issue_76432.rs:7:19: 7:29 + StorageLive(_4); // scope 0 at $DIR/issue_76432.rs:7:19: 7:29 + StorageLive(_5); // scope 0 at $DIR/issue_76432.rs:7:20: 7:29 + StorageLive(_6); // scope 0 at $DIR/issue_76432.rs:7:21: 7:22 + _6 = _1; // scope 0 at $DIR/issue_76432.rs:7:21: 7:22 + StorageLive(_7); // scope 0 at $DIR/issue_76432.rs:7:24: 7:25 + _7 = _1; // scope 0 at $DIR/issue_76432.rs:7:24: 7:25 + StorageLive(_8); // scope 0 at $DIR/issue_76432.rs:7:27: 7:28 + _8 = _1; // scope 0 at $DIR/issue_76432.rs:7:27: 7:28 + _5 = [move _6, move _7, move _8]; // scope 0 at $DIR/issue_76432.rs:7:20: 7:29 + StorageDead(_8); // scope 0 at $DIR/issue_76432.rs:7:28: 7:29 + StorageDead(_7); // scope 0 at $DIR/issue_76432.rs:7:28: 7:29 + StorageDead(_6); // scope 0 at $DIR/issue_76432.rs:7:28: 7:29 + _4 = &_5; // scope 0 at $DIR/issue_76432.rs:7:19: 7:29 + _3 = _4; // scope 0 at $DIR/issue_76432.rs:7:19: 7:29 + _2 = move _3 as &[T] (Pointer(Unsize)); // scope 0 at $DIR/issue_76432.rs:7:19: 7:29 + StorageDead(_3); // scope 0 at $DIR/issue_76432.rs:7:28: 7:29 + StorageDead(_4); // scope 0 at $DIR/issue_76432.rs:7:29: 7:30 + StorageLive(_9); // scope 1 at $DIR/issue_76432.rs:8:5: 11:6 + _10 = Len((*_2)); // scope 1 at $DIR/issue_76432.rs:9:9: 9:33 + _11 = const 3_usize; // scope 1 at $DIR/issue_76432.rs:9:9: 9:33 +- _12 = Eq(move _10, const 3_usize); // scope 1 at $DIR/issue_76432.rs:9:9: 9:33 +- switchInt(move _12) -> [false: bb1, otherwise: bb2]; // scope 1 at $DIR/issue_76432.rs:9:9: 9:33 ++ nop; // scope 1 at $DIR/issue_76432.rs:9:9: 9:33 ++ switchInt(move _10) -> [3_usize: bb2, otherwise: bb1]; // scope 1 at $DIR/issue_76432.rs:9:9: 9:33 + } + + bb1: { + StorageLive(_22); // scope 1 at $SRC_DIR/std/src/macros.rs:LL:COL + begin_panic::<&str>(const "internal error: entered unreachable code"); // scope 1 at $SRC_DIR/std/src/macros.rs:LL:COL + // mir::Constant + // + span: $SRC_DIR/std/src/macros.rs:LL:COL + // + literal: Const { ty: fn(&str) -> ! {std::rt::begin_panic::<&str>}, val: Value(Scalar()) } + // ty::Const + // + ty: &str + // + val: Value(Slice { data: Allocation { bytes: [105, 110, 116, 101, 114, 110, 97, 108, 32, 101, 114, 114, 111, 114, 58, 32, 101, 110, 116, 101, 114, 101, 100, 32, 117, 110, 114, 101, 97, 99, 104, 97, 98, 108, 101, 32, 99, 111, 100, 101], relocations: Relocations(SortedMap { data: [] }), init_mask: InitMask { blocks: [1099511627775], len: Size { raw: 40 } }, size: Size { raw: 40 }, align: Align { pow2: 0 }, mutability: Not, extra: () }, start: 0, end: 40 }) + // mir::Constant + // + span: $SRC_DIR/core/src/macros/mod.rs:LL:COL + // + literal: Const { ty: &str, val: Value(Slice { data: Allocation { bytes: [105, 110, 116, 101, 114, 110, 97, 108, 32, 101, 114, 114, 111, 114, 58, 32, 101, 110, 116, 101, 114, 101, 100, 32, 117, 110, 114, 101, 97, 99, 104, 97, 98, 108, 101, 32, 99, 111, 100, 101], relocations: Relocations(SortedMap { data: [] }), init_mask: InitMask { blocks: [1099511627775], len: Size { raw: 40 } }, size: Size { raw: 40 }, align: Align { pow2: 0 }, mutability: Not, extra: () }, start: 0, end: 40 }) } + } + + bb2: { + StorageLive(_13); // scope 1 at $DIR/issue_76432.rs:9:10: 9:16 + _13 = &(*_2)[0 of 3]; // scope 1 at $DIR/issue_76432.rs:9:10: 9:16 + StorageLive(_14); // scope 1 at $DIR/issue_76432.rs:9:18: 9:24 + _14 = &(*_2)[1 of 3]; // scope 1 at $DIR/issue_76432.rs:9:18: 9:24 + StorageLive(_15); // scope 1 at $DIR/issue_76432.rs:9:26: 9:32 + _15 = &(*_2)[2 of 3]; // scope 1 at $DIR/issue_76432.rs:9:26: 9:32 + StorageLive(_16); // scope 2 at $DIR/issue_76432.rs:9:38: 9:52 + StorageLive(_17); // scope 2 at $DIR/issue_76432.rs:9:38: 9:52 + _17 = &raw const (*_13); // scope 2 at $DIR/issue_76432.rs:9:38: 9:40 + _16 = _17; // scope 2 at $DIR/issue_76432.rs:9:38: 9:52 + StorageLive(_18); // scope 2 at $DIR/issue_76432.rs:9:54: 9:68 + StorageLive(_19); // scope 2 at $DIR/issue_76432.rs:9:54: 9:68 + _19 = &raw const (*_14); // scope 2 at $DIR/issue_76432.rs:9:54: 9:56 + _18 = _19; // scope 2 at $DIR/issue_76432.rs:9:54: 9:68 + StorageLive(_20); // scope 2 at $DIR/issue_76432.rs:9:70: 9:84 + StorageLive(_21); // scope 2 at $DIR/issue_76432.rs:9:70: 9:84 + _21 = &raw const (*_15); // scope 2 at $DIR/issue_76432.rs:9:70: 9:72 + _20 = _21; // scope 2 at $DIR/issue_76432.rs:9:70: 9:84 + _9 = [move _16, move _18, move _20]; // scope 2 at $DIR/issue_76432.rs:9:37: 9:85 + StorageDead(_21); // scope 2 at $DIR/issue_76432.rs:9:84: 9:85 + StorageDead(_20); // scope 2 at $DIR/issue_76432.rs:9:84: 9:85 + StorageDead(_19); // scope 2 at $DIR/issue_76432.rs:9:84: 9:85 + StorageDead(_18); // scope 2 at $DIR/issue_76432.rs:9:84: 9:85 + StorageDead(_17); // scope 2 at $DIR/issue_76432.rs:9:84: 9:85 + StorageDead(_16); // scope 2 at $DIR/issue_76432.rs:9:84: 9:85 + StorageDead(_15); // scope 1 at $DIR/issue_76432.rs:9:84: 9:85 + StorageDead(_14); // scope 1 at $DIR/issue_76432.rs:9:84: 9:85 + StorageDead(_13); // scope 1 at $DIR/issue_76432.rs:9:84: 9:85 + StorageDead(_9); // scope 1 at $DIR/issue_76432.rs:11:6: 11:7 + _0 = const (); // scope 0 at $DIR/issue_76432.rs:6:44: 12:2 + StorageDead(_5); // scope 0 at $DIR/issue_76432.rs:12:1: 12:2 + StorageDead(_2); // scope 0 at $DIR/issue_76432.rs:12:1: 12:2 + return; // scope 0 at $DIR/issue_76432.rs:12:2: 12:2 + } + } + From 9c5d0c18a73163a34c5a3fae8544537a2fed83ac Mon Sep 17 00:00:00 2001 From: Simon Vandel Sillesen Date: Sun, 13 Sep 2020 16:04:45 +0200 Subject: [PATCH 08/33] MIR pass to remove unneeded drops on types not needing drop This is heavily dependent on MIR inlining running to actually see the drop statement --- compiler/rustc_mir/src/transform/mod.rs | 2 + .../src/transform/remove_unneeded_drops.rs | 57 +++++++++++++++++++ ...annot_opt_generic.RemoveUnneededDrops.diff | 32 +++++++++++ ...ed_drops.dont_opt.RemoveUnneededDrops.diff | 32 +++++++++++ ...nneeded_drops.opt.RemoveUnneededDrops.diff | 29 ++++++++++ ....opt_generic_copy.RemoveUnneededDrops.diff | 29 ++++++++++ src/test/mir-opt/remove_unneeded_drops.rs | 28 +++++++++ 7 files changed, 209 insertions(+) create mode 100644 compiler/rustc_mir/src/transform/remove_unneeded_drops.rs create mode 100644 src/test/mir-opt/remove_unneeded_drops.cannot_opt_generic.RemoveUnneededDrops.diff create mode 100644 src/test/mir-opt/remove_unneeded_drops.dont_opt.RemoveUnneededDrops.diff create mode 100644 src/test/mir-opt/remove_unneeded_drops.opt.RemoveUnneededDrops.diff create mode 100644 src/test/mir-opt/remove_unneeded_drops.opt_generic_copy.RemoveUnneededDrops.diff create mode 100644 src/test/mir-opt/remove_unneeded_drops.rs diff --git a/compiler/rustc_mir/src/transform/mod.rs b/compiler/rustc_mir/src/transform/mod.rs index 8025b7c02043d..3e831a48c2577 100644 --- a/compiler/rustc_mir/src/transform/mod.rs +++ b/compiler/rustc_mir/src/transform/mod.rs @@ -36,6 +36,7 @@ pub mod nrvo; pub mod promote_consts; pub mod qualify_min_const_fn; pub mod remove_noop_landing_pads; +pub mod remove_unneeded_drops; pub mod required_consts; pub mod rustc_peek; pub mod simplify; @@ -455,6 +456,7 @@ fn run_optimization_passes<'tcx>( // The main optimizations that we do on MIR. let optimizations: &[&dyn MirPass<'tcx>] = &[ + &remove_unneeded_drops::RemoveUnneededDrops::new(def_id), &match_branches::MatchBranchSimplification, // inst combine is after MatchBranchSimplification to clean up Ne(_1, false) &instcombine::InstCombine, diff --git a/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs b/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs new file mode 100644 index 0000000000000..0f023d71dd1da --- /dev/null +++ b/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs @@ -0,0 +1,57 @@ +//! This pass replaces a drop of a type that does not need dropping, with a goto + +use crate::transform::{MirPass, MirSource}; +use rustc_hir::def_id::LocalDefId; +use rustc_middle::mir::visit::Visitor; +use rustc_middle::mir::*; +use rustc_middle::ty::TyCtxt; + +pub struct RemoveUnneededDrops { + def_id: LocalDefId, +} + +impl RemoveUnneededDrops { + pub fn new(def_id: LocalDefId) -> Self { + Self { def_id } + } +} + +impl<'tcx> MirPass<'tcx> for RemoveUnneededDrops { + fn run_pass(&self, tcx: TyCtxt<'tcx>, source: MirSource<'tcx>, body: &mut Body<'tcx>) { + trace!("Running SimplifyComparisonIntegral on {:?}", source); + let mut opt_finder = RemoveUnneededDropsOptimizationFinder { + tcx, + body, + optimizations: vec![], + def_id: self.def_id, + }; + opt_finder.visit_body(body); + for (loc, target) in opt_finder.optimizations { + let terminator = body.basic_blocks_mut()[loc.block].terminator_mut(); + debug!("SUCCESS: replacing `drop` with goto({:?})", target); + terminator.kind = TerminatorKind::Goto { target }; + } + } +} + +impl<'a, 'tcx> Visitor<'tcx> for RemoveUnneededDropsOptimizationFinder<'a, 'tcx> { + fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) { + match terminator.kind { + TerminatorKind::Drop { place, target, .. } => { + let ty = place.ty(self.body, self.tcx); + let needs_drop = ty.ty.needs_drop(self.tcx, self.tcx.param_env(self.def_id)); + if !needs_drop { + self.optimizations.push((location, target)); + } + } + _ => {} + } + self.super_terminator(terminator, location); + } +} +pub struct RemoveUnneededDropsOptimizationFinder<'a, 'tcx> { + tcx: TyCtxt<'tcx>, + body: &'a Body<'tcx>, + optimizations: Vec<(Location, BasicBlock)>, + def_id: LocalDefId, +} diff --git a/src/test/mir-opt/remove_unneeded_drops.cannot_opt_generic.RemoveUnneededDrops.diff b/src/test/mir-opt/remove_unneeded_drops.cannot_opt_generic.RemoveUnneededDrops.diff new file mode 100644 index 0000000000000..545ad2794ee17 --- /dev/null +++ b/src/test/mir-opt/remove_unneeded_drops.cannot_opt_generic.RemoveUnneededDrops.diff @@ -0,0 +1,32 @@ +- // MIR for `cannot_opt_generic` before RemoveUnneededDrops ++ // MIR for `cannot_opt_generic` after RemoveUnneededDrops + + fn cannot_opt_generic(_1: T) -> () { + debug x => _1; // in scope 0 at $DIR/remove_unneeded_drops.rs:19:26: 19:27 + let mut _0: (); // return place in scope 0 at $DIR/remove_unneeded_drops.rs:19:32: 19:32 + let _2: (); // in scope 0 at $DIR/remove_unneeded_drops.rs:20:5: 20:12 + let mut _3: T; // in scope 0 at $DIR/remove_unneeded_drops.rs:20:10: 20:11 + scope 1 { + debug _x => _3; // in scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL + } + + bb0: { + StorageLive(_2); // scope 0 at $DIR/remove_unneeded_drops.rs:20:5: 20:12 + StorageLive(_3); // scope 0 at $DIR/remove_unneeded_drops.rs:20:10: 20:11 + _3 = move _1; // scope 0 at $DIR/remove_unneeded_drops.rs:20:10: 20:11 + _2 = const (); // scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL + drop(_3) -> [return: bb2, unwind: bb1]; // scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL + } + + bb1 (cleanup): { + resume; // scope 0 at $DIR/remove_unneeded_drops.rs:19:1: 21:2 + } + + bb2: { + StorageDead(_3); // scope 0 at $DIR/remove_unneeded_drops.rs:20:11: 20:12 + StorageDead(_2); // scope 0 at $DIR/remove_unneeded_drops.rs:20:12: 20:13 + _0 = const (); // scope 0 at $DIR/remove_unneeded_drops.rs:19:32: 21:2 + return; // scope 0 at $DIR/remove_unneeded_drops.rs:21:2: 21:2 + } + } + diff --git a/src/test/mir-opt/remove_unneeded_drops.dont_opt.RemoveUnneededDrops.diff b/src/test/mir-opt/remove_unneeded_drops.dont_opt.RemoveUnneededDrops.diff new file mode 100644 index 0000000000000..78d65d13bd1bf --- /dev/null +++ b/src/test/mir-opt/remove_unneeded_drops.dont_opt.RemoveUnneededDrops.diff @@ -0,0 +1,32 @@ +- // MIR for `dont_opt` before RemoveUnneededDrops ++ // MIR for `dont_opt` after RemoveUnneededDrops + + fn dont_opt(_1: Vec) -> () { + debug x => _1; // in scope 0 at $DIR/remove_unneeded_drops.rs:7:13: 7:14 + let mut _0: (); // return place in scope 0 at $DIR/remove_unneeded_drops.rs:7:27: 7:27 + let _2: (); // in scope 0 at $DIR/remove_unneeded_drops.rs:8:5: 8:12 + let mut _3: std::vec::Vec; // in scope 0 at $DIR/remove_unneeded_drops.rs:8:10: 8:11 + scope 1 { + debug _x => _3; // in scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL + } + + bb0: { + StorageLive(_2); // scope 0 at $DIR/remove_unneeded_drops.rs:8:5: 8:12 + StorageLive(_3); // scope 0 at $DIR/remove_unneeded_drops.rs:8:10: 8:11 + _3 = move _1; // scope 0 at $DIR/remove_unneeded_drops.rs:8:10: 8:11 + _2 = const (); // scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL + drop(_3) -> [return: bb2, unwind: bb1]; // scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL + } + + bb1 (cleanup): { + resume; // scope 0 at $DIR/remove_unneeded_drops.rs:7:1: 9:2 + } + + bb2: { + StorageDead(_3); // scope 0 at $DIR/remove_unneeded_drops.rs:8:11: 8:12 + StorageDead(_2); // scope 0 at $DIR/remove_unneeded_drops.rs:8:12: 8:13 + _0 = const (); // scope 0 at $DIR/remove_unneeded_drops.rs:7:27: 9:2 + return; // scope 0 at $DIR/remove_unneeded_drops.rs:9:2: 9:2 + } + } + diff --git a/src/test/mir-opt/remove_unneeded_drops.opt.RemoveUnneededDrops.diff b/src/test/mir-opt/remove_unneeded_drops.opt.RemoveUnneededDrops.diff new file mode 100644 index 0000000000000..34193ccc5c7e7 --- /dev/null +++ b/src/test/mir-opt/remove_unneeded_drops.opt.RemoveUnneededDrops.diff @@ -0,0 +1,29 @@ +- // MIR for `opt` before RemoveUnneededDrops ++ // MIR for `opt` after RemoveUnneededDrops + + fn opt(_1: bool) -> () { + debug x => _1; // in scope 0 at $DIR/remove_unneeded_drops.rs:2:8: 2:9 + let mut _0: (); // return place in scope 0 at $DIR/remove_unneeded_drops.rs:2:17: 2:17 + let _2: (); // in scope 0 at $DIR/remove_unneeded_drops.rs:3:5: 3:12 + let mut _3: bool; // in scope 0 at $DIR/remove_unneeded_drops.rs:3:10: 3:11 + scope 1 { + debug _x => _3; // in scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL + } + + bb0: { + StorageLive(_2); // scope 0 at $DIR/remove_unneeded_drops.rs:3:5: 3:12 + StorageLive(_3); // scope 0 at $DIR/remove_unneeded_drops.rs:3:10: 3:11 + _3 = _1; // scope 0 at $DIR/remove_unneeded_drops.rs:3:10: 3:11 + _2 = const (); // scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL +- drop(_3) -> bb1; // scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL ++ goto -> bb1; // scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL + } + + bb1: { + StorageDead(_3); // scope 0 at $DIR/remove_unneeded_drops.rs:3:11: 3:12 + StorageDead(_2); // scope 0 at $DIR/remove_unneeded_drops.rs:3:12: 3:13 + _0 = const (); // scope 0 at $DIR/remove_unneeded_drops.rs:2:17: 4:2 + return; // scope 0 at $DIR/remove_unneeded_drops.rs:4:2: 4:2 + } + } + diff --git a/src/test/mir-opt/remove_unneeded_drops.opt_generic_copy.RemoveUnneededDrops.diff b/src/test/mir-opt/remove_unneeded_drops.opt_generic_copy.RemoveUnneededDrops.diff new file mode 100644 index 0000000000000..2e6c8c8a78a72 --- /dev/null +++ b/src/test/mir-opt/remove_unneeded_drops.opt_generic_copy.RemoveUnneededDrops.diff @@ -0,0 +1,29 @@ +- // MIR for `opt_generic_copy` before RemoveUnneededDrops ++ // MIR for `opt_generic_copy` after RemoveUnneededDrops + + fn opt_generic_copy(_1: T) -> () { + debug x => _1; // in scope 0 at $DIR/remove_unneeded_drops.rs:12:30: 12:31 + let mut _0: (); // return place in scope 0 at $DIR/remove_unneeded_drops.rs:12:36: 12:36 + let _2: (); // in scope 0 at $DIR/remove_unneeded_drops.rs:13:5: 13:12 + let mut _3: T; // in scope 0 at $DIR/remove_unneeded_drops.rs:13:10: 13:11 + scope 1 { + debug _x => _3; // in scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL + } + + bb0: { + StorageLive(_2); // scope 0 at $DIR/remove_unneeded_drops.rs:13:5: 13:12 + StorageLive(_3); // scope 0 at $DIR/remove_unneeded_drops.rs:13:10: 13:11 + _3 = _1; // scope 0 at $DIR/remove_unneeded_drops.rs:13:10: 13:11 + _2 = const (); // scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL +- drop(_3) -> bb1; // scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL ++ goto -> bb1; // scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL + } + + bb1: { + StorageDead(_3); // scope 0 at $DIR/remove_unneeded_drops.rs:13:11: 13:12 + StorageDead(_2); // scope 0 at $DIR/remove_unneeded_drops.rs:13:12: 13:13 + _0 = const (); // scope 0 at $DIR/remove_unneeded_drops.rs:12:36: 14:2 + return; // scope 0 at $DIR/remove_unneeded_drops.rs:14:2: 14:2 + } + } + diff --git a/src/test/mir-opt/remove_unneeded_drops.rs b/src/test/mir-opt/remove_unneeded_drops.rs new file mode 100644 index 0000000000000..17d7e79a1eb0a --- /dev/null +++ b/src/test/mir-opt/remove_unneeded_drops.rs @@ -0,0 +1,28 @@ +// EMIT_MIR remove_unneeded_drops.opt.RemoveUnneededDrops.diff +fn opt(x: bool) { + drop(x); +} + +// EMIT_MIR remove_unneeded_drops.dont_opt.RemoveUnneededDrops.diff +fn dont_opt(x: Vec) { + drop(x); +} + +// EMIT_MIR remove_unneeded_drops.opt_generic_copy.RemoveUnneededDrops.diff +fn opt_generic_copy(x: T) { + drop(x); +} + +// EMIT_MIR remove_unneeded_drops.cannot_opt_generic.RemoveUnneededDrops.diff +// since the pass is not running on monomorphisized code, +// we can't (but probably should) optimize this +fn cannot_opt_generic(x: T) { + drop(x); +} + +fn main() { + opt(true); + opt_generic_copy(42); + cannot_opt_generic(42); + dont_opt(vec![true]); +} From 9d47ecf327edbb5dd1b11dd112335506d8d26165 Mon Sep 17 00:00:00 2001 From: Simon Vandel Sillesen Date: Mon, 14 Sep 2020 22:56:39 +0200 Subject: [PATCH 09/33] Suggestion from review Co-authored-by: Andreas Jonson --- compiler/rustc_mir/src/transform/remove_unneeded_drops.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs b/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs index 0f023d71dd1da..393b0f923b910 100644 --- a/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs +++ b/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs @@ -18,7 +18,7 @@ impl RemoveUnneededDrops { impl<'tcx> MirPass<'tcx> for RemoveUnneededDrops { fn run_pass(&self, tcx: TyCtxt<'tcx>, source: MirSource<'tcx>, body: &mut Body<'tcx>) { - trace!("Running SimplifyComparisonIntegral on {:?}", source); + trace!("Running RemoveUnneededDrops on {:?}", source); let mut opt_finder = RemoveUnneededDropsOptimizationFinder { tcx, body, From 4675a3104b3ace025560337c5d164e330f6b9d68 Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Fri, 18 Sep 2020 09:51:26 +0200 Subject: [PATCH 10/33] Use intra-doc links in core/src/iter when possible --- library/core/src/iter/sources.rs | 76 +++++++------------- library/core/src/iter/traits/accum.rs | 46 ++++++------ library/core/src/iter/traits/collect.rs | 20 ++---- library/core/src/iter/traits/double_ended.rs | 39 +++++----- library/core/src/iter/traits/exact_size.rs | 23 +++--- library/core/src/iter/traits/iterator.rs | 1 - library/core/src/iter/traits/marker.rs | 18 ++--- 7 files changed, 91 insertions(+), 132 deletions(-) diff --git a/library/core/src/iter/sources.rs b/library/core/src/iter/sources.rs index d76fa89bd012c..0348d5a10d984 100644 --- a/library/core/src/iter/sources.rs +++ b/library/core/src/iter/sources.rs @@ -5,9 +5,7 @@ use super::{FusedIterator, TrustedLen}; /// An iterator that repeats an element endlessly. /// -/// This `struct` is created by the [`repeat`] function. See its documentation for more. -/// -/// [`repeat`]: fn.repeat.html +/// This `struct` is created by the [`repeat()`] function. See its documentation for more. #[derive(Clone, Debug)] #[stable(feature = "rust1", since = "1.0.0")] pub struct Repeat { @@ -47,15 +45,11 @@ unsafe impl TrustedLen for Repeat {} /// The `repeat()` function repeats a single value over and over again. /// /// Infinite iterators like `repeat()` are often used with adapters like -/// [`take`], in order to make them finite. -/// -/// [`take`]: trait.Iterator.html#method.take +/// [`Iterator::take()`], in order to make them finite. /// /// If the element type of the iterator you need does not implement `Clone`, /// or if you do not want to keep the repeated element in memory, you can -/// instead use the [`repeat_with`] function. -/// -/// [`repeat_with`]: fn.repeat_with.html +/// instead use the [`repeat_with()`] function. /// /// # Examples /// @@ -77,7 +71,7 @@ unsafe impl TrustedLen for Repeat {} /// assert_eq!(Some(4), fours.next()); /// ``` /// -/// Going finite with [`take`]: +/// Going finite with [`Iterator::take()`]: /// /// ``` /// use std::iter; @@ -102,10 +96,8 @@ pub fn repeat(elt: T) -> Repeat { /// An iterator that repeats elements of type `A` endlessly by /// applying the provided closure `F: FnMut() -> A`. /// -/// This `struct` is created by the [`repeat_with`] function. +/// This `struct` is created by the [`repeat_with()`] function. /// See its documentation for more. -/// -/// [`repeat_with`]: fn.repeat_with.html #[derive(Copy, Clone, Debug)] #[stable(feature = "iterator_repeat_with", since = "1.28.0")] pub struct RepeatWith { @@ -139,20 +131,18 @@ unsafe impl A> TrustedLen for RepeatWith {} /// The `repeat_with()` function calls the repeater over and over again. /// /// Infinite iterators like `repeat_with()` are often used with adapters like -/// [`take`], in order to make them finite. +/// [`Iterator::take()`], in order to make them finite. /// -/// [`take`]: trait.Iterator.html#method.take -/// -/// If the element type of the iterator you need implements `Clone`, and +/// If the element type of the iterator you need implements [`Clone`], and /// it is OK to keep the source element in memory, you should instead use -/// the [`repeat`] function. -/// -/// [`repeat`]: fn.repeat.html +/// the [`repeat()`] function. /// -/// An iterator produced by `repeat_with()` is not a `DoubleEndedIterator`. -/// If you need `repeat_with()` to return a `DoubleEndedIterator`, +/// An iterator produced by `repeat_with()` is not a [`DoubleEndedIterator`]. +/// If you need `repeat_with()` to return a [`DoubleEndedIterator`], /// please open a GitHub issue explaining your use case. /// +/// [`DoubleEndedIterator`]: crate::iter::DoubleEndedIterator +/// /// # Examples /// /// Basic usage: @@ -201,9 +191,7 @@ pub fn repeat_with A>(repeater: F) -> RepeatWith { /// An iterator that yields nothing. /// -/// This `struct` is created by the [`empty`] function. See its documentation for more. -/// -/// [`empty`]: fn.empty.html +/// This `struct` is created by the [`empty()`] function. See its documentation for more. #[stable(feature = "iter_empty", since = "1.2.0")] pub struct Empty(marker::PhantomData); @@ -292,9 +280,7 @@ pub const fn empty() -> Empty { /// An iterator that yields an element exactly once. /// -/// This `struct` is created by the [`once`] function. See its documentation for more. -/// -/// [`once`]: fn.once.html +/// This `struct` is created by the [`once()`] function. See its documentation for more. #[derive(Clone, Debug)] #[stable(feature = "iter_once", since = "1.2.0")] pub struct Once { @@ -336,12 +322,12 @@ impl FusedIterator for Once {} /// Creates an iterator that yields an element exactly once. /// -/// This is commonly used to adapt a single value into a [`chain`] of other +/// This is commonly used to adapt a single value into a [`chain()`] of other /// kinds of iteration. Maybe you have an iterator that covers almost /// everything, but you need an extra special case. Maybe you have a function /// which works on iterators, but you only need to process one value. /// -/// [`chain`]: trait.Iterator.html#method.chain +/// [`chain()`]: Iterator::chain /// /// # Examples /// @@ -393,10 +379,8 @@ pub fn once(value: T) -> Once { /// An iterator that yields a single element of type `A` by /// applying the provided closure `F: FnOnce() -> A`. /// -/// This `struct` is created by the [`once_with`] function. +/// This `struct` is created by the [`once_with()`] function. /// See its documentation for more. -/// -/// [`once_with`]: fn.once_with.html #[derive(Clone, Debug)] #[stable(feature = "iter_once_with", since = "1.43.0")] pub struct OnceWith { @@ -442,15 +426,14 @@ unsafe impl A> TrustedLen for OnceWith {} /// Creates an iterator that lazily generates a value exactly once by invoking /// the provided closure. /// -/// This is commonly used to adapt a single value generator into a [`chain`] of +/// This is commonly used to adapt a single value generator into a [`chain()`] of /// other kinds of iteration. Maybe you have an iterator that covers almost /// everything, but you need an extra special case. Maybe you have a function /// which works on iterators, but you only need to process one value. /// -/// Unlike [`once`], this function will lazily generate the value on request. +/// Unlike [`once()`], this function will lazily generate the value on request. /// -/// [`once`]: fn.once.html -/// [`chain`]: trait.Iterator.html#method.chain +/// [`chain()`]: Iterator::chain /// /// # Examples /// @@ -505,17 +488,16 @@ pub fn once_with A>(gen: F) -> OnceWith { /// /// This allows creating a custom iterator with any behavior /// without using the more verbose syntax of creating a dedicated type -/// and implementing the `Iterator` trait for it. +/// and implementing the [`Iterator`] trait for it. /// /// Note that the `FromFn` iterator doesn’t make assumptions about the behavior of the closure, /// and therefore conservatively does not implement [`FusedIterator`], -/// or override [`Iterator::size_hint`] from its default `(0, None)`. -/// -/// [`FusedIterator`]: trait.FusedIterator.html -/// [`Iterator::size_hint`]: trait.Iterator.html#method.size_hint +/// or override [`Iterator::size_hint()`] from its default `(0, None)`. /// /// The closure can use captures and its environment to track state across iterations. Depending on -/// how the iterator is used, this may require specifying the `move` keyword on the closure. +/// how the iterator is used, this may require specifying the [`move`] keyword on the closure. +/// +/// [`move`]: ../../../std/keyword.move.html /// /// # Examples /// @@ -549,10 +531,8 @@ where /// An iterator where each iteration calls the provided closure `F: FnMut() -> Option`. /// -/// This `struct` is created by the [`iter::from_fn`] function. +/// This `struct` is created by the [`from_fn()`] function. /// See its documentation for more. -/// -/// [`iter::from_fn`]: fn.from_fn.html #[derive(Clone)] #[stable(feature = "iter_from_fn", since = "1.34.0")] pub struct FromFn(F); @@ -601,10 +581,8 @@ where /// An new iterator where each successive item is computed based on the preceding one. /// -/// This `struct` is created by the [`successors`] function. +/// This `struct` is created by the [`successors()`] function. /// See its documentation for more. -/// -/// [`successors`]: fn.successors.html #[derive(Clone)] #[stable(feature = "iter_successors", since = "1.34.0")] pub struct Successors { diff --git a/library/core/src/iter/traits/accum.rs b/library/core/src/iter/traits/accum.rs index 494c75174ff83..dc0d8087ffbff 100644 --- a/library/core/src/iter/traits/accum.rs +++ b/library/core/src/iter/traits/accum.rs @@ -4,14 +4,13 @@ use crate::ops::{Add, Mul}; /// Trait to represent types that can be created by summing up an iterator. /// -/// This trait is used to implement the [`sum`] method on iterators. Types which -/// implement the trait can be generated by the [`sum`] method. Like +/// This trait is used to implement the [`sum()`] method on iterators. Types which +/// implement the trait can be generated by the [`sum()`] method. Like /// [`FromIterator`] this trait should rarely be called directly and instead -/// interacted with through [`Iterator::sum`]. +/// interacted with through [`Iterator::sum()`]. /// -/// [`sum`]: #tymethod.sum -/// [`FromIterator`]: crate::iter::FromIterator -/// [`Iterator::sum`]: crate::iter::Iterator::sum +/// [`sum()`]: Sum::sum +/// [`FromIterator`]: iter::FromIterator #[stable(feature = "iter_arith_traits", since = "1.12.0")] pub trait Sum: Sized { /// Method which takes an iterator and generates `Self` from the elements by @@ -23,14 +22,13 @@ pub trait Sum: Sized { /// Trait to represent types that can be created by multiplying elements of an /// iterator. /// -/// This trait is used to implement the [`product`] method on iterators. Types -/// which implement the trait can be generated by the [`product`] method. Like +/// This trait is used to implement the [`product()`] method on iterators. Types +/// which implement the trait can be generated by the [`product()`] method. Like /// [`FromIterator`] this trait should rarely be called directly and instead -/// interacted with through [`Iterator::product`]. +/// interacted with through [`Iterator::product()`]. /// -/// [`product`]: #tymethod.product -/// [`FromIterator`]: crate::iter::FromIterator -/// [`Iterator::product`]: crate::iter::Iterator::product +/// [`product()`]: Product::product +/// [`FromIterator`]: iter::FromIterator #[stable(feature = "iter_arith_traits", since = "1.12.0")] pub trait Product: Sized { /// Method which takes an iterator and generates `Self` from the elements by @@ -120,9 +118,9 @@ impl Sum> for Result where T: Sum, { - /// Takes each element in the `Iterator`: if it is an `Err`, no further - /// elements are taken, and the `Err` is returned. Should no `Err` occur, - /// the sum of all elements is returned. + /// Takes each element in the [`Iterator`]: if it is an [`Err`], no further + /// elements are taken, and the [`Err`] is returned. Should no [`Err`] + /// occur, the sum of all elements is returned. /// /// # Examples /// @@ -150,9 +148,9 @@ impl Product> for Result where T: Product, { - /// Takes each element in the `Iterator`: if it is an `Err`, no further - /// elements are taken, and the `Err` is returned. Should no `Err` occur, - /// the product of all elements is returned. + /// Takes each element in the [`Iterator`]: if it is an [`Err`], no further + /// elements are taken, and the [`Err`] is returned. Should no [`Err`] + /// occur, the product of all elements is returned. fn product(iter: I) -> Result where I: Iterator>, @@ -166,9 +164,9 @@ impl Sum> for Option where T: Sum, { - /// Takes each element in the `Iterator`: if it is a `None`, no further - /// elements are taken, and the `None` is returned. Should no `None` occur, - /// the sum of all elements is returned. + /// Takes each element in the [`Iterator`]: if it is a [`None`], no further + /// elements are taken, and the [`None`] is returned. Should no [`None`] + /// occur, the sum of all elements is returned. /// /// # Examples /// @@ -193,9 +191,9 @@ impl Product> for Option where T: Product, { - /// Takes each element in the `Iterator`: if it is a `None`, no further - /// elements are taken, and the `None` is returned. Should no `None` occur, - /// the product of all elements is returned. + /// Takes each element in the [`Iterator`]: if it is a [`None`], no further + /// elements are taken, and the [`None`] is returned. Should no [`None`] + /// occur, the product of all elements is returned. fn product(iter: I) -> Option where I: Iterator>, diff --git a/library/core/src/iter/traits/collect.rs b/library/core/src/iter/traits/collect.rs index 75827d785e10e..41a503c4abb4f 100644 --- a/library/core/src/iter/traits/collect.rs +++ b/library/core/src/iter/traits/collect.rs @@ -1,21 +1,15 @@ -/// Conversion from an `Iterator`. +/// Conversion from an [`Iterator`]. /// /// By implementing `FromIterator` for a type, you define how it will be /// created from an iterator. This is common for types which describe a /// collection of some kind. /// -/// `FromIterator`'s [`from_iter`] is rarely called explicitly, and is instead -/// used through [`Iterator`]'s [`collect`] method. See [`collect`]'s +/// [`FromIterator::from_iter()`] is rarely called explicitly, and is instead +/// used through [`Iterator::collect()`] method. See [`Iterator::collect()`]'s /// documentation for more examples. /// -/// [`from_iter`]: #tymethod.from_iter -/// [`Iterator`]: trait.Iterator.html -/// [`collect`]: trait.Iterator.html#method.collect -/// /// See also: [`IntoIterator`]. /// -/// [`IntoIterator`]: trait.IntoIterator.html -/// /// # Examples /// /// Basic usage: @@ -30,7 +24,7 @@ /// assert_eq!(v, vec![5, 5, 5, 5, 5]); /// ``` /// -/// Using [`collect`] to implicitly use `FromIterator`: +/// Using [`Iterator::collect()`] to implicitly use `FromIterator`: /// /// ``` /// let five_fives = std::iter::repeat(5).take(5); @@ -119,7 +113,7 @@ pub trait FromIterator: Sized { fn from_iter>(iter: T) -> Self; } -/// Conversion into an `Iterator`. +/// Conversion into an [`Iterator`]. /// /// By implementing `IntoIterator` for a type, you define how it will be /// converted to an iterator. This is common for types which describe a @@ -130,8 +124,6 @@ pub trait FromIterator: Sized { /// /// See also: [`FromIterator`]. /// -/// [`FromIterator`]: trait.FromIterator.html -/// /// # Examples /// /// Basic usage: @@ -326,7 +318,7 @@ pub trait Extend { /// As this is the only required method for this trait, the [trait-level] docs /// contain more details. /// - /// [trait-level]: trait.Extend.html + /// [trait-level]: Extend /// /// # Examples /// diff --git a/library/core/src/iter/traits/double_ended.rs b/library/core/src/iter/traits/double_ended.rs index a025bc8b56049..bc03c143d6afb 100644 --- a/library/core/src/iter/traits/double_ended.rs +++ b/library/core/src/iter/traits/double_ended.rs @@ -10,11 +10,12 @@ use crate::ops::{ControlFlow, Try}; /// and do not cross: iteration is over when they meet in the middle. /// /// In a similar fashion to the [`Iterator`] protocol, once a -/// `DoubleEndedIterator` returns `None` from a `next_back()`, calling it again -/// may or may not ever return `Some` again. `next()` and `next_back()` are -/// interchangeable for this purpose. +/// `DoubleEndedIterator` returns [`None`] from a [`next_back()`], calling it +/// again may or may not ever return [`Some`] again. [`next()`] and +/// [`next_back()`] are interchangeable for this purpose. /// -/// [`Iterator`]: trait.Iterator.html +/// [`next_back()`]: DoubleEndedIterator::next_back +/// [`next()`]: Iterator::next /// /// # Examples /// @@ -42,7 +43,7 @@ pub trait DoubleEndedIterator: Iterator { /// /// The [trait-level] docs contain more details. /// - /// [trait-level]: trait.DoubleEndedIterator.html + /// [trait-level]: DoubleEndedIterator /// /// # Examples /// @@ -66,7 +67,7 @@ pub trait DoubleEndedIterator: Iterator { /// # Remarks /// /// The elements yielded by `DoubleEndedIterator`'s methods may differ from - /// the ones yielded by `Iterator`'s methods: + /// the ones yielded by [`Iterator`]'s methods: /// /// ``` /// let vec = vec![(1, 'a'), (1, 'b'), (1, 'c'), (2, 'a'), (2, 'b')]; @@ -87,25 +88,23 @@ pub trait DoubleEndedIterator: Iterator { /// vec![(2, 'b'), (1, 'c')] /// ); /// ``` - /// #[stable(feature = "rust1", since = "1.0.0")] fn next_back(&mut self) -> Option; /// Returns the `n`th element from the end of the iterator. /// - /// This is essentially the reversed version of [`nth`]. Although like most indexing - /// operations, the count starts from zero, so `nth_back(0)` returns the first value from - /// the end, `nth_back(1)` the second, and so on. + /// This is essentially the reversed version of [`Iterator::nth()`]. + /// Although like most indexing operations, the count starts from zero, so + /// `nth_back(0)` returns the first value from the end, `nth_back(1)` the + /// second, and so on. /// /// Note that all elements between the end and the returned element will be /// consumed, including the returned element. This also means that calling /// `nth_back(0)` multiple times on the same iterator will return different /// elements. /// - /// `nth_back()` will return [`None`] if `n` is greater than or equal to the length of the - /// iterator. - /// - /// [`nth`]: crate::iter::Iterator::nth + /// `nth_back()` will return [`None`] if `n` is greater than or equal to the + /// length of the iterator. /// /// # Examples /// @@ -145,10 +144,8 @@ pub trait DoubleEndedIterator: Iterator { None } - /// This is the reverse version of [`try_fold()`]: it takes elements - /// starting from the back of the iterator. - /// - /// [`try_fold()`]: Iterator::try_fold + /// This is the reverse version of [`Iterator::try_fold()`]: it takes + /// elements starting from the back of the iterator. /// /// # Examples /// @@ -195,8 +192,8 @@ pub trait DoubleEndedIterator: Iterator { /// An iterator method that reduces the iterator's elements to a single, /// final value, starting from the back. /// - /// This is the reverse version of [`fold()`]: it takes elements starting from - /// the back of the iterator. + /// This is the reverse version of [`Iterator::fold()`]: it takes elements + /// starting from the back of the iterator. /// /// `rfold()` takes two arguments: an initial value, and a closure with two /// arguments: an 'accumulator', and an element. The closure returns the value that @@ -213,8 +210,6 @@ pub trait DoubleEndedIterator: Iterator { /// Folding is useful whenever you have a collection of something, and want /// to produce a single value from it. /// - /// [`fold()`]: Iterator::fold - /// /// # Examples /// /// Basic usage: diff --git a/library/core/src/iter/traits/exact_size.rs b/library/core/src/iter/traits/exact_size.rs index ad87d09588e3a..33ace60a27419 100644 --- a/library/core/src/iter/traits/exact_size.rs +++ b/library/core/src/iter/traits/exact_size.rs @@ -6,17 +6,14 @@ /// backwards, a good start is to know where the end is. /// /// When implementing an `ExactSizeIterator`, you must also implement -/// [`Iterator`]. When doing so, the implementation of [`size_hint`] *must* -/// return the exact size of the iterator. -/// -/// [`Iterator`]: trait.Iterator.html -/// [`size_hint`]: trait.Iterator.html#method.size_hint +/// [`Iterator`]. When doing so, the implementation of [`Iterator::size_hint`] +/// *must* return the exact size of the iterator. /// /// The [`len`] method has a default implementation, so you usually shouldn't /// implement it. However, you may be able to provide a more performant /// implementation than the default, so overriding it in this case makes sense. /// -/// [`len`]: #method.len +/// [`len`]: ExactSizeIterator::len /// /// # Examples /// @@ -72,17 +69,17 @@ pub trait ExactSizeIterator: Iterator { /// Returns the exact length of the iterator. /// /// The implementation ensures that the iterator will return exactly `len()` - /// more times a `Some(T)` value, before returning `None`. + /// more times a [`Some(T)`] value, before returning [`None`]. /// This method has a default implementation, so you usually should not /// implement it directly. However, if you can provide a more efficient /// implementation, you can do so. See the [trait-level] docs for an /// example. /// - /// This function has the same safety guarantees as the [`size_hint`] - /// function. + /// This function has the same safety guarantees as the + /// [`Iterator::size_hint`] function. /// - /// [trait-level]: trait.ExactSizeIterator.html - /// [`size_hint`]: trait.Iterator.html#method.size_hint + /// [trait-level]: ExactSizeIterator + /// [`Some(T)`]: Some /// /// # Examples /// @@ -108,8 +105,8 @@ pub trait ExactSizeIterator: Iterator { /// Returns `true` if the iterator is empty. /// - /// This method has a default implementation using `self.len()`, so you - /// don't need to implement it yourself. + /// This method has a default implementation using + /// [`ExactSizeIterator::len()`], so you don't need to implement it yourself. /// /// # Examples /// diff --git a/library/core/src/iter/traits/iterator.rs b/library/core/src/iter/traits/iterator.rs index b8a09f822b6da..f70e92f0ffafe 100644 --- a/library/core/src/iter/traits/iterator.rs +++ b/library/core/src/iter/traits/iterator.rs @@ -2203,7 +2203,6 @@ pub trait Iterator { /// /// `iter.find_map(f)` is equivalent to `iter.filter_map(f).next()`. /// - /// /// # Examples /// /// ``` diff --git a/library/core/src/iter/traits/marker.rs b/library/core/src/iter/traits/marker.rs index f287196da03ef..0900676146c0d 100644 --- a/library/core/src/iter/traits/marker.rs +++ b/library/core/src/iter/traits/marker.rs @@ -2,14 +2,13 @@ /// /// Calling next on a fused iterator that has returned `None` once is guaranteed /// to return [`None`] again. This trait should be implemented by all iterators -/// that behave this way because it allows optimizing [`Iterator::fuse`]. +/// that behave this way because it allows optimizing [`Iterator::fuse()`]. /// /// Note: In general, you should not use `FusedIterator` in generic bounds if -/// you need a fused iterator. Instead, you should just call [`Iterator::fuse`] +/// you need a fused iterator. Instead, you should just call [`Iterator::fuse()`] /// on the iterator. If the iterator is already fused, the additional [`Fuse`] /// wrapper will be a no-op with no performance penalty. /// -/// [`Iterator::fuse`]: crate::iter::Iterator::fuse /// [`Fuse`]: crate::iter::Fuse #[stable(feature = "fused", since = "1.26.0")] #[rustc_unsafe_specialization_marker] @@ -24,18 +23,18 @@ impl FusedIterator for &mut I {} /// (lower bound is equal to upper bound), or the upper bound is [`None`]. /// The upper bound must only be [`None`] if the actual iterator length is /// larger than [`usize::MAX`]. In that case, the lower bound must be -/// [`usize::MAX`], resulting in a [`.size_hint`] of `(usize::MAX, None)`. +/// [`usize::MAX`], resulting in a [`Iterator::size_hint()`] of +/// `(usize::MAX, None)`. /// /// The iterator must produce exactly the number of elements it reported /// or diverge before reaching the end. /// /// # Safety /// -/// This trait must only be implemented when the contract is upheld. -/// Consumers of this trait must inspect [`.size_hint`]’s upper bound. +/// This trait must only be implemented when the contract is upheld. Consumers +/// of this trait must inspect [`Iterator::size_hint()`]’s upper bound. /// /// [`usize::MAX`]: crate::usize::MAX -/// [`.size_hint`]: crate::iter::Iterator::size_hint #[unstable(feature = "trusted_len", issue = "37572")] #[rustc_unsafe_specialization_marker] pub unsafe trait TrustedLen: Iterator {} @@ -46,11 +45,12 @@ unsafe impl TrustedLen for &mut I {} /// An iterator that when yielding an item will have taken at least one element /// from its underlying [`SourceIter`]. /// -/// Calling next() guarantees that at least one value of the iterator's underlying source +/// Calling [`next()`] guarantees that at least one value of the iterator's underlying source /// has been moved out and the result of the iterator chain could be inserted in its place, /// assuming structural constraints of the source allow such an insertion. /// In other words this trait indicates that an iterator pipeline can be collected in place. /// -/// [`SourceIter`]: ../../std/iter/trait.SourceIter.html +/// [`SourceIter`]: crate::iter::SourceIter +/// [`next()`]: Iterator::next #[unstable(issue = "none", feature = "inplace_iteration")] pub unsafe trait InPlaceIterable: Iterator {} From bffd2111f7b033902e60889da5d3fa7a033a7d5e Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Fri, 18 Sep 2020 11:09:36 +0200 Subject: [PATCH 11/33] Finish moving to intra doc links for std::sync --- library/std/src/sync/barrier.rs | 23 +++++------- library/std/src/sync/once.rs | 62 ++++++++++++++------------------- 2 files changed, 34 insertions(+), 51 deletions(-) diff --git a/library/std/src/sync/barrier.rs b/library/std/src/sync/barrier.rs index 204d7f3084f03..b8b4baf14b4c1 100644 --- a/library/std/src/sync/barrier.rs +++ b/library/std/src/sync/barrier.rs @@ -43,11 +43,8 @@ struct BarrierState { generation_id: usize, } -/// A `BarrierWaitResult` is returned by [`wait`] when all threads in the [`Barrier`] -/// have rendezvoused. -/// -/// [`wait`]: struct.Barrier.html#method.wait -/// [`Barrier`]: struct.Barrier.html +/// A `BarrierWaitResult` is returned by [`Barrier::wait()`] when all threads +/// in the [`Barrier`] have rendezvoused. /// /// # Examples /// @@ -70,10 +67,10 @@ impl fmt::Debug for Barrier { impl Barrier { /// Creates a new barrier that can block a given number of threads. /// - /// A barrier will block `n`-1 threads which call [`wait`] and then wake up - /// all threads at once when the `n`th thread calls [`wait`]. + /// A barrier will block `n`-1 threads which call [`wait()`] and then wake + /// up all threads at once when the `n`th thread calls [`wait()`]. /// - /// [`wait`]: #method.wait + /// [`wait()`]: Barrier::wait /// /// # Examples /// @@ -99,10 +96,7 @@ impl Barrier { /// A single (arbitrary) thread will receive a [`BarrierWaitResult`] that /// returns `true` from [`is_leader`] when returning from this function, and /// all other threads will receive a result that will return `false` from - /// [`is_leader`]. - /// - /// [`BarrierWaitResult`]: struct.BarrierWaitResult.html - /// [`is_leader`]: struct.BarrierWaitResult.html#method.is_leader + /// [`BarrierWaitResult::is_leader`]. /// /// # Examples /// @@ -156,13 +150,12 @@ impl fmt::Debug for BarrierWaitResult { } impl BarrierWaitResult { - /// Returns `true` if this thread from [`wait`] is the "leader thread". + /// Returns `true` if this thread from [`Barrier::wait()`] is the + /// "leader thread". /// /// Only one thread will have `true` returned from their result, all other /// threads will have `false` returned. /// - /// [`wait`]: struct.Barrier.html#method.wait - /// /// # Examples /// /// ``` diff --git a/library/std/src/sync/once.rs b/library/std/src/sync/once.rs index 29ae338cb2ec7..ee8902bf764bf 100644 --- a/library/std/src/sync/once.rs +++ b/library/std/src/sync/once.rs @@ -95,11 +95,9 @@ use crate::thread::{self, Thread}; /// A synchronization primitive which can be used to run a one-time global /// initialization. Useful for one-time initialization for FFI or related -/// functionality. This type can only be constructed with the [`Once::new`] +/// functionality. This type can only be constructed with the [`Once::new()`] /// constructor. /// -/// [`Once::new`]: struct.Once.html#method.new -/// /// # Examples /// /// ``` @@ -126,11 +124,8 @@ unsafe impl Sync for Once {} #[stable(feature = "rust1", since = "1.0.0")] unsafe impl Send for Once {} -/// State yielded to [`call_once_force`]’s closure parameter. The state can be -/// used to query the poison status of the [`Once`]. -/// -/// [`call_once_force`]: struct.Once.html#method.call_once_force -/// [`Once`]: struct.Once.html +/// State yielded to [`Once::call_once_force()`]’s closure parameter. The state +/// can be used to query the poison status of the [`Once`]. #[unstable(feature = "once_poison", issue = "33577")] #[derive(Debug)] pub struct OnceState { @@ -140,8 +135,6 @@ pub struct OnceState { /// Initialization value for static [`Once`] values. /// -/// [`Once`]: struct.Once.html -/// /// # Examples /// /// ``` @@ -212,7 +205,7 @@ impl Once { /// happens-before relation between the closure and code executing after the /// return). /// - /// If the given closure recursively invokes `call_once` on the same `Once` + /// If the given closure recursively invokes `call_once` on the same [`Once`] /// instance the exact behavior is not specified, allowed outcomes are /// a panic or a deadlock. /// @@ -249,7 +242,7 @@ impl Once { /// /// The closure `f` will only be executed once if this is called /// concurrently amongst many threads. If that closure panics, however, then - /// it will *poison* this `Once` instance, causing all future invocations of + /// it will *poison* this [`Once`] instance, causing all future invocations of /// `call_once` to also panic. /// /// This is similar to [poisoning with mutexes][poison]. @@ -269,21 +262,21 @@ impl Once { self.call_inner(false, &mut |_| f.take().unwrap()()); } - /// Performs the same function as [`call_once`] except ignores poisoning. + /// Performs the same function as [`call_once()`] except ignores poisoning. /// - /// Unlike [`call_once`], if this `Once` has been poisoned (i.e., a previous - /// call to `call_once` or `call_once_force` caused a panic), calling - /// `call_once_force` will still invoke the closure `f` and will _not_ - /// result in an immediate panic. If `f` panics, the `Once` will remain - /// in a poison state. If `f` does _not_ panic, the `Once` will no - /// longer be in a poison state and all future calls to `call_once` or - /// `call_once_force` will be no-ops. + /// Unlike [`call_once()`], if this [`Once`] has been poisoned (i.e., a previous + /// call to [`call_once()`] or [`call_once_force()`] caused a panic), calling + /// [`call_once_force()`] will still invoke the closure `f` and will _not_ + /// result in an immediate panic. If `f` panics, the [`Once`] will remain + /// in a poison state. If `f` does _not_ panic, the [`Once`] will no + /// longer be in a poison state and all future calls to [`call_once()`] or + /// [`call_once_force()`] will be no-ops. /// /// The closure `f` is yielded a [`OnceState`] structure which can be used - /// to query the poison status of the `Once`. + /// to query the poison status of the [`Once`]. /// - /// [`call_once`]: struct.Once.html#method.call_once - /// [`OnceState`]: struct.OnceState.html + /// [`call_once()`]: Once::call_once + /// [`call_once_force()`]: Once::call_once_force /// /// # Examples /// @@ -329,18 +322,20 @@ impl Once { self.call_inner(true, &mut |p| f.take().unwrap()(p)); } - /// Returns `true` if some `call_once` call has completed + /// Returns `true` if some [`call_once()`] call has completed /// successfully. Specifically, `is_completed` will return false in /// the following situations: - /// * `call_once` was not called at all, - /// * `call_once` was called, but has not yet completed, - /// * the `Once` instance is poisoned + /// * [`call_once()`] was not called at all, + /// * [`call_once()`] was called, but has not yet completed, + /// * the [`Once`] instance is poisoned /// - /// This function returning `false` does not mean that `Once` has not been + /// This function returning `false` does not mean that [`Once`] has not been /// executed. For example, it may have been executed in the time between /// when `is_completed` starts executing and when it returns, in which case /// the `false` return value would be stale (but still permissible). /// + /// [`call_once()`]: Once::call_once + /// /// # Examples /// /// ``` @@ -519,14 +514,11 @@ impl Drop for WaiterQueue<'_> { impl OnceState { /// Returns `true` if the associated [`Once`] was poisoned prior to the - /// invocation of the closure passed to [`call_once_force`]. - /// - /// [`call_once_force`]: struct.Once.html#method.call_once_force - /// [`Once`]: struct.Once.html + /// invocation of the closure passed to [`Once::call_once_force()`]. /// /// # Examples /// - /// A poisoned `Once`: + /// A poisoned [`Once`]: /// /// ``` /// #![feature(once_poison)] @@ -547,7 +539,7 @@ impl OnceState { /// }); /// ``` /// - /// An unpoisoned `Once`: + /// An unpoisoned [`Once`]: /// /// ``` /// #![feature(once_poison)] @@ -565,8 +557,6 @@ impl OnceState { } /// Poison the associated [`Once`] without explicitly panicking. - /// - /// [`Once`]: struct.Once.html // NOTE: This is currently only exposed for the `lazy` module pub(crate) fn poison(&self) { self.set_state_on_drop_to.set(POISONED); From 982ec0d0c9ed93d806340502d48de190e1558a64 Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Fri, 18 Sep 2020 11:14:36 +0200 Subject: [PATCH 12/33] Fix broken link --- library/core/src/iter/sources.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/core/src/iter/sources.rs b/library/core/src/iter/sources.rs index 0348d5a10d984..c28538ef027c0 100644 --- a/library/core/src/iter/sources.rs +++ b/library/core/src/iter/sources.rs @@ -497,7 +497,7 @@ pub fn once_with A>(gen: F) -> OnceWith { /// The closure can use captures and its environment to track state across iterations. Depending on /// how the iterator is used, this may require specifying the [`move`] keyword on the closure. /// -/// [`move`]: ../../../std/keyword.move.html +/// [`move`]: ../../std/keyword.move.html /// /// # Examples /// From b534d9f6e1e2b3e77842c5ededa62f6bcfb2ea58 Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Fri, 18 Sep 2020 12:04:49 +0200 Subject: [PATCH 13/33] Fix broken link --- library/std/src/sync/barrier.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/std/src/sync/barrier.rs b/library/std/src/sync/barrier.rs index b8b4baf14b4c1..8d3e30dbd4545 100644 --- a/library/std/src/sync/barrier.rs +++ b/library/std/src/sync/barrier.rs @@ -94,9 +94,9 @@ impl Barrier { /// be used continuously. /// /// A single (arbitrary) thread will receive a [`BarrierWaitResult`] that - /// returns `true` from [`is_leader`] when returning from this function, and - /// all other threads will receive a result that will return `false` from - /// [`BarrierWaitResult::is_leader`]. + /// returns `true` from [`BarrierWaitResult::is_leader()`] when returning + /// from this function, and all other threads will receive a result that + /// will return `false` from [`BarrierWaitResult::is_leader()`]. /// /// # Examples /// From e3c6e46168758642f0bab64da374f93ed21b1cd0 Mon Sep 17 00:00:00 2001 From: Christiaan Dirkx Date: Fri, 18 Sep 2020 19:23:50 +0200 Subject: [PATCH 14/33] Make some methods of `Pin<&mut T>` unstable const Make the following methods unstable const under the `const_pin` feature: - `into_ref` - `get_mut` - `get_unchecked_mut` --- library/core/src/pin.rs | 13 ++++++++----- library/core/tests/lib.rs | 1 + library/core/tests/pin.rs | 12 +++++++++++- 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/library/core/src/pin.rs b/library/core/src/pin.rs index fa5b37edc36e6..9f0284d5d9542 100644 --- a/library/core/src/pin.rs +++ b/library/core/src/pin.rs @@ -708,8 +708,9 @@ impl<'a, T: ?Sized> Pin<&'a T> { impl<'a, T: ?Sized> Pin<&'a mut T> { /// Converts this `Pin<&mut T>` into a `Pin<&T>` with the same lifetime. #[inline(always)] + #[rustc_const_unstable(feature = "const_pin", issue = "76654")] #[stable(feature = "pin", since = "1.33.0")] - pub fn into_ref(self) -> Pin<&'a T> { + pub const fn into_ref(self) -> Pin<&'a T> { Pin { pointer: self.pointer } } @@ -722,9 +723,10 @@ impl<'a, T: ?Sized> Pin<&'a mut T> { /// that lives for as long as the borrow of the `Pin`, not the lifetime of /// the `Pin` itself. This method allows turning the `Pin` into a reference /// with the same lifetime as the original `Pin`. - #[stable(feature = "pin", since = "1.33.0")] #[inline(always)] - pub fn get_mut(self) -> &'a mut T + #[stable(feature = "pin", since = "1.33.0")] + #[rustc_const_unstable(feature = "const_pin", issue = "76654")] + pub const fn get_mut(self) -> &'a mut T where T: Unpin, { @@ -741,9 +743,10 @@ impl<'a, T: ?Sized> Pin<&'a mut T> { /// /// If the underlying data is `Unpin`, `Pin::get_mut` should be used /// instead. - #[stable(feature = "pin", since = "1.33.0")] #[inline(always)] - pub unsafe fn get_unchecked_mut(self) -> &'a mut T { + #[stable(feature = "pin", since = "1.33.0")] + #[rustc_const_unstable(feature = "const_pin", issue = "76654")] + pub const unsafe fn get_unchecked_mut(self) -> &'a mut T { self.pointer } diff --git a/library/core/tests/lib.rs b/library/core/tests/lib.rs index b8d67d7266543..490f016ab8b2e 100644 --- a/library/core/tests/lib.rs +++ b/library/core/tests/lib.rs @@ -39,6 +39,7 @@ #![feature(iter_order_by)] #![feature(cmp_min_max_by)] #![feature(iter_map_while)] +#![feature(const_mut_refs)] #![feature(const_pin)] #![feature(const_slice_from_raw_parts)] #![feature(const_raw_ptr_deref)] diff --git a/library/core/tests/pin.rs b/library/core/tests/pin.rs index 1363353163829..6f617c8d0c297 100644 --- a/library/core/tests/pin.rs +++ b/library/core/tests/pin.rs @@ -17,5 +17,15 @@ fn pin_const() { assert_eq!(INNER_UNCHECKED, POINTER); const REF: &'static usize = PINNED.get_ref(); - assert_eq!(REF, POINTER) + assert_eq!(REF, POINTER); + + // Note: `pin_mut_const` tests that the methods of `Pin<&mut T>` are usable in a const context. + // A const fn is used because `&mut` is not (yet) usable in constants. + const fn pin_mut_const() { + let _ = Pin::new(&mut 2).into_ref(); + let _ = Pin::new(&mut 2).get_mut(); + let _ = unsafe { Pin::new(&mut 2).get_unchecked_mut() }; + } + + pin_mut_const(); } From 673935fc0c8cbb4925e4fa0c936e4340aa818be0 Mon Sep 17 00:00:00 2001 From: Simon Vandel Sillesen Date: Sat, 19 Sep 2020 13:52:55 +0200 Subject: [PATCH 15/33] Get LocalDefId from source instead of passing in --- compiler/rustc_mir/src/transform/mod.rs | 2 +- .../rustc_mir/src/transform/remove_unneeded_drops.rs | 12 ++---------- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_mir/src/transform/mod.rs b/compiler/rustc_mir/src/transform/mod.rs index 3e831a48c2577..685a56ad5de9c 100644 --- a/compiler/rustc_mir/src/transform/mod.rs +++ b/compiler/rustc_mir/src/transform/mod.rs @@ -456,7 +456,7 @@ fn run_optimization_passes<'tcx>( // The main optimizations that we do on MIR. let optimizations: &[&dyn MirPass<'tcx>] = &[ - &remove_unneeded_drops::RemoveUnneededDrops::new(def_id), + &remove_unneeded_drops::RemoveUnneededDrops, &match_branches::MatchBranchSimplification, // inst combine is after MatchBranchSimplification to clean up Ne(_1, false) &instcombine::InstCombine, diff --git a/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs b/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs index 393b0f923b910..505133219b863 100644 --- a/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs +++ b/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs @@ -6,15 +6,7 @@ use rustc_middle::mir::visit::Visitor; use rustc_middle::mir::*; use rustc_middle::ty::TyCtxt; -pub struct RemoveUnneededDrops { - def_id: LocalDefId, -} - -impl RemoveUnneededDrops { - pub fn new(def_id: LocalDefId) -> Self { - Self { def_id } - } -} +pub struct RemoveUnneededDrops; impl<'tcx> MirPass<'tcx> for RemoveUnneededDrops { fn run_pass(&self, tcx: TyCtxt<'tcx>, source: MirSource<'tcx>, body: &mut Body<'tcx>) { @@ -23,7 +15,7 @@ impl<'tcx> MirPass<'tcx> for RemoveUnneededDrops { tcx, body, optimizations: vec![], - def_id: self.def_id, + def_id: source.def_id().expect_local(), }; opt_finder.visit_body(body); for (loc, target) in opt_finder.optimizations { From 30dd6cf9ba472d4ff12c9a28d03e3a61d54e34da Mon Sep 17 00:00:00 2001 From: Simon Vandel Sillesen Date: Sat, 19 Sep 2020 14:25:53 +0200 Subject: [PATCH 16/33] The optimization should also apply for DropAndReplace --- compiler/rustc_mir/src/transform/remove_unneeded_drops.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs b/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs index 505133219b863..b8ad816f75831 100644 --- a/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs +++ b/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs @@ -29,7 +29,8 @@ impl<'tcx> MirPass<'tcx> for RemoveUnneededDrops { impl<'a, 'tcx> Visitor<'tcx> for RemoveUnneededDropsOptimizationFinder<'a, 'tcx> { fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) { match terminator.kind { - TerminatorKind::Drop { place, target, .. } => { + TerminatorKind::Drop { place, target, .. } + | TerminatorKind::DropAndReplace { place, target, .. } => { let ty = place.ty(self.body, self.tcx); let needs_drop = ty.ty.needs_drop(self.tcx, self.tcx.param_env(self.def_id)); if !needs_drop { From 804f673762d732198e5023be6fa8f1e305e2c2ad Mon Sep 17 00:00:00 2001 From: Simon Vandel Sillesen Date: Sat, 19 Sep 2020 15:21:39 +0200 Subject: [PATCH 17/33] cleanup cfg after optimization --- .../rustc_mir/src/transform/remove_unneeded_drops.rs | 9 +++++++++ .../remove_unneeded_drops.opt.RemoveUnneededDrops.diff | 7 +++---- ...eeded_drops.opt_generic_copy.RemoveUnneededDrops.diff | 7 +++---- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs b/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs index b8ad816f75831..f027f5e33a187 100644 --- a/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs +++ b/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs @@ -6,6 +6,8 @@ use rustc_middle::mir::visit::Visitor; use rustc_middle::mir::*; use rustc_middle::ty::TyCtxt; +use super::simplify::simplify_cfg; + pub struct RemoveUnneededDrops; impl<'tcx> MirPass<'tcx> for RemoveUnneededDrops { @@ -18,11 +20,18 @@ impl<'tcx> MirPass<'tcx> for RemoveUnneededDrops { def_id: source.def_id().expect_local(), }; opt_finder.visit_body(body); + let should_simplify = !opt_finder.optimizations.is_empty(); for (loc, target) in opt_finder.optimizations { let terminator = body.basic_blocks_mut()[loc.block].terminator_mut(); debug!("SUCCESS: replacing `drop` with goto({:?})", target); terminator.kind = TerminatorKind::Goto { target }; } + + // if we applied optimizations, we potentially have some cfg to cleanup to + // make it easier for further passes + if should_simplify { + simplify_cfg(body); + } } } diff --git a/src/test/mir-opt/remove_unneeded_drops.opt.RemoveUnneededDrops.diff b/src/test/mir-opt/remove_unneeded_drops.opt.RemoveUnneededDrops.diff index 34193ccc5c7e7..eba839cf0a48b 100644 --- a/src/test/mir-opt/remove_unneeded_drops.opt.RemoveUnneededDrops.diff +++ b/src/test/mir-opt/remove_unneeded_drops.opt.RemoveUnneededDrops.diff @@ -16,10 +16,9 @@ _3 = _1; // scope 0 at $DIR/remove_unneeded_drops.rs:3:10: 3:11 _2 = const (); // scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL - drop(_3) -> bb1; // scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL -+ goto -> bb1; // scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL - } - - bb1: { +- } +- +- bb1: { StorageDead(_3); // scope 0 at $DIR/remove_unneeded_drops.rs:3:11: 3:12 StorageDead(_2); // scope 0 at $DIR/remove_unneeded_drops.rs:3:12: 3:13 _0 = const (); // scope 0 at $DIR/remove_unneeded_drops.rs:2:17: 4:2 diff --git a/src/test/mir-opt/remove_unneeded_drops.opt_generic_copy.RemoveUnneededDrops.diff b/src/test/mir-opt/remove_unneeded_drops.opt_generic_copy.RemoveUnneededDrops.diff index 2e6c8c8a78a72..840b1ba30fb9b 100644 --- a/src/test/mir-opt/remove_unneeded_drops.opt_generic_copy.RemoveUnneededDrops.diff +++ b/src/test/mir-opt/remove_unneeded_drops.opt_generic_copy.RemoveUnneededDrops.diff @@ -16,10 +16,9 @@ _3 = _1; // scope 0 at $DIR/remove_unneeded_drops.rs:13:10: 13:11 _2 = const (); // scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL - drop(_3) -> bb1; // scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL -+ goto -> bb1; // scope 1 at $SRC_DIR/core/src/mem/mod.rs:LL:COL - } - - bb1: { +- } +- +- bb1: { StorageDead(_3); // scope 0 at $DIR/remove_unneeded_drops.rs:13:11: 13:12 StorageDead(_2); // scope 0 at $DIR/remove_unneeded_drops.rs:13:12: 13:13 _0 = const (); // scope 0 at $DIR/remove_unneeded_drops.rs:12:36: 14:2 From 924cd135b6ab0fe48dae26d9ae30eaadebcd066d Mon Sep 17 00:00:00 2001 From: Giacomo Stevanato Date: Sat, 5 Sep 2020 23:16:56 +0200 Subject: [PATCH 18/33] Added benchmarks for BinaryHeap --- library/alloc/benches/binary_heap.rs | 91 ++++++++++++++++++++++++++++ library/alloc/benches/lib.rs | 1 + 2 files changed, 92 insertions(+) create mode 100644 library/alloc/benches/binary_heap.rs diff --git a/library/alloc/benches/binary_heap.rs b/library/alloc/benches/binary_heap.rs new file mode 100644 index 0000000000000..5b6538ea6c6b3 --- /dev/null +++ b/library/alloc/benches/binary_heap.rs @@ -0,0 +1,91 @@ +use std::collections::BinaryHeap; + +use rand::{seq::SliceRandom, thread_rng}; +use test::{black_box, Bencher}; + +#[bench] +fn bench_find_smallest_1000(b: &mut Bencher) { + let mut rng = thread_rng(); + let mut vec: Vec = (0..100_000).collect(); + vec.shuffle(&mut rng); + + b.iter(|| { + let mut iter = vec.iter().copied(); + let mut heap: BinaryHeap<_> = iter.by_ref().take(1000).collect(); + + for x in iter { + let mut max = heap.peek_mut().unwrap(); + // This comparison should be true only 1% of the time. + // Unnecessary `sift_down`s will degrade performance + if x < *max { + *max = x; + } + } + + heap + }) +} + +#[bench] +fn bench_peek_mut_deref_mut(b: &mut Bencher) { + let mut bheap = BinaryHeap::from(vec![42]); + let vec: Vec = (0..1_000_000).collect(); + + b.iter(|| { + let vec = black_box(&vec); + let mut peek_mut = bheap.peek_mut().unwrap(); + // The compiler shouldn't be able to optimize away the `sift_down` + // assignment in `PeekMut`'s `DerefMut` implementation since + // the loop may not run. + for &i in vec.iter() { + *peek_mut = i; + } + // Remove the already minimal overhead of the sift_down + std::mem::forget(peek_mut); + }) +} + +#[bench] +fn bench_from_vec(b: &mut Bencher) { + let mut rng = thread_rng(); + let mut vec: Vec = (0..100_000).collect(); + vec.shuffle(&mut rng); + + b.iter(|| BinaryHeap::from(vec.clone())) +} + +#[bench] +fn bench_into_sorted_vec(b: &mut Bencher) { + let bheap: BinaryHeap = (0..10_000).collect(); + + b.iter(|| bheap.clone().into_sorted_vec()) +} + +#[bench] +fn bench_push(b: &mut Bencher) { + let mut bheap = BinaryHeap::with_capacity(50_000); + let mut rng = thread_rng(); + let mut vec: Vec = (0..50_000).collect(); + vec.shuffle(&mut rng); + + b.iter(|| { + for &i in vec.iter() { + bheap.push(i); + } + black_box(&mut bheap); + bheap.clear(); + }) +} + +#[bench] +fn bench_pop(b: &mut Bencher) { + let mut bheap = BinaryHeap::with_capacity(10_000); + + b.iter(|| { + bheap.extend((0..10_000).rev()); + black_box(&mut bheap); + while let Some(elem) = bheap.pop() { + black_box(elem); + } + }) +} diff --git a/library/alloc/benches/lib.rs b/library/alloc/benches/lib.rs index 608eafc88d2a6..32edb86d10119 100644 --- a/library/alloc/benches/lib.rs +++ b/library/alloc/benches/lib.rs @@ -8,6 +8,7 @@ extern crate test; +mod binary_heap; mod btree; mod linked_list; mod slice; From af1e3633f734930a50000cc424fbcdc6629885c3 Mon Sep 17 00:00:00 2001 From: Giacomo Stevanato Date: Wed, 26 Aug 2020 23:52:20 +0200 Subject: [PATCH 19/33] Set sift=true only when PeekMut yields a mutable reference --- library/alloc/src/collections/binary_heap.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/alloc/src/collections/binary_heap.rs b/library/alloc/src/collections/binary_heap.rs index 477a598ff5b00..e3b738a70c888 100644 --- a/library/alloc/src/collections/binary_heap.rs +++ b/library/alloc/src/collections/binary_heap.rs @@ -293,6 +293,7 @@ impl Deref for PeekMut<'_, T> { impl DerefMut for PeekMut<'_, T> { fn deref_mut(&mut self) -> &mut T { debug_assert!(!self.heap.is_empty()); + self.sift = true; // SAFE: PeekMut is only instantiated for non-empty heaps unsafe { self.heap.data.get_unchecked_mut(0) } } @@ -401,7 +402,7 @@ impl BinaryHeap { /// Cost is *O*(1) in the worst case. #[stable(feature = "binary_heap_peek_mut", since = "1.12.0")] pub fn peek_mut(&mut self) -> Option> { - if self.is_empty() { None } else { Some(PeekMut { heap: self, sift: true }) } + if self.is_empty() { None } else { Some(PeekMut { heap: self, sift: false }) } } /// Removes the greatest item from the binary heap and returns it, or `None` if it From ca15e9d8a11e7c25eb85930d1da3009647c1680e Mon Sep 17 00:00:00 2001 From: Giacomo Stevanato Date: Sun, 20 Sep 2020 01:07:34 +0200 Subject: [PATCH 20/33] Fix time complexity in BinaryHeap::peek_mut docs --- library/alloc/src/collections/binary_heap.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/alloc/src/collections/binary_heap.rs b/library/alloc/src/collections/binary_heap.rs index e3b738a70c888..06e5ac037ca79 100644 --- a/library/alloc/src/collections/binary_heap.rs +++ b/library/alloc/src/collections/binary_heap.rs @@ -399,7 +399,8 @@ impl BinaryHeap { /// /// # Time complexity /// - /// Cost is *O*(1) in the worst case. + /// If the item is modified then the worst case time complexity is *O*(log(*n*)), + /// otherwise it's *O*(1). #[stable(feature = "binary_heap_peek_mut", since = "1.12.0")] pub fn peek_mut(&mut self) -> Option> { if self.is_empty() { None } else { Some(PeekMut { heap: self, sift: false }) } From 2a00dda90258576e3adf5ecae0437a8fe6fadbcf Mon Sep 17 00:00:00 2001 From: Bastian Kauschke Date: Thu, 10 Sep 2020 19:43:53 +0200 Subject: [PATCH 21/33] miri: correctly deal with `ConstKind::Bound` --- compiler/rustc_mir/src/interpret/operand.rs | 6 ++-- .../ui/const-generics/issues/issue-73260.rs | 20 +++++++++++++ .../const-generics/issues/issue-73260.stderr | 29 +++++++++++++++++++ .../ui/const-generics/issues/issue-74634.rs | 27 +++++++++++++++++ .../const-generics/issues/issue-74634.stderr | 10 +++++++ 5 files changed, 88 insertions(+), 4 deletions(-) create mode 100644 src/test/ui/const-generics/issues/issue-73260.rs create mode 100644 src/test/ui/const-generics/issues/issue-73260.stderr create mode 100644 src/test/ui/const-generics/issues/issue-74634.rs create mode 100644 src/test/ui/const-generics/issues/issue-74634.stderr diff --git a/compiler/rustc_mir/src/interpret/operand.rs b/compiler/rustc_mir/src/interpret/operand.rs index 57245696e576e..136a2699d20b2 100644 --- a/compiler/rustc_mir/src/interpret/operand.rs +++ b/compiler/rustc_mir/src/interpret/operand.rs @@ -549,7 +549,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { }; // Early-return cases. let val_val = match val.val { - ty::ConstKind::Param(_) => throw_inval!(TooGeneric), + ty::ConstKind::Param(_) | ty::ConstKind::Bound(..) => throw_inval!(TooGeneric), ty::ConstKind::Error(_) => throw_inval!(TypeckError(ErrorReported)), ty::ConstKind::Unevaluated(def, substs, promoted) => { let instance = self.resolve(def.did, substs)?; @@ -561,9 +561,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // happening. return Ok(self.const_eval(GlobalId { instance, promoted }, val.ty)?); } - ty::ConstKind::Infer(..) - | ty::ConstKind::Bound(..) - | ty::ConstKind::Placeholder(..) => { + ty::ConstKind::Infer(..) | ty::ConstKind::Placeholder(..) => { span_bug!(self.cur_span(), "const_to_op: Unexpected ConstKind {:?}", val) } ty::ConstKind::Value(val_val) => val_val, diff --git a/src/test/ui/const-generics/issues/issue-73260.rs b/src/test/ui/const-generics/issues/issue-73260.rs new file mode 100644 index 0000000000000..351d6849af5db --- /dev/null +++ b/src/test/ui/const-generics/issues/issue-73260.rs @@ -0,0 +1,20 @@ +// compile-flags: -Zsave-analysis + +#![feature(const_generics)] +#![allow(incomplete_features)] +struct Arr +where Assert::<{N < usize::max_value() / 2}>: IsTrue, //~ ERROR constant expression +{ +} + +enum Assert {} + +trait IsTrue {} + +impl IsTrue for Assert {} + +fn main() { + let x: Arr<{usize::max_value()}> = Arr {}; + //~^ ERROR mismatched types + //~| ERROR mismatched types +} diff --git a/src/test/ui/const-generics/issues/issue-73260.stderr b/src/test/ui/const-generics/issues/issue-73260.stderr new file mode 100644 index 0000000000000..e22612ed5ea63 --- /dev/null +++ b/src/test/ui/const-generics/issues/issue-73260.stderr @@ -0,0 +1,29 @@ +error: constant expression depends on a generic parameter + --> $DIR/issue-73260.rs:6:47 + | +LL | where Assert::<{N < usize::max_value() / 2}>: IsTrue, + | ^^^^^^ + | + = note: this may fail depending on what value the parameter takes + +error[E0308]: mismatched types + --> $DIR/issue-73260.rs:17:12 + | +LL | let x: Arr<{usize::max_value()}> = Arr {}; + | ^^^^^^^^^^^^^^^^^^^^^^^^^ expected `false`, found `true` + | + = note: expected type `false` + found type `true` + +error[E0308]: mismatched types + --> $DIR/issue-73260.rs:17:40 + | +LL | let x: Arr<{usize::max_value()}> = Arr {}; + | ^^^ expected `false`, found `true` + | + = note: expected type `false` + found type `true` + +error: aborting due to 3 previous errors + +For more information about this error, try `rustc --explain E0308`. diff --git a/src/test/ui/const-generics/issues/issue-74634.rs b/src/test/ui/const-generics/issues/issue-74634.rs new file mode 100644 index 0000000000000..0f23fa92c3679 --- /dev/null +++ b/src/test/ui/const-generics/issues/issue-74634.rs @@ -0,0 +1,27 @@ +#![feature(const_generics)] +#![allow(incomplete_features)] + +trait If {} +impl If for () {} + +trait IsZero { + type Answer; +} + +struct True; +struct False; + +impl IsZero for () +where (): If<{N == 0}> { //~ERROR constant expression + type Answer = True; +} + +trait Foobar {} + +impl Foobar for () +where (): IsZero {} + +impl Foobar for () +where (): IsZero {} + +fn main() {} diff --git a/src/test/ui/const-generics/issues/issue-74634.stderr b/src/test/ui/const-generics/issues/issue-74634.stderr new file mode 100644 index 0000000000000..091a1ac7b9981 --- /dev/null +++ b/src/test/ui/const-generics/issues/issue-74634.stderr @@ -0,0 +1,10 @@ +error: constant expression depends on a generic parameter + --> $DIR/issue-74634.rs:15:11 + | +LL | where (): If<{N == 0}> { + | ^^^^^^^^^^^^ + | + = note: this may fail depending on what value the parameter takes + +error: aborting due to previous error + From 67342304253d8af47fe4453fbe2396b627620431 Mon Sep 17 00:00:00 2001 From: Bastian Kauschke Date: Fri, 11 Sep 2020 18:39:26 +0200 Subject: [PATCH 22/33] do not ICE on `ty::Bound` in Layout::compute --- compiler/rustc_middle/src/ty/layout.rs | 4 ++-- .../ui/const-generics/issues/issue-76595.rs | 18 ++++++++++++++++ .../const-generics/issues/issue-76595.stderr | 21 +++++++++++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-) create mode 100644 src/test/ui/const-generics/issues/issue-76595.rs create mode 100644 src/test/ui/const-generics/issues/issue-76595.stderr diff --git a/compiler/rustc_middle/src/ty/layout.rs b/compiler/rustc_middle/src/ty/layout.rs index b0a1413a9d62f..cb79b089d94a0 100644 --- a/compiler/rustc_middle/src/ty/layout.rs +++ b/compiler/rustc_middle/src/ty/layout.rs @@ -1259,11 +1259,11 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { tcx.layout_raw(param_env.and(normalized))? } - ty::Bound(..) | ty::Placeholder(..) | ty::GeneratorWitness(..) | ty::Infer(_) => { + ty::Placeholder(..) | ty::GeneratorWitness(..) | ty::Infer(_) => { bug!("Layout::compute: unexpected type `{}`", ty) } - ty::Param(_) | ty::Error(_) => { + ty::Bound(..) | ty::Param(_) | ty::Error(_) => { return Err(LayoutError::Unknown(ty)); } }) diff --git a/src/test/ui/const-generics/issues/issue-76595.rs b/src/test/ui/const-generics/issues/issue-76595.rs new file mode 100644 index 0000000000000..0a16ca181f557 --- /dev/null +++ b/src/test/ui/const-generics/issues/issue-76595.rs @@ -0,0 +1,18 @@ +#![feature(const_generics, const_evaluatable_checked)] +#![allow(incomplete_features)] + +struct Bool; + +trait True {} + +impl True for Bool {} + +fn test() where Bool<{core::mem::size_of::() > 4}>: True { + todo!() +} + +fn main() { + test::<2>(); + //~^ ERROR wrong number of type + //~| ERROR constant expression depends +} diff --git a/src/test/ui/const-generics/issues/issue-76595.stderr b/src/test/ui/const-generics/issues/issue-76595.stderr new file mode 100644 index 0000000000000..4235bdc9b893c --- /dev/null +++ b/src/test/ui/const-generics/issues/issue-76595.stderr @@ -0,0 +1,21 @@ +error[E0107]: wrong number of type arguments: expected 1, found 0 + --> $DIR/issue-76595.rs:15:5 + | +LL | test::<2>(); + | ^^^^^^^^^ expected 1 type argument + +error: constant expression depends on a generic parameter + --> $DIR/issue-76595.rs:15:5 + | +LL | fn test() where Bool<{core::mem::size_of::() > 4}>: True { + | ---- required by a bound in this +... +LL | test::<2>(); + | ^^^^^^^^^ + | + = note: this may fail depending on what value the parameter takes + = note: required by this bound in `test` + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0107`. From 65b3419ca04fdf921309e7fc03010d9d2cc9b8f0 Mon Sep 17 00:00:00 2001 From: Bastian Kauschke Date: Sun, 20 Sep 2020 09:32:59 +0200 Subject: [PATCH 23/33] update stderr file --- src/test/ui/const-generics/issues/issue-76595.stderr | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/test/ui/const-generics/issues/issue-76595.stderr b/src/test/ui/const-generics/issues/issue-76595.stderr index 4235bdc9b893c..2e457595393ca 100644 --- a/src/test/ui/const-generics/issues/issue-76595.stderr +++ b/src/test/ui/const-generics/issues/issue-76595.stderr @@ -8,13 +8,12 @@ error: constant expression depends on a generic parameter --> $DIR/issue-76595.rs:15:5 | LL | fn test() where Bool<{core::mem::size_of::() > 4}>: True { - | ---- required by a bound in this + | ---- required by this bound in `test` ... LL | test::<2>(); | ^^^^^^^^^ | = note: this may fail depending on what value the parameter takes - = note: required by this bound in `test` error: aborting due to 2 previous errors From cebbd9fcd35a63569b8fb5c836b5a26089861c41 Mon Sep 17 00:00:00 2001 From: est31 Date: Sun, 20 Sep 2020 10:12:57 +0200 Subject: [PATCH 24/33] Use as_nanos in bench.rs and base.rs --- compiler/rustc_codegen_llvm/src/base.rs | 2 +- library/test/src/bench.rs | 6 +----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/base.rs b/compiler/rustc_codegen_llvm/src/base.rs index 6a1b373ef0711..f35708b1d0965 100644 --- a/compiler/rustc_codegen_llvm/src/base.rs +++ b/compiler/rustc_codegen_llvm/src/base.rs @@ -108,7 +108,7 @@ pub fn compile_codegen_unit( // We assume that the cost to run LLVM on a CGU is proportional to // the time we needed for codegenning it. - let cost = time_to_codegen.as_secs() * 1_000_000_000 + time_to_codegen.subsec_nanos() as u64; + let cost = time_to_codegen.as_nanos() as u64; fn module_codegen(tcx: TyCtxt<'_>, cgu_name: Symbol) -> ModuleCodegen { let cgu = tcx.codegen_unit(cgu_name); diff --git a/library/test/src/bench.rs b/library/test/src/bench.rs index e92e5b9829ec2..b709c76329637 100644 --- a/library/test/src/bench.rs +++ b/library/test/src/bench.rs @@ -98,10 +98,6 @@ fn fmt_thousands_sep(mut n: usize, sep: char) -> String { output } -fn ns_from_dur(dur: Duration) -> u64 { - dur.as_secs() * 1_000_000_000 + (dur.subsec_nanos() as u64) -} - fn ns_iter_inner(inner: &mut F, k: u64) -> u64 where F: FnMut() -> T, @@ -110,7 +106,7 @@ where for _ in 0..k { black_box(inner()); } - ns_from_dur(start.elapsed()) + start.elapsed().as_nanos() as u64 } pub fn iter(inner: &mut F) -> stats::Summary From 43193dcb882466163436057e50c96bb74d9bf50f Mon Sep 17 00:00:00 2001 From: est31 Date: Sun, 20 Sep 2020 10:27:14 +0200 Subject: [PATCH 25/33] Use as_secs_f64 in profiling.rs --- compiler/rustc_data_structures/src/profiling.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/compiler/rustc_data_structures/src/profiling.rs b/compiler/rustc_data_structures/src/profiling.rs index 07d16c6483ec7..363879cbb1d19 100644 --- a/compiler/rustc_data_structures/src/profiling.rs +++ b/compiler/rustc_data_structures/src/profiling.rs @@ -600,10 +600,7 @@ pub fn print_time_passes_entry(do_it: bool, what: &str, dur: Duration) { // Hack up our own formatting for the duration to make it easier for scripts // to parse (always use the same number of decimal places and the same unit). pub fn duration_to_secs_str(dur: std::time::Duration) -> String { - const NANOS_PER_SEC: f64 = 1_000_000_000.0; - let secs = dur.as_secs() as f64 + dur.subsec_nanos() as f64 / NANOS_PER_SEC; - - format!("{:.3}", secs) + format!("{:.3}", dur.as_secs_f64()) } // Memory reporting From 4bc0e55ac4280be80d8cd0f9bc26bd0949f75494 Mon Sep 17 00:00:00 2001 From: est31 Date: Sun, 20 Sep 2020 10:35:23 +0200 Subject: [PATCH 26/33] Replace write_fmt with write! Latter is simpler --- library/test/src/bench.rs | 20 ++++++++++---------- src/tools/unstable-book-gen/src/main.rs | 4 ++-- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/library/test/src/bench.rs b/library/test/src/bench.rs index e92e5b9829ec2..83be1cce63d52 100644 --- a/library/test/src/bench.rs +++ b/library/test/src/bench.rs @@ -61,15 +61,15 @@ pub fn fmt_bench_samples(bs: &BenchSamples) -> String { let median = bs.ns_iter_summ.median as usize; let deviation = (bs.ns_iter_summ.max - bs.ns_iter_summ.min) as usize; - output - .write_fmt(format_args!( - "{:>11} ns/iter (+/- {})", - fmt_thousands_sep(median, ','), - fmt_thousands_sep(deviation, ',') - )) - .unwrap(); + write!( + output, + "{:>11} ns/iter (+/- {})", + fmt_thousands_sep(median, ','), + fmt_thousands_sep(deviation, ',') + ) + .unwrap(); if bs.mb_s != 0 { - output.write_fmt(format_args!(" = {} MB/s", bs.mb_s)).unwrap(); + write!(output, " = {} MB/s", bs.mb_s).unwrap(); } output } @@ -83,9 +83,9 @@ fn fmt_thousands_sep(mut n: usize, sep: char) -> String { let base = 10_usize.pow(pow); if pow == 0 || trailing || n / base != 0 { if !trailing { - output.write_fmt(format_args!("{}", n / base)).unwrap(); + write!(output, "{}", n / base).unwrap(); } else { - output.write_fmt(format_args!("{:03}", n / base)).unwrap(); + write!(output, "{:03}", n / base).unwrap(); } if pow != 0 { output.push(sep); diff --git a/src/tools/unstable-book-gen/src/main.rs b/src/tools/unstable-book-gen/src/main.rs index 387b2acd1069e..e10f72a47b2c4 100644 --- a/src/tools/unstable-book-gen/src/main.rs +++ b/src/tools/unstable-book-gen/src/main.rs @@ -27,12 +27,12 @@ macro_rules! t { fn generate_stub_issue(path: &Path, name: &str, issue: u32) { let mut file = t!(File::create(path)); - t!(file.write_fmt(format_args!(include_str!("stub-issue.md"), name = name, issue = issue))); + t!(write!(file, include_str!("stub-issue.md"), name = name, issue = issue)); } fn generate_stub_no_issue(path: &Path, name: &str) { let mut file = t!(File::create(path)); - t!(file.write_fmt(format_args!(include_str!("stub-no-issue.md"), name = name))); + t!(write!(file, include_str!("stub-no-issue.md"), name = name)); } fn set_to_summary_str(set: &BTreeSet, dir: &str) -> String { From 0e56b5231947c794d891f52abdcd86f31fccf63a Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Sun, 20 Sep 2020 17:31:55 +0200 Subject: [PATCH 27/33] Fix accordingly to review --- library/std/src/thread/local.rs | 42 ++++++++++++++++----------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/library/std/src/thread/local.rs b/library/std/src/thread/local.rs index 4ac1e523a97ea..cba3f1b23fe45 100644 --- a/library/std/src/thread/local.rs +++ b/library/std/src/thread/local.rs @@ -284,13 +284,15 @@ mod lazy { } pub unsafe fn get(&self) -> Option<&'static T> { - // SAFETY: No reference is ever handed out to the inner cell nor - // mutable reference to the Option inside said cell. This make it - // safe to hand a reference, though the lifetime of 'static - // is itself unsafe, making the get method unsafe. + // SAFETY: The caller must ensure no reference is ever handed out to + // the inner cell nor mutable reference to the Option inside said + // cell. This make it safe to hand a reference, though the lifetime + // of 'static is itself unsafe, making the get method unsafe. unsafe { (*self.inner.get()).as_ref() } } + /// The caller must ensure that no reference is active: this method + /// needs unique access. pub unsafe fn initialize T>(&self, init: F) -> &'static T { // Execute the initialization up front, *then* move it into our slot, // just in case initialization fails. @@ -312,20 +314,13 @@ mod lazy { // destructor" flag we just use `mem::replace` which should sequence the // operations a little differently and make this safe to call. // - // `ptr` can be dereferenced safely since it was obtained from - // `UnsafeCell::get`, which should not return a non-aligned or NUL pointer. - // What's more a `LazyKeyInner` can only be created with `new`, which ensures - // `inner` is correctly initialized and all calls to methods on `LazyKeyInner` - // will leave `inner` initialized too. + // The precondition also ensures that we are the only one accessing + // `self` at the moment so replacing is fine. unsafe { let _ = mem::replace(&mut *ptr, Some(value)); } - // SAFETY: the `*ptr` operation is made safe by the `mem::replace` - // call above combined with `ptr` being correct from the beginning - // (see previous SAFETY: comment above). - // - // Plus, with the call to `mem::replace` it is guaranteed there is + // SAFETY: With the call to `mem::replace` it is guaranteed there is // a `Some` behind `ptr`, not a `None` so `unreachable_unchecked` // will never be reached. unsafe { @@ -341,11 +336,12 @@ mod lazy { } } + /// The other methods hand out references while taking &self. + /// As such, callers of this method must ensure no `&` and `&mut` are + /// available and used at the same time. #[allow(unused)] pub unsafe fn take(&mut self) -> Option { - // SAFETY: The other methods hand out references while taking &self. - // As such, callers of this method must ensure no `&` and `&mut` are - // available and used at the same time. + // SAFETY: See doc comment for this method. unsafe { (*self.inner.get()).take() } } } @@ -438,9 +434,10 @@ pub mod fast { // SAFETY: See the definitions of `LazyKeyInner::get` and // `try_initialize` for more informations. // - // The call to `get` is made safe because no mutable references are - // ever handed out and the `try_initialize` is dependant on the - // passed `init` function. + // The caller must ensure no mutable references are ever active to + // the inner cell or the inner T when this is called. + // The `try_initialize` is dependant on the passed `init` function + // for this. unsafe { match self.inner.get() { Some(val) => Some(val), @@ -546,9 +543,10 @@ pub mod os { Key { os: OsStaticKey::new(Some(destroy_value::)), marker: marker::PhantomData } } + /// It is a requirement for the caller to ensure that no mutable + /// reference is active when this method is called. pub unsafe fn get(&'static self, init: fn() -> T) -> Option<&'static T> { - // SAFETY: No mutable references are ever handed out meaning getting - // the value is ok. + // SAFETY: See the documentation for this method. let ptr = unsafe { self.os.get() as *mut Value }; if ptr as usize > 1 { // SAFETY: the check ensured the pointer is safe (its destructor From 08b85a6fc88b4c7b5e02742b5df825b62e168f81 Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Sun, 20 Sep 2020 18:04:12 +0200 Subject: [PATCH 28/33] use iter:: before free functions --- library/core/src/iter/sources.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/library/core/src/iter/sources.rs b/library/core/src/iter/sources.rs index c28538ef027c0..97562cf73b869 100644 --- a/library/core/src/iter/sources.rs +++ b/library/core/src/iter/sources.rs @@ -531,8 +531,10 @@ where /// An iterator where each iteration calls the provided closure `F: FnMut() -> Option`. /// -/// This `struct` is created by the [`from_fn()`] function. +/// This `struct` is created by the [`iter::from_fn()`] function. /// See its documentation for more. +/// +/// [`iter::from_fn()`]: from_fn #[derive(Clone)] #[stable(feature = "iter_from_fn", since = "1.34.0")] pub struct FromFn(F); @@ -581,8 +583,10 @@ where /// An new iterator where each successive item is computed based on the preceding one. /// -/// This `struct` is created by the [`successors()`] function. +/// This `struct` is created by the [`iter::successors()`] function. /// See its documentation for more. +/// +/// [`iter::successors()`]: successors #[derive(Clone)] #[stable(feature = "iter_successors", since = "1.34.0")] pub struct Successors { From 81699895073162cd6731413711a4357dde67d661 Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Sat, 19 Sep 2020 21:32:33 +0200 Subject: [PATCH 29/33] Add non-`unsafe` `.get_mut()` for `UnsafeCell` Update the tracking issue number Updated the documentation for `UnsafeCell` Address review comments Address more review comments + minor changes --- library/core/src/cell.rs | 93 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 84 insertions(+), 9 deletions(-) diff --git a/library/core/src/cell.rs b/library/core/src/cell.rs index cbbfcb4611321..44b863b220051 100644 --- a/library/core/src/cell.rs +++ b/library/core/src/cell.rs @@ -1543,8 +1543,11 @@ impl fmt::Display for RefMut<'_, T> { /// allow internal mutability, such as `Cell` and `RefCell`, use `UnsafeCell` to wrap their /// internal data. There is *no* legal way to obtain aliasing `&mut`, not even with `UnsafeCell`. /// -/// The `UnsafeCell` API itself is technically very simple: it gives you a raw pointer `*mut T` to -/// its contents. It is up to _you_ as the abstraction designer to use that raw pointer correctly. +/// The `UnsafeCell` API itself is technically very simple: [`.get()`] gives you a raw pointer +/// `*mut T` to its contents. It is up to _you_ as the abstraction designer to use that raw pointer +/// correctly. +/// +/// [`.get()`]: `UnsafeCell::get` /// /// The precise Rust aliasing rules are somewhat in flux, but the main points are not contentious: /// @@ -1571,21 +1574,70 @@ impl fmt::Display for RefMut<'_, T> { /// 2. A `&mut T` reference may be released to safe code provided neither other `&mut T` nor `&T` /// co-exist with it. A `&mut T` must always be unique. /// -/// Note that while mutating or mutably aliasing the contents of an `&UnsafeCell` is -/// ok (provided you enforce the invariants some other way), it is still undefined behavior -/// to have multiple `&mut UnsafeCell` aliases. +/// Note that whilst mutating the contents of an `&UnsafeCell` (even while other +/// `&UnsafeCell` references alias the cell) is +/// ok (provided you enforce the above invariants some other way), it is still undefined behavior +/// to have multiple `&mut UnsafeCell` aliases. That is, `UnsafeCell` is a wrapper +/// designed to have a special interaction with _shared_ accesses (_i.e._, through an +/// `&UnsafeCell<_>` reference); there is no magic whatsoever when dealing with _exclusive_ +/// accesses (_e.g._, through an `&mut UnsafeCell<_>`): neither the cell nor the wrapped value +/// may be aliased for the duration of that `&mut` borrow. +/// This is showcased by the [`.get_mut()`] accessor, which is a non-`unsafe` getter that yields +/// a `&mut T`. +/// +/// [`.get_mut()`]: `UnsafeCell::get_mut` /// /// # Examples /// +/// Here is an example showcasing how to soundly mutate the contents of an `UnsafeCell<_>` despite +/// there being multiple references aliasing the cell: +/// /// ``` /// use std::cell::UnsafeCell; /// -/// # #[allow(dead_code)] -/// struct NotThreadSafe { -/// value: UnsafeCell, +/// let x: UnsafeCell = 42.into(); +/// // Get multiple / concurrent / shared references to the same `x`. +/// let (p1, p2): (&UnsafeCell, &UnsafeCell) = (&x, &x); +/// +/// unsafe { +/// // SAFETY: within this scope there are no other references to `x`'s contents, +/// // so ours is effectively unique. +/// let p1_exclusive: &mut i32 = &mut *p1.get(); // -- borrow --+ +/// *p1_exclusive += 27; // | +/// } // <---------- cannot go beyond this point -------------------+ +/// +/// unsafe { +/// // SAFETY: within this scope nobody expects to have exclusive access to `x`'s contents, +/// // so we can have multiple shared accesses concurrently. +/// let p2_shared: &i32 = &*p2.get(); +/// assert_eq!(*p2_shared, 42 + 27); +/// let p1_shared: &i32 = &*p1.get(); +/// assert_eq!(*p1_shared, *p2_shared); /// } +/// ``` +/// +/// The following example showcases the fact that exclusive access to an `UnsafeCell` +/// implies exclusive access to its `T`: /// -/// unsafe impl Sync for NotThreadSafe {} +/// ```rust +/// #![feature(unsafe_cell_get_mut)] +/// #![forbid(unsafe_code)] // with exclusive accesses, +/// // `UnsafeCell` is a transparent no-op wrapper, +/// // so no need for `unsafe` here. +/// use std::cell::UnsafeCell; +/// +/// let mut x: UnsafeCell = 42.into(); +/// +/// // Get a compile-time-checked unique reference to `x`. +/// let p_unique: &mut UnsafeCell = &mut x; +/// // With an exclusive reference, we can mutate the contents for free. +/// *p_unique.get_mut() = 0; +/// // Or, equivalently: +/// x = UnsafeCell::new(0); +/// +/// // When we own the value, we can extract the contents for free. +/// let contents: i32 = x.into_inner(); +/// assert_eq!(contents, 0); /// ``` #[lang = "unsafe_cell"] #[stable(feature = "rust1", since = "1.0.0")] @@ -1663,6 +1715,29 @@ impl UnsafeCell { self as *const UnsafeCell as *const T as *mut T } + /// Returns a mutable reference to the underlying data. + /// + /// This call borrows the `UnsafeCell` mutably (at compile-time) which + /// guarantees that we possess the only reference. + /// + /// # Examples + /// + /// ``` + /// #![feature(unsafe_cell_get_mut)] + /// use std::cell::UnsafeCell; + /// + /// let mut c = UnsafeCell::new(5); + /// *c.get_mut() += 1; + /// + /// assert_eq!(*c.get_mut(), 6); + /// ``` + #[inline] + #[unstable(feature = "unsafe_cell_get_mut", issue = "76943")] + pub fn get_mut(&mut self) -> &mut T { + // SAFETY: (outer) `&mut` guarantees unique access. + unsafe { &mut *self.get() } + } + /// Gets a mutable pointer to the wrapped value. /// The difference to [`get`] is that this function accepts a raw pointer, /// which is useful to avoid the creation of temporary references. From 5886c38112c8bb347b1cbd46c28b1ca6f8bac88d Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Sat, 19 Sep 2020 21:33:40 +0200 Subject: [PATCH 30/33] Replace unneeded `unsafe` calls to `.get()` with calls to `.get_mut()` --- library/core/src/cell.rs | 8 ++------ library/core/src/sync/atomic.rs | 6 ++---- library/std/src/lib.rs | 1 + library/std/src/sync/mutex.rs | 4 +--- library/std/src/sync/rwlock.rs | 4 +--- 5 files changed, 7 insertions(+), 16 deletions(-) diff --git a/library/core/src/cell.rs b/library/core/src/cell.rs index 44b863b220051..f60aa2d24c5ca 100644 --- a/library/core/src/cell.rs +++ b/library/core/src/cell.rs @@ -496,10 +496,7 @@ impl Cell { #[inline] #[stable(feature = "cell_get_mut", since = "1.11.0")] pub fn get_mut(&mut self) -> &mut T { - // SAFETY: This can cause data races if called from a separate thread, - // but `Cell` is `!Sync` so this won't happen, and `&mut` guarantees - // unique access. - unsafe { &mut *self.value.get() } + self.value.get_mut() } /// Returns a `&Cell` from a `&mut T` @@ -945,8 +942,7 @@ impl RefCell { #[inline] #[stable(feature = "cell_get_mut", since = "1.11.0")] pub fn get_mut(&mut self) -> &mut T { - // SAFETY: `&mut` guarantees unique access. - unsafe { &mut *self.value.get() } + self.value.get_mut() } /// Undo the effect of leaked guards on the borrow state of the `RefCell`. diff --git a/library/core/src/sync/atomic.rs b/library/core/src/sync/atomic.rs index 9d74f537491b1..c67d6422c01ec 100644 --- a/library/core/src/sync/atomic.rs +++ b/library/core/src/sync/atomic.rs @@ -838,8 +838,7 @@ impl AtomicPtr { #[inline] #[stable(feature = "atomic_access", since = "1.15.0")] pub fn get_mut(&mut self) -> &mut *mut T { - // SAFETY: the mutable reference guarantees unique ownership. - unsafe { &mut *self.p.get() } + self.p.get_mut() } /// Get atomic access to a pointer. @@ -1275,8 +1274,7 @@ assert_eq!(some_var.load(Ordering::SeqCst), 5); #[inline] #[$stable_access] pub fn get_mut(&mut self) -> &mut $int_type { - // SAFETY: the mutable reference guarantees unique ownership. - unsafe { &mut *self.v.get() } + self.v.get_mut() } } diff --git a/library/std/src/lib.rs b/library/std/src/lib.rs index 5333d75ec1bc5..71b29cf5af99b 100644 --- a/library/std/src/lib.rs +++ b/library/std/src/lib.rs @@ -315,6 +315,7 @@ #![feature(try_reserve)] #![feature(unboxed_closures)] #![feature(unsafe_block_in_unsafe_fn)] +#![feature(unsafe_cell_get_mut)] #![feature(unsafe_cell_raw_get)] #![feature(untagged_unions)] #![feature(unwind_attributes)] diff --git a/library/std/src/sync/mutex.rs b/library/std/src/sync/mutex.rs index 240155b06b411..a1703c731d44d 100644 --- a/library/std/src/sync/mutex.rs +++ b/library/std/src/sync/mutex.rs @@ -406,9 +406,7 @@ impl Mutex { /// ``` #[stable(feature = "mutex_get_mut", since = "1.6.0")] pub fn get_mut(&mut self) -> LockResult<&mut T> { - // We know statically that there are no other references to `self`, so - // there's no need to lock the inner mutex. - let data = unsafe { &mut *self.data.get() }; + let data = self.data.get_mut(); poison::map_result(self.poison.borrow(), |_| data) } } diff --git a/library/std/src/sync/rwlock.rs b/library/std/src/sync/rwlock.rs index f38d6101da0d3..d967779ce361d 100644 --- a/library/std/src/sync/rwlock.rs +++ b/library/std/src/sync/rwlock.rs @@ -404,9 +404,7 @@ impl RwLock { /// ``` #[stable(feature = "rwlock_get_mut", since = "1.6.0")] pub fn get_mut(&mut self) -> LockResult<&mut T> { - // We know statically that there are no other references to `self`, so - // there's no need to lock the inner lock. - let data = unsafe { &mut *self.data.get() }; + let data = self.data.get_mut(); poison::map_result(self.poison.borrow(), |_| data) } } From aaddcdb0d097de1fee14be16479aeaeea41e8810 Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Sun, 20 Sep 2020 18:37:05 +0200 Subject: [PATCH 31/33] Fix nits --- library/std/src/sync/barrier.rs | 4 ++-- library/std/src/sync/once.rs | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/library/std/src/sync/barrier.rs b/library/std/src/sync/barrier.rs index 8d3e30dbd4545..eab26b6c7150c 100644 --- a/library/std/src/sync/barrier.rs +++ b/library/std/src/sync/barrier.rs @@ -150,8 +150,8 @@ impl fmt::Debug for BarrierWaitResult { } impl BarrierWaitResult { - /// Returns `true` if this thread from [`Barrier::wait()`] is the - /// "leader thread". + /// Returns `true` if this thread is the "leader thread" for the call to + /// [`Barrier::wait()`]. /// /// Only one thread will have `true` returned from their result, all other /// threads will have `false` returned. diff --git a/library/std/src/sync/once.rs b/library/std/src/sync/once.rs index ee8902bf764bf..de5ddf1daf27b 100644 --- a/library/std/src/sync/once.rs +++ b/library/std/src/sync/once.rs @@ -95,8 +95,7 @@ use crate::thread::{self, Thread}; /// A synchronization primitive which can be used to run a one-time global /// initialization. Useful for one-time initialization for FFI or related -/// functionality. This type can only be constructed with the [`Once::new()`] -/// constructor. +/// functionality. This type can only be constructed with [`Once::new()`]. /// /// # Examples /// From c9c8fb88cf1be7e0a6bd6fd049d8d28fb5d86135 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sat, 12 Sep 2020 00:42:52 -0400 Subject: [PATCH 32/33] Add sample defaults for config.toml - Allow including defaults in `src/bootstrap/defaults` using `profile = "..."` - Add default config files - Combine config files using the merge dependency. - Add comments to default config files - Add a README asking to open an issue if the defaults are bad - Give a loud error if trying to merge `.target`, since it's not currently supported - Use an exhaustive match - Use `` in config.toml.example to avoid confusion - Fix bugs in `Merge` derives Previously, it would completely ignore the profile defaults if there were any settings in `config.toml`. I sent an email to the `merge` maintainer asking them to make the behavior in this commit the default. This introduces a new dependency on `merge` that hasn't yet been vetted. I want to improve the output when `include = "x"` isn't found: ``` thread 'main' panicked at 'fs::read_to_string(&file) failed with No such file or directory (os error 2) ("configuration file did not exist")', src/bootstrap/config.rs:522:28 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace failed to run: /home/joshua/rustc/build/bootstrap/debug/bootstrap test tidy Build completed unsuccessfully in 0:00:00 ``` However that seems like it could be fixed in a follow-up. --- Cargo.lock | 23 +++++++ config.toml.example | 10 +++ src/bootstrap/Cargo.toml | 1 + src/bootstrap/config.rs | 76 ++++++++++++++------- src/bootstrap/defaults/README.md | 11 +++ src/bootstrap/defaults/config.toml.codegen | 13 ++++ src/bootstrap/defaults/config.toml.compiler | 8 +++ src/bootstrap/defaults/config.toml.library | 10 +++ src/bootstrap/defaults/config.toml.user | 9 +++ 9 files changed, 136 insertions(+), 25 deletions(-) create mode 100644 src/bootstrap/defaults/README.md create mode 100644 src/bootstrap/defaults/config.toml.codegen create mode 100644 src/bootstrap/defaults/config.toml.compiler create mode 100644 src/bootstrap/defaults/config.toml.library create mode 100644 src/bootstrap/defaults/config.toml.user diff --git a/Cargo.lock b/Cargo.lock index d3f777bc663dd..4c55fea30e0e4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -207,6 +207,7 @@ dependencies = [ "ignore", "lazy_static", "libc", + "merge", "num_cpus", "opener", "pretty_assertions", @@ -1900,6 +1901,28 @@ dependencies = [ "autocfg", ] +[[package]] +name = "merge" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "10bbef93abb1da61525bbc45eeaff6473a41907d19f8f9aa5168d214e10693e9" +dependencies = [ + "merge_derive", + "num-traits", +] + +[[package]] +name = "merge_derive" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "209d075476da2e63b4b29e72a2ef627b840589588e71400a25e3565c4f849d07" +dependencies = [ + "proc-macro-error", + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "minifier" version = "0.0.33" diff --git a/config.toml.example b/config.toml.example index 99e6f9dceb41c..8bf1b48ce830e 100644 --- a/config.toml.example +++ b/config.toml.example @@ -9,6 +9,16 @@ # a custom configuration file can also be specified with `--config` to the build # system. +# ============================================================================= +# Global Settings +# ============================================================================= + +# Use different pre-set defaults than the global defaults. +# +# See `src/bootstrap/defaults` for more information. +# Note that this has no default value (x.py uses the defaults in `config.toml.example`). +#profile = + # ============================================================================= # Tweaking how LLVM is compiled # ============================================================================= diff --git a/src/bootstrap/Cargo.toml b/src/bootstrap/Cargo.toml index faec2c53742ec..0177a9dab97e3 100644 --- a/src/bootstrap/Cargo.toml +++ b/src/bootstrap/Cargo.toml @@ -48,6 +48,7 @@ lazy_static = "1.3.0" time = "0.1" ignore = "0.4.10" opener = "0.4" +merge = "0.1.0" [target.'cfg(windows)'.dependencies.winapi] version = "0.3" diff --git a/src/bootstrap/config.rs b/src/bootstrap/config.rs index 7e2cb7721865e..d925af19a8426 100644 --- a/src/bootstrap/config.rs +++ b/src/bootstrap/config.rs @@ -16,6 +16,7 @@ use crate::flags::Flags; pub use crate::flags::Subcommand; use crate::util::exe; use build_helper::t; +use merge::Merge; use serde::Deserialize; macro_rules! check_ci_llvm { @@ -278,10 +279,31 @@ struct TomlConfig { rust: Option, target: Option>, dist: Option, + profile: Option, +} + +impl Merge for TomlConfig { + fn merge(&mut self, TomlConfig { build, install, llvm, rust, dist, target, profile: _ }: Self) { + fn do_merge(x: &mut Option, y: Option) { + if let Some(new) = y { + if let Some(original) = x { + original.merge(new); + } else { + *x = Some(new); + } + } + }; + do_merge(&mut self.build, build); + do_merge(&mut self.install, install); + do_merge(&mut self.llvm, llvm); + do_merge(&mut self.rust, rust); + do_merge(&mut self.dist, dist); + assert!(target.is_none(), "merging target-specific config is not currently supported"); + } } /// TOML representation of various global build decisions. -#[derive(Deserialize, Default, Clone)] +#[derive(Deserialize, Default, Clone, Merge)] #[serde(deny_unknown_fields, rename_all = "kebab-case")] struct Build { build: Option, @@ -321,7 +343,7 @@ struct Build { } /// TOML representation of various global install decisions. -#[derive(Deserialize, Default, Clone)] +#[derive(Deserialize, Default, Clone, Merge)] #[serde(deny_unknown_fields, rename_all = "kebab-case")] struct Install { prefix: Option, @@ -338,7 +360,7 @@ struct Install { } /// TOML representation of how the LLVM build is configured. -#[derive(Deserialize, Default)] +#[derive(Deserialize, Default, Merge)] #[serde(deny_unknown_fields, rename_all = "kebab-case")] struct Llvm { skip_rebuild: Option, @@ -365,7 +387,7 @@ struct Llvm { download_ci_llvm: Option, } -#[derive(Deserialize, Default, Clone)] +#[derive(Deserialize, Default, Clone, Merge)] #[serde(deny_unknown_fields, rename_all = "kebab-case")] struct Dist { sign_folder: Option, @@ -389,7 +411,7 @@ impl Default for StringOrBool { } /// TOML representation of how the Rust build is configured. -#[derive(Deserialize, Default)] +#[derive(Deserialize, Default, Merge)] #[serde(deny_unknown_fields, rename_all = "kebab-case")] struct Rust { optimize: Option, @@ -434,7 +456,7 @@ struct Rust { } /// TOML representation of how each build target is configured. -#[derive(Deserialize, Default)] +#[derive(Deserialize, Default, Merge)] #[serde(deny_unknown_fields, rename_all = "kebab-case")] struct TomlTarget { cc: Option, @@ -523,27 +545,31 @@ impl Config { } #[cfg(test)] - let toml = TomlConfig::default(); + let get_toml = |_| TomlConfig::default(); #[cfg(not(test))] - let toml = flags - .config - .map(|file| { - use std::process; - - let contents = t!(fs::read_to_string(&file)); - match toml::from_str(&contents) { - Ok(table) => table, - Err(err) => { - println!( - "failed to parse TOML configuration '{}': {}", - file.display(), - err - ); - process::exit(2); - } + let get_toml = |file: PathBuf| { + use std::process; + + let contents = t!(fs::read_to_string(&file), "configuration file did not exist"); + match toml::from_str(&contents) { + Ok(table) => table, + Err(err) => { + println!("failed to parse TOML configuration '{}': {}", file.display(), err); + process::exit(2); } - }) - .unwrap_or_else(TomlConfig::default); + } + }; + + let mut toml = flags.config.map(get_toml).unwrap_or_else(TomlConfig::default); + if let Some(include) = &toml.profile { + let mut include_path = config.src.clone(); + include_path.push("src"); + include_path.push("bootstrap"); + include_path.push("defaults"); + include_path.push(format!("config.toml.{}", include)); + let included_toml = get_toml(include_path); + toml.merge(included_toml); + } let build = toml.build.unwrap_or_default(); diff --git a/src/bootstrap/defaults/README.md b/src/bootstrap/defaults/README.md new file mode 100644 index 0000000000000..a91fc3538eb55 --- /dev/null +++ b/src/bootstrap/defaults/README.md @@ -0,0 +1,11 @@ +# About bootstrap defaults + +These defaults are intended to be a good starting point for working with x.py, +with the understanding that no one set of defaults make sense for everyone. + +They are still experimental, and we'd appreciate your help improving them! +If you use a setting that's not in these defaults that you think others would benefit from, please [file an issue] or make a PR with the changes. +Similarly, if one of these defaults doesn't match what you use personally, +please open an issue to get it changed. + +[file an issue]: https://github.com/rust-lang/rust/issues/new/choose diff --git a/src/bootstrap/defaults/config.toml.codegen b/src/bootstrap/defaults/config.toml.codegen new file mode 100644 index 0000000000000..a9505922ca7fc --- /dev/null +++ b/src/bootstrap/defaults/config.toml.codegen @@ -0,0 +1,13 @@ +# These defaults are meant for contributors to the compiler who modify codegen or LLVM +[llvm] +# This enables debug-assertions in LLVM, +# catching logic errors in codegen much earlier in the process. +assertions = true + +[rust] +# This enables `RUSTC_LOG=debug`, avoiding confusing situations +# where adding `debug!()` appears to do nothing. +# However, it makes running the compiler slightly slower. +debug-logging = true +# This greatly increases the speed of rebuilds, especially when there are only minor changes. However, it makes the initial build slightly slower. +incremental = true diff --git a/src/bootstrap/defaults/config.toml.compiler b/src/bootstrap/defaults/config.toml.compiler new file mode 100644 index 0000000000000..4772de8a2cb22 --- /dev/null +++ b/src/bootstrap/defaults/config.toml.compiler @@ -0,0 +1,8 @@ +# These defaults are meant for contributors to the compiler who do not modify codegen or LLVM +[rust] +# This enables `RUSTC_LOG=debug`, avoiding confusing situations +# where adding `debug!()` appears to do nothing. +# However, it makes running the compiler slightly slower. +debug-logging = true +# This greatly increases the speed of rebuilds, especially when there are only minor changes. However, it makes the initial build slightly slower. +incremental = true diff --git a/src/bootstrap/defaults/config.toml.library b/src/bootstrap/defaults/config.toml.library new file mode 100644 index 0000000000000..e4316f4d86440 --- /dev/null +++ b/src/bootstrap/defaults/config.toml.library @@ -0,0 +1,10 @@ +# These defaults are meant for contributors to the standard library and documentation. +[build] +# When building the standard library, you almost never want to build the compiler itself. +build-stage = 0 +test-stage = 0 +bench-stage = 0 + +[rust] +# This greatly increases the speed of rebuilds, especially when there are only minor changes. However, it makes the initial build slightly slower. +incremental = true diff --git a/src/bootstrap/defaults/config.toml.user b/src/bootstrap/defaults/config.toml.user new file mode 100644 index 0000000000000..6647061d88fcb --- /dev/null +++ b/src/bootstrap/defaults/config.toml.user @@ -0,0 +1,9 @@ +# These defaults are meant for users and distro maintainers building from source, without intending to make multiple changes. +[build] +# When compiling from source, you almost always want a full stage 2 build, +# which has all the latest optimizations from nightly. +build-stage = 2 +test-stage = 2 +doc-stage = 2 +# When compiling from source, you usually want all tools. +extended = true From 8e10905bf6983597fd3a540f8beb2c81f43e90b3 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Fri, 11 Sep 2020 22:55:16 -0400 Subject: [PATCH 33/33] Add a changelog for x.py - Add a changelog and instructions for updating it - Use `changelog-seen` in `config.toml` and `VERSION` in bootstrap to determine whether the changelog has been read - Nag people if they haven't read the x.py changelog + Print message twice to make sure it's seen - Give different error messages depending on whether the version needs to be updated or added --- config.toml.example | 6 ++++++ src/bootstrap/CHANGELOG.md | 32 ++++++++++++++++++++++++++++++++ src/bootstrap/README.md | 14 ++++++++++++++ src/bootstrap/bin/main.rs | 35 +++++++++++++++++++++++++++++++++++ src/bootstrap/config.rs | 4 ++++ 5 files changed, 91 insertions(+) create mode 100644 src/bootstrap/CHANGELOG.md diff --git a/config.toml.example b/config.toml.example index 99e6f9dceb41c..7f110c1622851 100644 --- a/config.toml.example +++ b/config.toml.example @@ -9,6 +9,12 @@ # a custom configuration file can also be specified with `--config` to the build # system. +# Keeps track of the last version of `x.py` used. +# If it does not match the version that is currently running, +# `x.py` will prompt you to update it and read the changelog. +# See `src/bootstrap/CHANGELOG.md` for more information. +changelog-seen = 1 + # ============================================================================= # Tweaking how LLVM is compiled # ============================================================================= diff --git a/src/bootstrap/CHANGELOG.md b/src/bootstrap/CHANGELOG.md new file mode 100644 index 0000000000000..208ef18f95df9 --- /dev/null +++ b/src/bootstrap/CHANGELOG.md @@ -0,0 +1,32 @@ +# Changelog + +All notable changes to bootstrap will be documented in this file. + +The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). + +## [Non-breaking changes since the last major version] + +- Add a changelog for x.py [#76626](https://github.com/rust-lang/rust/pull/76626) +- Optionally, download LLVM from CI on Linux and NixOS + + [#76439](https://github.com/rust-lang/rust/pull/76349) + + [#76667](https://github.com/rust-lang/rust/pull/76667) + + [#76708](https://github.com/rust-lang/rust/pull/76708) +- Distribute rustc sources as part of `rustc-dev` [#76856](https://github.com/rust-lang/rust/pull/76856) +- Make the default stage for x.py configurable [#76625](https://github.com/rust-lang/rust/pull/76625) +- Add a dedicated debug-logging option [#76588](https://github.com/rust-lang/rust/pull/76588) + +## [Version 0] - 2020-09-11 + +This is the first changelog entry, and it does not attempt to be an exhaustive list of features in x.py. +Instead, this documents the changes to bootstrap in the past 2 months. + +- Improve defaults in `x.py` [#73964](https://github.com/rust-lang/rust/pull/73964) + (see [blog post] for details) +- Set `ninja = true` by default [#74922](https://github.com/rust-lang/rust/pull/74922) +- Avoid trying to inversely cross-compile for build triple from host triples [#76415](https://github.com/rust-lang/rust/pull/76415) +- Allow blessing expect-tests in tools [#75975](https://github.com/rust-lang/rust/pull/75975) +- `x.py check` checks tests/examples/benches [#76258](https://github.com/rust-lang/rust/pull/76258) +- Fix `rust.use-lld` when linker is not set [#76326](https://github.com/rust-lang/rust/pull/76326) +- Build tests with LLD if `use-lld = true` was passed [#76378](https://github.com/rust-lang/rust/pull/76378) + +[blog post]: https://blog.rust-lang.org/inside-rust/2020/08/30/changes-to-x-py-defaults.html diff --git a/src/bootstrap/README.md b/src/bootstrap/README.md index a69bd1cc3bc53..bc8bae14b210c 100644 --- a/src/bootstrap/README.md +++ b/src/bootstrap/README.md @@ -313,6 +313,20 @@ are: `Config` struct. * Adding a sanity check? Take a look at `bootstrap/sanity.rs`. +If you make a major change, please remember to: + ++ Update `VERSION` in `src/bootstrap/main.rs`. +* Update `changelog-seen = N` in `config.toml.example`. +* Add an entry in `src/bootstrap/CHANGELOG.md`. + +A 'major change' includes + +* A new option or +* A change in the default options. + +Changes that do not affect contributors to the compiler or users +building rustc from source don't need an update to `VERSION`. + If you have any questions feel free to reach out on the `#t-infra` channel in the [Rust Zulip server][rust-zulip] or ask on internals.rust-lang.org. When you encounter bugs, please file issues on the rust-lang/rust issue tracker. diff --git a/src/bootstrap/bin/main.rs b/src/bootstrap/bin/main.rs index b67486c9628cd..f7512aa9fcebd 100644 --- a/src/bootstrap/bin/main.rs +++ b/src/bootstrap/bin/main.rs @@ -12,5 +12,40 @@ use bootstrap::{Build, Config}; fn main() { let args = env::args().skip(1).collect::>(); let config = Config::parse(&args); + + let changelog_suggestion = check_version(&config); + if let Some(suggestion) = &changelog_suggestion { + println!("{}", suggestion); + } + Build::new(config).build(); + + if let Some(suggestion) = changelog_suggestion { + println!("{}", suggestion); + println!("note: this message was printed twice to make it more likely to be seen"); + } +} + +fn check_version(config: &Config) -> Option { + const VERSION: usize = 1; + + let mut msg = String::new(); + + let suggestion = if let Some(seen) = config.changelog_seen { + if seen != VERSION { + msg.push_str("warning: there have been changes to x.py since you last updated.\n"); + format!("update `config.toml` to use `changelog-seen = {}` instead", VERSION) + } else { + return None; + } + } else { + msg.push_str("warning: x.py has made several changes recently you may want to look at\n"); + format!("add `changelog-seen = {}` to `config.toml`", VERSION) + }; + + msg.push_str("help: consider looking at the changes in `src/bootstrap/CHANGELOG.md`\n"); + msg.push_str("note: to silence this warning, "); + msg.push_str(&suggestion); + + Some(msg) } diff --git a/src/bootstrap/config.rs b/src/bootstrap/config.rs index 7e2cb7721865e..b81b2342c12c9 100644 --- a/src/bootstrap/config.rs +++ b/src/bootstrap/config.rs @@ -41,6 +41,7 @@ macro_rules! check_ci_llvm { /// `config.toml.example`. #[derive(Default)] pub struct Config { + pub changelog_seen: Option, pub ccache: Option, /// Call Build::ninja() instead of this. pub ninja_in_file: bool, @@ -272,6 +273,7 @@ impl Target { #[derive(Deserialize, Default)] #[serde(deny_unknown_fields, rename_all = "kebab-case")] struct TomlConfig { + changelog_seen: Option, build: Option, install: Option, llvm: Option, @@ -545,6 +547,8 @@ impl Config { }) .unwrap_or_else(TomlConfig::default); + config.changelog_seen = toml.changelog_seen; + let build = toml.build.unwrap_or_default(); // If --target was specified but --host wasn't specified, don't run any host-only tests.