Build `library/profiler_builtins` from `ci-llvm` if appropriate #129295

Zalathar · 2024-08-20T06:43:36Z

Running all of tests/coverage requires the LLVM profiler runtime, which requires setting build.profiler = true.

Historically, doing that has required checking out the entire src/llvm-project submodule. For compiler contributors who otherwise don't need that submodule (thanks to download-ci-vm), that's quite inconvenient.

However, thanks to #129116, the downloaded CI LLVM tarball now contains a copy of LLVM's compiler-rt directory, which includes all the files needed to build the profiler runtime. So with a little bit of extra logic in bootstrap, we can have library/profiler_builtins look for the compiler-rt files in ci-llvm instead of the src/llvm-project submodule.

rustbot · 2024-08-20T06:43:43Z

r? @Kobzol

rustbot has assigned @Kobzol.
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

Zalathar · 2024-08-21T00:58:35Z

As mentioned in the comments, I've added a new environment variable so that there's no risk of breaking whatever it is that compiler_builtins is currently doing.

Kobzol

Thanks! Left a few comments.

library/profiler_builtins/build.rs

src/bootstrap/src/core/build_steps/compile.rs

The current `build.rs` will automatically skip source files that don't exist. An unfortunate side-effect is that if _no_ files could be found (e.g. because the directory was wrong), the build fails with a mysterious linker error. We can reduce the awkwardness of this by explicitly checking that at least one source file was found.

Zalathar · 2024-08-23T00:27:39Z

Added fallback if RUST_COMPILER_RT_FOR_PROFILER was not set (diff).

Kobzol · 2024-08-23T06:49:30Z

After this PR, if people use download-ci-llvm = true, compiler-rt sources needed to build library/profiler_builtins (enabled with build.profiler = true in config.toml) will not be used from the local LLVM checkout, but from the downloaded CI LLVM archive. If download-ci-llvm = "if-unchanged" is used, local sources will be correctly used if someone modifies the compiler-rt sources locally.

@onur-ozkan Do you think that this warrants an Info record in the change tracker, to let people know that if they download LLVM from CI unconditionally, the local compiler-rt sources will not be used anymore?

onur-ozkan · 2024-08-23T07:45:10Z

After this PR, if people use download-ci-llvm = true, compiler-rt sources needed to build library/profiler_builtins (enabled with build.profiler = true in config.toml) will not be used from the local LLVM checkout, but from the downloaded CI LLVM archive. If download-ci-llvm = "if-unchanged" is used, local sources will be correctly used if someone modifies the compiler-rt sources locally.

@onur-ozkan Do you think that this warrants an Info record in the change tracker, to let people know that if they download LLVM from CI unconditionally, the local compiler-rt sources will not be used anymore?

IMO it's worth to have this information in the change history (so devs can open this PR and see the details behind this work).

Kobzol · 2024-08-23T07:49:20Z

Ok, agreed. @Zalathar could you please mention it there, with the INFO level? Thank you!

Kobzol · 2024-08-23T09:41:23Z

Thank you! :)

@bors r+

bors · 2024-08-23T09:41:26Z

📌 Commit 4acc531 has been approved by Kobzol

It is now in the queue for this repository.

Build `library/profiler_builtins` from `ci-llvm` if appropriate Running all of `tests/coverage` requires the LLVM profiler runtime, which requires setting `build.profiler = true`. Historically, doing that has required checking out the entire `src/llvm-project` submodule. For compiler contributors who otherwise don't need that submodule (thanks to `download-ci-vm`), that's quite inconvenient. However, thanks to rust-lang#129116, the downloaded CI LLVM tarball now contains a copy of LLVM's `compiler-rt` directory, which includes all the files needed to build the profiler runtime. So with a little bit of extra logic in bootstrap, we can have `library/profiler_builtins` look for the `compiler-rt` files in `ci-llvm` instead of the `src/llvm-project` submodule.

Rollup of 7 pull requests Successful merges: - rust-lang#126985 (Implement `-Z embed-source` (DWARFv5 source code embedding extension)) - rust-lang#128507 (Migrate `libtest-thread-limit` `run-make` test to rmake) - rust-lang#128935 (More work on `zstd` compression) - rust-lang#129190 (Add f16 and f128 to tests/ui/consts/const-float-bits-conv.rs) - rust-lang#129295 (Build `library/profiler_builtins` from `ci-llvm` if appropriate) - rust-lang#129416 (library: Move unstable API of new_uninit to new features) - rust-lang#129418 (rustc: Simplify getting sysroot library directory) r? `@ghost` `@rustbot` modify labels: rollup

jieyouxu · 2024-08-23T18:39:24Z

Failed in 🧻 #129474 (comment):

2024-08-23T18:28:32.1585250Z thread 'main' panicked at src/core/build_steps/compile.rs:480:9:
2024-08-23T18:28:32.1586270Z compiler-rt directory not found: "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/ci-llvm/compiler-rt"
2024-08-23T18:28:32.1587060Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

jieyouxu · 2024-08-23T18:39:32Z

@bors r-

Kobzol · 2024-08-23T18:52:22Z

We either need to wait until the ci-llvm archives are rebuilt on CI (after the next LLVM change), or implement the suggestion I had in a comment, to fallback to the checked out path.

Zalathar · 2024-08-24T05:44:14Z

The actual problem seems to be that llvm_from_ci is unexpectedly true on CI builds, so we can't rely on it when deciding whether using downloaded compiler-rt should work or not.

So I think my overall strategy is going to be:

Sync src/llvm-project if it's already active, and use compiler-rt from there if possible.
Otherwise, if llvm_from_ci is true and we can find its copy of compiler-rt, use that.
Otherwise, fall back to requiring the src/llvm-project submodule as before, and use compiler-rt from there.

Zalathar · 2024-08-24T06:02:20Z

I'm sad that I couldn't get this to be fully robust, but I don't think that's possible without sweeping changes to how bootstrap keeps track of which LLVM it's using.

So hopefully this should be good enough for realistic use cases.

Kobzol · 2024-08-24T07:25:58Z

The actual problem seems to be that llvm_from_ci is unexpectedly true on CI builds, so we can't rely on it when deciding whether using downloaded compiler-rt should work or not.

Yes, test builders use download-ci-llvm, but why is that a problem? On CI, it shouldn't matter if we use the sources from a previous CI archive, as long as the git hash of the LLVM subdirectory hasn't changed (in which case download-ci-llvm will not be applied).

bors · 2024-08-24T21:28:27Z

💔 Test failed - checks-actions

Kobzol · 2024-08-24T21:28:53Z

@bors retry (spurious Windows error)

Build `library/profiler_builtins` from `ci-llvm` if appropriate Running all of `tests/coverage` requires the LLVM profiler runtime, which requires setting `build.profiler = true`. Historically, doing that has required checking out the entire `src/llvm-project` submodule. For compiler contributors who otherwise don't need that submodule (thanks to `download-ci-vm`), that's quite inconvenient. However, thanks to rust-lang#129116, the downloaded CI LLVM tarball now contains a copy of LLVM's `compiler-rt` directory, which includes all the files needed to build the profiler runtime. So with a little bit of extra logic in bootstrap, we can have `library/profiler_builtins` look for the `compiler-rt` files in `ci-llvm` instead of the `src/llvm-project` submodule.

bors · 2024-08-25T01:06:19Z

⌛ Testing commit 94aadf0 with merge a9bc1a9...

rust-log-analyzer · 2024-08-25T02:49:09Z

The job x86_64-msvc-ext failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

test result: ok. 25 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.02s

     Running tests\ui.rs (build\x86_64-pc-windows-msvc\stage1-tools\x86_64-pc-windows-msvc\release\deps\ui-9700297a935a1cdc.exe)
## Running ui tests in tests/pass for x86_64-pc-windows-msvc
   Compiler: "MIRI_ENV_VAR_TEST"="0" "MIRI_TEMP"="C:\\a\\_temp\\msys64\\tmp\\miri-uitest-gkhHhS" "RUST_BACKTRACE"="1" C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1\bin\miri.exe "--error-format=json" "--sysroot=C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\miri-sysroot" "-Dwarnings" "-Dunused" "-Ainternal_features" "-Zmiri-provenance-gc=1" "-Zui-testing" "--target" "x86_64-pc-windows-msvc" "--out-dir" OUT_DIR
tests/pass\align_repeat_into_well_aligned_array.rs ... ok
tests/pass\alloc-access-tracking.rs ... ignored (in-test comment)
tests/pass\align_repeat_into_packed_field.rs ... ok
tests/pass\align.rs ... ok
---

test result: ok. 350 passed; 9 ignored;

## Running ui tests in tests/pass-dep for x86_64-pc-windows-msvc
   Compiler: "MIRI_ENV_VAR_TEST"="0" "MIRI_TEMP"="C:\\a\\_temp\\msys64\\tmp\\miri-uitest-gkhHhS" "RUST_BACKTRACE"="1" C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1\bin\miri.exe "--error-format=json" "--sysroot=C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\miri-sysroot" "-Dwarnings" "-Dunused" "-Ainternal_features" "-Zmiri-provenance-gc=1" "-Zui-testing" "--target" "x86_64-pc-windows-msvc" "--out-dir" OUT_DIR
tests/pass-dep\tempfile.rs ... ignored (in-test comment)
tests/pass-dep\concurrency\apple-os-unfair-lock.rs ... ignored (in-test comment)
tests/pass-dep\concurrency\env-cleanup-data-race.rs ... ignored (in-test comment)
tests/pass-dep\concurrency\libc_pthread_cond_timedwait.rs ... ignored (in-test comment)
---

test result: ok. 12 passed; 28 ignored;

## Running ui tests in tests/panic for x86_64-pc-windows-msvc
   Compiler: "MIRI_ENV_VAR_TEST"="0" "MIRI_TEMP"="C:\\a\\_temp\\msys64\\tmp\\miri-uitest-gkhHhS" "RUST_BACKTRACE"="1" C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1\bin\miri.exe "--error-format=json" "--sysroot=C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\miri-sysroot" "-Dwarnings" "-Dunused" "-Ainternal_features" "-Zmiri-provenance-gc=1" "-Zui-testing" "--target" "x86_64-pc-windows-msvc" "--out-dir" OUT_DIR
Building dependencies ... ok
tests/panic\overflowing-rsh-1.rs ... ok
tests/panic\overflowing-lsh-neg.rs ... ok
tests/panic\overflowing-rsh-2.rs ... ok
---

test result: ok. 14 passed; 2 ignored;

## Running ui tests in tests/fail for x86_64-pc-windows-msvc
   Compiler: "MIRI_ENV_VAR_TEST"="0" "MIRI_TEMP"="C:\\a\\_temp\\msys64\\tmp\\miri-uitest-gkhHhS" "RUST_BACKTRACE"="1" C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1\bin\miri.exe "--error-format=json" "--sysroot=C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\miri-sysroot" "-Dwarnings" "-Dunused" "-Ainternal_features" "-Zmiri-provenance-gc=1" "-Zui-testing" "--target" "x86_64-pc-windows-msvc" "--out-dir" OUT_DIR
tests/fail\deny_lint.rs ... ok
tests/fail\const-ub-checks.rs ... ok
tests/fail\dyn-call-trait-mismatch.rs ... ok
tests/fail\environ-gets-deallocated.rs ... ignored (in-test comment)
---

test result: ok. 451 passed; 4 ignored;

## Running ui tests in tests/fail-dep for x86_64-pc-windows-msvc
   Compiler: "MIRI_ENV_VAR_TEST"="0" "MIRI_TEMP"="C:\\a\\_temp\\msys64\\tmp\\miri-uitest-gkhHhS" "RUST_BACKTRACE"="1" C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1\bin\miri.exe "--error-format=json" "--sysroot=C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\miri-sysroot" "-Dwarnings" "-Dunused" "-Ainternal_features" "-Zmiri-provenance-gc=1" "-Zui-testing" "--target" "x86_64-pc-windows-msvc" "--out-dir" OUT_DIR
tests/fail-dep\concurrency\apple_os_unfair_lock_assert_owner.rs ... ignored (in-test comment)
tests/fail-dep\concurrency\apple_os_unfair_lock_reentrant.rs ... ignored (in-test comment)
tests/fail-dep\concurrency\apple_os_unfair_lock_unowned.rs ... ignored (in-test comment)
tests/fail-dep\concurrency\libc_pthread_cond_double_destroy.rs ... ignored (in-test comment)
---
[RUSTC-TIMING] miri test:false 4.385
error: failed to remove file `C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1-tools\x86_64-pc-windows-msvc\release\miri.exe`

Caused by:
  Access is denied. (os error 5)
Command has failed. Rerun with -v to see more details.
  local time: Sun, Aug 25, 2024  2:48:55 AM
  network time: Sun, 25 Aug 2024 02:48:55 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

bors · 2024-08-25T02:49:49Z

💔 Test failed - checks-actions

tgross35 · 2024-08-25T02:51:24Z

@bors retry #127883

bors · 2024-08-25T14:44:58Z

⌛ Testing commit 94aadf0 with merge 1a94d83...

bors · 2024-08-25T17:09:05Z

☀️ Test successful - checks-actions
Approved by: Kobzol
Pushing 1a94d83 to master...

rust-timer · 2024-08-25T18:24:00Z

Finished benchmarking commit (1a94d83): 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 (secondary 0.1%)

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)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary 2.6%)

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)	2.6%	[2.2%, 3.0%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 751.721s -> 752.849s (0.15%)
Artifact size: 338.75 MiB -> 338.80 MiB (0.02%)

Here are all the changes. I went through them one-by-one and confirmed that they should not be affecting us. In paritcular, we explicitly set rust.lld = false (because we want to use the lld that ships with clang), so the change in default does not affect us. There have been changes to x.py since you last updated: [INFO] New option `build.lldb` that will override the default lldb binary path used in debuginfo tests - PR Link rust-lang/rust#124501 [INFO] The compiler profile now defaults to rust.debuginfo-level = "line-tables-only" - PR Link rust-lang/rust#123337 [WARNING] `rust.lld` has a new default value of `true` on `x86_64-unknown-linux-gnu`. Starting at stage1, `rust-lld` will thus be this target's default linker. No config changes should be necessary. - PR Link rust-lang/rust#124129 [WARNING] Removed `dist.missing-tools` configuration as it was deprecated long time ago. - PR Link rust-lang/rust#125535 [WARNING] `llvm.lld` is enabled by default for the dist profile. If set to false, `lld` will not be included in the dist build. - PR Link rust-lang/rust#126701 [WARNING] `debug-logging` option has been removed from the default `tools` profile. - PR Link rust-lang/rust#127913 [INFO] the `wasm-component-ld` tool is now built as part of `build.extended` and can be a member of `build.tools` - PR Link rust-lang/rust#127866 [INFO] Removed android-ndk r25b support in favor of android-ndk r26d. - PR Link rust-lang/rust#120593 [WARNING] For tarball sources, default value for `rust.channel` will be taken from `src/ci/channel` file. - PR Link rust-lang/rust#125181 [INFO] New option `llvm.libzstd` to control whether llvm is built with zstd support. - PR Link rust-lang/rust#125642 [WARNING] ./x test --rustc-args was renamed to --compiletest-rustc-args as it only applies there. ./x miri --rustc-args was also removed. - PR Link rust-lang/rust#128841 [INFO] The `build.profiler` option now tries to use source code from `download-ci-llvm` if possible, instead of checking out the `src/llvm-project` submodule. - PR Link rust-lang/rust#129295 Original-Reviewed-on: https://fuchsia-review.googlesource.com/c/infra/recipes/+/1120078 Original-Revision: 27df37a30e50b14b9ffefc872b6997790f03d4ea GitOrigin-RevId: 341e222f002e36886b9960645b21faeaed633f90 Change-Id: Id1eb54a677a6f538bf7666d65b85d5fdba17ea42

rustbot assigned Kobzol Aug 20, 2024

rustbot added S-waiting-on-review T-bootstrap labels Aug 20, 2024

Kobzol reviewed Aug 22, 2024

View reviewed changes

library/profiler_builtins/build.rs Outdated Show resolved Hide resolved

src/bootstrap/src/core/build_steps/compile.rs Show resolved Hide resolved

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

Zalathar force-pushed the profiler-builtins branch 2 times, most recently from 880707b to a1d2971 Compare August 23, 2024 00:26

Zalathar force-pushed the profiler-builtins branch from a1d2971 to 4acc531 Compare August 23, 2024 09:32

bors added S-waiting-on-bors and removed S-waiting-on-review labels Aug 23, 2024

jieyouxu mentioned this pull request Aug 23, 2024

Rollup of 7 pull requests #129474

Closed

bors added S-waiting-on-author and removed S-waiting-on-bors labels Aug 23, 2024

Zalathar force-pushed the profiler-builtins branch from 4acc531 to e04e3fb Compare August 24, 2024 05:49

bors added S-waiting-on-review and removed S-waiting-on-bors labels Aug 24, 2024

bors added S-waiting-on-bors and removed S-waiting-on-review labels Aug 24, 2024

bors added S-waiting-on-review and removed S-waiting-on-bors labels Aug 25, 2024

bors added S-waiting-on-bors and removed S-waiting-on-review labels Aug 25, 2024

tgross35 mentioned this pull request Aug 25, 2024

Tracking Issue for high failure rates on Windows MSVC CI with filesystem errors #127883

Open

bors added the merged-by-bors label Aug 25, 2024

bors merged commit 1a94d83 into rust-lang:master Aug 25, 2024
7 checks passed

rustbot added this to the 1.82.0 milestone Aug 25, 2024

This was referenced Aug 25, 2024

Dummy PR to test coverage test changes in CI #114917

Draft

custom/external clippy support for bootstrapping #129152

Merged

Autodiff Upstreaming - enzyme backend #129176

Merged

Zalathar deleted the profiler-builtins branch August 25, 2024 22:22

jieyouxu added the CI-spurious-fail-msvc label Oct 15, 2024

Zalathar mentioned this pull request Oct 17, 2024

Changing build.profiler forces an unnecessary rebuild of std #131812

Closed

heitbaum mentioned this pull request Dec 16, 2024

rust: update to 1.83.0 LibreELEC/LibreELEC.tv#9582

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build `library/profiler_builtins` from `ci-llvm` if appropriate #129295

Build `library/profiler_builtins` from `ci-llvm` if appropriate #129295

Zalathar commented Aug 20, 2024 •

edited

Loading

rustbot commented Aug 20, 2024

Zalathar commented Aug 21, 2024

Kobzol left a comment

Zalathar commented Aug 23, 2024

Kobzol commented Aug 23, 2024

onur-ozkan commented Aug 23, 2024 •

edited

Loading

Kobzol commented Aug 23, 2024

Kobzol commented Aug 23, 2024

bors commented Aug 23, 2024

jieyouxu commented Aug 23, 2024

jieyouxu commented Aug 23, 2024

Kobzol commented Aug 23, 2024

Zalathar commented Aug 24, 2024

Zalathar commented Aug 24, 2024

Kobzol commented Aug 24, 2024

bors commented Aug 24, 2024

Kobzol commented Aug 24, 2024

bors commented Aug 25, 2024

rust-log-analyzer commented Aug 25, 2024

bors commented Aug 25, 2024

tgross35 commented Aug 25, 2024

bors commented Aug 25, 2024

bors commented Aug 25, 2024

rust-timer commented Aug 25, 2024

Build library/profiler_builtins from ci-llvm if appropriate #129295

Build library/profiler_builtins from ci-llvm if appropriate #129295

Conversation

Zalathar commented Aug 20, 2024 • edited Loading

rustbot commented Aug 20, 2024

Zalathar commented Aug 21, 2024

Kobzol left a comment

Choose a reason for hiding this comment

Zalathar commented Aug 23, 2024

Kobzol commented Aug 23, 2024

onur-ozkan commented Aug 23, 2024 • edited Loading

Kobzol commented Aug 23, 2024

Kobzol commented Aug 23, 2024

bors commented Aug 23, 2024

jieyouxu commented Aug 23, 2024

jieyouxu commented Aug 23, 2024

Kobzol commented Aug 23, 2024

Zalathar commented Aug 24, 2024

Zalathar commented Aug 24, 2024

Kobzol commented Aug 24, 2024

bors commented Aug 24, 2024

Kobzol commented Aug 24, 2024

bors commented Aug 25, 2024

rust-log-analyzer commented Aug 25, 2024

bors commented Aug 25, 2024

tgross35 commented Aug 25, 2024

bors commented Aug 25, 2024

bors commented Aug 25, 2024

rust-timer commented Aug 25, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Build `library/profiler_builtins` from `ci-llvm` if appropriate #129295

Build `library/profiler_builtins` from `ci-llvm` if appropriate #129295

Zalathar commented Aug 20, 2024 •

edited

Loading

onur-ozkan commented Aug 23, 2024 •

edited

Loading