Auto merge of rust-lang#86045 - jsgf:fix-emit-path-hashing, r=bjorn3

bors · bors · commit b8be3162d734 · 2021-06-22T17:34:55.000Z
Fix emit path hashing

With `--emit KIND=PATH`, the PATH should not affect hashes used for dependency tracking. It does not with other ways of specifying output paths (`-o` or `--out-dir`).

Also updates `rustc -Zls` to print more info about crates, which is used here to implement a `run-make` test.

It seems there was already a test explicitly checking that `OutputTypes` hash *is* affected by the path. I think this behaviour is wrong, so I updated the test.
diff --git a/compiler/rustc_interface/src/tests.rs b/compiler/rustc_interface/src/tests.rs
@@ -96,6 +96,14 @@ fn assert_different_hash(x: &Options, y: &Options) {
     assert_same_clone(y);
 }
 
+fn assert_non_crate_hash_different(x: &Options, y: &Options) {
+    assert_eq!(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() {
@@ -152,9 +160,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_different_hash(&v1, &v2);
-    assert_different_hash(&v1, &v3);
-    assert_different_hash(&v2, &v3);
+    assert_non_crate_hash_different(&v1, &v2);
+    assert_non_crate_hash_different(&v1, &v3);
+    assert_non_crate_hash_different(&v2, &v3);
 }
 
 #[test]
@@ -712,7 +720,6 @@ fn test_debugging_options_tracking_hash() {
     tracked!(mir_opt_level, Some(4));
     tracked!(mutable_noalias, Some(true));
     tracked!(new_llvm_pass_manager, Some(true));
-    tracked!(no_codegen, true);
     tracked!(no_generate_arange_section, true);
     tracked!(no_link, true);
     tracked!(osx_rpath_install_name, true);
@@ -747,6 +754,16 @@ fn test_debugging_options_tracking_hash() {
     tracked!(use_ctors_section, Some(true));
     tracked!(verify_llvm_ir, true);
     tracked!(wasi_exec_model, Some(WasiExecModel::Reactor));
+
+    macro_rules! tracked_no_crate_hash {
+        ($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_non_crate_hash_different(&reference, &opts);
+        };
+    }
+    tracked_no_crate_hash!(no_codegen, true);
 }
 
 #[test]
diff --git a/compiler/rustc_metadata/src/rmeta/decoder.rs b/compiler/rustc_metadata/src/rmeta/decoder.rs
@@ -601,10 +601,23 @@ impl MetadataBlob {
     }
 
     crate fn list_crate_metadata(&self, out: &mut dyn io::Write) -> io::Result<()> {
-        write!(out, "=External Dependencies=\n")?;
         let root = self.get_root();
+        writeln!(out, "Crate info:")?;
+        writeln!(out, "name {}{}", root.name, root.extra_filename)?;
+        writeln!(out, "hash {} stable_crate_id {:?}", root.hash, root.stable_crate_id)?;
+        writeln!(out, "proc_macro {:?}", root.proc_macro_data.is_some())?;
+        writeln!(out, "=External Dependencies=")?;
         for (i, dep) in root.crate_deps.decode(self).enumerate() {
-            write!(out, "{} {}{}\n", i + 1, dep.name, dep.extra_filename)?;
+            writeln!(
+                out,
+                "{} {}{} hash {} host_hash {:?} kind {:?}",
+                i + 1,
+                dep.name,
+                dep.extra_filename,
+                dep.hash,
+                dep.host_hash,
+                dep.kind
+            )?;
         }
         write!(out, "\n")?;
         Ok(())
diff --git a/compiler/rustc_session/src/config.rs b/compiler/rustc_session/src/config.rs
@@ -31,6 +31,7 @@ use std::collections::btree_map::{
 };
 use std::collections::{BTreeMap, BTreeSet};
 use std::fmt;
+use std::hash::Hash;
 use std::iter::{self, FromIterator};
 use std::path::{Path, PathBuf};
 use std::str::{self, FromStr};
@@ -325,12 +326,11 @@ impl Default for TrimmedDefPaths {
 
 /// Use tree-based collections to cheaply get a deterministic `Hash` implementation.
 /// *Do not* switch `BTreeMap` out for an unsorted container type! That would break
-/// dependency tracking for command-line arguments.
-#[derive(Clone, Hash, Debug)]
+/// dependency tracking for command-line arguments. Also only hash keys, since tracking
+/// should only depend on the output types, not the paths they're written to.
+#[derive(Clone, Debug, Hash)]
 pub struct OutputTypes(BTreeMap<OutputType, Option<PathBuf>>);
 
-impl_stable_hash_via_hash!(OutputTypes);
-
 impl OutputTypes {
     pub fn new(entries: &[(OutputType, Option<PathBuf>)]) -> OutputTypes {
         OutputTypes(BTreeMap::from_iter(entries.iter().map(|&(k, ref v)| (k, v.clone()))))
@@ -2426,8 +2426,8 @@ crate mod dep_tracking {
     use super::LdImpl;
     use super::{
         CFGuard, CrateType, DebugInfo, ErrorOutputType, InstrumentCoverage, LinkerPluginLto,
-        LtoCli, OptLevel, OutputTypes, Passes, SourceFileHashAlgorithm, SwitchWithOptPath,
-        SymbolManglingVersion, TrimmedDefPaths,
+        LtoCli, OptLevel, OutputType, OutputTypes, Passes, SourceFileHashAlgorithm,
+        SwitchWithOptPath, SymbolManglingVersion, TrimmedDefPaths,
     };
     use crate::lint;
     use crate::options::WasiExecModel;
@@ -2443,25 +2443,35 @@ crate mod dep_tracking {
     use std::path::PathBuf;
 
     pub trait DepTrackingHash {
-        fn hash(&self, hasher: &mut DefaultHasher, error_format: ErrorOutputType);
+        fn hash(
+            &self,
+            hasher: &mut DefaultHasher,
+            error_format: ErrorOutputType,
+            for_crate_hash: bool,
+        );
     }
 
     macro_rules! impl_dep_tracking_hash_via_hash {
         ($($t:ty),+ $(,)?) => {$(
             impl DepTrackingHash for $t {
-                fn hash(&self, hasher: &mut DefaultHasher, _: ErrorOutputType) {
+                fn hash(&self, hasher: &mut DefaultHasher, _: ErrorOutputType, _for_crate_hash: bool) {
                     Hash::hash(self, hasher);
                 }
             }
         )+};
     }
 
     impl<T: DepTrackingHash> DepTrackingHash for Option<T> {
-        fn hash(&self, hasher: &mut DefaultHasher, error_format: ErrorOutputType) {
+        fn hash(
+            &self,
+            hasher: &mut DefaultHasher,
+            error_format: ErrorOutputType,
+            for_crate_hash: bool,
+        ) {
             match self {
                 Some(x) => {
                     Hash::hash(&1, hasher);
-                    DepTrackingHash::hash(x, hasher, error_format);
+                    DepTrackingHash::hash(x, hasher, error_format, for_crate_hash);
                 }
                 None => Hash::hash(&0, hasher),
             }
@@ -2491,7 +2501,6 @@ crate mod dep_tracking {
         LtoCli,
         DebugInfo,
         UnstableFeatures,
-        OutputTypes,
         NativeLib,
         NativeLibKind,
         SanitizerSet,
@@ -2505,18 +2514,24 @@ crate mod dep_tracking {
         SourceFileHashAlgorithm,
         TrimmedDefPaths,
         Option<LdImpl>,
+        OutputType,
     );
 
     impl<T1, T2> DepTrackingHash for (T1, T2)
     where
         T1: DepTrackingHash,
         T2: DepTrackingHash,
     {
-        fn hash(&self, hasher: &mut DefaultHasher, error_format: ErrorOutputType) {
+        fn hash(
+            &self,
+            hasher: &mut DefaultHasher,
+            error_format: ErrorOutputType,
+            for_crate_hash: bool,
+        ) {
             Hash::hash(&0, hasher);
-            DepTrackingHash::hash(&self.0, hasher, error_format);
+            DepTrackingHash::hash(&self.0, hasher, error_format, for_crate_hash);
             Hash::hash(&1, hasher);
-            DepTrackingHash::hash(&self.1, hasher, error_format);
+            DepTrackingHash::hash(&self.1, hasher, error_format, for_crate_hash);
         }
     }
 
@@ -2526,22 +2541,49 @@ crate mod dep_tracking {
         T2: DepTrackingHash,
         T3: DepTrackingHash,
     {
-        fn hash(&self, hasher: &mut DefaultHasher, error_format: ErrorOutputType) {
+        fn hash(
+            &self,
+            hasher: &mut DefaultHasher,
+            error_format: ErrorOutputType,
+            for_crate_hash: bool,
+        ) {
             Hash::hash(&0, hasher);
-            DepTrackingHash::hash(&self.0, hasher, error_format);
+            DepTrackingHash::hash(&self.0, hasher, error_format, for_crate_hash);
             Hash::hash(&1, hasher);
-            DepTrackingHash::hash(&self.1, hasher, error_format);
+            DepTrackingHash::hash(&self.1, hasher, error_format, for_crate_hash);
             Hash::hash(&2, hasher);
-            DepTrackingHash::hash(&self.2, hasher, error_format);
+            DepTrackingHash::hash(&self.2, hasher, error_format, for_crate_hash);
         }
     }
 
     impl<T: DepTrackingHash> DepTrackingHash for Vec<T> {
-        fn hash(&self, hasher: &mut DefaultHasher, error_format: ErrorOutputType) {
+        fn hash(
+            &self,
+            hasher: &mut DefaultHasher,
+            error_format: ErrorOutputType,
+            for_crate_hash: bool,
+        ) {
             Hash::hash(&self.len(), hasher);
             for (index, elem) in self.iter().enumerate() {
                 Hash::hash(&index, hasher);
-                DepTrackingHash::hash(elem, hasher, error_format);
+                DepTrackingHash::hash(elem, hasher, error_format, for_crate_hash);
+            }
+        }
+    }
+
+    impl DepTrackingHash for OutputTypes {
+        fn hash(
+            &self,
+            hasher: &mut DefaultHasher,
+            error_format: ErrorOutputType,
+            for_crate_hash: bool,
+        ) {
+            Hash::hash(&self.0.len(), hasher);
+            for (key, val) in &self.0 {
+                DepTrackingHash::hash(key, hasher, error_format, for_crate_hash);
+                if !for_crate_hash {
+                    DepTrackingHash::hash(val, hasher, error_format, for_crate_hash);
+                }
             }
         }
     }
@@ -2551,13 +2593,14 @@ crate mod dep_tracking {
         sub_hashes: BTreeMap<&'static str, &dyn DepTrackingHash>,
         hasher: &mut DefaultHasher,
         error_format: ErrorOutputType,
+        for_crate_hash: bool,
     ) {
         for (key, sub_hash) in sub_hashes {
             // Using Hash::hash() instead of DepTrackingHash::hash() is fine for
             // the keys, as they are just plain strings
             Hash::hash(&key.len(), hasher);
             Hash::hash(key, hasher);
-            sub_hash.hash(hasher, error_format);
+            sub_hash.hash(hasher, error_format, for_crate_hash);
         }
     }
 }
diff --git a/compiler/rustc_session/src/options.rs b/compiler/rustc_session/src/options.rs
@@ -48,7 +48,11 @@ macro_rules! hash_substruct {
     ($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);
+        $opt_expr.dep_tracking_hash($for_crate_hash, $error_format).hash(
+            $hasher,
+            $error_format,
+            $for_crate_hash,
+        );
     };
 }
 
@@ -79,7 +83,8 @@ macro_rules! top_level_options {
                 let mut hasher = DefaultHasher::new();
                 dep_tracking::stable_hash(sub_hashes,
                                           &mut hasher,
-                                          self.error_format);
+                                          self.error_format,
+                                          for_crate_hash);
                 $({
                     hash_substruct!($opt,
                         &self.$opt,
@@ -236,19 +241,21 @@ macro_rules! options {
             build_options(matches, $stat, $prefix, $outputname, error_format)
         }
 
-        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();
             dep_tracking::stable_hash(sub_hashes,
                                         &mut hasher,
-                                        error_format);
+                                        error_format,
+                                        for_crate_hash
+                                        );
             hasher.finish()
         }
     }
@@ -1148,7 +1155,7 @@ options! {
         "the directory the NLL facts are dumped into (default: `nll-facts`)"),
     no_analysis: bool = (false, parse_no_flag, [UNTRACKED],
         "parse and expand the source, but run no analysis"),
-    no_codegen: bool = (false, parse_no_flag, [TRACKED],
+    no_codegen: bool = (false, parse_no_flag, [TRACKED_NO_CRATE_HASH],
         "run all passes except codegen; no output"),
     no_generate_arange_section: bool = (false, parse_no_flag, [TRACKED],
         "omit DWARF address ranges that give faster lookups"),
diff --git a/src/test/run-make/emit-path-unhashed/Makefile b/src/test/run-make/emit-path-unhashed/Makefile
@@ -0,0 +1,37 @@
+-include ../../run-make-fulldeps/tools.mk
+
+OUT=$(TMPDIR)/emit
+
+# --emit KIND=PATH should not affect crate hash vs --emit KIND
+all: $(OUT)/a/libfoo.rlib $(OUT)/b/libfoo.rlib $(OUT)/c/libfoo.rlib \
+		$(TMPDIR)/libfoo.rlib
+	$(RUSTC) -Zls $(TMPDIR)/libfoo.rlib > $(TMPDIR)/base.txt
+	$(RUSTC) -Zls $(OUT)/a/libfoo.rlib > $(TMPDIR)/a.txt
+	$(RUSTC) -Zls $(OUT)/b/libfoo.rlib > $(TMPDIR)/b.txt
+	$(RUSTC) -Zls $(OUT)/c/libfoo.rlib > $(TMPDIR)/c.txt
+
+	diff $(TMPDIR)/base.txt $(TMPDIR)/a.txt
+	diff $(TMPDIR)/base.txt $(TMPDIR)/b.txt
+
+	# Different KIND parameters do affect hash.
+	# diff exits 1 on difference, 2 on trouble
+	diff $(TMPDIR)/base.txt $(TMPDIR)/c.txt ; test "$$?" -eq 1
+
+# Default output name
+$(TMPDIR)/libfoo.rlib: foo.rs
+	$(RUSTC) --emit link foo.rs
+
+# Output named with -o
+$(OUT)/a/libfoo.rlib: foo.rs
+	mkdir -p $(OUT)/a
+	$(RUSTC) --emit link -o $@ foo.rs
+
+# Output named with KIND=PATH
+$(OUT)/b/libfoo.rlib: foo.rs
+	mkdir -p $(OUT)/b
+	$(RUSTC) --emit link=$@ foo.rs
+
+# Output multiple kinds
+$(OUT)/c/libfoo.rlib: foo.rs
+	mkdir -p $(OUT)/c
+	$(RUSTC) --emit link=$@,metadata foo.rs
diff --git a/src/test/run-make/emit-path-unhashed/foo.rs b/src/test/run-make/emit-path-unhashed/foo.rs
@@ -0,0 +1 @@
+#![crate_type = "rlib"]
