Use the correct crt*.o files when linking musl targets. #50105
Conversation
pietroalbini
added
the
S-waiting-on-review
|
Thanks for this PR! Assigned a reviewer to it. |
|
Ping from triage @Mark-Simulacrum ! This PR needs your review. |
|
I noticed (some days ago, but pushed some minutes ago, in case github using the commit date confused anyone) that bootstrapping with host = build was not possible from the binaries I produced from a glibc system, which the two additional commits fix. (I incuded them in this pull request because they depend on this change) I also updated the build script I used for testing to work on systems with http_parser.h in /usr/include while cross-compiling. |
src/bootstrap/builder.rs
Outdated
| @@ -586,6 +586,29 @@ impl<'a> Builder<'a> { | |||
| extra_args.push_str(&s); | |||
| } | |||
|
|
|||
| let mut target_features = Vec::new(); | |||
There was a problem hiding this comment.
I would prefer to keep this logic inside rustc.rs instead of through RUSTFLAGS.
There was a problem hiding this comment.
As noted in the commit message some tools are built with a compiler requirement of stage = 0 and build = host, which causes them not to go through rustc.rs:
Lines 509 to 515 in 25749ad
Lines 1201 to 1203 in 25749ad
When deciding how to fix it (I could as well try to make it go through rustc.rs even for stage = 0 and build = host, though my first attempt at it broke the build completely) I was going by the comment at the start of rustc.rs that suggests this is the way forward for some parts of rustc.rs:
rust/src/bootstrap/bin/rustc.rs
Lines 24 to 26 in 25749ad
There was a problem hiding this comment.
I believe all invocations of rustc are intended to go through rustc.rs -- the code you point to doesn't quite indicate to me that rustc is being invoked directly. I personally prefer using the rustc shim over rustflags when possible since it's easier to read and interpret (at least for me).
There was a problem hiding this comment.
It seems I was misreading the code and you are right. The result of the function I linked seems to mainly be used to set the environment variable RUSTC_REAL, while setting RUSTC to the shim.
This leaves only the logic in rustc.rs that disables most of the flag passing if --target wasn't passed as culprit for my build failure.
There was a problem hiding this comment.
This should be done properly now in the newest version this pull request.
|
Not sure who should review this, but not familiar enough with our management of CRTs on various platforms myself. |
|
☔ The latest upstream changes (presumably #50228) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Thanks for the PR @mixi! I wonder, is a new feature needed here? Could we avoid passing the objects if |
|
The problem (#36710) also affects static linking. See my example poc with -crt-static: One thing that could be done on top of this is to default to Another option would be to completely fix the included start files, including A problem I'm not aware of any other problems with just replacing the startfiles, but that doesn't mean there aren't any. Musl's upstream usually recommends against using the |
|
@mixi hm true but so one point is that adding another stable feature like |
|
☔ The latest upstream changes (presumably #50188) made this pull request unmergeable. Please resolve the merge conflicts. |
This fixes rust-lang#36710 with -crt-static.
Bootstrap requires serde_derive, which needs proc-macro crate types, so it won't work with crt-static.
This fixes rust-lang#36710 with +crt-static. We only need to add crtbegin.o and crtend.o as we only do static linking with the bundled start files and there is no static-pie support in rustc yet.
|
The latest commits now do a combination of the proposed alternatives:
With this it should be "good enough" for now, even though I'd like to some day support using the system-provided libc.a and startfiles even when linking statically. |
mixi
changed the title
|
@mixi could we avoid passing objects entirely if |
|
@alexcrichton that is exactly what this patch does. For |
|
Ah ok I see! If the extra code here is fixing bugs, can tests be added for the fixes? |
Yes, of course. Though it was a bit difficult to decide how to test for it without compiling C++ code. The current test checks for the internal symbol |
|
Looking over this again, won't this run a risk of breaking the distributed musl target? We're not shipping crtend/crtbegin files, so by default those targets are going to start requiring them, right? |
|
Yes, they are required, but both all glibc-targeting and all musl-targeting compilers already ship their own crtend/crtbegin files (e.g. here) and they are compatible. So as long as there is a compiler for the target (which is already required, it's just not required to be a musl-targeting compiler) these files exist and the compiler will be able to link in its copy of crtbegin.o/crtend.o. This means it shouldn't™ break any case that currently works. |
bors
added
S-waiting-on-review
|
"LNK1181 Cannot open input file" in the test |
kennytm
added
S-waiting-on-author
|
It was an incompatibility between GCC and MSVC CLI option syntax (and of course me just assuming GCC syntax in the test). I dusted off an old windows to actually test it and the test should now be fixed on MSVC for good. |
|
@bors: r+ |
|
📌 Commit 490d050 has been approved by |
bors
added
S-waiting-on-bors
Use the correct crt*.o files when linking musl targets. This is supposed to support optionally using the system copy of musl libc instead of the included one if supported. This currently only affects the start files, which is enough to allow building rustc on musl targets. Most of the changes are analogous to crt-static. Excluding the start files is something musl based distributions usually patch into their copy of rustc: - https://github.com/alpinelinux/aports/blob/eb064c8/community/rust/musl-fix-linux_musl_base.patch - https://github.com/voidlinux/void-packages/blob/77400fc/srcpkgs/rust/patches/link-musl-dynamically.patch For third-party distributions that not yet carry those patches it would be nice if it was supported without the need to patch upstream sources. ## Reasons ### What breaks? Some start files were missed when originally writing the logic to swap in musl start files (gcc comes with its own start files, which are suppressed by -nostdlib, but not manually included later on). This caused #36710, which also affects rustc with the internal llvm copy or any other system libraries that need crtbegin/crtend. ### How is it fixed? The system linker already has all the logic to decide which start files to include, so we can just defer to it (except of course if it doesn't target musl). ### Why is it optional? In #40113 it was first tried to remove the start files, which broke compiling musl-targeting static binaries with a glibc-targeting compiler. This is why it eventually landed without removing the start files. Being an option side-steps the issue. ### Why are the start files still installed? This has the nice side-effect, that the produced rust-std-* binaries can still be used by on a glibc-targeting system with a rustc built against glibc. ## Does it work? With the following build script (using [musl-cross-make](https://github.com/richfelker/musl-cross-make)): https://shadowice.org/~mixi/rust-musl/build.sh, I was able to cross-compile a musl-host musl-targeting rustc on a glibc-based system. The resulting binaries are at https://shadowice.org/~mixi/rust-musl/binaries/. This also requires #50103 and #50104 (which are also applied to the branch the build script uses).
|
☀️ Test successful - status-appveyor, status-travis |
|
I don't have a ton of context on this change, but my |
|
Oops sorry @BurntSushi! I've posted a revert of this at #50709 The issue I think is that crtbegin/crtend aren't present for i686 (when cross-compiling), so the new arguments to the linker here are causing the compilation to fail. |
|
Ah awesome, thanks! And no worries. This is why I test on nightly CI. :-) It happens! |
Revert #50105 until regression is fixed Discovered at #50105 (comment) it looks like this caused a regression with i686 musl, so let's revert in the meantime while a fix is worked out
Fix building rustc on and for musl hosts. This fixes all problems I had when trying to compile rustc on a musl-based distribution (with `crt-static = false` in `config.toml`). This is a fixed version of what ended up being #50105, making it possible to compile rustc on musl targets. The differences to the old (now merged and subsequently reverted) pull request are: - The commit (6d9154a) that caused the regression for which the original commits were reverted in #50709 is left out. This means the corresponding bug #36710 is still not fixed with `+crt-static`. - The test for issue 36710 is skipped for musl targets (until the issue is properly fixed). - Building cargo-vendor if `crt-static = false` is needed was broken (cargo-vendor links to some shared libraries if they exist on the system and this produces broken binaries with `+crt-static`) CC @alexcrichton
Revert #50105 until regression is fixed Discovered at rust-lang/rust#50105 (comment) it looks like this caused a regression with i686 musl, so let's revert in the meantime while a fix is worked out
This is supposed to support optionally using the system copy of musl
libc instead of the included one if supported. This currently only
affects the start files, which is enough to allow building rustc on musl
targets.
Most of the changes are analogous to crt-static.
Excluding the start files is something musl based distributions usually patch into their copy of rustc:
For third-party distributions that not yet carry those patches it would be nice if it was supported without the need to patch upstream sources.
Reasons
What breaks?
Some start files were missed when originally writing the logic to swap in musl start files (gcc comes with its own start files, which are suppressed by -nostdlib, but not manually included later on). This caused #36710, which also affects rustc with the internal llvm copy or any other system libraries that need crtbegin/crtend.
How is it fixed?
The system linker already has all the logic to decide which start files to include, so we can just defer to it (except of course if it doesn't target musl).
Why is it optional?
In #40113 it was first tried to remove the start files, which broke compiling musl-targeting static binaries with a glibc-targeting compiler. This is why it eventually landed without removing the start files. Being an option side-steps the issue.
Why are the start files still installed?
This has the nice side-effect, that the produced rust-std-* binaries can still be used by on a glibc-targeting system with a rustc built against glibc.
Does it work?
With the following build script (using musl-cross-make): https://shadowice.org/~mixi/rust-musl/build.sh, I was able to cross-compile a musl-host musl-targeting rustc on a glibc-based system. The resulting binaries are at https://shadowice.org/~mixi/rust-musl/binaries/. This also requires #50103 and #50104 (which are also applied to the branch the build script uses).