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

#[deny(unsafe_op_in_unsafe_fn)] in sys/wasm #74477

Merged
merged 5 commits into from
Oct 26, 2020
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
16 changes: 12 additions & 4 deletions library/std/src/sys/wasm/alloc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,26 +24,34 @@ static mut DLMALLOC: dlmalloc::Dlmalloc = dlmalloc::DLMALLOC_INIT;
unsafe impl GlobalAlloc for System {
#[inline]
unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
// SAFETY: DLMALLOC access is guranteed to be safe because the lock gives us unique and non-reentrant access.
// Calling malloc() is safe because preconditions on this function match the trait method preconditions.
let _lock = lock::lock();
DLMALLOC.malloc(layout.size(), layout.align())
unsafe { DLMALLOC.malloc(layout.size(), layout.align()) }
Copy link
Member

Choose a reason for hiding this comment

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

There are two preconditions that we need to document for these calls:

  • DLMALLOC access is safe: this is true because the lock gives us unique and non-reentrant access.
  • Calling the function itself is safe: Preconditions on these functions match the trait method preconditions, so this is safe.

The comments so far only sort of talk about the first, but should address both. I think basically copy/pasting my two bullet points makes sense.

}

#[inline]
unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
// SAFETY: DLMALLOC access is guranteed to be safe because the lock gives us unique and non-reentrant access.
// Calling calloc() is safe because preconditions on this function match the trait method preconditions.
let _lock = lock::lock();
DLMALLOC.calloc(layout.size(), layout.align())
unsafe { DLMALLOC.calloc(layout.size(), layout.align()) }
}

#[inline]
unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
// SAFETY: DLMALLOC access is guranteed to be safe because the lock gives us unique and non-reentrant access.
// Calling free() is safe because preconditions on this function match the trait method preconditions.
let _lock = lock::lock();
DLMALLOC.free(ptr, layout.size(), layout.align())
unsafe { DLMALLOC.free(ptr, layout.size(), layout.align()) }
}

#[inline]
unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 {
// SAFETY: DLMALLOC access is guranteed to be safe because the lock gives us unique and non-reentrant access.
// Calling realloc() is safe because preconditions on this function match the trait method preconditions.
let _lock = lock::lock();
DLMALLOC.realloc(ptr, layout.size(), layout.align(), new_size)
unsafe { DLMALLOC.realloc(ptr, layout.size(), layout.align(), new_size) }
}
}

Expand Down
10 changes: 8 additions & 2 deletions library/std/src/sys/wasm/condvar_atomics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,19 @@ impl Condvar {

pub unsafe fn notify_one(&self) {
self.cnt.fetch_add(1, SeqCst);
wasm32::memory_atomic_notify(self.ptr(), 1);
// SAFETY: ptr() is always valid
unsafe {
wasm32::memory_atomic_notify(self.ptr(), 1);
}
}

#[inline]
pub unsafe fn notify_all(&self) {
self.cnt.fetch_add(1, SeqCst);
wasm32::memory_atomic_notify(self.ptr(), u32::MAX); // -1 == "wake everyone"
// SAFETY: ptr() is always valid
unsafe {
wasm32::memory_atomic_notify(self.ptr(), u32::MAX); // -1 == "wake everyone"
Mark-Simulacrum marked this conversation as resolved.
Show resolved Hide resolved
}
}

pub unsafe fn wait(&self, mutex: &Mutex) {
Expand Down
2 changes: 2 additions & 0 deletions library/std/src/sys/wasm/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
//! compiling for wasm. That way it's a compile time error for something that's
//! guaranteed to be a runtime error!

#![deny(unsafe_op_in_unsafe_fn)]

pub mod alloc;
pub mod args;
#[path = "../unsupported/cmath.rs"]
Expand Down
25 changes: 16 additions & 9 deletions library/std/src/sys/wasm/mutex_atomics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,14 @@ impl Mutex {

pub unsafe fn lock(&self) {
while !self.try_lock() {
let val = wasm32::memory_atomic_wait32(
self.ptr(),
1, // we expect our mutex is locked
-1, // wait infinitely
);
// SAFETY: the caller must uphold the safety contract for `memory_atomic_wait32`.
Mark-Simulacrum marked this conversation as resolved.
Show resolved Hide resolved
let val = unsafe {
wasm32::memory_atomic_wait32(
self.ptr(),
1, // we expect our mutex is locked
-1, // wait infinitely
)
};
// we should have either woke up (0) or got a not-equal due to a
// race (1). We should never time out (2)
debug_assert!(val == 0 || val == 1);
Expand Down Expand Up @@ -93,19 +96,20 @@ impl ReentrantMutex {
pub unsafe fn lock(&self) {
let me = thread::my_id();
while let Err(owner) = self._try_lock(me) {
let val = wasm32::memory_atomic_wait32(self.ptr(), owner as i32, -1);
// SAFETY: the caller must gurantee that `self.ptr()` and `owner` are valid i32.
let val = unsafe { wasm32::memory_atomic_wait32(self.ptr(), owner as i32, -1) };
debug_assert!(val == 0 || val == 1);
}
}

#[inline]
pub unsafe fn try_lock(&self) -> bool {
self._try_lock(thread::my_id()).is_ok()
unsafe { self._try_lock(thread::my_id()).is_ok() }
}

#[inline]
unsafe fn _try_lock(&self, id: u32) -> Result<(), u32> {
let id = id.checked_add(1).unwrap(); // make sure `id` isn't 0
let id = id.checked_add(1).unwrap();
match self.owner.compare_exchange(0, id, SeqCst, SeqCst) {
// we transitioned from unlocked to locked
Ok(_) => {
Expand All @@ -132,7 +136,10 @@ impl ReentrantMutex {
match *self.recursions.get() {
0 => {
self.owner.swap(0, SeqCst);
wasm32::memory_atomic_notify(self.ptr() as *mut i32, 1); // wake up one waiter, if any
// SAFETY: the caller must gurantee that `self.ptr()` is valid i32.
unsafe {
wasm32::atomic_notify(self.ptr() as *mut i32, 1);
} // wake up one waiter, if any
Copy link
Member

Choose a reason for hiding this comment

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

Can you put this comment above the unsafe block instead?

}
ref mut n => *n -= 1,
}
Expand Down