-
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Remove miri hack #1
Conversation
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 ```
bors r+ |
Build succeeded: |
@@ -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. |
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.
Just noticed this by coincidence, and I am trying to understand what happens here.
If I understand correctly, this code effectively performs a copy at a type like u64
, and due to rust-lang/rust#69488, that "inflates" uninit which causes problems. However, if that is the case, it will cause problems even without -Zmiri-check-number-validity
, the problems are just slightly delayed: if x
is uninit, x = 1
will fail in Miri even without that flag.
Moreover, arguably, this is not just a Miri limitation, it actually accurately reflects the reference. I have ideas for fixing rust-lang/rust#69488; they will not fix the problem described here.
Finally, if we start emitting noundef
flags for LLVM on our integer types (which the reference says we can), then the approach in this crate will be problematic even if we ignore Rust UB and consider only LLVM UB.
So, I agree that AtomicMaybeUninit
is the solution -- but until we have that, the problem isn't just a limitation in Miri, it is a limitation in Rust generally.
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.
Thanks. I agree that the current readme is incorrect. (Filed #5 to fix readme)
5: Fix docs about uninitialized bytes r=taiki-e a=taiki-e Based on [the feedback](#1 (comment)) from `@RalfJung.` Co-authored-by: Taiki Endo <te316e89@gmail.com>
We 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).atomic-memcpy/src/lib.rs
Lines 426 to 450 in 3507fef
However, this actually causes another "unsupported operation" Miri error.