-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prepare Rust for opaque pointers #94214
Conversation
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 52196c92d12aa9221b27ee2e4276cd60ae9e387c with merge 3510be61bf5fddada12fcce24c6b7b1b1e3cc06a... |
☀️ Try build successful - checks-actions |
Queued 3510be61bf5fddada12fcce24c6b7b1b1e3cc06a with parent 026d8ce, future comparison URL. |
Finished benchmarking commit (3510be61bf5fddada12fcce24c6b7b1b1e3cc06a): comparison url. Summary: This benchmark run shows 149 relevant improvements 🎉 but 159 relevant regressions 😿 to instruction counts.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never |
@nikic Would it make sense to do another perf run with opaque pointers toggled off, but the same EDIT: oh, wait, opaque pointers are coming entirely from the submodule switch? I guess in that case my idea would require adding a way to control it through FFI, sorry I missed that aspect at first. |
@@ -7,4 +7,4 @@ | |||
fn main() { | |||
} | |||
|
|||
// CHECK: define i32 @main(i32{{( %0)?}}, i8**{{( %1)?}}) | |||
// CHECK: define i32 @main(i32{{( %0)?}}, {{i8**|ptr}}{{( %1)?}}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably needs i8\*\*
since it's inside a regex, right? (Similar thing in other files)
52196c9
to
d5dfac2
Compare
This comment has been minimized.
This comment has been minimized.
8d6d821
to
d27788c
Compare
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit d27788cf1775f500a171d9a9d389408f6110b181 with merge 622e70f20deebb450e36b82cbabaa1a359e3b228... |
☀️ Try build successful - checks-actions |
Queued 622e70f20deebb450e36b82cbabaa1a359e3b228 with parent 9f8f0a6, future comparison URL. |
Finished benchmarking commit (622e70f20deebb450e36b82cbabaa1a359e3b228): comparison url. Summary: This benchmark run shows 134 relevant improvements 🎉 but 163 relevant regressions 😿 to instruction counts.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never |
Testing baseline without opaque pointers now... @bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
r? @cuviper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whew, it will be a happy day when we can unify on ptr
in all those tests...
@@ -78,7 +78,7 @@ pub fn array_u8_1(f: fn(*const u8)) { | |||
#[no_mangle] | |||
pub fn array_u8_small(f: fn(*const u8)) { | |||
let a = [0u8; 2]; | |||
let b = [0u8; 8]; | |||
let b = [0u8; 7]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change somehow related to opaque pointers? Or is it something else incidental on llvm main? I don't see a problem with the change, but it seems curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intended to test a heuristic that does not place a stack protector for arrays (strictly) smaller than 8, so it should be using size 7. The test still worked previously, because the [8 x i8]
alloca was converted into an i64
alloca, and did not receive the stack protector for that reason. This doesn't happen anymore with opaque pointers. The way these heuristics are currently implemented (inspecting alloca types in the backend) is unfortunately not stable under optimization (probably it should be a frontend responsibility to add metadata to allocas that should receive a protector, or something like that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation says that it is for any N > 8, not any N >= 8:
rust/compiler/rustc_codegen_llvm/src/lib.rs
Line 308 in ed76b77
- local variables of `[T; N]` type, where `T` is byte-sized and `N` > 8. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the documentation is wrong as well. I'll fix it. For reference, the actual logic is here: https://github.com/llvm/llvm-project/blob/65af17c7a00df3f8906cded34b63b309e687cf61/llvm/lib/CodeGen/StackProtector.cpp#L134
A stack protector is used for N >= 8, not N > 8.
@bors r+ |
📌 Commit 1ff051a has been approved by |
⌛ Testing commit 1ff051a with merge 843c7e900179da05320dc6f7a4a1699dddaa592f... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
@bors retry spurious network error |
☀️ Test successful - checks-actions |
Finished benchmarking commit (9d1aeae): comparison url. Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
Pkgsrc changes: * Adjust patches as needed & checksum updates. Upstream changes: Version 1.63.0 (2022-08-11) ========================== Language -------- - [Remove migrate borrowck mode for pre-NLL errors.][95565] - [Modify MIR building to drop repeat expressions with length zero.][95953] - [Remove label/lifetime shadowing warnings.][96296] - [Allow explicit generic arguments in the presence of `impl Trait` args.] [96868] - [Make `cenum_impl_drop_cast` warnings deny-by-default.][97652] - [Prevent unwinding when `-C panic=abort` is used regardless of declared ABI.][96959] - [lub: don't bail out due to empty binders.][97867] Compiler -------- - [Stabilize the `bundle` native library modifier,][95818] also removing the deprecated `static-nobundle` linking kind. - [Add Apple WatchOS compile targets\*.][95243] - [Add a Windows application manifest to rustc-main.][96737] \* Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Implement `Copy`, `Clone`, `PartialEq` and `Eq` for `core::fmt::Alignment`.][94530] - [Extend `ptr::null` and `null_mut` to all thin (including extern) types.][94954] - [`impl Read and Write for VecDeque<u8>`.][95632] - [STD support for the Nintendo 3DS.][95897] - [Make write/print macros eagerly drop temporaries.][96455] - [Implement internal traits that enable `[OsStr]::join`.][96881] - [Implement `Hash` for `core::alloc::Layout`.][97034] - [Add capacity documentation for `OsString`.][97202] - [Put a bound on collection misbehavior.][97316] - [Make `std::mem::needs_drop` accept `?Sized`.][97675] - [`impl Termination for Infallible` and then make the `Result` impls of `Termination` more generic.][97803] - [Document Rust's stance on `/proc/self/mem`.][97837] Stabilized APIs --------------- - [`array::from_fn`] - [`Box::into_pin`] - [`BinaryHeap::try_reserve`] - [`BinaryHeap::try_reserve_exact`] - [`OsString::try_reserve`] - [`OsString::try_reserve_exact`] - [`PathBuf::try_reserve`] - [`PathBuf::try_reserve_exact`] - [`Path::try_exists`] - [`Ref::filter_map`] - [`RefMut::filter_map`] - [`NonNull::<[T]>::len`][`NonNull::<slice>::len`] - [`ToOwned::clone_into`] - [`Ipv6Addr::to_ipv4_mapped`] - [`unix::io::AsFd`] - [`unix::io::BorrowedFd<'fd>`] - [`unix::io::OwnedFd`] - [`windows::io::AsHandle`] - [`windows::io::BorrowedHandle<'handle>`] - [`windows::io::OwnedHandle`] - [`windows::io::HandleOrInvalid`] - [`windows::io::HandleOrNull`] - [`windows::io::InvalidHandleError`] - [`windows::io::NullHandleError`] - [`windows::io::AsSocket`] - [`windows::io::BorrowedSocket<'handle>`] - [`windows::io::OwnedSocket`] - [`thread::scope`] - [`thread::Scope`] - [`thread::ScopedJoinHandle`] These APIs are now usable in const contexts: - [`array::from_ref`] - [`slice::from_ref`] - [`intrinsics::copy`] - [`intrinsics::copy_nonoverlapping`] - [`<*const T>::copy_to`] - [`<*const T>::copy_to_nonoverlapping`] - [`<*mut T>::copy_to`] - [`<*mut T>::copy_to_nonoverlapping`] - [`<*mut T>::copy_from`] - [`<*mut T>::copy_from_nonoverlapping`] - [`str::from_utf8`] - [`Utf8Error::error_len`] - [`Utf8Error::valid_up_to`] - [`Condvar::new`] - [`Mutex::new`] - [`RwLock::new`] Cargo ----- - [Stabilize the `--config path` command-line argument.][cargo/10755] - [Expose rust-version in the environment as `CARGO_PKG_RUST_VERSION`.][cargo/10713] Compatibility Notes ------------------- - [`#[link]` attributes are now checked more strictly,][96885] which may introduce errors for invalid attribute arguments that were previously ignored. Internal Changes ---------------- These changes provide no direct user facing benefits, but represent significant improvements to the internals and overall performance of rustc and related tools. - [Prepare Rust for LLVM opaque pointers.][94214] [94214]: rust-lang/rust#94214 [94530]: rust-lang/rust#94530 [94954]: rust-lang/rust#94954 [95243]: rust-lang/rust#95243 [95565]: rust-lang/rust#95565 [95632]: rust-lang/rust#95632 [95818]: rust-lang/rust#95818 [95897]: rust-lang/rust#95897 [95953]: rust-lang/rust#95953 [96296]: rust-lang/rust#96296 [96455]: rust-lang/rust#96455 [96737]: rust-lang/rust#96737 [96868]: rust-lang/rust#96868 [96881]: rust-lang/rust#96881 [96885]: rust-lang/rust#96885 [96959]: rust-lang/rust#96959 [97034]: rust-lang/rust#97034 [97202]: rust-lang/rust#97202 [97316]: rust-lang/rust#97316 [97652]: rust-lang/rust#97652 [97675]: rust-lang/rust#97675 [97803]: rust-lang/rust#97803 [97837]: rust-lang/rust#97837 [97867]: rust-lang/rust#97867 [cargo/10713]: rust-lang/cargo#10713 [cargo/10755]: rust-lang/cargo#10755 [`array::from_fn`]: https://doc.rust-lang.org/stable/std/array/fn.from_fn.html [`Box::into_pin`]: https://doc.rust-lang.org/stable/std/boxed/struct.Box.html#method.into_pin [`BinaryHeap::try_reserve_exact`]: https://doc.rust-lang.org/stable/alloc/collections/binary_heap/struct.BinaryHeap.html#method.try_reserve_exact [`BinaryHeap::try_reserve`]: https://doc.rust-lang.org/stable/std/collections/struct.BinaryHeap.html#method.try_reserve [`OsString::try_reserve`]: https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.try_reserve [`OsString::try_reserve_exact`]: https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.try_reserve_exact [`PathBuf::try_reserve`]: https://doc.rust-lang.org/stable/std/path/struct.PathBuf.html#method.try_reserve [`PathBuf::try_reserve_exact`]: https://doc.rust-lang.org/stable/std/path/struct.PathBuf.html#method.try_reserve_exact [`Path::try_exists`]: https://doc.rust-lang.org/stable/std/path/struct.Path.html#method.try_exists [`Ref::filter_map`]: https://doc.rust-lang.org/stable/std/cell/struct.Ref.html#method.filter_map [`RefMut::filter_map`]: https://doc.rust-lang.org/stable/std/cell/struct.RefMut.html#method.filter_map [`NonNull::<slice>::len`]: https://doc.rust-lang.org/stable/std/ptr/struct.NonNull.html#method.len [`ToOwned::clone_into`]: https://doc.rust-lang.org/stable/std/borrow/trait.ToOwned.html#method.clone_into [`Ipv6Addr::to_ipv4_mapped`]: https://doc.rust-lang.org/stable/std/net/struct.Ipv6Addr.html#method.to_ipv4_mapped [`unix::io::AsFd`]: https://doc.rust-lang.org/stable/std/os/unix/io/trait.AsFd.html [`unix::io::BorrowedFd<'fd>`]: https://doc.rust-lang.org/stable/std/os/unix/io/struct.BorrowedFd.html [`unix::io::OwnedFd`]: https://doc.rust-lang.org/stable/std/os/unix/io/struct.OwnedFd.html [`windows::io::AsHandle`]: https://doc.rust-lang.org/stable/std/os/windows/io/trait.AsHandle.html [`windows::io::BorrowedHandle<'handle>`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.BorrowedHandle.html [`windows::io::OwnedHandle`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.OwnedHandle.html [`windows::io::HandleOrInvalid`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.HandleOrInvalid.html [`windows::io::HandleOrNull`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.HandleOrNull.html [`windows::io::InvalidHandleError`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.InvalidHandleError.html [`windows::io::NullHandleError`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.NullHandleError.html [`windows::io::AsSocket`]: https://doc.rust-lang.org/stable/std/os/windows/io/trait.AsSocket.html [`windows::io::BorrowedSocket<'handle>`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.BorrowedSocket.html [`windows::io::OwnedSocket`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.OwnedSocket.html [`thread::scope`]: https://doc.rust-lang.org/stable/std/thread/fn.scope.html [`thread::Scope`]: https://doc.rust-lang.org/stable/std/thread/struct.Scope.html [`thread::ScopedJoinHandle`]: https://doc.rust-lang.org/stable/std/thread/struct.ScopedJoinHandle.html [`array::from_ref`]: https://doc.rust-lang.org/stable/std/array/fn.from_ref.html [`slice::from_ref`]: https://doc.rust-lang.org/stable/std/slice/fn.from_ref.html [`intrinsics::copy`]: https://doc.rust-lang.org/stable/std/intrinsics/fn.copy.html [`intrinsics::copy_nonoverlapping`]: https://doc.rust-lang.org/stable/std/intrinsics/fn.copy_nonoverlapping.html [`<*const T>::copy_to`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to [`<*const T>::copy_to_nonoverlapping`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to_nonoverlapping [`<*mut T>::copy_to`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to-1 [`<*mut T>::copy_to_nonoverlapping`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to_nonoverlapping-1 [`<*mut T>::copy_from`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_from [`<*mut T>::copy_from_nonoverlapping`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_from_nonoverlapping [`str::from_utf8`]: https://doc.rust-lang.org/stable/std/str/fn.from_utf8.html [`Utf8Error::error_len`]: https://doc.rust-lang.org/stable/std/str/struct.Utf8Error.html#method.error_len [`Utf8Error::valid_up_to`]: https://doc.rust-lang.org/stable/std/str/struct.Utf8Error.html#method.valid_up_to [`Condvar::new`]: https://doc.rust-lang.org/stable/std/sync/struct.Condvar.html#method.new [`Mutex::new`]: https://doc.rust-lang.org/stable/std/sync/struct.Mutex.html#method.new [`RwLock::new`]: https://doc.rust-lang.org/stable/std/sync/struct.RwLock.html#method.new
Fix one codegen bug with opaque pointers, and update our IR tests to accept both typed pointer and opaque pointer IR. This is a bit annoying, but unavoidable if we want decent test coverage on both LLVM 14 and LLVM 15.
This prepares Rust for when LLVM will enable opaque pointers by default.