Auto merge of #9418 - ehuss:always-meta, r=alexcrichton

Always use full metadata hash for -C metadata.

This changes it so that cargo always uses the full metadata hash for `-C metadata`. This ensures that even if a unit isn't using `-C extra-filename` that the symbol hashing uses all the fields used in the other cases.

This fixes an issue on macOS where a combination of split-debuginfo and incremental caused the same `.o` filenames to be used, which caused corruption in the incremental cache (see issue for details).

Fixes #9353.

Author: bors

src/cargo/core/compiler/context/compilation_files.rs

@@ -80,6 +80,17 @@ impl fmt::Debug for Metadata {
     }
 }
 
+/// Information about the metadata hashes used for a `Unit`.
+struct MetaInfo {
+    /// The symbol hash to use.
+    meta_hash: Metadata,
+    /// Whether or not the `-C extra-filename` flag is used to generate unique
+    /// output filenames for this `Unit`.
+    ///
+    /// If this is `true`, the `meta_hash` is used for the filename.
+    use_extra_filename: bool,
+}
+
 /// Collection of information about the files emitted by the compiler, and the
 /// output directory structure.
 pub struct CompilationFiles<'a, 'cfg> {
@@ -94,7 +105,7 @@ pub struct CompilationFiles<'a, 'cfg> {
     roots: Vec<Unit>,
     ws: &'a Workspace<'cfg>,
     /// Metadata hash to use for each unit.
-    metas: HashMap<Unit, Option<Metadata>>,
+    metas: HashMap<Unit, MetaInfo>,
     /// For each Unit, a list all files produced.
     outputs: HashMap<Unit, LazyCell<Arc<Vec<OutputFile>>>>,
 }
@@ -160,11 +171,14 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
     /// Gets the metadata for the given unit.
     ///
     /// See module docs for more details.
-    ///
-    /// Returns `None` if the unit should not use a metadata hash (like
-    /// rustdoc, or some dylibs).
-    pub fn metadata(&self, unit: &Unit) -> Option<Metadata> {
-        self.metas[unit]
+    pub fn metadata(&self, unit: &Unit) -> Metadata {
+        self.metas[unit].meta_hash
+    }
+
+    /// Returns whether or not `-C extra-filename` is used to extend the
+    /// output filenames to make them unique.
+    pub fn use_extra_filename(&self, unit: &Unit) -> bool {
+        self.metas[unit].use_extra_filename
     }
 
     /// Gets the short hash based only on the `PackageId`.
@@ -201,9 +215,11 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
     /// taken in those cases!
     fn pkg_dir(&self, unit: &Unit) -> String {
         let name = unit.pkg.package_id().name();
-        match self.metas[unit] {
-            Some(ref meta) => format!("{}-{}", name, meta),
-            None => format!("{}-{}", name, self.target_short_hash(unit)),
+        let meta = &self.metas[unit];
+        if meta.use_extra_filename {
+            format!("{}-{}", name, meta.meta_hash)
+        } else {
+            format!("{}-{}", name, self.target_short_hash(unit))
         }
     }
 
@@ -448,8 +464,9 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
         // Convert FileType to OutputFile.
         let mut outputs = Vec::new();
         for file_type in file_types {
-            let meta = self.metadata(unit).map(|m| m.to_string());
-            let path = out_dir.join(file_type.output_filename(&unit.target, meta.as_deref()));
+            let meta = &self.metas[unit];
+            let meta_opt = meta.use_extra_filename.then(|| meta.meta_hash.to_string());
+            let path = out_dir.join(file_type.output_filename(&unit.target, meta_opt.as_deref()));
             let hardlink = self.uplift_to(unit, &file_type, &path);
             let export_path = if unit.target.is_custom_build() {
                 None
@@ -471,30 +488,27 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
     }
 }
 
-fn metadata_of(
+fn metadata_of<'a>(
     unit: &Unit,
     cx: &Context<'_, '_>,
-    metas: &mut HashMap<Unit, Option<Metadata>>,
-) -> Option<Metadata> {
+    metas: &'a mut HashMap<Unit, MetaInfo>,
+) -> &'a MetaInfo {
     if !metas.contains_key(unit) {
         let meta = compute_metadata(unit, cx, metas);
         metas.insert(unit.clone(), meta);
         for dep in cx.unit_deps(unit) {
             metadata_of(&dep.unit, cx, metas);
         }
     }
-    metas[unit]
+    &metas[unit]
 }
 
 fn compute_metadata(
     unit: &Unit,
     cx: &Context<'_, '_>,
-    metas: &mut HashMap<Unit, Option<Metadata>>,
-) -> Option<Metadata> {
+    metas: &mut HashMap<Unit, MetaInfo>,
+) -> MetaInfo {
     let bcx = &cx.bcx;
-    if !should_use_metadata(bcx, unit) {
-        return None;
-    }
     let mut hasher = StableHasher::new();
 
     METADATA_VERSION.hash(&mut hasher);
@@ -514,7 +528,7 @@ fn compute_metadata(
     let mut deps_metadata = cx
         .unit_deps(unit)
         .iter()
-        .map(|dep| metadata_of(&dep.unit, cx, metas))
+        .map(|dep| metadata_of(&dep.unit, cx, metas).meta_hash)
         .collect::<Vec<_>>();
     deps_metadata.sort();
     deps_metadata.hash(&mut hasher);
@@ -561,7 +575,10 @@ fn compute_metadata(
     // with user dependencies.
     unit.is_std.hash(&mut hasher);
 
-    Some(Metadata(hasher.finish()))
+    MetaInfo {
+        meta_hash: Metadata(hasher.finish()),
+        use_extra_filename: should_use_metadata(bcx, unit),
+    }
 }
 
 fn hash_rustc_version(bcx: &BuildContext<'_, '_>, hasher: &mut StableHasher) {

src/cargo/core/compiler/context/mod.rs

@@ -391,9 +391,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
     /// Returns the metadata hash for a RunCustomBuild unit.
     pub fn get_run_build_script_metadata(&self, unit: &Unit) -> Metadata {
         assert!(unit.mode.is_run_custom_build());
-        self.files()
-            .metadata(unit)
-            .expect("build script should always have hash")
+        self.files().metadata(unit)
     }
 
     pub fn is_primary_package(&self, unit: &Unit) -> bool {

src/cargo/core/compiler/mod.rs

@@ -229,9 +229,14 @@ fn rustc(cx: &mut Context<'_, '_>, unit: &Unit, exec: &Arc<dyn Executor>) -> Car
     let pass_l_flag = unit.target.is_lib() || !unit.pkg.targets().iter().any(|t| t.is_lib());
     let link_type = (&unit.target).into();
 
-    let dep_info_name = match cx.files().metadata(unit) {
-        Some(metadata) => format!("{}-{}.d", unit.target.crate_name(), metadata),
-        None => format!("{}.d", unit.target.crate_name()),
+    let dep_info_name = if cx.files().use_extra_filename(unit) {
+        format!(
+            "{}-{}.d",
+            unit.target.crate_name(),
+            cx.files().metadata(unit)
+        )
+    } else {
+        format!("{}.d", unit.target.crate_name())
     };
     let rustc_dep_info_loc = root.join(dep_info_name);
     let dep_info_loc = fingerprint::dep_info_loc(cx, unit);
@@ -881,15 +886,10 @@ fn build_base_args(
         cmd.arg("--cfg").arg(&format!("feature=\"{}\"", feat));
     }
 
-    match cx.files().metadata(unit) {
-        Some(m) => {
-            cmd.arg("-C").arg(&format!("metadata={}", m));
-            cmd.arg("-C").arg(&format!("extra-filename=-{}", m));
-        }
-        None => {
-            cmd.arg("-C")
-                .arg(&format!("metadata={}", cx.files().target_short_hash(unit)));
-        }
+    let meta = cx.files().metadata(unit);
+    cmd.arg("-C").arg(&format!("metadata={}", meta));
+    if cx.files().use_extra_filename(unit) {
+        cmd.arg("-C").arg(&format!("extra-filename=-{}", meta));
     }
 
     if rpath {

tests/testsuite/old_cargos.rs

@@ -587,3 +587,60 @@ foo v0.1.0 [..]
         )
         .run();
 }
+
+#[cargo_test]
+#[ignore]
+fn avoids_split_debuginfo_collision() {
+    // Checks for a bug where .o files were being incorrectly shared between
+    // different toolchains using incremental and split-debuginfo on macOS.
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+
+                [profile.dev]
+                split-debuginfo = "unpacked"
+            "#,
+        )
+        .file("src/main.rs", "fn main() {}")
+        .build();
+
+    execs()
+        .with_process_builder(tc_process("cargo", "stable"))
+        .arg("build")
+        .env("CARGO_INCREMENTAL", "1")
+        .cwd(p.root())
+        .with_stderr(
+            "\
+[COMPILING] foo v0.1.0 [..]
+[FINISHED] [..]
+",
+        )
+        .run();
+
+    p.cargo("build")
+        .env("CARGO_INCREMENTAL", "1")
+        .with_stderr(
+            "\
+[COMPILING] foo v0.1.0 [..]
+[FINISHED] [..]
+",
+        )
+        .run();
+
+    execs()
+        .with_process_builder(tc_process("cargo", "stable"))
+        .arg("build")
+        .env("CARGO_INCREMENTAL", "1")
+        .cwd(p.root())
+        .with_stderr(
+            "\
+[COMPILING] foo v0.1.0 [..]
+[FINISHED] [..]
+",
+        )
+        .run();
+}