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.

Sign up for GitHub

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

Jump to bottom

Fix rust.use-lld when linker is not set #76326

Merged
merged 1 commit into from
Sep 6, 2020
Merged

Fix rust.use-lld when linker is not set #76326

merged 1 commit into from
Sep 6, 2020

Conversation

mati865
Copy link
Contributor

@mati865 mati865 commented Sep 4, 2020 •
edited
Loading

Fixes #76127 (comment)

Previously when [<target>].linker was not configured rust.use-lld would set it to rust-lld on platforms where it should not.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 4, 2020
@mati865
Copy link
Contributor Author

mati865 commented Sep 4, 2020

cc @petrochenkov

@Mark-Simulacrum
Copy link
Member

I wonder if we should be warning folks if they do set use-lld and then we are just basically ignoring that.

@mati865
Copy link
Contributor Author

mati865 commented Sep 4, 2020

@Mark-Simulacrum Since #75111 rust.use-lld will add "-Clink-args=-fuse-ld=lld" on non MSVC targets. Before this change when [<target>].linker was not set, rust.use-lld would set [<target>].linker to rust-lld and then also append "-Clink-args=-fuse-ld=lld".

@Mark-Simulacrum
Copy link
Member

Ah right, okay, makes sense. I'm going to @bors r+ this then, but we can back out if people disagree. It sounds like the right thing.

@bors
Copy link
Contributor

bors commented Sep 4, 2020

📌 Commit a2fbf39 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 4, 2020
@petrochenkov
Copy link
Contributor

I can confirm that hangs in #76127 (comment) occur in winpthreads, and that rust-lld shipped with rustc was used, and this PR fixes that.

The fix however causes an error during linking

note: x86_64-w64-mingw32-gcc.exe: error: unrecognized command line option '-fuse-ld=lld'; did you mean '-fuse-ld=bfd'?

which is very strange because x86_64-w64-mingw32-gcc -fuse-ld=lld hello.c works.
Currently investigating.

$ x86_64-w64-mingw32-gcc --version
x86_64-w64-mingw32-gcc.exe (Rev1, Built by MSYS2 project) 10.2.0
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ ld.lld --version
LLD 10.0.1 (https://github.com/Alexpux/MINGW-packages.git 94b687508346d10cec7e8c769785f73da18b4e54) (compatible with GNU linkers)

@mati865
Copy link
Contributor Author

mati865 commented Sep 4, 2020

@petrochenkov at which stage?
Sounds like build/x86_64-pc-windows-gnu/stage0/lib/rustlib/x86_64-pc-windows-gnu/bin/self-contained/x86_64-w64-mingw32-gcc.exe was used but this is totally wrong. Bundled linker should not be used for building Rust.

@petrochenkov
Copy link
Contributor

Sounds like build/x86_64-pc-windows-gnu/stage0/lib/rustlib/x86_64-pc-windows-gnu/bin/self-contained/x86_64-w64-mingw32-gcc.exe was used

Ha, maybe!
It's stage 0, ./x.py build tidy from scratch.

@mati865
Copy link
Contributor Author

mati865 commented Sep 4, 2020 •
edited
Loading

I cannot test it right now but sounds like this could be fixed when #76167 lands in bootstrap compiler.
Alternatively for a quickly fix we can avoid downloading rust-mingw component in bootstrap (this will also marginally improve build times).

@petrochenkov
Copy link
Contributor

This is indeed the reason, the ancient x86_64-w64-mingw32-gcc shipped with rustc is getting used as a linker wrapper.

@petrochenkov
Copy link
Contributor

petrochenkov commented Sep 4, 2020 •
edited
Loading

Alternatively for a quickly fix we can avoid downloading rust-mingw component in bootstrap (this will also marginally improve build times).

We need an external toolchain for x.py anyway to build LLVM? rust-mingw component is not enough?
Did anyone try to x.py with an external pre-build LLVM and no external C/C++ toolchain?

UPD: We have our own rust_llvm with C++ code, so we need an external toolchain and should be able to avoid downloading the rust-mingw component.

@petrochenkov
Copy link
Contributor

(Anyway, the local workaround of nuking

build/x86_64-pc-windows-gnu/stage0/lib/rustlib/x86_64-pc-windows-gnu/bin/self-contained
build/x86_64-pc-windows-gnu/stage0/lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained

works.)

@petrochenkov
Copy link
Contributor

Full ./x.py test --bless --stage 2 on target.x86_64-pc-windows-gnu is confirmed to pass with LLD 🎉

@mati865
Copy link
Contributor Author

mati865 commented Sep 4, 2020

Alternatively for a quickly fix we can avoid downloading rust-mingw component in bootstrap (this will also marginally improve build times).

We need an external toolchain for x.py anyway to build LLVM? rust-mingw component is not enough?
Did anyone try to x.py with an external pre-build LLVM and no external C/C++ toolchain?

UPD: We have our own rust_llvm with C++ code, so we need an external toolchain and should be able to avoid downloading the rust-mingw component.

rust-mingw doesn't have headers, assembler and ships only some of the mingw-w64 libs. It can be used only for linking.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 5, 2020
@Dylan-DPC
Rollup merge of rust-lang#76326 - mati865:use_lld-no-linker, r=Mark-S…
16e7251 
…imulacrum

Fix rust.use-lld when linker is not set

Fixes rust-lang#76127 (comment)

Previously when `[<target>].linker` was not configured `rust.use-lld` would set it to `rust-lld` on platforms where it should not.
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Sep 5, 2020
Closed
@petrochenkov petrochenkov mentioned this pull request Sep 5, 2020
Merged
@petrochenkov
Copy link
Contributor

The rust-mingw component change is submitted in #76381.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 5, 2020
@Dylan-DPC
Rollup merge of rust-lang#76326 - mati865:use_lld-no-linker, r=Mark-S…
18b785e 
…imulacrum

Fix rust.use-lld when linker is not set

Fixes rust-lang#76127 (comment)

Previously when `[<target>].linker` was not configured `rust.use-lld` would set it to `rust-lld` on platforms where it should not.
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Sep 5, 2020
Closed
@bors
Copy link
Contributor

bors commented Sep 6, 2020

⌛ Testing commit a2fbf39 with merge b40abfd...

@bors
Copy link
Contributor

bors commented Sep 6, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing b40abfd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 6, 2020
@bors bors merged commit b40abfd into rust-lang:master Sep 6, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 6, 2020
@bors bors mentioned this pull request Sep 6, 2020
Merged
@mati865 mati865 deleted the use_lld-no-linker branch September 6, 2020 07:51
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2020
@bors
Auto merge of rust-lang#76381 - petrochenkov:nomingwcomp, r=Mark-Simu…
f5b7dd8 
…lacrum

rustbuild: Do not use `rust-mingw` component when bootstrapping windows-gnu targets

Addresses rust-lang#76326 (comment) (ancient `x86_64-w64-mingw32-gcc` is selected as a linker wrapper, which is not usable in `use_lld=true` mode).

Perhaps the comment about incompatible mingw was true in the past, but many things changed since then.
With this change I was able to build everything successfully locally using a newer mingw toolchain, if it passes through the older toolchain on CI, then it should be good, I think.
@jyn514 jyn514 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Sep 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.
7 participants
@mati865 @rust-highfive @Mark-Simulacrum @bors @petrochenkov @jyn514 @rustbot