Auto merge of #48936 - Zoxc:cstore, r=michaelwoerister

Make CrateMetadata and CStore thread-safe r? @michaelwoerister
rust-lang · Mar 17, 2018 · adf2135 · adf2135
2 parents 61b6bf5 + 5b8f9c5
commit adf2135
Show file tree

Hide file tree

Showing 7 changed files with 68 additions and 43 deletions.
diff --git a/src/librustc_metadata/creader.rs b/src/librustc_metadata/creader.rs
@@ -14,7 +14,7 @@ use cstore::{self, CStore, CrateSource, MetadataBlob};
 use locator::{self, CratePaths};
 use native_libs::relevant_lib;
 use schema::CrateRoot;
-use rustc_data_structures::sync::Lrc;
+use rustc_data_structures::sync::{Lrc, RwLock, Lock};
 
 use rustc::hir::def_id::{CrateNum, CRATE_DEF_INDEX};
 use rustc::hir::svh::Svh;
@@ -30,7 +30,6 @@ use rustc::util::common::record_time;
 use rustc::util::nodemap::FxHashSet;
 use rustc::hir::map::Definitions;
 
-use std::cell::{RefCell, Cell};
 use std::ops::Deref;
 use std::path::PathBuf;
 use std::{cmp, fs};
@@ -63,7 +62,7 @@ fn dump_crates(cstore: &CStore) {
         info!("  name: {}", data.name());
         info!("  cnum: {}", data.cnum);
         info!("  hash: {}", data.hash());
-        info!("  reqd: {:?}", data.dep_kind.get());
+        info!("  reqd: {:?}", *data.dep_kind.lock());
         let CrateSource { dylib, rlib, rmeta } = data.source.clone();
         dylib.map(|dl| info!("  dylib: {}", dl.0.display()));
         rlib.map(|rl|  info!("   rlib: {}", rl.0.display()));
@@ -233,19 +232,19 @@ impl<'a> CrateLoader<'a> {
 
         let mut cmeta = cstore::CrateMetadata {
             name,
-            extern_crate: Cell::new(None),
+            extern_crate: Lock::new(None),
             def_path_table: Lrc::new(def_path_table),
             trait_impls,
             proc_macros: crate_root.macro_derive_registrar.map(|_| {
                 self.load_derive_macros(&crate_root, dylib.clone().map(|p| p.0), span)
             }),
             root: crate_root,
             blob: metadata,
-            cnum_map: RefCell::new(cnum_map),
+            cnum_map: Lock::new(cnum_map),
             cnum,
-            codemap_import_info: RefCell::new(vec![]),
-            attribute_cache: RefCell::new([Vec::new(), Vec::new()]),
-            dep_kind: Cell::new(dep_kind),
+            codemap_import_info: RwLock::new(vec![]),
+            attribute_cache: Lock::new([Vec::new(), Vec::new()]),
+            dep_kind: Lock::new(dep_kind),
             source: cstore::CrateSource {
                 dylib,
                 rlib,
@@ -335,7 +334,9 @@ impl<'a> CrateLoader<'a> {
                 if data.root.macro_derive_registrar.is_some() {
                     dep_kind = DepKind::UnexportedMacrosOnly;
                 }
-                data.dep_kind.set(cmp::max(data.dep_kind.get(), dep_kind));
+                data.dep_kind.with_lock(|data_dep_kind| {
+                    *data_dep_kind = cmp::max(*data_dep_kind, dep_kind);
+                });
                 (cnum, data)
             }
             LoadResult::Loaded(library) => {
@@ -379,14 +380,14 @@ impl<'a> CrateLoader<'a> {
         if !visited.insert((cnum, extern_crate.direct)) { return }
 
         let cmeta = self.cstore.get_crate_data(cnum);
-        let old_extern_crate = cmeta.extern_crate.get();
+        let mut old_extern_crate = cmeta.extern_crate.borrow_mut();
 
         // Prefer:
         // - something over nothing (tuple.0);
         // - direct extern crate to indirect (tuple.1);
         // - shorter paths to longer (tuple.2).
         let new_rank = (true, extern_crate.direct, !extern_crate.path_len);
-        let old_rank = match old_extern_crate {
+        let old_rank = match *old_extern_crate {
             None => (false, false, !0),
             Some(ref c) => (true, c.direct, !c.path_len),
         };
@@ -395,7 +396,9 @@ impl<'a> CrateLoader<'a> {
             return; // no change needed
         }
 
-        cmeta.extern_crate.set(Some(extern_crate));
+        *old_extern_crate = Some(extern_crate);
+        drop(old_extern_crate);
+
         // Propagate the extern crate info to dependencies.
         extern_crate.direct = false;
         for &dep_cnum in cmeta.cnum_map.borrow().iter() {
@@ -646,7 +649,7 @@ impl<'a> CrateLoader<'a> {
                 // #![panic_runtime] crate.
                 self.inject_dependency_if(cnum, "a panic runtime",
                                           &|data| data.needs_panic_runtime(sess));
-                runtime_found = runtime_found || data.dep_kind.get() == DepKind::Explicit;
+                runtime_found = runtime_found || *data.dep_kind.lock() == DepKind::Explicit;
             }
         });
 

diff --git a/src/librustc_metadata/cstore.rs b/src/librustc_metadata/cstore.rs
@@ -22,8 +22,7 @@ use rustc_back::PanicStrategy;
 use rustc_data_structures::indexed_vec::IndexVec;
 use rustc::util::nodemap::{FxHashMap, FxHashSet, NodeMap};
 
-use std::cell::{RefCell, Cell};
-use rustc_data_structures::sync::Lrc;
+use rustc_data_structures::sync::{Lrc, RwLock, Lock};
 use syntax::{ast, attr};
 use syntax::ext::base::SyntaxExtension;
 use syntax::symbol::Symbol;
@@ -62,13 +61,13 @@ pub struct CrateMetadata {
     /// Information about the extern crate that caused this crate to
     /// be loaded. If this is `None`, then the crate was injected
     /// (e.g., by the allocator)
-    pub extern_crate: Cell<Option<ExternCrate>>,
+    pub extern_crate: Lock<Option<ExternCrate>>,
 
     pub blob: MetadataBlob,
-    pub cnum_map: RefCell<CrateNumMap>,
+    pub cnum_map: Lock<CrateNumMap>,
     pub cnum: CrateNum,
-    pub codemap_import_info: RefCell<Vec<ImportedFileMap>>,
-    pub attribute_cache: RefCell<[Vec<Option<Lrc<[ast::Attribute]>>>; 2]>,
+    pub codemap_import_info: RwLock<Vec<ImportedFileMap>>,
+    pub attribute_cache: Lock<[Vec<Option<Lrc<[ast::Attribute]>>>; 2]>,
 
     pub root: schema::CrateRoot,
 
@@ -81,7 +80,7 @@ pub struct CrateMetadata {
 
     pub trait_impls: FxHashMap<(u32, DefIndex), schema::LazySeq<DefIndex>>,
 
-    pub dep_kind: Cell<DepKind>,
+    pub dep_kind: Lock<DepKind>,
     pub source: CrateSource,
 
     pub proc_macros: Option<Vec<(ast::Name, Lrc<SyntaxExtension>)>>,
@@ -90,21 +89,23 @@ pub struct CrateMetadata {
 }
 
 pub struct CStore {
-    metas: RefCell<IndexVec<CrateNum, Option<Lrc<CrateMetadata>>>>,
+    metas: RwLock<IndexVec<CrateNum, Option<Lrc<CrateMetadata>>>>,
     /// Map from NodeId's of local extern crate statements to crate numbers
-    extern_mod_crate_map: RefCell<NodeMap<CrateNum>>,
-    pub metadata_loader: Box<MetadataLoader>,
+    extern_mod_crate_map: Lock<NodeMap<CrateNum>>,
+    pub metadata_loader: Box<MetadataLoader + Sync>,
 }
 
 impl CStore {
-    pub fn new(metadata_loader: Box<MetadataLoader>) -> CStore {
+    pub fn new(metadata_loader: Box<MetadataLoader + Sync>) -> CStore {
         CStore {
-            metas: RefCell::new(IndexVec::new()),
-            extern_mod_crate_map: RefCell::new(FxHashMap()),
+            metas: RwLock::new(IndexVec::new()),
+            extern_mod_crate_map: Lock::new(FxHashMap()),
             metadata_loader,
         }
     }
 
+    /// You cannot use this function to allocate a CrateNum in a thread-safe manner.
+    /// It is currently only used in CrateLoader which is single-threaded code.
     pub fn next_crate_num(&self) -> CrateNum {
         CrateNum::new(self.metas.borrow().len() + 1)
     }

diff --git a/src/librustc_metadata/cstore_impl.rs b/src/librustc_metadata/cstore_impl.rs
@@ -175,7 +175,10 @@ provide! { <'tcx> tcx, def_id, other, cdata,
     is_sanitizer_runtime => { cdata.is_sanitizer_runtime(tcx.sess) }
     is_profiler_runtime => { cdata.is_profiler_runtime(tcx.sess) }
     panic_strategy => { cdata.panic_strategy() }
-    extern_crate => { Lrc::new(cdata.extern_crate.get()) }
+    extern_crate => {
+        let r = Lrc::new(*cdata.extern_crate.lock());
+        r
+    }
     is_no_builtins => { cdata.is_no_builtins(tcx.sess) }
     impl_defaultness => { cdata.get_impl_defaultness(def_id.index) }
     reachable_non_generics => {
@@ -225,7 +228,10 @@ provide! { <'tcx> tcx, def_id, other, cdata,
         cdata.is_dllimport_foreign_item(def_id.index)
     }
     visibility => { cdata.get_visibility(def_id.index) }
-    dep_kind => { cdata.dep_kind.get() }
+    dep_kind => {
+        let r = *cdata.dep_kind.lock();
+        r
+    }
     crate_name => { cdata.name }
     item_children => {
         let mut result = vec![];
@@ -241,10 +247,11 @@ provide! { <'tcx> tcx, def_id, other, cdata,
     }
 
     missing_extern_crate_item => {
-        match cdata.extern_crate.get() {
+        let r = match *cdata.extern_crate.borrow() {
             Some(extern_crate) if !extern_crate.direct => true,
             _ => false,
-        }
+        };
+        r
     }
 
     used_crate_source => { Lrc::new(cdata.source.clone()) }
@@ -419,13 +426,16 @@ impl CrateStore for cstore::CStore {
 
     fn dep_kind_untracked(&self, cnum: CrateNum) -> DepKind
     {
-        self.get_crate_data(cnum).dep_kind.get()
+        let data = self.get_crate_data(cnum);
+        let r = *data.dep_kind.lock();
+        r
     }
 
     fn export_macros_untracked(&self, cnum: CrateNum) {
         let data = self.get_crate_data(cnum);
-        if data.dep_kind.get() == DepKind::UnexportedMacrosOnly {
-            data.dep_kind.set(DepKind::MacrosOnly)
+        let mut dep_kind = data.dep_kind.lock();
+        if *dep_kind == DepKind::UnexportedMacrosOnly {
+            *dep_kind = DepKind::MacrosOnly;
         }
     }
 

diff --git a/src/librustc_metadata/decoder.rs b/src/librustc_metadata/decoder.rs
@@ -13,7 +13,7 @@
 use cstore::{self, CrateMetadata, MetadataBlob, NativeLibrary};
 use schema::*;
 
-use rustc_data_structures::sync::Lrc;
+use rustc_data_structures::sync::{Lrc, ReadGuard};
 use rustc::hir::map::{DefKey, DefPath, DefPathData, DefPathHash};
 use rustc::hir;
 use rustc::middle::cstore::{LinkagePreference, ExternConstBody,
@@ -31,7 +31,6 @@ use rustc::ty::codec::TyDecoder;
 use rustc::mir::Mir;
 use rustc::util::nodemap::FxHashMap;
 
-use std::cell::Ref;
 use std::collections::BTreeMap;
 use std::io;
 use std::mem;
@@ -714,7 +713,7 @@ impl<'a, 'tcx> CrateMetadata {
         };
 
         // Iterate over all children.
-        let macros_only = self.dep_kind.get().macros_only();
+        let macros_only = self.dep_kind.lock().macros_only();
         for child_index in item.children.decode((self, sess)) {
             if macros_only {
                 continue
@@ -950,6 +949,8 @@ impl<'a, 'tcx> CrateMetadata {
         if vec_.len() < node_index + 1 {
             vec_.resize(node_index + 1, None);
         }
+        // This can overwrite the result produced by another thread, but the value
+        // written should be the same
         vec_[node_index] = Some(result.clone());
         result
     }
@@ -1156,14 +1157,22 @@ impl<'a, 'tcx> CrateMetadata {
     /// for items inlined from other crates.
     pub fn imported_filemaps(&'a self,
                              local_codemap: &codemap::CodeMap)
-                             -> Ref<'a, Vec<cstore::ImportedFileMap>> {
+                             -> ReadGuard<'a, Vec<cstore::ImportedFileMap>> {
         {
             let filemaps = self.codemap_import_info.borrow();
             if !filemaps.is_empty() {
                 return filemaps;
             }
         }
 
+        // Lock the codemap_import_info to ensure this only happens once
+        let mut codemap_import_info = self.codemap_import_info.borrow_mut();
+
+        if !codemap_import_info.is_empty() {
+            drop(codemap_import_info);
+            return self.codemap_import_info.borrow();
+        }
+
         let external_codemap = self.root.codemap.decode(self);
 
         let imported_filemaps = external_codemap.map(|filemap_to_import| {
@@ -1222,8 +1231,10 @@ impl<'a, 'tcx> CrateMetadata {
             }
         }).collect();
 
+        *codemap_import_info = imported_filemaps;
+        drop(codemap_import_info);
+
         // This shouldn't borrow twice, but there is no way to downgrade RefMut to Ref.
-        *self.codemap_import_info.borrow_mut() = imported_filemaps;
         self.codemap_import_info.borrow()
     }
 }
diff --git a/src/librustc_trans/lib.rs b/src/librustc_trans/lib.rs
@@ -201,7 +201,7 @@ impl TransCrate for LlvmTransCrate {
         target_features(sess)
     }
 
-    fn metadata_loader(&self) -> Box<MetadataLoader> {
+    fn metadata_loader(&self) -> Box<MetadataLoader + Sync> {
         box metadata::LlvmMetadataLoader
     }
 

diff --git a/src/librustc_trans_utils/trans_crate.rs b/src/librustc_trans_utils/trans_crate.rs
@@ -58,7 +58,7 @@ pub trait TransCrate {
     fn print_version(&self) {}
     fn diagnostics(&self) -> &[(&'static str, &'static str)] { &[] }
 
-    fn metadata_loader(&self) -> Box<MetadataLoader>;
+    fn metadata_loader(&self) -> Box<MetadataLoader + Sync>;
     fn provide(&self, _providers: &mut Providers);
     fn provide_extern(&self, _providers: &mut Providers);
     fn trans_crate<'a, 'tcx>(
@@ -84,7 +84,7 @@ pub trait TransCrate {
 pub struct DummyTransCrate;
 
 impl TransCrate for DummyTransCrate {
-    fn metadata_loader(&self) -> Box<MetadataLoader> {
+    fn metadata_loader(&self) -> Box<MetadataLoader + Sync> {
         box DummyMetadataLoader(())
     }
 
@@ -195,7 +195,7 @@ impl TransCrate for MetadataOnlyTransCrate {
         }
     }
 
-    fn metadata_loader(&self) -> Box<MetadataLoader> {
+    fn metadata_loader(&self) -> Box<MetadataLoader + Sync> {
         box NoLlvmMetadataLoader
     }
 

diff --git a/src/test/run-make/hotplug_codegen_backend/the_backend.rs b/src/test/run-make/hotplug_codegen_backend/the_backend.rs
@@ -28,7 +28,7 @@ use rustc_trans_utils::trans_crate::{TransCrate, MetadataOnlyTransCrate};
 struct TheBackend(Box<TransCrate>);
 
 impl TransCrate for TheBackend {
-    fn metadata_loader(&self) -> Box<MetadataLoader> {
+    fn metadata_loader(&self) -> Box<MetadataLoader + Sync> {
         self.0.metadata_loader()
     }