From fb7018b41e66aa2edfaf2d4fbb8fe260cb411ac2 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Thu, 15 Apr 2021 23:06:32 -0400 Subject: [PATCH 1/5] Test that non_default_option is not the default option Otherwise the test is useless and does nothing. This caught 2 bugs in the test suite. --- compiler/rustc_interface/src/tests.rs | 8 ++++++-- compiler/rustc_session/src/config.rs | 6 +++--- compiler/rustc_session/src/options.rs | 2 +- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_interface/src/tests.rs b/compiler/rustc_interface/src/tests.rs index 9685d21762b..5800fa173dd 100644 --- a/compiler/rustc_interface/src/tests.rs +++ b/compiler/rustc_interface/src/tests.rs @@ -391,6 +391,7 @@ fn test_codegen_options_tracking_hash() { macro_rules! untracked { ($name: ident, $non_default_value: expr) => { + assert_ne!(opts.cg.$name, $non_default_value); opts.cg.$name = $non_default_value; assert_eq!(reference.dep_tracking_hash(), opts.dep_tracking_hash()); }; @@ -416,6 +417,7 @@ fn test_codegen_options_tracking_hash() { macro_rules! tracked { ($name: ident, $non_default_value: expr) => { opts = reference.clone(); + assert_ne!(opts.cg.$name, $non_default_value); opts.cg.$name = $non_default_value; assert_ne!(reference.dep_tracking_hash(), opts.dep_tracking_hash()); }; @@ -461,6 +463,7 @@ fn test_debugging_options_tracking_hash() { macro_rules! untracked { ($name: ident, $non_default_value: expr) => { + assert_ne!(opts.debugging_opts.$name, $non_default_value); opts.debugging_opts.$name = $non_default_value; assert_eq!(reference.dep_tracking_hash(), opts.dep_tracking_hash()); }; @@ -471,7 +474,7 @@ fn test_debugging_options_tracking_hash() { untracked!(ast_json, true); untracked!(ast_json_noexpand, true); untracked!(borrowck, String::from("other")); - untracked!(deduplicate_diagnostics, true); + untracked!(deduplicate_diagnostics, false); untracked!(dep_tasks, true); untracked!(dont_buffer_diagnostics, true); untracked!(dump_dep_graph, true); @@ -515,7 +518,7 @@ fn test_debugging_options_tracking_hash() { untracked!(self_profile_events, Some(vec![String::new()])); untracked!(span_debug, true); untracked!(span_free_formats, true); - untracked!(strip, Strip::None); + untracked!(strip, Strip::Debuginfo); untracked!(terminal_width, Some(80)); untracked!(threads, 99); untracked!(time, true); @@ -532,6 +535,7 @@ fn test_debugging_options_tracking_hash() { macro_rules! tracked { ($name: ident, $non_default_value: expr) => { opts = reference.clone(); + assert_ne!(opts.debugging_opts.$name, $non_default_value); opts.debugging_opts.$name = $non_default_value; assert_ne!(reference.dep_tracking_hash(), opts.dep_tracking_hash()); }; diff --git a/compiler/rustc_session/src/config.rs b/compiler/rustc_session/src/config.rs index 52a6e4ff924..a7b9d03844c 100644 --- a/compiler/rustc_session/src/config.rs +++ b/compiler/rustc_session/src/config.rs @@ -156,7 +156,7 @@ pub enum InstrumentCoverage { Off, } -#[derive(Clone, PartialEq, Hash)] +#[derive(Clone, PartialEq, Hash, Debug)] pub enum LinkerPluginLto { LinkerPlugin(PathBuf), LinkerPluginAuto, @@ -172,7 +172,7 @@ impl LinkerPluginLto { } } -#[derive(Clone, PartialEq, Hash)] +#[derive(Clone, PartialEq, Hash, Debug)] pub enum SwitchWithOptPath { Enabled(Option), Disabled, @@ -778,7 +778,7 @@ pub enum CrateType { impl_stable_hash_via_hash!(CrateType); -#[derive(Clone, Hash)] +#[derive(Clone, Hash, Debug, PartialEq, Eq)] pub enum Passes { Some(Vec), All, diff --git a/compiler/rustc_session/src/options.rs b/compiler/rustc_session/src/options.rs index fd26f50da5a..759110f6859 100644 --- a/compiler/rustc_session/src/options.rs +++ b/compiler/rustc_session/src/options.rs @@ -1220,7 +1220,7 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, // - compiler/rustc_interface/src/tests.rs } -#[derive(Clone, Hash)] +#[derive(Clone, Hash, PartialEq, Eq, Debug)] pub enum WasiExecModel { Command, Reactor, From 272015190d058b7c802331e870b23857eeba22cd Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Thu, 15 Apr 2021 19:25:01 -0400 Subject: [PATCH 2/5] Add [TRACKED_NO_CRATE_HASH] and [SUBSTRUCT] directives This is necessary for options that should invalidate the incremental hash but *not* affect the crate hash (e.g. --remap-path-prefix). This doesn't add `for_crate_hash` to the trait directly because it's not relevant for *types*, only for *options*, which are fields on a larger struct. Instead, it adds a new `SUBSTRUCT` directive for options, which does take a `for_crate_hash` parameter. - Use TRACKED_NO_CRATE_HASH for --remap-path-prefix - Add test that `remap_path_prefix` is tracked - Reduce duplication in the test suite to avoid future churn --- .../rustc_incremental/src/persist/load.rs | 2 +- .../rustc_incremental/src/persist/save.rs | 2 +- compiler/rustc_interface/src/tests.rs | 131 +++++++++--------- compiler/rustc_middle/src/hir/map/mod.rs | 2 +- compiler/rustc_session/src/config.rs | 1 + compiler/rustc_session/src/options.rs | 85 ++++++++---- 6 files changed, 133 insertions(+), 90 deletions(-) diff --git a/compiler/rustc_incremental/src/persist/load.rs b/compiler/rustc_incremental/src/persist/load.rs index 259e540c612..2661afd7ffc 100644 --- a/compiler/rustc_incremental/src/persist/load.rs +++ b/compiler/rustc_incremental/src/persist/load.rs @@ -104,7 +104,7 @@ pub fn load_dep_graph(sess: &Session) -> DepGraphFuture { // Fortunately, we just checked that this isn't the case. let path = dep_graph_path_from(&sess.incr_comp_session_dir()); let report_incremental_info = sess.opts.debugging_opts.incremental_info; - let expected_hash = sess.opts.dep_tracking_hash(); + let expected_hash = sess.opts.dep_tracking_hash(false); let mut prev_work_products = FxHashMap::default(); let nightly_build = sess.is_nightly_build(); diff --git a/compiler/rustc_incremental/src/persist/save.rs b/compiler/rustc_incremental/src/persist/save.rs index d558af3c1d5..1484088837a 100644 --- a/compiler/rustc_incremental/src/persist/save.rs +++ b/compiler/rustc_incremental/src/persist/save.rs @@ -219,7 +219,7 @@ pub fn build_dep_graph( } // First encode the commandline arguments hash - if let Err(err) = sess.opts.dep_tracking_hash().encode(&mut encoder) { + if let Err(err) = sess.opts.dep_tracking_hash(false).encode(&mut encoder) { sess.err(&format!( "failed to write dependency graph hash `{}`: {}", path_buf.display(), diff --git a/compiler/rustc_interface/src/tests.rs b/compiler/rustc_interface/src/tests.rs index 5800fa173dd..62c2d3c722f 100644 --- a/compiler/rustc_interface/src/tests.rs +++ b/compiler/rustc_interface/src/tests.rs @@ -19,6 +19,7 @@ use rustc_span::symbol::sym; use rustc_span::SourceFileHashAlgorithm; use rustc_target::spec::{CodeModel, LinkerFlavor, MergeFunctions, PanicStrategy}; use rustc_target::spec::{RelocModel, RelroLevel, SanitizerSet, SplitDebuginfo, TlsModel}; + use std::collections::{BTreeMap, BTreeSet}; use std::iter::FromIterator; use std::num::NonZeroUsize; @@ -74,6 +75,27 @@ fn mk_map(entries: Vec<(K, V)>) -> BTreeMap { BTreeMap::from_iter(entries.into_iter()) } +fn assert_same_clone(x: &Options) { + assert_eq!(x.dep_tracking_hash(true), x.clone().dep_tracking_hash(true)); + assert_eq!(x.dep_tracking_hash(false), x.clone().dep_tracking_hash(false)); +} + +fn assert_same_hash(x: &Options, y: &Options) { + assert_eq!(x.dep_tracking_hash(true), y.dep_tracking_hash(true)); + assert_eq!(x.dep_tracking_hash(false), y.dep_tracking_hash(false)); + // Check clone + assert_same_clone(x); + assert_same_clone(y); +} + +fn assert_different_hash(x: &Options, y: &Options) { + assert_ne!(x.dep_tracking_hash(true), y.dep_tracking_hash(true)); + assert_ne!(x.dep_tracking_hash(false), y.dep_tracking_hash(false)); + // Check clone + assert_same_clone(x); + assert_same_clone(y); +} + // When the user supplies --test we should implicitly supply --cfg test #[test] fn test_switch_implies_cfg_test() { @@ -130,14 +152,9 @@ fn test_output_types_tracking_hash_different_paths() { v2.output_types = OutputTypes::new(&[(OutputType::Exe, Some(PathBuf::from("/some/thing")))]); v3.output_types = OutputTypes::new(&[(OutputType::Exe, None)]); - assert!(v1.dep_tracking_hash() != v2.dep_tracking_hash()); - assert!(v1.dep_tracking_hash() != v3.dep_tracking_hash()); - assert!(v2.dep_tracking_hash() != v3.dep_tracking_hash()); - - // Check clone - assert_eq!(v1.dep_tracking_hash(), v1.clone().dep_tracking_hash()); - assert_eq!(v2.dep_tracking_hash(), v2.clone().dep_tracking_hash()); - assert_eq!(v3.dep_tracking_hash(), v3.clone().dep_tracking_hash()); + assert_different_hash(&v1, &v2); + assert_different_hash(&v1, &v3); + assert_different_hash(&v2, &v3); } #[test] @@ -155,10 +172,7 @@ fn test_output_types_tracking_hash_different_construction_order() { (OutputType::Exe, Some(PathBuf::from("./some/thing"))), ]); - assert_eq!(v1.dep_tracking_hash(), v2.dep_tracking_hash()); - - // Check clone - assert_eq!(v1.dep_tracking_hash(), v1.clone().dep_tracking_hash()); + assert_same_hash(&v1, &v2); } #[test] @@ -182,14 +196,9 @@ fn test_externs_tracking_hash_different_construction_order() { (String::from("d"), new_public_extern_entry(vec!["f", "e"])), ])); - assert_eq!(v1.dep_tracking_hash(), v2.dep_tracking_hash()); - assert_eq!(v1.dep_tracking_hash(), v3.dep_tracking_hash()); - assert_eq!(v2.dep_tracking_hash(), v3.dep_tracking_hash()); - - // Check clone - assert_eq!(v1.dep_tracking_hash(), v1.clone().dep_tracking_hash()); - assert_eq!(v2.dep_tracking_hash(), v2.clone().dep_tracking_hash()); - assert_eq!(v3.dep_tracking_hash(), v3.clone().dep_tracking_hash()); + assert_same_hash(&v1, &v2); + assert_same_hash(&v1, &v3); + assert_same_hash(&v2, &v3); } #[test] @@ -219,14 +228,9 @@ fn test_lints_tracking_hash_different_values() { (String::from("d"), Level::Deny), ]; - assert!(v1.dep_tracking_hash() != v2.dep_tracking_hash()); - assert!(v1.dep_tracking_hash() != v3.dep_tracking_hash()); - assert!(v2.dep_tracking_hash() != v3.dep_tracking_hash()); - - // Check clone - assert_eq!(v1.dep_tracking_hash(), v1.clone().dep_tracking_hash()); - assert_eq!(v2.dep_tracking_hash(), v2.clone().dep_tracking_hash()); - assert_eq!(v3.dep_tracking_hash(), v3.clone().dep_tracking_hash()); + assert_different_hash(&v1, &v2); + assert_different_hash(&v1, &v3); + assert_different_hash(&v2, &v3); } #[test] @@ -248,11 +252,7 @@ fn test_lints_tracking_hash_different_construction_order() { (String::from("d"), Level::Forbid), ]; - assert_eq!(v1.dep_tracking_hash(), v2.dep_tracking_hash()); - - // Check clone - assert_eq!(v1.dep_tracking_hash(), v1.clone().dep_tracking_hash()); - assert_eq!(v2.dep_tracking_hash(), v2.clone().dep_tracking_hash()); + assert_same_hash(&v1, &v2); } #[test] @@ -292,15 +292,9 @@ fn test_search_paths_tracking_hash_different_order() { v4.search_paths.push(SearchPath::from_cli_opt("dependency=ghi", JSON)); v4.search_paths.push(SearchPath::from_cli_opt("framework=jkl", JSON)); - assert!(v1.dep_tracking_hash() == v2.dep_tracking_hash()); - assert!(v1.dep_tracking_hash() == v3.dep_tracking_hash()); - assert!(v1.dep_tracking_hash() == v4.dep_tracking_hash()); - - // Check clone - assert_eq!(v1.dep_tracking_hash(), v1.clone().dep_tracking_hash()); - assert_eq!(v2.dep_tracking_hash(), v2.clone().dep_tracking_hash()); - assert_eq!(v3.dep_tracking_hash(), v3.clone().dep_tracking_hash()); - assert_eq!(v4.dep_tracking_hash(), v4.clone().dep_tracking_hash()); + assert_same_hash(&v1, &v2); + assert_same_hash(&v1, &v3); + assert_same_hash(&v1, &v4); } #[test] @@ -338,15 +332,9 @@ fn test_native_libs_tracking_hash_different_values() { (String::from("c"), None, NativeLibKind::Unspecified), ]; - assert!(v1.dep_tracking_hash() != v2.dep_tracking_hash()); - assert!(v1.dep_tracking_hash() != v3.dep_tracking_hash()); - assert!(v1.dep_tracking_hash() != v4.dep_tracking_hash()); - - // Check clone - assert_eq!(v1.dep_tracking_hash(), v1.clone().dep_tracking_hash()); - assert_eq!(v2.dep_tracking_hash(), v2.clone().dep_tracking_hash()); - assert_eq!(v3.dep_tracking_hash(), v3.clone().dep_tracking_hash()); - assert_eq!(v4.dep_tracking_hash(), v4.clone().dep_tracking_hash()); + assert_different_hash(&v1, &v2); + assert_different_hash(&v1, &v3); + assert_different_hash(&v1, &v4); } #[test] @@ -374,14 +362,9 @@ fn test_native_libs_tracking_hash_different_order() { (String::from("b"), None, NativeLibKind::Framework), ]; - assert!(v1.dep_tracking_hash() == v2.dep_tracking_hash()); - assert!(v1.dep_tracking_hash() == v3.dep_tracking_hash()); - assert!(v2.dep_tracking_hash() == v3.dep_tracking_hash()); - - // Check clone - assert_eq!(v1.dep_tracking_hash(), v1.clone().dep_tracking_hash()); - assert_eq!(v2.dep_tracking_hash(), v2.clone().dep_tracking_hash()); - assert_eq!(v3.dep_tracking_hash(), v3.clone().dep_tracking_hash()); + assert_same_hash(&v1, &v2); + assert_same_hash(&v1, &v3); + assert_same_hash(&v2, &v3); } #[test] @@ -393,7 +376,7 @@ fn test_codegen_options_tracking_hash() { ($name: ident, $non_default_value: expr) => { assert_ne!(opts.cg.$name, $non_default_value); opts.cg.$name = $non_default_value; - assert_eq!(reference.dep_tracking_hash(), opts.dep_tracking_hash()); + assert_same_hash(&reference, &opts); }; } @@ -419,7 +402,7 @@ fn test_codegen_options_tracking_hash() { opts = reference.clone(); assert_ne!(opts.cg.$name, $non_default_value); opts.cg.$name = $non_default_value; - assert_ne!(reference.dep_tracking_hash(), opts.dep_tracking_hash()); + assert_different_hash(&reference, &opts); }; } @@ -456,6 +439,28 @@ fn test_codegen_options_tracking_hash() { tracked!(target_feature, String::from("all the features, all of them")); } +#[test] +fn test_top_level_options_tracked_no_crate() { + let reference = Options::default(); + let mut opts; + + macro_rules! tracked { + ($name: ident, $non_default_value: expr) => { + opts = reference.clone(); + assert_ne!(opts.$name, $non_default_value); + opts.$name = $non_default_value; + // The crate hash should be the same + assert_eq!(reference.dep_tracking_hash(true), opts.dep_tracking_hash(true)); + // The incremental hash should be different + assert_ne!(reference.dep_tracking_hash(false), opts.dep_tracking_hash(false)); + }; + } + + // Make sure that changing a [TRACKED_NO_CRATE_HASH] option leaves the crate hash unchanged but changes the incremental hash. + // This list is in alphabetical order. + tracked!(remap_path_prefix, vec![("/home/bors/rust".into(), "src".into())]); +} + #[test] fn test_debugging_options_tracking_hash() { let reference = Options::default(); @@ -465,7 +470,7 @@ fn test_debugging_options_tracking_hash() { ($name: ident, $non_default_value: expr) => { assert_ne!(opts.debugging_opts.$name, $non_default_value); opts.debugging_opts.$name = $non_default_value; - assert_eq!(reference.dep_tracking_hash(), opts.dep_tracking_hash()); + assert_same_hash(&reference, &opts); }; } @@ -537,7 +542,7 @@ fn test_debugging_options_tracking_hash() { opts = reference.clone(); assert_ne!(opts.debugging_opts.$name, $non_default_value); opts.debugging_opts.$name = $non_default_value; - assert_ne!(reference.dep_tracking_hash(), opts.dep_tracking_hash()); + assert_different_hash(&reference, &opts); }; } diff --git a/compiler/rustc_middle/src/hir/map/mod.rs b/compiler/rustc_middle/src/hir/map/mod.rs index d155276051e..4cd126988f9 100644 --- a/compiler/rustc_middle/src/hir/map/mod.rs +++ b/compiler/rustc_middle/src/hir/map/mod.rs @@ -943,7 +943,7 @@ pub(super) fn index_hir<'tcx>(tcx: TyCtxt<'tcx>, cnum: CrateNum) -> &'tcx Indexe intravisit::walk_crate(&mut collector, tcx.untracked_crate); let crate_disambiguator = tcx.sess.local_crate_disambiguator(); - let cmdline_args = tcx.sess.opts.dep_tracking_hash(); + let cmdline_args = tcx.sess.opts.dep_tracking_hash(true); collector.finalize_and_compute_crate_hash(crate_disambiguator, &*tcx.cstore, cmdline_args) }; diff --git a/compiler/rustc_session/src/config.rs b/compiler/rustc_session/src/config.rs index a7b9d03844c..5d588ad1be2 100644 --- a/compiler/rustc_session/src/config.rs +++ b/compiler/rustc_session/src/config.rs @@ -2374,6 +2374,7 @@ crate mod dep_tracking { impl_dep_tracking_hash_for_sortable_vec_of!(String); impl_dep_tracking_hash_for_sortable_vec_of!(PathBuf); + impl_dep_tracking_hash_for_sortable_vec_of!((PathBuf, PathBuf)); impl_dep_tracking_hash_for_sortable_vec_of!(CrateType); impl_dep_tracking_hash_for_sortable_vec_of!((String, lint::Level)); impl_dep_tracking_hash_for_sortable_vec_of!((String, Option, NativeLibKind)); diff --git a/compiler/rustc_session/src/options.rs b/compiler/rustc_session/src/options.rs index 759110f6859..226c7bda052 100644 --- a/compiler/rustc_session/src/options.rs +++ b/compiler/rustc_session/src/options.rs @@ -20,21 +20,41 @@ use std::num::NonZeroUsize; use std::path::PathBuf; use std::str; -macro_rules! hash_option { - ($opt_name:ident, $opt_expr:expr, $sub_hashes:expr, [UNTRACKED]) => {{}}; - ($opt_name:ident, $opt_expr:expr, $sub_hashes:expr, [TRACKED]) => {{ +macro_rules! insert { + ($opt_name:ident, $opt_expr:expr, $sub_hashes:expr) => { if $sub_hashes .insert(stringify!($opt_name), $opt_expr as &dyn dep_tracking::DepTrackingHash) .is_some() { panic!("duplicate key in CLI DepTrackingHash: {}", stringify!($opt_name)) } + }; +} + +macro_rules! hash_opt { + ($opt_name:ident, $opt_expr:expr, $sub_hashes:expr, $_for_crate_hash: ident, [UNTRACKED]) => {{}}; + ($opt_name:ident, $opt_expr:expr, $sub_hashes:expr, $_for_crate_hash: ident, [TRACKED]) => {{ insert!($opt_name, $opt_expr, $sub_hashes) }}; + ($opt_name:ident, $opt_expr:expr, $sub_hashes:expr, $for_crate_hash: ident, [TRACKED_NO_CRATE_HASH]) => {{ + if !$for_crate_hash { + insert!($opt_name, $opt_expr, $sub_hashes) + } }}; + ($opt_name:ident, $opt_expr:expr, $sub_hashes:expr, $_for_crate_hash: ident, [SUBSTRUCT]) => {{}}; +} + +macro_rules! hash_substruct { + ($opt_name:ident, $opt_expr:expr, $error_format:expr, $for_crate_hash:expr, $hasher:expr, [UNTRACKED]) => {{}}; + ($opt_name:ident, $opt_expr:expr, $error_format:expr, $for_crate_hash:expr, $hasher:expr, [TRACKED]) => {{}}; + ($opt_name:ident, $opt_expr:expr, $error_format:expr, $for_crate_hash:expr, $hasher:expr, [TRACKED_NO_CRATE_HASH]) => {{}}; + ($opt_name:ident, $opt_expr:expr, $error_format:expr, $for_crate_hash:expr, $hasher:expr, [SUBSTRUCT]) => { + use crate::config::dep_tracking::DepTrackingHash; + $opt_expr.dep_tracking_hash($for_crate_hash, $error_format).hash($hasher, $error_format); + }; } macro_rules! top_level_options { (pub struct Options { $( - $opt:ident : $t:ty [$dep_tracking_marker:ident $($warn_val:expr, $warn_text:expr)*], + $opt:ident : $t:ty [$dep_tracking_marker:ident], )* } ) => ( #[derive(Clone)] pub struct Options { @@ -42,20 +62,27 @@ macro_rules! top_level_options { } impl Options { - pub fn dep_tracking_hash(&self) -> u64 { + pub fn dep_tracking_hash(&self, for_crate_hash: bool) -> u64 { let mut sub_hashes = BTreeMap::new(); $({ - hash_option!($opt, - &self.$opt, - &mut sub_hashes, - [$dep_tracking_marker $($warn_val, - $warn_text, - self.error_format)*]); + hash_opt!($opt, + &self.$opt, + &mut sub_hashes, + for_crate_hash, + [$dep_tracking_marker]); })* let mut hasher = DefaultHasher::new(); dep_tracking::stable_hash(sub_hashes, &mut hasher, self.error_format); + $({ + hash_substruct!($opt, + &self.$opt, + self.error_format, + for_crate_hash, + &mut hasher, + [$dep_tracking_marker]); + })* hasher.finish() } } @@ -72,9 +99,16 @@ macro_rules! top_level_options { // A change in the given field will cause the compiler to completely clear the // incremental compilation cache before proceeding. // +// [TRACKED_NO_CRATE_HASH] +// Same as [TRACKED], but will not affect the crate hash. This is useful for options that only +// affect the incremental cache. +// // [UNTRACKED] // Incremental compilation is not influenced by this option. // +// [SUBSTRUCT] +// Second-level sub-structs containing more options. +// // If you add a new option to this struct or one of the sub-structs like // `CodegenOptions`, think about how it influences incremental compilation. If in // doubt, specify [TRACKED], which is always "correct" but might lead to @@ -106,12 +140,12 @@ top_level_options!( // directory to store intermediate results. incremental: Option [UNTRACKED], - debugging_opts: DebuggingOptions [TRACKED], + debugging_opts: DebuggingOptions [SUBSTRUCT], prints: Vec [UNTRACKED], // Determines which borrow checker(s) to run. This is the parsed, sanitized // version of `debugging_opts.borrowck`, which is just a plain string. borrowck_mode: BorrowckMode [UNTRACKED], - cg: CodegenOptions [TRACKED], + cg: CodegenOptions [SUBSTRUCT], externs: Externs [UNTRACKED], extern_dep_specs: ExternDepSpecs [UNTRACKED], crate_name: Option [TRACKED], @@ -139,7 +173,7 @@ top_level_options!( cli_forced_thinlto_off: bool [UNTRACKED], // Remap source path prefixes in all output (messages, object files, debug, etc.). - remap_path_prefix: Vec<(PathBuf, PathBuf)> [UNTRACKED], + remap_path_prefix: Vec<(PathBuf, PathBuf)> [TRACKED_NO_CRATE_HASH], edition: Edition [TRACKED], @@ -169,7 +203,7 @@ macro_rules! options { $($opt:ident : $t:ty = ( $init:expr, $parse:ident, - [$dep_tracking_marker:ident $(($dep_warn_val:expr, $dep_warn_text:expr))*], + [$dep_tracking_marker:ident], $desc:expr) ),* ,) => ( @@ -219,18 +253,21 @@ macro_rules! options { return op; } - impl dep_tracking::DepTrackingHash for $struct_name { - fn hash(&self, hasher: &mut DefaultHasher, error_format: ErrorOutputType) { + impl $struct_name { + fn dep_tracking_hash(&self, for_crate_hash: bool, error_format: ErrorOutputType) -> u64 { let mut sub_hashes = BTreeMap::new(); $({ - hash_option!($opt, - &self.$opt, - &mut sub_hashes, - [$dep_tracking_marker $($dep_warn_val, - $dep_warn_text, - error_format)*]); + hash_opt!($opt, + &self.$opt, + &mut sub_hashes, + for_crate_hash, + [$dep_tracking_marker]); })* - dep_tracking::stable_hash(sub_hashes, hasher, error_format); + let mut hasher = DefaultHasher::new(); + dep_tracking::stable_hash(sub_hashes, + &mut hasher, + error_format); + hasher.finish() } } From 39648ea467a39afa3676d900656874947c747690 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Tue, 27 Apr 2021 16:25:12 +0000 Subject: [PATCH 3/5] Make `real_rust_path_dir` a TRACKED_NO_CRATE_HASH option This also adds support for doc-comments to Options. --- compiler/rustc_interface/src/tests.rs | 4 +++ compiler/rustc_metadata/src/rmeta/decoder.rs | 6 ++-- compiler/rustc_session/src/config.rs | 30 ++++++++++++++++++++ compiler/rustc_session/src/options.rs | 18 ++++++++++-- compiler/rustc_session/src/session.rs | 30 -------------------- 5 files changed, 52 insertions(+), 36 deletions(-) diff --git a/compiler/rustc_interface/src/tests.rs b/compiler/rustc_interface/src/tests.rs index 62c2d3c722f..d8c1a7a2682 100644 --- a/compiler/rustc_interface/src/tests.rs +++ b/compiler/rustc_interface/src/tests.rs @@ -459,6 +459,10 @@ fn test_top_level_options_tracked_no_crate() { // Make sure that changing a [TRACKED_NO_CRATE_HASH] option leaves the crate hash unchanged but changes the incremental hash. // This list is in alphabetical order. tracked!(remap_path_prefix, vec![("/home/bors/rust".into(), "src".into())]); + tracked!( + real_rust_source_base_dir, + Some("/home/bors/rust/.rustup/toolchains/nightly/lib/rustlib/src/rust".into()) + ); } #[test] diff --git a/compiler/rustc_metadata/src/rmeta/decoder.rs b/compiler/rustc_metadata/src/rmeta/decoder.rs index 19ae5ce69c1..2ade1bb4f95 100644 --- a/compiler/rustc_metadata/src/rmeta/decoder.rs +++ b/compiler/rustc_metadata/src/rmeta/decoder.rs @@ -1617,7 +1617,7 @@ impl<'a, 'tcx> CrateMetadataRef<'a> { .map(Path::new) .filter(|_| { // Only spend time on further checks if we have what to translate *to*. - sess.real_rust_source_base_dir.is_some() + sess.opts.real_rust_source_base_dir.is_some() }) .filter(|virtual_dir| { // Don't translate away `/rustc/$hash` if we're still remapping to it, @@ -1629,11 +1629,11 @@ impl<'a, 'tcx> CrateMetadataRef<'a> { debug!( "try_to_translate_virtual_to_real(name={:?}): \ virtual_rust_source_base_dir={:?}, real_rust_source_base_dir={:?}", - name, virtual_rust_source_base_dir, sess.real_rust_source_base_dir, + name, virtual_rust_source_base_dir, sess.opts.real_rust_source_base_dir, ); if let Some(virtual_dir) = virtual_rust_source_base_dir { - if let Some(real_dir) = &sess.real_rust_source_base_dir { + if let Some(real_dir) = &sess.opts.real_rust_source_base_dir { if let rustc_span::FileName::Real(old_name) = name { if let rustc_span::RealFileName::Named(one_path) = old_name { if let Ok(rest) = one_path.strip_prefix(virtual_dir) { diff --git a/compiler/rustc_session/src/config.rs b/compiler/rustc_session/src/config.rs index 5d588ad1be2..1f5cb5b8abc 100644 --- a/compiler/rustc_session/src/config.rs +++ b/compiler/rustc_session/src/config.rs @@ -702,6 +702,7 @@ impl Default for Options { cli_forced_codegen_units: None, cli_forced_thinlto_off: false, remap_path_prefix: Vec::new(), + real_rust_source_base_dir: None, edition: DEFAULT_EDITION, json_artifact_notifications: false, json_unused_externs: false, @@ -1980,6 +1981,34 @@ pub fn build_session_options(matches: &getopts::Matches) -> Options { } } + // Try to find a directory containing the Rust `src`, for more details see + // the doc comment on the `real_rust_source_base_dir` field. + let tmp_buf; + let sysroot = match &sysroot_opt { + Some(s) => s, + None => { + tmp_buf = crate::filesearch::get_or_default_sysroot(); + &tmp_buf + } + }; + let real_rust_source_base_dir = { + // This is the location used by the `rust-src` `rustup` component. + let mut candidate = sysroot.join("lib/rustlib/src/rust"); + if let Ok(metadata) = candidate.symlink_metadata() { + // Replace the symlink rustbuild creates, with its destination. + // We could try to use `fs::canonicalize` instead, but that might + // produce unnecessarily verbose path. + if metadata.file_type().is_symlink() { + if let Ok(symlink_dest) = std::fs::read_link(&candidate) { + candidate = symlink_dest; + } + } + } + + // Only use this directory if it has a file we can expect to always find. + if candidate.join("library/std/src/lib.rs").is_file() { Some(candidate) } else { None } + }; + Options { crate_types, optimize: opt_level, @@ -2010,6 +2039,7 @@ pub fn build_session_options(matches: &getopts::Matches) -> Options { cli_forced_codegen_units: codegen_units, cli_forced_thinlto_off: disable_thinlto, remap_path_prefix, + real_rust_source_base_dir, edition, json_artifact_notifications, json_unused_externs, diff --git a/compiler/rustc_session/src/options.rs b/compiler/rustc_session/src/options.rs index 226c7bda052..d15dec622e5 100644 --- a/compiler/rustc_session/src/options.rs +++ b/compiler/rustc_session/src/options.rs @@ -54,11 +54,15 @@ macro_rules! hash_substruct { macro_rules! top_level_options { (pub struct Options { $( + $( #[$attr:meta] )* $opt:ident : $t:ty [$dep_tracking_marker:ident], )* } ) => ( #[derive(Clone)] pub struct Options { - $(pub $opt: $t),* + $( + $( #[$attr] )* + pub $opt: $t + ),* } impl Options { @@ -174,6 +178,14 @@ top_level_options!( // Remap source path prefixes in all output (messages, object files, debug, etc.). remap_path_prefix: Vec<(PathBuf, PathBuf)> [TRACKED_NO_CRATE_HASH], + /// Base directory containing the `src/` for the Rust standard library, and + /// potentially `rustc` as well, if we can can find it. Right now it's always + /// `$sysroot/lib/rustlib/src/rust` (i.e. the `rustup` `rust-src` component). + /// + /// This directory is what the virtual `/rustc/$hash` is translated back to, + /// if Rust was built with path remapping to `/rustc/$hash` enabled + /// (the `rust.remap-debuginfo` option in `config.toml`). + real_rust_source_base_dir: Option [TRACKED_NO_CRATE_HASH], edition: Edition [TRACKED], @@ -254,13 +266,13 @@ macro_rules! options { } impl $struct_name { - fn dep_tracking_hash(&self, for_crate_hash: bool, error_format: ErrorOutputType) -> u64 { + fn dep_tracking_hash(&self, _for_crate_hash: bool, error_format: ErrorOutputType) -> u64 { let mut sub_hashes = BTreeMap::new(); $({ hash_opt!($opt, &self.$opt, &mut sub_hashes, - for_crate_hash, + _for_crate_hash, [$dep_tracking_marker]); })* let mut hasher = DefaultHasher::new(); diff --git a/compiler/rustc_session/src/session.rs b/compiler/rustc_session/src/session.rs index 7bff634fb2d..e7dfc4b8c41 100644 --- a/compiler/rustc_session/src/session.rs +++ b/compiler/rustc_session/src/session.rs @@ -214,15 +214,6 @@ pub struct Session { /// drown everything else in noise. miri_unleashed_features: Lock)>>, - /// Base directory containing the `src/` for the Rust standard library, and - /// potentially `rustc` as well, if we can can find it. Right now it's always - /// `$sysroot/lib/rustlib/src/rust` (i.e. the `rustup` `rust-src` component). - /// - /// This directory is what the virtual `/rustc/$hash` is translated back to, - /// if Rust was built with path remapping to `/rustc/$hash` enabled - /// (the `rust.remap-debuginfo` option in `config.toml`). - pub real_rust_source_base_dir: Option, - /// Architecture to use for interpreting asm!. pub asm_arch: Option, @@ -1390,26 +1381,6 @@ pub fn build_session( _ => CtfeBacktrace::Disabled, }); - // Try to find a directory containing the Rust `src`, for more details see - // the doc comment on the `real_rust_source_base_dir` field. - let real_rust_source_base_dir = { - // This is the location used by the `rust-src` `rustup` component. - let mut candidate = sysroot.join("lib/rustlib/src/rust"); - if let Ok(metadata) = candidate.symlink_metadata() { - // Replace the symlink rustbuild creates, with its destination. - // We could try to use `fs::canonicalize` instead, but that might - // produce unnecessarily verbose path. - if metadata.file_type().is_symlink() { - if let Ok(symlink_dest) = std::fs::read_link(&candidate) { - candidate = symlink_dest; - } - } - } - - // Only use this directory if it has a file we can expect to always find. - if candidate.join("library/std/src/lib.rs").is_file() { Some(candidate) } else { None } - }; - let asm_arch = if target_cfg.allow_asm { InlineAsmArch::from_str(&target_cfg.arch).ok() } else { None }; @@ -1453,7 +1424,6 @@ pub fn build_session( system_library_path: OneThread::new(RefCell::new(Default::default())), ctfe_backtrace, miri_unleashed_features: Lock::new(Default::default()), - real_rust_source_base_dir, asm_arch, target_features: FxHashSet::default(), known_attrs: Lock::new(MarkedAttrs::new()), From bfa74c270ffa93d69ede85824367edc4ff23e95f Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Tue, 27 Apr 2021 16:44:14 +0000 Subject: [PATCH 4/5] Use doc-comment instad of comments consistently This makes the comments show up in the generated docs. - Fix markdown formatting --- compiler/rustc_session/src/options.rs | 117 +++++++++++++------------- 1 file changed, 59 insertions(+), 58 deletions(-) diff --git a/compiler/rustc_session/src/options.rs b/compiler/rustc_session/src/options.rs index d15dec622e5..1c2a7f7716d 100644 --- a/compiler/rustc_session/src/options.rs +++ b/compiler/rustc_session/src/options.rs @@ -53,11 +53,12 @@ macro_rules! hash_substruct { } macro_rules! top_level_options { - (pub struct Options { $( + ( $( #[$top_level_attr:meta] )* pub struct Options { $( $( #[$attr:meta] )* $opt:ident : $t:ty [$dep_tracking_marker:ident], )* } ) => ( #[derive(Clone)] + $( #[$top_level_attr] )* pub struct Options { $( $( #[$attr] )* @@ -93,38 +94,38 @@ macro_rules! top_level_options { ); } -// The top-level command-line options struct. -// -// For each option, one has to specify how it behaves with regard to the -// dependency tracking system of incremental compilation. This is done via the -// square-bracketed directive after the field type. The options are: -// -// [TRACKED] -// A change in the given field will cause the compiler to completely clear the -// incremental compilation cache before proceeding. -// -// [TRACKED_NO_CRATE_HASH] -// Same as [TRACKED], but will not affect the crate hash. This is useful for options that only -// affect the incremental cache. -// -// [UNTRACKED] -// Incremental compilation is not influenced by this option. -// -// [SUBSTRUCT] -// Second-level sub-structs containing more options. -// -// If you add a new option to this struct or one of the sub-structs like -// `CodegenOptions`, think about how it influences incremental compilation. If in -// doubt, specify [TRACKED], which is always "correct" but might lead to -// unnecessary re-compilation. top_level_options!( + /// The top-level command-line options struct. + /// + /// For each option, one has to specify how it behaves with regard to the + /// dependency tracking system of incremental compilation. This is done via the + /// square-bracketed directive after the field type. The options are: + /// + /// - `[TRACKED]` + /// A change in the given field will cause the compiler to completely clear the + /// incremental compilation cache before proceeding. + /// + /// - `[TRACKED_NO_CRATE_HASH]` + /// Same as `[TRACKED]`, but will not affect the crate hash. This is useful for options that only + /// affect the incremental cache. + /// + /// - `[UNTRACKED]` + /// Incremental compilation is not influenced by this option. + /// + /// - `[SUBSTRUCT]` + /// Second-level sub-structs containing more options. + /// + /// If you add a new option to this struct or one of the sub-structs like + /// `CodegenOptions`, think about how it influences incremental compilation. If in + /// doubt, specify `[TRACKED]`, which is always "correct" but might lead to + /// unnecessary re-compilation. pub struct Options { - // The crate config requested for the session, which may be combined - // with additional crate configurations during the compile process. + /// The crate config requested for the session, which may be combined + /// with additional crate configurations during the compile process. crate_types: Vec [TRACKED], optimize: OptLevel [TRACKED], - // Include the `debug_assertions` flag in dependency tracking, since it - // can influence whether overflow checks are done or not. + /// Include the `debug_assertions` flag in dependency tracking, since it + /// can influence whether overflow checks are done or not. debug_assertions: bool [TRACKED], debuginfo: DebugInfo [TRACKED], lint_opts: Vec<(String, lint::Level)> [TRACKED], @@ -140,43 +141,43 @@ top_level_options!( test: bool [TRACKED], error_format: ErrorOutputType [UNTRACKED], - // If `Some`, enable incremental compilation, using the given - // directory to store intermediate results. + /// If `Some`, enable incremental compilation, using the given + /// directory to store intermediate results. incremental: Option [UNTRACKED], debugging_opts: DebuggingOptions [SUBSTRUCT], prints: Vec [UNTRACKED], - // Determines which borrow checker(s) to run. This is the parsed, sanitized - // version of `debugging_opts.borrowck`, which is just a plain string. + /// Determines which borrow checker(s) to run. This is the parsed, sanitized + /// version of `debugging_opts.borrowck`, which is just a plain string. borrowck_mode: BorrowckMode [UNTRACKED], cg: CodegenOptions [SUBSTRUCT], externs: Externs [UNTRACKED], extern_dep_specs: ExternDepSpecs [UNTRACKED], crate_name: Option [TRACKED], - // An optional name to use as the crate for std during std injection, - // written `extern crate name as std`. Defaults to `std`. Used by - // out-of-tree drivers. + /// An optional name to use as the crate for std during std injection, + /// written `extern crate name as std`. Defaults to `std`. Used by + /// out-of-tree drivers. alt_std_name: Option [TRACKED], - // Indicates how the compiler should treat unstable features. + /// Indicates how the compiler should treat unstable features. unstable_features: UnstableFeatures [TRACKED], - // Indicates whether this run of the compiler is actually rustdoc. This - // is currently just a hack and will be removed eventually, so please - // try to not rely on this too much. + /// Indicates whether this run of the compiler is actually rustdoc. This + /// is currently just a hack and will be removed eventually, so please + /// try to not rely on this too much. actually_rustdoc: bool [TRACKED], - // Control path trimming. + /// Control path trimming. trimmed_def_paths: TrimmedDefPaths [TRACKED], - // Specifications of codegen units / ThinLTO which are forced as a - // result of parsing command line options. These are not necessarily - // what rustc was invoked with, but massaged a bit to agree with - // commands like `--emit llvm-ir` which they're often incompatible with - // if we otherwise use the defaults of rustc. + /// Specifications of codegen units / ThinLTO which are forced as a + /// result of parsing command line options. These are not necessarily + /// what rustc was invoked with, but massaged a bit to agree with + /// commands like `--emit llvm-ir` which they're often incompatible with + /// if we otherwise use the defaults of rustc. cli_forced_codegen_units: Option [UNTRACKED], cli_forced_thinlto_off: bool [UNTRACKED], - // Remap source path prefixes in all output (messages, object files, debug, etc.). + /// Remap source path prefixes in all output (messages, object files, debug, etc.). remap_path_prefix: Vec<(PathBuf, PathBuf)> [TRACKED_NO_CRATE_HASH], /// Base directory containing the `src/` for the Rust standard library, and /// potentially `rustc` as well, if we can can find it. Right now it's always @@ -189,11 +190,11 @@ top_level_options!( edition: Edition [TRACKED], - // `true` if we're emitting JSON blobs about each artifact produced - // by the compiler. + /// `true` if we're emitting JSON blobs about each artifact produced + /// by the compiler. json_artifact_notifications: bool [TRACKED], - // `true` if we're emitting a JSON blob containing the unused externs + /// `true` if we're emitting a JSON blob containing the unused externs json_unused_externs: bool [UNTRACKED], pretty: Option [UNTRACKED], @@ -212,7 +213,7 @@ macro_rules! options { ($struct_name:ident, $setter_name:ident, $defaultfn:ident, $buildfn:ident, $prefix:expr, $outputname:expr, $stat:ident, $mod_desc:ident, $mod_set:ident, - $($opt:ident : $t:ty = ( + $($( #[$attr:meta] )* $opt:ident : $t:ty = ( $init:expr, $parse:ident, [$dep_tracking_marker:ident], @@ -223,7 +224,7 @@ macro_rules! options { pub struct $struct_name { $(pub $opt: $t),* } pub fn $defaultfn() -> $struct_name { - $struct_name { $($opt: $init),* } + $struct_name { $( $( #[$attr] )* $opt: $init),* } } pub fn $buildfn(matches: &getopts::Matches, error_format: ErrorOutputType) -> $struct_name @@ -1177,7 +1178,7 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, self_profile: SwitchWithOptPath = (SwitchWithOptPath::Disabled, parse_switch_with_opt_path, [UNTRACKED], "run the self profiler and output the raw event data"), - // keep this in sync with the event filter names in librustc_data_structures/profiling.rs + /// keep this in sync with the event filter names in librustc_data_structures/profiling.rs self_profile_events: Option> = (None, parse_opt_comma_list, [UNTRACKED], "specify the events recorded by the self profiler; for example: `-Z self-profile-events=default,query-keys` @@ -1189,7 +1190,7 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, "show spans for compiler debugging (expr|pat|ty)"), span_debug: bool = (false, parse_bool, [UNTRACKED], "forward proc_macro::Span's `Debug` impl to `Span`"), - // o/w tests have closure@path + /// o/w tests have closure@path span_free_formats: bool = (false, parse_bool, [UNTRACKED], "exclude spans when debug-printing compiler state (default: no)"), src_hash_algorithm: Option = (None, parse_src_file_hash, [TRACKED], @@ -1210,10 +1211,10 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, "select processor to schedule for (`rustc --print target-cpus` for details)"), thinlto: Option = (None, parse_opt_bool, [TRACKED], "enable ThinLTO when possible"), - // We default to 1 here since we want to behave like - // a sequential compiler for now. This'll likely be adjusted - // in the future. Note that -Zthreads=0 is the way to get - // the num_cpus behavior. + /// We default to 1 here since we want to behave like + /// a sequential compiler for now. This'll likely be adjusted + /// in the future. Note that -Zthreads=0 is the way to get + /// the num_cpus behavior. threads: usize = (1, parse_threads, [UNTRACKED], "use a thread pool with N threads"), time: bool = (false, parse_bool, [UNTRACKED], From 5a692a7838046a53c80536f4102071010d222bf6 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Wed, 28 Apr 2021 21:47:26 +0000 Subject: [PATCH 5/5] Add integration test for `--remap-pathh-prefix` --- src/test/incremental/commandline-args.rs | 7 +++++-- .../run-make-fulldeps/incr-add-rust-src-component/Makefile | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/test/incremental/commandline-args.rs b/src/test/incremental/commandline-args.rs index 08a0232f661..35b7183db7f 100644 --- a/src/test/incremental/commandline-args.rs +++ b/src/test/incremental/commandline-args.rs @@ -2,20 +2,23 @@ // the cache while changing an untracked one doesn't. // ignore-asmjs wasm2js does not support source maps yet -// revisions:rpass1 rpass2 rpass3 +// revisions:rpass1 rpass2 rpass3 rpass4 // compile-flags: -Z query-dep-graph #![feature(rustc_attrs)] #![rustc_partition_codegened(module="commandline_args", cfg="rpass2")] #![rustc_partition_reused(module="commandline_args", cfg="rpass3")] +#![rustc_partition_codegened(module="commandline_args", cfg="rpass4")] // Between revisions 1 and 2, we are changing the debuginfo-level, which should // invalidate the cache. Between revisions 2 and 3, we are adding `--verbose` -// which should have no effect on the cache: +// which should have no effect on the cache. Between revisions, we are adding +// `--remap-path-prefix` which should invalidate the cache: //[rpass1] compile-flags: -C debuginfo=0 //[rpass2] compile-flags: -C debuginfo=2 //[rpass3] compile-flags: -C debuginfo=2 --verbose +//[rpass4] compile-flags: -C debuginfo=2 --verbose --remap-path-prefix=/home/bors/rust=src pub fn main() { // empty diff --git a/src/test/run-make-fulldeps/incr-add-rust-src-component/Makefile b/src/test/run-make-fulldeps/incr-add-rust-src-component/Makefile index 50ff3dd56ce..371f94715a8 100644 --- a/src/test/run-make-fulldeps/incr-add-rust-src-component/Makefile +++ b/src/test/run-make-fulldeps/incr-add-rust-src-component/Makefile @@ -1,7 +1,7 @@ -include ../tools.mk # rust-lang/rust#70924: Test that if we add rust-src component in between two -# incremetnal compiles, the compiler does not ICE on the second. +# incremental compiles, the compiler does not ICE on the second. # This test uses `ln -s` rather than copying to save testing time, but its # usage doesn't work on windows. So ignore windows.