From b4e1a53b7a146fe16fc3347b30700c602110a5f6 Mon Sep 17 00:00:00 2001 From: WANG Rui Date: Sun, 28 Jul 2024 12:46:46 +0800 Subject: [PATCH 1/9] Update target-spec metadata for loongarch64 targets --- .../src/spec/targets/loongarch64_unknown_linux_musl.rs | 8 ++++---- .../src/spec/targets/loongarch64_unknown_none.rs | 2 +- .../spec/targets/loongarch64_unknown_none_softfloat.rs | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_target/src/spec/targets/loongarch64_unknown_linux_musl.rs b/compiler/rustc_target/src/spec/targets/loongarch64_unknown_linux_musl.rs index 19b04607f0e..f7a6e0bd857 100644 --- a/compiler/rustc_target/src/spec/targets/loongarch64_unknown_linux_musl.rs +++ b/compiler/rustc_target/src/spec/targets/loongarch64_unknown_linux_musl.rs @@ -4,10 +4,10 @@ pub fn target() -> Target { Target { llvm_target: "loongarch64-unknown-linux-musl".into(), metadata: crate::spec::TargetMetadata { - description: Some("LoongArch64 Linux (LP64D ABI) with musl 1.2.3".into()), - tier: Some(3), - host_tools: Some(false), - std: None, // ? + description: Some("LoongArch64 Linux (LP64D ABI) with musl 1.2.5".into()), + tier: Some(2), + host_tools: Some(true), + std: Some(true), }, pointer_width: 64, data_layout: "e-m:e-p:64:64-i64:64-i128:128-n32:64-S128".into(), diff --git a/compiler/rustc_target/src/spec/targets/loongarch64_unknown_none.rs b/compiler/rustc_target/src/spec/targets/loongarch64_unknown_none.rs index 744ffff721f..9d80b4c901d 100644 --- a/compiler/rustc_target/src/spec/targets/loongarch64_unknown_none.rs +++ b/compiler/rustc_target/src/spec/targets/loongarch64_unknown_none.rs @@ -5,7 +5,7 @@ pub fn target() -> Target { Target { llvm_target: "loongarch64-unknown-none".into(), metadata: crate::spec::TargetMetadata { - description: None, + description: Some("Freestanding/bare-metal LoongArch64".into()), tier: Some(2), host_tools: Some(false), std: Some(false), diff --git a/compiler/rustc_target/src/spec/targets/loongarch64_unknown_none_softfloat.rs b/compiler/rustc_target/src/spec/targets/loongarch64_unknown_none_softfloat.rs index a382e7a53fb..d55611ffee3 100644 --- a/compiler/rustc_target/src/spec/targets/loongarch64_unknown_none_softfloat.rs +++ b/compiler/rustc_target/src/spec/targets/loongarch64_unknown_none_softfloat.rs @@ -5,7 +5,7 @@ pub fn target() -> Target { Target { llvm_target: "loongarch64-unknown-none".into(), metadata: crate::spec::TargetMetadata { - description: None, + description: Some("Freestanding/bare-metal LoongArch64 softfloat".into()), tier: Some(2), host_tools: Some(false), std: Some(false), From a75d2f9d384135c5f0f6bd6d26ec2ba9aa76745a Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Wed, 24 Jul 2024 22:02:48 +0000 Subject: [PATCH 2/9] Cleanup sys module to match house style --- .../std/src/{sys/anonymous_pipe => pipe}/tests.rs | 1 + library/std/src/sys/anonymous_pipe/mod.rs | 14 +++++--------- library/std/src/sys/anonymous_pipe/unix.rs | 8 ++++---- library/std/src/sys/anonymous_pipe/unsupported.rs | 2 +- library/std/src/sys/anonymous_pipe/windows.rs | 8 ++++---- library/std/src/sys/mod.rs | 1 - 6 files changed, 15 insertions(+), 19 deletions(-) rename library/std/src/{sys/anonymous_pipe => pipe}/tests.rs (90%) diff --git a/library/std/src/sys/anonymous_pipe/tests.rs b/library/std/src/pipe/tests.rs similarity index 90% rename from library/std/src/sys/anonymous_pipe/tests.rs rename to library/std/src/pipe/tests.rs index 865d24ec855..9c38e106787 100644 --- a/library/std/src/sys/anonymous_pipe/tests.rs +++ b/library/std/src/pipe/tests.rs @@ -2,6 +2,7 @@ use crate::pipe::pipe; #[test] +#[cfg(all(windows, unix, not(miri)))] fn pipe_creation_clone_and_rw() { let (rx, tx) = pipe().unwrap(); diff --git a/library/std/src/sys/anonymous_pipe/mod.rs b/library/std/src/sys/anonymous_pipe/mod.rs index 74875677cf3..aa14c8b650d 100644 --- a/library/std/src/sys/anonymous_pipe/mod.rs +++ b/library/std/src/sys/anonymous_pipe/mod.rs @@ -1,18 +1,14 @@ +#![forbid(unsafe_op_in_unsafe_fn)] + cfg_if::cfg_if! { if #[cfg(unix)] { mod unix; - pub(crate) use unix::{AnonPipe, pipe}; - - #[cfg(all(test, not(miri)))] - mod tests; + pub use unix::{AnonPipe, pipe}; } else if #[cfg(windows)] { mod windows; - pub(crate) use windows::{AnonPipe, pipe}; - - #[cfg(all(test, not(miri)))] - mod tests; + pub use windows::{AnonPipe, pipe}; } else { mod unsupported; - pub(crate) use unsupported::{AnonPipe, pipe}; + pub use unsupported::{AnonPipe, pipe}; } } diff --git a/library/std/src/sys/anonymous_pipe/unix.rs b/library/std/src/sys/anonymous_pipe/unix.rs index 7c0e75208c4..9168024730e 100644 --- a/library/std/src/sys/anonymous_pipe/unix.rs +++ b/library/std/src/sys/anonymous_pipe/unix.rs @@ -6,10 +6,10 @@ use crate::sys::pipe::anon_pipe; use crate::sys_common::{FromInner, IntoInner}; -pub(crate) type AnonPipe = FileDesc; +pub type AnonPipe = FileDesc; #[inline] -pub(crate) fn pipe() -> io::Result<(AnonPipe, AnonPipe)> { +pub fn pipe() -> io::Result<(AnonPipe, AnonPipe)> { anon_pipe().map(|(rx, wx)| (rx.into_inner(), wx.into_inner())) } @@ -34,7 +34,7 @@ fn from(pipe: PipeReader) -> Self { #[unstable(feature = "anonymous_pipe", issue = "127154")] impl FromRawFd for PipeReader { unsafe fn from_raw_fd(raw_fd: RawFd) -> Self { - Self(FileDesc::from_raw_fd(raw_fd)) + unsafe { Self(FileDesc::from_raw_fd(raw_fd)) } } } #[unstable(feature = "anonymous_pipe", issue = "127154")] @@ -71,7 +71,7 @@ fn from(pipe: PipeWriter) -> Self { #[unstable(feature = "anonymous_pipe", issue = "127154")] impl FromRawFd for PipeWriter { unsafe fn from_raw_fd(raw_fd: RawFd) -> Self { - Self(FileDesc::from_raw_fd(raw_fd)) + unsafe { Self(FileDesc::from_raw_fd(raw_fd)) } } } #[unstable(feature = "anonymous_pipe", issue = "127154")] diff --git a/library/std/src/sys/anonymous_pipe/unsupported.rs b/library/std/src/sys/anonymous_pipe/unsupported.rs index f3c826d2f8d..da04e8e6b25 100644 --- a/library/std/src/sys/anonymous_pipe/unsupported.rs +++ b/library/std/src/sys/anonymous_pipe/unsupported.rs @@ -4,7 +4,7 @@ pub(crate) use crate::sys::pipe::AnonPipe; #[inline] -pub(crate) fn pipe() -> io::Result<(AnonPipe, AnonPipe)> { +pub fn pipe() -> io::Result<(AnonPipe, AnonPipe)> { Err(io::Error::UNSUPPORTED_PLATFORM) } diff --git a/library/std/src/sys/anonymous_pipe/windows.rs b/library/std/src/sys/anonymous_pipe/windows.rs index 0f746b1082b..1b63e9a9b92 100644 --- a/library/std/src/sys/anonymous_pipe/windows.rs +++ b/library/std/src/sys/anonymous_pipe/windows.rs @@ -8,10 +8,10 @@ use crate::sys::pipe::unnamed_anon_pipe; use crate::sys_common::{FromInner, IntoInner}; -pub(crate) type AnonPipe = Handle; +pub type AnonPipe = Handle; #[inline] -pub(crate) fn pipe() -> io::Result<(AnonPipe, AnonPipe)> { +pub fn pipe() -> io::Result<(AnonPipe, AnonPipe)> { unnamed_anon_pipe().map(|(rx, wx)| (rx.into_inner(), wx.into_inner())) } @@ -31,7 +31,7 @@ fn as_raw_handle(&self) -> RawHandle { #[unstable(feature = "anonymous_pipe", issue = "127154")] impl FromRawHandle for PipeReader { unsafe fn from_raw_handle(raw_handle: RawHandle) -> Self { - Self(Handle::from_raw_handle(raw_handle)) + unsafe { Self(Handle::from_raw_handle(raw_handle)) } } } #[unstable(feature = "anonymous_pipe", issue = "127154")] @@ -70,7 +70,7 @@ fn as_raw_handle(&self) -> RawHandle { #[unstable(feature = "anonymous_pipe", issue = "127154")] impl FromRawHandle for PipeWriter { unsafe fn from_raw_handle(raw_handle: RawHandle) -> Self { - Self(Handle::from_raw_handle(raw_handle)) + unsafe { Self(Handle::from_raw_handle(raw_handle)) } } } #[unstable(feature = "anonymous_pipe", issue = "127154")] diff --git a/library/std/src/sys/mod.rs b/library/std/src/sys/mod.rs index 202997b7495..a86b3628f24 100644 --- a/library/std/src/sys/mod.rs +++ b/library/std/src/sys/mod.rs @@ -7,7 +7,6 @@ mod personality; -#[unstable(feature = "anonymous_pipe", issue = "127154")] pub mod anonymous_pipe; pub mod backtrace; pub mod cmath; From 9169622027f1dcb702f789076da1b8b98c01a215 Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Wed, 24 Jul 2024 22:22:09 +0000 Subject: [PATCH 3/9] Move Windows implementation of anon pipe --- library/std/src/sys/anonymous_pipe/windows.rs | 16 ++++++++++++---- library/std/src/sys/pal/windows/pipe.rs | 17 ----------------- 2 files changed, 12 insertions(+), 21 deletions(-) diff --git a/library/std/src/sys/anonymous_pipe/windows.rs b/library/std/src/sys/anonymous_pipe/windows.rs index 1b63e9a9b92..a48198f8a81 100644 --- a/library/std/src/sys/anonymous_pipe/windows.rs +++ b/library/std/src/sys/anonymous_pipe/windows.rs @@ -1,18 +1,26 @@ -use crate::io; use crate::os::windows::io::{ AsHandle, AsRawHandle, BorrowedHandle, FromRawHandle, IntoRawHandle, OwnedHandle, RawHandle, }; use crate::pipe::{PipeReader, PipeWriter}; use crate::process::Stdio; +use crate::sys::c; use crate::sys::handle::Handle; -use crate::sys::pipe::unnamed_anon_pipe; use crate::sys_common::{FromInner, IntoInner}; +use crate::{io, ptr}; pub type AnonPipe = Handle; -#[inline] pub fn pipe() -> io::Result<(AnonPipe, AnonPipe)> { - unnamed_anon_pipe().map(|(rx, wx)| (rx.into_inner(), wx.into_inner())) + let mut read_pipe = c::INVALID_HANDLE_VALUE; + let mut write_pipe = c::INVALID_HANDLE_VALUE; + + let ret = unsafe { c::CreatePipe(&mut read_pipe, &mut write_pipe, ptr::null_mut(), 0) }; + + if ret == 0 { + Err(io::Error::last_os_error()) + } else { + unsafe { Ok((Handle::from_raw_handle(read_pipe), Handle::from_raw_handle(write_pipe))) } + } } #[unstable(feature = "anonymous_pipe", issue = "127154")] diff --git a/library/std/src/sys/pal/windows/pipe.rs b/library/std/src/sys/pal/windows/pipe.rs index 9eb19fcf2d7..7d1b5aca1d5 100644 --- a/library/std/src/sys/pal/windows/pipe.rs +++ b/library/std/src/sys/pal/windows/pipe.rs @@ -36,23 +36,6 @@ pub struct Pipes { pub theirs: AnonPipe, } -/// Create true unnamed anonymous pipe. -pub fn unnamed_anon_pipe() -> io::Result<(AnonPipe, AnonPipe)> { - let mut read_pipe = c::INVALID_HANDLE_VALUE; - let mut write_pipe = c::INVALID_HANDLE_VALUE; - - let ret = unsafe { c::CreatePipe(&mut read_pipe, &mut write_pipe, ptr::null_mut(), 0) }; - - if ret == 0 { - Err(io::Error::last_os_error()) - } else { - Ok(( - AnonPipe::from_inner(unsafe { Handle::from_raw_handle(read_pipe) }), - AnonPipe::from_inner(unsafe { Handle::from_raw_handle(write_pipe) }), - )) - } -} - /// Although this looks similar to `anon_pipe` in the Unix module it's actually /// subtly different. Here we'll return two pipes in the `Pipes` return value, /// but one is intended for "us" where as the other is intended for "someone From e84a7d91b7b618fe5bb8d3c0face3384c768fb2c Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Wed, 31 Jul 2024 13:45:14 +0000 Subject: [PATCH 4/9] Remove unneeded `pub(crate)` --- library/std/src/sys/anonymous_pipe/unsupported.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/sys/anonymous_pipe/unsupported.rs b/library/std/src/sys/anonymous_pipe/unsupported.rs index da04e8e6b25..dd51e70315e 100644 --- a/library/std/src/sys/anonymous_pipe/unsupported.rs +++ b/library/std/src/sys/anonymous_pipe/unsupported.rs @@ -1,7 +1,7 @@ use crate::io; use crate::pipe::{PipeReader, PipeWriter}; use crate::process::Stdio; -pub(crate) use crate::sys::pipe::AnonPipe; +pub use crate::sys::pipe::AnonPipe; #[inline] pub fn pipe() -> io::Result<(AnonPipe, AnonPipe)> { From cf11f499b37e557e41f5eeac157c623d08e0068d Mon Sep 17 00:00:00 2001 From: joboet Date: Wed, 10 Jul 2024 14:12:58 +0200 Subject: [PATCH 5/9] std: implement the `once_wait` feature --- library/std/src/sync/once.rs | 41 ++++++ library/std/src/sync/once_lock.rs | 28 ++++ library/std/src/sys/sync/once/futex.rs | 124 ++++++++++++----- library/std/src/sys/sync/once/no_threads.rs | 6 + library/std/src/sys/sync/once/queue.rs | 142 ++++++++++++-------- 5 files changed, 247 insertions(+), 94 deletions(-) diff --git a/library/std/src/sync/once.rs b/library/std/src/sync/once.rs index 9d969af8c6d..17c56c85016 100644 --- a/library/std/src/sync/once.rs +++ b/library/std/src/sync/once.rs @@ -264,6 +264,47 @@ pub fn is_completed(&self) -> bool { self.inner.is_completed() } + /// Blocks the current thread until initialization has completed. + /// + /// # Example + /// + /// ```rust + /// #![feature(once_wait)] + /// + /// use std::sync::Once; + /// use std::thread; + /// + /// static READY: Once = Once::new(); + /// + /// let thread = thread::spawn(|| { + /// READY.wait(); + /// println!("everything is ready"); + /// }); + /// + /// READY.call_once(|| println!("performing setup")); + /// ``` + /// + /// # Panics + /// + /// If this [`Once`] has been poisoned because an initialization closure has + /// panicked, this method will also panic. Use [`wait_force`](Self::wait_force) + /// if this behaviour is not desired. + #[unstable(feature = "once_wait", issue = "127527")] + pub fn wait(&self) { + if !self.inner.is_completed() { + self.inner.wait(false); + } + } + + /// Blocks the current thread until initialization has completed, ignoring + /// poisoning. + #[unstable(feature = "once_wait", issue = "127527")] + pub fn wait_force(&self) { + if !self.inner.is_completed() { + self.inner.wait(true); + } + } + /// Returns the current state of the `Once` instance. /// /// Since this takes a mutable reference, no initialization can currently diff --git a/library/std/src/sync/once_lock.rs b/library/std/src/sync/once_lock.rs index 60e43a1cde1..efc1f415edf 100644 --- a/library/std/src/sync/once_lock.rs +++ b/library/std/src/sync/once_lock.rs @@ -167,6 +167,34 @@ pub fn get_mut(&mut self) -> Option<&mut T> { } } + /// Blocks the current thread until the cell is initialized. + /// + /// # Example + /// + /// Waiting for a computation on another thread to finish: + /// ```rust + /// #![feature(once_wait)] + /// + /// use std::thread; + /// use std::sync::OnceLock; + /// + /// let value = OnceLock::new(); + /// + /// thread::scope(|s| { + /// s.spawn(|| value.set(1 + 1)); + /// + /// let result = value.wait(); + /// assert_eq!(result, &2); + /// }) + /// ``` + #[inline] + #[unstable(feature = "once_wait", issue = "127527")] + pub fn wait(&self) -> &T { + self.once.wait_force(); + + unsafe { self.get_unchecked() } + } + /// Sets the contents of this cell to `value`. /// /// May block if another thread is currently attempting to initialize the cell. The cell is diff --git a/library/std/src/sys/sync/once/futex.rs b/library/std/src/sys/sync/once/futex.rs index e683777803f..2c8a054282b 100644 --- a/library/std/src/sys/sync/once/futex.rs +++ b/library/std/src/sys/sync/once/futex.rs @@ -6,7 +6,7 @@ use crate::sys::futex::{futex_wait, futex_wake_all}; // On some platforms, the OS is very nice and handles the waiter queue for us. -// This means we only need one atomic value with 5 states: +// This means we only need one atomic value with 4 states: /// No initialization has run yet, and no thread is currently using the Once. const INCOMPLETE: u32 = 0; @@ -17,16 +17,20 @@ /// Some thread is currently attempting to run initialization. It may succeed, /// so all future threads need to wait for it to finish. const RUNNING: u32 = 2; -/// Some thread is currently attempting to run initialization and there are threads -/// waiting for it to finish. -const QUEUED: u32 = 3; /// Initialization has completed and all future calls should finish immediately. -const COMPLETE: u32 = 4; +const COMPLETE: u32 = 3; -// Threads wait by setting the state to QUEUED and calling `futex_wait` on the state +// An additional bit indicates whether there are waiting threads: + +/// May only be set if the state is not COMPLETE. +const QUEUED: u32 = 4; + +// Threads wait by setting the QUEUED bit and calling `futex_wait` on the state // variable. When the running thread finishes, it will wake all waiting threads using // `futex_wake_all`. +const STATE_MASK: u32 = 0b11; + pub struct OnceState { poisoned: bool, set_state_to: Cell, @@ -45,7 +49,7 @@ pub fn poison(&self) { } struct CompletionGuard<'a> { - state: &'a AtomicU32, + state_and_queued: &'a AtomicU32, set_state_on_drop_to: u32, } @@ -54,32 +58,32 @@ fn drop(&mut self) { // Use release ordering to propagate changes to all threads checking // up on the Once. `futex_wake_all` does its own synchronization, hence // we do not need `AcqRel`. - if self.state.swap(self.set_state_on_drop_to, Release) == QUEUED { - futex_wake_all(self.state); + if self.state_and_queued.swap(self.set_state_on_drop_to, Release) & QUEUED != 0 { + futex_wake_all(self.state_and_queued); } } } pub struct Once { - state: AtomicU32, + state_and_queued: AtomicU32, } impl Once { #[inline] pub const fn new() -> Once { - Once { state: AtomicU32::new(INCOMPLETE) } + Once { state_and_queued: AtomicU32::new(INCOMPLETE) } } #[inline] pub fn is_completed(&self) -> bool { // Use acquire ordering to make all initialization changes visible to the // current thread. - self.state.load(Acquire) == COMPLETE + self.state_and_queued.load(Acquire) == COMPLETE } #[inline] pub(crate) fn state(&mut self) -> ExclusiveState { - match *self.state.get_mut() { + match *self.state_and_queued.get_mut() { INCOMPLETE => ExclusiveState::Incomplete, POISONED => ExclusiveState::Poisoned, COMPLETE => ExclusiveState::Complete, @@ -87,31 +91,73 @@ pub(crate) fn state(&mut self) -> ExclusiveState { } } - // This uses FnMut to match the API of the generic implementation. As this - // implementation is quite light-weight, it is generic over the closure and - // so avoids the cost of dynamic dispatch. #[cold] #[track_caller] - pub fn call(&self, ignore_poisoning: bool, f: &mut impl FnMut(&public::OnceState)) { - let mut state = self.state.load(Acquire); + pub fn wait(&self, ignore_poisoning: bool) { + let mut state_and_queued = self.state_and_queued.load(Acquire); loop { + let state = state_and_queued & STATE_MASK; + let queued = state_and_queued & QUEUED != 0; match state { + COMPLETE => return, + POISONED if !ignore_poisoning => { + // Panic to propagate the poison. + panic!("Once instance has previously been poisoned"); + } + _ => { + // Set the QUEUED bit if it has not already been set. + if !queued { + state_and_queued += QUEUED; + if let Err(new) = self.state_and_queued.compare_exchange_weak( + state, + state_and_queued, + Relaxed, + Acquire, + ) { + state_and_queued = new; + continue; + } + } + + futex_wait(&self.state_and_queued, state_and_queued, None); + state_and_queued = self.state_and_queued.load(Acquire); + } + } + } + } + + #[cold] + #[track_caller] + pub fn call(&self, ignore_poisoning: bool, f: &mut dyn FnMut(&public::OnceState)) { + let mut state_and_queued = self.state_and_queued.load(Acquire); + loop { + let state = state_and_queued & STATE_MASK; + let queued = state_and_queued & QUEUED != 0; + match state { + COMPLETE => return, POISONED if !ignore_poisoning => { // Panic to propagate the poison. panic!("Once instance has previously been poisoned"); } INCOMPLETE | POISONED => { // Try to register the current thread as the one running. - if let Err(new) = - self.state.compare_exchange_weak(state, RUNNING, Acquire, Acquire) - { - state = new; + let next = RUNNING + if queued { QUEUED } else { 0 }; + if let Err(new) = self.state_and_queued.compare_exchange_weak( + state_and_queued, + next, + Acquire, + Acquire, + ) { + state_and_queued = new; continue; } + // `waiter_queue` will manage other waiting threads, and // wake them up on drop. - let mut waiter_queue = - CompletionGuard { state: &self.state, set_state_on_drop_to: POISONED }; + let mut waiter_queue = CompletionGuard { + state_and_queued: &self.state_and_queued, + set_state_on_drop_to: POISONED, + }; // Run the function, letting it know if we're poisoned or not. let f_state = public::OnceState { inner: OnceState { @@ -123,21 +169,27 @@ pub fn call(&self, ignore_poisoning: bool, f: &mut impl FnMut(&public::OnceState waiter_queue.set_state_on_drop_to = f_state.inner.set_state_to.get(); return; } - RUNNING | QUEUED => { - // Set the state to QUEUED if it is not already. - if state == RUNNING - && let Err(new) = - self.state.compare_exchange_weak(RUNNING, QUEUED, Relaxed, Acquire) - { - state = new; - continue; + _ => { + // All other values must be RUNNING. + assert!(state == RUNNING); + + // Set the QUEUED bit if it is not already set. + if !queued { + state_and_queued += QUEUED; + if let Err(new) = self.state_and_queued.compare_exchange_weak( + state, + state_and_queued, + Relaxed, + Acquire, + ) { + state_and_queued = new; + continue; + } } - futex_wait(&self.state, QUEUED, None); - state = self.state.load(Acquire); + futex_wait(&self.state_and_queued, state_and_queued, None); + state_and_queued = self.state_and_queued.load(Acquire); } - COMPLETE => return, - _ => unreachable!("state is never set to invalid values"), } } } diff --git a/library/std/src/sys/sync/once/no_threads.rs b/library/std/src/sys/sync/once/no_threads.rs index 11fde1888ba..12c1d9f5a6c 100644 --- a/library/std/src/sys/sync/once/no_threads.rs +++ b/library/std/src/sys/sync/once/no_threads.rs @@ -55,6 +55,12 @@ pub(crate) fn state(&mut self) -> ExclusiveState { } } + #[cold] + #[track_caller] + pub fn wait(&self, _ignore_poisoning: bool) { + panic!("not implementable on this target"); + } + #[cold] #[track_caller] pub fn call(&self, ignore_poisoning: bool, f: &mut impl FnMut(&public::OnceState)) { diff --git a/library/std/src/sys/sync/once/queue.rs b/library/std/src/sys/sync/once/queue.rs index b04d252f8b9..7a020c94080 100644 --- a/library/std/src/sys/sync/once/queue.rs +++ b/library/std/src/sys/sync/once/queue.rs @@ -56,20 +56,21 @@ // allowed, so no need for `SeqCst`. use crate::cell::Cell; -use crate::sync::atomic::{AtomicBool, AtomicPtr, Ordering}; +use crate::sync::atomic::Ordering::{AcqRel, Acquire, Release}; +use crate::sync::atomic::{AtomicBool, AtomicPtr}; use crate::sync::once::ExclusiveState; use crate::thread::{self, Thread}; use crate::{fmt, ptr, sync as public}; -type Masked = (); +type StateAndQueue = *mut (); pub struct Once { - state_and_queue: AtomicPtr, + state_and_queue: AtomicPtr<()>, } pub struct OnceState { poisoned: bool, - set_state_on_drop_to: Cell<*mut Masked>, + set_state_on_drop_to: Cell, } // Four states that a Once can be in, encoded into the lower bits of @@ -81,7 +82,8 @@ pub struct OnceState { // Mask to learn about the state. All other bits are the queue of waiters if // this is in the RUNNING state. -const STATE_MASK: usize = 0x3; +const STATE_MASK: usize = 0b11; +const QUEUE_MASK: usize = !STATE_MASK; // Representation of a node in the linked list of waiters, used while in the // RUNNING state. @@ -93,15 +95,23 @@ pub struct OnceState { struct Waiter { thread: Cell>, signaled: AtomicBool, - next: *const Waiter, + next: Cell<*const Waiter>, } // Head of a linked list of waiters. // Every node is a struct on the stack of a waiting thread. // Will wake up the waiters when it gets dropped, i.e. also on panic. struct WaiterQueue<'a> { - state_and_queue: &'a AtomicPtr, - set_state_on_drop_to: *mut Masked, + state_and_queue: &'a AtomicPtr<()>, + set_state_on_drop_to: StateAndQueue, +} + +fn to_queue(current: StateAndQueue) -> *const Waiter { + current.mask(QUEUE_MASK).cast() +} + +fn to_state(current: StateAndQueue) -> usize { + current.addr() & STATE_MASK } impl Once { @@ -117,7 +127,7 @@ pub fn is_completed(&self) -> bool { // operations visible to us, and, this being a fast path, weaker // ordering helps with performance. This `Acquire` synchronizes with // `Release` operations on the slow path. - self.state_and_queue.load(Ordering::Acquire).addr() == COMPLETE + self.state_and_queue.load(Acquire).addr() == COMPLETE } #[inline] @@ -130,6 +140,25 @@ pub(crate) fn state(&mut self) -> ExclusiveState { } } + #[cold] + #[track_caller] + pub fn wait(&self, ignore_poisoning: bool) { + let mut current = self.state_and_queue.load(Acquire); + loop { + let state = to_state(current); + match state { + COMPLETE => return, + POISONED if !ignore_poisoning => { + // Panic to propagate the poison. + panic!("Once instance has previously been poisoned"); + } + _ => { + current = wait(&self.state_and_queue, current); + } + } + } + } + // This is a non-generic function to reduce the monomorphization cost of // using `call_once` (this isn't exactly a trivial or small implementation). // @@ -144,9 +173,10 @@ pub(crate) fn state(&mut self) -> ExclusiveState { #[cold] #[track_caller] pub fn call(&self, ignore_poisoning: bool, init: &mut dyn FnMut(&public::OnceState)) { - let mut state_and_queue = self.state_and_queue.load(Ordering::Acquire); + let mut current = self.state_and_queue.load(Acquire); loop { - match state_and_queue.addr() { + let state = to_state(current); + match state { COMPLETE => break, POISONED if !ignore_poisoning => { // Panic to propagate the poison. @@ -154,16 +184,16 @@ pub fn call(&self, ignore_poisoning: bool, init: &mut dyn FnMut(&public::OnceSta } POISONED | INCOMPLETE => { // Try to register this thread as the one RUNNING. - let exchange_result = self.state_and_queue.compare_exchange( - state_and_queue, - ptr::without_provenance_mut(RUNNING), - Ordering::Acquire, - Ordering::Acquire, - ); - if let Err(old) = exchange_result { - state_and_queue = old; + if let Err(new) = self.state_and_queue.compare_exchange_weak( + current, + current.mask(QUEUE_MASK).wrapping_byte_add(RUNNING), + Acquire, + Acquire, + ) { + current = new; continue; } + // `waiter_queue` will manage other waiting threads, and // wake them up on drop. let mut waiter_queue = WaiterQueue { @@ -174,54 +204,53 @@ pub fn call(&self, ignore_poisoning: bool, init: &mut dyn FnMut(&public::OnceSta // poisoned or not. let init_state = public::OnceState { inner: OnceState { - poisoned: state_and_queue.addr() == POISONED, + poisoned: state == POISONED, set_state_on_drop_to: Cell::new(ptr::without_provenance_mut(COMPLETE)), }, }; init(&init_state); waiter_queue.set_state_on_drop_to = init_state.inner.set_state_on_drop_to.get(); - break; + return; } _ => { // All other values must be RUNNING with possibly a // pointer to the waiter queue in the more significant bits. - assert!(state_and_queue.addr() & STATE_MASK == RUNNING); - wait(&self.state_and_queue, state_and_queue); - state_and_queue = self.state_and_queue.load(Ordering::Acquire); + assert!(state == RUNNING); + current = wait(&self.state_and_queue, current); } } } } } -fn wait(state_and_queue: &AtomicPtr, mut current_state: *mut Masked) { - // Note: the following code was carefully written to avoid creating a - // mutable reference to `node` that gets aliased. +fn wait(state_and_queue: &AtomicPtr<()>, mut current: StateAndQueue) -> StateAndQueue { + let node = &Waiter { + thread: Cell::new(Some(thread::current())), + signaled: AtomicBool::new(false), + next: Cell::new(ptr::null()), + }; + loop { - // Don't queue this thread if the status is no longer running, - // otherwise we will not be woken up. - if current_state.addr() & STATE_MASK != RUNNING { - return; + let state = to_state(current); + let queue = to_queue(current); + + // If initialization has finished, return. + if matches!(state, POISONED | COMPLETE) { + return current; } - // Create the node for our current thread. - let node = Waiter { - thread: Cell::new(Some(thread::current())), - signaled: AtomicBool::new(false), - next: current_state.with_addr(current_state.addr() & !STATE_MASK) as *const Waiter, - }; - let me = core::ptr::addr_of!(node) as *const Masked as *mut Masked; + // Update the node for our current thread. + node.next.set(queue); // Try to slide in the node at the head of the linked list, making sure // that another thread didn't just replace the head of the linked list. - let exchange_result = state_and_queue.compare_exchange( - current_state, - me.with_addr(me.addr() | RUNNING), - Ordering::Release, - Ordering::Relaxed, - ); - if let Err(old) = exchange_result { - current_state = old; + if let Err(new) = state_and_queue.compare_exchange_weak( + current, + ptr::from_ref(node).wrapping_byte_add(state) as StateAndQueue, + Release, + Acquire, + ) { + current = new; continue; } @@ -230,14 +259,15 @@ fn wait(state_and_queue: &AtomicPtr, mut current_state: *mut Masked) { // would drop our `Waiter` node and leave a hole in the linked list // (and a dangling reference). Guard against spurious wakeups by // reparking ourselves until we are signaled. - while !node.signaled.load(Ordering::Acquire) { + while !node.signaled.load(Acquire) { // If the managing thread happens to signal and unpark us before we // can park ourselves, the result could be this thread never gets // unparked. Luckily `park` comes with the guarantee that if it got // an `unpark` just before on an unparked thread it does not park. thread::park(); } - break; + + return state_and_queue.load(Acquire); } } @@ -251,11 +281,10 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { impl Drop for WaiterQueue<'_> { fn drop(&mut self) { // Swap out our state with however we finished. - let state_and_queue = - self.state_and_queue.swap(self.set_state_on_drop_to, Ordering::AcqRel); + let current = self.state_and_queue.swap(self.set_state_on_drop_to, AcqRel); // We should only ever see an old state which was RUNNING. - assert_eq!(state_and_queue.addr() & STATE_MASK, RUNNING); + assert_eq!(current.addr() & STATE_MASK, RUNNING); // Walk the entire linked list of waiters and wake them up (in lifo // order, last to register is first to wake up). @@ -264,16 +293,13 @@ fn drop(&mut self) { // free `node` if there happens to be has a spurious wakeup. // So we have to take out the `thread` field and copy the pointer to // `next` first. - let mut queue = - state_and_queue.with_addr(state_and_queue.addr() & !STATE_MASK) as *const Waiter; + let mut queue = to_queue(current); while !queue.is_null() { - let next = (*queue).next; + let next = (*queue).next.get(); let thread = (*queue).thread.take().unwrap(); - (*queue).signaled.store(true, Ordering::Release); - // ^- FIXME (maybe): This is another case of issue #55005 - // `store()` has a potentially dangling ref to `signaled`. - queue = next; + (*queue).signaled.store(true, Release); thread.unpark(); + queue = next; } } } From 1d49aad8445881ac060cfae0e351207c05f82dfd Mon Sep 17 00:00:00 2001 From: joboet Date: Fri, 12 Jul 2024 13:17:43 +0200 Subject: [PATCH 6/9] std: fix busy-waiting in `Once::wait_force`, add more tests --- library/std/src/sync/once/tests.rs | 47 ++++++++++++++++++++++++++ library/std/src/sys/sync/once/queue.rs | 12 ++++--- 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/library/std/src/sync/once/tests.rs b/library/std/src/sync/once/tests.rs index d43dabc1cf1..ce96468aeb6 100644 --- a/library/std/src/sync/once/tests.rs +++ b/library/std/src/sync/once/tests.rs @@ -1,5 +1,8 @@ use super::Once; +use crate::sync::atomic::AtomicBool; +use crate::sync::atomic::Ordering::Relaxed; use crate::sync::mpsc::channel; +use crate::time::Duration; use crate::{panic, thread}; #[test] @@ -113,3 +116,47 @@ fn wait_for_force_to_finish() { assert!(t1.join().is_ok()); assert!(t2.join().is_ok()); } + +#[test] +fn wait() { + for _ in 0..50 { + let val = AtomicBool::new(false); + let once = Once::new(); + + thread::scope(|s| { + for _ in 0..4 { + s.spawn(|| { + once.wait(); + assert!(val.load(Relaxed)); + }); + } + + once.call_once(|| val.store(true, Relaxed)); + }); + } +} + +#[test] +fn wait_on_poisoned() { + let once = Once::new(); + + panic::catch_unwind(|| once.call_once(|| panic!())).unwrap_err(); + panic::catch_unwind(|| once.wait()).unwrap_err(); +} + +#[test] +fn wait_force_on_poisoned() { + let once = Once::new(); + + thread::scope(|s| { + panic::catch_unwind(|| once.call_once(|| panic!())).unwrap_err(); + + s.spawn(|| { + thread::sleep(Duration::from_millis(100)); + + once.call_once_force(|_| {}); + }); + + once.wait_force(); + }) +} diff --git a/library/std/src/sys/sync/once/queue.rs b/library/std/src/sys/sync/once/queue.rs index 7a020c94080..86f72c82008 100644 --- a/library/std/src/sys/sync/once/queue.rs +++ b/library/std/src/sys/sync/once/queue.rs @@ -153,7 +153,7 @@ pub fn wait(&self, ignore_poisoning: bool) { panic!("Once instance has previously been poisoned"); } _ => { - current = wait(&self.state_and_queue, current); + current = wait(&self.state_and_queue, current, !ignore_poisoning); } } } @@ -216,14 +216,18 @@ pub fn call(&self, ignore_poisoning: bool, init: &mut dyn FnMut(&public::OnceSta // All other values must be RUNNING with possibly a // pointer to the waiter queue in the more significant bits. assert!(state == RUNNING); - current = wait(&self.state_and_queue, current); + current = wait(&self.state_and_queue, current, true); } } } } } -fn wait(state_and_queue: &AtomicPtr<()>, mut current: StateAndQueue) -> StateAndQueue { +fn wait( + state_and_queue: &AtomicPtr<()>, + mut current: StateAndQueue, + return_on_poisoned: bool, +) -> StateAndQueue { let node = &Waiter { thread: Cell::new(Some(thread::current())), signaled: AtomicBool::new(false), @@ -235,7 +239,7 @@ fn wait(state_and_queue: &AtomicPtr<()>, mut current: StateAndQueue) -> StateAnd let queue = to_queue(current); // If initialization has finished, return. - if matches!(state, POISONED | COMPLETE) { + if state == COMPLETE || (return_on_poisoned && state == POISONED) { return current; } From 74754b878621b42097de33f9579d7630fa704627 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 31 Jul 2024 12:24:22 -0400 Subject: [PATCH 7/9] Properly mark loop as diverging if it has no breaks --- compiler/rustc_hir_typeck/src/expr.rs | 2 + .../rustc_hir_typeck/src/fn_ctxt/_impl.rs | 58 +++++++++++-------- tests/ui/async-await/unreachable-lint-2.rs | 15 +++++ .../ui/async-await/unreachable-lint-2.stderr | 17 ++++++ 4 files changed, 69 insertions(+), 23 deletions(-) create mode 100644 tests/ui/async-await/unreachable-lint-2.rs create mode 100644 tests/ui/async-await/unreachable-lint-2.stderr diff --git a/compiler/rustc_hir_typeck/src/expr.rs b/compiler/rustc_hir_typeck/src/expr.rs index d75a5f8806b..e54f9486f6a 100644 --- a/compiler/rustc_hir_typeck/src/expr.rs +++ b/compiler/rustc_hir_typeck/src/expr.rs @@ -1306,6 +1306,8 @@ fn check_expr_loop( // No way to know whether it's diverging because // of a `break` or an outer `break` or `return`. self.diverges.set(Diverges::Maybe); + } else { + self.diverges.set(self.diverges.get() | Diverges::always(expr.span)); } // If we permit break with a value, then result type is diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs index dea125bb9b1..40d9a2985da 100644 --- a/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs +++ b/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs @@ -48,30 +48,42 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { /// Produces warning on the given node, if the current point in the /// function is unreachable, and there hasn't been another warning. pub(crate) fn warn_if_unreachable(&self, id: HirId, span: Span, kind: &str) { - // FIXME: Combine these two 'if' expressions into one once - // let chains are implemented - if let Diverges::Always { span: orig_span, custom_note } = self.diverges.get() { - // If span arose from a desugaring of `if` or `while`, then it is the condition itself, - // which diverges, that we are about to lint on. This gives suboptimal diagnostics. - // Instead, stop here so that the `if`- or `while`-expression's block is linted instead. - if !span.is_desugaring(DesugaringKind::CondTemporary) - && !span.is_desugaring(DesugaringKind::Async) - && !orig_span.is_desugaring(DesugaringKind::Await) - { - self.diverges.set(Diverges::WarnedAlways); - - debug!("warn_if_unreachable: id={:?} span={:?} kind={}", id, span, kind); - - let msg = format!("unreachable {kind}"); - self.tcx().node_span_lint(lint::builtin::UNREACHABLE_CODE, id, span, |lint| { - lint.primary_message(msg.clone()); - lint.span_label(span, msg).span_label( - orig_span, - custom_note.unwrap_or("any code following this expression is unreachable"), - ); - }) - } + // If span arose from a desugaring of `if` or `while`, then it is the condition itself, + // which diverges, that we are about to lint on. This gives suboptimal diagnostics. + // Instead, stop here so that the `if`- or `while`-expression's block is linted instead. + if span.is_desugaring(DesugaringKind::CondTemporary) { + return; } + + // Don't lint if the result of an async block or async function is `!`. + // This does not affect the unreachable lints *within* the body. + if span.is_desugaring(DesugaringKind::Async) { + return; + } + + // Don't lint *within* the `.await` operator, since that's all just desugaring junk. + // We only want to lint if there is a subsequent expression after the `.await`. + if span.is_desugaring(DesugaringKind::Await) { + return; + } + + let Diverges::Always { span: orig_span, custom_note } = self.diverges.get() else { + return; + }; + + // Don't warn twice. + self.diverges.set(Diverges::WarnedAlways); + + debug!("warn_if_unreachable: id={:?} span={:?} kind={}", id, span, kind); + + let msg = format!("unreachable {kind}"); + self.tcx().node_span_lint(lint::builtin::UNREACHABLE_CODE, id, span, |lint| { + lint.primary_message(msg.clone()); + lint.span_label(span, msg).span_label( + orig_span, + custom_note.unwrap_or("any code following this expression is unreachable"), + ); + }) } /// Resolves type and const variables in `ty` if possible. Unlike the infcx diff --git a/tests/ui/async-await/unreachable-lint-2.rs b/tests/ui/async-await/unreachable-lint-2.rs new file mode 100644 index 00000000000..137cb32481b --- /dev/null +++ b/tests/ui/async-await/unreachable-lint-2.rs @@ -0,0 +1,15 @@ +//@ edition:2018 + +#![deny(unreachable_code)] + +async fn foo() { + endless().await; + println!("this is unreachable!"); + //~^ ERROR unreachable statement +} + +async fn endless() -> ! { + loop {} +} + +fn main() { } diff --git a/tests/ui/async-await/unreachable-lint-2.stderr b/tests/ui/async-await/unreachable-lint-2.stderr new file mode 100644 index 00000000000..cbebc9951f3 --- /dev/null +++ b/tests/ui/async-await/unreachable-lint-2.stderr @@ -0,0 +1,17 @@ +error: unreachable statement + --> $DIR/unreachable-lint-2.rs:7:5 + | +LL | endless().await; + | ----- any code following this expression is unreachable +LL | println!("this is unreachable!"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unreachable statement + | +note: the lint level is defined here + --> $DIR/unreachable-lint-2.rs:3:9 + | +LL | #![deny(unreachable_code)] + | ^^^^^^^^^^^^^^^^ + = note: this error originates in the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: aborting due to 1 previous error + From 840ca3cbef88919bfcacf0572f605145712195d7 Mon Sep 17 00:00:00 2001 From: Urgau Date: Wed, 31 Jul 2024 19:36:47 +0200 Subject: [PATCH 8/9] Temporarily switch `ambiguous_negative_literals` lint to allow --- compiler/rustc_lint/src/precedence.rs | 3 ++- tests/ui/lint/negative_literals.rs | 2 ++ tests/ui/lint/negative_literals.stderr | 28 +++++++++++++++----------- 3 files changed, 20 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_lint/src/precedence.rs b/compiler/rustc_lint/src/precedence.rs index eb2ba397277..52321e25c7d 100644 --- a/compiler/rustc_lint/src/precedence.rs +++ b/compiler/rustc_lint/src/precedence.rs @@ -16,6 +16,7 @@ /// ### Example /// /// ```rust,compile_fail + /// # #![deny(ambiguous_negative_literals)] /// # #![allow(unused)] /// -1i32.abs(); // equals -1, while `(-1i32).abs()` equals 1 /// ``` @@ -27,7 +28,7 @@ /// Method calls take precedence over unary precedence. Setting the /// precedence explicitly makes the code clearer and avoid potential bugs. pub AMBIGUOUS_NEGATIVE_LITERALS, - Deny, + Allow, "ambiguous negative literals operations", report_in_external_macro } diff --git a/tests/ui/lint/negative_literals.rs b/tests/ui/lint/negative_literals.rs index 048fcd6ff57..5964bbb559a 100644 --- a/tests/ui/lint/negative_literals.rs +++ b/tests/ui/lint/negative_literals.rs @@ -1,5 +1,7 @@ //@ check-fail +#![deny(ambiguous_negative_literals)] + fn main() { let _ = -1i32.abs(); //~^ ERROR `-` has lower precedence than method calls diff --git a/tests/ui/lint/negative_literals.stderr b/tests/ui/lint/negative_literals.stderr index df000a71882..b0323cc96a0 100644 --- a/tests/ui/lint/negative_literals.stderr +++ b/tests/ui/lint/negative_literals.stderr @@ -1,11 +1,15 @@ error: `-` has lower precedence than method calls, which might be unexpected - --> $DIR/negative_literals.rs:4:13 + --> $DIR/negative_literals.rs:6:13 | LL | let _ = -1i32.abs(); | ^^^^^^^^^^^ | = note: e.g. `-4.abs()` equals `-4`; while `(-4).abs()` equals `4` - = note: `#[deny(ambiguous_negative_literals)]` on by default +note: the lint level is defined here + --> $DIR/negative_literals.rs:3:9 + | +LL | #![deny(ambiguous_negative_literals)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: add parentheses around the `-` and the literal to call the method on a negative literal | LL | let _ = (-1i32).abs(); @@ -16,7 +20,7 @@ LL | let _ = -(1i32.abs()); | + + error: `-` has lower precedence than method calls, which might be unexpected - --> $DIR/negative_literals.rs:6:13 + --> $DIR/negative_literals.rs:8:13 | LL | let _ = -1f32.abs(); | ^^^^^^^^^^^ @@ -32,7 +36,7 @@ LL | let _ = -(1f32.abs()); | + + error: `-` has lower precedence than method calls, which might be unexpected - --> $DIR/negative_literals.rs:8:13 + --> $DIR/negative_literals.rs:10:13 | LL | let _ = -1f64.asin(); | ^^^^^^^^^^^^ @@ -48,7 +52,7 @@ LL | let _ = -(1f64.asin()); | + + error: `-` has lower precedence than method calls, which might be unexpected - --> $DIR/negative_literals.rs:10:13 + --> $DIR/negative_literals.rs:12:13 | LL | let _ = -1f64.asinh(); | ^^^^^^^^^^^^^ @@ -64,7 +68,7 @@ LL | let _ = -(1f64.asinh()); | + + error: `-` has lower precedence than method calls, which might be unexpected - --> $DIR/negative_literals.rs:12:13 + --> $DIR/negative_literals.rs:14:13 | LL | let _ = -1f64.tan(); | ^^^^^^^^^^^ @@ -80,7 +84,7 @@ LL | let _ = -(1f64.tan()); | + + error: `-` has lower precedence than method calls, which might be unexpected - --> $DIR/negative_literals.rs:14:13 + --> $DIR/negative_literals.rs:16:13 | LL | let _ = -1f64.tanh(); | ^^^^^^^^^^^^ @@ -96,7 +100,7 @@ LL | let _ = -(1f64.tanh()); | + + error: `-` has lower precedence than method calls, which might be unexpected - --> $DIR/negative_literals.rs:16:13 + --> $DIR/negative_literals.rs:18:13 | LL | let _ = -1.0_f64.cos().cos(); | ^^^^^^^^^^^^^^^^^^^^ @@ -112,7 +116,7 @@ LL | let _ = -(1.0_f64.cos().cos()); | + + error: `-` has lower precedence than method calls, which might be unexpected - --> $DIR/negative_literals.rs:18:13 + --> $DIR/negative_literals.rs:20:13 | LL | let _ = -1.0_f64.cos().sin(); | ^^^^^^^^^^^^^^^^^^^^ @@ -128,7 +132,7 @@ LL | let _ = -(1.0_f64.cos().sin()); | + + error: `-` has lower precedence than method calls, which might be unexpected - --> $DIR/negative_literals.rs:20:13 + --> $DIR/negative_literals.rs:22:13 | LL | let _ = -1.0_f64.sin().cos(); | ^^^^^^^^^^^^^^^^^^^^ @@ -144,7 +148,7 @@ LL | let _ = -(1.0_f64.sin().cos()); | + + error: `-` has lower precedence than method calls, which might be unexpected - --> $DIR/negative_literals.rs:22:13 + --> $DIR/negative_literals.rs:24:13 | LL | let _ = -1f64.sin().sin(); | ^^^^^^^^^^^^^^^^^ @@ -160,7 +164,7 @@ LL | let _ = -(1f64.sin().sin()); | + + error: `-` has lower precedence than method calls, which might be unexpected - --> $DIR/negative_literals.rs:25:11 + --> $DIR/negative_literals.rs:27:11 | LL | dbg!( -1.0_f32.cos() ); | ^^^^^^^^^^^^^^ From 14a0963f96a68fe4d0cb18cbe9ef052890a58e6e Mon Sep 17 00:00:00 2001 From: Ding Xiang Fei Date: Wed, 31 Jul 2024 22:50:45 +0800 Subject: [PATCH 9/9] reject pointee without ?Sized --- .../src/deriving/smart_ptr.rs | 50 +++++++++---------- .../ui/deriving/deriving-smart-pointer-neg.rs | 30 +++++++++++ .../deriving-smart-pointer-neg.stderr | 26 ++++++---- 3 files changed, 69 insertions(+), 37 deletions(-) diff --git a/compiler/rustc_builtin_macros/src/deriving/smart_ptr.rs b/compiler/rustc_builtin_macros/src/deriving/smart_ptr.rs index 02555bd799c..7eb1f17a59c 100644 --- a/compiler/rustc_builtin_macros/src/deriving/smart_ptr.rs +++ b/compiler/rustc_builtin_macros/src/deriving/smart_ptr.rs @@ -1,5 +1,3 @@ -use std::mem::swap; - use ast::ptr::P; use ast::HasAttrs; use rustc_ast::mut_visit::MutVisitor; @@ -154,13 +152,28 @@ pub fn expand_deriving_smart_ptr( { let pointee = &mut impl_generics.params[pointee_param_idx]; self_bounds = pointee.bounds.clone(); + if !contains_maybe_sized_bound(&self_bounds) + && !contains_maybe_sized_bound_on_pointee( + &generics.where_clause.predicates, + pointee_ty_ident.name, + ) + { + cx.dcx() + .struct_span_err( + pointee_ty_ident.span, + format!( + "`derive(SmartPointer)` requires {} to be marked `?Sized`", + pointee_ty_ident.name + ), + ) + .emit(); + return; + } let arg = GenericArg::Type(s_ty.clone()); let unsize = cx.path_all(span, true, path!(span, core::marker::Unsize), vec![arg]); pointee.bounds.push(cx.trait_bound(unsize, false)); - let mut attrs = thin_vec![]; - swap(&mut pointee.attrs, &mut attrs); // Drop `#[pointee]` attribute since it should not be recognized outside `derive(SmartPointer)` - pointee.attrs = attrs.into_iter().filter(|attr| !attr.has_name(sym::pointee)).collect(); + pointee.attrs.retain(|attr| !attr.has_name(sym::pointee)); } // # Rewrite generic parameter bounds @@ -169,14 +182,14 @@ pub fn expand_deriving_smart_ptr( // ``` // struct< // U: Trait, - // #[pointee] T: Trait, + // #[pointee] T: Trait + ?Sized, // V: Trait> ... // ``` // ... generates this `impl` generic parameters // ``` // impl< // U: Trait + Trait<__S>, - // T: Trait + Unsize<__S>, // (**) + // T: Trait + ?Sized + Unsize<__S>, // (**) // __S: Trait<__S> + ?Sized, // (*) // V: Trait + Trait<__S>> ... // ``` @@ -218,23 +231,6 @@ pub fn expand_deriving_smart_ptr( // // We now insert `__S` with the missing bounds marked with (*) above. // We should also write the bounds from `#[pointee]` to `__S` as required by `Unsize<__S>`. - let sized = cx.path_global(span, path!(span, core::marker::Sized)); - // For some reason, we are not allowed to write `?Sized` bound twice like `__S: ?Sized + ?Sized`. - if !contains_maybe_sized_bound(&self_bounds) - && !contains_maybe_sized_bound_on_pointee( - &generics.where_clause.predicates, - pointee_ty_ident.name, - ) - { - self_bounds.push(GenericBound::Trait( - cx.poly_trait_ref(span, sized), - TraitBoundModifiers { - polarity: ast::BoundPolarity::Maybe(span), - constness: ast::BoundConstness::Never, - asyncness: ast::BoundAsyncness::Normal, - }, - )); - } { let mut substitution = TypeSubstitution { from_name: pointee_ty_ident.name, to_ty: &s_ty, rewritten: false }; @@ -252,7 +248,7 @@ pub fn expand_deriving_smart_ptr( // where // U: Trait + Trait, // Companion: Trait, - // T: Trait, + // T: Trait + ?Sized, // { .. } // ``` // ... will have a impl prelude like so @@ -263,8 +259,8 @@ pub fn expand_deriving_smart_ptr( // U: Trait<__S>, // Companion: Trait, // Companion<__S>: Trait<__S>, - // T: Trait, - // __S: Trait<__S>, + // T: Trait + ?Sized, + // __S: Trait<__S> + ?Sized, // ``` // // We should also write a few new `where` bounds from `#[pointee] T` to `__S` diff --git a/tests/ui/deriving/deriving-smart-pointer-neg.rs b/tests/ui/deriving/deriving-smart-pointer-neg.rs index bfb4e86b396..04f52a154fe 100644 --- a/tests/ui/deriving/deriving-smart-pointer-neg.rs +++ b/tests/ui/deriving/deriving-smart-pointer-neg.rs @@ -1,5 +1,6 @@ #![feature(derive_smart_pointer, arbitrary_self_types)] +extern crate core; use std::marker::SmartPointer; #[derive(SmartPointer)] @@ -35,6 +36,13 @@ struct NotTransparent<'a, #[pointee] T: ?Sized> { ptr: &'a T, } +#[derive(SmartPointer)] +#[repr(transparent)] +struct NoMaybeSized<'a, #[pointee] T> { + //~^ ERROR: `derive(SmartPointer)` requires T to be marked `?Sized` + ptr: &'a T, +} + // However, reordering attributes should work nevertheless. #[repr(transparent)] #[derive(SmartPointer)] @@ -42,4 +50,26 @@ struct ThisIsAPossibleSmartPointer<'a, #[pointee] T: ?Sized> { ptr: &'a T, } +// Also, these paths to Sized should work +#[derive(SmartPointer)] +#[repr(transparent)] +struct StdSized<'a, #[pointee] T: ?std::marker::Sized> { + ptr: &'a T, +} +#[derive(SmartPointer)] +#[repr(transparent)] +struct CoreSized<'a, #[pointee] T: ?core::marker::Sized> { + ptr: &'a T, +} +#[derive(SmartPointer)] +#[repr(transparent)] +struct GlobalStdSized<'a, #[pointee] T: ?::std::marker::Sized> { + ptr: &'a T, +} +#[derive(SmartPointer)] +#[repr(transparent)] +struct GlobalCoreSized<'a, #[pointee] T: ?::core::marker::Sized> { + ptr: &'a T, +} + fn main() {} diff --git a/tests/ui/deriving/deriving-smart-pointer-neg.stderr b/tests/ui/deriving/deriving-smart-pointer-neg.stderr index d994a6ee376..8b0f91d41fb 100644 --- a/tests/ui/deriving/deriving-smart-pointer-neg.stderr +++ b/tests/ui/deriving/deriving-smart-pointer-neg.stderr @@ -1,5 +1,5 @@ error: `SmartPointer` can only be derived on `struct`s with `#[repr(transparent)]` - --> $DIR/deriving-smart-pointer-neg.rs:5:10 + --> $DIR/deriving-smart-pointer-neg.rs:6:10 | LL | #[derive(SmartPointer)] | ^^^^^^^^^^^^ @@ -7,7 +7,7 @@ LL | #[derive(SmartPointer)] = note: this error originates in the derive macro `SmartPointer` (in Nightly builds, run with -Z macro-backtrace for more info) error: At least one generic type should be designated as `#[pointee]` in order to derive `SmartPointer` traits - --> $DIR/deriving-smart-pointer-neg.rs:11:10 + --> $DIR/deriving-smart-pointer-neg.rs:12:10 | LL | #[derive(SmartPointer)] | ^^^^^^^^^^^^ @@ -15,7 +15,7 @@ LL | #[derive(SmartPointer)] = note: this error originates in the derive macro `SmartPointer` (in Nightly builds, run with -Z macro-backtrace for more info) error: `SmartPointer` can only be derived on `struct`s with at least one field - --> $DIR/deriving-smart-pointer-neg.rs:18:10 + --> $DIR/deriving-smart-pointer-neg.rs:19:10 | LL | #[derive(SmartPointer)] | ^^^^^^^^^^^^ @@ -23,7 +23,7 @@ LL | #[derive(SmartPointer)] = note: this error originates in the derive macro `SmartPointer` (in Nightly builds, run with -Z macro-backtrace for more info) error: `SmartPointer` can only be derived on `struct`s with at least one field - --> $DIR/deriving-smart-pointer-neg.rs:25:10 + --> $DIR/deriving-smart-pointer-neg.rs:26:10 | LL | #[derive(SmartPointer)] | ^^^^^^^^^^^^ @@ -31,15 +31,21 @@ LL | #[derive(SmartPointer)] = note: this error originates in the derive macro `SmartPointer` (in Nightly builds, run with -Z macro-backtrace for more info) error: `SmartPointer` can only be derived on `struct`s with `#[repr(transparent)]` - --> $DIR/deriving-smart-pointer-neg.rs:32:10 + --> $DIR/deriving-smart-pointer-neg.rs:33:10 | LL | #[derive(SmartPointer)] | ^^^^^^^^^^^^ | = note: this error originates in the derive macro `SmartPointer` (in Nightly builds, run with -Z macro-backtrace for more info) +error: `derive(SmartPointer)` requires T to be marked `?Sized` + --> $DIR/deriving-smart-pointer-neg.rs:41:36 + | +LL | struct NoMaybeSized<'a, #[pointee] T> { + | ^ + error[E0392]: lifetime parameter `'a` is never used - --> $DIR/deriving-smart-pointer-neg.rs:21:16 + --> $DIR/deriving-smart-pointer-neg.rs:22:16 | LL | struct NoField<'a, #[pointee] T: ?Sized> {} | ^^ unused lifetime parameter @@ -47,7 +53,7 @@ LL | struct NoField<'a, #[pointee] T: ?Sized> {} = help: consider removing `'a`, referring to it in a field, or using a marker such as `PhantomData` error[E0392]: type parameter `T` is never used - --> $DIR/deriving-smart-pointer-neg.rs:21:31 + --> $DIR/deriving-smart-pointer-neg.rs:22:31 | LL | struct NoField<'a, #[pointee] T: ?Sized> {} | ^ unused type parameter @@ -55,7 +61,7 @@ LL | struct NoField<'a, #[pointee] T: ?Sized> {} = help: consider removing `T`, referring to it in a field, or using a marker such as `PhantomData` error[E0392]: lifetime parameter `'a` is never used - --> $DIR/deriving-smart-pointer-neg.rs:28:20 + --> $DIR/deriving-smart-pointer-neg.rs:29:20 | LL | struct NoFieldUnit<'a, #[pointee] T: ?Sized>(); | ^^ unused lifetime parameter @@ -63,13 +69,13 @@ LL | struct NoFieldUnit<'a, #[pointee] T: ?Sized>(); = help: consider removing `'a`, referring to it in a field, or using a marker such as `PhantomData` error[E0392]: type parameter `T` is never used - --> $DIR/deriving-smart-pointer-neg.rs:28:35 + --> $DIR/deriving-smart-pointer-neg.rs:29:35 | LL | struct NoFieldUnit<'a, #[pointee] T: ?Sized>(); | ^ unused type parameter | = help: consider removing `T`, referring to it in a field, or using a marker such as `PhantomData` -error: aborting due to 9 previous errors +error: aborting due to 10 previous errors For more information about this error, try `rustc --explain E0392`.