Skip to content
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

compiler_fence documentation: emphasize synchronization, not reordering #129856

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Sep 1, 2024

Our fence docs have at some point been update to explain that they are about synchronization, not about "preventing reordering". This updates the compiler_fence docs n the same vein, mostly by referring to the fence docs.

The old docs make it sound like I can put a compiler_fence in the middle of a bunch of non-atomic operations and that would achieve any kind of guarantee. It does not, atomic operations are still required to do synchronization.

I also slightly tweaked the fence docs, to put the synchronization first and the "prevent reordering" second.

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

Fixes #129189
Fixes #54962

@rustbot
Copy link
Collaborator

rustbot commented Sep 1, 2024

r? @thomcc

rustbot has assigned @thomcc.
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 rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 1, 2024
@chorman0773
Copy link
Contributor

Ideally, I'd remove all of the mentions of reordering from the docs.

Otherwise, looks to be correct.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 1, 2024

Ideally, I'd remove all of the mentions of reordering from the docs.

Yeah... I didn't dare suggest that as it is such a common thing to talk about in weak memory models. So I opted for just emphasizing them less.

Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

///
/// The re-ordering prevented by the different ordering semantics are:
/// See [`fence`] for how a fence can be used to achieve synchronization. Note that just like
/// [`fence`], synchronization still requires atomic operations to be used in both threads -- it is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL, I thought they could be volatile too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah would be nice to use this for volatile ordering, but that's not per-spec... and IMO the fix for that is to make volatile operations also be atomic, but that project has stalled it seems.

@thomcc
Copy link
Member

thomcc commented Sep 2, 2024

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Sep 2, 2024

📌 Commit 32a30dd has been approved by thomcc

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 2, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 2, 2024
…iaskrgr

Rollup of 12 pull requests

Successful merges:

 - rust-lang#129748 (Box validity: update for new zero-sized rules)
 - rust-lang#129829 (Make decoding non-optional `LazyArray` panic if not set)
 - rust-lang#129856 (compiler_fence documentation: emphasize synchronization, not reordering)
 - rust-lang#129868 (Remove kobzol vacation status)
 - rust-lang#129875 (chore: Fix typos in 'compiler' (batch 1))
 - rust-lang#129877 (chore: Fix typos in 'compiler' (batch 2))
 - rust-lang#129878 (chore: Fix typos in 'compiler' (batch 3))
 - rust-lang#129890 (Remove stray word in a comment)
 - rust-lang#129892 (Clarify language around ptrs in slice::raw)
 - rust-lang#129905 (mailmap: add new email for davidtwco)
 - rust-lang#129906 (mailmapper?)
 - rust-lang#129907 (Fix compile error in solid's remove_dir_all)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f0072bf into rust-lang:master Sep 3, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 3, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 3, 2024
Rollup merge of rust-lang#129856 - RalfJung:compiler_fence, r=thomcc

compiler_fence documentation: emphasize synchronization, not reordering

Our `fence` docs have at some point been update to explain that they are about synchronization, not about "preventing reordering". This updates the `compiler_fence` docs n the same vein, mostly by referring to the `fence` docs.

The old docs make it sound like I can put a compiler_fence in the middle of a bunch of non-atomic operations and that would achieve any kind of guarantee. It does not, atomic operations are still required to do synchronization.

I also slightly tweaked the `fence` docs, to put the synchronization first and the "prevent reordering" second.

Cc `@rust-lang/opsem` `@chorman0773` `@m-ou-se`

Fixes rust-lang#129189
Fixes rust-lang#54962
@RalfJung RalfJung deleted the compiler_fence branch September 3, 2024 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

compiler_fence documentation is misleading compiler_fence SeqCst docs are incorrect
5 participants