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

wesleywiser · 2021-12-20T22:09:12Z

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 #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 #91661
Fixes #86177
Fixes #85718
Fixes #79622

r? @tmandry
cc @richkadel

This PR is not urgent so please don't let it interrupt your holidays! 🎄 🎁

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. 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 simplier 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#86177 Fixes rust-lang#85718 Fixes rust-lang#79622

richkadel

This looks really nice. Thank you!

It's hard to generate an extensive test to validate this works as you've explained, but I trust you are right. But assuming it does work, I'm very excited about this improvement!

I don't catch any code issues or have questions about the implementation, but I have a couple of comments and a nit.

compiler/rustc_monomorphize/src/partitioning/mod.rs

src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt

wesleywiser · 2021-12-21T01:37:18Z

Side note: while working on this PR, I've started to wonder if we should really be stabilizing these options in #90132:

-Z instrument-coverage=except-unused-generics: Instrument all functions except unused generics.
-Z instrument-coverage=except-unused-functions: Instrument only used (called) functions and instantiated generic functions.

My perception is that we've added these options to allow users to work around issues caused by our attempts to generate coverage data for dead functions and uninstantiated generic functions. Do you (@tmandry and @richkadel) think these options are useful to users for any other reasons? If not, I'll make sure we update #90132 to only stabilize -Cinstrument-coverage and -Cinstrument-coverage=off so that we can eventually remove these options when they are no longer useful.

tmandry

I like the simplicity. Generating these stubs is hopefully going to be fast, so hopefully putting them on one CGU doesn't cause performance issues. Though it would be if we could measure this somehow.

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

compiler/rustc_monomorphize/src/partitioning/mod.rs

tmandry · 2021-12-21T01:57:35Z

I don't recall why we added those options. The only reason I think we might want to keep them would be build performance.

richkadel · 2021-12-21T06:27:02Z

Do you (@tmandry and @richkadel) think these options are useful to users for any other reasons? If not, I'll make sure we update #90132 to only stabilize -Cinstrument-coverage and -Cinstrument-coverage=off so that we can eventually remove these options when they are no longer useful.

I would support that change. I think I agree with you.

wesleywiser · 2021-12-21T19:28:36Z

Thanks! Reviews addressed

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

compiler/rustc_monomorphize/src/partitioning/mod.rs

tmandry · 2021-12-23T01:07:19Z

compiler/rustc_monomorphize/src/partitioning/mod.rs

-
-        let dead_code_cgu = cgus_with_exported_symbols.last_mut().unwrap();
+            .last()
+            .unwrap();


Is it possible we'd have no CGUs with externally linked symbols?

If I understand correctly, a CGU compiles an LLVM Module.

Logically (but maybe naively), I assume a Module without any externally linked symbols could not be incorporated into a program that you could then link and run. And if that's the case, there would be no way to run a binary, using that Module, in order to generate coverage profraw data.

So even if we had a Module with no externally linked symbols, the only way to add coverage mapping data (and unused function placeholders) would be to add them to a Module that is not being compiled (which is, at least, impractical).

Yeah, @richkadel is correct. There are basically two cases: we are compiling an executable or we are compiling a library. For executables, the entry point is externally visible/linked. For libraries, something has to be externally linked or the library isn't useable.

For libraries, something has to be externally linked or the library isn't useable.

Ah, so this isn't quite true. If a library contains only macros, then it will have one CGU which is empty. In this case, I'll just fall back to the first CGU and also add an assert to verify there is at least one CGU.

tmandry · 2022-01-05T00:56:03Z

Thanks for this!!

@bors r+

bors · 2022-01-05T00:56:04Z

📌 Commit ebc0d0d has been approved by tmandry

…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 · 2022-01-05T21:20:31Z

@bors r-
looks like this failed in a rollup:
#92590 (comment)

wesleywiser · 2022-01-08T02:23:35Z

@richkadel do you have any suggestions for troubleshooting the failure? I've tried compiling the test case manually and running the coverage tools but the output seems to match the expected test results. I haven't had any luck getting the run-make-fulldeps tests working on Windows so I can't repro locally.

As discussed in rust-lang#92142 (comment), tests that contain multiple files order their results differently on Windows and Linux which the test runner currently can't handle correctly. For now, ignore the "bin" part of the test and only include the unused library dependency which is what the test really cares about anyway.

wesleywiser · 2022-01-10T16:21:20Z

Yep, that's exactly it. Thanks for looking at this @richkadel! I've added the entry point file for that test to the ignored files list and it resolves the issue.

This should be ready to go.

richkadel · 2022-01-10T20:24:08Z

@tmandry - PTAL ... Should be good to resubmit to bors queue.

tmandry · 2022-01-11T00:42:23Z

@bors r+

bors · 2022-01-11T00:42:24Z

📌 Commit e9cac4c has been approved by tmandry

…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! 🎄 🎁

…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! 🎄 🎁

…askrgr Rollup of 9 pull requests Successful merges: - rust-lang#90001 (Make rlib metadata strip works with MIPSr6 architecture) - rust-lang#91687 (rustdoc: do not emit tuple variant fields if none are documented) - rust-lang#91938 (Add `std::error::Report` type) - rust-lang#92006 (Welcome opaque types into the fold) - rust-lang#92142 ([code coverage] Fix missing dead code in modules that are never called) - rust-lang#92277 (rustc_metadata: Stop passing `CrateMetadataRef` by reference (step 1)) - rust-lang#92334 (rustdoc: Preserve rendering of macro_rules matchers when possible) - rust-lang#92807 (Update cargo) - rust-lang#92832 (Update RELEASES for 1.58.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

The comment says "Find the smallest CGU that has exported symbols and put the dead function stubs in that CGU". But the code sorts the CGUs by size (smallest first) and then searches them in reverse order, which means it will find the *largest* CGU that has exported symbols. The erroneous code was introduced in rust-lang#92142. This commit changes it to use a simpler search, avoiding the sort, and fixes the bug in the process.

wesleywiser added 2 commits December 20, 2021 16:53

Add test case for rust-lang#86177 and rust-lang#85718

341d65d

rust-highfive assigned tmandry Dec 20, 2021

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 20, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 20, 2021

wesleywiser added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Dec 20, 2021

richkadel suggested changes Dec 20, 2021

View reviewed changes

compiler/rustc_monomorphize/src/partitioning/mod.rs Outdated Show resolved Hide resolved

compiler/rustc_monomorphize/src/partitioning/mod.rs Outdated Show resolved Hide resolved

src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt Outdated Show resolved Hide resolved

tmandry reviewed Dec 21, 2021

View reviewed changes

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs Outdated Show resolved Hide resolved

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs Outdated Show resolved Hide resolved

compiler/rustc_monomorphize/src/partitioning/mod.rs Outdated Show resolved Hide resolved

wesleywiser mentioned this pull request Dec 21, 2021

Make sure dead function stubs are always included in the final binary #92165

Open

wesleywiser force-pushed the fix_codecoverage_partitioning branch from 9fde2a3 to dbc33b5 Compare December 21, 2021 18:31

wesleywiser mentioned this pull request Dec 21, 2021

Stabilize -Z instrument-coverage as -C instrument-coverage #90132

Merged

tmandry requested changes Dec 23, 2021

View reviewed changes

wesleywiser mentioned this pull request Dec 27, 2021

Doubts about Incorrect Source-Based Test Coverage Results #91661

Closed

Address review comments

ebc0d0d

wesleywiser force-pushed the fix_codecoverage_partitioning branch from dbc33b5 to ebc0d0d Compare December 27, 2021 19:07

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 5, 2022

matthiaskrgr mentioned this pull request Jan 5, 2022

Rollup of 7 pull requests #92590

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 5, 2022

wesleywiser force-pushed the fix_codecoverage_partitioning branch from b45a3f2 to e014dba Compare January 10, 2022 15:17

wesleywiser force-pushed the fix_codecoverage_partitioning branch from e014dba to 15a5a19 Compare January 10, 2022 15:17

wesleywiser force-pushed the fix_codecoverage_partitioning branch from 15a5a19 to e9cac4c Compare January 10, 2022 15:32

richkadel approved these changes Jan 10, 2022

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 11, 2022

matthiaskrgr mentioned this pull request Jan 12, 2022

Rollup of 10 pull requests #92826

Closed

matthiaskrgr mentioned this pull request Jan 13, 2022

Rollup of 10 pull requests #92842

Closed

matthiaskrgr mentioned this pull request Jan 13, 2022

Rollup of 9 pull requests #92844

Merged

bors merged commit 5e04f51 into rust-lang:master Jan 14, 2022

rustbot added this to the 1.60.0 milestone Jan 14, 2022

richkadel mentioned this pull request Jan 17, 2022

Nightly - LLVM code coverage - Issue with generating code coverage when using Prometheus crate #79645

Closed

sticnarf mentioned this pull request Jan 19, 2022

*: update rust toolchain to 2022-01-07 tikv/tikv#11875

Merged

fpoli mentioned this pull request Jan 19, 2022

LLVM coverage regression in nightly-2022-01-15 #93054

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wesleywiser commented Dec 20, 2021 •

edited

Loading

richkadel left a comment

wesleywiser commented Dec 21, 2021

tmandry left a comment

tmandry commented Dec 21, 2021

richkadel commented Dec 21, 2021

wesleywiser commented Dec 21, 2021

tmandry Dec 23, 2021

richkadel Dec 23, 2021

wesleywiser Dec 27, 2021

wesleywiser Dec 27, 2021

tmandry commented Jan 5, 2022

bors commented Jan 5, 2022

matthiaskrgr commented Jan 5, 2022

wesleywiser commented Jan 8, 2022

wesleywiser commented Jan 10, 2022

richkadel commented Jan 10, 2022

tmandry commented Jan 11, 2022

bors commented Jan 11, 2022

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

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

Conversation

wesleywiser commented Dec 20, 2021 • edited Loading

richkadel left a comment

Choose a reason for hiding this comment

wesleywiser commented Dec 21, 2021

tmandry left a comment

Choose a reason for hiding this comment

tmandry commented Dec 21, 2021

richkadel commented Dec 21, 2021

wesleywiser commented Dec 21, 2021

tmandry Dec 23, 2021

Choose a reason for hiding this comment

richkadel Dec 23, 2021

Choose a reason for hiding this comment

wesleywiser Dec 27, 2021

Choose a reason for hiding this comment

wesleywiser Dec 27, 2021

Choose a reason for hiding this comment

tmandry commented Jan 5, 2022

bors commented Jan 5, 2022

matthiaskrgr commented Jan 5, 2022

wesleywiser commented Jan 8, 2022

wesleywiser commented Jan 10, 2022

richkadel commented Jan 10, 2022

tmandry commented Jan 11, 2022

bors commented Jan 11, 2022

wesleywiser commented Dec 20, 2021 •

edited

Loading