Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Doubts about Incorrect Source-Based Test Coverage Results #91661

Closed
heisen-li opened this issue Dec 8, 2021 · 10 comments · Fixed by #92142
Closed

Doubts about Incorrect Source-Based Test Coverage Results #91661

heisen-li opened this issue Dec 8, 2021 · 10 comments · Fixed by #92142
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) C-bug Category: This is a bug.

Comments

@heisen-li
Copy link
Contributor

heisen-li commented Dec 8, 2021

I used grcov to measure the test coverage of the project, but the results didn't seem very good, both source-based and gcov-based.@richkadel

The source-based approach is shown here.

To test the rand project, perform the following steps:

rustup component add llvm-tools-preview
export RUSTFLAGS="-Zinstrument-coverage"
cargo build
export LLVM_PROFILE_FILE="your_name-%p-%m.profraw"
cargo test
grcov . -s . --binary-path ./target/debug/ -t html --branch --ignore-not-existing -o ./target/debug/coverage/

In the rand test result, for example, in the /rand/target/debug/coverage/rand_core/src/le.rs file:

Q1: In this file, two similar functions, one is detected and the other is not found. I don't know why.
image

Only one function is shown here, and the other function seems odd that grcov did not find it.
image

Note: In the /rand/target/debug/coverage/rand_core/src/block.rs file, a large number of codes are not covered. Judgment here seems to have failed.
image
image
...

Q2:In addition, the test function does not appear to be detected. In other files, the test function is executed.
Do not execute test code:/rand/target/debug/coverage/rand_core/src/os.rs
image
execute test code:/rand/target/debug/coverage/src/seq/mod.rs
image

Q3:Sometimes structs/enumerations are included in the row coverage, but sometimes they are not. What are the criteria for this?

rustc --version --verbose:

rustc 1.58.0-nightly (29b124802 2021-10-25)
binary: rustc
commit-hash: 29b1248025b19bd132c8047fc710ea9314b9b76b
commit-date: 2021-10-25
host: x86_64-unknown-linux-gnu
release: 1.58.0-nightly
LLVM version: 13.0.0

@heisen-li heisen-li added the C-bug Category: This is a bug. label Dec 8, 2021
@Amanieu Amanieu added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Dec 8, 2021
@richkadel
Copy link
Contributor

@qiangheisenberg I don't test with grcov. It's a layer above the LLVM tools that generate the coverage reports, as I understand it.

It's possible you need to tell grcov other things, to get it to process the files properly (for example, to tell it where the test binaries are... here's a similar bug report where the author realized this was an issue: #79456).

If you really think there's a bug in coverage, please generate an MCVE, using llvm-cov to generate the report. (See the Rust Unstable Book for detailed steps; those steps include an example of getting the test binaries, by the way.)

If you use the documented process on a small example, preferably with a single Rust source file, that's the best way to give me something I can use to debug a coverage problem.

Thanks!

@heisen-li
Copy link
Contributor Author

Sorry to reply now.

I test a simple program.
The following is the test code and results:
image

image
and
image

image

My doubt is:

  1. The content of the comment does not seem to be overwritten;
  2. Why is the main function counted as multiple functions?
  3. Why is it not detected after adding a new function, but it shows that it is not covered?

@hkratz
Copy link
Contributor

hkratz commented Dec 23, 2021

@qiangheisenberg Do you get the same results using the instructions in the Unstable book and llvm-cov as @richkadel suggested?

@heisen-li
Copy link
Contributor Author

@hkratz thank you for your reply. I followed the steps in the unstable book to test. The effect is better, but I still haven't explained my doubts.
After installing the required tools, my running steps are:

cargo clean
RUSTFLAGS="-Z instrument-coverage" cargo build
RUSTFLAGS="-Z instrument-coverage" LLVM_PROFILE_FILE="json5format-%m.profraw" cargo test --tests
cargo profdata -- merge -sparse json5format-*.profraw -o json5format.profdata

// Analyze test data
cargo cov -- report --use-color --ignore-filename-regex='/.cargo/registry' --instr-profile=json5format.profdata --object target/debug/deps/cov_test-86f2fdf290095642

// Analyze test data
cargo cov -- show --use-color --ignore-filename-regex='/.cargo/registry' --instr-profile=json5format.profdata --object target/debug/deps/rand-b20edb5fd980bac1 --object target/debug/deps/cov_test-e20c2ec08166eaaf --show-instantiations --show-line-counts-or-regions --Xdemangler=rustfilt | less -R

There are still undetected codes in the file ../rand/rand_core/src/block.rs:. For example:
image
and
.../rand/rand_core/src/le.rs
image
Digression: gcov can detect this situation.
image

In summary, the steps and results of my test. From the results, the results of using llvm-cov, grcov, and gcov are:

test lines(sum/covered/percentage) functions(sum/covered/percentage)
llvm-cov 4306/3734/86.72% 579/426/73.58%
gcov 2787/2228/79.94% 1780/1254/70.45%
grcov 4165/3476/83.46% 1904/1226/64.39

Their test environment is the same.

root@szxphis00500:/home/temp/rand# llvm-cov --version
LLVM (http://llvm.org/):
  LLVM version 13.0.0
  Optimized build.
  Default target: x86_64-unknown-linux-gnu
  Host CPU: broadwell

root@szxphis00500:/home/temp/rand# rustc --version
rustc 1.59.0-nightly (34926f0a1 2021-12-22)

root@szxphis00500:/home/temp/rand# lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 18.04.2 LTS
Release:        18.04
Codename:       bionic

Is it because I missed something? If you test rand, will the result be different from mine? thanks.

@wesleywiser
Copy link
Member

wesleywiser commented Dec 27, 2021

@qiangheisenberg this appears to be the same underlying issue as the other reports of missed code coverage data and is fixed by #92142 as well. With those changes applied, I see the following from llvm-cov:

For Q1:

D:\code\temp\rand\rand_core\src\le.rs:
    1|       |// Copyright 2018 Developers of the Rand project.
    2|       |//
    3|       |// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
    4|       |// https://www.apache.org/licenses/LICENSE-2.0> or the MIT license
    5|       |// <LICENSE-MIT or https://opensource.org/licenses/MIT>, at your
    6|       |// option. This file may not be copied, modified, or distributed
    7|       |// except according to those terms.
    8|       |
    9|       |//! Little-Endian utilities
   10|       |//!
   11|       |//! Little-Endian order has been chosen for internal usage; this makes some
   12|       |//! useful functions available.
   13|       |
   14|       |use core::convert::TryInto;
   15|       |
   16|       |/// Reads unsigned 32 bit integers from `src` into `dst`.
   17|       |#[inline]
   18|      0|pub fn read_u32_into(src: &[u8], dst: &mut [u32]) {
   19|      0|    assert!(src.len() >= 4 * dst.len());
   20|      0|    for (out, chunk) in dst.iter_mut().zip(src.chunks_exact(4)) {
   21|      0|        *out = u32::from_le_bytes(chunk.try_into().unwrap());
   22|      0|    }
   23|      0|}
   24|       |
   25|       |/// Reads unsigned 64 bit integers from `src` into `dst`.
   26|       |#[inline]
   27|      0|pub fn read_u64_into(src: &[u8], dst: &mut [u64]) {
   28|      0|    assert!(src.len() >= 8 * dst.len());
   29|      0|    for (out, chunk) in dst.iter_mut().zip(src.chunks_exact(8)) {
   30|      0|        *out = u64::from_le_bytes(chunk.try_into().unwrap());
   31|      0|    }
   32|      0|}
  330|       |    /// Reset the number of available results.
  331|       |    /// This will force a new set of results to be generated on next use.
  332|       |    #[inline]
  333|      0|    pub fn reset(&mut self) {
  334|      0|        self.index = self.results.as_ref().len();
  335|      0|        self.half_used = false;
  336|      0|    }
  412|       |impl<R: BlockRngCore + SeedableRng> SeedableRng for BlockRng64<R> {
  413|       |    type Seed = R::Seed;
  414|       |
  415|       |    #[inline(always)]
  416|      0|    fn from_seed(seed: Self::Seed) -> Self {
  417|      0|        Self::new(R::from_seed(seed))
  418|      0|    }
  419|       |
  420|       |    #[inline(always)]
  421|      0|    fn seed_from_u64(seed: u64) -> Self {
  422|      0|        Self::new(R::seed_from_u64(seed))
  423|      0|    }
  424|       |
  425|       |    #[inline(always)]
  426|      0|    fn from_rng<S: RngCore>(rng: S) -> Result<Self, Error> {
  427|      0|        Ok(Self::new(R::from_rng(rng)?))
  428|      0|    }
  429|       |}

For Q2:

test_os_rng and test_construction are still not covered but that is likely because those tests aren't run at all. I suspect the library those tests are in isn't set up correctly in Cargo.toml for testing so those tests are never compiled. This is therefore not an issue with -Zinstrument-coverage but with the crate's configuration.

> cargo +stage1 test --tests
    Finished test [unoptimized + debuginfo] target(s) in 2.13s
     Running unittests (target\debug\deps\rand-c30b5860dafe0096.exe)

running 72 tests
test distributions::bernoulli::test::test_trivial ... ok
test distributions::bernoulli::test::value_stability ... ok
test distributions::distribution::tests::test_distributions_iter ... ok
test distributions::distribution::tests::test_dist_string ... ok
test distributions::distribution::tests::test_distributions_map ... ok
test distributions::distribution::tests::test_make_an_iter ... ok
test distributions::float::tests::f32_edge_cases ... ok
test distributions::float::tests::f64_edge_cases ... ok
test distributions::float::tests::value_stability ... ok
test distributions::integer::tests::test_integers ... ok
test distributions::integer::tests::value_stability ... ok
test distributions::other::tests::test_alphanumeric ... ok
test distributions::other::tests::test_chars ... ok
test distributions::other::tests::test_misc ... ok
test distributions::other::tests::value_stability ... ok
test distributions::uniform::tests::test_char ... ok
test distributions::uniform::tests::test_custom_uniform ... ok
test distributions::uniform::tests::test_float_overflow - should panic ... ok
test distributions::uniform::tests::test_float_overflow_single - should panic ... ok
test distributions::uniform::tests::test_durations ... ok
test distributions::uniform::tests::test_float_assertions ... ok
test distributions::uniform::tests::test_uniform_bad_limits_equal_int - should panic ... ok
test distributions::uniform::tests::test_uniform_bad_limits_flipped_int - should panic ... ok
test distributions::uniform::tests::test_uniform_from_std_range ... ok
test distributions::uniform::tests::test_uniform_from_std_range_inclusive ... ok
test distributions::uniform::tests::test_uniform_good_limits_equal_int ... ok
test distributions::uniform::tests::test_floats ... ok
test distributions::uniform::tests::value_stability ... ok
test distributions::weighted_index::test::test_accepting_nan ... ok
test distributions::weighted_index::test::test_update_weights ... ok
test distributions::weighted_index::test::value_stability ... ok
test rng::test::test_fill ... ok
test rng::test::test_fill_bytes_default ... ok
test rng::test::test_fill_empty ... ok
test rng::test::test_gen_bool ... ok
test rng::test::test_gen_range_panic_int - should panic ... ok
test rng::test::test_gen_range_panic_usize - should panic ... ok
test rng::test::test_rng_boxed_trait ... ok
test rng::test::test_rng_trait_object ... ok
test rngs::adapter::read::test::test_reader_rng_fill_bytes ... ok
test rngs::adapter::read::test::test_reader_rng_insufficient_bytes ... ok
test rngs::adapter::read::test::test_reader_rng_u32 ... ok
test rngs::adapter::read::test::test_reader_rng_u64 ... ok
test rngs::adapter::reseeding::test::test_clone_reseeding ... ok
test rng::test::test_gen_range_float ... ok
test rngs::adapter::reseeding::test::test_reseeding ... ok
test rngs::std::test::test_stdrng_construction ... ok
test rngs::thread::test::test_thread_rng ... ok
test seq::index::test::test_sample_boundaries ... ok
test rng::test::test_gen_range_int ... ok
test seq::index::test::test_sample_weighted ... ok
test distributions::weighted_index::test::test_weightedindex ... ok
test seq::index::test::value_stability_sample ... ok
test seq::index::test::test_sample_alg ... ok
test seq::test::test_multiple_weighted_edge_cases ... ok
test seq::test::test_partial_shuffle ... ok
test seq::test::test_sample_iter ... ok
test seq::test::test_iterator_choose ... ok
test seq::test::test_slice_choose ... ok
test distributions::bernoulli::test::test_average ... ok
test seq::test::value_stability_choose ... ok
test seq::test::value_stability_choose_multiple ... ok
test seq::test::value_stability_choose_stable ... ok
test seq::test::value_stability_slice ... ok
test test::test_random ... ok
test seq::test::test_shuffle ... ok
test seq::test::test_weighted ... ok
test rng::test::test_gen_ratio_average ... ok
test seq::test::test_multiple_weighted_distributions ... ok
test seq::test::test_iterator_choose_stable ... ok
test seq::test::test_iterator_choose_stable_stability ... ok
test distributions::uniform::tests::test_integers ... ok

test result: ok. 72 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.06s

(Notice that no tests from rand_core or the os submodule are run via cargo test)

For Q3: If you still observe this after #92142 is merged, please open an issue with examples so we can take a closer look.

@heisen-li
Copy link
Contributor Author

Oh, my God, I'm so excited. Thank you for your work.It's true that a lot of functions are covered. The ability to test coverage is almost perfect. There's only one thing I'm confused about:
rand/rand_pcg/src/pcg128.rs

   45|       |impl Lcg128Xsl64 {
   46|       |    /// Multi-step advance functions (jump-ahead, jump-back)
   47|       |    ///
   48|       |    /// The method used here is based on Brown, "Random Number Generation
   49|       |    /// with Arbitrary Stride,", Transactions of the American Nuclear
   50|       |    /// Society (Nov. 1994).  The algorithm is very similar to fast
   51|       |    /// exponentiation.
   52|       |    ///
   53|       |    /// Even though delta is an unsigned integer, we can pass a
   54|       |    /// signed integer to go backwards, it just goes "the long way round".
   55|       |    ///
   56|       |    /// Using this function is equivalent to calling `next_64()` `delta`
   57|       |    /// number of times.
   58|       |    #[inline]
   59|      0|    pub fn advance(&mut self, delta: u128) {
   60|      0|        let mut acc_mult: u128 = 1;
   61|      0|        let mut acc_plus: u128 = 0;
   62|      0|        let mut cur_mult = MULTIPLIER;
   63|      0|        let mut cur_plus = self.increment;
   64|      0|        let mut mdelta = delta;
   65|       |
   66|      0|        while mdelta > 0 {
   67|      0|            if (mdelta & 1) != 0 {
   68|      0|                acc_mult = acc_mult.wrapping_mul(cur_mult);
   69|      0|                acc_plus = acc_plus.wrapping_mul(cur_mult).wrapping_add(cur_plus);
   70|      0|            }
   71|      0|            cur_plus = cur_mult.wrapping_add(1).wrapping_mul(cur_plus);
   72|      0|            cur_mult = cur_mult.wrapping_mul(cur_mult);
   73|      0|            mdelta /= 2;
   74|       |        }
   75|      0|        self.state = acc_mult.wrapping_mul(self.state).wrapping_add(acc_plus);
   76|      0|    }
   77|       |
   78|       |    /// Construct an instance compatible with PCG seed and stream.
   79|       |    ///
   80|       |    /// Note that the highest bit of the `stream` parameter is discarded
   81|       |    /// to simplify upholding internal invariants.
   82|       |    ///
   83|       |    /// Note that two generators with different stream parameters may be closely
   84|       |    /// correlated.
   85|       |    ///
   86|       |    /// PCG specifies the following default values for both parameters:
   87|       |    ///
   88|       |    /// - `state = 0xcafef00dd15ea5e5`
   89|       |    /// - `stream = 0xa02bdbf7bb3c0a7ac28fa16a64abf96`
   90|       |    pub fn new(state: u128, stream: u128) -> Self {
   91|       |        // The increment must be odd, hence we discard one bit:
   92|       |        let increment = (stream << 1) | 1;
   93|       |        Lcg128Xsl64::from_state_incr(state, increment)
   94|       |    }
   95|       |
   96|       |    #[inline]
   97|       |    fn from_state_incr(state: u128, increment: u128) -> Self {
   98|       |        let mut pcg = Lcg128Xsl64 { state, increment };
   99|       |        // Move away from initial value:
  100|       |        pcg.state = pcg.state.wrapping_add(pcg.increment);
  101|       |        pcg.step();
  102|       |        pcg
  103|       |    }
  104|       |
  105|       |    #[inline]
  106|       |    fn step(&mut self) {
  107|       |        // prepare the LCG for the next round
  108|       |        self.state = self
  109|       |            .state
  110|       |            .wrapping_mul(MULTIPLIER)
  111|       |            .wrapping_add(self.increment);
  112|       |    }
  113|       |}
  114|       |
  115|       |// Custom Debug implementation that does not expose the internal state
  116|       |impl fmt::Debug for Lcg128Xsl64 {
  117|       |    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
  118|       |        write!(f, "Lcg128Xsl64 {{}}")
  119|       |    }
  120|       |}
  121|       |
  122|       |impl SeedableRng for Lcg128Xsl64 {
  123|       |    type Seed = [u8; 32];
  124|       |
  125|       |    /// We use a single 255-bit seed to initialise the state and select a stream.
  126|       |    /// One `seed` bit (lowest bit of `seed[8]`) is ignored.
  127|       |    fn from_seed(seed: Self::Seed) -> Self {
  128|       |        let mut seed_u64 = [0u64; 4];
  129|       |        le::read_u64_into(&seed, &mut seed_u64);
  130|       |        let state = u128::from(seed_u64[0]) | (u128::from(seed_u64[1]) << 64);
  131|       |        let incr = u128::from(seed_u64[2]) | (u128::from(seed_u64[3]) << 64);
  132|       |
  133|       |        // The increment must be odd, hence we discard one bit:
  134|       |        Lcg128Xsl64::from_state_incr(state, incr | 1)
  135|       |    }
  136|       |}

@heisen-li
Copy link
Contributor Author

heisen-li commented Dec 30, 2021

Plus, there's a bit of annotation problem, and it seems that large paragraphs of comments are also counted.Presentation only:
image

image

@richkadel
Copy link
Contributor

Plus, there's a bit of annotation problem, and it seems that large paragraphs of comments are also counted.Presentation only:

This works as intended (WAI), for now.

LLVM coverage profiling tracks code regions (starting file position and ending file position). In Rust's implementation of LLVM coverage, comments don't break up the code region, which starts (for example, in your second image) after the open brace and ends before the closing brace.

See the Rust coverage test results to get a full understanding of what is currently "expected" coverage results. For example, the following coverage report test result shows fully-covered functions, including comments in the code region:

49| 1|fn j(x: u8) {
50| 1| // non-async versions of `c()`, `d()`, and `f()` to make it similar to async `i()`.
51| 1| fn c(x: u8) -> u8 {
52| 1| if x == 8 {
53| 1| 1 // This line appears covered, but the 1-character expression span covering the `1`
^0
54| 1| // is not executed. (`llvm-cov show` displays a `^0` below the `1` ). This is because
55| 1| // `fn j()` executes the open brace for the funciton body, followed by the function's
56| 1| // first executable statement, `match x`. Inner function declarations are not
57| 1| // "visible" to the MIR for `j()`, so the code region counts all lines between the
58| 1| // open brace and the first statement as executed, which is, in a sense, true.
59| 1| // `llvm-cov show` overcomes this kind of situation by showing the actual counts
60| 1| // of the enclosed coverages, (that is, the `1` expression was not executed, and
61| 1| // accurately displays a `0`).
62| 1| } else {
63| 1| 0
64| 1| }
65| 1| }

@richkadel
Copy link
Contributor

The ability to test coverage is almost perfect. There's only one thing I'm confused about:
rand/rand_pcg/src/pcg128.rs

@qiangheisenberg - I think there is a problem in how you are generating the coverage report.

I ran the rand_pcg tests and got the following results, showing all functions covered:

   54|       |    /// signed integer to go backwards, it just goes "the long way round".
   55|       |    ///
   56|       |    /// Using this function is equivalent to calling `next_64()` `delta`
   57|       |    /// number of times.
   58|       |    #[inline]
   59|     20|    pub fn advance(&mut self, delta: u128) {
   60|     20|        let mut acc_mult: u128 = 1;
   61|     20|        let mut acc_plus: u128 = 0;
   62|     20|        let mut cur_mult = MULTIPLIER;
   63|     20|        let mut cur_plus = self.increment;
   64|     20|        let mut mdelta = delta;
   65|       |
   66|    120|        while mdelta > 0 {
   67|    100|            if (mdelta & 1) != 0 {
   68|     40|                acc_mult = acc_mult.wrapping_mul(cur_mult);
   69|     40|                acc_plus = acc_plus.wrapping_mul(cur_mult).wrapping_add(cur_plus);
   70|     60|            }
   71|    100|            cur_plus = cur_mult.wrapping_add(1).wrapping_mul(cur_plus);
   72|    100|            cur_mult = cur_mult.wrapping_mul(cur_mult);
   73|    100|            mdelta /= 2;
   74|       |        }
   75|     20|        self.state = acc_mult.wrapping_mul(self.state).wrapping_add(acc_plus);
   76|     20|    }
   77|       |
   78|       |    /// Construct an instance compatible with PCG seed and stream.
   79|       |    ///
   80|       |    /// Note that the highest bit of the `stream` parameter is discarded
   81|       |    /// to simplify upholding internal invariants.
   82|       |    ///
   83|       |    /// Note that two generators with different stream parameters may be closely
   84|       |    /// correlated.
   85|       |    ///
   86|       |    /// PCG specifies the following default values for both parameters:
   87|       |    ///
   88|       |    /// - `state = 0xcafef00dd15ea5e5`
   89|       |    /// - `stream = 0xa02bdbf7bb3c0a7ac28fa16a64abf96`
   90|      1|    pub fn new(state: u128, stream: u128) -> Self {
   91|      1|        // The increment must be odd, hence we discard one bit:
   92|      1|        let increment = (stream << 1) | 1;
   93|      1|        Lcg128Xsl64::from_state_incr(state, increment)
   94|      1|    }
   95|       |
   96|       |    #[inline]
   97|     25|    fn from_state_incr(state: u128, increment: u128) -> Self {
   98|     25|        let mut pcg = Lcg128Xsl64 { state, increment };
   99|     25|        // Move away from initial value:
  100|     25|        pcg.state = pcg.state.wrapping_add(pcg.increment);
  101|     25|        pcg.step();
  102|     25|        pcg
  103|     25|    }
  104|       |
  105|       |    #[inline]
  106|    439|    fn step(&mut self) {
  107|    439|        // prepare the LCG for the next round
  108|    439|        self.state = self
  109|    439|            .state
  110|    439|            .wrapping_mul(MULTIPLIER)
  111|    439|            .wrapping_add(self.increment);
  112|    439|    }
  113|       |}

@richkadel
Copy link
Contributor

Thinking about possible causes for your missing coverage, I have another theory. The advance() function (the only function showing coverage, in your example) is inline. If you created a test that only calls that function, the inlined instantiation may have been compiled inside the calling module. The module that includes all of the other functions might then be an unused module, and therefore might not get coverage.

This is something @wesleywiser already mentioned, and fixed in a pending PR (#92142) to be merged.

Please wait until that PR is merged and lands in nightly, then try again and see if it resolves your issue.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 5, 2022
…ioning, r=tmandry

[code coverage] Fix missing dead code in modules that are never called

The issue here is that the logic used to determine which CGU to put the dead function stubs in doesn't handle cases where a module is never assigned to a CGU (which is what happens when all of the code in the module is dead).

The partitioning logic also caused issues in rust-lang#85461 where inline functions were duplicated into multiple CGUs resulting in duplicate symbols.

This commit fixes the issue by removing the complex logic used to assign dead code stubs to CGUs and replaces it with a much simpler model: we pick one CGU to hold all the dead code stubs. We pick a CGU which has exported items which increases the likelihood the linker won't throw away our dead functions and we pick the smallest to minimize the impact on compilation times for crates with very large CGUs.

Fixes rust-lang#91661
Fixes rust-lang#86177
Fixes rust-lang#85718
Fixes rust-lang#79622

r? `@tmandry`
cc `@richkadel`

This PR is not urgent so please don't let it interrupt your holidays! 🎄 🎁
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 12, 2022
…ioning, r=tmandry

[code coverage] Fix missing dead code in modules that are never called

The issue here is that the logic used to determine which CGU to put the dead function stubs in doesn't handle cases where a module is never assigned to a CGU (which is what happens when all of the code in the module is dead).

The partitioning logic also caused issues in rust-lang#85461 where inline functions were duplicated into multiple CGUs resulting in duplicate symbols.

This commit fixes the issue by removing the complex logic used to assign dead code stubs to CGUs and replaces it with a much simpler model: we pick one CGU to hold all the dead code stubs. We pick a CGU which has exported items which increases the likelihood the linker won't throw away our dead functions and we pick the smallest to minimize the impact on compilation times for crates with very large CGUs.

Fixes rust-lang#91661
Fixes rust-lang#86177
Fixes rust-lang#85718
Fixes rust-lang#79622

r? `@tmandry`
cc `@richkadel`

This PR is not urgent so please don't let it interrupt your holidays! 🎄 🎁
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 13, 2022
…ioning, r=tmandry

[code coverage] Fix missing dead code in modules that are never called

The issue here is that the logic used to determine which CGU to put the dead function stubs in doesn't handle cases where a module is never assigned to a CGU (which is what happens when all of the code in the module is dead).

The partitioning logic also caused issues in rust-lang#85461 where inline functions were duplicated into multiple CGUs resulting in duplicate symbols.

This commit fixes the issue by removing the complex logic used to assign dead code stubs to CGUs and replaces it with a much simpler model: we pick one CGU to hold all the dead code stubs. We pick a CGU which has exported items which increases the likelihood the linker won't throw away our dead functions and we pick the smallest to minimize the impact on compilation times for crates with very large CGUs.

Fixes rust-lang#91661
Fixes rust-lang#86177
Fixes rust-lang#85718
Fixes rust-lang#79622

r? ``@tmandry``
cc ``@richkadel``

This PR is not urgent so please don't let it interrupt your holidays! 🎄 🎁
@bors bors closed this as completed in 5e04f51 Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants