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 is misleading #129189

Closed
RalfJung opened this issue Aug 17, 2024 · 1 comment · Fixed by #129856
Closed

compiler_fence documentation is misleading #129189

RalfJung opened this issue Aug 17, 2024 · 1 comment · Fixed by #129856
Labels
A-concurrency Area: Concurrency A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-discussion Category: Discussion or questions that doesn't represent real issues. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team

Comments

@RalfJung
Copy link
Member

The current docs for compiler_fence explain that function in terms of reorderings:

compiler_fence does not emit any machine code, but restricts the kinds of memory re-ordering the compiler is allowed to do. Specifically, depending on the given Ordering semantics, the compiler may be disallowed from moving reads or writes from before or after the call to the other side of the call to compiler_fence. Note that it does not prevent the hardware from doing such re-ordering. This is not a problem in a single-threaded, execution context, but when other threads may modify memory at the same time, stronger synchronization primitives such as fence are required.

That's somewhat misleading. For good reasons, our fence docs quickly transition from "reordering" to "synchronization" terminology:

Depending on the specified order, a fence prevents the compiler and CPU from reordering certain types of memory operations around it. That creates synchronizes-with relationships between it and atomic operations or fences in other threads.

compiler_fence should also be explained in terms of synchronizes-with, to avoid raising any false expectations. Specifically, compiler_fence is equivalent to C++ atomic_signal_fence, and so its synchronization rules are very similar to those of fence except that synchronization only occurs between a thread and its signal handler, not across different threads.

Cc @rust-lang/libs-api @rust-lang/opsem
Cc rust-lang/unsafe-code-guidelines#347 #54962

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 17, 2024
@jieyouxu jieyouxu added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team labels Aug 17, 2024
@RalfJung RalfJung removed the T-opsem Relevant to the opsem team label Aug 17, 2024
@jieyouxu jieyouxu added A-concurrency Area: Concurrency T-opsem Relevant to the opsem team labels Aug 17, 2024
@jieyouxu
Copy link
Member

(Sorry, race condition...)

@Noratrieb Noratrieb removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 17, 2024
@jieyouxu jieyouxu added the C-discussion Category: Discussion or questions that doesn't represent real issues. label Aug 17, 2024
@bors bors closed this as completed in f0072bf Sep 3, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: Concurrency A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-discussion Category: Discussion or questions that doesn't represent real issues. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants