From d1ff5779e1671d067a7880a2e5acc3a27e3b131c Mon Sep 17 00:00:00 2001 From: bors Date: Fri, 28 Aug 2020 15:39:13 +0000 Subject: [PATCH] Auto merge of #8657 - ehuss:fix-doctest-lto, r=alexcrichton Fix LTO with doctests. This fixes an issue where `cargo test --release` would fail to run doctests if LTO is set in `profile.release` or `profile.bench`. The issue is that dependencies were built with `-Clinker-plugin-lto`, but the final rustdoc invocation did not issue `-C lto`. This causes a link failure (or crash!) because the necessary object code was missing. This is because rustdoc historically did not support codegen flags, so Cargo has never passed them in. Rustdoc now supports codegen flags (via https://github.com/rust-lang/rust/pull/63827), so it should be safe to start passing them in. For now, I am only adding LTO, but more should be added in the future. There are two bugs here. One is that LTO flags aren't passed to rustdoc. The other is that the "doctest" unit was using the wrong profile (it was using dev/release when it should be using test/bench). There are two distinct scenarios here. One where just `release` has `lto` set. And one where both `release` and `bench` have `lto` set. This is relevant because LTO mostly cares about what the final artifact wants, and in the case where `bench` does not have `lto` set, then LTO will not be used for tests. This will hopefully be a little cleaner in the future when #6988 is stabilized, which causes the test/bench profiles to *inherit* from the dev/bench profiles, which means you won't need to manually synchronize the test/bench profiles with dev/release. Fixes #8654 --- src/cargo/core/compiler/context/mod.rs | 3 +- src/cargo/core/compiler/mod.rs | 38 +++++++------- src/cargo/core/profiles.rs | 7 +-- tests/testsuite/lto.rs | 73 +++++++++++++++++++++++--- 4 files changed, 87 insertions(+), 34 deletions(-) diff --git a/src/cargo/core/compiler/context/mod.rs b/src/cargo/core/compiler/context/mod.rs index 52b954dd12d..82831f42314 100644 --- a/src/cargo/core/compiler/context/mod.rs +++ b/src/cargo/core/compiler/context/mod.rs @@ -210,7 +210,8 @@ impl<'a, 'cfg> Context<'a, 'cfg> { // Collect information for `rustdoc --test`. if unit.mode.is_doc_test() { let mut unstable_opts = false; - let args = compiler::extern_args(&self, unit, &mut unstable_opts)?; + let mut args = compiler::extern_args(&self, unit, &mut unstable_opts)?; + args.extend(compiler::lto_args(&self, unit)); self.compilation.to_doc_test.push(compilation::Doctest { unit: unit.clone(), args, diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 97012d2ee16..5c118454979 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -797,28 +797,9 @@ fn build_base_args( cmd.arg("-C").arg(format!("panic={}", panic)); } - match cx.lto[unit] { - lto::Lto::Run(None) => { - cmd.arg("-C").arg("lto"); - } - lto::Lto::Run(Some(s)) => { - cmd.arg("-C").arg(format!("lto={}", s)); - } - lto::Lto::Off => { - cmd.arg("-C").arg("lto=off"); - } - lto::Lto::ObjectAndBitcode => {} // this is rustc's default - lto::Lto::OnlyBitcode => { - cmd.arg("-C").arg("linker-plugin-lto"); - } - lto::Lto::OnlyObject => { - cmd.arg("-C").arg("embed-bitcode=no"); - } - } + cmd.args(<o_args(cx, unit)); if let Some(n) = codegen_units { - // There are some restrictions with LTO and codegen-units, so we - // only add codegen units when LTO is not used. cmd.arg("-C").arg(&format!("codegen-units={}", n)); } @@ -946,6 +927,23 @@ fn build_base_args( Ok(()) } +fn lto_args(cx: &Context<'_, '_>, unit: &Unit) -> Vec { + let mut result = Vec::new(); + let mut push = |arg: &str| { + result.push(OsString::from("-C")); + result.push(OsString::from(arg)); + }; + match cx.lto[unit] { + lto::Lto::Run(None) => push("lto"), + lto::Lto::Run(Some(s)) => push(&format!("lto={}", s)), + lto::Lto::Off => push("lto=off"), + lto::Lto::ObjectAndBitcode => {} // this is rustc's default + lto::Lto::OnlyBitcode => push("linker-plugin-lto"), + lto::Lto::OnlyObject => push("embed-bitcode=no"), + } + result +} + fn build_deps_args( cmd: &mut ProcessBuilder, cx: &mut Context<'_, '_>, diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index 6bae0337f7c..d4859cb2078 100644 --- a/src/cargo/core/profiles.rs +++ b/src/cargo/core/profiles.rs @@ -299,7 +299,7 @@ impl Profiles { let release = matches!(self.requested_profile.as_str(), "release" | "bench"); match mode { - CompileMode::Test | CompileMode::Bench => { + CompileMode::Test | CompileMode::Bench | CompileMode::Doctest => { if release { ( InternedString::new("bench"), @@ -312,10 +312,7 @@ impl Profiles { ) } } - CompileMode::Build - | CompileMode::Check { .. } - | CompileMode::Doctest - | CompileMode::RunCustomBuild => { + CompileMode::Build | CompileMode::Check { .. } | CompileMode::RunCustomBuild => { // Note: `RunCustomBuild` doesn't normally use this code path. // `build_unit_profiles` normally ensures that it selects the // ancestor's profile. However, `cargo clean -p` can hit this diff --git a/tests/testsuite/lto.rs b/tests/testsuite/lto.rs index 9488b271b20..87524c4815a 100644 --- a/tests/testsuite/lto.rs +++ b/tests/testsuite/lto.rs @@ -1,6 +1,6 @@ use cargo::core::compiler::Lto; use cargo_test_support::registry::Package; -use cargo_test_support::{project, Project}; +use cargo_test_support::{basic_manifest, project, Project}; use std::process::Output; #[cargo_test] @@ -514,16 +514,19 @@ fn cdylib_and_rlib() { p.cargo("test --release -v --manifest-path bar/Cargo.toml") .with_stderr_unordered( "\ -[FRESH] registry v0.0.1 -[FRESH] registry-shared v0.0.1 +[COMPILING] registry v0.0.1 +[COMPILING] registry-shared v0.0.1 +[RUNNING] `rustc --crate-name registry [..]-C embed-bitcode=no[..] +[RUNNING] `rustc --crate-name registry_shared [..]-C embed-bitcode=no[..] [COMPILING] bar [..] +[RUNNING] `rustc --crate-name bar [..]--crate-type cdylib --crate-type rlib [..]-C embed-bitcode=no[..] [RUNNING] `rustc --crate-name bar [..]-C embed-bitcode=no --test[..] [RUNNING] `rustc --crate-name b [..]-C embed-bitcode=no --test[..] [FINISHED] [..] -[RUNNING] [..] -[RUNNING] [..] +[RUNNING] [..]target/release/deps/bar-[..] +[RUNNING] [..]target/release/deps/b-[..] [DOCTEST] bar -[RUNNING] `rustdoc --crate-type cdylib --crate-type rlib --test [..] +[RUNNING] `rustdoc --crate-type cdylib --crate-type rlib --test [..]-C embed-bitcode=no[..] ", ) .run(); @@ -627,7 +630,7 @@ fn test_profile() { [COMPILING] bar v0.0.1 [RUNNING] `rustc --crate-name bar [..]crate-type lib[..] [COMPILING] foo [..] -[RUNNING] `rustc --crate-name foo [..]--crate-type lib --emit=dep-info,metadata,link -C embed-bitcode=no[..] +[RUNNING] `rustc --crate-name foo [..]--crate-type lib --emit=dep-info,metadata,link -C linker-plugin-lto[..] [RUNNING] `rustc --crate-name foo [..]--emit=dep-info,link -C lto=thin [..]--test[..] [FINISHED] [..] [RUNNING] [..] @@ -680,7 +683,7 @@ fn dev_profile() { [COMPILING] bar v0.0.1 [RUNNING] `rustc --crate-name bar [..]crate-type lib[..] [COMPILING] foo [..] -[RUNNING] `rustc --crate-name foo [..]--crate-type lib --emit=dep-info,metadata,link -C linker-plugin-lto [..] +[RUNNING] `rustc --crate-name foo [..]--crate-type lib --emit=dep-info,metadata,link -C embed-bitcode=no [..] [RUNNING] `rustc --crate-name foo [..]--emit=dep-info,link -C embed-bitcode=no [..]--test[..] [FINISHED] [..] [RUNNING] [..] @@ -689,3 +692,57 @@ fn dev_profile() { ") .run(); } + +#[cargo_test] +fn doctest() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + edition = "2018" + + [profile.release] + lto = true + + [dependencies] + bar = { path = "bar" } + "#, + ) + .file( + "src/lib.rs", + r#" + /// Foo! + /// + /// ``` + /// foo::foo(); + /// ``` + pub fn foo() { bar::bar(); } + "#, + ) + .file("bar/Cargo.toml", &basic_manifest("bar", "0.1.0")) + .file( + "bar/src/lib.rs", + r#" + pub fn bar() { println!("hi!"); } + "#, + ) + .build(); + + p.cargo("test --doc --release -v") + .with_stderr_contains("[..]`rustc --crate-name bar[..]-C embed-bitcode=no[..]") + .with_stderr_contains("[..]`rustc --crate-name foo[..]-C embed-bitcode=no[..]") + // embed-bitcode should be harmless here + .with_stderr_contains("[..]`rustdoc [..]-C embed-bitcode=no[..]") + .run(); + + // Try with bench profile. + p.cargo("test --doc --release -v") + .env("CARGO_PROFILE_BENCH_LTO", "true") + .with_stderr_contains("[..]`rustc --crate-name bar[..]-C linker-plugin-lto[..]") + .with_stderr_contains("[..]`rustc --crate-name foo[..]-C linker-plugin-lto[..]") + .with_stderr_contains("[..]`rustdoc [..]-C lto[..]") + .run(); +}