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

Add -C link-dead-code option (to improve kcov code coverage) #31368

Merged
merged 1 commit into from Feb 12, 2016

Conversation

Projects
None yet
@JohanLorenzo
Contributor

JohanLorenzo commented Feb 2, 2016

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!

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Feb 2, 2016

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.

@sfackler

This comment has been minimized.

Member

sfackler commented Feb 2, 2016

@JohanLorenzo

This comment has been minimized.

Contributor

JohanLorenzo commented Feb 2, 2016

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.

@alexcrichton alexcrichton assigned alexcrichton and unassigned jroesch Feb 2, 2016

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 2, 2016

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

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 2, 2016

Oh and it looks like a flag like that has even been requested before!

@JohanLorenzo

This comment has been minimized.

Contributor

JohanLorenzo commented Feb 3, 2016

Thanks for the feedback @alexcrichton. Here's a new revision that implements -C no-gc-sections asked in #17141.

r? @alexcrichton

@JohanLorenzo JohanLorenzo referenced this pull request Feb 3, 2016

Closed

Add first test #1

// reduction.
} else if !is_dylib {
self.cmd.arg("-Wl,--gc-sections");
if !self.sess.opts.cg.no_gc_sections {

This comment has been minimized.

@pnkfelix

pnkfelix Feb 3, 2016

Member

This diff would be very short if you just did

if !self.sess.opts.cg.no_gc_sections {
    return;
}

This comment has been minimized.

@pnkfelix

pnkfelix Feb 3, 2016

Member

(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(..))

This comment has been minimized.

@JohanLorenzo

JohanLorenzo Feb 3, 2016

Contributor

Good point. I'll wrap the call instead.

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Feb 3, 2016

@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"'

This comment has been minimized.

@JohanLorenzo

JohanLorenzo Feb 3, 2016

Contributor

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.

This comment has been minimized.

@alexcrichton

alexcrichton Feb 3, 2016

Member

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.

@JohanLorenzo JohanLorenzo force-pushed the JohanLorenzo:dont-strip-if-test-build branch from 207f3f3 to 4364e7a Feb 3, 2016

@@ -501,6 +501,8 @@ options! {CodegenOptions, CodegenSetter, basic_codegen_options,
"system linker to link outputs with"),
link_args: Option<Vec<String>> = (None, parse_opt_list,
"extra arguments to pass to the linker (space separated)"),
no_gc_sections: bool = (false, parse_bool,

This comment has been minimized.

@alexcrichton

alexcrichton Feb 3, 2016

Member

I'd personally prefer to call this just gc_sections with a default of true, it helps avoid double negatives later on. That way to turn it off you specify -C gc-sections=no or whatever we interpret as "falsey" values

This comment has been minimized.

@JohanLorenzo

JohanLorenzo Feb 3, 2016

Contributor

Agreed. Done

@JohanLorenzo JohanLorenzo force-pushed the JohanLorenzo:dont-strip-if-test-build branch from b45760f to 7eae48d Feb 3, 2016

# Should contain gc-sections...
$(RUSTC) -Z print-link-args dummy.rs 2>&1 | \
grep -e '--gc-sections\|-dead_strip'

This comment has been minimized.

@JohanLorenzo

JohanLorenzo Feb 3, 2016

Contributor

I didn't grep on any other value because only 2 are defined in gc_sections()

This comment has been minimized.

@alexcrichton

alexcrichton Feb 3, 2016

Member

There's also gc sections below for MSVC

This comment has been minimized.

@JohanLorenzo

JohanLorenzo Feb 3, 2016

Contributor

Oops, sorry I missed it. Fixed

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 3, 2016

Thanks! Can you squash these commits down into one as well?

@JohanLorenzo JohanLorenzo force-pushed the JohanLorenzo:dont-strip-if-test-build branch 2 times, most recently from d4d10c0 to 5d61a4d Feb 4, 2016

@JohanLorenzo JohanLorenzo changed the title from Prevent stripping if we're producing test builds to Add -C gc-sections=no option (to improve kcov code coverage) Feb 4, 2016

@JohanLorenzo

This comment has been minimized.

Contributor

JohanLorenzo commented Feb 4, 2016

@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?

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 4, 2016

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)

@alexcrichton alexcrichton added the T-tools label Feb 4, 2016

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 4, 2016

cc @rust-lang/tools

@JohanLorenzo

This comment has been minimized.

Contributor

JohanLorenzo commented Feb 4, 2016

RUSTFLAGS will be good enough, once it's landed. Thanks for the background details.

@brson

This comment has been minimized.

Contributor

brson commented Feb 5, 2016

lgtm

@huonw

This comment has been minimized.

Member

huonw commented Feb 7, 2016

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".

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 8, 2016

@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.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 11, 2016

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?

@JohanLorenzo JohanLorenzo changed the title from Add -C gc-sections=no option (to improve kcov code coverage) to Add -C link-dead-code=yes option (to improve kcov code coverage) Feb 11, 2016

@JohanLorenzo JohanLorenzo changed the title from Add -C link-dead-code=yes option (to improve kcov code coverage) to Add -C link-dead-code option (to improve kcov code coverage) Feb 11, 2016

Add -C link-dead-code option r=alexcrichton
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.

@JohanLorenzo JohanLorenzo force-pushed the JohanLorenzo:dont-strip-if-test-build branch from 5d61a4d to 274f27a Feb 11, 2016

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 11, 2016

bors added a commit that referenced this pull request Feb 11, 2016

Auto merge of #31368 - JohanLorenzo:dont-strip-if-test-build, r=alexc…
…richton

Tools which rely on DWARF for generating code coverage report, don't generate accurate numbers on test builds. For instance, [this sample main](https://github.com/JohanLorenzo/rust-testing-example/blob/757bdbf3887f43db9771c20cb72dfc32aa8f4321/src/main.rs) returns [100% coverage](https://coveralls.io/builds/4940156/source?filename=main.rs) when [kcov](https://github.com/SimonKagstrom/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!
@bors

This comment has been minimized.

Contributor

bors commented Feb 11, 2016

⌛️ Testing commit 274f27a with merge c01baae...

@bors

This comment has been minimized.

Contributor

bors commented Feb 11, 2016

💔 Test failed - auto-mac-64-opt

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 11, 2016

@bors: retry

On Thu, Feb 11, 2016 at 2:18 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-mac-64-opt
http://buildbot.rust-lang.org/builders/auto-mac-64-opt/builds/8009


Reply to this email directly or view it on GitHub
#31368 (comment).

@bors

This comment has been minimized.

Contributor

bors commented Feb 12, 2016

⌛️ Testing commit 274f27a with merge d561382...

bors added a commit that referenced this pull request Feb 12, 2016

Auto merge of #31368 - JohanLorenzo:dont-strip-if-test-build, r=alexc…
…richton

Tools which rely on DWARF for generating code coverage report, don't generate accurate numbers on test builds. For instance, [this sample main](https://github.com/JohanLorenzo/rust-testing-example/blob/757bdbf3887f43db9771c20cb72dfc32aa8f4321/src/main.rs) returns [100% coverage](https://coveralls.io/builds/4940156/source?filename=main.rs) when [kcov](https://github.com/SimonKagstrom/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!
@bors

This comment has been minimized.

Contributor

bors commented Feb 12, 2016

💔 Test failed - auto-mac-64-nopt-t

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 12, 2016

@bors: retry

On Thu, Feb 11, 2016 at 8:14 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-mac-64-nopt-t
http://buildbot.rust-lang.org/builders/auto-mac-64-nopt-t/builds/8049


Reply to this email directly or view it on GitHub
#31368 (comment).

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 12, 2016

@bors: force

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 12, 2016

@bors: retry

1 similar comment
@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 12, 2016

@bors: retry

@bors

This comment has been minimized.

Contributor

bors commented Feb 12, 2016

⌛️ Testing commit 274f27a with merge 77f9231...

bors added a commit that referenced this pull request Feb 12, 2016

Auto merge of #31368 - JohanLorenzo:dont-strip-if-test-build, r=alexc…
…richton

Tools which rely on DWARF for generating code coverage report, don't generate accurate numbers on test builds. For instance, [this sample main](https://github.com/JohanLorenzo/rust-testing-example/blob/757bdbf3887f43db9771c20cb72dfc32aa8f4321/src/main.rs) returns [100% coverage](https://coveralls.io/builds/4940156/source?filename=main.rs) when [kcov](https://github.com/SimonKagstrom/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!
@bors

This comment has been minimized.

Contributor

bors commented Feb 12, 2016

💔 Test failed - auto-mac-64-opt

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 12, 2016

@bors: retry

On Thu, Feb 11, 2016 at 11:04 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-mac-64-opt
http://buildbot.rust-lang.org/builders/auto-mac-64-opt/builds/8017


Reply to this email directly or view it on GitHub
#31368 (comment).

@bors bors merged commit 274f27a into rust-lang:master Feb 12, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@jonas-schievink

This comment has been minimized.

Contributor

jonas-schievink commented Feb 12, 2016

Yay ⭐️

@@ -507,6 +507,8 @@ options! {CodegenOptions, CodegenSetter, basic_codegen_options,
"system linker to link outputs with"),
link_args: Option<Vec<String>> = (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)"),

This comment has been minimized.

@tamird

tamird Feb 12, 2016

Contributor

this description is not right, seems it should be "let the linker link dead code" or simply "link dead code"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment