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

code-cov: generate dead functions with private/default linkage #91470

Merged
merged 1 commit into from
Dec 11, 2021

Conversation

wesleywiser
Copy link
Member

@wesleywiser wesleywiser commented Dec 2, 2021

As discovered in #85461, the MSVC linker treats weak symbols slightly
differently than unix-y linkers do. This causes link.exe to fail with
LNK1227 "conflicting weak extern definition" where as other targets are
able to link successfully.

This changes the dead functions from being generated as weak/hidden to
private/default which, as the LLVM reference says:

Global values with “private” linkage are only directly accessible by
objects in the current module. In particular, linking code into a module
with a private global value may cause the private to be renamed as
necessary to avoid collisions. Because the symbol is private to the
module, all references can be updated. This doesn’t show up in any
symbol table in the object file.

This fixes the conflicting weak symbols but doesn't address the reason
why we have conflicting symbols for these dead functions. The test
cases added in this commit contain a minimal repro of the fundamental
issue which is that the logic used to decide what dead code functions
should be codegen'd in the current CGU doesn't take into account that
functions can be duplicated across multiple CGUs (for instance, in the
case of #[inline(always)] functions).

Fixing that is likely to be a more complex change (see
#85461 (comment)).

Fixes #85461

@richkadel
Copy link
Contributor

richkadel commented Dec 2, 2021

I don't think this ran run-make-fulldeps (which would mean most coverage tests were not attempted). You may need to bors try, or push it to bors with iffy or something.

@wesleywiser
Copy link
Member Author

It looks like the run-make-fulldeps tests ran here although a fair number are ignored.

https://github.com/rust-lang/rust/runs/4400964544?check_suite_focus=true#step:25:3278

@richkadel
Copy link
Contributor

Oh, OK, great. Then I'm assuming the coverage tests did run, and this looks like good news to me.

@richkadel
Copy link
Contributor

r? @tmandry

@richkadel
Copy link
Contributor

(maybe I'm not allowed to trigger highfive that way on someone else's PR)

@wesleywiser wesleywiser force-pushed the code_coverage_link_error branch 2 times, most recently from 5aa4f7a to a71b51f Compare December 3, 2021 16:42
@wesleywiser wesleywiser changed the title [WIP] Generate dead code-coverage functions with private/default code-cov: generate dead functions with private/default linkage Dec 3, 2021
@rust-log-analyzer

This comment has been minimized.

As discovered in rust-lang#85461, the MSVC linker treats weak symbols slightly
differently than unix-y linkers do. This causes link.exe to fail with
LNK1227 "conflicting weak extern definition" where as other targets are
able to link successfully.

This changes the dead functions from being generated as weak/hidden to
private/default which, as the LLVM reference says:

> Global values with “private” linkage are only directly accessible by
objects in the current module. In particular, linking code into a module
with a private global value may cause the private to be renamed as
necessary to avoid collisions. Because the symbol is private to the
module, all references can be updated. This doesn’t show up in any
symbol table in the object file.

This fixes the conflicting weak symbols but doesn't address the reason
*why* we have conflicting symbols for these dead functions. The test
cases added in this commit contain a minimal repro of the fundamental
issue which is that the logic used to decide what dead code functions
should be codegen'd in the current CGU doesn't take into account that
functions can be duplicated across multiple CGUs (for instance, in the
case of `#[inline(always)]` functions).

Fixing that is likely to be a more complex change (see
rust-lang#85461 (comment)).

Fixes rust-lang#85461
@wesleywiser wesleywiser force-pushed the code_coverage_link_error branch from a71b51f to d5f6b9c Compare December 3, 2021 17:00
@wesleywiser
Copy link
Member Author

I ran the tests locally in a Linux environment and they definitely ran and passed there as well.

r? @tmandry

@tmandry
Copy link
Member

tmandry commented Dec 7, 2021

Looks good. Thanks for the investigation and fix!

@bors r+

@bors
Copy link
Contributor

bors commented Dec 7, 2021

📌 Commit d5f6b9c has been approved by tmandry

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 7, 2021
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 9, 2021
@wesleywiser
Copy link
Member Author

@bors rollup

Code coverage is not tested on perf.rlo so this can't have an impact on measured performance.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 10, 2021
…r, r=tmandry

code-cov: generate dead functions with private/default linkage

As discovered in rust-lang#85461, the MSVC linker treats weak symbols slightly
differently than unix-y linkers do. This causes link.exe to fail with
LNK1227 "conflicting weak extern definition" where as other targets are
able to link successfully.

This changes the dead functions from being generated as weak/hidden to
private/default which, as the LLVM reference says:

> Global values with “private” linkage are only directly accessible by
objects in the current module. In particular, linking code into a module
with a private global value may cause the private to be renamed as
necessary to avoid collisions. Because the symbol is private to the
module, all references can be updated. This doesn’t show up in any
symbol table in the object file.

This fixes the conflicting weak symbols but doesn't address the reason
*why* we have conflicting symbols for these dead functions. The test
cases added in this commit contain a minimal repro of the fundamental
issue which is that the logic used to decide what dead code functions
should be codegen'd in the current CGU doesn't take into account that
functions can be duplicated across multiple CGUs (for instance, in the
case of `#[inline(always)]` functions).

Fixing that is likely to be a more complex change (see
rust-lang#85461 (comment)).

Fixes rust-lang#85461
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 10, 2021
…r, r=tmandry

code-cov: generate dead functions with private/default linkage

As discovered in rust-lang#85461, the MSVC linker treats weak symbols slightly
differently than unix-y linkers do. This causes link.exe to fail with
LNK1227 "conflicting weak extern definition" where as other targets are
able to link successfully.

This changes the dead functions from being generated as weak/hidden to
private/default which, as the LLVM reference says:

> Global values with “private” linkage are only directly accessible by
objects in the current module. In particular, linking code into a module
with a private global value may cause the private to be renamed as
necessary to avoid collisions. Because the symbol is private to the
module, all references can be updated. This doesn’t show up in any
symbol table in the object file.

This fixes the conflicting weak symbols but doesn't address the reason
*why* we have conflicting symbols for these dead functions. The test
cases added in this commit contain a minimal repro of the fundamental
issue which is that the logic used to decide what dead code functions
should be codegen'd in the current CGU doesn't take into account that
functions can be duplicated across multiple CGUs (for instance, in the
case of `#[inline(always)]` functions).

Fixing that is likely to be a more complex change (see
rust-lang#85461 (comment)).

Fixes rust-lang#85461
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 10, 2021
…askrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#90407 (Document all public items in `rustc_incremental`)
 - rust-lang#90897 (Fix incorrect stability attributes)
 - rust-lang#91105 (Fix method name reference in stream documentation)
 - rust-lang#91325 (adjust const_eval_select documentation)
 - rust-lang#91470 (code-cov: generate dead functions with private/default linkage)
 - rust-lang#91482 (Update documentation to use `from()` to initialize `HashMap`s and `BTreeMap`s)
 - rust-lang#91524 (Fix Vec::extend_from_slice docs)
 - rust-lang#91575 (Fix ICE on format string of macro with secondary-label)
 - rust-lang#91625 (Remove redundant [..]s)
 - rust-lang#91646 (Fix documentation for `core::ready::Ready`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b7b4d77 into rust-lang:master Dec 11, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to link regex crate with "-Z instrument-coverage" on x86_64-pc-windows-msvc
7 participants