From 71c4b2453de6538960eee85c3715e41024ebe7ba Mon Sep 17 00:00:00 2001 From: Taiki Endo Date: Sat, 13 May 2023 18:08:57 +0900 Subject: [PATCH 1/2] Use llvm-args instead of RUST_TEST_THREADS --- .github/.cspell/project-dictionary.txt | 1 + CHANGELOG.md | 2 ++ README.md | 2 +- src/cli.rs | 4 ++-- src/main.rs | 8 +++++--- 5 files changed, 11 insertions(+), 6 deletions(-) diff --git a/.github/.cspell/project-dictionary.txt b/.github/.cspell/project-dictionary.txt index b248788a..aeeb2fff 100644 --- a/.github/.cspell/project-dictionary.txt +++ b/.github/.cspell/project-dictionary.txt @@ -6,6 +6,7 @@ doctestbins easytime fcoverage fprofile +instrprof libdir libhello microkernel diff --git a/CHANGELOG.md b/CHANGELOG.md index 14a8694b..11f04632 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ Note: In this file, do not use the hard wrap in the middle of a sentence for com ## [Unreleased] +- cargo-llvm-cov no longer sets the `RUST_TEST_THREADS` environment variable and uses llvm-args instead for workaround [rust-lang/rust#91092](https://github.com/rust-lang/rust/issues/91092). + ## [0.5.19] - 2023-04-28 - Fix handling of `--cargo-profile` option for `cargo llvm-cov nextest`. ([#269](https://github.com/taiki-e/cargo-llvm-cov/pull/269)) diff --git a/README.md b/README.md index 11106f7c..a029041e 100644 --- a/README.md +++ b/README.md @@ -622,7 +622,7 @@ pacman -S cargo-llvm-cov - Branch coverage is not supported yet. See [#8] and [rust-lang/rust#79649] for more. - Support for doc tests is unstable and has known issues. See [#2] and [rust-lang/rust#79417] for more. -- All the tests are run with `RUST_TEST_THREADS=1` to work around [rust-lang/rust#91092]. You can pass `--test-threads` (e.g., `--test-threads=$(nproc)`) to override this behavior. (As for `nextest`, we also set `NEXTEST_TEST_THREADS=1`.) +- `cargo llvm-cov nextest` is run with `NEXTEST_TEST_THREADS=1` to work around [rust-lang/rust#91092]. You can pass `--test-threads` (e.g., `--test-threads=$(nproc)`) to override this behavior. See also [the code-coverage-related issues reported in rust-lang/rust](https://github.com/rust-lang/rust/labels/A-code-coverage). diff --git a/src/cli.rs b/src/cli.rs index cd6febb5..5bd28418 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -358,10 +358,10 @@ impl Args { // build options Short('r') | Long("release") => parse_flag_passthrough!(release), Long("profile") if subcommand != Subcommand::Nextest => { - parse_opt_passthrough!(profile) + parse_opt_passthrough!(profile); } Long("cargo-profile") if subcommand == Subcommand::Nextest => { - parse_opt_passthrough!(profile) + parse_opt_passthrough!(profile); } Long("target") => parse_opt_passthrough!(target), Long("coverage-target-only") => parse_flag!(coverage_target_only), diff --git a/src/main.rs b/src/main.rs index 181b5e98..25ced483 100644 --- a/src/main.rs +++ b/src/main.rs @@ -190,6 +190,10 @@ fn set_env(cx: &Context, env: &mut dyn EnvTarget, IsNextest(is_nextest): IsNexte flags.push("codegen-units=1"); } } + // Workaround for https://github.com/rust-lang/rust/issues/91092 + // TODO: skip passing this on newer nightly that includes https://github.com/rust-lang/rust/pull/111469. + flags.push("-C"); + flags.push("llvm-args=--instrprof-atomic-counter-update-all"); if !cx.args.cov.no_cfg_coverage { flags.push("--cfg=coverage"); } @@ -275,7 +279,7 @@ fn set_env(cx: &Context, env: &mut dyn EnvTarget, IsNextest(is_nextest): IsNexte Err(_) => std::env::var("CXXFLAGS").unwrap_or_default(), }, }; - let clang_flags = " -fprofile-instr-generate -fcoverage-mapping"; + let clang_flags = " -fprofile-instr-generate -fcoverage-mapping -fprofile-update=atomic"; cflags.push_str(clang_flags); cxxflags.push_str(clang_flags); env.set(cflags_key, &cflags)?; @@ -283,8 +287,6 @@ fn set_env(cx: &Context, env: &mut dyn EnvTarget, IsNextest(is_nextest): IsNexte } env.set("LLVM_PROFILE_FILE", llvm_profile_file.as_str())?; env.set("CARGO_INCREMENTAL", "0")?; - // Workaround for https://github.com/rust-lang/rust/issues/91092 - env.set("RUST_TEST_THREADS", "1")?; if is_nextest { // Same as above env.set("NEXTEST_TEST_THREADS", "1")?; From ee3e3b844a3843a080906f2e73303df225224172 Mon Sep 17 00:00:00 2001 From: Taiki Endo Date: Sat, 3 Jun 2023 02:26:09 +0900 Subject: [PATCH 2/2] Use %Nm instead of NEXTEST_TEST_THREADS --- CHANGELOG.md | 4 +++- README.md | 2 -- src/main.rs | 27 +++++++++++++++++++++------ 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 11f04632..c8ff9e7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,9 @@ Note: In this file, do not use the hard wrap in the middle of a sentence for com ## [Unreleased] -- cargo-llvm-cov no longer sets the `RUST_TEST_THREADS` environment variable and uses llvm-args instead for workaround [rust-lang/rust#91092](https://github.com/rust-lang/rust/issues/91092). +- cargo-llvm-cov no longer sets the `RUST_TEST_THREADS` and `NEXTEST_TEST_THREADS` environment variables. cargo-llvm-cov now adopts another efficient way to workaround [rust-lang/rust#91092](https://github.com/rust-lang/rust/issues/91092). ([#279](https://github.com/taiki-e/cargo-llvm-cov/pull/279)) + + This may greatly improve performance, [especially when using `cargo llvm-cov nextest`](https://github.com/taiki-e/cargo-llvm-cov/pull/279#issuecomment-1552058044). ## [0.5.19] - 2023-04-28 diff --git a/README.md b/README.md index a029041e..a1d69469 100644 --- a/README.md +++ b/README.md @@ -622,7 +622,6 @@ pacman -S cargo-llvm-cov - Branch coverage is not supported yet. See [#8] and [rust-lang/rust#79649] for more. - Support for doc tests is unstable and has known issues. See [#2] and [rust-lang/rust#79417] for more. -- `cargo llvm-cov nextest` is run with `NEXTEST_TEST_THREADS=1` to work around [rust-lang/rust#91092]. You can pass `--test-threads` (e.g., `--test-threads=$(nproc)`) to override this behavior. See also [the code-coverage-related issues reported in rust-lang/rust](https://github.com/rust-lang/rust/labels/A-code-coverage). @@ -648,7 +647,6 @@ See also [the code-coverage-related issues reported in rust-lang/rust](https://g [rust-lang/rust#79417]: https://github.com/rust-lang/rust/issues/79417 [rust-lang/rust#79649]: https://github.com/rust-lang/rust/issues/79649 [rust-lang/rust#84605]: https://github.com/rust-lang/rust/issues/84605 -[rust-lang/rust#91092]: https://github.com/rust-lang/rust/issues/91092 [xtask]: https://github.com/matklad/cargo-xtask ## License diff --git a/src/main.rs b/src/main.rs index 25ced483..b79cc9e0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -202,7 +202,27 @@ fn set_env(cx: &Context, env: &mut dyn EnvTarget, IsNextest(is_nextest): IsNexte } } - let llvm_profile_file = cx.ws.target_dir.join(format!("{}-%p-%m.profraw", cx.ws.name)); + let llvm_profile_file = if is_nextest { + // https://github.com/taiki-e/cargo-llvm-cov/issues/258 + // https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#running-the-instrumented-program + // Select the number of threads that is the same as the one nextest uses by default here. + // https://github.com/nextest-rs/nextest/blob/c54694dfe7be016993983b5dedbcf2b50d4b1a6e/nextest-runner/src/config/test_threads.rs + // https://github.com/nextest-rs/nextest/blob/c54694dfe7be016993983b5dedbcf2b50d4b1a6e/nextest-runner/src/config/config_impl.rs#L30 + // TODO: should we respect custom test-threads? + // - If the number of threads specified by the user is negative or + // less or equal to available cores, it should not really be a problem + // because it does not exceed the number of available cores. + // - Even if the number of threads specified by the user is greater than + // available cores, it is expected that the number of threads that can + // write simultaneously will not exceed the number of available cores. + cx.ws.target_dir.join(format!( + "{}-%p-%{}m.profraw", + cx.ws.name, + std::thread::available_parallelism().map_or(1, usize::from) + )) + } else { + cx.ws.target_dir.join(format!("{}-%p-%m.profraw", cx.ws.name)) + }; let rustflags = &mut cx.ws.config.rustflags(&cx.ws.target_for_config)?.unwrap_or_default(); push_common_flags(cx, rustflags); @@ -287,10 +307,6 @@ fn set_env(cx: &Context, env: &mut dyn EnvTarget, IsNextest(is_nextest): IsNexte } env.set("LLVM_PROFILE_FILE", llvm_profile_file.as_str())?; env.set("CARGO_INCREMENTAL", "0")?; - if is_nextest { - // Same as above - env.set("NEXTEST_TEST_THREADS", "1")?; - } env.set("CARGO_LLVM_COV", "1")?; Ok(()) } @@ -372,7 +388,6 @@ fn run_nextest(cx: &Context) -> Result<()> { { let mut cargo = cargo.clone(); cargo.arg("--no-run"); - cargo.env_remove("NEXTEST_TEST_THREADS"); // error: the argument '--no-run' cannot be used with '--test-threads ' cargo::test_or_run_args(cx, &mut cargo); if term::verbose() { status!("Running", "{cargo}");