Add -C link-dead-code option (to improve kcov code coverage) by JohanLorenzo · Pull Request #31368 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation44 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Tools which rely on DWARF for generating code coverage report, don't generate accurate numbers on test builds. For instance, this sample main returns 100% coverage when kcov runs.
With @pnkfelix 's great help, we could narrow down the issue: The linker strips unused function during phase 6. Here's a patch which stops stripping when someone calls rustc --test $ARGS. @pnkfelix wasn't sure if we should add a new flag, or just use --test. What do you think @alexcrichton ?
Also, I'm not too sure: where is the best place to add a test for this addition?
Thanks for the help!
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jroesch (or someone else) soon.
If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.
Please see the contribution instructions for more information.
Thanks for the link @sfackler! I'm glad there's some documentation on this topic.
Based on the thread, this might not be the best solution for everybody. Then, I guess I can close this PR, until a there's a consensus.
Thanks for the PR @JohanLorenzo! I'm always happy to see movement on this :).
I think that the best option that came out of @sfackler's link is that for now it's probably best to not have --test modify how the linker is called, but we may rather prefer to have a separate compiler option for this. I also believe that enabling gc-sections is the right default, so we can perhaps have a new option like -C gc-sections=no, -C link-dead-code=yes, or something like that.
I think that we can end up passing this down to Cargo via either the profile idea that @huonw mentioned or perhaps the RUSTFLAGS support @brson is implementing
| // reduction. |
|---|
| } else if !is_dylib { |
| self.cmd.arg("-Wl,--gc-sections"); |
| if !self.sess.opts.cg.no_gc_sections { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This diff would be very short if you just did
if !self.sess.opts.cg.no_gc_sections { return; }
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(but also, I am the kind of person that would put the conditional check around the call-site to linker.gc_sections(..); this change is making this function act more like fn maybe_gc_sections(..))
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'll wrap the call instead.
@JohanLorenzo If you're interested in trying to make a test of this functionality, I bet you might be able to do it with a run-make test (where it would dump the generated object code and grep for the names that should or should not be included. At least on the platforms that provide a object code dumping utility that we can call out to easily). Ping me if you want to try to explore this.
| # Should contain gc-sections... |
| $(RUSTC) -Z print-link-args dummy.rs 2>&1 | \ |
| grep '"-Wl,--gc-sections"' |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discussed with @pnkfelix about testing also on IRC. A better approach would be to use
objdump --disassemble $(TMPDIR)/dummy | grep 'unused_function'
I tried to implement this check, but it turns out $(TMPDIR)/dummy is not accessible when run in the Makefile. When I ran the same command locally, it works like a charm. I'm under the impression there's a race condition within the Makefile.
I git grep'd to find if a similar case existed in the code base, but I didn't manage to find any. Then, here's another proposal, closer to what already exist in the current Makefile.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check will probably need to be guarded based on platform as well. I do agree that objdump would probably be the best way to go here, but it's probably only reliably available on Linux (and maybe some BSDs). By default it's not on OSX and I doubt it's available on Windows much.
Same with checking for --gc-sections as well, unfortunately that's only the name of the argument on Linux. On OSX it's -Wl,-dead_strip and on MSVC it's something different as well.
Thanks! Can you squash these commits down into one as well?
Prevent stripping if we're producing test builds Add -C gc-sections=no option (to improve kcov code coverage)
@alexcrichton Thank you for the help and the review. Here's the squashed commit.
I have a couple of questions remaining: How would it be possible to pass this flag to cargo? I looked up and saw cargo build shouldn't allow to pass these kind of flags. Would cargo rustc -- -C gc-sections=no resolve the dependencies and produce the non-stripped binary?
Currently there's not a fantastic way to pass this sort of option down through Cargo to the compiler, but something like RUSTFLAGS is what should work in the long run. That way whenever you're compiling for code coverage you can pass RUSTFLAGS=-Cgc-sections=no cargo test --no-run.
Ideally I'd like a cargo coverage-like command which takes care of all those defaults for you in the long run, but we may not be quite ready to do that on all platforms (e.g. I've only ever seen good code coverage for native object code on Linux so far using kcov)
RUSTFLAGS will be good enough, once it's landed. Thanks for the background details.
I idly wonder if this argument might be better phrased as -C strip-dead-code or something, i.e. avoiding the "jargon" of gc-sections, and avoiding (weakly) locking us into the concept of "sections".
@huonw I was thinking something similar as well, yeah. I think that anything relating to stripping dead code, however, may also be a bit of a misnomer for a few reasons:
- We already strip dead code within a library (e.g. optimizations), this is just stripping dead code across libraries.
- On platforms like linux, the implementation will strip more than dead code, it'll strip dead sections.
Not that the points are really much in favor of gc-sections, however, as it's not what happens on Windows or OSX really. I figure it's a semi-obscure option that'll only be learned on demand anyway.
Ok, the tools team talked about this during triage today and the conclusion was that this is a good option to have, but we likely want to not call it gc-sections.
I'm personally kinda leaning towards "link-dead-code" as it matches what happens on OSX and MSVC (the linker just literally strips dead code) and the interpretation on Linux basically just ends up being the same thing.
@JohanLorenzo what do you think?
link-dead-code is more self-explanatory than gc-sections. So, that makes perfect sense to me. I'm updating the patch.
Add -C gc-sections=no option (to improve kcov code coverage) Add -C link-dead-code=yes option (to improve kcov code coverage)
Add -C link-dead-code=yes option (to improve kcov code coverage) Add -C link-dead-code option (to improve kcov code coverage)
Turning gc-sections off improves code coverage based for tools which use DWARF debugging information (like kcov). Otherwise dead code is stripped and kcov returns a coverage percentage that doesn't reflect reality.
⌛ Testing commit 274f27a with merge c01baae...
⌛ Testing commit 274f27a with merge d561382...
1 similar comment
bors added a commit that referenced this pull request
⚡ Previous build results for auto-linux-64-nopt-t, auto-linux-cross-opt, auto-mac-ios-opt are reusable. Rebuilding only auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt...
☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt
| @@ -507,6 +507,8 @@ options! {CodegenOptions, CodegenSetter, basic_codegen_options, |
|---|
| "system linker to link outputs with"), |
| link_args: Option<Vec> = (None, parse_opt_list, |
| "extra arguments to pass to the linker (space separated)"), |
| link_dead_code: bool = (false, parse_bool, |
| "let the linker strip dead coded (turning it on can be used for code coverage)"), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this description is not right, seems it should be "let the linker link dead code" or simply "link dead code"
Manishearth pushed a commit to Manishearth/rust that referenced this pull request
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
Update docs for -Clink-dead-code to discourage its use
The -Clink-dead-code flag was originally added way back in rust-lang#31368, apparently to help improve the output of some older forms of code coverage measurement, and also to address some use-cases for wanting to suppress linker flags like -dead_strip and --gc-section.
In the past it might have also been useful in conjunction with -Cinstrument-coverage, but subsequent improvements to coverage instrumentation have made it unnecessary there.
It is also currently used by cargo-fuzz by default, for reasons that are possibly no longer relevant.
The flag currently does more than its name suggests, affecting not just linker flags, but also monomorphization decisions. It has also contributed to ICEs (e.g. rust-lang#135515) that would not have occurred without link-dead-code.
For now, this PR just updates the documentation to be more realistic about what the flag does, and when it should be used (approximately never). In the future, it might be worth looking into properly deprecating this flag, and perhaps making it a no-op if feasible.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#135561 - Zalathar:link-dead-code, r=saethlin
Update docs for -Clink-dead-code to discourage its use
The -Clink-dead-code flag was originally added way back in rust-lang#31368, apparently to help improve the output of some older forms of code coverage measurement, and also to address some use-cases for wanting to suppress linker flags like -dead_strip and --gc-section.
In the past it might have also been useful in conjunction with -Cinstrument-coverage, but subsequent improvements to coverage instrumentation have made it unnecessary there.
It is also currently used by cargo-fuzz by default, for reasons that are possibly no longer relevant.
The flag currently does more than its name suggests, affecting not just linker flags, but also monomorphization decisions. It has also contributed to ICEs (e.g. rust-lang#135515) that would not have occurred without link-dead-code.
For now, this PR just updates the documentation to be more realistic about what the flag does, and when it should be used (approximately never). In the future, it might be worth looking into properly deprecating this flag, and perhaps making it a no-op if feasible.