Merged
Conversation
Contributor
Author
|
CLANG32 is the loner... |
Contributor
Author
|
Some remarks/questions:
@mati865, @jeremyd2019 any hunch as to what is going on ? |
Contributor
Author
|
Interestingly, the CLANG32 build that uses "our" previous Rust (i.e. with |
Closed
Collaborator
|
forgot to say. we've upgraded to llvm 18, but rust will support it only at 1.78.0. try backporting relevant PR update: rust-lang/rust#120055 |
Collaborator
|
Chstk issue is basically lack of this PR: rust-lang/compiler-builtins#575 Rsbegin and rsend crates should not be built in clang envs, probably we have to disable them somewhere in bootstrap crate. |
Contributor
Author
The patch fails to apply (same as here #20337) EDIT: that patch seems to be mostly concerned about building LLVM 18. |
Collaborator
Since our targets still identify as |
Contributor
Author
Just tried that and it did not make a difference. |
Contributor
Author
|
The problematic rsbuild.o and rsend.o are not the result of bootstrap compilation but are installed from the previous rust version. |
Contributor
Author
|
Here is a verbose build up to the first linking error: My understanding is that the missing symbols ( |
Collaborator
|
@MehdiChinoune it will not since it's 64-bit target and both the issues (rt{begin,end} and chsktk) affect only 32-bit platforms and since we have no 32-bit ARM target it affects only i686. |
Collaborator
They are provided by Rust: https://github.com/rust-lang/rust/tree/master/library/rtstartup I had missed the fact it fails during build of bootstrap which complicates things as it means my hack to the bootstrap no longer works for some reason. |
Member
|
How are things (the rust ecosystem I guess) with respect to using the -gnullvm targets instead of patching the -gnu ones (#17466)? Would that avoid this issue? |
Member
|
I'm got the same error trying to build rust 1.76.0 on clang32, when testing to build -gnullvm target. Perhaps it is due to too-new llvm? Oops, that was the chkstk one, there was a PR for that referenced above, let me try that patch. |
Contributor
Author
Are you sure that they are provided by Rust ? Seems that they are referenced but not provided: https://github.com/rust-lang/rust/blob/master/library/rtstartup/rsbegin.rs#L76 EDIT: a search for |
Member
|
OK, register/deregister error building rust on CLANG32 from master... So it's a regression due to what's in the repo, not due to this PR. Back to thinking it is due to llvm update somehow |
Collaborator
Yeah, chkstk is due to too new LLVM.
Ah, sorry I have confused the symbols. Those are (or used to be) provided by libgcc/libunwind. |
Member
|
Looks like libunwind provides |
Contributor
Author
Do we know why and when they were removed ? And what were they replaced with (if anything) ? |
Member
|
llvm/llvm-project@5eb44df (the commit message says default this to on, but it appears to be defaulted to off) |
Collaborator
|
So if we temporarily enable |
Member
|
Running into issues trying to backport rust-lang/compiler-builtins#575 on CLANG32, due to the necessity to not vendor because |
Contributor
Author
|
This package does stuff with manifest : https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-rust/0011-disable-uac-for-installer.patch EDIT: this patch is applied only for CLANG32. |
Collaborator
|
This might be fragile but should work without reenabling vendored deps: ❯ git diff
diff --git a/Cargo.lock b/Cargo.lock
index 3110f32ade9..c2380770c02 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -741,8 +741,7 @@ checksum = "55b672471b4e9f9e95499ea597ff64941a309b2cdbffcc46f2cc5e2d971fd335"
[[package]]
name = "compiler_builtins"
version = "0.1.108"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "d68bc55329711cd719c2687bb147bc06211b0521f97ef398280108ccb23227e9"
+source = "git+https://github.com/kleisauke/compiler-builtins?branch=sync-chkstk#f20144145358bd4149c78617332062f8dd7f864d"
dependencies = [
"cc",
"rustc-std-workspace-core",
diff --git a/Cargo.toml b/Cargo.toml
index 5dd315ef2f7..f752267df82 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -120,6 +120,7 @@ strip = true
rustc-std-workspace-core = { path = 'library/rustc-std-workspace-core' }
rustc-std-workspace-alloc = { path = 'library/rustc-std-workspace-alloc' }
rustc-std-workspace-std = { path = 'library/rustc-std-workspace-std' }
+compiler_builtins = { git = "https://github.com/kleisauke/compiler-builtins", branch = "sync-chkstk"}
[patch."https://github.com/rust-lang/rust-clippy"]
clippy_lints = { path = "src/tools/clippy/clippy_lints" }
|
Member
|
That's got things going further. Will probably need to do something different if we need to backport that here for real. |
Member
|
This is what I get now (oh, in case I didn't make it clear - I'm still trying to build 1.76.0, not this PR): |
Member
|
I'm guessing that your rebase didn't drop (some of) the old commits, since I used a squash-merge on this PR (the history was kind of complex to merge into master IMO). I had to do rebase -i and drop the old commits manually on the branches where I am playing with switching to -gnullvm targets for CLANG64 & CLANG32. |
Collaborator
|
Looks more like corrupt archive, maybe it hasn't downloaded properly? |
Contributor
Author
|
It was a CI build failure. Still building but went past the last failure : https://github.com/msys2/MINGW-packages/pull/16207/checks |
Contributor
Author
|
Build successful. |
Contributor
|
is test supposed to fail? |
Contributor
Author
yes, afaik, the tests have never worked. |
Contributor
ah, we have 15875 done, 195 tests to go, in that case, haha. |
Collaborator
|
Do you have the list of failed tests? |
Contributor
I assume the test that failed is |
Contributor
Author
|
The first failure is: The test: The expected output: The test is actually correct but the test framework fails to "normalize" the std err output. Seems the test should use a versioned hashbrown. There are three hashbrown crates in src/vendor:
I guess it is supposed to use one that has a version. |
Contributor
Author
|
Second failure: |
Contributor
Author
|
PS: the two failures are with MINGW64. |
Collaborator
|
I was referring mostly to:
I assumed those 195 tests are the failures and there is a list somewhere. Hashbrown issue seems worthy reporting upstream, |
Contributor
Author
|
The hashbrown test failure is probably caused by the About the missing symbols: Is it expected to see lib/libmsvcrt.a(lib64_libmsvcrt_common_a-log.o when linking ? |
Collaborator
If you are asking about the name then yes, |
Contributor
Author
|
Removing the About the missing symbol : the |
Member
For me, it is two-fold.
|
Contributor
Author
|
Opened a ticket upstream for the vendoring issue: rust-lang/rust#123688 |
Contributor
Author
|
The hashbrown/vendored issue was already fixed upstream in 1.79.0, so all good. I am starting to look into the missing symbol issue. This issue is kinda strange as sin and log are missing but not cos and other math related functions. |
Member
|
That might be a candidate to ask about on the mingw-w64-public@lists.sourceforge.net list. if it looks like a mingw-w64 thing rather than rust. |
Contributor
Author
|
It is for sure not related to vendored. |
Contributor
Author
|
I'll ask on the mingw-w64 mailing list. In the meantime, anyone knows why there is a weird |
Collaborator
No, the test links
It's build path from the package builder. |
Collaborator
|
The error seems to happen because Rust pulls in let mingw_libs = &[
"-lmsvcrt",
"-lmingwex",
"-lmingw32",
"-lgcc", // alas, mingw* libraries above depend on libgcc
// mingw's msvcrt is a weird hybrid import library and static library.
// And it seems that the linker fails to use import symbols from msvcrt
// that are required from functions in msvcrt in certain cases. For example
// `_fmode` that is used by an implementation of `__p__fmode` in x86_64.
// The library is purposely listed twice to fix that.
//
// See https://github.com/rust-lang/rust/pull/47483 for some more details.
"-lmsvcrt",
"-luser32",
"-lkernel32",
];Due to how ld.bfd works it'd need another If I'm right UCRT and LLD (with any CRT) are unaffected. |
Contributor
Author
|
The OP reported the hashbrown issue on UCRT64 and was not affected by the missing symbols. I'll try to add another EDIT: I overlooked that the link to windows_gnu.rs was given. Thanks... PS: on CLANG we have a patch that nukes the libs list: https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-rust/0007-clang-subsystem.patch#L76 |
Contributor
Author
|
Adding the extra Should the fix be submitted upstream or is it msys2 specific ? |
Member
|
I think it will be necessary upstream to work with newer mingw-w64. Assuming that this was an intentional effect of the mingw-w64 change. You still might want to ask about it on their mailing list. |
Collaborator
|
Definitely upstream, it will be required once Rust upgrades their toolchain. |
Contributor
Author
|
Switching over to #20651 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://blog.rust-lang.org/inside-rust/2024/03/17/1.77.0-prerelease.html