Debug builds of compiler_builtins fail to link due to references to precondition_check in libcore #121552
Comments
rustbot
added
the
needs-triage
Amanieu
changed the title
|
You mentioned |
saethlin
added
A-linkage
saethlin
added
T-compiler
Avoid lowering code under dead SwitchInt targets The objective of this PR is to detect and eliminate code which is guarded by an `if false`, even if that `false` is a constant which is not known until monomorphization, or is `intrinsics::debug_assertions()`. The effect of this is that we generate no LLVM IR the standard library's unsafe preconditions, when they are compiled in a build where they should be immediately optimized out. This mono-time optimization ensures that builds which disable debug assertions do not grow a linkage requirement against `core`, which compiler-builtins currently needs: rust-lang#121552 This revives the codegen side of rust-lang#91222 as planned in rust-lang#120848.
Amanieu
added
the
I-prioritize
|
Hm didn't notice this because it was not marked as a regression. Assigning P-high (Zulip discussion) after reading @Amanieu comment (and following). @rustbot label -I-prioritize +P-high |
rustbot
added
P-high
|
It's a nightly-to-nightly regression, I don't think we have a label for that? Nightly features can break after all. It's just that compiler-builtins relies on an incredibly fragile undocumented contract that is not checked by rustc CI. |
Thanks for the additional context. IIUC in this case it's nothing risking to reach beta if it's shielded by a nightly-only flag. @rustbot label -I-prioritize -P-high +P-medium |
Avoid lowering code under dead SwitchInt targets The objective of this PR is to detect and eliminate code which is guarded by an `if false`, even if that `false` is a constant which is not known until monomorphization, or is `intrinsics::debug_assertions()`. The effect of this is that we generate no LLVM IR the standard library's unsafe preconditions, when they are compiled in a build where they should be immediately optimized out. This mono-time optimization ensures that builds which disable debug assertions do not grow a linkage requirement against `core`, which compiler-builtins currently needs: rust-lang#121552 This revives the codegen side of rust-lang#91222 as planned in rust-lang#120848.
|
I checked this with the latest nightly: it seems new failures are due to However I would have expected #121421 to address this since all of these are gated around code like this: if ::core::intrinsics::$kind() {
precondition_check($($arg,)*);
} |
|
Do you know why the linker errors only happen on Windows? |
|
It now happens on Linux as well. |
|
This works fine on my Linux machine: |
|
The link failure doesn't happen, but that's because (I think) the function is pruned by the linker. If you look at the rlib with |
|
Hmm actually now that I'm re-reading #121421 it only applies to basic blocks within a function and doesn't affect function-level reachability analysis. Perhaps the failure was only observed on msvc because it uses a different linker (MSVC link.exe)? |
|
Right. That PR only avoids lowering the calls, it doesn't touch reachability analysis. The previous PR that I based this on did change reachability analysis and MIR collection, but that approach means that some kind of errors under and |
|
Is there a way to print the linker invocation when I'm building for the MSVC target? As far as I can tell from the code in cg_ssa: rust/compiler/rustc_codegen_ssa/src/back/linker.rs Lines 854 to 865 in 72d7897 which is called from here: rust/compiler/rustc_codegen_ssa/src/back/link.rs Lines 2408 to 2423 in 72d7897
we are already asking MSVC to eliminate the failing-to-link code. So either I'm reading this wrong (quite possible) or MSVC's linker optimization here is quite poor, or this is an MSVC linker bug. |
Maybe try something like: |
|
I'm doing a new CI run in rust-lang/compiler-builtins#582, this time specifically ignoring whether the symbols are referenced from the .rlib and instead only checking whether linking succeeds. |
|
Still failing on MSVC: |
|
I disabled fast-fast in CI so we can get a clearer picture of the failures: https://github.com/Amanieu/rustc-builtins/actions/runs/8301447504 It seems to also fail on windows-gnu targets: |
|
We're passing the linker flags Some of the undefined references here double-checking situations that ought to be impossible. We could simply delete these checks: master...saethlin:rust:some-bad-ideas diff --git a/library/core/src/intrinsics.rs b/library/core/src/intrinsics.rs
index 86b9a39d68a..2d1a81c3193 100644
--- a/library/core/src/intrinsics.rs
+++ b/library/core/src/intrinsics.rs
@@ -2844,9 +2844,7 @@ fn runtime(src: *const (), dst: *const (), size: usize, count: usize) -> bool {
let src_usize = src.addr();
let dst_usize = dst.addr();
let Some(size) = size.checked_mul(count) else {
- crate::panicking::panic_nounwind(
- "is_nonoverlapping: `size_of::<T>() * count` overflows a usize",
- )
+ crate::intrinsics::abort()
};
let diff = src_usize.abs_diff(dst_usize);
// If the absolute distance between the ptrs is at least as big as the size of the buffer,
diff --git a/library/core/src/ptr/const_ptr.rs b/library/core/src/ptr/const_ptr.rs
index a0c04d3f65d..e57c64df9c0 100644
--- a/library/core/src/ptr/const_ptr.rs
+++ b/library/core/src/ptr/const_ptr.rs
@@ -1630,10 +1630,6 @@ pub const fn is_aligned(self) -> bool
#[unstable(feature = "pointer_is_aligned", issue = "96284")]
#[rustc_const_unstable(feature = "const_pointer_is_aligned", issue = "104203")]
pub const fn is_aligned_to(self, align: usize) -> bool {
- if !align.is_power_of_two() {
- panic!("is_aligned_to: align is not a power-of-two");
- }
-
#[inline]
fn runtime_impl(ptr: *const (), align: usize) -> bool {
ptr.addr() & (align - 1) == 0
diff --git a/library/core/src/ptr/mut_ptr.rs b/library/core/src/ptr/mut_ptr.rs
index 6a9033a144d..f069cce94e9 100644
--- a/library/core/src/ptr/mut_ptr.rs
+++ b/library/core/src/ptr/mut_ptr.rs
@@ -1888,10 +1888,6 @@ pub const fn is_aligned(self) -> bool
#[unstable(feature = "pointer_is_aligned", issue = "96284")]
#[rustc_const_unstable(feature = "const_pointer_is_aligned", issue = "104203")]
pub const fn is_aligned_to(self, align: usize) -> bool {
- if !align.is_power_of_two() {
- panic!("is_aligned_to: align is not a power-of-two");
- }
-
#[inline]
fn runtime_impl(ptr: *mut (), align: usize) -> bool {
ptr.addr() & (align - 1) == 0I would send a PR like that, except that does not remove all the linker errors. On MSVC I still see one error... except that if I also add |
|
Actually, it looks like with |
|
Presumably that would have the effect of splitting it up into more object files which causes the linker to simply drop ones that do not provide a necessary symbol. |
|
Trying to rely on MSVC linker to not pull in external symbols via @saethlin Messing with the code gen units means that the external symbol for panics is landing in an obj that happens to not be pulled in, hence MSVC doesn't complain about it being missing. This is an incredibly fragile solution... If we can't rely on libcore, would it be possible to implement a "mini panic" in compiler-builtins instead? |
I'm not proposing any solution. |
The undefined symbols in this case are only referenced by functions that are never called. So that sounds right. |
|
I think I have a solution. I'll put up a PR soon... |
|
This is my idea: #122580 |
|
This was marked as fixed by #122580, but that PR doesn't change the library or how code is generated, so it can't by itself change anything about the linker errors, can it? |
|
It does fix the issue by replacing all "external" calls with |
|
Ah, makes sense, thanks. |
Likely caused by #120594. cc @saethlin
CI for compiler_builtins is failing with:
The cause seems to be from a combination of factors:
#[inline], since those are codegen'ed into compiler_builtins.rlib.precondition_checkis marked as#[inline(never)], which breaks this.core::intrinsics::debug_assertionsis resolved late in the codegen backend, after any MIR optimizations.ifbranch either.The text was updated successfully, but these errors were encountered: