From b45fd8ff0af26cc8f196f78bf01fc83b84a3face Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 18 May 2020 05:37:23 -0700 Subject: [PATCH] Handle LTO with an rlib/cdylib crate type In the case that LTO is happening but we're also generating a cdylib/rlib simultatneously that means that the final artifact will use the rlib but the cdylib still needs to be produced. To get this to work the cdylib's dependency tree needs to be compiled with embedded bitcode. The cdylib itself will be linked with the linker because we can't LTO an rlib+cdylib compilation, but the final executable will need to load bitcode from all its deps. This is meant to address rust-lang/rust#72268 --- src/cargo/core/compiler/lto.rs | 66 ++++++++++++++++++------------ src/cargo/core/manifest.rs | 11 ----- tests/testsuite/lto.rs | 75 ++++++++++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 37 deletions(-) diff --git a/src/cargo/core/compiler/lto.rs b/src/cargo/core/compiler/lto.rs index c808520d39b..564d6f57fc6 100644 --- a/src/cargo/core/compiler/lto.rs +++ b/src/cargo/core/compiler/lto.rs @@ -1,6 +1,7 @@ use crate::core::compiler::{Context, Unit}; use crate::core::interning::InternedString; use crate::core::profiles; +use crate::core::TargetKind; use crate::util::errors::CargoResult; use std::collections::hash_map::{Entry, HashMap}; @@ -26,7 +27,7 @@ pub enum Lto { pub fn generate(cx: &mut Context<'_, '_>) -> CargoResult<()> { let mut map = HashMap::new(); for unit in cx.bcx.roots.iter() { - calculate(cx, &mut map, unit, false)?; + calculate(cx, &mut map, unit, Lto::None)?; } cx.lto = map; Ok(()) @@ -36,34 +37,49 @@ fn calculate( cx: &Context<'_, '_>, map: &mut HashMap, unit: &Unit, - require_bitcode: bool, + lto_for_deps: Lto, ) -> CargoResult<()> { - let (lto, require_bitcode_for_deps) = if unit.target.for_host() { + let (lto, lto_for_deps) = if unit.target.for_host() { // Disable LTO for host builds since we only really want to perform LTO // for the final binary, and LTO on plugins/build scripts/proc macros is // largely not desired. - (Lto::None, false) - } else if unit.target.can_lto() { - // Otherwise if this target can perform LTO then we're going to read the - // LTO value out of the profile. Note that we ignore `require_bitcode` + (Lto::None, Lto::None) + } else if unit.target.is_linkable() { + // A "linkable" target is one that produces and rlib or dylib in this + // case. In this scenario we cannot pass `-Clto` to the compiler because + // that is an invalid request, this is simply a dependency. What we do, + // however, is respect the request for whatever dependencies need to + // have. + // + // Here if no LTO is requested then we keep it turned off. Otherwise LTO + // is requested in some form, which means ideally we need just what's + // requested, nothing else. It's possible, though, to have libraries + // which are both a cdylib and and rlib, for example, which means that + // object files are getting sent to the linker. That means that we need + // to fully embed bitcode rather than simply generating just bitcode. + let has_non_linkable_lib = match unit.target.kind() { + TargetKind::Lib(kinds) => kinds.iter().any(|k| !k.is_linkable()), + _ => true, + }; + match lto_for_deps { + Lto::None => (Lto::None, Lto::None), + _ if has_non_linkable_lib => (Lto::EmbedBitcode, Lto::EmbedBitcode), + other => (other, other), + } + } else { + // Otherwise this target can perform LTO and we're going to read the + // LTO value out of the profile. Note that we ignore `lto_for_deps` // here because if a unit depends on another unit than can LTO this // isn't a rustc-level dependency but rather a Cargo-level dependency. // For example this is an integration test depending on a binary. match unit.profile.lto { profiles::Lto::Named(s) => match s.as_str() { - "n" | "no" | "off" => (Lto::Run(Some(s)), false), - _ => (Lto::Run(Some(s)), true), + "n" | "no" | "off" => (Lto::Run(Some(s)), Lto::None), + _ => (Lto::Run(Some(s)), Lto::OnlyBitcode), }, - profiles::Lto::Bool(true) => (Lto::Run(None), true), - profiles::Lto::Bool(false) => (Lto::None, false), + profiles::Lto::Bool(true) => (Lto::Run(None), Lto::OnlyBitcode), + profiles::Lto::Bool(false) => (Lto::None, Lto::None), } - } else if require_bitcode { - // Otherwise we're a dependency of something, an rlib. This means that - // if our parent required bitcode of some kind then we need to generate - // bitcode. - (Lto::OnlyBitcode, true) - } else { - (Lto::None, false) }; match map.entry(unit.clone()) { @@ -93,14 +109,12 @@ fn calculate( // either only-objects or only-bitcode we have to embed both in // rlibs (used for different compilations), so we switch to // embedding bitcode. - (Lto::OnlyBitcode, Lto::None) - | (Lto::OnlyBitcode, Lto::EmbedBitcode) - | (Lto::None, Lto::OnlyBitcode) - | (Lto::None, Lto::EmbedBitcode) => Lto::EmbedBitcode, + (Lto::OnlyBitcode, Lto::None) | (Lto::None, Lto::OnlyBitcode) => Lto::EmbedBitcode, - // Currently this variant is never calculated above, so no need - // to handle this case. - (Lto::EmbedBitcode, _) => unreachable!(), + // Once a target has requested bitcode embedding that's the + // maximal amount of work that can be done, so we just keep + // doing that work. + (Lto::EmbedBitcode, _) | (_, Lto::EmbedBitcode) => Lto::EmbedBitcode, }; // No need to recurse if we calculated the same value as before. if result == *v.get() { @@ -111,7 +125,7 @@ fn calculate( } for dep in cx.unit_deps(unit) { - calculate(cx, map, &dep.unit, require_bitcode_for_deps)?; + calculate(cx, map, &dep.unit, lto_for_deps)?; } Ok(()) } diff --git a/src/cargo/core/manifest.rs b/src/cargo/core/manifest.rs index 221b769295b..6978081a6fa 100644 --- a/src/cargo/core/manifest.rs +++ b/src/cargo/core/manifest.rs @@ -842,17 +842,6 @@ impl Target { self.kind().rustc_crate_types() } - pub fn can_lto(&self) -> bool { - match self.kind() { - TargetKind::Lib(v) => { - !v.contains(&CrateType::Rlib) - && !v.contains(&CrateType::Dylib) - && !v.contains(&CrateType::Lib) - } - _ => true, - } - } - pub fn set_tested(&mut self, tested: bool) -> &mut Target { Arc::make_mut(&mut self.inner).tested = tested; self diff --git a/tests/testsuite/lto.rs b/tests/testsuite/lto.rs index f5e2fb5bd68..99abbdefce9 100644 --- a/tests/testsuite/lto.rs +++ b/tests/testsuite/lto.rs @@ -336,3 +336,78 @@ fn test_all_and_bench() { .with_stderr_contains("[RUNNING] `rustc[..]--crate-name foo[..]-C lto[..]") .run(); } + +#[cargo_test] +fn cdylib_and_rlib() { + if !cargo_test_support::is_nightly() { + return; + } + + Package::new("registry", "0.0.1") + .file("src/lib.rs", "pub fn foo() {}") + .publish(); + Package::new("registry-shared", "0.0.1") + .file("src/lib.rs", "pub fn foo() {}") + .publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.0" + + [workspace] + + [dependencies] + bar = { path = 'bar' } + registry-shared = "*" + + [profile.release] + lto = true + "#, + ) + .file( + "src/main.rs", + " + fn main() { + bar::foo(); + registry_shared::foo(); + } + ", + ) + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar" + version = "0.0.0" + + [dependencies] + registry = "*" + registry-shared = "*" + + [lib] + crate-type = ['cdylib', 'rlib'] + "#, + ) + .file( + "bar/src/lib.rs", + " + pub fn foo() { + registry::foo(); + registry_shared::foo(); + } + ", + ) + .file("tests/a.rs", "") + .file("bar/tests/b.rs", "") + .build(); + p.cargo("build --release -v").run(); + p.cargo("test --release -v").run(); + p.cargo("build --release -v --manifest-path bar/Cargo.toml") + .run(); + p.cargo("test --release -v --manifest-path bar/Cargo.toml") + .run(); +}