atomics: allow atomic and non-atomic reads to race

[RalfJung]

trafficstars

We currently define our atomics in terms of C++ atomic_ref. That has the unfortunate side-effect of making it UB for an atomic and a non-atomic read to race (concretely, this code has UB). There's really no good reason for this, all the academic models of the C++ memory model I am aware of allow this -- C++ just disallows this because of their insistence on an "object model" with typed memory, where atomic_ref temporarily creates an "atomic object" that may not be accesses via regular non-atomic operations.

So instead of tying our operations to atomic_ref, let us tie them directly to the underlying C++ memory model. I am not sure what is the best way to phrase this, so here's a first attempt.

We also carve out an exception from the "no mixed-size atomic accesses" rule to permit mixed-size atomic reads -- given that we permit mixed-size non-atomic reads, it seems odd that this would be disallowed for atomic reads. However, when an atomic write races with any other atomic operation, they must use the same size.

With this change, it is finally the case that every non-atomic access can be replaced by an atomic access without introducing UB.

Cc @rust-lang/opsem @chorman0773 @m-ou-se @WaffleLapkin @Amanieu

Fixes https://github.com/rust-lang/unsafe-code-guidelines/issues/483

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum. They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[rust-log-analyzer]

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

## Running ui tests in tests/fail for x86_64-unknown-linux-gnu
   Compiler: "MIRI_ENV_VAR_TEST"="0" "MIRI_TEMP"="/tmp/miri-uitest-0ytfAN" "RUST_BACKTRACE"="1" /checkout/obj/build/x86_64-unknown-linux-gnu/stage1/bin/miri "--error-format=json" "--sysroot=/checkout/obj/build/x86_64-unknown-linux-gnu/miri-sysroot" "-Dwarnings" "-Dunused" "-Ainternal_features" "-Zui-testing" "--target" "x86_64-unknown-linux-gnu" "--out-dir" OUT_DIR

FAILED TEST: tests/fail/weak_memory/racing_mixed_size_read.rs
command: MIRI_ENV_VAR_TEST="0" MIRI_TEMP="/tmp/miri-uitest-0ytfAN" RUST_BACKTRACE="1" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/bin/miri" "--error-format=json" "--sysroot=/checkout/obj/build/x86_64-unknown-linux-gnu/miri-sysroot" "-Dwarnings" "-Dunused" "-Ainternal_features" "-Zui-testing" "--target" "x86_64-unknown-linux-gnu" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/miri_ui/tests/fail/weak_memory" "tests/fail/weak_memory/racing_mixed_size_read.rs" "-Zmiri-preemption-rate=0" "-Zmiri-address-reuse-cross-thread-rate=0" "--edition" "2021"
error: actual output differed from expected
Execute `./miri test --bless` to update `tests/fail/weak_memory/racing_mixed_size_read.stderr` to the actual output
--- tests/fail/weak_memory/racing_mixed_size_read.stderr
+++ <stderr output>
+++ <stderr output>
-error: Undefined Behavior: Race condition detected between (1) 4-byte atomic load on thread `unnamed-ID` and (2) 2-byte atomic load on thread `unnamed-ID` at ALLOC. (2) just happened here
+error: memory leaked: ALLOC (Rust heap, size: 4, align: 4), allocated here:
+  --> RUSTLIB/alloc/src/alloc.rs:LL:CC
    |
    |
-LL |             (*hi).load(Relaxed);
+LL |         __rust_alloc(layout.size(), layout.align())
-   |             ^^^^^^^^^^^^^^^^^^^ Race condition detected between (1) 4-byte atomic load on thread `unnamed-ID` and (2) 2-byte atomic load on thread `unnamed-ID` at ALLOC. (2) just happened here
    |
    |
-help: and (1) occurred earlier here
   --> $DIR/racing_mixed_size_read.rs:LL:CC
    |
    |
-LL |         x.load(Relaxed);
+LL |     let ret = Box::leak(Box::new(AtomicU32::new(val)));
+   |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-   = help: overlapping unsynchronized atomic accesses must use the same access size
+note: inside `main`
-   = help: see https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#memory-model-for-atomic-accesses for more information about the Rust memory model
-   = help: see https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#memory-model-for-atomic-accesses for more information about the Rust memory model
+  --> $DIR/racing_mixed_size_read.rs:LL:CC
-   = 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
+LL |     let x = static_atomic(0);
-   = note: BACKTRACE (of the first span) on thread `unnamed-ID`:
 
 note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
 
+note: set `MIRIFLAGS=-Zmiri-ignore-leaks` to disable this check
+note: set `MIRIFLAGS=-Zmiri-ignore-leaks` to disable this check
+
 error: aborting due to 1 previous error
 


error: `(1) 4-byte atomic load on thread `unnamed-1` and (2) 2-byte atomic load` not found in diagnostics on line 33
   |
   |
33 |             (*hi).load(Relaxed); //~ ERROR: (1) 4-byte atomic load on thread `unnamed-1` and (2) 2-byte atomic load
   |

error: there were 1 unmatched diagnostics that occurred outside the testfile and had no pattern
error: there were 1 unmatched diagnostics that occurred outside the testfile and had no pattern
    Error: memory leaked: alloc1099 (Rust heap, size: 4, align: 4), allocated here:
full stderr:
full stderr:
error: memory leaked: alloc1099 (Rust heap, size: 4, align: 4), allocated here:
   |
LL |         __rust_alloc(layout.size(), layout.align())
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
---
   = note: inside `std::boxed::Box::<std::sync::atomic::AtomicU32>::new` at /checkout/library/alloc/src/boxed.rs:257:9: 257:20
note: inside `static_atomic`
  --> tests/fail/weak_memory/racing_mixed_size_read.rs:10:25
   |
LL |     let ret = Box::leak(Box::new(AtomicU32::new(val)));
note: inside `main`
  --> tests/fail/weak_memory/racing_mixed_size_read.rs:22:13
   |
LL |     let x = static_atomic(0);
---
Error: 
   0: ui tests in tests/fail for x86_64-unknown-linux-gnu failed
   1: tests failed

Backtrace omitted. Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.
error: test failed, to rerun pass `--test ui`
Caused by:
  process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/x86_64-unknown-linux-gnu/release/deps/ui-358d384d97c64c78 --quiet` (exit status: 1)
  process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/x86_64-unknown-linux-gnu/release/deps/ui-358d384d97c64c78 --quiet` (exit status: 1)
Command has failed. Rerun with -v to see more details.
  local time: Sat Aug 10 18:17:05 UTC 2024
  network time: Sat, 10 Aug 2024 18:17:05 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

[RalfJung]

Nominating for t-lang to get their take on this, and to ask them who should be included in the FCP -- just t-opsem, or also t-lang?

[traviscross]

@rfcbot fcp merge

We discussed this in triage today. This sounded right to us. We'll do this via FCP with T-opsem.

[rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• [x] @CAD97
• [x] @JakobDegen
• [x] @RalfJung
• [ ] @digama0
• [x] @joshtriplett
• [x] @nikomatsakis
• [x] @pnkfelix
• [x] @saethlin
• [x] @scottmcm
• [x] @tmandry
• [x] @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. See this document for info about what commands tagged team members can give me.

[scottmcm]

I find the argument that it should always be ok to make a non-atomic read into an atomic read persuasive, so it sounds good from an intent perspective. So as long as the experts agree with how we're formally saying that, sounds good.

@rfcbot reviewed

(I wonder if it's possible to write a codegen test that would have a reasonable chance of noticing LLVM deciding to turn such a read-race into unreachable. That's not blocking here, though.)

[RalfJung]

LLVM doesn't have a notion of "atomic object", their entire memory model is access-based, so I can't imagine they'd ever make this UB. I also don't think LLVM turns any obvious data races into unreachable, it "just" performs transformations that exploit the no-data-race assumption, so I can't think of a way to write a test like that.

[Noratrieb]

You mentioned that you discussed this in the lang meeting, so I removed the label.

[tmandry]

@rfcbot reviewed

[RalfJung]

@digama0 @nikomatsakis @pnkfelix @saethlin friendly FCP reminder ping :)

[RalfJung]

@digama0 @nikomatsakis @pnkfelix @saethlin there's an FCP waiting for you here :)

[bors]

:umbrella: The latest upstream changes (presumably #130401) made this pull request unmergeable. Please resolve the merge conflicts.

[rfcbot]

:bell: This is now entering its final comment period, as per the review above. :bell:

[nikomatsakis]

@rfcbot reviewed

[RalfJung]

@Mark-Simulacrum is this ready to land from your side, assuming FCP will pass uneventful?

[bors]

:umbrella: The latest upstream changes (presumably #130483) made this pull request unmergeable. Please resolve the merge conflicts.

[Mark-Simulacrum]

Yeah, r=me

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[RalfJung]

@bors r=Mark-Simulacrum

[bors]

:pushpin: Commit 96be76bf53b2a9472f03786ccc3b291196e5a77d has been approved by Mark-Simulacrum

It is now in the queue for this repository.

[bors]

:hourglass: Testing commit 96be76bf53b2a9472f03786ccc3b291196e5a77d with merge e6eb45143cc4dd236dacd6fd8dc74eb1ee6852b2...