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

improve compiler&tool documenting and re-enable cranelift on CI #117574

Merged
merged 8 commits into from
Nov 5, 2023

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Nov 4, 2023

First commit addresses the linking issue with compiler crates. Second one ensures that compiler crates are linked correctly (with later commits we added this check for tools as well), allowing us to detect these hard-to-catch bugs on CI. Following three commits cherry-picked from #117328 to re-enable the Cranelift backend on CI.

More info: #117430

cc @bjorn3 @RalfJung

@rustbot
Copy link
Collaborator

rustbot commented Nov 4, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc 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) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Nov 4, 2023
onur-ozkan and others added 5 commits November 4, 2023 15:11
Either generate it with the actual codegen backend dylib or omit it
entirely when the cranelift backend is disabled for this build.
self.number_of_times_dry_runs_have_caused_issues += 1;
@onur-ozkan onur-ozkan force-pushed the fix-compiler-crate-linking branch from 91d297f to 451778b Compare November 4, 2023 12:12
@onur-ozkan onur-ozkan force-pushed the fix-compiler-crate-linking branch from 49e5ef5 to 55b4945 Compare November 4, 2023 12:46
@bjorn3
Copy link
Member

bjorn3 commented Nov 4, 2023

Thanks a lot for working on this!

Could you add the following patch? I found this unintended change while I was (unsuccessfully) trying to figure out the doc issue too.

Author: bjorn3 <[email protected]>
Date:   Sat Nov 4 12:47:05 2023 +0000

    Don't disable inline asm usage in compiler-builtins when the cranelift backend is enabled
    
    This was a leftover from when inline asm wasn't universally supported by
    the cranelift backend yet. It would cause the optimized inline asm
    intrinsics to be avoided for the standard library distributed with
    rustup now that we build the cranelift backend by default on nightly for
    several tier 1 targets.

diff --git a/src/bootstrap/src/core/build_steps/compile.rs b/src/bootstrap/src/core/build_steps/compile.rs
index 1eed534150b..8ab0ef61c50 100644
--- a/src/bootstrap/src/core/build_steps/compile.rs
+++ b/src/bootstrap/src/core/build_steps/compile.rs
@@ -413,11 +413,6 @@ pub fn std_cargo(builder: &Builder<'_>, target: TargetSelection, stage: u32, car
 
     let mut features = String::new();
 
-    // Cranelift doesn't support `asm`.
-    if stage != 0 && builder.config.default_codegen_backend().unwrap_or_default() == "cranelift" {
-        features += " compiler-builtins-no-asm";
-    }
-
     if builder.no_std(target) == Some(true) {
         features += " compiler-builtins-mem";
         if !target.starts_with("bpf") {

@onur-ozkan onur-ozkan force-pushed the fix-compiler-crate-linking branch from 2501bb6 to 55b4945 Compare November 4, 2023 12:56
@onur-ozkan
Copy link
Member Author

onur-ozkan commented Nov 4, 2023

Thanks a lot for working on this!

Could you add the following patch? I found this unintended change while I was (unsuccessfully) trying to figure out the doc issue too.

Author: bjorn3 <[email protected]>
Date:   Sat Nov 4 12:47:05 2023 +0000

    Don't disable inline asm usage in compiler-builtins when the cranelift backend is enabled
    
    This was a leftover from when inline asm wasn't universally supported by
    the cranelift backend yet. It would cause the optimized inline asm
    intrinsics to be avoided for the standard library distributed with
    rustup now that we build the cranelift backend by default on nightly for
    several tier 1 targets.

diff --git a/src/bootstrap/src/core/build_steps/compile.rs b/src/bootstrap/src/core/build_steps/compile.rs
index 1eed534150b..8ab0ef61c50 100644
--- a/src/bootstrap/src/core/build_steps/compile.rs
+++ b/src/bootstrap/src/core/build_steps/compile.rs
@@ -413,11 +413,6 @@ pub fn std_cargo(builder: &Builder<'_>, target: TargetSelection, stage: u32, car
 
     let mut features = String::new();
 
-    // Cranelift doesn't support `asm`.
-    if stage != 0 && builder.config.default_codegen_backend().unwrap_or_default() == "cranelift" {
-        features += " compiler-builtins-no-asm";
-    }
-
     if builder.no_std(target) == Some(true) {
         features += " compiler-builtins-mem";
         if !target.starts_with("bpf") {

It gives "fatal: empty ident name (for <>) not allowed" when I try to git am on the patch. Is it ok for you if I do git apply on it (it will show my name instead of yours on the commit)?

@bjorn3
Copy link
Member

bjorn3 commented Nov 4, 2023

You could try git commit --amend --author "bjorn3 <[email protected]>" to change the author of the commit. If that doesn't work, plain git apply is fine with me.

…t backend is enabled

This was a leftover from when inline asm wasn't universally supported by
the cranelift backend yet. It would cause the optimized inline asm
intrinsics to be avoided for the standard library distributed with
rustup now that we build the cranelift backend by default on nightly for
several tier 1 targets.
@onur-ozkan
Copy link
Member Author

Seems like rustdoc and rustdoc_json_types docs are also missing. Let's block the PR until I fix them.

@rustbot blocked

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2023
@Mark-Simulacrum
Copy link
Member

In general r=me, but happy to see this build docs in CI (via a try build, likely with a change like this to re-enable docs: 43f592f) before we land it. Happy to re-review with the extra fix you mention (but not strictly needed if similar changes).

@onur-ozkan
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Nov 4, 2023

⌛ Trying commit a348b00 with merge 4d1b721...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 4, 2023
…g, r=<try>

re-enable cranelift on CI

First commit addresses the linking issue with compiler crates. Second one ensures that compiler crates are linked correctly, allowing us to detect these hard-to-catch bugs on CI. The remaining three commits cherry-picked from rust-lang#117328 to re-enable the Cranelift backend on CI.

More info: rust-lang#117430

cc `@bjorn3` `@RalfJung`
@onur-ozkan onur-ozkan removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Nov 4, 2023
@bors
Copy link
Contributor

bors commented Nov 4, 2023

💔 Test failed - checks-actions

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 4, 2023
@onur-ozkan
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Nov 5, 2023

⌛ Trying commit a3fe394 with merge 83e9fc5...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 5, 2023
…g, r=<try>

re-enable cranelift on CI

First commit addresses the linking issue with compiler crates. Second one ensures that compiler crates are linked correctly, allowing us to detect these hard-to-catch bugs on CI. Following three commits cherry-picked from rust-lang#117328 to re-enable the Cranelift backend on CI.

More info: rust-lang#117430

cc `@bjorn3` `@RalfJung`
@bors
Copy link
Contributor

bors commented Nov 5, 2023

💔 Test failed - checks-actions

@onur-ozkan onur-ozkan force-pushed the fix-compiler-crate-linking branch from a3fe394 to 32946d2 Compare November 5, 2023 09:56
@onur-ozkan
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Nov 5, 2023

⌛ Trying commit 32946d2 with merge dc8c831...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 5, 2023
…g, r=<try>

re-enable cranelift on CI

First commit addresses the linking issue with compiler crates. Second one ensures that compiler crates are linked correctly, allowing us to detect these hard-to-catch bugs on CI. Following three commits cherry-picked from rust-lang#117328 to re-enable the Cranelift backend on CI.

More info: rust-lang#117430

cc `@bjorn3` `@RalfJung`
@bors
Copy link
Contributor

bors commented Nov 5, 2023

☀️ Try build successful - checks-actions
Build commit: dc8c831 (dc8c8311ce31a9e8e92f63c098be23b53f2bfc21)

@onur-ozkan
Copy link
Member Author

onur-ozkan commented Nov 5, 2023

Here the try build that includes compiler docs: https://ci-artifacts.rust-lang.org/rustc-builds/dc8c8311ce31a9e8e92f63c098be23b53f2bfc21/rustc-docs-nightly-x86_64-unknown-linux-gnu.tar.xz. However, it's missing rustdoc pages. I will trigger bors for another try build (I can't debug this build locally, working with a laptop atm) to see if this issue gets resolved. If not, I will continue working on it tonight or early tomorrow on my desktop (so I can debug things faster).

EDIT: It seems the last attempt fixed rustdoc problem!

@onur-ozkan onur-ozkan force-pushed the fix-compiler-crate-linking branch from 32946d2 to 43bc2dd Compare November 5, 2023 11:51
@onur-ozkan
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Nov 5, 2023

⌛ Trying commit 43bc2dd with merge d4862cd...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 5, 2023
…g, r=<try>

re-enable cranelift on CI

First commit addresses the linking issue with compiler crates. Second one ensures that compiler crates are linked correctly, allowing us to detect these hard-to-catch bugs on CI. Following three commits cherry-picked from rust-lang#117328 to re-enable the Cranelift backend on CI.

More info: rust-lang#117430

cc `@bjorn3` `@RalfJung`
@bors
Copy link
Contributor

bors commented Nov 5, 2023

☀️ Try build successful - checks-actions
Build commit: d4862cd (d4862cdec9eae562e5318823f9a7374b10d6acb6)

@onur-ozkan
Copy link
Member Author

@onur-ozkan onur-ozkan changed the title re-enable cranelift on CI improve compiler&tool documenting and re-enable cranelift on CI Nov 5, 2023
@onur-ozkan onur-ozkan force-pushed the fix-compiler-crate-linking branch from 43bc2dd to b0df821 Compare November 5, 2023 14:02
@onur-ozkan
Copy link
Member Author

Happy to re-review with the extra fix you mention (but not strictly needed if similar changes).

No strict changes have been applied. Also, we are able to catch missing-doc problem for compiler and tool crates (like here https://github.com/rust-lang-ci/rust/actions/runs/6760207883/job/18373700086#step:25:28933), so it should be fine to r.

@bors r=Mark-Simulacrum rollup=never

@bors
Copy link
Contributor

bors commented Nov 5, 2023

📌 Commit b0df821 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 Nov 5, 2023
@bors
Copy link
Contributor

bors commented Nov 5, 2023

⌛ Testing commit b0df821 with merge 6c7de31...

@bors
Copy link
Contributor

bors commented Nov 5, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 5, 2023
@bors bors merged commit 6c7de31 into rust-lang:master Nov 5, 2023
12 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Nov 5, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6c7de31): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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)
2.7% [1.6%, 3.7%] 5
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.1% [-1.1%, -1.0%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.6% [-1.1%, 3.7%] 7

Cycles

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.6% [0.5%, 0.6%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.5% [-0.6%, -0.5%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.0% [-0.6%, 0.6%] 4

Binary size

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

Bootstrap: 635.747s -> 638.861s (0.49%)
Artifact size: 304.50 MiB -> 304.51 MiB (0.00%)

bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Nov 6, 2023
84: Automated pull from upstream `master` r=Dajamante a=github-actions[bot]


This PR pulls the following changes from the upstream repository:

* rust-lang/rust#117585
* rust-lang/rust#117576
* rust-lang/rust#96979
* rust-lang/rust#117191
* rust-lang/rust#117179
* rust-lang/rust#117574
* rust-lang/rust#117537
* rust-lang/rust#117608
  * rust-lang/rust#117596
  * rust-lang/rust#117588
  * rust-lang/rust#117524
  * rust-lang/rust#116017
* rust-lang/rust#117504
* rust-lang/rust#117469
* rust-lang/rust#116218
* rust-lang/rust#117589
* rust-lang/rust#117581
* rust-lang/rust#117503
* rust-lang/rust#117590
  * rust-lang/rust#117583
  * rust-lang/rust#117570
  * rust-lang/rust#117562
  * rust-lang/rust#117534
  * rust-lang/rust#116894
  * rust-lang/rust#110340
* rust-lang/rust#113343
* rust-lang/rust#117579
* rust-lang/rust#117094
* rust-lang/rust#117566
* rust-lang/rust#117564
  * rust-lang/rust#117554
  * rust-lang/rust#117550
  * rust-lang/rust#117343
* rust-lang/rust#115274
* rust-lang/rust#117540
* rust-lang/rust#116412
* rust-lang/rust#115333
* rust-lang/rust#117507
* rust-lang/rust#117538
  * rust-lang/rust#117533
  * rust-lang/rust#117523
  * rust-lang/rust#117520
  * rust-lang/rust#117505
  * rust-lang/rust#117434
* rust-lang/rust#117535
* rust-lang/rust#117510
* rust-lang/rust#116439
* rust-lang/rust#117508



Co-authored-by: Ben Wiederhake <[email protected]>
Co-authored-by: SabrinaJewson <[email protected]>
Co-authored-by: J-ZhengLi <[email protected]>
Co-authored-by: koka <[email protected]>
Co-authored-by: bjorn3 <[email protected]>
Co-authored-by: Joshua Liebow-Feeser <[email protected]>
Co-authored-by: lengyijun <[email protected]>
Co-authored-by: Zalathar <[email protected]>
Co-authored-by: Oli Scherer <[email protected]>
Co-authored-by: Philipp Krones <[email protected]>
Co-authored-by: y21 <[email protected]>
Co-authored-by: bors <[email protected]>
Co-authored-by: bohan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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) 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.

7 participants