From b91d211b40300a3c026b330e50a6e3e19d71351c Mon Sep 17 00:00:00 2001 From: Peter Jin Date: Mon, 31 Dec 2018 10:58:13 -0800 Subject: [PATCH 1/8] Add a target option "merge-functions" taking values in ("disabled", "trampolines", or "aliases (the default)) to allow targets to opt out of the MergeFunctions LLVM pass. Also add a corresponding -Z option with the same name and values. This works around: https://github.com/rust-lang/rust/issues/57356 Motivation: Basically, the problem is that the MergeFunctions pass, which rustc currently enables by default at -O2 and -O3, and `extern "ptx-kernel"` functions (specific to the NVPTX target) are currently not compatible with each other. If the MergeFunctions pass is allowed to run, rustc can generate invalid PTX assembly (i.e. a PTX file that is not accepted by the native PTX assembler ptxas). Therefore we would like a way to opt out of the MergeFunctions pass, which is what our target option does. Related work: The current behavior of rustc is to enable MergeFunctions at -O2 and -O3, and also to enable the use of function aliases within MergeFunctions. MergeFunctions both with and without function aliases is incompatible with the NVPTX target. clang's "solution" is to have a "-fmerge-functions" flag that opts in to the MergeFunctions pass, but it is not enabled by default. --- src/librustc/session/config.rs | 28 +++++++++-- src/librustc_codegen_llvm/llvm_util.rs | 10 +++- src/librustc_codegen_ssa/back/write.rs | 21 +++++++- src/librustc_target/spec/mod.rs | 66 +++++++++++++++++++++++++- 4 files changed, 117 insertions(+), 8 deletions(-) diff --git a/src/librustc/session/config.rs b/src/librustc/session/config.rs index 33409f9b4a74f..ca4ab15d79b1d 100644 --- a/src/librustc/session/config.rs +++ b/src/librustc/session/config.rs @@ -6,7 +6,7 @@ use std::str::FromStr; use session::{early_error, early_warn, Session}; use session::search_paths::SearchPath; -use rustc_target::spec::{LinkerFlavor, PanicStrategy, RelroLevel}; +use rustc_target::spec::{LinkerFlavor, MergeFunctions, PanicStrategy, RelroLevel}; use rustc_target::spec::{Target, TargetTriple}; use lint; use middle::cstore; @@ -808,13 +808,16 @@ macro_rules! options { pub const parse_cross_lang_lto: Option<&str> = Some("either a boolean (`yes`, `no`, `on`, `off`, etc), \ or the path to the linker plugin"); + pub const parse_merge_functions: Option<&str> = + Some("one of: `disabled`, `trampolines`, or `aliases`"); } #[allow(dead_code)] mod $mod_set { use super::{$struct_name, Passes, Sanitizer, LtoCli, CrossLangLto}; - use rustc_target::spec::{LinkerFlavor, PanicStrategy, RelroLevel}; + use rustc_target::spec::{LinkerFlavor, MergeFunctions, PanicStrategy, RelroLevel}; use std::path::PathBuf; + use std::str::FromStr; $( pub fn $opt(cg: &mut $struct_name, v: Option<&str>) -> bool { @@ -1046,6 +1049,14 @@ macro_rules! options { }; true } + + fn parse_merge_functions(slot: &mut Option, v: Option<&str>) -> bool { + match v.and_then(|s| MergeFunctions::from_str(s).ok()) { + Some(mergefunc) => *slot = Some(mergefunc), + _ => return false, + } + true + } } ) } @@ -1380,6 +1391,9 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, "whether to use the PLT when calling into shared libraries; only has effect for PIC code on systems with ELF binaries (default: PLT is disabled if full relro is enabled)"), + merge_functions: Option = (None, parse_merge_functions, [TRACKED], + "control the operation of the MergeFunctions LLVM pass, taking + the same values as the target option of the same name"), } pub fn default_lib_output() -> CrateType { @@ -2398,7 +2412,7 @@ mod dep_tracking { use super::{CrateType, DebugInfo, ErrorOutputType, OptLevel, OutputTypes, Passes, Sanitizer, LtoCli, CrossLangLto}; use syntax::feature_gate::UnstableFeatures; - use rustc_target::spec::{PanicStrategy, RelroLevel, TargetTriple}; + use rustc_target::spec::{MergeFunctions, PanicStrategy, RelroLevel, TargetTriple}; use syntax::edition::Edition; pub trait DepTrackingHash { @@ -2441,12 +2455,14 @@ mod dep_tracking { impl_dep_tracking_hash_via_hash!(Option); impl_dep_tracking_hash_via_hash!(Option); impl_dep_tracking_hash_via_hash!(Option<(String, u64)>); + impl_dep_tracking_hash_via_hash!(Option); impl_dep_tracking_hash_via_hash!(Option); impl_dep_tracking_hash_via_hash!(Option); impl_dep_tracking_hash_via_hash!(Option); impl_dep_tracking_hash_via_hash!(Option); impl_dep_tracking_hash_via_hash!(Option); impl_dep_tracking_hash_via_hash!(CrateType); + impl_dep_tracking_hash_via_hash!(MergeFunctions); impl_dep_tracking_hash_via_hash!(PanicStrategy); impl_dep_tracking_hash_via_hash!(RelroLevel); impl_dep_tracking_hash_via_hash!(Passes); @@ -2532,7 +2548,7 @@ mod tests { use std::iter::FromIterator; use std::path::PathBuf; use super::{Externs, OutputType, OutputTypes}; - use rustc_target::spec::{PanicStrategy, RelroLevel}; + use rustc_target::spec::{MergeFunctions, PanicStrategy, RelroLevel}; use syntax::symbol::Symbol; use syntax::edition::{Edition, DEFAULT_EDITION}; use syntax; @@ -3187,6 +3203,10 @@ mod tests { opts = reference.clone(); opts.debugging_opts.cross_lang_lto = CrossLangLto::LinkerPluginAuto; assert!(reference.dep_tracking_hash() != opts.dep_tracking_hash()); + + opts = reference.clone(); + opts.debugging_opts.merge_functions = Some(MergeFunctions::Disabled); + assert!(reference.dep_tracking_hash() != opts.dep_tracking_hash()); } #[test] diff --git a/src/librustc_codegen_llvm/llvm_util.rs b/src/librustc_codegen_llvm/llvm_util.rs index ad9ebbcde8ab9..dc70ebcf943a5 100644 --- a/src/librustc_codegen_llvm/llvm_util.rs +++ b/src/librustc_codegen_llvm/llvm_util.rs @@ -3,6 +3,7 @@ use back::write::create_target_machine; use llvm; use rustc::session::Session; use rustc::session::config::PrintRequest; +use rustc_target::spec::MergeFunctions; use libc::c_int; use std::ffi::CString; use syntax::feature_gate::UnstableFeatures; @@ -61,7 +62,14 @@ unsafe fn configure_llvm(sess: &Session) { add("-disable-preinline"); } if llvm::LLVMRustIsRustLLVM() { - add("-mergefunc-use-aliases"); + match sess.opts.debugging_opts.merge_functions + .unwrap_or(sess.target.target.options.merge_functions) { + MergeFunctions::Disabled | + MergeFunctions::Trampolines => {} + MergeFunctions::Aliases => { + add("-mergefunc-use-aliases"); + } + } } // HACK(eddyb) LLVM inserts `llvm.assume` calls to preserve align attributes diff --git a/src/librustc_codegen_ssa/back/write.rs b/src/librustc_codegen_ssa/back/write.rs index fb3e7ea696363..39bdc70f8322e 100644 --- a/src/librustc_codegen_ssa/back/write.rs +++ b/src/librustc_codegen_ssa/back/write.rs @@ -24,6 +24,7 @@ use rustc_fs_util::link_or_copy; use rustc_data_structures::svh::Svh; use rustc_errors::{Handler, Level, DiagnosticBuilder, FatalError, DiagnosticId}; use rustc_errors::emitter::{Emitter}; +use rustc_target::spec::MergeFunctions; use syntax::attr; use syntax::ext::hygiene::Mark; use syntax_pos::MultiSpan; @@ -152,8 +153,24 @@ impl ModuleConfig { sess.opts.optimize == config::OptLevel::Aggressive && !sess.target.target.options.is_like_emscripten; - self.merge_functions = sess.opts.optimize == config::OptLevel::Default || - sess.opts.optimize == config::OptLevel::Aggressive; + // Some targets (namely, NVPTX) interact badly with the MergeFunctions + // pass. This is because MergeFunctions can generate new function calls + // which may interfere with the target calling convention; e.g. for the + // NVPTX target, PTX kernels should not call other PTX kernels. + // MergeFunctions can also be configured to generate aliases instead, + // but aliases are not supported by some backends (again, NVPTX). + // Therefore, allow targets to opt out of the MergeFunctions pass, + // but otherwise keep the pass enabled (at O2 and O3) since it can be + // useful for reducing code size. + self.merge_functions = match sess.opts.debugging_opts.merge_functions + .unwrap_or(sess.target.target.options.merge_functions) { + MergeFunctions::Disabled => false, + MergeFunctions::Trampolines | + MergeFunctions::Aliases => { + sess.opts.optimize == config::OptLevel::Default || + sess.opts.optimize == config::OptLevel::Aggressive + } + }; } pub fn bitcode_needed(&self) -> bool { diff --git a/src/librustc_target/spec/mod.rs b/src/librustc_target/spec/mod.rs index f42b0a1c3c958..3a21ca19b176b 100644 --- a/src/librustc_target/spec/mod.rs +++ b/src/librustc_target/spec/mod.rs @@ -217,6 +217,46 @@ impl ToJson for RelroLevel { } } +#[derive(Clone, Copy, Debug, PartialEq, Hash, RustcEncodable, RustcDecodable)] +pub enum MergeFunctions { + Disabled, + Trampolines, + Aliases +} + +impl MergeFunctions { + pub fn desc(&self) -> &str { + match *self { + MergeFunctions::Disabled => "disabled", + MergeFunctions::Trampolines => "trampolines", + MergeFunctions::Aliases => "aliases", + } + } +} + +impl FromStr for MergeFunctions { + type Err = (); + + fn from_str(s: &str) -> Result { + match s { + "disabled" => Ok(MergeFunctions::Disabled), + "trampolines" => Ok(MergeFunctions::Trampolines), + "aliases" => Ok(MergeFunctions::Aliases), + _ => Err(()), + } + } +} + +impl ToJson for MergeFunctions { + fn to_json(&self) -> Json { + match *self { + MergeFunctions::Disabled => "disabled".to_json(), + MergeFunctions::Trampolines => "trampolines".to_json(), + MergeFunctions::Aliases => "aliases".to_json(), + } + } +} + pub type LinkArgs = BTreeMap>; pub type TargetResult = Result; @@ -690,7 +730,15 @@ pub struct TargetOptions { /// If set, have the linker export exactly these symbols, instead of using /// the usual logic to figure this out from the crate itself. - pub override_export_symbols: Option> + pub override_export_symbols: Option>, + + /// Determines how or whether the MergeFunctions LLVM pass should run for + /// this target. Either "disabled", "trampolines", or "aliases". + /// The MergeFunctions pass is generally useful, but some targets may need + /// to opt out. The default is "aliases". + /// + /// Workaround for: https://github.com/rust-lang/rust/issues/57356 + pub merge_functions: MergeFunctions } impl Default for TargetOptions { @@ -773,6 +821,7 @@ impl Default for TargetOptions { requires_uwtable: false, simd_types_indirect: true, override_export_symbols: None, + merge_functions: MergeFunctions::Aliases, } } } @@ -875,6 +924,19 @@ impl Target { .map(|o| o.as_u64() .map(|s| base.options.$key_name = Some(s))); } ); + ($key_name:ident, MergeFunctions) => ( { + let name = (stringify!($key_name)).replace("_", "-"); + obj.find(&name[..]).and_then(|o| o.as_string().and_then(|s| { + match s.parse::() { + Ok(mergefunc) => base.options.$key_name = mergefunc, + _ => return Some(Err(format!("'{}' is not a valid value for \ + merge-functions. Use 'disabled', \ + 'trampolines', or 'aliases'.", + s))), + } + Some(Ok(())) + })).unwrap_or(Ok(())) + } ); ($key_name:ident, PanicStrategy) => ( { let name = (stringify!($key_name)).replace("_", "-"); obj.find(&name[..]).and_then(|o| o.as_string().and_then(|s| { @@ -1064,6 +1126,7 @@ impl Target { key!(requires_uwtable, bool); key!(simd_types_indirect, bool); key!(override_export_symbols, opt_list); + key!(merge_functions, MergeFunctions)?; if let Some(array) = obj.find("abi-blacklist").and_then(Json::as_array) { for name in array.iter().filter_map(|abi| abi.as_string()) { @@ -1275,6 +1338,7 @@ impl ToJson for Target { target_option_val!(requires_uwtable); target_option_val!(simd_types_indirect); target_option_val!(override_export_symbols); + target_option_val!(merge_functions); if default.abi_blacklist != self.options.abi_blacklist { d.insert("abi-blacklist".to_string(), self.options.abi_blacklist.iter() From ebdd072e3b1d4b3eb39048c27be4e9b2ccc849a8 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Sun, 13 Jan 2019 00:04:47 +0300 Subject: [PATCH 2/8] resolve: Add a test for issue #57539 --- src/test/ui/imports/issue-57539.rs | 8 ++++++++ src/test/ui/imports/issue-57539.stderr | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 src/test/ui/imports/issue-57539.rs create mode 100644 src/test/ui/imports/issue-57539.stderr diff --git a/src/test/ui/imports/issue-57539.rs b/src/test/ui/imports/issue-57539.rs new file mode 100644 index 0000000000000..90b74eb464779 --- /dev/null +++ b/src/test/ui/imports/issue-57539.rs @@ -0,0 +1,8 @@ +// edition:2018 + +mod core { + use core; //~ ERROR `core` is ambiguous + use crate::*; +} + +fn main() {} diff --git a/src/test/ui/imports/issue-57539.stderr b/src/test/ui/imports/issue-57539.stderr new file mode 100644 index 0000000000000..3f745fd8204bf --- /dev/null +++ b/src/test/ui/imports/issue-57539.stderr @@ -0,0 +1,18 @@ +error[E0659]: `core` is ambiguous (name vs any other name during import resolution) + --> $DIR/issue-57539.rs:4:9 + | +LL | use core; //~ ERROR `core` is ambiguous + | ^^^^ ambiguous name + | + = note: `core` could refer to a built-in extern crate + = help: use `::core` to refer to this extern crate unambiguously +note: `core` could also refer to the module imported here + --> $DIR/issue-57539.rs:5:9 + | +LL | use crate::*; + | ^^^^^^^^ + = help: use `self::core` to refer to this module unambiguously + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0659`. From f4ded5b5591b3ec1446422dd9d62d404d4335753 Mon Sep 17 00:00:00 2001 From: Michael Bradshaw Date: Mon, 24 Dec 2018 16:07:10 -0700 Subject: [PATCH 3/8] Add a regression test for mutating a non-mut #[thread_local] --- src/test/ui/thread-local-mutation.nll.stderr | 9 +++++++++ src/test/ui/thread-local-mutation.rs | 18 ++++++++++++++++++ src/test/ui/thread-local-mutation.stderr | 9 +++++++++ 3 files changed, 36 insertions(+) create mode 100644 src/test/ui/thread-local-mutation.nll.stderr create mode 100644 src/test/ui/thread-local-mutation.rs create mode 100644 src/test/ui/thread-local-mutation.stderr diff --git a/src/test/ui/thread-local-mutation.nll.stderr b/src/test/ui/thread-local-mutation.nll.stderr new file mode 100644 index 0000000000000..0a3664b0d9d40 --- /dev/null +++ b/src/test/ui/thread-local-mutation.nll.stderr @@ -0,0 +1,9 @@ +error[E0594]: cannot assign to immutable static item `S` + --> $DIR/thread-local-mutation.rs:11:5 + | +LL | S = "after"; //~ ERROR cannot assign to immutable + | ^^^^^^^^^^^ cannot assign + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0594`. diff --git a/src/test/ui/thread-local-mutation.rs b/src/test/ui/thread-local-mutation.rs new file mode 100644 index 0000000000000..e738225ce2a48 --- /dev/null +++ b/src/test/ui/thread-local-mutation.rs @@ -0,0 +1,18 @@ +// Regression test for #54901: immutable thread locals could be mutated. See: +// https://github.com/rust-lang/rust/issues/29594#issuecomment-328177697 +// https://github.com/rust-lang/rust/issues/54901 + +#![feature(thread_local)] + +#[thread_local] +static S: &str = "before"; + +fn set_s() { + S = "after"; //~ ERROR cannot assign to immutable +} + +fn main() { + println!("{}", S); + set_s(); + println!("{}", S); +} diff --git a/src/test/ui/thread-local-mutation.stderr b/src/test/ui/thread-local-mutation.stderr new file mode 100644 index 0000000000000..bf298523e1b73 --- /dev/null +++ b/src/test/ui/thread-local-mutation.stderr @@ -0,0 +1,9 @@ +error[E0594]: cannot assign to immutable thread-local static item + --> $DIR/thread-local-mutation.rs:11:5 + | +LL | S = "after"; //~ ERROR cannot assign to immutable + | ^^^^^^^^^^^ + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0594`. From ee10d99b9a44c57f450678310b16e651d08075cd Mon Sep 17 00:00:00 2001 From: Andy Russell Date: Sat, 15 Dec 2018 12:56:45 -0500 Subject: [PATCH 4/8] generalize markdown to source span calculation --- .../passes/collect_intra_doc_links.rs | 63 +------------- src/librustdoc/passes/mod.rs | 83 +++++++++++++++++++ 2 files changed, 87 insertions(+), 59 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index fdc1c0616187a..56d6671266065 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -451,17 +451,9 @@ pub fn span_of_attrs(attrs: &Attributes) -> syntax_pos::Span { /// Reports a resolution failure diagnostic. /// -/// Ideally we can report the diagnostic with the actual span in the source where the link failure -/// occurred. However, there's a mismatch between the span in the source code and the span in the -/// markdown, so we have to do a bit of work to figure out the correspondence. -/// -/// It's not too hard to find the span for sugared doc comments (`///` and `/**`), because the -/// source will match the markdown exactly, excluding the comment markers. However, it's much more -/// difficult to calculate the spans for unsugared docs, because we have to deal with escaping and -/// other source features. So, we attempt to find the exact source span of the resolution failure -/// in sugared docs, but use the span of the documentation attributes themselves for unsugared -/// docs. Because this span might be overly large, we display the markdown line containing the -/// failure as a note. +/// If we cannot find the exact source span of the resolution failure, we use the span of the +/// documentation attributes themselves. This is a little heavy-handed, so we display the markdown +/// line containing the failure as a note as well. fn resolution_failure( cx: &DocContext, attrs: &Attributes, @@ -473,54 +465,7 @@ fn resolution_failure( let msg = format!("`[{}]` cannot be resolved, ignoring it...", path_str); let mut diag = if let Some(link_range) = link_range { - let src = cx.sess().source_map().span_to_snippet(sp); - let is_all_sugared_doc = attrs.doc_strings.iter().all(|frag| match frag { - DocFragment::SugaredDoc(..) => true, - _ => false, - }); - - if let (Ok(src), true) = (src, is_all_sugared_doc) { - // The number of markdown lines up to and including the resolution failure. - let num_lines = dox[..link_range.start].lines().count(); - - // We use `split_terminator('\n')` instead of `lines()` when counting bytes to ensure - // that DOS-style line endings do not cause the spans to be calculated incorrectly. - let mut src_lines = src.split_terminator('\n'); - let mut md_lines = dox.split_terminator('\n').take(num_lines).peekable(); - - // The number of bytes from the start of the source span to the resolution failure that - // are *not* part of the markdown, like comment markers. - let mut extra_src_bytes = 0; - - while let Some(md_line) = md_lines.next() { - loop { - let source_line = src_lines - .next() - .expect("could not find markdown line in source"); - - match source_line.find(md_line) { - Some(offset) => { - extra_src_bytes += if md_lines.peek().is_some() { - source_line.len() - md_line.len() - } else { - offset - }; - break; - } - None => { - // Since this is a source line that doesn't include a markdown line, - // we have to count the newline that we split from earlier. - extra_src_bytes += source_line.len() + 1; - } - } - } - } - - let sp = sp.from_inner_byte_pos( - link_range.start + extra_src_bytes, - link_range.end + extra_src_bytes, - ); - + if let Some(sp) = super::source_span_for_markdown_range(cx, dox, &link_range, attrs) { let mut diag = cx.tcx.struct_span_lint_node( lint::builtin::INTRA_DOC_LINK_RESOLUTION_FAILURE, NodeId::from_u32(0), diff --git a/src/librustdoc/passes/mod.rs b/src/librustdoc/passes/mod.rs index e897b9a7de58e..23581d0051166 100644 --- a/src/librustdoc/passes/mod.rs +++ b/src/librustdoc/passes/mod.rs @@ -8,6 +8,8 @@ use rustc::util::nodemap::DefIdSet; use std::mem; use std::fmt; use syntax::ast::NodeId; +use syntax_pos::Span; +use std::ops::Range; use clean::{self, GetDefId, Item}; use core::{DocContext, DocAccessLevels}; @@ -396,3 +398,84 @@ pub fn look_for_tests<'a, 'tcx: 'a, 'rcx: 'a>( } } } + +/// Attempts to match a range of bytes from parsed markdown to a `Span` in the source code. +/// +/// This method will return `None` if we cannot construct a span from the source map or if the +/// attributes are not all sugared doc comments. It's difficult to calculate the correct span in +/// that case due to escaping and other source features. +crate fn source_span_for_markdown_range( + cx: &DocContext, + markdown: &str, + md_range: &Range, + attrs: &clean::Attributes, +) -> Option { + let is_all_sugared_doc = attrs.doc_strings.iter().all(|frag| match frag { + clean::DocFragment::SugaredDoc(..) => true, + _ => false, + }); + + if !is_all_sugared_doc { + return None; + } + + let snippet = cx + .sess() + .source_map() + .span_to_snippet(span_of_attrs(attrs)) + .ok()?; + + let starting_line = markdown[..md_range.start].lines().count() - 1; + let ending_line = markdown[..md_range.end].lines().count() - 1; + + // We use `split_terminator('\n')` instead of `lines()` when counting bytes so that we only + // we can treat CRLF and LF line endings the same way. + let mut src_lines = snippet.split_terminator('\n'); + let md_lines = markdown.split_terminator('\n'); + + // The number of bytes from the source span to the markdown span that are not part + // of the markdown, like comment markers. + let mut start_bytes = 0; + let mut end_bytes = 0; + + 'outer: for (line_no, md_line) in md_lines.enumerate() { + loop { + let source_line = src_lines.next().expect("could not find markdown in source"); + match source_line.find(md_line) { + Some(offset) => { + if line_no == starting_line { + start_bytes += offset; + + if starting_line == ending_line { + break 'outer; + } + } else if line_no == ending_line { + end_bytes += offset; + break 'outer; + } else if line_no < starting_line { + start_bytes += source_line.len() - md_line.len(); + } else { + end_bytes += source_line.len() - md_line.len(); + } + break; + } + None => { + // Since this is a source line that doesn't include a markdown line, + // we have to count the newline that we split from earlier. + if line_no <= starting_line { + start_bytes += source_line.len() + 1; + } else { + end_bytes += source_line.len() + 1; + } + } + } + } + } + + let sp = span_of_attrs(attrs).from_inner_byte_pos( + md_range.start + start_bytes, + md_range.end + start_bytes + end_bytes, + ); + + Some(sp) +} From 8c93798e9f80d6d6ed9a2ab4b01af06a8fea23b7 Mon Sep 17 00:00:00 2001 From: Andy Russell Date: Sat, 15 Dec 2018 16:25:50 -0500 Subject: [PATCH 5/8] rustdoc: check code block syntax in early pass --- src/librustdoc/html/highlight.rs | 92 +++++++++------ src/librustdoc/html/markdown.rs | 109 ++++++++++++++++++ src/librustdoc/lib.rs | 1 + .../passes/check_code_block_syntax.rs | 109 ++++++++++++++++++ .../passes/collect_intra_doc_links.rs | 12 +- src/librustdoc/passes/mod.rs | 20 +++- src/libsyntax/parse/lexer/mod.rs | 13 --- src/test/rustdoc-ui/invalid-syntax.rs | 61 +++++++++- src/test/rustdoc-ui/invalid-syntax.stderr | 105 +++++++++++++++-- src/test/rustdoc/bad-codeblock-syntax.rs | 27 +++++ 10 files changed, 476 insertions(+), 73 deletions(-) create mode 100644 src/librustdoc/passes/check_code_block_syntax.rs create mode 100644 src/test/rustdoc/bad-codeblock-syntax.rs diff --git a/src/librustdoc/html/highlight.rs b/src/librustdoc/html/highlight.rs index 87e979b93e9ef..29a41384edcda 100644 --- a/src/librustdoc/html/highlight.rs +++ b/src/librustdoc/html/highlight.rs @@ -25,40 +25,51 @@ pub fn render_with_highlighting( tooltip: Option<(&str, &str)>, ) -> String { debug!("highlighting: ================\n{}\n==============", src); - let sess = parse::ParseSess::new(FilePathMapping::empty()); - let fm = sess.source_map().new_source_file(FileName::Custom("stdin".to_string()), - src.to_string()); - let mut out = Vec::new(); if let Some((tooltip, class)) = tooltip { write!(out, "
ⓘ{}
", class, tooltip).unwrap(); } - write_header(class, &mut out).unwrap(); - - let lexer = match lexer::StringReader::new_without_err(&sess, fm, None, "Output from rustc:") { - Ok(l) => l, - Err(_) => { - let first_line = src.lines().next().unwrap_or_else(|| ""); - let mut err = sess.span_diagnostic - .struct_warn(&format!("Invalid doc comment starting with: `{}`\n\ - (Ignoring this codeblock)", - first_line)); - err.emit(); - return String::new(); + + let sess = parse::ParseSess::new(FilePathMapping::empty()); + let fm = sess.source_map().new_source_file( + FileName::Custom(String::from("rustdoc-highlighting")), + src.to_owned(), + ); + let highlight_result = + lexer::StringReader::new_or_buffered_errs(&sess, fm, None).and_then(|lexer| { + let mut classifier = Classifier::new(lexer, sess.source_map()); + + let mut highlighted_source = vec![]; + if classifier.write_source(&mut highlighted_source).is_err() { + Err(classifier.lexer.buffer_fatal_errors()) + } else { + Ok(String::from_utf8_lossy(&highlighted_source).into_owned()) + } + }); + + match highlight_result { + Ok(highlighted_source) => { + write_header(class, &mut out).unwrap(); + write!(out, "{}", highlighted_source).unwrap(); + if let Some(extension) = extension { + write!(out, "{}", extension).unwrap(); + } + write_footer(&mut out).unwrap(); } - }; - let mut classifier = Classifier::new(lexer, sess.source_map()); - if classifier.write_source(&mut out).is_err() { - classifier.lexer.emit_fatal_errors(); - return format!("
{}
", src); - } + Err(errors) => { + // If errors are encountered while trying to highlight, cancel the errors and just emit + // the unhighlighted source. The errors will have already been reported in the + // `check-code-block-syntax` pass. + for mut error in errors { + error.cancel(); + } - if let Some(extension) = extension { - write!(out, "{}", extension).unwrap(); + write!(out, "
{}
", src).unwrap(); + } } - write_footer(&mut out).unwrap(); + String::from_utf8_lossy(&out[..]).into_owned() } @@ -151,6 +162,17 @@ impl Writer for U { } } +enum HighlightError { + LexError, + IoError(io::Error), +} + +impl From for HighlightError { + fn from(err: io::Error) -> Self { + HighlightError::IoError(err) + } +} + impl<'a> Classifier<'a> { fn new(lexer: lexer::StringReader<'a>, source_map: &'a SourceMap) -> Classifier<'a> { Classifier { @@ -162,17 +184,11 @@ impl<'a> Classifier<'a> { } } - /// Gets the next token out of the lexer, emitting fatal errors if lexing fails. - fn try_next_token(&mut self) -> io::Result { + /// Gets the next token out of the lexer. + fn try_next_token(&mut self) -> Result { match self.lexer.try_next_token() { Ok(tas) => Ok(tas), - Err(_) => { - let mut err = self.lexer.sess.span_diagnostic - .struct_warn("Backing out of syntax highlighting"); - err.note("You probably did not intend to render this as a rust code-block"); - err.emit(); - Err(io::Error::new(io::ErrorKind::Other, "")) - } + Err(_) => Err(HighlightError::LexError), } } @@ -185,7 +201,7 @@ impl<'a> Classifier<'a> { /// source. fn write_source(&mut self, out: &mut W) - -> io::Result<()> { + -> Result<(), HighlightError> { loop { let next = self.try_next_token()?; if next.tok == token::Eof { @@ -202,7 +218,7 @@ impl<'a> Classifier<'a> { fn write_token(&mut self, out: &mut W, tas: TokenAndSpan) - -> io::Result<()> { + -> Result<(), HighlightError> { let klass = match tas.tok { token::Shebang(s) => { out.string(Escape(&s.as_str()), Class::None)?; @@ -341,7 +357,9 @@ impl<'a> Classifier<'a> { // Anything that didn't return above is the simple case where we the // class just spans a single token, so we can use the `string` method. - out.string(Escape(&self.snip(tas.sp)), klass) + out.string(Escape(&self.snip(tas.sp)), klass)?; + + Ok(()) } // Helper function to get a snippet from the source_map. diff --git a/src/librustdoc/html/markdown.rs b/src/librustdoc/html/markdown.rs index 05a9a2d1312ae..6b7f54044ca1d 100644 --- a/src/librustdoc/html/markdown.rs +++ b/src/librustdoc/html/markdown.rs @@ -919,6 +919,115 @@ pub fn markdown_links(md: &str) -> Vec<(String, Option>)> { links } +#[derive(Debug)] +crate struct RustCodeBlock { + /// The range in the markdown that the code block occupies. Note that this includes the fences + /// for fenced code blocks. + pub range: Range, + /// The range in the markdown that the code within the code block occupies. + pub code: Range, + pub is_fenced: bool, + pub syntax: Option, +} + +/// Returns a range of bytes for each code block in the markdown that is tagged as `rust` or +/// untagged (and assumed to be rust). +crate fn rust_code_blocks(md: &str) -> Vec { + let mut code_blocks = vec![]; + + if md.is_empty() { + return code_blocks; + } + + let mut opts = Options::empty(); + opts.insert(OPTION_ENABLE_TABLES); + opts.insert(OPTION_ENABLE_FOOTNOTES); + let mut p = Parser::new_ext(md, opts); + + let mut code_block_start = 0; + let mut code_start = 0; + let mut is_fenced = false; + let mut previous_offset = 0; + let mut in_rust_code_block = false; + while let Some(event) = p.next() { + let offset = p.get_offset(); + + match event { + Event::Start(Tag::CodeBlock(syntax)) => { + let lang_string = if syntax.is_empty() { + LangString::all_false() + } else { + LangString::parse(&*syntax, ErrorCodes::Yes) + }; + + if lang_string.rust { + in_rust_code_block = true; + + code_start = offset; + code_block_start = match md[previous_offset..offset].find("```") { + Some(fence_idx) => { + is_fenced = true; + previous_offset + fence_idx + } + None => offset, + }; + } + } + Event::End(Tag::CodeBlock(syntax)) if in_rust_code_block => { + in_rust_code_block = false; + + let code_block_end = if is_fenced { + let fence_str = &md[previous_offset..offset] + .chars() + .rev() + .collect::(); + fence_str + .find("```") + .map(|fence_idx| offset - fence_idx) + .unwrap_or_else(|| offset) + } else if md + .as_bytes() + .get(offset) + .map(|b| *b == b'\n') + .unwrap_or_default() + { + offset - 1 + } else { + offset + }; + + let code_end = if is_fenced { + previous_offset + } else { + code_block_end + }; + + code_blocks.push(RustCodeBlock { + is_fenced, + range: Range { + start: code_block_start, + end: code_block_end, + }, + code: Range { + start: code_start, + end: code_end, + }, + syntax: if !syntax.is_empty() { + Some(syntax.into_owned()) + } else { + None + }, + }); + } + _ => (), + } + + previous_offset = offset; + } + + code_blocks +} + #[derive(Clone, Default, Debug)] pub struct IdMap { map: FxHashMap, diff --git a/src/librustdoc/lib.rs b/src/librustdoc/lib.rs index 1b6d7e87192d6..6e7141f94a776 100644 --- a/src/librustdoc/lib.rs +++ b/src/librustdoc/lib.rs @@ -3,6 +3,7 @@ html_root_url = "https://doc.rust-lang.org/nightly/", html_playground_url = "https://play.rust-lang.org/")] +#![feature(bind_by_move_pattern_guards)] #![feature(rustc_private)] #![feature(box_patterns)] #![feature(box_syntax)] diff --git a/src/librustdoc/passes/check_code_block_syntax.rs b/src/librustdoc/passes/check_code_block_syntax.rs new file mode 100644 index 0000000000000..a013cc36c722d --- /dev/null +++ b/src/librustdoc/passes/check_code_block_syntax.rs @@ -0,0 +1,109 @@ +use errors::Applicability; +use syntax::parse::lexer::{TokenAndSpan, StringReader as Lexer}; +use syntax::parse::{ParseSess, token}; +use syntax::source_map::FilePathMapping; +use syntax_pos::FileName; + +use clean; +use core::DocContext; +use fold::DocFolder; +use html::markdown::{self, RustCodeBlock}; +use passes::Pass; + +pub const CHECK_CODE_BLOCK_SYNTAX: Pass = + Pass::early("check-code-block-syntax", check_code_block_syntax, + "validates syntax inside Rust code blocks"); + +pub fn check_code_block_syntax(krate: clean::Crate, cx: &DocContext) -> clean::Crate { + SyntaxChecker { cx }.fold_crate(krate) +} + +struct SyntaxChecker<'a, 'tcx: 'a, 'rcx: 'a> { + cx: &'a DocContext<'a, 'tcx, 'rcx>, +} + +impl<'a, 'tcx, 'rcx> SyntaxChecker<'a, 'tcx, 'rcx> { + fn check_rust_syntax(&self, item: &clean::Item, dox: &str, code_block: RustCodeBlock) { + let sess = ParseSess::new(FilePathMapping::empty()); + let source_file = sess.source_map().new_source_file( + FileName::Custom(String::from("doctest")), + dox[code_block.code].to_owned(), + ); + + let errors = Lexer::new_or_buffered_errs(&sess, source_file, None).and_then(|mut lexer| { + while let Ok(TokenAndSpan { tok, .. }) = lexer.try_next_token() { + if tok == token::Eof { + break; + } + } + + let errors = lexer.buffer_fatal_errors(); + + if !errors.is_empty() { + Err(errors) + } else { + Ok(()) + } + }); + + if let Err(errors) = errors { + let mut diag = if let Some(sp) = + super::source_span_for_markdown_range(self.cx, &dox, &code_block.range, &item.attrs) + { + let mut diag = self + .cx + .sess() + .struct_span_warn(sp, "could not parse code block as Rust code"); + + for mut err in errors { + diag.note(&format!("error from rustc: {}", err.message())); + err.cancel(); + } + + if code_block.syntax.is_none() && code_block.is_fenced { + let sp = sp.from_inner_byte_pos(0, 3); + diag.span_suggestion_with_applicability( + sp, + "mark blocks that do not contain Rust code as text", + String::from("```text"), + Applicability::MachineApplicable, + ); + } + + diag + } else { + // We couldn't calculate the span of the markdown block that had the error, so our + // diagnostics are going to be a bit lacking. + let mut diag = self.cx.sess().struct_span_warn( + super::span_of_attrs(&item.attrs), + "doc comment contains an invalid Rust code block", + ); + + for mut err in errors { + // Don't bother reporting the error, because we can't show where it happened. + err.cancel(); + } + + if code_block.syntax.is_none() && code_block.is_fenced { + diag.help("mark blocks that do not contain Rust code as text: ```text"); + } + + diag + }; + + diag.emit(); + } + } +} + +impl<'a, 'tcx, 'rcx> DocFolder for SyntaxChecker<'a, 'tcx, 'rcx> { + fn fold_item(&mut self, item: clean::Item) -> Option { + if let Some(dox) = &item.attrs.collapsed_doc_value() { + for code_block in markdown::rust_code_blocks(&dox) { + self.check_rust_syntax(&item, &dox, code_block); + } + } + + self.fold_item_recur(item) + } +} diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 56d6671266065..3d6096b07ce43 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -6,7 +6,7 @@ use syntax; use syntax::ast::{self, Ident, NodeId}; use syntax::feature_gate::UnstableFeatures; use syntax::symbol::Symbol; -use syntax_pos::{self, DUMMY_SP}; +use syntax_pos::DUMMY_SP; use std::ops::Range; @@ -16,6 +16,7 @@ use html::markdown::markdown_links; use clean::*; use passes::{look_for_tests, Pass}; +use super::span_of_attrs; pub const COLLECT_INTRA_DOC_LINKS: Pass = Pass::early("collect-intra-doc-links", collect_intra_doc_links, @@ -440,15 +441,6 @@ fn macro_resolve(cx: &DocContext, path_str: &str) -> Option { None } -pub fn span_of_attrs(attrs: &Attributes) -> syntax_pos::Span { - if attrs.doc_strings.is_empty() { - return DUMMY_SP; - } - let start = attrs.doc_strings[0].span(); - let end = attrs.doc_strings.last().expect("No doc strings provided").span(); - start.to(end) -} - /// Reports a resolution failure diagnostic. /// /// If we cannot find the exact source span of the resolution failure, we use the span of the diff --git a/src/librustdoc/passes/mod.rs b/src/librustdoc/passes/mod.rs index 23581d0051166..c9a3a2c003fe0 100644 --- a/src/librustdoc/passes/mod.rs +++ b/src/librustdoc/passes/mod.rs @@ -8,7 +8,7 @@ use rustc::util::nodemap::DefIdSet; use std::mem; use std::fmt; use syntax::ast::NodeId; -use syntax_pos::Span; +use syntax_pos::{DUMMY_SP, Span}; use std::ops::Range; use clean::{self, GetDefId, Item}; @@ -18,8 +18,6 @@ use fold::StripItem; use html::markdown::{find_testable_code, ErrorCodes, LangString}; -use self::collect_intra_doc_links::span_of_attrs; - mod collapse_docs; pub use self::collapse_docs::COLLAPSE_DOCS; @@ -47,6 +45,9 @@ pub use self::private_items_doc_tests::CHECK_PRIVATE_ITEMS_DOC_TESTS; mod collect_trait_impls; pub use self::collect_trait_impls::COLLECT_TRAIT_IMPLS; +mod check_code_block_syntax; +pub use self::check_code_block_syntax::CHECK_CODE_BLOCK_SYNTAX; + /// Represents a single pass. #[derive(Copy, Clone)] pub enum Pass { @@ -137,6 +138,7 @@ pub const PASSES: &'static [Pass] = &[ STRIP_PRIV_IMPORTS, PROPAGATE_DOC_CFG, COLLECT_INTRA_DOC_LINKS, + CHECK_CODE_BLOCK_SYNTAX, COLLECT_TRAIT_IMPLS, ]; @@ -147,6 +149,7 @@ pub const DEFAULT_PASSES: &'static [&'static str] = &[ "strip-hidden", "strip-private", "collect-intra-doc-links", + "check-code-block-syntax", "collapse-docs", "unindent-comments", "propagate-doc-cfg", @@ -158,6 +161,7 @@ pub const DEFAULT_PRIVATE_PASSES: &'static [&'static str] = &[ "check-private-items-doc-tests", "strip-priv-imports", "collect-intra-doc-links", + "check-code-block-syntax", "collapse-docs", "unindent-comments", "propagate-doc-cfg", @@ -399,6 +403,16 @@ pub fn look_for_tests<'a, 'tcx: 'a, 'rcx: 'a>( } } +/// Return a span encompassing all the given attributes. +crate fn span_of_attrs(attrs: &clean::Attributes) -> Span { + if attrs.doc_strings.is_empty() { + return DUMMY_SP; + } + let start = attrs.doc_strings[0].span(); + let end = attrs.doc_strings.last().expect("No doc strings provided").span(); + start.to(end) +} + /// Attempts to match a range of bytes from parsed markdown to a `Span` in the source code. /// /// This method will return `None` if we cannot construct a span from the source map or if the diff --git a/src/libsyntax/parse/lexer/mod.rs b/src/libsyntax/parse/lexer/mod.rs index ecb34e43c590c..53650bd55aea3 100644 --- a/src/libsyntax/parse/lexer/mod.rs +++ b/src/libsyntax/parse/lexer/mod.rs @@ -238,19 +238,6 @@ impl<'a> StringReader<'a> { sr } - pub fn new_without_err(sess: &'a ParseSess, - source_file: Lrc, - override_span: Option, - prepend_error_text: &str) -> Result { - let mut sr = StringReader::new_raw(sess, source_file, override_span); - if sr.advance_token().is_err() { - eprintln!("{}", prepend_error_text); - sr.emit_fatal_errors(); - return Err(()); - } - Ok(sr) - } - pub fn new_or_buffered_errs(sess: &'a ParseSess, source_file: Lrc, override_span: Option) -> Result> { diff --git a/src/test/rustdoc-ui/invalid-syntax.rs b/src/test/rustdoc-ui/invalid-syntax.rs index 537816be8d735..924e0386d3191 100644 --- a/src/test/rustdoc-ui/invalid-syntax.rs +++ b/src/test/rustdoc-ui/invalid-syntax.rs @@ -1,7 +1,66 @@ // compile-pass -// compile-flags: --error-format=human /// ``` /// \__________pkt->size___________/ \_result->size_/ \__pkt->size__/ /// ``` pub fn foo() {} + +/// ``` +/// | +/// LL | use foobar::Baz; +/// | ^^^^^^ did you mean `baz::foobar`? +/// ``` +pub fn bar() {} + +/// ``` +/// valid +/// ``` +/// +/// ``` +/// \_ +/// ``` +/// +/// ```text +/// "invalid +/// ``` +pub fn valid_and_invalid() {} + +/// This is a normal doc comment, but... +/// +/// There's a code block with bad syntax in it: +/// +/// ```rust +/// \_ +/// ``` +/// +/// Good thing we tested it! +pub fn baz() {} + +/// Indented block start +/// +/// code with bad syntax +/// \_ +/// +/// Indented block end +pub fn quux() {} + +/// Unclosed fence +/// +/// ``` +/// slkdjf +pub fn xyzzy() {} + +/// Indented code that contains a fence +/// +/// ``` +pub fn blah() {} + +/// ```edition2018 +/// \_ +/// ``` +pub fn blargh() {} + +#[doc = "```"] +/// \_ +#[doc = "```"] +pub fn crazy_attrs() {} diff --git a/src/test/rustdoc-ui/invalid-syntax.stderr b/src/test/rustdoc-ui/invalid-syntax.stderr index b5661332e8d0e..10800380a05d3 100644 --- a/src/test/rustdoc-ui/invalid-syntax.stderr +++ b/src/test/rustdoc-ui/invalid-syntax.stderr @@ -1,10 +1,97 @@ -Output from rustc: -error: unknown start of token: / - --> :1:1 - | -1 | /__________pkt->size___________/ /_result->size_/ /__pkt->size__/ - | ^ - -warning: Invalid doc comment starting with: `/__________pkt->size___________/ /_result->size_/ /__pkt->size__/` -(Ignoring this codeblock) +warning: could not parse code block as Rust code + --> $DIR/invalid-syntax.rs:3:5 + | +LL | /// ``` + | _____^ +LL | | /// /__________pkt->size___________/ /_result->size_/ /__pkt->size__/ +LL | | /// ``` + | |_______^ + | + = note: error from rustc: unknown start of token: / +help: mark blocks that do not contain Rust code as text + | +LL | /// ```text + | ^^^^^^^ + +warning: could not parse code block as Rust code + --> $DIR/invalid-syntax.rs:8:5 + | +LL | /// ``` + | _____^ +LL | | /// | +LL | | /// LL | use foobar::Baz; +LL | | /// | ^^^^^^ did you mean `baz::foobar`? +LL | | /// ``` + | |_______^ + | + = note: error from rustc: unknown start of token: ` +help: mark blocks that do not contain Rust code as text + | +LL | /// ```text + | ^^^^^^^ + +warning: could not parse code block as Rust code + --> $DIR/invalid-syntax.rs:19:5 + | +LL | /// ``` + | _____^ +LL | | /// /_ +LL | | /// ``` + | |_______^ + | + = note: error from rustc: unknown start of token: / +help: mark blocks that do not contain Rust code as text + | +LL | /// ```text + | ^^^^^^^ + +warning: could not parse code block as Rust code + --> $DIR/invalid-syntax.rs:32:5 + | +LL | /// ```rust + | _____^ +LL | | /// /_ +LL | | /// ``` + | |_______^ + | + = note: error from rustc: unknown start of token: / + +warning: could not parse code block as Rust code + --> $DIR/invalid-syntax.rs:41:9 + | +LL | /// code with bad syntax + | _________^ +LL | | /// /_ + | |__________^ + | + = note: error from rustc: unknown start of token: / + +warning: could not parse code block as Rust code + --> $DIR/invalid-syntax.rs:55:9 + | +LL | /// ``` + | ^^^ + | + = note: error from rustc: unknown start of token: ` + +warning: could not parse code block as Rust code + --> $DIR/invalid-syntax.rs:58:5 + | +LL | /// ```edition2018 + | _____^ +LL | | /// /_ +LL | | /// ``` + | |_______^ + | + = note: error from rustc: unknown start of token: / + +warning: doc comment contains an invalid Rust code block + --> $DIR/invalid-syntax.rs:63:1 + | +LL | / #[doc = "```"] +LL | | /// /_ +LL | | #[doc = "```"] + | |______________^ + | + = help: mark blocks that do not contain Rust code as text: ```text diff --git a/src/test/rustdoc/bad-codeblock-syntax.rs b/src/test/rustdoc/bad-codeblock-syntax.rs new file mode 100644 index 0000000000000..0ab2f68fcdebe --- /dev/null +++ b/src/test/rustdoc/bad-codeblock-syntax.rs @@ -0,0 +1,27 @@ +// @has bad_codeblock_syntax/fn.foo.html +// @has - '//*[@class="docblock"]/pre/code' '\_' +/// ``` +/// \_ +/// ``` +pub fn foo() {} + +// @has bad_codeblock_syntax/fn.bar.html +// @has - '//*[@class="docblock"]/pre/code' '`baz::foobar`' +/// ``` +/// `baz::foobar` +/// ``` +pub fn bar() {} + +// @has bad_codeblock_syntax/fn.quux.html +// @has - '//*[@class="docblock"]/pre/code' '\_' +/// ```rust +/// \_ +/// ``` +pub fn quux() {} + +// @has bad_codeblock_syntax/fn.ok.html +// @has - '//*[@class="docblock"]/pre/code[@class="language-text"]' '\_' +/// ```text +/// \_ +/// ``` +pub fn ok() {} From 260fb31fd5d4d90d6694570c8a1964d7e1c5f81b Mon Sep 17 00:00:00 2001 From: Hirokazu Hata Date: Tue, 15 Jan 2019 00:09:21 +0900 Subject: [PATCH 6/8] Add missing unpretty option help message --- src/librustc/session/config.rs | 9 +++++++-- src/librustc_driver/pretty.rs | 3 ++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/librustc/session/config.rs b/src/librustc/session/config.rs index 42adc6a87fdba..03dbd7b54e1e3 100644 --- a/src/librustc/session/config.rs +++ b/src/librustc/session/config.rs @@ -1342,10 +1342,15 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, unpretty: Option = (None, parse_unpretty, [UNTRACKED], "Present the input source, unstable (and less-pretty) variants; valid types are any of the types for `--pretty`, as well as: + `expanded`, `expanded,identified`, + `expanded,hygiene` (with internal representations), `flowgraph=` (graphviz formatted flowgraph for node), + `flowgraph,unlabelled=` (unlabelled graphviz formatted flowgraph for node), `everybody_loops` (all function bodies replaced with `loop {}`), - `hir` (the HIR), `hir,identified`, or - `hir,typed` (HIR with types for each node)."), + `hir` (the HIR), `hir,identified`, + `hir,typed` (HIR with types for each node), + `hir-tree` (dump the raw HIR), + `mir` (the MIR), or `mir-cfg` (graphviz formatted MIR)"), run_dsymutil: Option = (None, parse_opt_bool, [TRACKED], "run `dsymutil` and delete intermediate object files"), ui_testing: bool = (false, parse_bool, [UNTRACKED], diff --git a/src/librustc_driver/pretty.rs b/src/librustc_driver/pretty.rs index a9ec99358c1b2..dfd7a14a86435 100644 --- a/src/librustc_driver/pretty.rs +++ b/src/librustc_driver/pretty.rs @@ -123,7 +123,8 @@ pub fn parse_pretty(sess: &Session, sess.fatal(&format!("argument to `unpretty` must be one of `normal`, \ `expanded`, `flowgraph[,unlabelled]=`, \ `identified`, `expanded,identified`, `everybody_loops`, \ - `hir`, `hir,identified`, `hir,typed`, or `mir`; got {}", + `hir`, `hir,identified`, `hir,typed`, `hir-tree`, \ + `mir` or `mir-cfg`; got {}", name)); } else { sess.fatal(&format!("argument to `pretty` must be one of `normal`, `expanded`, \ From 8ef7413f23eca3921aa3ca1d0ebf72abd4a9eef0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Sat, 22 Dec 2018 18:03:40 +0100 Subject: [PATCH 7/8] Optimize try_mark_green and eliminate the lock on dep node colors --- src/librustc/dep_graph/graph.rs | 165 ++++++++++++++++----------- src/librustc/dep_graph/prev.rs | 13 +-- src/librustc/ty/query/plumbing.rs | 49 ++------ src/librustc_data_structures/sync.rs | 6 +- 4 files changed, 117 insertions(+), 116 deletions(-) diff --git a/src/librustc/dep_graph/graph.rs b/src/librustc/dep_graph/graph.rs index 501ef01d74c6e..f8e426088dee5 100644 --- a/src/librustc/dep_graph/graph.rs +++ b/src/librustc/dep_graph/graph.rs @@ -3,7 +3,7 @@ use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_data_structures::indexed_vec::{Idx, IndexVec}; use smallvec::SmallVec; -use rustc_data_structures::sync::{Lrc, Lock}; +use rustc_data_structures::sync::{Lrc, Lock, AtomicU32, Ordering}; use std::env; use std::hash::Hash; use std::collections::hash_map::Entry; @@ -58,7 +58,7 @@ struct DepGraphData { /// nodes and edges as well as all fingerprints of nodes that have them. previous: PreviousDepGraph, - colors: Lock, + colors: DepNodeColorMap, /// When we load, there may be `.o` files, cached mir, or other such /// things available to us. If we find that they are not dirty, we @@ -84,7 +84,7 @@ impl DepGraph { dep_node_debug: Default::default(), current: Lock::new(CurrentDepGraph::new(prev_graph_node_count)), previous: prev_graph, - colors: Lock::new(DepNodeColorMap::new(prev_graph_node_count)), + colors: DepNodeColorMap::new(prev_graph_node_count), loaded_from_cache: Default::default(), })), } @@ -282,12 +282,11 @@ impl DepGraph { DepNodeColor::Red }; - let mut colors = data.colors.borrow_mut(); - debug_assert!(colors.get(prev_index).is_none(), + debug_assert!(data.colors.get(prev_index).is_none(), "DepGraph::with_task() - Duplicate DepNodeColor \ insertion for {:?}", key); - colors.insert(prev_index, color); + data.colors.insert(prev_index, color); } (result, dep_node_index) @@ -502,7 +501,7 @@ impl DepGraph { pub fn node_color(&self, dep_node: &DepNode) -> Option { if let Some(ref data) = self.data { if let Some(prev_index) = data.previous.node_to_index_opt(dep_node) { - return data.colors.borrow().get(prev_index) + return data.colors.get(prev_index) } else { // This is a node that did not exist in the previous compilation // session, so we consider it to be red. @@ -513,56 +512,86 @@ impl DepGraph { None } - pub fn try_mark_green<'tcx>(&self, - tcx: TyCtxt<'_, 'tcx, 'tcx>, - dep_node: &DepNode) - -> Option { - debug!("try_mark_green({:?}) - BEGIN", dep_node); - let data = self.data.as_ref().unwrap(); + /// Try to read a node index for the node dep_node. + /// A node will have an index, when it's already been marked green, or when we can mark it + /// green. This function will mark the current task as a reader of the specified node, when + /// a node index can be found for that node. + pub fn try_mark_green_and_read( + &self, + tcx: TyCtxt<'_, '_, '_>, + dep_node: &DepNode + ) -> Option<(SerializedDepNodeIndex, DepNodeIndex)> { + self.try_mark_green(tcx, dep_node).map(|(prev_index, dep_node_index)| { + debug_assert!(self.is_green(&dep_node)); + self.read_index(dep_node_index); + (prev_index, dep_node_index) + }) + } - #[cfg(not(parallel_queries))] - debug_assert!(!data.current.borrow().node_to_node_index.contains_key(dep_node)); - - if dep_node.kind.is_input() { - // We should only hit try_mark_green() for inputs that do not exist - // anymore in the current compilation session. Existing inputs are - // eagerly marked as either red/green before any queries are - // executed. - debug_assert!(dep_node.extract_def_id(tcx).is_none()); - debug!("try_mark_green({:?}) - END - DepNode is deleted input", dep_node); - return None; - } + pub fn try_mark_green( + &self, + tcx: TyCtxt<'_, '_, '_>, + dep_node: &DepNode + ) -> Option<(SerializedDepNodeIndex, DepNodeIndex)> { + debug_assert!(!dep_node.kind.is_input()); - let (prev_deps, prev_dep_node_index) = match data.previous.edges_from(dep_node) { - Some(prev) => { - // This DepNode and the corresponding query invocation existed - // in the previous compilation session too, so we can try to - // mark it as green by recursively marking all of its - // dependencies green. - prev - } + // Return None if the dep graph is disabled + let data = self.data.as_ref()?; + + // Return None if the dep node didn't exist in the previous session + let prev_index = data.previous.node_to_index_opt(dep_node)?; + + match data.colors.get(prev_index) { + Some(DepNodeColor::Green(dep_node_index)) => Some((prev_index, dep_node_index)), + Some(DepNodeColor::Red) => None, None => { - // This DepNode did not exist in the previous compilation session, - // so we cannot mark it as green. - debug!("try_mark_green({:?}) - END - DepNode does not exist in \ - current compilation session anymore", dep_node); - return None + self.try_mark_previous_green( + tcx.global_tcx(), + data, + prev_index, + &dep_node + ).map(|dep_node_index| { + (prev_index, dep_node_index) + }) } - }; + } + } - debug_assert!(data.colors.borrow().get(prev_dep_node_index).is_none()); + fn try_mark_previous_green<'tcx>( + &self, + tcx: TyCtxt<'_, 'tcx, 'tcx>, + data: &DepGraphData, + prev_dep_node_index: SerializedDepNodeIndex, + dep_node: &DepNode + ) -> Option { + debug!("try_mark_previous_green({:?}) - BEGIN", dep_node); + + #[cfg(not(parallel_queries))] + { + debug_assert!(!data.current.borrow().node_to_node_index.contains_key(dep_node)); + debug_assert!(data.colors.get(prev_dep_node_index).is_none()); + } + + debug_assert!(!dep_node.kind.is_input()); + debug_assert_eq!(data.previous.index_to_node(prev_dep_node_index), *dep_node); + + // We never try to mark inputs as green + // FIXME: Make an debug_assert! + assert!(!dep_node.kind.is_input()); + + let prev_deps = data.previous.edge_targets_from(prev_dep_node_index); let mut current_deps = SmallVec::new(); for &dep_dep_node_index in prev_deps { - let dep_dep_node_color = data.colors.borrow().get(dep_dep_node_index); + let dep_dep_node_color = data.colors.get(dep_dep_node_index); match dep_dep_node_color { Some(DepNodeColor::Green(node_index)) => { // This dependency has been marked as green before, we are // still fine and can continue with checking the other // dependencies. - debug!("try_mark_green({:?}) --- found dependency {:?} to \ + debug!("try_mark_previous_green({:?}) --- found dependency {:?} to \ be immediately green", dep_node, data.previous.index_to_node(dep_dep_node_index)); @@ -573,7 +602,7 @@ impl DepGraph { // compared to the previous compilation session. We cannot // mark the DepNode as green and also don't need to bother // with checking any of the other dependencies. - debug!("try_mark_green({:?}) - END - dependency {:?} was \ + debug!("try_mark_previous_green({:?}) - END - dependency {:?} was \ immediately red", dep_node, data.previous.index_to_node(dep_dep_node_index)); @@ -585,12 +614,18 @@ impl DepGraph { // We don't know the state of this dependency. If it isn't // an input node, let's try to mark it green recursively. if !dep_dep_node.kind.is_input() { - debug!("try_mark_green({:?}) --- state of dependency {:?} \ + debug!("try_mark_previous_green({:?}) --- state of dependency {:?} \ is unknown, trying to mark it green", dep_node, dep_dep_node); - if let Some(node_index) = self.try_mark_green(tcx, dep_dep_node) { - debug!("try_mark_green({:?}) --- managed to MARK \ + let node_index = self.try_mark_previous_green( + tcx, + data, + dep_dep_node_index, + dep_dep_node + ); + if let Some(node_index) = node_index { + debug!("try_mark_previous_green({:?}) --- managed to MARK \ dependency {:?} as green", dep_node, dep_dep_node); current_deps.push(node_index); continue; @@ -620,20 +655,20 @@ impl DepGraph { } // We failed to mark it green, so we try to force the query. - debug!("try_mark_green({:?}) --- trying to force \ + debug!("try_mark_previous_green({:?}) --- trying to force \ dependency {:?}", dep_node, dep_dep_node); if ::ty::query::force_from_dep_node(tcx, dep_dep_node) { - let dep_dep_node_color = data.colors.borrow().get(dep_dep_node_index); + let dep_dep_node_color = data.colors.get(dep_dep_node_index); match dep_dep_node_color { Some(DepNodeColor::Green(node_index)) => { - debug!("try_mark_green({:?}) --- managed to \ + debug!("try_mark_previous_green({:?}) --- managed to \ FORCE dependency {:?} to green", dep_node, dep_dep_node); current_deps.push(node_index); } Some(DepNodeColor::Red) => { - debug!("try_mark_green({:?}) - END - \ + debug!("try_mark_previous_green({:?}) - END - \ dependency {:?} was red after forcing", dep_node, dep_dep_node); @@ -641,7 +676,7 @@ impl DepGraph { } None => { if !tcx.sess.has_errors() { - bug!("try_mark_green() - Forcing the DepNode \ + bug!("try_mark_previous_green() - Forcing the DepNode \ should have set its color") } else { // If the query we just forced has resulted @@ -653,7 +688,7 @@ impl DepGraph { } } else { // The DepNode could not be forced. - debug!("try_mark_green({:?}) - END - dependency {:?} \ + debug!("try_mark_previous_green({:?}) - END - dependency {:?} \ could not be forced", dep_node, dep_dep_node); return None } @@ -705,16 +740,15 @@ impl DepGraph { } // ... and finally storing a "Green" entry in the color map. - let mut colors = data.colors.borrow_mut(); // Multiple threads can all write the same color here #[cfg(not(parallel_queries))] - debug_assert!(colors.get(prev_dep_node_index).is_none(), - "DepGraph::try_mark_green() - Duplicate DepNodeColor \ + debug_assert!(data.colors.get(prev_dep_node_index).is_none(), + "DepGraph::try_mark_previous_green() - Duplicate DepNodeColor \ insertion for {:?}", dep_node); - colors.insert(prev_dep_node_index, DepNodeColor::Green(dep_node_index)); + data.colors.insert(prev_dep_node_index, DepNodeColor::Green(dep_node_index)); - debug!("try_mark_green({:?}) - END - successfully marked as green", dep_node); + debug!("try_mark_previous_green({:?}) - END - successfully marked as green", dep_node); Some(dep_node_index) } @@ -735,9 +769,8 @@ impl DepGraph { pub fn exec_cache_promotions<'a, 'tcx>(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>) { let green_nodes: Vec = { let data = self.data.as_ref().unwrap(); - let colors = data.colors.borrow(); - colors.values.indices().filter_map(|prev_index| { - match colors.get(prev_index) { + data.colors.values.indices().filter_map(|prev_index| { + match data.colors.get(prev_index) { Some(DepNodeColor::Green(_)) => { let dep_node = data.previous.index_to_node(prev_index); if dep_node.cache_on_disk(tcx) { @@ -1035,7 +1068,7 @@ pub struct TaskDeps { // A data structure that stores Option values as a contiguous // array, using one u32 per entry. struct DepNodeColorMap { - values: IndexVec, + values: IndexVec, } const COMPRESSED_NONE: u32 = 0; @@ -1045,12 +1078,12 @@ const COMPRESSED_FIRST_GREEN: u32 = 2; impl DepNodeColorMap { fn new(size: usize) -> DepNodeColorMap { DepNodeColorMap { - values: IndexVec::from_elem_n(COMPRESSED_NONE, size) + values: (0..size).map(|_| AtomicU32::new(COMPRESSED_NONE)).collect(), } } fn get(&self, index: SerializedDepNodeIndex) -> Option { - match self.values[index] { + match self.values[index].load(Ordering::Acquire) { COMPRESSED_NONE => None, COMPRESSED_RED => Some(DepNodeColor::Red), value => Some(DepNodeColor::Green(DepNodeIndex::from_u32( @@ -1059,10 +1092,10 @@ impl DepNodeColorMap { } } - fn insert(&mut self, index: SerializedDepNodeIndex, color: DepNodeColor) { - self.values[index] = match color { + fn insert(&self, index: SerializedDepNodeIndex, color: DepNodeColor) { + self.values[index].store(match color { DepNodeColor::Red => COMPRESSED_RED, DepNodeColor::Green(index) => index.as_u32() + COMPRESSED_FIRST_GREEN, - } + }, Ordering::Release) } } diff --git a/src/librustc/dep_graph/prev.rs b/src/librustc/dep_graph/prev.rs index 76d2954b4e35c..ea5350ac97fee 100644 --- a/src/librustc/dep_graph/prev.rs +++ b/src/librustc/dep_graph/prev.rs @@ -19,14 +19,11 @@ impl PreviousDepGraph { } #[inline] - pub fn edges_from(&self, - dep_node: &DepNode) - -> Option<(&[SerializedDepNodeIndex], SerializedDepNodeIndex)> { - self.index - .get(dep_node) - .map(|&node_index| { - (self.data.edge_targets_from(node_index), node_index) - }) + pub fn edge_targets_from( + &self, + dep_node_index: SerializedDepNodeIndex + ) -> &[SerializedDepNodeIndex] { + self.data.edge_targets_from(dep_node_index) } #[inline] diff --git a/src/librustc/ty/query/plumbing.rs b/src/librustc/ty/query/plumbing.rs index 562cd75a75ff4..5d827e07c5997 100644 --- a/src/librustc/ty/query/plumbing.rs +++ b/src/librustc/ty/query/plumbing.rs @@ -2,7 +2,7 @@ //! that generate the actual methods on tcx which find and execute the //! provider, manage the caches, and so forth. -use dep_graph::{DepNodeIndex, DepNode, DepKind, DepNodeColor}; +use dep_graph::{DepNodeIndex, DepNode, DepKind, SerializedDepNodeIndex}; use errors::DiagnosticBuilder; use errors::Level; use errors::Diagnostic; @@ -335,40 +335,6 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { eprintln!("end of query stack"); } - /// Try to read a node index for the node dep_node. - /// A node will have an index, when it's already been marked green, or when we can mark it - /// green. This function will mark the current task as a reader of the specified node, when - /// a node index can be found for that node. - pub(super) fn try_mark_green_and_read(self, dep_node: &DepNode) -> Option { - match self.dep_graph.node_color(dep_node) { - Some(DepNodeColor::Green(dep_node_index)) => { - self.dep_graph.read_index(dep_node_index); - Some(dep_node_index) - } - Some(DepNodeColor::Red) => { - None - } - None => { - // try_mark_green (called below) will panic when full incremental - // compilation is disabled. If that's the case, we can't try to mark nodes - // as green anyway, so we can safely return None here. - if !self.dep_graph.is_fully_enabled() { - return None; - } - match self.dep_graph.try_mark_green(self.global_tcx(), &dep_node) { - Some(dep_node_index) => { - debug_assert!(self.dep_graph.is_green(&dep_node)); - self.dep_graph.read_index(dep_node_index); - Some(dep_node_index) - } - None => { - None - } - } - } - } - } - #[inline(never)] fn try_get_with>( self, @@ -435,10 +401,13 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { } if !dep_node.kind.is_input() { - if let Some(dep_node_index) = self.try_mark_green_and_read(&dep_node) { + if let Some((prev_dep_node_index, + dep_node_index)) = self.dep_graph.try_mark_green_and_read(self, + &dep_node) { return Ok(self.load_from_disk_and_cache_in_memory::( key, job, + prev_dep_node_index, dep_node_index, &dep_node )) @@ -454,6 +423,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { self, key: Q::Key, job: JobOwner<'a, 'gcx, Q>, + prev_dep_node_index: SerializedDepNodeIndex, dep_node_index: DepNodeIndex, dep_node: &DepNode ) -> Q::Value @@ -466,10 +436,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { // First we try to load the result from the on-disk cache let result = if Q::cache_on_disk(key.clone()) && self.sess.opts.debugging_opts.incremental_queries { - let prev_dep_node_index = - self.dep_graph.prev_dep_node_index_of(dep_node); - let result = Q::try_load_from_disk(self.global_tcx(), - prev_dep_node_index); + let result = Q::try_load_from_disk(self.global_tcx(), prev_dep_node_index); // We always expect to find a cached result for things that // can be forced from DepNode. @@ -624,7 +591,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { // Ensuring an "input" or anonymous query makes no sense assert!(!dep_node.kind.is_anon()); assert!(!dep_node.kind.is_input()); - if self.try_mark_green_and_read(&dep_node).is_none() { + if self.dep_graph.try_mark_green_and_read(self, &dep_node).is_none() { // A None return from `try_mark_green_and_read` means that this is either // a new dep node or that the dep node has already been marked red. // Either way, we can't call `dep_graph.read()` as we don't have the diff --git a/src/librustc_data_structures/sync.rs b/src/librustc_data_structures/sync.rs index f9f94f0be7b9a..0253eef4dfa3a 100644 --- a/src/librustc_data_structures/sync.rs +++ b/src/librustc_data_structures/sync.rs @@ -70,6 +70,7 @@ cfg_if! { pub struct Atomic(Cell); impl Atomic { + #[inline] pub fn new(v: T) -> Self { Atomic(Cell::new(v)) } @@ -80,10 +81,12 @@ cfg_if! { self.0.into_inner() } + #[inline] pub fn load(&self, _: Ordering) -> T { self.0.get() } + #[inline] pub fn store(&self, val: T, _: Ordering) { self.0.set(val) } @@ -118,6 +121,7 @@ cfg_if! { pub type AtomicUsize = Atomic; pub type AtomicBool = Atomic; + pub type AtomicU32 = Atomic; pub type AtomicU64 = Atomic; pub use self::serial_join as join; @@ -223,7 +227,7 @@ cfg_if! { pub use parking_lot::MutexGuard as LockGuard; pub use parking_lot::MappedMutexGuard as MappedLockGuard; - pub use std::sync::atomic::{AtomicBool, AtomicUsize, AtomicU64}; + pub use std::sync::atomic::{AtomicBool, AtomicUsize, AtomicU32, AtomicU64}; pub use std::sync::Arc as Lrc; pub use std::sync::Weak as Weak; From 13136787849c960db824b2ad15d227dc15fadd70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Tue, 15 Jan 2019 10:39:35 +0100 Subject: [PATCH 8/8] Address comments --- src/librustc/dep_graph/graph.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/librustc/dep_graph/graph.rs b/src/librustc/dep_graph/graph.rs index f8e426088dee5..0a0dff05cb1ab 100644 --- a/src/librustc/dep_graph/graph.rs +++ b/src/librustc/dep_graph/graph.rs @@ -545,6 +545,10 @@ impl DepGraph { Some(DepNodeColor::Green(dep_node_index)) => Some((prev_index, dep_node_index)), Some(DepNodeColor::Red) => None, None => { + // This DepNode and the corresponding query invocation existed + // in the previous compilation session too, so we can try to + // mark it as green by recursively marking all of its + // dependencies green. self.try_mark_previous_green( tcx.global_tcx(), data, @@ -557,6 +561,7 @@ impl DepGraph { } } + /// Try to mark a dep-node which existed in the previous compilation session as green fn try_mark_previous_green<'tcx>( &self, tcx: TyCtxt<'_, 'tcx, 'tcx>, @@ -572,12 +577,10 @@ impl DepGraph { debug_assert!(data.colors.get(prev_dep_node_index).is_none()); } + // We never try to mark inputs as green debug_assert!(!dep_node.kind.is_input()); - debug_assert_eq!(data.previous.index_to_node(prev_dep_node_index), *dep_node); - // We never try to mark inputs as green - // FIXME: Make an debug_assert! - assert!(!dep_node.kind.is_input()); + debug_assert_eq!(data.previous.index_to_node(prev_dep_node_index), *dep_node); let prev_deps = data.previous.edge_targets_from(prev_dep_node_index);