Unable to link regex crate with "-Z instrument-coverage" on x86_64-pc-windows-msvc #85461
Comments
jonas-schievink
added
the
A-code-coverage
|
I'm adding a note here because my brain gets confused by github's bold-text "mentioned this issue" notes. I see the "No counters provided the source-hash" and jump to the wrong conclusion that this is related. (I think I've done this more than once.) So, NOTE TO SELF, this issue is NOT related to the "No counters provided the source hash" bug... Thanks and sorry for the churn... 😄 |
Even worth, I suspect you deleted your previous comment 😋. In my mail box I got from github on your behalf, I have the message "This should be fixed. There was a brief window in the nightly builds (less than a week I think) where this was a known bug. It was resolved."), but I have no trace of it in the conversation, so I am even more lost with just above message. If you wish you can edit messages instead of deleting. Users can see that your message was edited and they can access edition history to see what changed. But considering all the effort you are giving with instrument coverage, and how important this is to Rust (in my humble opinion), you are fully forgiven 😄 |
|
I see |
|
@wesleywiser I rarely have a useable windows configuration for debugging coverage. Any chance you can look into this? Also note the theory and workaround suggested in microsoft/windows-rs#1006 (comment) I think stabilizing the flag does not imply every edge case is understood and resolved so it may not block stabilization, but I haven't had any time to look into this one since it was filed. I suspect it has to do with generating function symbols for unused functions in the covmap generation code but not sure if something else is involved (macros were mentioned in the linked comment). Since I'm out for 3 weeks I'm not going to have a chance to look at this for at least that long. Seems very rare too. Thanks! Cc: @tmandry |
|
@richkadel Yep, I'll take a look this week! |
I hit this with the |
* -C codegen-units=1 rust-lang/rust#85461 * Look for profraws in workspace folders * Upgrade llvm-profparser to one that handles merging no files
101: Force 1 codegen unit r=taiki-e a=taiki-e Workaround for rust-lang/rust#85461 See also microsoft/windows-rs#1006 (comment) Co-authored-by: Taiki Endo <[email protected]>
101: Force 1 codegen unit on windows r=taiki-e a=taiki-e Workaround for rust-lang/rust#85461 See also microsoft/windows-rs#1006 (comment) Co-authored-by: Taiki Endo <[email protected]>
|
@wesleywiser - Did you have any opportunity to look into this? |
|
I'm still looking into this but I have managed to construct a minimal (I believe) repro: # Cargo.toml
[package]
name = "memchr"
version = "2.4.1"
edition = "2018"// src/main.rs
use memchr::memchr;
fn main() {
memchr();
}// src/lib.rs
mod cow {
#[derive(Debug)]
struct CowBytes<'a>(&'a ());
#[derive(Debug)]
enum Imp<'a> {
Owned(&'a [u8]),
}
}
pub fn memchr() {
static FN: std::sync::Once = std::sync::Once::new();
let _f = &FN.is_completed();
}
mod memmem;// src/memmem/mod.rs
mod genericsimd {
use crate::memmem::PrefilterFnTy;
#[inline(always)]
pub(crate) fn find(
haystack: &(),
fallback: PrefilterFnTy,
) { }
fn find_in_chunk2() { }
}
mod x86 {
pub(crate) mod avx {
use core::arch::x86_64::__m256i;
use crate::memmem::PrefilterFnTy;
const _: PrefilterFnTy = find;
pub(crate) fn find(
haystack: &(),
) {
super::super::genericsimd::find(
haystack,
super::sse::find2,
)
}
}
pub(crate) mod sse {
use core::arch::x86_64::__m128i;
pub(crate) fn find2(
haystack: &(),
) {
super::super::genericsimd::find(
haystack,
|_| { },
)
}
}
}
#[derive(Debug)]
struct NeedleInfo { }
type PrefilterFnTy = fn(
haystack: &(),
);Which, with the appropriate flags, will then trigger the link error:
There's quite a lot of dead code in the "minimal" repro but removing any of it or even moving it around causes the error to go away. We need basically this exact structure to get the codegen units to be placed into the state which triggers this bug. |
|
Thank you! Having a reproducible example like this is still very helpful. It's not a stretch to assume the trigger relates to the CGU-specific logic in My educated guess (still a guess though) is that we're trying to I will try your example on Linux as well, just to be certain this reproducible case really is Windows specific. If it is the Some half-baked theories:
|
|
It might also be worth logging the |
|
@wesleywiser - What is the filename for the last chunk of rust code? It doesn't appear to be referenced from any of the other files, unless I'm missing something. I wonder why it gets pulled into the compilation? |
|
@richkadel Woops, sorry about that! I've updated the comment to include the file name. It's pulled in by the
Codegen on Linux does use multiple cgus (especially when
I haven't had any issue reproducing with this source (ie, it's not intermittent when the source code is not changed). So it doesn't feel like a parallelism/determinism issue to me.
This seems like the most likely issue to me. I'm digging into the symbol table and LLVM IR ... |
|
FYI, I just compiled this on my Linux machine, using This log is printed twice, with two different cgu names: |
|
(In case you'd like to try the same, I added this line right before the call to debug!("define_unused_fn({:?}) in CGU: {}", def_id, cx.codegen_unit.name()); |
|
Here's another (surprising? to me anyway) clue: I added a debug log here: let mut first_covered_def_id_by_file: FxHashMap<Symbol, DefId> = FxHashMap::default();
for &def_id in sorted_codegenned_def_ids.iter() {
if let Some(covered_file_name) = tcx.covered_file_name(def_id) {
// Only add files known to have unused functions
if unused_def_ids_by_file.contains_key(covered_file_name) {
debug!(
"CGU {} compiles {:?} from {}",
cx.codegen_unit.name(),
def_id,
covered_file_name
);
first_covered_def_id_by_file.entry(*covered_file_name).or_insert(def_id);
}
}
}And two CGU's ( Doesn't this mean that the two CGUs are redundantly generating code for the same 5 functions (6 including the undefined function coverage is adding)? Is there something about the symbols generated for the "used" functions shown here that makes them unique within their CGU, so Windows is not getting linker errors on the used functions as well? Wishful thinking, maybe this identifies an opportunity to improve compiler performance, but more likely I just misunderstood the relationship between the CGUs and the results from the call to |
|
Ok, that was super helpful and pointed me in the direction of the actual minimal repro. Thanks @richkadel! // lib.rs
mod foo {
#[inline(always)]
pub fn called() { }
fn uncalled() { }
}
pub mod bar {
pub fn call_me() {
super::foo::called();
}
}
pub mod baz {
pub fn call_me() {
super::foo::called();
}
}// repro.rs
use lib::{bar, baz};
fn main() {
bar::call_me();
baz::call_me();
}> rustc +nightly --crate-type lib -Zinstrument-coverage --crate-name lib lib.rs
> rustc +nightly --edition 2021 --extern lib=liblib.rlib repro.rs
error: linking with `link.exe` failed: exit code: 1227
|
= note: liblib.rlib(lib.lib.f2c86790-cgu.0.rcgu.o) : fatal error LNK1227: conflicting weak extern definition for '_RNvNtCslAVHmXkNn7E_3lib3foo8uncalled'. New default '.weak._RNvNtCslAVHmXkNn7E_3lib3foo8uncalled.default._RNvNtCslAVHmXkNn7E_3lib3baz7call_me' conflicts with old default '.weak._RNvNtCslAVHmXkNn7E_3lib3foo8uncalled.default._RNvNtCslAVHmXkNn7E_3lib3bar7call_me' in liblib.rlib(lib.lib.f2c86790-cgu.1.rcgu.o)Since I'm a bit curious why we use rust/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs Lines 215 to 216 in b27661e Using Changing to use those attributes instead seems like a quick, easy fix to resolve this for now. A better solution might be to reserve one CGU and put all of the dead code in that CGU which would sidestep the entire issue by ensuring there is only one CGU with these functions in it. |
Let's try it! Are you planning to submit the PR or do you want me to.
I think this is harder than it might seem on the surface. If I recall correctly, this was something I looked into, but the information required was not where I needed it, after CGU partitioning. Someone may know a way to do it, but I just want to caution that it might not be that trivial. |
|
Let's see if CI passes 🤞 #91470 |
|
Historical context: See #83307 (comment) ... My depth of understanding of all of the different linkage options is still more shallow than I'd like. I read the LLVM documentation descriptions several times, but the number of combinations, and their effects, are still hard for me to understand. Some choices I had to make were an "educated guess" on my part. As you can see, MOST of the time, the current linkage works, but if your suggested changes resolve these issues without breaking anything else, that's fantastic. |
|
Ah, thanks! I won't claim my understanding of the linkage options is complete either, but I think |
…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
…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
…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
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
rust-lang/rust#85461 is now fixed on Nightly
…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! 🎄 🎁
…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! 🎄 🎁
* -C codegen-units=1 rust-lang/rust#85461 * Look for profraws in workspace folders * Upgrade llvm-profparser to one that handles merging no files
* -C codegen-units=1 rust-lang/rust#85461 * Look for profraws in workspace folders * Upgrade llvm-profparser to one that handles merging no files
* -C codegen-units=1 rust-lang/rust#85461 * Look for profraws in workspace folders * Upgrade llvm-profparser to one that handles merging no files
199: Don't emit `codegen-units=1` when `stable_coverage` is set r=taiki-e a=ldm0 Do the same thing in <https://github.com/taiki-e/cargo-llvm-cov/pull/130/files#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcR204> since rust-lang/rust#85461 is already closed. Co-authored-by: Liu Dingming <[email protected]>
* -C codegen-units=1 rust-lang/rust#85461 * Look for profraws in workspace folders * Upgrade llvm-profparser to one that handles merging no files
* -C codegen-units=1 rust-lang/rust#85461 * Look for profraws in workspace folders * Upgrade llvm-profparser to one that handles merging no files
Hello,
Linking regex crate on
x86_64-pc-windows-msvcfails when using-Z instrument-coverage.The link succeeds when using
-Z instrument-coverage=except-unused-functionsI expected to see this happen: linking successful.
Instead, this happened:
Meta
rustc --version --verbose:The text was updated successfully, but these errors were encountered: