atomics: allow atomic and non-atomic reads to race

[RalfJung]

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 rust-lang/unsafe-code-guidelines#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

[Mark-Simulacrum]

Presumably this will want T-opsem and/or T-lang FCP. Left one comment on the current proposed wording though.

[RalfJung]

Previously in this documentation we used the term "non-synchronized" for accesses which are not happens-before ordered. I wonder if "unordered" would be a better term?

The C++ memory model does not define a term for this, they just spell out "not happens-before ordered".

[WaffleLapkin]

"unordered" sounds better to me, but 🤷🏻

[RalfJung]

@rust-lang/opsem @rust-lang/lang any opinion on this -- should we rename "non-synchronized" to "unordered"?

[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:

• @CAD97
• @JakobDegen
• @RalfJung
• @digama0
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @saethlin
• @scottmcm
• @tmandry
• @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.

[rfcbot]

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

[nikomatsakis]

@rfcbot reviewed

[RalfJung]

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

[bors]

☔ 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]

📌 Commit 96be76b has been approved by Mark-Simulacrum

It is now in the queue for this repository.

[bors]

⌛ Testing commit 96be76b with merge e6eb451...