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

Loose Send/Sync bounds for WorkerLocal used in rayon fork for parallel-compiler #81425

Closed
ammaraskar opened this issue Jan 27, 2021 · 1 comment · Fixed by #82999
Closed

Loose Send/Sync bounds for WorkerLocal used in rayon fork for parallel-compiler #81425

ammaraskar opened this issue Jan 27, 2021 · 1 comment · Fixed by #82999
Labels
C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-parallel Working group: Parallelizing the compiler

Comments

@ammaraskar
Copy link

ammaraskar commented Jan 27, 2021

The WorkerLocal type used in the parallel-compiler cfg:

pub use rayon_core::WorkerLocal;

which comes from the Rust fork of rayon, rustc-rayon, implements Send and Sync unconditionally for all types T:

https://github.com/rust-lang/rustc-rayon/blob/ae7bbbd2756a324c493aef5f5d52473101b2f491/rayon-core/src/worker_local.rs#L10-L19

/// Holds worker-locals values for each thread in a thread pool.
/// You can only access the worker local value through the Deref impl
/// on the thread pool it was constructed on. It will panic otherwise
pub struct WorkerLocal<T> {
    locals: Vec<CacheAligned<T>>,
    registry: Arc<Registry>,
}

unsafe impl<T> Send for WorkerLocal<T> {}
unsafe impl<T> Sync for WorkerLocal<T> {}

While all the current inhabitants of the type T in rustc are sound, as in they are safe to send across threads, this allows for the potential of data races if someone accidentally uses a non-Send type such as Rc in a WorkerLocal.

As per @cuviper in #t-compiler/wg-parallel-rustc on Zulip, this should be bound by T: Send. https://rust-lang.zulipchat.com/#narrow/stream/187679-t-compiler.2Fwg-parallel-rustc/topic/WorkerLocal.20type.20in.20rustc.20rayon.20fork/near/224118580

@ammaraskar
Copy link
Author

Opened rust-lang/rustc-rayon#8 to fix this.

@ammaraskar ammaraskar changed the title Incorrect bounds for WorkerLocal used in rayon fork for parallel-compiler Loose Send/Sync bounds for WorkerLocal used in rayon fork for parallel-compiler Jan 27, 2021
@camelid camelid added WG-compiler-parallel Working group: Parallelizing the compiler C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 29, 2021
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 15, 2021
…ulacrum

Update to rustc-rayon 0.3.1

This pulls in rust-lang/rustc-rayon#8 to fix rust-lang#81425. (h/t `@ammaraskar)`

That revealed weak constraints on `rustc_arena::DropArena`, because its
`DropType` was holding type-erased raw pointers to generic `T`. We can
implement `Send` for `DropType` (under `cfg(parallel_compiler)`) by
requiring all `T: Send` before they're type-erased.
@bors bors closed this as completed in f7e75a2 Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-parallel Working group: Parallelizing the compiler
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants