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

Dead code not linked in beta or nightly #46467

Closed
xd009642 opened this issue Dec 3, 2017 · 11 comments
Closed

Dead code not linked in beta or nightly #46467

xd009642 opened this issue Dec 3, 2017 · 11 comments
Assignees
Labels
C-bug Category: This is a bug. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@xd009642
Copy link
Contributor

xd009642 commented Dec 3, 2017

I've noticed with beta and nightly the rust flag link-dead-code no longer includes unused source files when running cargo test. Testing it out on this test project, I used objdump -WL to get the decoded lines in the binary. With rustc stable both source files in the crate are linked into the test binary (lib.rs and unused.rs), however with stable and nightly only lib.rs is linked.

This is counter to what I'd expect link-dead-code to do. Is this a desired feature? And if so is there the means to obtain the old behaviour?

@Mark-Simulacrum
Copy link
Member

Probably a regression related to ThinLTO work... Cc @alexcrichton

@kennytm kennytm added C-bug Category: This is a bug. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Dec 3, 2017
@alexcrichton
Copy link
Member

My guess is this is #45575 (cc @michaelwoerister) rather than ThinLTO b/c the mention of cargo test here I think indicates debug mode?

@michaelwoerister
Copy link
Member

My guess is this is #45575 (cc @michaelwoerister) rather than ThinLTO

Yes, that could well be. We still would need to decide whether link-dead-code is supposed to also generate dead code (as opposed to what the name implies in a strict sense).

@nikomatsakis
Copy link
Contributor

Discussed in @rust-lang/compiler meeting. Consensus was that -Clink-dead-code should make compiler generate dead code, as it used to, since there isn't much value in just having the linker preserve dead code (but not having compiler generate it). We considered changing the name but decided that was more trouble than it's worth, and it'd be better to just adjust the -Chelp message to clarify.

@michaelwoerister michaelwoerister self-assigned this Dec 7, 2017
@nikomatsakis
Copy link
Contributor

triage: P-high

@rust-highfive rust-highfive added the P-high High priority label Dec 7, 2017
@arielb1 arielb1 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 21, 2017
bors added a commit that referenced this issue Jan 4, 2018
…xcrichton

Generate code for unused const- and inline-fns if -Clink-dead-code is specified.

Fixes #46467.

r? @alexcrichton
@xd009642
Copy link
Contributor Author

xd009642 commented Jan 4, 2018

Did the associated PR for this manage to make it into 1.23? I'm only asking because I just updated my stable and this issue still seems to be present.

@michaelwoerister
Copy link
Member

Unfortunately, the PR did not make it into 1.23. Version 1.24 will contain the patch though.

In the @rust-lang/compiler meeting we discussed if it would make sense to make a 1.23.1 release that includes the PR, but concluded that we won't do that since support for code coverage tools is not considered production quality yet.

@xd009642, would you mind testing your project with the latest nightly? This is the first version that includes the patch.

@xd009642
Copy link
Contributor Author

xd009642 commented Jan 5, 2018

Just tested with nightly and it works, thanks.

@xd009642 xd009642 closed this as completed Jan 5, 2018
@michaelwoerister
Copy link
Member

Thank you, @xd009642!

@nikomatsakis
Copy link
Contributor

@xd009642 is it a problem for you to use nightly for a bit until this fix propagates?

@xd009642
Copy link
Contributor Author

xd009642 commented Jan 6, 2018

So personally it's not a problem for me, however I make an application with users so this does impact the user experience. I've put a note at the top of the project readme which will hopefully mitigate any issues people face and the early February release date for 1.24 lessens the impact as well.

RE the support for code coverage tools not being considered production quality, I do think there is a slight difference between being something supported by the compiler (in a production ready form) versus being hampered by the compiler. For example, if I was to mitigate this in my tool I'd have to increase the impact of the syntax analysis portion to identity code the compiler may miss out and issue a release just for 1.23. So while it's not supported by rustc, rustc has impinged code coverage tools.

Personally, I don't think there's a big issue tarpaulin-wise, since it's just a nightly run away from expected behaviour. People choosing to use kcov etc. will find that their coverage tools suddenly don't work as expected and they're getting a lot of false negatives.

I don't want to sound too negative, this is a small thing language-wise, it's just a big thing in code coverage. I trust the rust team will make the right decision for users and am a big fan of the work you guys are doing 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants