Pass -Werror when building the LLVM wrapper

[rperier]

cc #109712

[rustbot]

r? @cuviper

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

[cuviper]

I disagree with applying -Werror wholesale, because that can break anyone who happens to use a different C++ compiler than what we already tested with, especially for new warnings that may not even be real problems, or may be caused by external headers out of our control.

Maybe we could inject -Werror only for Rust CI, where we control the entire build environment.

[rperier]

Even if warnings are not enabled ? Well, even if you can still get warnings that are not parts of -Wall and -Wextra...
I understand now what @jyn514 meant by "in CI". I gonna to investigate to find how to inject this only for Rust CI.

[Noratrieb]

note that this here isn't actually building llvm, it's just building rustc's wrapper. the actual LLVM build happens in src/bootstrap

[cuviper]

I gonna to investigate to find how to inject this only for Rust CI.

I don't think it needs to be sophisticated -- maybe just check for the CI environment variable, which has precedent:

rust/compiler/rustc_codegen_llvm/src/llvm_util.rs

Line 55 in 855e0fe

if std::env::var_os("CI").is_some() {

[Noratrieb]

Bootstrap has existing methods for checking whether it's in rust-lang/rust CI, maybe you could reuse those by piping through environment variables or something like that

[rperier]

Boostrap has existing types and methods for checking whether it's in rust-lang/rust CI, that's the CiEnv enum. However, it seems that .github/workflows/ci.yml calls src/ci/docker/run.sh at some point. The docker script wrapper set the environment variable CI before running the container. That's the variable that is used later by compiler/rustc_codegen_llvm/src/llvm_util.rs. Checking for the presence of the CI environment variable looks simpler, as we already have everything we need, in my humble opinion.

[cuviper]

@bors r+ rollup=iffy

[bors]

📌 Commit 8271991 has been approved by cuviper

It is now in the queue for this repository.

[Zalathar]

Failed in rollup: #145031 (comment)

@bors r-

[tgross35]

@bors r- #145031 (comment)

@bors2 try jobs=aarch64-msvc-1

[rust-bors]

⌛ Trying commit 8271991 with merge ebba86f…

To cancel the try build, run the command @bors try cancel.

[rust-bors]

💔 Test failed (CI). Failed jobs:

• bors build finished (web logs, extended logs)
• try - aarch64-msvc-1 (web logs, extended logs)

[rperier]

Whooops, sorry, it seems that something went wrong. I guess that I am able to reproduce the issue by using the try command with bors (at least according to the doc). So either I exclude warnings_into_errors for these targets (by using the TARGET variable)... but it looks a workaround to me, or I fix the warnings (do you want to invest time for this ?)

[rperier]

@bors try jobs=aarch64-msvc-1

[rust-bors]

@rperier: 🔑 Insufficient privileges: not in try users

[rperier]

As this PR was intended to be merged as a trivial change, I propose the following fix, it is less intrusive than fixing the warnings (hope this helps as I cannot try the job via bors).

@rustbot ready

[tgross35]

@bors2 try jobs=aarch64-msvc-1

[rust-bors]

⌛ Trying commit 57491cb with merge e255101…

To cancel the try build, run the command @bors try cancel.

[rust-bors]

☀️ Try build successful (CI)
Build commit: e255101 (e255101d6c5c5b93497c2d6860caa21624efcf3d, parent: cd434309efcf5dc68b253e5ef6ba40c1c43711c9)

[cuviper]

@bors r+

[bors]

📌 Commit d37fae3 has been approved by cuviper

It is now in the queue for this repository.

[bors]

⌛ Testing commit d37fae3 with merge a980cd4...

[bors]

☀️ Test successful - checks-actions
Approved by: cuviper
Pushing a980cd4 to master...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 2fd855f (parent) -> a980cd4 (this PR)

Test differences

Show 4 test diffs

4 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard a980cd4311ae4b5bf9099d418e32643d068f1344 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. x86_64-apple-1: 10634.4s -> 6526.5s (-38.6%)
2. x86_64-apple-2: 3565.5s -> 4757.7s (33.4%)
3. dist-x86_64-apple: 9494.9s -> 11888.7s (25.2%)
4. dist-powerpc64le-linux-musl: 5856.7s -> 5094.9s (-13.0%)
5. aarch64-apple: 6339.5s -> 6972.1s (10.0%)
6. dist-arm-linux-gnueabi: 5272.4s -> 4776.4s (-9.4%)
7. dist-loongarch64-musl: 5465.9s -> 4955.1s (-9.3%)
8. aarch64-gnu-llvm-19-1: 3478.3s -> 3167.5s (-8.9%)
9. dist-armv7-linux: 5523.5s -> 5062.7s (-8.3%)
10. aarch64-gnu: 7019.8s -> 6525.2s (-7.0%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (a980cd4): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.6%	[-0.6%, -0.5%]	3
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary 3.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Cycles

Results (secondary 0.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.4%	[2.6%, 6.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.0%	[-3.7%, -2.2%]	2
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 465.394s -> 465.2s (-0.04%)
Artifact size: 377.41 MiB -> 377.41 MiB (0.00%)