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

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

Merged
merged 6 commits into from
Jun 6, 2024

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented May 22, 2024

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 #123037

@rustbot
Copy link
Collaborator

rustbot commented May 22, 2024

r? @albertlarsan68

rustbot has assigned @albertlarsan68.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels May 22, 2024
@onur-ozkan
Copy link
Member Author

r? Mark-Simulacrum

@rust-log-analyzer

This comment has been minimized.

@onur-ozkan onur-ozkan force-pushed the sanity-check-libstdc++ branch 2 times, most recently from f2d2bc4 to 1c38c46 Compare May 22, 2024 16:45
if !build.config.dry_run() && build.config.llvm_from_ci {
let cxx = build.cxx(build.build).unwrap();
let mut cxx_cmd = Command::new(cxx);
cxx_cmd.arg("-dumpversion");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this checking the compiler version rather than libstdcxx version? These are fairly likely to be the same if the compiler is gcc, but not otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was the case for any c++ compiler

// In case the user has a custom setup that includes a recent libstdc++ with an old c++ compiler, this information might be useful to them.

I have to check if there is a better way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler and the used C++ standard library are independent. gcc will usually use libstdc++ of the same version (but isn't required). clang on Linux will usually use some libstdc++ version -- in this case the compiler version and libstdc++ version have no relation at all. clang on other platforms like macos will use libc++, in which case the clang version and libc++ version will likely match. Of course, libstdc++ and libc++ are ABI incompatible as well :)

(Not sure whether our apple builders link against libstdc++ or libc++.)

Copy link
Member Author

@onur-ozkan onur-ozkan May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check libc++ version too? If so, what would be a good threshold for it?

I suppose we don't need to check for it. ABI mismatch only occurs when using download-ci-llvm and ci llvm is built from our builders which use libstdc++.

@rust-log-analyzer

This comment has been minimized.

@onur-ozkan onur-ozkan added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2024
@rust-log-analyzer

This comment has been minimized.

@onur-ozkan onur-ozkan force-pushed the sanity-check-libstdc++ branch 3 times, most recently from 4c06ca3 to c5755fa Compare May 23, 2024 09:02
@rustbot rustbot added the A-meta Area: Issues & PRs about the rust-lang/rust repository itself label May 23, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 23, 2024

triagebot.toml has been modified, there may have been changes to the review queue.

cc @davidtwco, @wesleywiser

@onur-ozkan
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. A-testsuite Area: The testsuite used to check the correctness of rustc and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 23, 2024
@@ -35,6 +36,10 @@ const STAGE0_MISSING_TARGETS: &[&str] = &[
// just a dummy comment so the list doesn't get onelined
];

/// Minimum version threshold for libstdc++ required when using prebuilt LLVM
/// from CI (with`llvm.download-ci-llvm` option).
const LIBSTDCXX_MIN_VERSION_THRESHOLD: usize = 8;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is minimum really the right thing here? I'd expect that we need an exact match?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless there is a breaking change, different versions can be compatible (just like libc). It doesn't need to be the same version.

@Mark-Simulacrum
Copy link
Member

I suspect we'll hit problems and need to revert & re-land with fixes, just based on something being off (e.g., cxx invocation needing more flags or similar) but this seems OK to start with.

@bors r+

@bors
Copy link
Contributor

bors commented May 27, 2024

📌 Commit 480670e has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 6, 2024
@bors
Copy link
Contributor

bors commented Jun 6, 2024

⌛ Testing commit dd99021 with merge 50297bb...

@bors
Copy link
Contributor

bors commented Jun 6, 2024

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 50297bb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 6, 2024
@bors bors merged commit 50297bb into rust-lang:master Jun 6, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Jun 6, 2024
@onur-ozkan onur-ozkan deleted the sanity-check-libstdc++ branch June 6, 2024 15:18
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (50297bb): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Warning ⚠: The following benchmark(s) failed to build:

  • rustc

cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.3% [0.3%, 0.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary -3.1%, secondary -4.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.1% [-3.1%, -3.1%] 1
Improvements ✅
(secondary)
-4.3% [-4.3%, -4.3%] 1
All ❌✅ (primary) -3.1% [-3.1%, -3.1%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results (secondary -0.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.0% [-0.0%, -0.0%] 1
All ❌✅ (primary) - - 0

Bootstrap: missing data
Artifact size: 319.39 MiB -> 319.43 MiB (0.01%)

@lqd
Copy link
Member

lqd commented Jun 6, 2024

Hum, benchmarking bootstrapping broke 👀

@onur-ozkan
Copy link
Member Author

Interesting.. Is there a way to check build logs of benchmark runner?

@Kobzol
Copy link
Contributor

Kobzol commented Jun 6, 2024

https://perf.rust-lang.org/status.html click on Show 1 error on the right.

@onur-ozkan
Copy link
Member Author

Your system's libstdc++ version is too old for the llvm.download-ci-llvm option.
Current version detected: '7'
Minimum required version: '8'
Consider upgrading libstdc++ or disabling the llvm.download-ci-llvm option.
Building bootstrap
Build completed unsuccessfully in 0:00:16

The runner is using libstdc++ version 7 which is old. Can we use more recent version? The main reason for setting version 8 as the minimum threshold is that version 7 is not compatible with >=8 (see #110472).

@lqd
Copy link
Member

lqd commented Jun 6, 2024

cc @Mark-Simulacrum on that

let compiler = builder.cxx(self.target).unwrap();
let mut cmd = Command::new(compiler);

let executable = out_dir.join("libcxx-version");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@onur-ozkan

Suggested change
let executable = out_dir.join("libcxx-version");
let executable = out_dir.join("libcxx-version.exe");

How did it pass CI on Windows?
It trivially fails for me locally, because there's no libcxx-version, only libcxx-version.exe.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#126086 should handle this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it passed because windows-gnu CI uses env that disables llvm download?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it passed because windows-gnu CI uses env that disables llvm download?

Yeah, most likely.

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jun 6, 2024
use windows compatible executable name for libcxx-version

Resolves #rust-lang#125411 (comment)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 7, 2024
Rollup merge of rust-lang#126086 - onur-ozkan:use-exe, r=petrochenkov

use windows compatible executable name for libcxx-version

Resolves #rust-lang#125411 (comment)
@Kobzol
Copy link
Contributor

Kobzol commented Jun 8, 2024

On the perf collector, we have GCC 7.5, which has libstdcxx release 7, sadly. I would suggest that we revert this PR if we don't find a way to update the GCC on the collector quickly (I'm not sure if updating the GCC there wouldn't cause some side effects).

Btw, I think that we should be setting LIBSTDCXX_MIN_VERSION_THRESHOLD according to what is actually used on our CI, not just an arbitrarily selected number. On the x64 builder, we currently use GCC 9.5.0, which returns 9 for _GLIBCXX_RELEASE.

@onur-ozkan
Copy link
Member Author

It's not arbitrarily selected number, see #110472 (comment).

@Kobzol
Copy link
Contributor

Kobzol commented Jun 8, 2024

Right, but the fact that 8 works in this case isn't a general fix. If the .so file is built with 9, shouldn't the minimum also be 9? Maybe for libLLVM.so specifically in this case 8 is indeed enough for everyone, but it seems difficult to find the correct version, in general (e.g. once we update GCC on CI sometime in the future).

@onur-ozkan
Copy link
Member Author

Setting the threshold to match the version we use in the CI builder would be a solid fix, but it may limit the number of people who can utilize CI LLVM. AFAIK, previous libstdcxx versions can be compatible unless they are excessively outdated.

@onur-ozkan
Copy link
Member Author

it may limit the number of people who can utilize CI LLVM

The trade-off here is either ensuring that the threshold version is always correct (by ensuring it matches with the libstdcxx version in CI LLVM builder using some script) or manually finding and updating the version in the bootstrap. Your suggestion seems much more reliable here.

@nikic
Copy link
Contributor

nikic commented Jun 8, 2024

The specific problem with libstdc++ 7 here is that it did not yet have a stable C++ 17 ABI. It is generally not necessary to match the libstdc++ version used to build libLLVM.so exactly (the worst that can usually happen is undefined symbol errors, and I think even that isn't possible because we're linking libstdc++ statically).

The C++ 17 ABI is only officially stable since libstdc++ 9, but in practice the ABI of types currently used by the libLLVM <-> rustc_llvm interface didn't change between 8 -> 9 either, which is why it works as a minimum version.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 9, 2024
…ck, r=<try>

Remove libstdc++ version check error

This keeps the error message from rust-lang#125411, but removes the `exit(1)` call.

This PR is mostly a hotfix to unblock bootstrap benchmarks in rustc-perf.

However, I think that it might be better to just print a warning, in general. If the ABI version does not match, the build might or might not work locally (as we can see on rustc-perf, where it works even if the reported ABI is 7).

If it does not work (and **if** we can always recognize this during the LLVM wrapper build, instead of having some silent miscompilations), then the user will have to update their libstdc++ anyway, the error does not help them out on its own. So it should be enough to just provide a better error message, without blocking the build.

But I'm not adamant on that, I just want to unblock bootstrap benchmarks until we can find a way to update libstdc++ on the collector machine.

CC `@onur-ozkan`

r? `@Mark-Simulacrum`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 10, 2024
…ck, r=Mark-Simulacrum

Remove libstdc++ version check error

This keeps the error message from rust-lang#125411, but removes the `exit(1)` call.

This PR is mostly a hotfix to unblock bootstrap benchmarks in rustc-perf.

However, I think that it might be better to just print a warning, in general. If the ABI version does not match, the build might or might not work locally (as we can see on rustc-perf, where it works even if the reported ABI is 7).

If it does not work (and **if** we can always recognize this during the LLVM wrapper build, instead of having some silent miscompilations), then the user will have to update their libstdc++ anyway, the error does not help them out on its own. So it should be enough to just provide a better error message, without blocking the build.

But I'm not adamant on that, I just want to unblock bootstrap benchmarks until we can find a way to update libstdc++ on the collector machine.

CC `@onur-ozkan`

r? `@Mark-Simulacrum`
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jun 28, 2024
…k-Simulacrum

Remove libstdc++ version check error

This keeps the error message from rust-lang/rust#125411, but removes the `exit(1)` call.

This PR is mostly a hotfix to unblock bootstrap benchmarks in rustc-perf.

However, I think that it might be better to just print a warning, in general. If the ABI version does not match, the build might or might not work locally (as we can see on rustc-perf, where it works even if the reported ABI is 7).

If it does not work (and **if** we can always recognize this during the LLVM wrapper build, instead of having some silent miscompilations), then the user will have to update their libstdc++ anyway, the error does not help them out on its own. So it should be enough to just provide a better error message, without blocking the build.

But I'm not adamant on that, I just want to unblock bootstrap benchmarks until we can find a way to update libstdc++ on the collector machine.

CC `@onur-ozkan`

r? `@Mark-Simulacrum`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-testsuite Area: The testsuite used to check the correctness of rustc 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
Development

Successfully merging this pull request may close these issues.

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