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

jyn514 · 2022-09-15T00:34:44Z

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.

rust-highfive · 2022-09-15T00:34:47Z

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

src/bootstrap/compile.rs

src/bootstrap/config.rs

src/ci/run.sh

rustbot · 2022-09-25T14:46:51Z

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 · 2022-09-25T16:33:03Z

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 :)

Mark-Simulacrum · 2022-09-26T13:04:23Z

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 · 2022-09-26T13:04:25Z

📌 Commit f8527fd has been approved by Mark-Simulacrum

It is now in the queue for this repository.

jyn514 · 2022-09-26T17:26:36Z

/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

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 · 2022-09-28T08:42:01Z

@bors r=Mark-Simulacrum

bors · 2022-09-28T08:42:03Z

📌 Commit 3acb505 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

bors · 2022-09-29T02:28:55Z

⌛ Testing commit 3acb505 with merge bf40408...

bors · 2022-09-29T05:12:24Z

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

rust-timer · 2022-09-29T07:14:05Z

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.

	mean¹	range	count²
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.

	mean¹	range	count²
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.

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

tmandry · 2022-09-30T18:58:35Z

src/bootstrap/dist.rs

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


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".

tmandry · 2022-09-30T19:00:03Z

src/bootstrap/compile.rs

-    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) {


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.

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.

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 · 2022-10-06T14:46:33Z

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)

pietroalbini · 2022-10-06T14:54:00Z

src/bootstrap/compile.rs

+            panic!(
+                "need a managed LLVM submodule for optimized intrinsics support; unset `llvm-config` or `optimized-compiler-builtins`"
+            );


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)

Do you mean

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

?

Yep, sorry!

…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.

…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

rust-highfive assigned Mark-Simulacrum Sep 15, 2022

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 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 15, 2022

jyn514 force-pushed the cross-compile-compiler-builtins branch from 0592216 to 2da1286 Compare September 15, 2022 00:42

Mark-Simulacrum reviewed Sep 17, 2022

View reviewed changes

src/bootstrap/compile.rs Outdated Show resolved Hide resolved

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 force-pushed the cross-compile-compiler-builtins branch from 2da1286 to c3675ed Compare September 19, 2022 01:00

rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Sep 19, 2022

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 force-pushed the cross-compile-compiler-builtins branch from c3675ed to 405f0c2 Compare September 19, 2022 01:03

This comment has been minimized.

Sign in to view

jyn514 force-pushed the cross-compile-compiler-builtins branch from 405f0c2 to 2959eec Compare September 19, 2022 23:14

Mark-Simulacrum reviewed Sep 20, 2022

View reviewed changes

src/bootstrap/config.rs Outdated Show resolved Hide resolved

src/ci/run.sh Show resolved Hide resolved

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

This comment has been minimized.

Sign in to view

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

jyn514 force-pushed the cross-compile-compiler-builtins branch from dd13636 to f8527fd Compare September 25, 2022 16:35

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 added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 26, 2022

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 26, 2022

bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 26, 2022

bjorn3 reviewed Sep 26, 2022

View reviewed changes

src/bootstrap/dist.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

jyn514 force-pushed the cross-compile-compiler-builtins branch from f8527fd to 3acb505 Compare September 28, 2022 08:21

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 added the merged-by-bors This PR was explicitly merged by bors. label Sep 29, 2022

bors merged commit bf40408 into rust-lang:master Sep 29, 2022

rustbot added this to the 1.66.0 milestone Sep 29, 2022

bors mentioned this pull request Sep 29, 2022

build-std compatible profile(code coverage) support #101392

Closed

tmandry reviewed Sep 30, 2022

View reviewed changes

jyn514 mentioned this pull request Oct 2, 2022

Decouple is_rust_llvm from whether LLVM is in-tree or not #102560

Closed

jyn514 deleted the cross-compile-compiler-builtins branch October 2, 2022 03:25

tmandry mentioned this pull request Oct 2, 2022

Mock environment testing for bootstrap #102563

Open

pietroalbini reviewed Oct 6, 2022

View reviewed changes

pietroalbini mentioned this pull request Oct 6, 2022

Reland optimized-compiler-builtins config #102579

Closed

Mark-Simulacrum mentioned this pull request Oct 29, 2022

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

Merged

This was referenced Mar 10, 2023

bootstrap cannot check library/std for windows from macOS (and possibly others) #101172

Closed

+outline-atomics violates "core shall not depend on libc" #109064

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jyn514 commented Sep 15, 2022

rust-highfive commented Sep 15, 2022

This comment has been minimized.

This comment has been minimized.

rustbot commented Sep 25, 2022

jyn514 commented Sep 25, 2022

Mark-Simulacrum commented Sep 26, 2022

bors commented Sep 26, 2022

jyn514 commented Sep 26, 2022

This comment has been minimized.

jyn514 commented Sep 28, 2022

bors commented Sep 28, 2022

bors commented Sep 29, 2022

bors commented Sep 29, 2022

rust-timer commented Sep 29, 2022

tmandry Sep 30, 2022

tmandry Sep 30, 2022

jyn514 Oct 2, 2022

tmandry Oct 2, 2022

pietroalbini commented Oct 6, 2022

pietroalbini Oct 6, 2022

tmandry Oct 18, 2022

pietroalbini Oct 20, 2022

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

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

Conversation

jyn514 commented Sep 15, 2022

rust-highfive commented Sep 15, 2022

This comment has been minimized.

This comment has been minimized.

rustbot commented Sep 25, 2022

jyn514 commented Sep 25, 2022

Mark-Simulacrum commented Sep 26, 2022

bors commented Sep 26, 2022

jyn514 commented Sep 26, 2022

This comment has been minimized.

jyn514 commented Sep 28, 2022

bors commented Sep 28, 2022

bors commented Sep 29, 2022

bors commented Sep 29, 2022

rust-timer commented Sep 29, 2022

Overall result: ✅ improvements - no action needed

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pietroalbini commented Oct 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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