Skip to content

Work towards thread safety in rustc #46779

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

Merged
merged 8 commits into from
Dec 22, 2017
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/librustc/ich/caching_codemap_view.rs
Original file line number Diff line number Diff line change
@@ -78,11 +78,9 @@ impl<'cm> CachingCodemapView<'cm> {
// If the entry doesn't point to the correct file, fix it up
if pos < cache_entry.file.start_pos || pos >= cache_entry.file.end_pos {
let file_valid;
let files = self.codemap.files();

if files.len() > 0 {
if self.codemap.files().len() > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need a better locking discipline for the codemap? It feels like "too many locks, not enough discipline". Maybe just add a fixme to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that I just convert all RefCells into locks, there's probably plenty of places where there are too many locks.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

let file_index = self.codemap.lookup_filemap_idx(pos);
let file = files[file_index].clone();
let file = self.codemap.files()[file_index].clone();

if pos >= file.start_pos && pos < file.end_pos {
cache_entry.file = file;
6 changes: 6 additions & 0 deletions src/librustc/session/config.rs
Original file line number Diff line number Diff line change
@@ -1067,6 +1067,8 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
"prints the llvm optimization passes being run"),
ast_json: bool = (false, parse_bool, [UNTRACKED],
"print the AST as JSON and halt"),
query_threads: Option<usize> = (None, parse_opt_uint, [UNTRACKED],
"execute queries on a thread pool with N threads"),
ast_json_noexpand: bool = (false, parse_bool, [UNTRACKED],
"print the pre-expansion AST as JSON and halt"),
ls: bool = (false, parse_bool, [UNTRACKED],
@@ -1663,6 +1665,10 @@ pub fn build_session_options_and_crate_config(matches: &getopts::Matches)
}
}

if debugging_opts.query_threads == Some(0) {
early_error(error_format, "Value for query threads must be a positive nonzero integer");
}

if codegen_units == Some(0) {
early_error(error_format, "Value for codegen units must be a positive nonzero integer");
}
6 changes: 6 additions & 0 deletions src/librustc/session/mod.rs
Original file line number Diff line number Diff line change
@@ -725,6 +725,12 @@ impl Session {
ret
}

/// Returns the number of query threads that should be used for this
/// compilation
pub fn query_threads(&self) -> usize {
self.opts.debugging_opts.query_threads.unwrap_or(1)
}

/// Returns the number of codegen units that should be used for this
/// compilation
pub fn codegen_units(&self) -> usize {
21 changes: 17 additions & 4 deletions src/librustc/ty/context.rs
Original file line number Diff line number Diff line change
@@ -76,6 +76,20 @@ use syntax_pos::Span;

use hir;

pub struct AllArenas<'tcx> {
pub global: GlobalArenas<'tcx>,
pub interner: DroplessArena,
}

impl<'tcx> AllArenas<'tcx> {
pub fn new() -> Self {
AllArenas {
global: GlobalArenas::new(),
interner: DroplessArena::new(),
}
}
}

/// Internal storage
pub struct GlobalArenas<'tcx> {
// internings
@@ -1120,8 +1134,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
cstore: &'tcx CrateStore,
local_providers: ty::maps::Providers<'tcx>,
extern_providers: ty::maps::Providers<'tcx>,
arenas: &'tcx GlobalArenas<'tcx>,
arena: &'tcx DroplessArena,
arenas: &'tcx AllArenas<'tcx>,
resolutions: ty::Resolutions,
hir: hir_map::Map<'tcx>,
on_disk_query_result_cache: maps::OnDiskCache<'tcx>,
@@ -1132,7 +1145,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
where F: for<'b> FnOnce(TyCtxt<'b, 'tcx, 'tcx>) -> R
{
let data_layout = TargetDataLayout::parse(s);
let interners = CtxtInterners::new(arena);
let interners = CtxtInterners::new(&arenas.interner);
let common_types = CommonTypes::new(&interners);
let dep_graph = hir.dep_graph.clone();
let max_cnum = cstore.crates_untracked().iter().map(|c| c.as_usize()).max().unwrap_or(0);
@@ -1184,7 +1197,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
tls::enter_global(GlobalCtxt {
sess: s,
cstore,
global_arenas: arenas,
global_arenas: &arenas.global,
global_interners: interners,
dep_graph: dep_graph.clone(),
on_disk_query_result_cache,
2 changes: 1 addition & 1 deletion src/librustc/ty/mod.rs
Original file line number Diff line number Diff line change
@@ -77,7 +77,7 @@ pub use self::sty::TypeVariants::*;
pub use self::binding::BindingMode;
pub use self::binding::BindingMode::*;

pub use self::context::{TyCtxt, GlobalArenas, tls, keep_local};
pub use self::context::{TyCtxt, GlobalArenas, AllArenas, tls, keep_local};
pub use self::context::{Lift, TypeckTables};

pub use self::instance::{Instance, InstanceDef};
2 changes: 1 addition & 1 deletion src/librustc_data_structures/indexed_vec.rs
Original file line number Diff line number Diff line change
@@ -327,7 +327,7 @@ macro_rules! newtype_index {
#[derive(Clone, PartialEq, Eq)]
pub struct IndexVec<I: Idx, T> {
pub raw: Vec<T>,
_marker: PhantomData<Fn(&I)>
_marker: PhantomData<fn(&I)>
}

// Whether `IndexVec` is `Send` depends only on the data,
20 changes: 5 additions & 15 deletions src/librustc_driver/driver.rs
Original file line number Diff line number Diff line change
@@ -22,7 +22,7 @@ use rustc::lint;
use rustc::middle::{self, stability, reachable, resolve_lifetime};
use rustc::middle::cstore::CrateStore;
use rustc::middle::privacy::AccessLevels;
use rustc::ty::{self, TyCtxt, Resolutions, GlobalArenas};
use rustc::ty::{self, TyCtxt, Resolutions, AllArenas};
use rustc::traits;
use rustc::util::common::{ErrorReported, time};
use rustc_allocator as allocator;
@@ -62,7 +62,6 @@ use syntax::util::node_count::NodeCounter;
use syntax_pos::FileName;
use syntax;
use syntax_ext;
use arena::DroplessArena;

use derive_registrar;
use pretty::ReplaceBodyWithLoop;
@@ -169,8 +168,7 @@ pub fn compile_input(sess: &Session,
return Ok(())
}

let arena = DroplessArena::new();
let arenas = GlobalArenas::new();
let arenas = AllArenas::new();

// Construct the HIR map
let hir_map = time(sess.time_passes(),
@@ -185,7 +183,6 @@ pub fn compile_input(sess: &Session,
sess,
outdir,
output,
&arena,
&arenas,
&cstore,
&hir_map,
@@ -215,7 +212,6 @@ pub fn compile_input(sess: &Session,
hir_map,
analysis,
resolutions,
&arena,
&arenas,
&crate_name,
&outputs,
@@ -401,8 +397,7 @@ pub struct CompileState<'a, 'tcx: 'a> {
pub output_filenames: Option<&'a OutputFilenames>,
pub out_dir: Option<&'a Path>,
pub out_file: Option<&'a Path>,
pub arena: Option<&'tcx DroplessArena>,
pub arenas: Option<&'tcx GlobalArenas<'tcx>>,
pub arenas: Option<&'tcx AllArenas<'tcx>>,
pub expanded_crate: Option<&'a ast::Crate>,
pub hir_crate: Option<&'a hir::Crate>,
pub hir_map: Option<&'a hir_map::Map<'tcx>>,
@@ -422,7 +417,6 @@ impl<'a, 'tcx> CompileState<'a, 'tcx> {
session,
out_dir: out_dir.as_ref().map(|s| &**s),
out_file: None,
arena: None,
arenas: None,
krate: None,
registry: None,
@@ -477,8 +471,7 @@ impl<'a, 'tcx> CompileState<'a, 'tcx> {
session: &'tcx Session,
out_dir: &'a Option<PathBuf>,
out_file: &'a Option<PathBuf>,
arena: &'tcx DroplessArena,
arenas: &'tcx GlobalArenas<'tcx>,
arenas: &'tcx AllArenas<'tcx>,
cstore: &'tcx CStore,
hir_map: &'a hir_map::Map<'tcx>,
analysis: &'a ty::CrateAnalysis,
@@ -490,7 +483,6 @@ impl<'a, 'tcx> CompileState<'a, 'tcx> {
-> Self {
CompileState {
crate_name: Some(crate_name),
arena: Some(arena),
arenas: Some(arenas),
cstore: Some(cstore),
hir_map: Some(hir_map),
@@ -959,8 +951,7 @@ pub fn phase_3_run_analysis_passes<'tcx, F, R>(control: &CompileController,
hir_map: hir_map::Map<'tcx>,
mut analysis: ty::CrateAnalysis,
resolutions: Resolutions,
arena: &'tcx DroplessArena,
arenas: &'tcx GlobalArenas<'tcx>,
arenas: &'tcx AllArenas<'tcx>,
name: &str,
output_filenames: &OutputFilenames,
f: F)
@@ -1020,7 +1011,6 @@ pub fn phase_3_run_analysis_passes<'tcx, F, R>(control: &CompileController,
local_providers,
extern_providers,
arenas,
arena,
resolutions,
hir_map,
query_result_on_disk_cache,
5 changes: 2 additions & 3 deletions src/librustc_driver/lib.rs
Original file line number Diff line number Diff line change
@@ -227,7 +227,7 @@ pub fn run_compiler<'a>(args: &[String],
},
};

let cstore = Rc::new(CStore::new(DefaultTransCrate::metadata_loader()));
let cstore = CStore::new(DefaultTransCrate::metadata_loader());

let loader = file_loader.unwrap_or(box RealFileLoader);
let codemap = Rc::new(CodeMap::with_file_loader(loader, sopts.file_path_mapping()));
@@ -243,7 +243,7 @@ pub fn run_compiler<'a>(args: &[String],

do_or_return!(callbacks.late_callback(&matches,
&sess,
&*cstore,
&cstore,
&input,
&odir,
&ofile), Some(sess));
@@ -579,7 +579,6 @@ impl<'a> CompilerCalls<'a> for RustcDefaultCalls {
&state.expanded_crate.take().unwrap(),
state.crate_name.unwrap(),
ppm,
state.arena.unwrap(),
state.arenas.unwrap(),
state.output_filenames.unwrap(),
opt_uii.clone(),
20 changes: 4 additions & 16 deletions src/librustc_driver/pretty.rs
Original file line number Diff line number Diff line change
@@ -17,7 +17,7 @@ use self::NodesMatchingUII::*;

use {abort_on_err, driver};

use rustc::ty::{self, TyCtxt, GlobalArenas, Resolutions};
use rustc::ty::{self, TyCtxt, Resolutions, AllArenas};
use rustc::cfg;
use rustc::cfg::graphviz::LabelledCFG;
use rustc::middle::cstore::CrateStore;
@@ -51,8 +51,6 @@ use rustc::hir::map::blocks;
use rustc::hir;
use rustc::hir::print as pprust_hir;

use arena::DroplessArena;

#[derive(Copy, Clone, PartialEq, Debug)]
pub enum PpSourceMode {
PpmNormal,
@@ -205,8 +203,7 @@ impl PpSourceMode {
hir_map: &hir_map::Map<'tcx>,
analysis: &ty::CrateAnalysis,
resolutions: &Resolutions,
arena: &'tcx DroplessArena,
arenas: &'tcx GlobalArenas<'tcx>,
arenas: &'tcx AllArenas<'tcx>,
output_filenames: &OutputFilenames,
id: &str,
f: F)
@@ -237,7 +234,6 @@ impl PpSourceMode {
hir_map.clone(),
analysis.clone(),
resolutions.clone(),
arena,
arenas,
id,
output_filenames,
@@ -912,8 +908,7 @@ pub fn print_after_hir_lowering<'tcx, 'a: 'tcx>(sess: &'a Session,
krate: &ast::Crate,
crate_name: &str,
ppm: PpMode,
arena: &'tcx DroplessArena,
arenas: &'tcx GlobalArenas<'tcx>,
arenas: &'tcx AllArenas<'tcx>,
output_filenames: &OutputFilenames,
opt_uii: Option<UserIdentifiedItem>,
ofile: Option<&Path>) {
@@ -924,7 +919,6 @@ pub fn print_after_hir_lowering<'tcx, 'a: 'tcx>(sess: &'a Session,
analysis,
resolutions,
crate_name,
arena,
arenas,
output_filenames,
ppm,
@@ -963,7 +957,6 @@ pub fn print_after_hir_lowering<'tcx, 'a: 'tcx>(sess: &'a Session,
hir_map,
analysis,
resolutions,
arena,
arenas,
output_filenames,
crate_name,
@@ -988,7 +981,6 @@ pub fn print_after_hir_lowering<'tcx, 'a: 'tcx>(sess: &'a Session,
hir_map,
analysis,
resolutions,
arena,
arenas,
output_filenames,
crate_name,
@@ -1005,7 +997,6 @@ pub fn print_after_hir_lowering<'tcx, 'a: 'tcx>(sess: &'a Session,
hir_map,
analysis,
resolutions,
arena,
arenas,
output_filenames,
crate_name,
@@ -1040,7 +1031,6 @@ pub fn print_after_hir_lowering<'tcx, 'a: 'tcx>(sess: &'a Session,
hir_map,
analysis,
resolutions,
arena,
arenas,
output_filenames,
crate_name,
@@ -1071,8 +1061,7 @@ fn print_with_analysis<'tcx, 'a: 'tcx>(sess: &'a Session,
analysis: &ty::CrateAnalysis,
resolutions: &Resolutions,
crate_name: &str,
arena: &'tcx DroplessArena,
arenas: &'tcx GlobalArenas<'tcx>,
arenas: &'tcx AllArenas<'tcx>,
output_filenames: &OutputFilenames,
ppm: PpMode,
uii: Option<UserIdentifiedItem>,
@@ -1094,7 +1083,6 @@ fn print_with_analysis<'tcx, 'a: 'tcx>(sess: &'a Session,
hir_map.clone(),
analysis.clone(),
resolutions.clone(),
arena,
arenas,
crate_name,
output_filenames,
5 changes: 1 addition & 4 deletions src/librustc_driver/test.rs
Original file line number Diff line number Diff line change
@@ -40,7 +40,6 @@ use errors::{Level, DiagnosticBuilder};
use syntax::feature_gate::UnstableFeatures;
use syntax::symbol::Symbol;
use syntax_pos::DUMMY_SP;
use arena::DroplessArena;

use rustc::hir;

@@ -131,8 +130,7 @@ fn test_env<F>(source_string: &str,
.expect("phase 2 aborted")
};

let arena = DroplessArena::new();
let arenas = ty::GlobalArenas::new();
let arenas = ty::AllArenas::new();
let hir_map = hir_map::map_crate(&sess, &*cstore, &mut hir_forest, &defs);

// run just enough stuff to build a tcx:
@@ -149,7 +147,6 @@ fn test_env<F>(source_string: &str,
ty::maps::Providers::default(),
ty::maps::Providers::default(),
&arenas,
&arena,
resolutions,
hir_map,
OnDiskCache::new_empty(sess.codemap()),
18 changes: 10 additions & 8 deletions src/librustc_errors/lib.rs
Original file line number Diff line number Diff line change
@@ -42,6 +42,8 @@ use std::cell::{RefCell, Cell};
use std::mem;
use std::rc::Rc;
use std::{error, fmt};
use std::sync::atomic::AtomicUsize;
Copy link
Contributor

@arielb1 arielb1 Dec 18, 2017

Choose a reason for hiding this comment

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

err_count probably needs to be done in a smarter way than here to assure thread-safety, so could you add a FIXME? In any case this is better than the way before this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In which case is this not thread safe?

use std::sync::atomic::Ordering::SeqCst;

mod diagnostic;
mod diagnostic_builder;
@@ -236,7 +238,7 @@ pub use diagnostic_builder::DiagnosticBuilder;
pub struct Handler {
pub flags: HandlerFlags,

err_count: Cell<usize>,
err_count: AtomicUsize,
emitter: RefCell<Box<Emitter>>,
continue_after_error: Cell<bool>,
delayed_span_bug: RefCell<Option<Diagnostic>>,
@@ -295,7 +297,7 @@ impl Handler {
pub fn with_emitter_and_flags(e: Box<Emitter>, flags: HandlerFlags) -> Handler {
Handler {
flags,
err_count: Cell::new(0),
err_count: AtomicUsize::new(0),
emitter: RefCell::new(e),
continue_after_error: Cell::new(true),
delayed_span_bug: RefCell::new(None),
@@ -311,7 +313,7 @@ impl Handler {
// NOTE: DO NOT call this function from rustc, as it relies on `err_count` being non-zero
// if an error happened to avoid ICEs. This function should only be called from tools.
pub fn reset_err_count(&self) {
self.err_count.set(0);
self.err_count.store(0, SeqCst);
}

pub fn struct_dummy<'a>(&'a self) -> DiagnosticBuilder<'a> {
@@ -507,19 +509,19 @@ impl Handler {

fn bump_err_count(&self) {
self.panic_if_treat_err_as_bug();
self.err_count.set(self.err_count.get() + 1);
self.err_count.fetch_add(1, SeqCst);
}

pub fn err_count(&self) -> usize {
self.err_count.get()
self.err_count.load(SeqCst)
}

pub fn has_errors(&self) -> bool {
self.err_count.get() > 0
self.err_count() > 0
}
pub fn abort_if_errors(&self) {
let s;
match self.err_count.get() {
match self.err_count() {
0 => {
if let Some(bug) = self.delayed_span_bug.borrow_mut().take() {
DiagnosticBuilder::new_diagnostic(self, bug).emit();
@@ -528,7 +530,7 @@ impl Handler {
}
1 => s = "aborting due to previous error".to_string(),
_ => {
s = format!("aborting due to {} previous errors", self.err_count.get());
s = format!("aborting due to {} previous errors", self.err_count());
}
}

7 changes: 2 additions & 5 deletions src/librustdoc/core.rs
Original file line number Diff line number Diff line change
@@ -14,7 +14,7 @@ use rustc::session::{self, config};
use rustc::hir::def_id::DefId;
use rustc::hir::def::Def;
use rustc::middle::privacy::AccessLevels;
use rustc::ty::{self, TyCtxt, GlobalArenas};
use rustc::ty::{self, TyCtxt, AllArenas};
use rustc::hir::map as hir_map;
use rustc::lint;
use rustc::util::nodemap::FxHashMap;
@@ -37,7 +37,6 @@ use visit_ast::RustdocVisitor;
use clean;
use clean::Clean;
use html::render::RenderInfo;
use arena::DroplessArena;

pub use rustc::session::config::Input;
pub use rustc::session::search_paths::SearchPaths;
@@ -170,8 +169,7 @@ pub fn run_core(search_paths: SearchPaths,
abort_on_err(result, &sess)
};

let arena = DroplessArena::new();
let arenas = GlobalArenas::new();
let arenas = AllArenas::new();
let hir_map = hir_map::map_crate(&sess, &*cstore, &mut hir_forest, &defs);
let output_filenames = driver::build_output_filenames(&input,
&None,
@@ -185,7 +183,6 @@ pub fn run_core(search_paths: SearchPaths,
hir_map,
analysis,
resolutions,
&arena,
&arenas,
&name,
&output_filenames,
24 changes: 24 additions & 0 deletions src/libserialize/collection_impls.rs
Original file line number Diff line number Diff line change
@@ -15,6 +15,7 @@ use std::hash::{Hash, BuildHasher};
use {Decodable, Encodable, Decoder, Encoder};
use std::collections::{LinkedList, VecDeque, BTreeMap, BTreeSet, HashMap, HashSet};
use std::rc::Rc;
use std::sync::Arc;

impl<
T: Encodable
@@ -218,3 +219,26 @@ impl<T: Decodable> Decodable for Rc<[T]> {
})
}
}

impl<T: Encodable> Encodable for Arc<[T]> {
fn encode<E: Encoder>(&self, s: &mut E) -> Result<(), E::Error> {
s.emit_seq(self.len(), |s| {
for (index, e) in self.iter().enumerate() {
s.emit_seq_elt(index, |s| e.encode(s))?;
}
Ok(())
})
}
}

impl<T: Decodable> Decodable for Arc<[T]> {
fn decode<D: Decoder>(d: &mut D) -> Result<Arc<[T]>, D::Error> {
d.read_seq(|d, len| {
let mut vec = Vec::with_capacity(len);
for index in 0..len {
vec.push(d.read_seq_elt(index, |d| Decodable::decode(d))?);
}
Ok(vec.into())
})
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Missing trailing newline

16 changes: 7 additions & 9 deletions src/libsyntax/attr.rs
Original file line number Diff line number Diff line change
@@ -31,7 +31,7 @@ use symbol::Symbol;
use tokenstream::{TokenStream, TokenTree, Delimited};
use util::ThinVec;

use std::cell::{RefCell, Cell};
use std::cell::RefCell;
use std::iter;

thread_local! {
@@ -419,16 +419,14 @@ pub fn mk_spanned_word_item(sp: Span, name: Name) -> MetaItem {
MetaItem { span: sp, name: name, node: MetaItemKind::Word }
}

pub fn mk_attr_id() -> AttrId {
use std::sync::atomic::AtomicUsize;
use std::sync::atomic::Ordering;

static NEXT_ATTR_ID: AtomicUsize = AtomicUsize::new(0);

thread_local! { static NEXT_ATTR_ID: Cell<usize> = Cell::new(0) }

pub fn mk_attr_id() -> AttrId {
let id = NEXT_ATTR_ID.with(|slot| {
let r = slot.get();
slot.set(r + 1);
r
});
let id = NEXT_ATTR_ID.fetch_add(1, Ordering::SeqCst);
Copy link
Member

Choose a reason for hiding this comment

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

@rust-lang/compiler Not sure how I feel about this kind of stuff - it will behave non-deterministically in any process running multiple rustc threads in parallel (atm rustc "owns" a thread).
IMO all thread-locals should change to scoped thread locals initialized to point to some memory owned by the thread that started the compilation, if we want multiple threads per rustc "instance".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll make this either a scoped thread local or put it in ParseSess if that happens to be easy.

assert!(id != ::std::usize::MAX);
AttrId(id)
}

1 change: 1 addition & 0 deletions src/libsyntax/lib.rs
Original file line number Diff line number Diff line change
@@ -24,6 +24,7 @@
#![feature(rustc_diagnostic_macros)]
#![feature(match_default_bindings)]
#![feature(i128_type)]
#![feature(const_atomic_usize_new)]

// See librustc_cratesio_shim/Cargo.toml for a comment explaining this.
#[allow(unused_extern_crates)]