Skip to content

Commit

Permalink
Auto merge of #8657 - ehuss:fix-doctest-lto, r=alexcrichton
Browse files Browse the repository at this point in the history
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 rust-lang/rust#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
  • Loading branch information
bors committed Aug 28, 2020
2 parents 888ae72 + 5bbad10 commit 55e59bd
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 34 deletions.
3 changes: 2 additions & 1 deletion src/cargo/core/compiler/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
38 changes: 18 additions & 20 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(&lto_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));
}

Expand Down Expand Up @@ -946,6 +927,23 @@ fn build_base_args(
Ok(())
}

fn lto_args(cx: &Context<'_, '_>, unit: &Unit) -> Vec<OsString> {
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<'_, '_>,
Expand Down
7 changes: 2 additions & 5 deletions src/cargo/core/profiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand All @@ -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
Expand Down
73 changes: 65 additions & 8 deletions tests/testsuite/lto.rs
Original file line number Diff line number Diff line change
@@ -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]
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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] [..]
Expand Down Expand Up @@ -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] [..]
Expand All @@ -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();
}

0 comments on commit 55e59bd

Please sign in to comment.