reference icon indicating copy to clipboard operation
reference copied to clipboard

a more reasonable explanation for atomic operation on static is unsafe

Open zylthinking opened this issue 2 years ago • 1 comments
trafficstars

a more reasonable explanation for atomic operation on static is unsafe

zylthinking avatar Apr 17 '23 07:04 zylthinking

While you’re at it improving this example:

I think this example should probably also be re-written to use add_of_mut. There’s nothing “safe” about this function as it stands right now, under up-to-date Rust semantics.

Even something like this (that doesn’t even use the reference) is already considered UB by miri:

use std::thread;

static mut LEVELS: u32 = 0;

fn bump_levels_unsafe2() {
    let _ = unsafe { &mut LEVELS };
}

fn main() {
    thread::scope(|s| {
        s.spawn(bump_levels_unsafe2);
        s.spawn(bump_levels_unsafe2);
    });
}
error: Undefined Behavior: Data race detected between (1) non-atomic write on thread `<unnamed>` and (2) non-atomic write on thread `<unnamed>` at alloc1. (2) just happened here
 --> src/main.rs:6:22
  |
6 |     let _ = unsafe { &mut LEVELS };
  |                      ^^^^^^^^^^^ Data race detected between (1) non-atomic write on thread `<unnamed>` and (2) non-atomic write on thread `<unnamed>` at alloc1. (2) just happened here
  |
help: and (1) occurred earlier here
 --> src/main.rs:6:22
  |
6 |     let _ = unsafe { &mut LEVELS };
  |                      ^^^^^^^^^^^
  = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
  = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
  = note: BACKTRACE (of the first span):
  = note: inside `bump_levels_unsafe2` at src/main.rs:6:22: 6:33

Also, Rustc rightfully warns and suggests usage of addr_of_mut

warning: mutable reference of mutable static is discouraged
 --> src/main.rs:6:22
  |
6 |     let _ = unsafe { &mut LEVELS };
  |                      ^^^^^^^^^^^ mutable reference of mutable static
  |
  = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447>
  = note: reference of mutable static is a hard error from 2024 edition
  = note: mutable statics can be written to by multiple threads: aliasing violations or data races will cause undefined behavior
  = note: `#[warn(static_mut_ref)]` on by default
help: mutable references are dangerous since if there's any other pointer or reference used for that static while the reference lives, that's UB; use `addr_of_mut!` instead to create a raw pointer
  |
6 |     let _ = unsafe { addr_of_mut!(LEVELS) };
  |                      ~~~~~~~~~~~~~~~~~~~~

And given that it’s a very low-level thing, this atomic_add function, not using any safe wrapper type (like AtomicU32) but operating on the u32 directly, it’s only natural that *mut u32 is the right argument type here (not &mut u32!)… so the call would then look like atomic_add(addr_of_mut!(LEVELS), 1);


If you want, you can also improve the style and refactor the usage of early-return statements (return EXPR;) at the end of the functions into ordinary final expression in a block for return value.

steffahn avatar Jan 28 '24 08:01 steffahn

Closing in favor of #1544, thanks!

ehuss avatar Jul 23 '24 21:07 ehuss