Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

kmc-solid: std::sys code maintenance #105120

Merged
merged 4 commits into from
Dec 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions library/std/src/sys/itron/condvar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ impl Condvar {
}
}

unsafe { mutex.lock() };
mutex.lock();
}

pub unsafe fn wait_timeout(&self, mutex: &Mutex, dur: Duration) -> bool {
Expand Down Expand Up @@ -109,7 +109,7 @@ impl Condvar {
// we woke up because of `notify_*`.
let success = self.waiters.with_locked(|waiters| unsafe { !waiters.remove(waiter) });

unsafe { mutex.lock() };
mutex.lock();
success
}
}
Expand Down
2 changes: 1 addition & 1 deletion library/std/src/sys/itron/mutex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ pub(super) struct MutexGuard<'a>(&'a Mutex);
impl<'a> MutexGuard<'a> {
#[inline]
pub(super) fn lock(x: &'a Mutex) -> Self {
unsafe { x.lock() };
x.lock();
Self(x)
}
}
Expand Down
48 changes: 30 additions & 18 deletions library/std/src/sys/itron/thread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,25 @@ use crate::{
ffi::CStr,
hint, io,
mem::ManuallyDrop,
ptr::NonNull,
sync::atomic::{AtomicUsize, Ordering},
sys::thread_local_dtor::run_dtors,
time::Duration,
};

pub struct Thread {
inner: ManuallyDrop<Box<ThreadInner>>,
p_inner: NonNull<ThreadInner>,

/// The ID of the underlying task.
task: abi::ID,
}

// Safety: There's nothing in `Thread` that ties it to the original creator. It
// can be dropped by any threads.
unsafe impl Send for Thread {}
// Safety: `Thread` provides no methods that take `&self`.
unsafe impl Sync for Thread {}
Comment on lines +30 to +31
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not intuitive to me that implementing a function or not that takes &self affects safety for this case, as borrowing is a primitive notion in Rust, so anyone can hypothetically take &Thread from a given Thread and run off with it? Could you explain that one?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the same reasoning as the one for Exclusive: since you cannot do anything with the reference, the struct can be Sync without causing any UB.

I'm not sure this is the best reasoning here, but it is correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, so it's actually talking about Thread as a "smart pointer" (wrapper around the pointer). It interposes and blocks access to the pointer inside, and then never allows you to touch that inner pointer using a method that would take &Thread. Thus it's correct to unsafe impl Sync for Thread, since the behavior of the type can only be accessed via &mut Thread or Thread.

Yeah for some reason that didn't actually register immediately when I read it.


/// State data shared between a parent thread and child thread. It's dropped on
/// a transition to one of the final states.
struct ThreadInner {
Expand Down Expand Up @@ -90,8 +97,9 @@ impl Thread {
});

unsafe extern "C" fn trampoline(exinf: isize) {
let p_inner: *mut ThreadInner = crate::ptr::from_exposed_addr_mut(exinf as usize);
// Safety: `ThreadInner` is alive at this point
let inner = unsafe { &*(exinf as *const ThreadInner) };
let inner = unsafe { &*p_inner };

// Safety: Since `trampoline` is called only once for each
// `ThreadInner` and only `trampoline` touches `start`,
Expand Down Expand Up @@ -119,13 +127,13 @@ impl Thread {
// No one will ever join, so we'll ask the collector task to
// delete the task.

// In this case, `inner`'s ownership has been moved to us,
// And we are responsible for dropping it. The acquire
// In this case, `*p_inner`'s ownership has been moved to
// us, and we are responsible for dropping it. The acquire
// ordering is not necessary because the parent thread made
// no memory access needing synchronization since the call
// to `acre_tsk`.
// Safety: See above.
let _ = unsafe { Box::from_raw(inner as *const _ as *mut ThreadInner) };
let _ = unsafe { Box::from_raw(p_inner) };

// Safety: There are no pinned references to the stack
unsafe { terminate_and_delete_current_task() };
Expand Down Expand Up @@ -162,13 +170,14 @@ impl Thread {
}
}

let inner_ptr = (&*inner) as *const ThreadInner;
// Safety: `Box::into_raw` returns a non-null pointer
let p_inner = unsafe { NonNull::new_unchecked(Box::into_raw(inner)) };

let new_task = ItronError::err_if_negative(unsafe {
abi::acre_tsk(&abi::T_CTSK {
// Activate this task immediately
tskatr: abi::TA_ACT,
exinf: inner_ptr as abi::EXINF,
exinf: p_inner.as_ptr().expose_addr() as abi::EXINF,
// The entry point
task: Some(trampoline),
// Inherit the calling task's base priority
Expand All @@ -180,7 +189,7 @@ impl Thread {
})
.map_err(|e| e.as_io_error())?;

Ok(Self { inner: ManuallyDrop::new(inner), task: new_task })
Ok(Self { p_inner, task: new_task })
}

pub fn yield_now() {
Expand All @@ -197,8 +206,9 @@ impl Thread {
}
}

pub fn join(mut self) {
let inner = &*self.inner;
pub fn join(self) {
// Safety: `ThreadInner` is alive at this point
let inner = unsafe { self.p_inner.as_ref() };
// Get the current task ID. Panicking here would cause a resource leak,
// so just abort on failure.
let current_task = task::current_task_id_aborting();
Expand Down Expand Up @@ -243,8 +253,8 @@ impl Thread {
unsafe { terminate_and_delete_task(self.task) };

// In either case, we are responsible for dropping `inner`.
// Safety: The contents of `self.inner` will not be accessed hereafter
let _inner = unsafe { ManuallyDrop::take(&mut self.inner) };
// Safety: The contents of `*p_inner` will not be accessed hereafter
let _inner = unsafe { Box::from_raw(self.p_inner.as_ptr()) };

// Skip the destructor (because it would attempt to detach the thread)
crate::mem::forget(self);
Expand All @@ -253,13 +263,16 @@ impl Thread {

impl Drop for Thread {
fn drop(&mut self) {
// Safety: `ThreadInner` is alive at this point
let inner = unsafe { self.p_inner.as_ref() };

// Detach the thread.
match self.inner.lifecycle.swap(LIFECYCLE_DETACHED_OR_JOINED, Ordering::Acquire) {
match inner.lifecycle.swap(LIFECYCLE_DETACHED_OR_JOINED, Ordering::Acquire) {
LIFECYCLE_INIT => {
// [INIT → DETACHED]
// When the time comes, the child will figure out that no
// one will ever join it.
// The ownership of `self.inner` is moved to the child thread.
// The ownership of `*p_inner` is moved to the child thread.
// However, the release ordering is not necessary because we
// made no memory access needing synchronization since the call
// to `acre_tsk`.
Expand All @@ -278,10 +291,9 @@ impl Drop for Thread {
// delete by entering the `FINISHED` state.
unsafe { terminate_and_delete_task(self.task) };

// Wwe are responsible for dropping `inner`.
// Safety: The contents of `self.inner` will not be accessed
// hereafter
unsafe { ManuallyDrop::drop(&mut self.inner) };
// Wwe are responsible for dropping `*p_inner`.
// Safety: The contents of `*p_inner` will not be accessed hereafter
let _ = unsafe { Box::from_raw(self.p_inner.as_ptr()) };
}
_ => unsafe { hint::unreachable_unchecked() },
}
Expand Down
4 changes: 4 additions & 0 deletions library/std/src/sys/solid/io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,3 +75,7 @@ impl<'a> IoSliceMut<'a> {
unsafe { slice::from_raw_parts_mut(self.vec.iov_base as *mut u8, self.vec.iov_len) }
}
}

pub fn is_terminal<T>(_: &T) -> bool {
false
}
3 changes: 1 addition & 2 deletions library/std/src/sys/solid/os.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use super::unsupported;
use crate::convert::TryFrom;
use crate::error::Error as StdError;
use crate::ffi::{CStr, CString, OsStr, OsString};
use crate::ffi::{CStr, OsStr, OsString};
use crate::fmt;
use crate::io;
use crate::os::{
Expand Down