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.

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

x.py should check host's version of libstdc++ before defaulting to download-ci-llvm #123037

Closed
pnkfelix opened this issue Mar 25, 2024 · 5 comments · Fixed by #125411
Closed

x.py should check host's version of libstdc++ before defaulting to download-ci-llvm #123037

pnkfelix opened this issue Mar 25, 2024 · 5 comments · Fixed by #125411
Assignees
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Mar 25, 2024

Spawned off of #110472 (comment):

Per https://pkgs.org/search/?q=libstdc%2B%2B it looks like Amazon Linux 2 uses libstdc++ 7, which is ABI incompatible with the libLLVM.so for download-ci-llvm. I believe you need at least libstdc++ 8.

Possibly the build system should check the version.

I'm not 100% sure what would be involved in such a check here, but it seems like a good thing to investigate, because the nature of the resulting bootstrap failures is super frustrating.

(I'll admit, I don't know if the right thing would be to hard error out of all uses of download-ci-llvm for potentially-incompatible libstdc++, or only to hard error out for defaulting uses... but a warning seems warranted in any such cases.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 25, 2024
@pnkfelix pnkfelix added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Mar 25, 2024
@pnkfelix pnkfelix changed the title x.py should check host's version of libstdc++ before doing download-ci-llvm x.py should check host's version of libstdc++ before defaulting to download-ci-llvm Mar 25, 2024
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Mar 25, 2024

I guess the problem here is that we're compiling the Rust LLVM wrapper code locally and linking that against the downloaded toolchain. A check like suggested here makes sense to me in the meantime, but I suppose long-term the "right" fix here is to get rid of any C++ code we need to separately compile by upstreaming C bindings into LLVM proper, and then we can just call those on the Rust side without any ABI issues.

(FWIW, I was initially confused since we explicitly statically link in the stdcpp in the distributed libLLVM.so. I guess maybe we could also fix this by skipping linking to a local stdcpp while compiling the Rust wrapper code? Not sure if that works out.)

@onur-ozkan onur-ozkan removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 25, 2024
@jieyouxu jieyouxu added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Mar 26, 2024
@onur-ozkan onur-ozkan self-assigned this May 22, 2024
@onur-ozkan
Copy link
Member

(FWIW, I was initially confused since we explicitly statically link in the stdcpp in the distributed libLLVM.so. I guess maybe we could also fix this by skipping linking to a local stdcpp while compiling the Rust wrapper code? Not sure if that works out.)

This is the particular line

println!("cargo:rustc-link-lib={stdcppname}");

which leads to failure with download-ci-llvm when there is an ABI mismatch. In my experiment removing it resulted in linking errors on rustc_driver.

I suppose long-term the "right" fix here is to get rid of any C++ code we need to separately compile by upstreaming C bindings into LLVM proper, and then we can just call those on the Rust side without any ABI issues.

I agree, this solves the problem completely without applying hacks (in my opinion, there is no simple or clean way of comparing ABI or libstd versions) on bootstrap. Maybe, for now, we should just print a warning when downloading ci llvm?

@Mark-Simulacrum
Copy link
Member

You mean always print a warning? I don't think that's a good idea, it's noisy and we generally expect that people are happy using download CI LLVM. I don't think a warning is helpful here if we can't scope it down to where it matters (in which case it can probably be an error).

@onur-ozkan
Copy link
Member

Not always, only once while downloading it.

@Mark-Simulacrum
Copy link
Member

I think that falls in the same "not particularly helpful" case. Most users will miss the message, and even if they don't it's not clear how they should respond. I at least don't know how to check whether my machine has everything setup right. If we could check then we could limit the warning, in which case I'm broadly in favor of making it perhaps even a hard error.

jieyouxu added a commit to jieyouxu/rust that referenced this issue May 28, 2024
… r=Mark-Simulacrum

check host's libstdc++ version when using ci llvm

If the host's libstdc++ version is too old using ci-llvm may result in an ABI mismatch between the local libstdc++ and libLLVM.so. This PR adds a sanity check to immediately fail at the beginning of the bootstrap before starting the actual build. I am not sure if '8' is the best threshold, but it should be a good start and we can increase it anytime if needed.

Fixes rust-lang#123037
bors added a commit to rust-lang-ci/rust that referenced this issue May 28, 2024
…=Mark-Simulacrum

check host's libstdc++ version when using ci llvm

If the host's libstdc++ version is too old using ci-llvm may result in an ABI mismatch between the local libstdc++ and libLLVM.so. This PR adds a sanity check to immediately fail at the beginning of the bootstrap before starting the actual build. I am not sure if '8' is the best threshold, but it should be a good start and we can increase it anytime if needed.

Fixes rust-lang#123037
workingjubilee added a commit to workingjubilee/rustc that referenced this issue May 28, 2024
… r=Mark-Simulacrum

check host's libstdc++ version when using ci llvm

If the host's libstdc++ version is too old using ci-llvm may result in an ABI mismatch between the local libstdc++ and libLLVM.so. This PR adds a sanity check to immediately fail at the beginning of the bootstrap before starting the actual build. I am not sure if '8' is the best threshold, but it should be a good start and we can increase it anytime if needed.

Fixes rust-lang#123037
bors added a commit to rust-lang-ci/rust that referenced this issue May 28, 2024
…=Mark-Simulacrum

check host's libstdc++ version when using ci llvm

If the host's libstdc++ version is too old using ci-llvm may result in an ABI mismatch between the local libstdc++ and libLLVM.so. This PR adds a sanity check to immediately fail at the beginning of the bootstrap before starting the actual build. I am not sure if '8' is the best threshold, but it should be a good start and we can increase it anytime if needed.

Fixes rust-lang#123037
@bors bors closed this as completed in 50297bb Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
5 participants