Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always use full metadata hash for -C metadata. #9418

Merged
merged 1 commit into from
Apr 27, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 39 additions & 22 deletions src/cargo/core/compiler/context/compilation_files.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand All @@ -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>>>>,
}
Expand Down Expand Up @@ -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`.
Expand Down Expand Up @@ -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))
}
}

Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down
4 changes: 1 addition & 3 deletions src/cargo/core/compiler/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
24 changes: 12 additions & 12 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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 {
Expand Down
57 changes: 57 additions & 0 deletions tests/testsuite/old_cargos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}