From 4bb68828de9c424c572a7ec11417660478ca7501 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Sat, 18 Jan 2020 14:08:11 -0500 Subject: [PATCH 1/2] Read metadata from rmeta exclusively, if possible When we're producing an rlib, we do not need anything more than an rmeta file for each of our dependencies (this is indeed utilized by Cargo for pipelining). Previously, we were still storing the paths of possible rlib/dylib crates, which meant that they could still plausibly be accessed. With -Zbinary-dep-depinfo, that meant that Cargo thought that rustc was using both the rlib and an (earlier emitted) rmeta, and so needed a recompile, as the rlib may have finished writing *after* compilation started (for more detail, see issue 68149). This commit changes metadata loading to not store the filepaths of dylib/rlib if we're going to end up creating an rlib only. --- src/librustc_metadata/locator.rs | 44 +++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/src/librustc_metadata/locator.rs b/src/librustc_metadata/locator.rs index 4745ad02a3aa4..75182e32ae5b9 100644 --- a/src/librustc_metadata/locator.rs +++ b/src/librustc_metadata/locator.rs @@ -656,14 +656,36 @@ impl<'a> CrateLocator<'a> { dylibs: FxHashMap, ) -> Option<(Svh, Library)> { let mut slot = None; + // Order here matters, rmeta should come first. See comment in + // `extract_one` below. let source = CrateSource { - rlib: self.extract_one(rlibs, CrateFlavor::Rlib, &mut slot), rmeta: self.extract_one(rmetas, CrateFlavor::Rmeta, &mut slot), + rlib: self.extract_one(rlibs, CrateFlavor::Rlib, &mut slot), dylib: self.extract_one(dylibs, CrateFlavor::Dylib, &mut slot), }; slot.map(|(svh, metadata)| (svh, Library { source, metadata })) } + fn needs_crate_flavor(&self, flavor: CrateFlavor) -> bool { + if flavor == CrateFlavor::Dylib && self.is_proc_macro == Some(true) { + return true; + } + + // The all loop is because `--crate-type=rlib --crate-type=rlib` is + // legal and produces both inside this type. + let is_rlib = self.sess.crate_types.borrow().iter().all(|c| *c == config::CrateType::Rlib); + let needs_object_code = self.sess.opts.output_types.should_codegen(); + // If we're producing an rlib, then we don't need object code. + // Or, if we're not producing object code, then we don't need it either + // (e.g., if we're a cdylib but emitting just metadata). + if is_rlib || !needs_object_code { + flavor == CrateFlavor::Rmeta + } else { + // we need all flavors (perhaps not true, but what we do for now) + true + } + } + // Attempts to extract *one* library from the set `m`. If the set has no // elements, `None` is returned. If the set has more than one element, then // the errors and notes are emitted about the set of libraries. @@ -681,12 +703,22 @@ impl<'a> CrateLocator<'a> { let mut ret: Option<(PathBuf, PathKind)> = None; let mut error = 0; + // If we are producing an rlib, and we've already loaded metadata, then + // we should not attempt to discover further crate sources (unless we're + // locating a proc macro; exact logic is in needs_crate_flavor). This means + // that under -Zbinary-dep-depinfo we will not emit a dependency edge on + // the *unused* rlib, and by returning `None` here immediately we + // guarantee that we do indeed not use it. + // + // See also #68149 which provides more detail on why emitting the + // dependency on the rlib is a bad thing. + // + // We currenty do not verify that these other sources are even in sync, + // and this is arguably a bug (see #10786), but because reading metadata + // is quite slow (especially from dylibs) we currently do not read it + // from the other crate sources. if slot.is_some() { - // FIXME(#10786): for an optimization, we only read one of the - // libraries' metadata sections. In theory we should - // read both, but reading dylib metadata is quite - // slow. - if m.is_empty() { + if m.is_empty() || !self.needs_crate_flavor(flavor) { return None; } else if m.len() == 1 { return Some(m.into_iter().next().unwrap()); From be663bf850fcdcedc678782e5e0945124d5791fb Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Mon, 20 Jan 2020 15:35:43 -0500 Subject: [PATCH 2/2] Correct rmeta/rlib test --- src/test/ui/rmeta-rpass.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/test/ui/rmeta-rpass.rs b/src/test/ui/rmeta-rpass.rs index 5a63b5b8598bc..173a6a394eb05 100644 --- a/src/test/ui/rmeta-rpass.rs +++ b/src/test/ui/rmeta-rpass.rs @@ -1,6 +1,11 @@ // run-pass // Test that using rlibs and rmeta dep crates work together. Specifically, that -// there can be both an rmeta and an rlib file and rustc will prefer the rlib. +// there can be both an rmeta and an rlib file and rustc will prefer the rmeta +// file. +// +// This behavior is simply making sure this doesn't accidentally change; in this +// case we want to make sure that the rlib isn't being used as that would cause +// bugs in -Zbinary-dep-depinfo (see #68298). // aux-build:rmeta-rmeta.rs // aux-build:rmeta-rlib-rpass.rs @@ -9,5 +14,5 @@ extern crate rmeta_aux; use rmeta_aux::Foo; pub fn main() { - let _ = Foo { field: 42 }; + let _ = Foo { field2: 42 }; }