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

Make the c feature for compiler-builtins an explicit opt-in #101833

Merged
merged 1 commit into from
Sep 29, 2022

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Sep 15, 2022

Its build script doesn't support cross-compilation. I tried fixing it, but the cc crate itself doesn't appear to support cross-compiling to windows either unless you use the -gnu toolchain:

  error occurred: Failed to find tool. Is `lib.exe` installed?

Fixes #101172.

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Sep 15, 2022
@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 15, 2022
@jyn514 jyn514 force-pushed the cross-compile-compiler-builtins branch from 0592216 to 2da1286 Compare September 15, 2022 00:42
@Mark-Simulacrum Mark-Simulacrum 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 Sep 17, 2022
@jyn514 jyn514 force-pushed the cross-compile-compiler-builtins branch from 2da1286 to c3675ed Compare September 19, 2022 01:00
@rustbot rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Sep 19, 2022
@jyn514 jyn514 added A-cross Area: Cross compilation S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 19, 2022
@jyn514 jyn514 force-pushed the cross-compile-compiler-builtins branch from c3675ed to 405f0c2 Compare September 19, 2022 01:03
@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 force-pushed the cross-compile-compiler-builtins branch from 405f0c2 to 2959eec Compare September 19, 2022 23:14
src/bootstrap/config.rs Outdated Show resolved Hide resolved
src/ci/run.sh Show resolved Hide resolved
@Mark-Simulacrum Mark-Simulacrum 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 Sep 20, 2022
@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Sep 25, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

Some changes occurred in src/tools/cargo

cc @ehuss

@jyn514 jyn514 force-pushed the cross-compile-compiler-builtins branch from 3328c50 to 4398aff Compare September 25, 2022 14:49
@jyn514
Copy link
Member Author

jyn514 commented Sep 25, 2022

dist-various-2 works except for the very last step, after the build:

Run src/ci/scripts/upload-artifacts.sh
cp: cannot stat 'obj/build/metrics.json': No such file or directory
Error: Process completed with exit code 1.

I think it's just not meant to run in PR CI and it's fine. So this PR should be good to go after I revert the CI changes :)

@jyn514 jyn514 force-pushed the cross-compile-compiler-builtins branch from dd13636 to f8527fd Compare September 25, 2022 16:35
@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 25, 2022
@Mark-Simulacrum Mark-Simulacrum added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 26, 2022
@Mark-Simulacrum
Copy link
Member

I'm a little worried that e.g. distros won't turn this on even if they can and end up with slower builds as a result. Not sure it's worth being too concerned about though. I think we can re-assess if we run into trouble as a result of this; if needed we can flip the default.

I'll flag as relnotes so we can mention it in internal changes/compat notes, I think.

@bors r+

@bors
Copy link
Contributor

bors commented Sep 26, 2022

📌 Commit f8527fd has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 26, 2022
@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 26, 2022
@jyn514
Copy link
Member Author

jyn514 commented Sep 26, 2022

/checkout/obj/build/x86_64-unknown-linux-gnu/stage1/bin/rustc: error while loading shared libraries: libLLVM-15-rust-1.66.0-nightly.so: cannot open shared object file: No such file or directory

???

will dig into this later

src/bootstrap/dist.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

The build script for `compiler_builtins` doesn't support cross-compilation. I tried fixing it, but the cc crate itself
doesn't appear to support cross-compiling to windows either unless you use the -gnu toolchain:
```
  error occurred: Failed to find tool. Is `lib.exe` installed?
```

Rather than trying to fix it or special-case the platforms without bugs,
make it opt-in instead of automatic.
@jyn514 jyn514 force-pushed the cross-compile-compiler-builtins branch from f8527fd to 3acb505 Compare September 28, 2022 08:21
@jyn514
Copy link
Member Author

jyn514 commented Sep 28, 2022

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Sep 28, 2022

📌 Commit 3acb505 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 28, 2022
@bors
Copy link
Contributor

bors commented Sep 29, 2022

⌛ Testing commit 3acb505 with merge bf40408...

@bors
Copy link
Contributor

bors commented Sep 29, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 29, 2022
@bors bors merged commit bf40408 into rust-lang:master Sep 29, 2022
@rustbot rustbot added this to the 1.66.0 milestone Sep 29, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (bf40408): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

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

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.6% [-2.6%, -2.6%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

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.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.4% [3.4%, 3.4%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.9% [-3.5%, -2.2%] 3
All ❌✅ (primary) - - 0

Cycles

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

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

// still want to install it, as it otherwise won't be available.
return false;
}
if !builder.is_rust_llvm(target) {
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this. The point of is_rust_llvm is to know whether it contains Rust patches. In #101072 I added a config option so you can use a vanilla upstream LLVM without affecting other parts of how bootstrap works, this breaks that by re-coupling the idea of "is patched" to "is in tree".

let compiler_builtins_root = builder.src.join("src/llvm-project/compiler-rt");
let compiler_builtins_c_feature = if compiler_builtins_root.exists() {
let compiler_builtins_c_feature = if builder.config.optimized_compiler_builtins {
if !builder.is_rust_llvm(target) {
Copy link
Member

Choose a reason for hiding this comment

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

Likewise, this should be checking if a source checkout of LLVM is being used. It shouldn't be affected by the llvm-has-rust-patches target option.

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 think is_rust_llvm is exactly what we want here, actually. We want to know that if we use the sources in src/llvm-project, the instrumentation there will match the instrumentation in libLLVM.so, which is true exactly when that has the same patches as the submodule. Whether it was originally built from source or not shouldn't matter.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah you want to know if the versions match, which isn't the same as whether you're building from source.. it also isn't the same as whether it includes Rust patches, though. I commented on the other PR

@pietroalbini
Copy link
Member

I want to note that disabling the c feature appears to have more side effects than just less optimizations.

At least on the aarch64-unknown-linux-gnu target, disabling that flag results in rustc being unable to compile anything when using GCC < 9.4 as the linker, due to its lack of support for -m outline-atomics (introduced in GCC 9.4). Linking anything results in a bunch of linking errors, that are suppressed by either using a newer GCC as the linker or enabling the new config flag.

(I discovered this when pulling the latest changes into Ferrocene)

Comment on lines +315 to +317
panic!(
"need a managed LLVM submodule for optimized intrinsics support; unset `llvm-config` or `optimized-compiler-builtins`"
);
Copy link
Member

Choose a reason for hiding this comment

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

If this check is present, there has to be a way to provide the path to the compiler-rt source code of the custom LLVM, otherwise it will be impossible to use custom LLVMs on some platforms (see #101833 (comment)).

What I would do is have a check like:

if optimized_compiler_builtins && (!is_rust_llvm() || compiler_rt_src_path)

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean

if optimized_compiler_builtins && !(is_rust_llvm() || compiler_rt_src_path)

?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, sorry!

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Oct 29, 2022
…f inferred"

This reverts commit 3acb505
(PR rust-lang#101833).

The changes in this commit caused several bugs or at least
incompatibilies. For now we're reverting this commit and will re-land it
alongside fixes for those bugs.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 31, 2022
…iltins, r=jyn514

Revert "Make the `c` feature for `compiler-builtins` opt-in instead of inferred"

This reverts commit 3acb505 (PR rust-lang#101833).

The changes in this commit caused several bugs/incompatibilities (rust-lang#101833 (comment), rust-lang#102560). For now we're reverting this commit and will re-land it alongside fixes for those bugs.

Re-opens rust-lang#101172
cc rust-lang#102560
cc rust-lang#102579
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cross Area: Cross compilation merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. 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) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bootstrap cannot check library/std for windows from macOS (and possibly others)
10 participants