Skip to content

Commit

Permalink
Merge #1
Browse files Browse the repository at this point in the history 
1: Remove miri hack r=taiki-e a=taiki-e

Use currently use a hack to avoid rust-lang/rust#69488 and to make sure that Miri errors for  atomic load/store of integers containing uninitialized bytes (which is probably not a problem and uncharted territory at best [1] [2] [3], and can be detected by `-Zmiri-check-number-validity` [4]), do not mask Miri errors for the use of uninitialized bytes (which is definitely a problem).

https://github.com/taiki-e/atomic-memcpy/blob/3507fef17534e4825b2b303d04702b4678e29dd0/src/lib.rs#L426-L450

[1]: crossbeam-rs/crossbeam#315 
[2]: rust-lang/unsafe-code-guidelines#158 
[3]: rust-lang/unsafe-code-guidelines#71 
[4]: rust-lang/miri#1904 

However, this actually causes another "unsupported operation" Miri error.

```
error: unsupported operation: unable to turn pointer into raw bytes
   --> /Users/taiki/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:701:9
    |
701 |         copy_nonoverlapping(src, tmp.as_mut_ptr(), 1);
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unable to turn pointer into raw bytes
    |
    = help: this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support
```


Co-authored-by: Taiki Endo <[email protected]>
  • Loading branch information
@bors @taiki-e
bors[bot] and taiki-e authored Feb 13, 2022
2 parents 3507fef + b1c9ebd commit a9de7a6
Show file tree
Hide file tree
Showing 6 changed files with 45 additions and 54 deletions.
7 changes: 4 additions & 3 deletions .github/workflows/ci.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,11 +146,12 @@ jobs:
component: miri
- run: cargo miri test --workspace -- --test-threads=1
env:
MIRIFLAGS: -Zmiri-symbolic-alignment-check -Zmiri-tag-raw-pointers -Zmiri-disable-isolation
# tests/padding.rs intentional contains tests for cases that are incompatible with -Zmiri-check-number-validity.
- run: cargo miri test --test test -- --test-threads=1
MIRIFLAGS: -Zmiri-check-number-validity -Zmiri-tag-raw-pointers -Zmiri-disable-isolation
- run: cargo miri test --workspace -- --test-threads=1
env:
MIRIFLAGS: -Zmiri-check-number-validity -Zmiri-symbolic-alignment-check -Zmiri-tag-raw-pointers -Zmiri-disable-isolation
# -Zmiri-symbolic-alignment-check is incompatible with the code that does manual integer arithmetic to ensure alignment.
RUSTFLAGS: ${{ env.RUSTFLAGS }} --cfg atomic_memcpy_symbolic_alignment_check_compat

san:
strategy:
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ Note: In this file, do not use the hard wrap in the middle of a sentence for com

## [Unreleased]

- Fix "unsupported operation: unable to turn pointer into raw bytes" Miri error. ([#1](https://github.com/taiki-e/atomic-memcpy/pull/1))

## [0.1.0] - 2022-02-12

Initial release
Expand Down
3 changes: 2 additions & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@ See [P1478R1][p1478r1] for more.
- If the alignment of the type being copied is the same as the pointer width, `atomic_load` is possible to produce an assembly roughly equivalent to the case of using volatile read + atomic fence on many platforms. (See [`tests/asm-test/asm`][asm-test] directory for more).
- If the alignment of the type being copied is smaller than the pointer width, there will be some performance degradation. However, it is implemented in such a way that it does not cause extreme performance degradation at least on x86_64. (See [the implementation comments of `atomic_load`][implementation] for more.) It is possible that there is still room for improvement, especially on non-x86_64 platforms.
- Optimization for the case where the alignment of the type being copied is larger than the pointer width has not yet been fully investigated. It is possible that there is still room for improvement, especially on 32-bit platforms where `AtomicU64` is available.
- If the type being copied contains uninitialized bytes (e.g., padding), it is incompatible with `-Zmiri-check-number-validity`. This will probably not be resolved until something like `AtomicMaybeUninit` is supported.
- If the type being copied contains uninitialized bytes (e.g., padding), it is incompatible with `-Zmiri-check-number-validity`. This will probably not be resolved until something like `AtomicMaybeUninit` is supported. **Note**: Due to [Miri cannot track uninitialized bytes on a per byte basis for partially initialized scalars][rust-lang/rust#69488], Miri may report this case as an access to an uninitialized byte, regardless of whether the uninitialized byte is actually accessed or not.

[p1478r1]: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1478r1.html
[implementation]: https://github.com/taiki-e/atomic-memcpy/blob/279d7041e48fae0943a50102ebab97e7ed3977ae/src/lib.rs#L359-L403
[asm-test]: tests/asm-test/asm
[rust-lang/rust#69488]: https://github.com/rust-lang/rust/issues/69488

## License

Expand Down
56 changes: 12 additions & 44 deletions src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,13 +213,17 @@ mod imp {
sync::atomic::{AtomicU16, AtomicUsize, Ordering},
};

#[cfg(not(atomic_memcpy_symbolic_alignment_check_compat))]
#[cfg(target_pointer_width = "32")]
type Half = u16;
#[cfg(not(atomic_memcpy_symbolic_alignment_check_compat))]
#[cfg(target_pointer_width = "32")]
type AtomicHalf = AtomicU16;

#[cfg(not(atomic_memcpy_symbolic_alignment_check_compat))]
#[cfg(target_pointer_width = "64")]
type Half = u32;
#[cfg(not(atomic_memcpy_symbolic_alignment_check_compat))]
#[cfg(target_pointer_width = "64")]
type AtomicHalf = AtomicU32;

Expand Down Expand Up @@ -327,9 +331,10 @@ mod imp {
}
}

#[cfg(not(atomic_memcpy_symbolic_alignment_check_compat))]
#[cfg(not(target_pointer_width = "16"))]
#[cfg_attr(feature = "inline-always", inline(always))]
#[cfg_attr(not(feature = "inline-always"), inline)]
#[cfg(not(target_pointer_width = "16"))]
pub(super) unsafe fn atomic_load_half(&mut self) {
use super::{AtomicHalf, Half};
debug_assert!(self.remaining() >= mem::size_of::<Half>());
Expand Down Expand Up @@ -423,45 +428,15 @@ mod imp {
return result;
}

// HACK: Miri cannot track uninitialized bytes on a per byte basis for
// partially initialized scalars: https://github.com/rust-lang/rust/issues/69488
//
// This hack allows miri to properly track the use of uninitialized
// bytes. See also tests/uninit.rs that is a test to check if
// valgrind/sanitizer/miri can properly detect the use of uninitialized
// bytes.
//
// Note: With or without this hack, atomic load/store of integers
// containing uninitialized bytes is technically an undefined behavior.
// The only purpose of this hack is to make sure that Miri errors for
// atomic load/store of integers containing uninitialized bytes
// (which is probably not a problem and uncharted territory at best [1] [2] [3],
// and can be detected by `-Zmiri-check-number-validity` [4]),
// do not mask Miri errors for the use of uninitialized bytes (which is definitely a problem).
// See also tests/padding.rs.
//
// [1] https://github.com/crossbeam-rs/crossbeam/issues/315
// [2] https://github.com/rust-lang/unsafe-code-guidelines/issues/158
// [3] https://github.com/rust-lang/unsafe-code-guidelines/issues/71
// [4] https://github.com/rust-lang/miri/pull/1904
//
// rust-lang/rust#69488 affects only CTFE(compile-time function evaluation)/Miri
// and atomic operations cannot be called in const context, so our code
// is only affected in the case of cfg(miri).
if cfg!(miri) {
let mut state = load::LoadState::new(result.as_mut_ptr(), src);
state.atomic_load_u8(state.remaining());
debug_assert_eq!(state.remaining(), 0);
return result;
}

// Branch 1: If the alignment of `T` is less than usize, but `T` can be read as
// at least one or more usize, compute the align offset and read it
// like `(&[AtomicU8], &[AtomicUsize], &[AtomicU8])`.
if mem::align_of::<T>() < mem::align_of::<AtomicUsize>()
&& mem::size_of::<T>() >= mem::size_of::<usize>() * 4
{
let mut state = load::LoadState::new(result.as_mut_ptr(), src);
// -Zmiri-symbolic-alignment-check is incompatible with the code that does manual integer arithmetic to ensure alignment.
#[cfg(not(atomic_memcpy_symbolic_alignment_check_compat))]
#[cfg(not(target_pointer_width = "16"))]
{
// Since the caller guarantees that the pointer is properly aligned,
Expand Down Expand Up @@ -702,9 +677,10 @@ mod imp {
}
}

#[cfg(not(atomic_memcpy_symbolic_alignment_check_compat))]
#[cfg(not(target_pointer_width = "16"))]
#[cfg_attr(feature = "inline-always", inline(always))]
#[cfg_attr(not(feature = "inline-always"), inline)]
#[cfg(not(target_pointer_width = "16"))]
pub(super) unsafe fn atomic_store_half(&mut self) {
use super::{AtomicHalf, Half};
debug_assert!(self.remaining() >= mem::size_of::<Half>());
Expand Down Expand Up @@ -769,25 +745,17 @@ mod imp {
return;
}

// HACK: See the `atomic_load` function for the detailed comment.
if cfg!(miri) {
let mut state = store::StoreState::new(dst, &*val);
state.atomic_store_u8(state.remaining());
debug_assert_eq!(state.remaining(), 0);
mem::forget(guard);
return;
}

// Branch 1: If the alignment of `T` is less than usize, but `T` can be write as
// at least one or more usize, compute the align offset and write it
// like `(&[AtomicU8], &[AtomicUsize], &[AtomicU8])`.
if mem::align_of::<T>() < mem::align_of::<AtomicUsize>()
&& mem::size_of::<T>() >= mem::size_of::<usize>() * 4
{
let mut state = store::StoreState::new(dst, &*val);
// See the `atomic_load` function for the detailed comment.
#[cfg(not(atomic_memcpy_symbolic_alignment_check_compat))]
#[cfg(not(target_pointer_width = "16"))]
{
// See the `atomic_load` function for the detailed comment.
if mem::align_of::<T>() >= mem::align_of::<Half>() {
if dst as usize % mem::size_of::<usize>() == 0 {
// SAFETY:
Expand Down
20 changes: 14 additions & 6 deletions tests/padding.rs
View file
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
// https://github.com/rust-lang/unsafe-code-guidelines/issues/71
// https://github.com/rust-lang/miri/pull/1904
//
// With miri:
// MIRIFLAGS='-Zmiri-check-number-validity' cargo miri test --test padding -- --test-threads=1

use std::{cell::UnsafeCell, mem, sync::atomic::Ordering};

use atomic_memcpy::{atomic_load, atomic_store};

#[test]
fn enum_padding() {
// Miri cannot track uninitialized bytes on a per byte basis for partially
// initialized scalars: https://github.com/rust-lang/rust/issues/69488
// See also https://github.com/crossbeam-rs/crossbeam/issues/748#issuecomment-1022432401
if cfg!(miri) {
return;
}

#[allow(dead_code)]
#[repr(align(8))]
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
Expand Down Expand Up @@ -37,6 +38,13 @@ fn enum_padding() {

#[test]
fn union_padding() {
// Miri cannot track uninitialized bytes on a per byte basis for partially
// initialized scalars: https://github.com/rust-lang/rust/issues/69488
// See also https://github.com/crossbeam-rs/crossbeam/issues/748#issuecomment-1022432401
if cfg!(miri) {
return;
}

#[allow(dead_code)]
#[repr(C, align(8))]
#[derive(Clone, Copy)]
Expand Down
11 changes: 11 additions & 0 deletions tests/test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,17 @@ fn basic_unit() {
}
}

#[test]
fn basic_ptr() {
unsafe {
let x = UnsafeCell::<*mut u8>::new(ptr::null_mut());
assert!(atomic_load(x.get(), Ordering::Relaxed).assume_init().is_null());
let mut v = 0u8;
atomic_store(x.get(), &mut v, Ordering::Relaxed);
assert!(!atomic_load(x.get(), Ordering::Relaxed).assume_init().is_null());
}
}

#[cfg(not(feature = "no-panic"))]
#[track_caller]
fn assert_panic<T: std::fmt::Debug>(f: impl FnOnce() -> T) -> std::string::String {
Expand Down

0 comments on commit a9de7a6

Please sign in to comment.