diff --git a/compiler/rustc_data_structures/src/sync/parallel.rs b/compiler/rustc_data_structures/src/sync/parallel.rs index f8ddde4298c40..1442baa337b37 100644 --- a/compiler/rustc_data_structures/src/sync/parallel.rs +++ b/compiler/rustc_data_structures/src/sync/parallel.rs @@ -185,12 +185,18 @@ pub fn par_for_each_in>( }); } -// FIXME: actually make parallel and `T: DynSend` -pub fn par_for_each_slice(items: &mut [T], for_each: impl Fn(&mut T)) { +pub fn par_for_each_slice( + items: &mut [T], + for_each: impl Fn(&mut T) + DynSync + DynSend, +) { parallel_guard(|guard| { - items.iter_mut().for_each(|i| { - guard.run(|| for_each(i)); - }); + if let Some(proof) = mode::check_dyn_thread_safe() { + par_slice(items, guard, |i| for_each(i), proof) + } else { + items.iter_mut().for_each(|i| { + guard.run(|| for_each(i)); + }); + } }); } diff --git a/compiler/rustc_resolve/src/build_reduced_graph.rs b/compiler/rustc_resolve/src/build_reduced_graph.rs index e92cd82b60e55..6d5f7231f5269 100644 --- a/compiler/rustc_resolve/src/build_reduced_graph.rs +++ b/compiler/rustc_resolve/src/build_reduced_graph.rs @@ -5,7 +5,6 @@ //! unexpanded macros in the fragment are visited and registered. //! Imports are also considered items and placed into modules here, but not resolved yet. -use std::cell::RefMut; use std::sync::Arc; use rustc_ast::visit::{self, AssocCtxt, Visitor, WalkItemKind}; @@ -16,6 +15,7 @@ use rustc_ast::{ }; use rustc_attr_parsing::AttributeParser; use rustc_data_structures::fx::FxIndexMap; +use rustc_data_structures::sync::WriteGuard; use rustc_expand::base::{ResolverExpand, SyntaxExtension, SyntaxExtensionKind}; use rustc_hir::Attribute; use rustc_hir::attrs::{AttributeKind, MacroUseArgs}; @@ -135,7 +135,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { fn get_extern_module_with_lock( &self, def_id: DefId, - map_lock: &mut RefMut<'_, FxIndexMap>>, + map_lock: &mut WriteGuard<'_, FxIndexMap>>, ) -> Option> { if let module @ Some(..) = map_lock.get(&def_id) { return module.copied(); diff --git a/compiler/rustc_resolve/src/diagnostics/impls.rs b/compiler/rustc_resolve/src/diagnostics/impls.rs index ce41dd86a48be..65da410305bd8 100644 --- a/compiler/rustc_resolve/src/diagnostics/impls.rs +++ b/compiler/rustc_resolve/src/diagnostics/impls.rs @@ -1487,7 +1487,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { // Never recommend deprecated helper attributes. } Scope::MacroRules(macro_rules_scope) => { - if let MacroRulesScope::Def(macro_rules_def) = macro_rules_scope.get() { + if let MacroRulesScope::Def(macro_rules_def) = *macro_rules_scope.read() { let res = macro_rules_def.decl.res(); if filter_fn(res) { suggestions.push(TypoSuggestion::new( diff --git a/compiler/rustc_resolve/src/ident.rs b/compiler/rustc_resolve/src/ident.rs index d58ec6b5032b8..d4248a2d22bbe 100644 --- a/compiler/rustc_resolve/src/ident.rs +++ b/compiler/rustc_resolve/src/ident.rs @@ -142,11 +142,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { // used to avoid long scope chains, see the comments on `MacroRulesScopeRef`. // As another consequence of this optimization visitors never observe invocation // scopes for macros that were already expanded. - let mut scope = macro_rules_scope.get(); + let mut scope = *macro_rules_scope.borrow(); while let MacroRulesScope::Invocation(invoc_id) = scope { if let Some(next) = self.output_macro_rules_scopes.get(&invoc_id) { - scope = next.get(); - macro_rules_scope.set(scope); + scope = *next.borrow(); + *macro_rules_scope.borrow_mut() = scope; } else { break; } @@ -187,7 +187,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { } } Scope::DeriveHelpersCompat => Scope::MacroRules(parent_scope.macro_rules), - Scope::MacroRules(macro_rules_scope) => match macro_rules_scope.get() { + Scope::MacroRules(macro_rules_scope) => match *macro_rules_scope.read() { MacroRulesScope::Def(binding) => { Scope::MacroRules(binding.parent_macro_rules_scope) } @@ -592,7 +592,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { } result } - Scope::MacroRules(macro_rules_scope) => match macro_rules_scope.get() { + Scope::MacroRules(macro_rules_scope) => match *macro_rules_scope.read() { MacroRulesScope::Def(macro_rules_def) if ident == macro_rules_def.ident => { Ok(macro_rules_def.decl) } diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index 2b1c0bf80a694..c098241f29d9d 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/compiler/rustc_resolve/src/lib.rs @@ -24,7 +24,7 @@ use std::cell::RefMut; use std::collections::BTreeSet; use std::ops::ControlFlow; -use std::sync::{Arc, OnceLock}; +use std::sync::{Arc, Mutex, OnceLock}; use std::{fmt, mem}; use diagnostics::{ParamKindInEnumDiscriminant, ParamKindInNonTrivialAnonConst}; @@ -46,7 +46,7 @@ use rustc_ast::{ use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexMap, FxIndexSet, default}; use rustc_data_structures::intern::Interned; use rustc_data_structures::steal::Steal; -use rustc_data_structures::sync::{FreezeReadGuard, FreezeWriteGuard, WorkerLocal}; +use rustc_data_structures::sync::{FreezeReadGuard, FreezeWriteGuard, RwLock, WorkerLocal}; use rustc_data_structures::unord::{UnordItems, UnordMap, UnordSet}; use rustc_errors::{Applicability, Diag, ErrCode, ErrorGuaranteed, LintBuffer}; use rustc_expand::base::{DeriveResolution, SyntaxExtension, SyntaxExtensionKind}; @@ -1274,7 +1274,7 @@ struct ExternPreludeEntry<'ra> { item_decl: Option<(Decl<'ra>, Span, /* introduced by item */ bool)>, /// Name declaration from an `--extern` flag, lazily populated on first use. flag_decl: Option< - CacheCell<( + Mutex<( PendingDecl<'ra>, /* finalized */ bool, /* open flag (namespaced crate) */ bool, @@ -1290,14 +1290,14 @@ impl ExternPreludeEntry<'_> { fn flag() -> Self { ExternPreludeEntry { item_decl: None, - flag_decl: Some(CacheCell::new((PendingDecl::Pending, false, false))), + flag_decl: Some(Mutex::new((PendingDecl::Pending, false, false))), } } fn open_flag() -> Self { ExternPreludeEntry { item_decl: None, - flag_decl: Some(CacheCell::new((PendingDecl::Pending, false, true))), + flag_decl: Some(Mutex::new((PendingDecl::Pending, false, true))), } } @@ -1397,7 +1397,7 @@ pub struct Resolver<'ra, 'tcx> { /// Eagerly populated map of all local non-block modules. local_module_map: FxIndexMap>, /// Lazily populated cache of modules loaded from external crates. - extern_module_map: CacheRefCell>>, + extern_module_map: RwLock>>, /// Maps glob imports to the names of items actually imported. glob_map: FxIndexMap>, @@ -1429,7 +1429,7 @@ pub struct Resolver<'ra, 'tcx> { /// Eagerly populated map of all local macro definitions. local_macro_map: FxHashMap> = default::fx_hash_map(), /// Lazily populated cache of macro definitions loaded from external crates. - extern_macro_map: CacheRefCell>>, + extern_macro_map: RwLock>>, dummy_ext_bang: &'ra Arc, dummy_ext_derive: &'ra Arc, non_macro_attr: &'ra Arc, @@ -1602,7 +1602,7 @@ impl<'ra> ResolverArenas<'ra> { Interned::new_unchecked(self.name_resolutions.alloc(CmRefCell::new(resolution))) } fn alloc_macro_rules_scope(&'ra self, scope: MacroRulesScope<'ra>) -> MacroRulesScopeRef<'ra> { - self.dropless.alloc(CacheCell::new(scope)) + self.dropless.alloc(RwLock::new(scope)) } fn alloc_macro_rules_decl(&'ra self, decl: MacroRulesDecl<'ra>) -> &'ra MacroRulesDecl<'ra> { self.dropless.alloc(decl) @@ -2456,7 +2456,8 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { ) -> Option> { let entry = self.extern_prelude.get(&ident); entry.and_then(|entry| entry.flag_decl.as_ref()).and_then(|flag_decl| { - let (pending_decl, finalized, is_open) = flag_decl.get(); + let mut flag_decl = flag_decl.lock().unwrap(); // Lock for this entire process + let (pending_decl, finalized, is_open) = *flag_decl; let decl = match pending_decl { PendingDecl::Ready(decl) => { if finalize && !finalized && !is_open { @@ -2491,7 +2492,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { } } }; - flag_decl.set((PendingDecl::Ready(decl), finalize || finalized, is_open)); + *flag_decl = (PendingDecl::Ready(decl), finalize || finalized, is_open); decl.or_else(|| finalize.then_some(self.dummy_decl)) }) } @@ -2831,16 +2832,31 @@ pub fn provide(providers: &mut Providers) { /// Prefer constructing it through `Resolver::cm(_mut)` to ensure correctness. type CmResolver<'r, 'ra, 'tcx> = ref_mut::RefOrMut<'r, Resolver<'ra, 'tcx>>; -// FIXME: These are cells for caches that can be populated even during speculative resolution, -// and should be replaced with mutexes, atomics, or other synchronized data when migrating to -// parallel name resolution. -use std::cell::{Cell as CacheCell, RefCell as CacheRefCell}; - +/// The [`Resolver`] has different resolution phases, which share most of the resolution logic. +/// Some of these phases mutate the resolver (`&mut Resolver`) during resolution. However, we are +/// transforming import resolution (one of these phases), often referred to as +/// "speculative resolution", to a parallel algorithm, which requires 2 things to change: +/// - A `&Resolver`, because cannot mutate any fields in it. +/// - Some fields with interior mutability may not be changed during import resolution, as +/// this can conflict with other in progress resolutions (so locks are not the solution). +/// But these fields do require interior mutability during the other phases. +/// +/// Because refactoring the entire name resolution code to be split into "immutable" and "mutable" +/// logic, we opted for a more "developer friendly" and "unsafe" approach using data structures +/// that are immutable during import resolution. This module is the collection of 3 data structures +/// that offer use the above 2 points: +/// - a `RefOrMut<'a, T>` smart pointer that gates a `&'a mut T` through a flag +/// (i.e. are we in import res?). +/// - `CmCell` and `CmRefCell`, which are conditionally mutable through a flag. +/// +/// We hope one day to not have to do this :D mod ref_mut { use std::cell::{BorrowMutError, Cell, Ref, RefCell, RefMut}; use std::fmt; use std::ops::Deref; + use rustc_data_structures::sync::DynSync; + use crate::Resolver; /// A reference type that conditionally allows mutable access. @@ -2893,6 +2909,12 @@ mod ref_mut { #[derive(Default)] pub(crate) struct CmCell(Cell); + // SAFETY: `CmCell` is `Sync` only because every path that can call `Cell::set` + // (i.e. `CmCell::set`, and `update`, which is built on top of it) first checks + // `r.assert_speculative` and panics if it's set, refusing to mutate. Soundness + // therefore depends on proper usage of the `assert_speculative` field in the `Resolver`. + unsafe impl DynSync for CmCell {} + impl fmt::Debug for CmCell { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_tuple("CmCell").field(&self.get()).finish() @@ -2981,10 +3003,20 @@ mod ref_mut { } } - /// A wrapper around a [`RefCell`] that only allows writes (mutable borrows) based on a condition in the resolver. #[derive(Default)] pub(crate) struct CmRefCell(RefCell); + // SAFETY: This is safe because we can only mutate the inner state (borrow counter and `T`) + // if we are not in speculative resolution, which is run in parallel. This is checked + // dynamically with the `resolver.speculative_flag` field: + // + // - Any form of `borrow_mut` causes an immediate panic if that flag is set to ture. + // - We can only ever update the read counter in `borrow` if we have a `&mut Resolver`, + // thus proving no speculative resolution exists. If we do need a shared borrow, the + // `borrow_checked` function can be used, which gives out a `CmRef`. (see the safety comment + // in `borrow_checked` for why this works) + unsafe impl DynSync for CmRefCell {} + impl CmRefCell { pub(crate) fn new(value: T) -> CmRefCell { CmRefCell(RefCell::new(value)) diff --git a/compiler/rustc_resolve/src/macros.rs b/compiler/rustc_resolve/src/macros.rs index 9d5a59eb405c4..98b22e226f3e9 100644 --- a/compiler/rustc_resolve/src/macros.rs +++ b/compiler/rustc_resolve/src/macros.rs @@ -7,6 +7,7 @@ use std::sync::Arc; use rustc_ast::{self as ast, Crate, DelegationSuffixes, NodeId}; use rustc_ast_pretty::pprust; use rustc_attr_parsing::AttributeParser; +use rustc_data_structures::sync::RwLock; use rustc_errors::{Applicability, StashKey}; use rustc_expand::base::{ Annotatable, DeriveResolution, Indeterminate, ResolverExpand, SyntaxExtension, @@ -41,7 +42,7 @@ use crate::diagnostics::{ use crate::hygiene::Macros20NormalizedSyntaxContext; use crate::imports::Import; use crate::{ - BindingKey, CacheCell, CmResolver, Decl, DeclKind, DeriveData, Determinacy, Finalize, IdentKey, + BindingKey, CmResolver, Decl, DeclKind, DeriveData, Determinacy, Finalize, IdentKey, InvocationParent, ModuleKind, ModuleOrUniformRoot, ParentScope, PathResult, Res, ResolutionError, Resolver, ScopeSet, Segment, Used, }; @@ -79,7 +80,7 @@ pub(crate) enum MacroRulesScope<'ra> { /// This helps to avoid uncontrollable growth of `macro_rules!` scope chains, /// which usually grow linearly with the number of macro invocations /// in a module (including derives) and hurt performance. -pub(crate) type MacroRulesScopeRef<'ra> = &'ra CacheCell>; +pub(crate) type MacroRulesScopeRef<'ra> = &'ra RwLock>; /// Macro namespace is separated into two sub-namespaces, one for bang macros and /// one for attribute-like macros (attributes, derives).