From 4cc5aaada2f8ffd444a7fbb10394b83ba3156525 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Sat, 7 Sep 2019 12:09:52 -0400 Subject: [PATCH] Protect error handler fields with single lock This avoids concurrency-related bugs when locks are acquired for too short a time and similar cases. --- src/librustc/session/config/tests.rs | 6 +- src/librustc_errors/lib.rs | 336 ++++++++++++++++----------- 2 files changed, 207 insertions(+), 135 deletions(-) diff --git a/src/librustc/session/config/tests.rs b/src/librustc/session/config/tests.rs index 3d6312548a47b..9eb68056bfd97 100644 --- a/src/librustc/session/config/tests.rs +++ b/src/librustc/session/config/tests.rs @@ -87,7 +87,7 @@ fn test_can_print_warnings() { let registry = errors::registry::Registry::new(&[]); let (sessopts, _) = build_session_options_and_crate_config(&matches); let sess = build_session(sessopts, None, registry); - assert!(!sess.diagnostic().flags.can_emit_warnings); + assert!(!sess.diagnostic().can_emit_warnings()); }); syntax::with_default_globals(|| { @@ -97,7 +97,7 @@ fn test_can_print_warnings() { let registry = errors::registry::Registry::new(&[]); let (sessopts, _) = build_session_options_and_crate_config(&matches); let sess = build_session(sessopts, None, registry); - assert!(sess.diagnostic().flags.can_emit_warnings); + assert!(sess.diagnostic().can_emit_warnings()); }); syntax::with_default_globals(|| { @@ -105,7 +105,7 @@ fn test_can_print_warnings() { let registry = errors::registry::Registry::new(&[]); let (sessopts, _) = build_session_options_and_crate_config(&matches); let sess = build_session(sessopts, None, registry); - assert!(sess.diagnostic().flags.can_emit_warnings); + assert!(sess.diagnostic().can_emit_warnings()); }); } diff --git a/src/librustc_errors/lib.rs b/src/librustc_errors/lib.rs index bd57cc41ef335..a1a63664a4be3 100644 --- a/src/librustc_errors/lib.rs +++ b/src/librustc_errors/lib.rs @@ -16,7 +16,7 @@ use Level::*; use emitter::{Emitter, EmitterWriter}; use registry::Registry; -use rustc_data_structures::sync::{self, Lrc, Lock, AtomicUsize, AtomicBool, SeqCst}; +use rustc_data_structures::sync::{self, Lrc, Lock}; use rustc_data_structures::fx::FxHashSet; use rustc_data_structures::stable_hasher::StableHasher; @@ -298,30 +298,34 @@ pub use diagnostic_builder::DiagnosticBuilder; /// Certain errors (fatal, bug, unimpl) may cause immediate exit, /// others log errors for later reporting. pub struct Handler { - pub flags: HandlerFlags, + flags: HandlerFlags, + inner: Lock, +} +struct HandlerInner { + flags: HandlerFlags, /// The number of errors that have been emitted, including duplicates. /// /// This is not necessarily the count that's reported to the user once /// compilation ends. - err_count: AtomicUsize, - deduplicated_err_count: AtomicUsize, - emitter: Lock>, - continue_after_error: AtomicBool, - delayed_span_bugs: Lock>, + err_count: usize, + deduplicated_err_count: usize, + emitter: Box, + continue_after_error: bool, + delayed_span_bugs: Vec, /// This set contains the `DiagnosticId` of all emitted diagnostics to avoid /// emitting the same diagnostic with extended help (`--teach`) twice, which /// would be uneccessary repetition. - taught_diagnostics: Lock>, + taught_diagnostics: FxHashSet, /// Used to suggest rustc --explain - emitted_diagnostic_codes: Lock>, + emitted_diagnostic_codes: FxHashSet, /// This set contains a hash of every diagnostic that has been emitted by /// this handler. These hashes is used to avoid emitting the same error /// twice. - emitted_diagnostics: Lock>, + emitted_diagnostics: FxHashSet, } fn default_track_diagnostic(_: &Diagnostic) {} @@ -329,7 +333,7 @@ fn default_track_diagnostic(_: &Diagnostic) {} thread_local!(pub static TRACK_DIAGNOSTICS: Cell = Cell::new(default_track_diagnostic)); -#[derive(Default)] +#[derive(Copy, Clone, Default)] pub struct HandlerFlags { /// If false, warning-level lints are suppressed. /// (rustc: see `--allow warnings` and `--cap-lints`) @@ -348,13 +352,13 @@ pub struct HandlerFlags { pub external_macro_backtrace: bool, } -impl Drop for Handler { +impl Drop for HandlerInner { fn drop(&mut self) { - if !self.has_errors() { - let mut bugs = self.delayed_span_bugs.borrow_mut(); + if self.err_count == 0 { + let bugs = std::mem::replace(&mut self.delayed_span_bugs, Vec::new()); let has_bugs = !bugs.is_empty(); - for bug in bugs.drain(..) { - DiagnosticBuilder::new_diagnostic(self, bug).emit(); + for bug in bugs { + self.emit_diagnostic(&bug); } if has_bugs { panic!("no errors encountered even though `delay_span_bug` issued"); @@ -405,19 +409,28 @@ impl Handler { { Handler { flags, - err_count: AtomicUsize::new(0), - deduplicated_err_count: AtomicUsize::new(0), - emitter: Lock::new(e), - continue_after_error: AtomicBool::new(true), - delayed_span_bugs: Lock::new(Vec::new()), - taught_diagnostics: Default::default(), - emitted_diagnostic_codes: Default::default(), - emitted_diagnostics: Default::default(), + inner: Lock::new(HandlerInner { + flags, + err_count: 0, + deduplicated_err_count: 0, + emitter: e, + continue_after_error: true, + delayed_span_bugs: Vec::new(), + taught_diagnostics: Default::default(), + emitted_diagnostic_codes: Default::default(), + emitted_diagnostics: Default::default(), + }), } } pub fn set_continue_after_error(&self, continue_after_error: bool) { - self.continue_after_error.store(continue_after_error, SeqCst); + self.inner.borrow_mut().continue_after_error = continue_after_error; + } + + // This is here to not allow mutation of flags; + // as of this writing it's only used in tests in librustc. + pub fn can_emit_warnings(&self) -> bool { + self.flags.can_emit_warnings } /// Resets the diagnostic error count as well as the cached emitted diagnostics. @@ -425,11 +438,13 @@ impl Handler { /// NOTE: *do not* call this function from rustc. It is only meant to be called from external /// tools that want to reuse a `Parser` cleaning the previously emitted diagnostics as well as /// the overall count of emitted error diagnostics. + // FIXME: this does not clear inner entirely pub fn reset_err_count(&self) { + let mut inner = self.inner.borrow_mut(); // actually frees the underlying memory (which `clear` would not do) - *self.emitted_diagnostics.borrow_mut() = Default::default(); - self.deduplicated_err_count.store(0, SeqCst); - self.err_count.store(0, SeqCst); + inner.emitted_diagnostics = Default::default(); + inner.deduplicated_err_count = 0; + inner.err_count = 0; } pub fn struct_dummy(&self) -> DiagnosticBuilder<'_> { @@ -520,24 +535,6 @@ impl Handler { DiagnosticBuilder::new(self, Level::Fatal, msg) } - fn panic_if_treat_err_as_bug(&self) { - if self.treat_err_as_bug() { - let s = match (self.err_count(), self.flags.treat_err_as_bug.unwrap_or(0)) { - (0, _) => return, - (1, 1) => "aborting due to `-Z treat-err-as-bug=1`".to_string(), - (1, _) => return, - (count, as_bug) => { - format!( - "aborting after {} errors due to `-Z treat-err-as-bug={}`", - count, - as_bug, - ) - } - }; - panic!(s); - } - } - pub fn span_fatal>(&self, sp: S, msg: &str) -> FatalError { self.emit_diagnostic(Diagnostic::new(Fatal, msg).set_span(sp)); self.abort_if_errors_and_should_abort(); @@ -577,24 +574,10 @@ impl Handler { self.abort_if_errors_and_should_abort(); } pub fn span_bug>(&self, sp: S, msg: &str) -> ! { - self.emit_diagnostic(Diagnostic::new(Bug, msg).set_span(sp)); - self.abort_if_errors_and_should_abort(); - panic!(ExplicitBug); + self.inner.borrow_mut().span_bug(sp, msg) } pub fn delay_span_bug>(&self, sp: S, msg: &str) { - if self.treat_err_as_bug() { - // FIXME: don't abort here if report_delayed_bugs is off - self.span_bug(sp, msg); - } - let mut diagnostic = Diagnostic::new(Level::Bug, msg); - diagnostic.set_span(sp.into()); - self.delay_as_bug(diagnostic); - } - fn delay_as_bug(&self, diagnostic: Diagnostic) { - if self.flags.report_delayed_bugs { - self.emit_diagnostic(&diagnostic); - } - self.delayed_span_bugs.borrow_mut().push(diagnostic); + self.inner.borrow_mut().delay_span_bug(sp, msg) } pub fn span_bug_no_panic>(&self, sp: S, msg: &str) { self.emit_diagnostic(Diagnostic::new(Bug, msg).set_span(sp)); @@ -613,46 +596,28 @@ impl Handler { db } pub fn failure(&self, msg: &str) { - DiagnosticBuilder::new(self, FailureNote, msg).emit() + self.inner.borrow_mut().failure(msg); } pub fn fatal(&self, msg: &str) -> FatalError { - if self.treat_err_as_bug() { - self.bug(msg); - } - DiagnosticBuilder::new(self, Fatal, msg).emit(); - FatalError + self.inner.borrow_mut().fatal(msg) } pub fn err(&self, msg: &str) { - if self.treat_err_as_bug() { - self.bug(msg); - } - let mut db = DiagnosticBuilder::new(self, Error, msg); - db.emit(); + self.inner.borrow_mut().err(msg); } pub fn warn(&self, msg: &str) { let mut db = DiagnosticBuilder::new(self, Warning, msg); db.emit(); } - fn treat_err_as_bug(&self) -> bool { - self.flags.treat_err_as_bug.map(|c| self.err_count() >= c).unwrap_or(false) - } pub fn note_without_error(&self, msg: &str) { let mut db = DiagnosticBuilder::new(self, Note, msg); db.emit(); } pub fn bug(&self, msg: &str) -> ! { - let mut db = DiagnosticBuilder::new(self, Bug, msg); - db.emit(); - panic!(ExplicitBug); - } - - fn bump_err_count(&self) { - self.err_count.fetch_add(1, SeqCst); - self.panic_if_treat_err_as_bug(); + self.inner.borrow_mut().bug(msg) } pub fn err_count(&self) -> usize { - self.err_count.load(SeqCst) + self.inner.borrow().err_count } pub fn has_errors(&self) -> bool { @@ -660,7 +625,99 @@ impl Handler { } pub fn print_error_count(&self, registry: &Registry) { - let s = match self.deduplicated_err_count.load(SeqCst) { + self.inner.borrow_mut().print_error_count(registry) + } + + pub fn abort_if_errors(&self) { + self.inner.borrow().abort_if_errors() + } + + pub fn abort_if_errors_and_should_abort(&self) { + self.inner.borrow().abort_if_errors_and_should_abort() + } + + pub fn must_teach(&self, code: &DiagnosticId) -> bool { + self.inner.borrow_mut().must_teach(code) + } + + pub fn force_print_diagnostic(&self, db: Diagnostic) { + self.inner.borrow_mut().force_print_diagnostic(db) + } + + pub fn emit_diagnostic(&self, diagnostic: &Diagnostic) { + self.inner.borrow_mut().emit_diagnostic(diagnostic) + } + + pub fn emit_artifact_notification(&self, path: &Path, artifact_type: &str) { + self.inner.borrow_mut().emit_artifact_notification(path, artifact_type) + } + + pub fn delay_as_bug(&self, diagnostic: Diagnostic) { + self.inner.borrow_mut().delay_as_bug(diagnostic) + } +} + +impl HandlerInner { + /// `true` if we haven't taught a diagnostic with this code already. + /// The caller must then teach the user about such a diagnostic. + /// + /// Used to suppress emitting the same error multiple times with extended explanation when + /// calling `-Zteach`. + fn must_teach(&mut self, code: &DiagnosticId) -> bool { + self.taught_diagnostics.insert(code.clone()) + } + + fn force_print_diagnostic(&mut self, db: Diagnostic) { + self.emitter.emit_diagnostic(&db); + } + + fn emit_diagnostic(&mut self, diagnostic: &Diagnostic) { + if diagnostic.cancelled() { + return; + } + + if diagnostic.level == Warning && !self.flags.can_emit_warnings { + return; + } + + TRACK_DIAGNOSTICS.with(|track_diagnostics| { + track_diagnostics.get()(diagnostic); + }); + + if let Some(ref code) = diagnostic.code { + self.emitted_diagnostic_codes.insert(code.clone()); + } + + let diagnostic_hash = { + use std::hash::Hash; + let mut hasher = StableHasher::new(); + diagnostic.hash(&mut hasher); + hasher.finish() + }; + + // Only emit the diagnostic if we haven't already emitted an equivalent + // one: + if self.emitted_diagnostics.insert(diagnostic_hash) { + self.emitter.emit_diagnostic(diagnostic); + if diagnostic.is_error() { + self.deduplicated_err_count += 1; + } + } + if diagnostic.is_error() { + self.bump_err_count(); + } + } + + fn emit_artifact_notification(&mut self, path: &Path, artifact_type: &str) { + self.emitter.emit_artifact_notification(path, artifact_type); + } + + fn treat_err_as_bug(&self) -> bool { + self.flags.treat_err_as_bug.map(|c| self.err_count >= c).unwrap_or(false) + } + + fn print_error_count(&mut self, registry: &Registry) { + let s = match self.deduplicated_err_count { 0 => return, 1 => "aborting due to previous error".to_string(), count => format!("aborting due to {} previous errors", count) @@ -671,12 +728,11 @@ impl Handler { let _ = self.fatal(&s); - let can_show_explain = self.emitter.borrow().should_show_explain(); - let are_there_diagnostics = !self.emitted_diagnostic_codes.borrow().is_empty(); + let can_show_explain = self.emitter.should_show_explain(); + let are_there_diagnostics = !self.emitted_diagnostic_codes.is_empty(); if can_show_explain && are_there_diagnostics { let mut error_codes = self .emitted_diagnostic_codes - .borrow() .iter() .filter_map(|x| match &x { DiagnosticId::Error(s) if registry.find_description(s).is_some() => { @@ -704,70 +760,86 @@ impl Handler { } } - pub fn abort_if_errors_and_should_abort(&self) { - if self.has_errors() && !self.continue_after_error.load(SeqCst) { + fn abort_if_errors_and_should_abort(&self) { + if self.err_count > 0 && !self.continue_after_error { FatalError.raise(); } } - pub fn abort_if_errors(&self) { - if self.has_errors() { + fn abort_if_errors(&self) { + if self.err_count > 0 { FatalError.raise(); } } - /// `true` if we haven't taught a diagnostic with this code already. - /// The caller must then teach the user about such a diagnostic. - /// - /// Used to suppress emitting the same error multiple times with extended explanation when - /// calling `-Zteach`. - pub fn must_teach(&self, code: &DiagnosticId) -> bool { - self.taught_diagnostics.borrow_mut().insert(code.clone()) + fn span_bug>(&mut self, sp: S, msg: &str) -> ! { + self.emit_diagnostic(Diagnostic::new(Bug, msg).set_span(sp)); + self.abort_if_errors_and_should_abort(); + panic!(ExplicitBug); } - pub fn force_print_diagnostic(&self, db: Diagnostic) { - self.emitter.borrow_mut().emit_diagnostic(&db); + fn delay_span_bug>(&mut self, sp: S, msg: &str) { + if self.treat_err_as_bug() { + // FIXME: don't abort here if report_delayed_bugs is off + self.span_bug(sp, msg); + } + let mut diagnostic = Diagnostic::new(Level::Bug, msg); + diagnostic.set_span(sp.into()); + self.delay_as_bug(diagnostic) } - pub fn emit_diagnostic(&self, diagnostic: &Diagnostic) { - if diagnostic.cancelled() { - return; - } + fn failure(&mut self, msg: &str) { + self.emit_diagnostic(&Diagnostic::new(FailureNote, msg)); + } - if diagnostic.level == Warning && !self.flags.can_emit_warnings { - return; + fn fatal(&mut self, msg: &str) -> FatalError { + if self.treat_err_as_bug() { + self.bug(msg); } + self.emit_diagnostic(&Diagnostic::new(Fatal, msg)); + FatalError + } - TRACK_DIAGNOSTICS.with(|track_diagnostics| { - track_diagnostics.get()(diagnostic); - }); - - if let Some(ref code) = diagnostic.code { - self.emitted_diagnostic_codes.borrow_mut().insert(code.clone()); + fn err(&mut self, msg: &str) { + if self.treat_err_as_bug() { + self.bug(msg); } + self.emit_diagnostic(&Diagnostic::new(Error, msg)); + } - let diagnostic_hash = { - use std::hash::Hash; - let mut hasher = StableHasher::new(); - diagnostic.hash(&mut hasher); - hasher.finish() - }; + fn bug(&mut self, msg: &str) -> ! { + self.emit_diagnostic(&Diagnostic::new(Bug, msg)); + panic!(ExplicitBug); + } - // Only emit the diagnostic if we haven't already emitted an equivalent - // one: - if self.emitted_diagnostics.borrow_mut().insert(diagnostic_hash) { - self.emitter.borrow_mut().emit_diagnostic(diagnostic); - if diagnostic.is_error() { - self.deduplicated_err_count.fetch_add(1, SeqCst); - } - } - if diagnostic.is_error() { - self.bump_err_count(); + fn delay_as_bug(&mut self, diagnostic: Diagnostic) { + if self.flags.report_delayed_bugs { + self.emit_diagnostic(&diagnostic); } + self.delayed_span_bugs.push(diagnostic); } - pub fn emit_artifact_notification(&self, path: &Path, artifact_type: &str) { - self.emitter.borrow_mut().emit_artifact_notification(path, artifact_type); + fn bump_err_count(&mut self) { + self.err_count += 1; + self.panic_if_treat_err_as_bug(); + } + + fn panic_if_treat_err_as_bug(&self) { + if self.treat_err_as_bug() { + let s = match (self.err_count, self.flags.treat_err_as_bug.unwrap_or(0)) { + (0, _) => return, + (1, 1) => "aborting due to `-Z treat-err-as-bug=1`".to_string(), + (1, _) => return, + (count, as_bug) => { + format!( + "aborting after {} errors due to `-Z treat-err-as-bug={}`", + count, + as_bug, + ) + } + }; + panic!(s); + } } }