From f5feb3e3ca739d0f2eeef6e914f97316a05af01d Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sun, 16 Jul 2023 13:11:10 +0000 Subject: Turn copy into moves during DSE. --- tests/codegen/iter-repeat-n-trivial-drop.rs | 2 +- tests/codegen/move-operands.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'tests/codegen') diff --git a/tests/codegen/iter-repeat-n-trivial-drop.rs b/tests/codegen/iter-repeat-n-trivial-drop.rs index 65a0f7e7ffb..24059f190ac 100644 --- a/tests/codegen/iter-repeat-n-trivial-drop.rs +++ b/tests/codegen/iter-repeat-n-trivial-drop.rs @@ -33,7 +33,7 @@ pub fn iter_repeat_n_next(it: &mut std::iter::RepeatN) -> Option Date: Tue, 18 Jul 2023 18:18:17 +0000 Subject: Enable MIR opts for test. --- tests/codegen/move-operands.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'tests/codegen') diff --git a/tests/codegen/move-operands.rs b/tests/codegen/move-operands.rs index 3892740fdc1..df4fbe29ffd 100644 --- a/tests/codegen/move-operands.rs +++ b/tests/codegen/move-operands.rs @@ -1,4 +1,5 @@ -// compile-flags: -C no-prepopulate-passes +// Verify that optimized MIR only copies `a` once. +// compile-flags: -O -C no-prepopulate-passes #![crate_type = "lib"] -- cgit 1.4.1-3-g733a5 From 254bf6027d83a94e93b5112d558c81a3e050413f Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Wed, 19 Jul 2023 09:59:35 +0000 Subject: Make test order-independent. --- tests/codegen/iter-repeat-n-trivial-drop.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tests/codegen') diff --git a/tests/codegen/iter-repeat-n-trivial-drop.rs b/tests/codegen/iter-repeat-n-trivial-drop.rs index 24059f190ac..68b10934a0d 100644 --- a/tests/codegen/iter-repeat-n-trivial-drop.rs +++ b/tests/codegen/iter-repeat-n-trivial-drop.rs @@ -33,7 +33,7 @@ pub fn iter_repeat_n_next(it: &mut std::iter::RepeatN) -> Option Date: Tue, 4 Jul 2023 16:01:16 -0700 Subject: Get `!nonnull` metadata consistently in slice iterators, without needing `assume`s --- library/core/src/ptr/non_null.rs | 24 ++++ library/core/src/slice/iter.rs | 25 ++-- library/core/src/slice/iter/macros.rs | 146 +++++++++++---------- tests/codegen/iter-repeat-n-trivial-drop.rs | 1 + tests/codegen/slice-iter-len-eq-zero.rs | 4 +- tests/codegen/slice-iter-nonnull.rs | 39 ++++++ ...numerated_loop.PreCodegen.after.panic-abort.mir | 4 +- ...umerated_loop.PreCodegen.after.panic-unwind.mir | 4 +- ...r.forward_loop.PreCodegen.after.panic-abort.mir | 4 +- ....forward_loop.PreCodegen.after.panic-unwind.mir | 4 +- ...r.reverse_loop.PreCodegen.after.panic-abort.mir | 4 +- ....reverse_loop.PreCodegen.after.panic-unwind.mir | 4 +- 12 files changed, 170 insertions(+), 93 deletions(-) (limited to 'tests/codegen') diff --git a/library/core/src/ptr/non_null.rs b/library/core/src/ptr/non_null.rs index b492d2f07bc..a8074c8659b 100644 --- a/library/core/src/ptr/non_null.rs +++ b/library/core/src/ptr/non_null.rs @@ -462,6 +462,30 @@ impl NonNull { // And the caller promised the `delta` is sound to add. unsafe { NonNull { pointer: self.pointer.add(delta) } } } + + /// See [`pointer::sub`] for semantics and safety requirements. + #[inline] + pub(crate) const unsafe fn sub(self, delta: usize) -> Self + where + T: Sized, + { + // SAFETY: We require that the delta stays in-bounds of the object, and + // thus it cannot become null, as no legal objects can be allocated + // in such as way that the null address is part of them. + // And the caller promised the `delta` is sound to subtract. + unsafe { NonNull { pointer: self.pointer.sub(delta) } } + } + + /// See [`pointer::sub_ptr`] for semantics and safety requirements. + #[inline] + pub(crate) const unsafe fn sub_ptr(self, subtrahend: Self) -> usize + where + T: Sized, + { + // SAFETY: The caller promised that this is safe to do, and + // the non-nullness is irrelevant to the operation. + unsafe { self.pointer.sub_ptr(subtrahend.pointer) } + } } impl NonNull<[T]> { diff --git a/library/core/src/slice/iter.rs b/library/core/src/slice/iter.rs index 5369fe0a9a9..cc931355318 100644 --- a/library/core/src/slice/iter.rs +++ b/library/core/src/slice/iter.rs @@ -13,7 +13,7 @@ use crate::iter::{ use crate::marker::{PhantomData, Send, Sized, Sync}; use crate::mem::{self, SizedTypeProperties}; use crate::num::NonZeroUsize; -use crate::ptr::{invalid, invalid_mut, NonNull}; +use crate::ptr::{self, invalid, invalid_mut, NonNull}; use super::{from_raw_parts, from_raw_parts_mut}; @@ -68,7 +68,7 @@ pub struct Iter<'a, T: 'a> { /// For non-ZSTs, the non-null pointer to the past-the-end element. /// /// For ZSTs, this is `ptr::invalid(len)`. - end: *const T, + end_or_len: *const T, _marker: PhantomData<&'a T>, } @@ -90,9 +90,9 @@ impl<'a, T> Iter<'a, T> { let ptr = slice.as_ptr(); // SAFETY: Similar to `IterMut::new`. unsafe { - let end = if T::IS_ZST { invalid(slice.len()) } else { ptr.add(slice.len()) }; + let end_or_len = if T::IS_ZST { invalid(slice.len()) } else { ptr.add(slice.len()) }; - Self { ptr: NonNull::new_unchecked(ptr as *mut T), end, _marker: PhantomData } + Self { ptr: NonNull::new_unchecked(ptr as *mut T), end_or_len, _marker: PhantomData } } } @@ -128,7 +128,7 @@ impl<'a, T> Iter<'a, T> { } } -iterator! {struct Iter -> *const T, &'a T, const, {/* no mut */}, { +iterator! {struct Iter -> *const T, &'a T, const, {/* no mut */}, as_ref, { fn is_sorted_by(self, mut compare: F) -> bool where Self: Sized, @@ -142,7 +142,7 @@ iterator! {struct Iter -> *const T, &'a T, const, {/* no mut */}, { impl Clone for Iter<'_, T> { #[inline] fn clone(&self) -> Self { - Iter { ptr: self.ptr, end: self.end, _marker: self._marker } + Iter { ptr: self.ptr, end_or_len: self.end_or_len, _marker: self._marker } } } @@ -189,7 +189,7 @@ pub struct IterMut<'a, T: 'a> { /// For non-ZSTs, the non-null pointer to the past-the-end element. /// /// For ZSTs, this is `ptr::invalid_mut(len)`. - end: *mut T, + end_or_len: *mut T, _marker: PhantomData<&'a mut T>, } @@ -220,15 +220,16 @@ impl<'a, T> IterMut<'a, T> { // for direct pointer equality with `ptr` to check if the iterator is // done. // - // In the case of a ZST, the end pointer is just the start pointer plus - // the length, to also allows for the fast `ptr == end` check. + // In the case of a ZST, the end pointer is just the length. It's never + // used as a pointer at all, and thus it's fine to have no provenance. // // See the `next_unchecked!` and `is_empty!` macros as well as the // `post_inc_start` method for more information. unsafe { - let end = if T::IS_ZST { invalid_mut(slice.len()) } else { ptr.add(slice.len()) }; + let end_or_len = + if T::IS_ZST { invalid_mut(slice.len()) } else { ptr.add(slice.len()) }; - Self { ptr: NonNull::new_unchecked(ptr), end, _marker: PhantomData } + Self { ptr: NonNull::new_unchecked(ptr), end_or_len, _marker: PhantomData } } } @@ -360,7 +361,7 @@ impl AsRef<[T]> for IterMut<'_, T> { // } // } -iterator! {struct IterMut -> *mut T, &'a mut T, mut, {mut}, {}} +iterator! {struct IterMut -> *mut T, &'a mut T, mut, {mut}, as_mut, {}} /// An internal abstraction over the splitting iterators, so that /// splitn, splitn_mut etc can be implemented once. diff --git a/library/core/src/slice/iter/macros.rs b/library/core/src/slice/iter/macros.rs index 96a145e22ed..95bcd123b82 100644 --- a/library/core/src/slice/iter/macros.rs +++ b/library/core/src/slice/iter/macros.rs @@ -1,45 +1,62 @@ //! Macros used by iterators of slice. -// Shrinks the iterator when T is a ZST, setting the length to `new_len`. -// `new_len` must not exceed `self.len()`. -macro_rules! zst_set_len { - ($self: ident, $new_len: expr) => {{ +/// Convenience & performance macro for consuming the `end_or_len` field, by +/// giving a `(&mut) usize` or `(&mut) NonNull` depending whether `T` is +/// or is not a ZST respectively. +/// +/// Internally, this reads the `end` through a pointer-to-`NonNull` so that +/// it'll get the appropriate non-null metadata in the backend without needing +/// to call `assume` manually. +macro_rules! if_zst { + (mut $this:ident, $len:ident => $zst_body:expr, $end:ident => $other_body:expr,) => {{ #![allow(unused_unsafe)] // we're sometimes used within an unsafe block - // SAFETY: same as `invalid(_mut)`, but the macro doesn't know - // which versions of that function to call, so open-code it. - $self.end = unsafe { mem::transmute::($new_len) }; + if T::IS_ZST { + // SAFETY: for ZSTs, the pointer is storing a provenance-free length, + // so consuming and updating it as a `usize` is fine. + let $len = unsafe { &mut *ptr::addr_of_mut!($this.end_or_len).cast::() }; + $zst_body + } else { + // SAFETY: for non-ZSTs, the type invariant ensures it cannot be null + let $end = unsafe { &mut *ptr::addr_of_mut!($this.end_or_len).cast::>() }; + $other_body + } }}; -} + ($this:ident, $len:ident => $zst_body:expr, $end:ident => $other_body:expr,) => {{ + #![allow(unused_unsafe)] // we're sometimes used within an unsafe block -// Shrinks the iterator when T is a ZST, reducing the length by `n`. -// `n` must not exceed `self.len()`. -macro_rules! zst_shrink { - ($self: ident, $n: ident) => { - let new_len = $self.end.addr() - $n; - zst_set_len!($self, new_len); - }; + if T::IS_ZST { + let $len = $this.end_or_len.addr(); + $zst_body + } else { + // SAFETY: for non-ZSTs, the type invariant ensures it cannot be null + let $end = unsafe { *ptr::addr_of!($this.end_or_len).cast::>() }; + $other_body + } + }}; } // Inlining is_empty and len makes a huge performance difference macro_rules! is_empty { ($self: ident) => { - if T::IS_ZST { $self.end.addr() == 0 } else { $self.ptr.as_ptr() as *const _ == $self.end } + if_zst!($self, + len => len == 0, + end => $self.ptr == end, + ) }; } macro_rules! len { ($self: ident) => {{ - #![allow(unused_unsafe)] // we're sometimes used within an unsafe block - - if T::IS_ZST { - $self.end.addr() - } else { - // To get rid of some bounds checks (see `position`), we use ptr_sub instead of - // offset_from (Tested by `codegen/slice-position-bounds-check`.) - // SAFETY: by the type invariant pointers are aligned and `start <= end` - unsafe { $self.end.sub_ptr($self.ptr.as_ptr()) } - } + if_zst!($self, + len => len, + end => { + // To get rid of some bounds checks (see `position`), we use ptr_sub instead of + // offset_from (Tested by `codegen/slice-position-bounds-check`.) + // SAFETY: by the type invariant pointers are aligned and `start <= end` + unsafe { end.sub_ptr($self.ptr) } + }, + ) }}; } @@ -50,20 +67,21 @@ macro_rules! iterator { $elem:ty, $raw_mut:tt, {$( $mut_:tt )?}, + $into_ref:ident, {$($extra:tt)*} ) => { // Returns the first element and moves the start of the iterator forwards by 1. // Greatly improves performance compared to an inlined function. The iterator // must not be empty. macro_rules! next_unchecked { - ($self: ident) => {& $( $mut_ )? *$self.post_inc_start(1)} + ($self: ident) => { $self.post_inc_start(1).$into_ref() } } // Returns the last element and moves the end of the iterator backwards by 1. // Greatly improves performance compared to an inlined function. The iterator // must not be empty. macro_rules! next_back_unchecked { - ($self: ident) => {& $( $mut_ )? *$self.pre_dec_end(1)} + ($self: ident) => { $self.pre_dec_end(1).$into_ref() } } impl<'a, T> $name<'a, T> { @@ -80,33 +98,40 @@ macro_rules! iterator { // returning the old start. // Unsafe because the offset must not exceed `self.len()`. #[inline(always)] - unsafe fn post_inc_start(&mut self, offset: usize) -> * $raw_mut T { + unsafe fn post_inc_start(&mut self, offset: usize) -> NonNull { let old = self.ptr; - if T::IS_ZST { - zst_shrink!(self, offset); - } else { - // SAFETY: the caller guarantees that `offset` doesn't exceed `self.len()`, - // so this new pointer is inside `self` and thus guaranteed to be non-null. - self.ptr = unsafe { self.ptr.add(offset) }; + + // SAFETY: the caller guarantees that `offset` doesn't exceed `self.len()`, + // so this new pointer is inside `self` and thus guaranteed to be non-null. + unsafe { + if_zst!(mut self, + len => *len = len.unchecked_sub(offset), + _end => self.ptr = self.ptr.add(offset), + ); } - old.as_ptr() + old } // Helper function for moving the end of the iterator backwards by `offset` elements, // returning the new end. // Unsafe because the offset must not exceed `self.len()`. #[inline(always)] - unsafe fn pre_dec_end(&mut self, offset: usize) -> * $raw_mut T { - if T::IS_ZST { - zst_shrink!(self, offset); - self.ptr.as_ptr() - } else { + unsafe fn pre_dec_end(&mut self, offset: usize) -> NonNull { + if_zst!(mut self, + // SAFETY: By our precondition, `offset` can be at most the + // current length, so the subtraction can never overflow. + len => unsafe { + *len = len.unchecked_sub(offset); + self.ptr + }, // SAFETY: the caller guarantees that `offset` doesn't exceed `self.len()`, // which is guaranteed to not overflow an `isize`. Also, the resulting pointer // is in bounds of `slice`, which fulfills the other requirements for `offset`. - self.end = unsafe { self.end.sub(offset) }; - self.end - } + end => unsafe { + *end = end.sub(offset); + *end + }, + ) } } @@ -131,13 +156,9 @@ macro_rules! iterator { fn next(&mut self) -> Option<$elem> { // could be implemented with slices, but this avoids bounds checks - // SAFETY: `assume` call is safe because slices over non-ZSTs must - // have a non-null end pointer. The call to `next_unchecked!` is + // SAFETY: The call to `next_unchecked!` is // safe since we check if the iterator is empty first. unsafe { - if !::IS_ZST { - assume(!self.end.is_null()); - } if is_empty!(self) { None } else { @@ -161,14 +182,10 @@ macro_rules! iterator { fn nth(&mut self, n: usize) -> Option<$elem> { if n >= len!(self) { // This iterator is now empty. - if T::IS_ZST { - zst_set_len!(self, 0); - } else { - // SAFETY: end can't be 0 if T isn't ZST because ptr isn't 0 and end >= ptr - unsafe { - self.ptr = NonNull::new_unchecked(self.end as *mut T); - } - } + if_zst!(mut self, + len => *len = 0, + end => self.ptr = *end, + ); return None; } // SAFETY: We are in bounds. `post_inc_start` does the right thing even for ZSTs. @@ -375,13 +392,9 @@ macro_rules! iterator { fn next_back(&mut self) -> Option<$elem> { // could be implemented with slices, but this avoids bounds checks - // SAFETY: `assume` call is safe because slices over non-ZSTs must - // have a non-null end pointer. The call to `next_back_unchecked!` + // SAFETY: The call to `next_back_unchecked!` // is safe since we check if the iterator is empty first. unsafe { - if !::IS_ZST { - assume(!self.end.is_null()); - } if is_empty!(self) { None } else { @@ -394,11 +407,10 @@ macro_rules! iterator { fn nth_back(&mut self, n: usize) -> Option<$elem> { if n >= len!(self) { // This iterator is now empty. - if T::IS_ZST { - zst_set_len!(self, 0); - } else { - self.end = self.ptr.as_ptr(); - } + if_zst!(mut self, + len => *len = 0, + end => *end = self.ptr, + ); return None; } // SAFETY: We are in bounds. `pre_dec_end` does the right thing even for ZSTs. diff --git a/tests/codegen/iter-repeat-n-trivial-drop.rs b/tests/codegen/iter-repeat-n-trivial-drop.rs index 68b10934a0d..0b08e578151 100644 --- a/tests/codegen/iter-repeat-n-trivial-drop.rs +++ b/tests/codegen/iter-repeat-n-trivial-drop.rs @@ -34,6 +34,7 @@ pub fn iter_repeat_n_next(it: &mut std::iter::RepeatN) -> Option) -> bool { // CHECK-NOT: sub - // CHECK: %_0 = icmp eq {{i8\*|ptr}} {{%1|%0}}, {{%1|%0}} - // CHECK: ret i1 %_0 + // CHECK: %[[RET:.+]] = icmp eq {{i8\*|ptr}} {{%1|%0}}, {{%1|%0}} + // CHECK: ret i1 %[[RET]] y.len() == 0 } diff --git a/tests/codegen/slice-iter-nonnull.rs b/tests/codegen/slice-iter-nonnull.rs index 997bdaf5636..f7d164bc856 100644 --- a/tests/codegen/slice-iter-nonnull.rs +++ b/tests/codegen/slice-iter-nonnull.rs @@ -2,11 +2,16 @@ // compile-flags: -O // ignore-debug (these add extra checks that make it hard to verify) #![crate_type = "lib"] +#![feature(exact_size_is_empty)] // The slice iterator used to `assume` that the `start` pointer was non-null. // That ought to be unneeded, though, since the type is `NonNull`, so this test // confirms that the appropriate metadata is included to denote that. +// It also used to `assume` the `end` pointer was non-null, but that's no longer +// needed as the code changed to read it as a `NonNull`, and thus gets the +// appropriate `!nonnull` annotations naturally. + // CHECK-LABEL: @slice_iter_next( #[no_mangle] pub fn slice_iter_next<'a>(it: &mut std::slice::Iter<'a, u32>) -> Option<&'a u32> { @@ -75,3 +80,37 @@ pub fn slice_iter_mut_new(slice: &mut [u32]) -> std::slice::IterMut<'_, u32> { // CHECK: } slice.iter_mut() } + +// CHECK-LABEL: @slice_iter_is_empty +#[no_mangle] +pub fn slice_iter_is_empty(it: &std::slice::Iter<'_, u32>) -> bool { + // CHECK: %[[ENDP:.+]] = getelementptr{{.+}}ptr %it,{{.+}} 1 + // CHECK: %[[END:.+]] = load ptr, ptr %[[ENDP]] + // CHECK-SAME: !nonnull + // CHECK-SAME: !noundef + // CHECK: %[[START:.+]] = load ptr, ptr %it, + // CHECK-SAME: !nonnull + // CHECK-SAME: !noundef + + // CHECK: %[[RET:.+]] = icmp eq ptr %[[START]], %[[END]] + // CHECK: ret i1 %[[RET]] + it.is_empty() +} + +// CHECK-LABEL: @slice_iter_len +#[no_mangle] +pub fn slice_iter_len(it: &std::slice::Iter<'_, u32>) -> usize { + // CHECK: %[[START:.+]] = load ptr, ptr %it, + // CHECK-SAME: !nonnull + // CHECK-SAME: !noundef + // CHECK: %[[ENDP:.+]] = getelementptr{{.+}}ptr %it,{{.+}} 1 + // CHECK: %[[END:.+]] = load ptr, ptr %[[ENDP]] + // CHECK-SAME: !nonnull + // CHECK-SAME: !noundef + + // CHECK: ptrtoint + // CHECK: ptrtoint + // CHECK: sub nuw + // CHECK: lshr exact + it.len() +} diff --git a/tests/mir-opt/pre-codegen/slice_iter.enumerated_loop.PreCodegen.after.panic-abort.mir b/tests/mir-opt/pre-codegen/slice_iter.enumerated_loop.PreCodegen.after.panic-abort.mir index 2cf81d86267..89009864c32 100644 --- a/tests/mir-opt/pre-codegen/slice_iter.enumerated_loop.PreCodegen.after.panic-abort.mir +++ b/tests/mir-opt/pre-codegen/slice_iter.enumerated_loop.PreCodegen.after.panic-abort.mir @@ -38,7 +38,7 @@ fn enumerated_loop(_1: &[T], _2: impl Fn(usize, &T)) -> () { scope 6 { let _7: *const T; scope 7 { - debug end => _7; + debug end_or_len => _7; scope 13 (inlined NonNull::::new_unchecked) { debug ptr => _9; let mut _10: *const T; @@ -138,7 +138,7 @@ fn enumerated_loop(_1: &[T], _2: impl Fn(usize, &T)) -> () { StorageDead(_9); StorageLive(_12); _12 = _7; - _13 = std::slice::Iter::<'_, T> { ptr: move _11, end: move _12, _marker: const ZeroSized: PhantomData<&T> }; + _13 = std::slice::Iter::<'_, T> { ptr: move _11, end_or_len: move _12, _marker: const ZeroSized: PhantomData<&T> }; StorageDead(_12); StorageDead(_11); StorageDead(_7); diff --git a/tests/mir-opt/pre-codegen/slice_iter.enumerated_loop.PreCodegen.after.panic-unwind.mir b/tests/mir-opt/pre-codegen/slice_iter.enumerated_loop.PreCodegen.after.panic-unwind.mir index 6985806ec93..836fa2677b1 100644 --- a/tests/mir-opt/pre-codegen/slice_iter.enumerated_loop.PreCodegen.after.panic-unwind.mir +++ b/tests/mir-opt/pre-codegen/slice_iter.enumerated_loop.PreCodegen.after.panic-unwind.mir @@ -38,7 +38,7 @@ fn enumerated_loop(_1: &[T], _2: impl Fn(usize, &T)) -> () { scope 6 { let _7: *const T; scope 7 { - debug end => _7; + debug end_or_len => _7; scope 13 (inlined NonNull::::new_unchecked) { debug ptr => _9; let mut _10: *const T; @@ -138,7 +138,7 @@ fn enumerated_loop(_1: &[T], _2: impl Fn(usize, &T)) -> () { StorageDead(_9); StorageLive(_12); _12 = _7; - _13 = std::slice::Iter::<'_, T> { ptr: move _11, end: move _12, _marker: const ZeroSized: PhantomData<&T> }; + _13 = std::slice::Iter::<'_, T> { ptr: move _11, end_or_len: move _12, _marker: const ZeroSized: PhantomData<&T> }; StorageDead(_12); StorageDead(_11); StorageDead(_7); diff --git a/tests/mir-opt/pre-codegen/slice_iter.forward_loop.PreCodegen.after.panic-abort.mir b/tests/mir-opt/pre-codegen/slice_iter.forward_loop.PreCodegen.after.panic-abort.mir index a4b8460e98e..146fa57a0b1 100644 --- a/tests/mir-opt/pre-codegen/slice_iter.forward_loop.PreCodegen.after.panic-abort.mir +++ b/tests/mir-opt/pre-codegen/slice_iter.forward_loop.PreCodegen.after.panic-abort.mir @@ -35,7 +35,7 @@ fn forward_loop(_1: &[T], _2: impl Fn(&T)) -> () { scope 6 { let _7: *const T; scope 7 { - debug end => _7; + debug end_or_len => _7; scope 13 (inlined NonNull::::new_unchecked) { debug ptr => _9; let mut _10: *const T; @@ -128,7 +128,7 @@ fn forward_loop(_1: &[T], _2: impl Fn(&T)) -> () { StorageDead(_9); StorageLive(_12); _12 = _7; - _13 = std::slice::Iter::<'_, T> { ptr: move _11, end: move _12, _marker: const ZeroSized: PhantomData<&T> }; + _13 = std::slice::Iter::<'_, T> { ptr: move _11, end_or_len: move _12, _marker: const ZeroSized: PhantomData<&T> }; StorageDead(_12); StorageDead(_11); StorageDead(_7); diff --git a/tests/mir-opt/pre-codegen/slice_iter.forward_loop.PreCodegen.after.panic-unwind.mir b/tests/mir-opt/pre-codegen/slice_iter.forward_loop.PreCodegen.after.panic-unwind.mir index 58f312b1aac..65baaf64a9e 100644 --- a/tests/mir-opt/pre-codegen/slice_iter.forward_loop.PreCodegen.after.panic-unwind.mir +++ b/tests/mir-opt/pre-codegen/slice_iter.forward_loop.PreCodegen.after.panic-unwind.mir @@ -35,7 +35,7 @@ fn forward_loop(_1: &[T], _2: impl Fn(&T)) -> () { scope 6 { let _7: *const T; scope 7 { - debug end => _7; + debug end_or_len => _7; scope 13 (inlined NonNull::::new_unchecked) { debug ptr => _9; let mut _10: *const T; @@ -128,7 +128,7 @@ fn forward_loop(_1: &[T], _2: impl Fn(&T)) -> () { StorageDead(_9); StorageLive(_12); _12 = _7; - _13 = std::slice::Iter::<'_, T> { ptr: move _11, end: move _12, _marker: const ZeroSized: PhantomData<&T> }; + _13 = std::slice::Iter::<'_, T> { ptr: move _11, end_or_len: move _12, _marker: const ZeroSized: PhantomData<&T> }; StorageDead(_12); StorageDead(_11); StorageDead(_7); diff --git a/tests/mir-opt/pre-codegen/slice_iter.reverse_loop.PreCodegen.after.panic-abort.mir b/tests/mir-opt/pre-codegen/slice_iter.reverse_loop.PreCodegen.after.panic-abort.mir index b550711aa41..a5df36ca388 100644 --- a/tests/mir-opt/pre-codegen/slice_iter.reverse_loop.PreCodegen.after.panic-abort.mir +++ b/tests/mir-opt/pre-codegen/slice_iter.reverse_loop.PreCodegen.after.panic-abort.mir @@ -39,7 +39,7 @@ fn reverse_loop(_1: &[T], _2: impl Fn(&T)) -> () { scope 6 { let _7: *const T; scope 7 { - debug end => _7; + debug end_or_len => _7; scope 13 (inlined NonNull::::new_unchecked) { debug ptr => _9; let mut _10: *const T; @@ -139,7 +139,7 @@ fn reverse_loop(_1: &[T], _2: impl Fn(&T)) -> () { StorageDead(_9); StorageLive(_12); _12 = _7; - _13 = std::slice::Iter::<'_, T> { ptr: move _11, end: move _12, _marker: const ZeroSized: PhantomData<&T> }; + _13 = std::slice::Iter::<'_, T> { ptr: move _11, end_or_len: move _12, _marker: const ZeroSized: PhantomData<&T> }; StorageDead(_12); StorageDead(_11); StorageDead(_7); diff --git a/tests/mir-opt/pre-codegen/slice_iter.reverse_loop.PreCodegen.after.panic-unwind.mir b/tests/mir-opt/pre-codegen/slice_iter.reverse_loop.PreCodegen.after.panic-unwind.mir index 23444241cd2..f681da4d275 100644 --- a/tests/mir-opt/pre-codegen/slice_iter.reverse_loop.PreCodegen.after.panic-unwind.mir +++ b/tests/mir-opt/pre-codegen/slice_iter.reverse_loop.PreCodegen.after.panic-unwind.mir @@ -39,7 +39,7 @@ fn reverse_loop(_1: &[T], _2: impl Fn(&T)) -> () { scope 6 { let _7: *const T; scope 7 { - debug end => _7; + debug end_or_len => _7; scope 13 (inlined NonNull::::new_unchecked) { debug ptr => _9; let mut _10: *const T; @@ -139,7 +139,7 @@ fn reverse_loop(_1: &[T], _2: impl Fn(&T)) -> () { StorageDead(_9); StorageLive(_12); _12 = _7; - _13 = std::slice::Iter::<'_, T> { ptr: move _11, end: move _12, _marker: const ZeroSized: PhantomData<&T> }; + _13 = std::slice::Iter::<'_, T> { ptr: move _11, end_or_len: move _12, _marker: const ZeroSized: PhantomData<&T> }; StorageDead(_12); StorageDead(_11); StorageDead(_7); -- cgit 1.4.1-3-g733a5 From 74b8d324eb77a8f337b35dc68ac91b0c2c06debc Mon Sep 17 00:00:00 2001 From: Miguel Ojeda Date: Sun, 29 May 2022 01:10:44 +0200 Subject: Support `.comment` section like GCC/Clang (`!llvm.ident`) Both GCC and Clang write by default a `.comment` section with compiler information: ```txt $ gcc -c -xc /dev/null && readelf -p '.comment' null.o String dump of section '.comment': [ 1] GCC: (GNU) 11.2.0 $ clang -c -xc /dev/null && readelf -p '.comment' null.o String dump of section '.comment': [ 1] clang version 14.0.1 (https://github.com/llvm/llvm-project.git c62053979489ccb002efe411c3af059addcb5d7d) ``` They also implement the `-Qn` flag to avoid doing so: ```txt $ gcc -Qn -c -xc /dev/null && readelf -p '.comment' null.o readelf: Warning: Section '.comment' was not dumped because it does not exist! $ clang -Qn -c -xc /dev/null && readelf -p '.comment' null.o readelf: Warning: Section '.comment' was not dumped because it does not exist! ``` So far, `rustc` only does it for WebAssembly targets and only when debug info is enabled: ```txt $ echo 'fn main(){}' | rustc --target=wasm32-unknown-unknown --emit=llvm-ir -Cdebuginfo=2 - && grep llvm.ident rust_out.ll !llvm.ident = !{!27} ``` In the RFC part of this PR it was decided to always add the information, which gets us closer to other popular compilers. An opt-out flag like GCC and Clang may be added later on if deemed necessary. Implementation-wise, this covers both `ModuleLlvm::new()` and `ModuleLlvm::new_metadata()` cases by moving the addition to `context::create_module` and adds a few test cases. ThinLTO also sees the `llvm.ident` named metadata duplicated (in temporary outputs), so this deduplicates it like it is done for `wasm.custom_sections`. The tests also check this duplication does not take place. Signed-off-by: Miguel Ojeda --- compiler/rustc_codegen_llvm/src/context.rs | 18 ++++++++++++++++++ compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs | 15 --------------- compiler/rustc_llvm/llvm-wrapper/PassWrapper.cpp | 5 +++++ tests/codegen/llvm-ident.rs | 15 +++++++++++++++ tests/run-make/comment-section/Makefile | 15 +++++++++++++++ tests/run-make/llvm-ident/Makefile | 19 +++++++++++++++++++ 6 files changed, 72 insertions(+), 15 deletions(-) create mode 100644 tests/codegen/llvm-ident.rs create mode 100644 tests/run-make/comment-section/Makefile create mode 100644 tests/run-make/llvm-ident/Makefile (limited to 'tests/codegen') diff --git a/compiler/rustc_codegen_llvm/src/context.rs b/compiler/rustc_codegen_llvm/src/context.rs index e1e0a442845..5bf641055c5 100644 --- a/compiler/rustc_codegen_llvm/src/context.rs +++ b/compiler/rustc_codegen_llvm/src/context.rs @@ -33,6 +33,7 @@ use rustc_target::abi::{ use rustc_target::spec::{HasTargetSpec, RelocModel, Target, TlsModel}; use smallvec::SmallVec; +use libc::c_uint; use std::cell::{Cell, RefCell}; use std::ffi::CStr; use std::str; @@ -349,6 +350,23 @@ pub unsafe fn create_module<'ll>( ); } + // Insert `llvm.ident` metadata. + // + // On the wasm targets it will get hooked up to the "producer" sections + // `processed-by` information. + let rustc_producer = + format!("rustc version {}", option_env!("CFG_VERSION").expect("CFG_VERSION")); + let name_metadata = llvm::LLVMMDStringInContext( + llcx, + rustc_producer.as_ptr().cast(), + rustc_producer.as_bytes().len() as c_uint, + ); + llvm::LLVMAddNamedMetadataOperand( + llmod, + cstr!("llvm.ident").as_ptr(), + llvm::LLVMMDNodeInContext(llcx, &name_metadata, 1), + ); + llmod } diff --git a/compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs b/compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs index c6996f2e16a..905e0e541a8 100644 --- a/compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs +++ b/compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs @@ -888,21 +888,6 @@ pub fn build_compile_unit_di_node<'ll, 'tcx>( llvm::LLVMAddNamedMetadataOperand(debug_context.llmod, llvm_gcov_ident.as_ptr(), val); } - // Insert `llvm.ident` metadata on the wasm targets since that will - // get hooked up to the "producer" sections `processed-by` information. - if tcx.sess.target.is_like_wasm { - let name_metadata = llvm::LLVMMDStringInContext( - debug_context.llcontext, - rustc_producer.as_ptr().cast(), - rustc_producer.as_bytes().len() as c_uint, - ); - llvm::LLVMAddNamedMetadataOperand( - debug_context.llmod, - cstr!("llvm.ident").as_ptr(), - llvm::LLVMMDNodeInContext(debug_context.llcontext, &name_metadata, 1), - ); - } - return unit_metadata; }; diff --git a/compiler/rustc_llvm/llvm-wrapper/PassWrapper.cpp b/compiler/rustc_llvm/llvm-wrapper/PassWrapper.cpp index e5fb6b0953f..71df17c9ce7 100644 --- a/compiler/rustc_llvm/llvm-wrapper/PassWrapper.cpp +++ b/compiler/rustc_llvm/llvm-wrapper/PassWrapper.cpp @@ -1366,6 +1366,11 @@ LLVMRustPrepareThinLTOImport(const LLVMRustThinLTOData *Data, LLVMModuleRef M, if (WasmCustomSections) WasmCustomSections->eraseFromParent(); + // `llvm.ident` named metadata also gets duplicated. + auto *llvmIdent = (*MOrErr)->getNamedMetadata("llvm.ident"); + if (llvmIdent) + llvmIdent->eraseFromParent(); + return MOrErr; }; bool ClearDSOLocal = clearDSOLocalOnDeclarations(Mod, Target); diff --git a/tests/codegen/llvm-ident.rs b/tests/codegen/llvm-ident.rs new file mode 100644 index 00000000000..927f0d602ad --- /dev/null +++ b/tests/codegen/llvm-ident.rs @@ -0,0 +1,15 @@ +// Verifies that the `!llvm.ident` named metadata is emitted. +// +// revisions: NONE OPT DEBUG +// +// [OPT] compile-flags: -Copt-level=2 +// [DEBUG] compile-flags: -Cdebuginfo=2 + +// The named metadata should contain a single metadata node (see +// `LLVMRustPrepareThinLTOImport` for details). +// CHECK: !llvm.ident = !{![[ID:[0-9]+]]} + +// In addition, check that the metadata node has the expected content. +// CHECK: ![[ID]] = !{!"rustc version 1.{{.*}}"} + +fn main() {} diff --git a/tests/run-make/comment-section/Makefile b/tests/run-make/comment-section/Makefile new file mode 100644 index 00000000000..9f810063cc8 --- /dev/null +++ b/tests/run-make/comment-section/Makefile @@ -0,0 +1,15 @@ +include ../tools.mk + +# only-linux + +all: + echo 'fn main(){}' | $(RUSTC) - --emit=link,obj -Csave-temps --target=$(TARGET) + + # Check linked output has a `.comment` section with the expected content. + readelf -p '.comment' $(TMPDIR)/rust_out | $(CGREP) -F 'rustc version 1.' + + # Check all object files (including temporary outputs) have a `.comment` + # section with the expected content. + set -e; for f in $(TMPDIR)/*.o; do \ + readelf -p '.comment' $$f | $(CGREP) -F 'rustc version 1.'; \ + done diff --git a/tests/run-make/llvm-ident/Makefile b/tests/run-make/llvm-ident/Makefile new file mode 100644 index 00000000000..e583e6018e0 --- /dev/null +++ b/tests/run-make/llvm-ident/Makefile @@ -0,0 +1,19 @@ +include ../tools.mk + +# only-linux + +all: + # `-Ccodegen-units=16 -Copt-level=2` is used here to trigger thin LTO + # across codegen units to test deduplication of the named metadata + # (see `LLVMRustPrepareThinLTOImport` for details). + echo 'fn main(){}' | $(RUSTC) - --emit=link,obj -Csave-temps -Ccodegen-units=16 -Copt-level=2 --target=$(TARGET) + + # `llvm-dis` is used here since `--emit=llvm-ir` does not emit LLVM IR + # for temporary outputs. + "$(LLVM_BIN_DIR)"/llvm-dis $(TMPDIR)/*.bc + + # Check LLVM IR files (including temporary outputs) have `!llvm.ident` + # named metadata, reusing the related codegen test. + set -e; for f in $(TMPDIR)/*.ll; do \ + $(LLVM_FILECHECK) --input-file $$f ../../codegen/llvm-ident.rs; \ + done -- cgit 1.4.1-3-g733a5