Update to LLVM 19

[nikic]

The LLVM 19.1.0 final release is planned for Sep 3rd. The rustc 1.82 stable release will be on Oct 17th.

The unstable MC/DC coverage support is temporarily broken by this update. It will be restored by #126733. The implementation changed substantially in LLVM 19, and there are no plans to support both the LLVM 18 and LLVM 19 implementation at the same time.

This update enables additional target features by default for wasm targets. See #128511 (comment) for details.

Related changes:

• Remove extern "wasm" ABI #127605
• Update dist-riscv64-linux to binutils 2.40 #127613
• Fix incorrect NDEBUG handling in LLVM bindings #127654
• Set branch protection function attributes #128141
• [nsan] Swap alignas and visibility order (NFC) llvm/llvm-project#98933

Fixes #121444.
Fixes #128212.

[nikic]

@bors try

[bors]

⌛ Trying commit a23e9a5 with merge d009d2d...

[bors]

☀️ Try build successful - checks-actions
Build commit: d009d2d (d009d2d060b39b34c04f23a9b0700a72d95ed9ea)

[nikic]

@rust-timer build d009d2d

[rust-timer]

Finished benchmarking commit (d009d2d): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +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)	1.4%	[0.3%, 3.2%]	10
Regressions ❌ (secondary)	0.9%	[0.2%, 3.2%]	34
Improvements ✅ (primary)	-3.7%	[-15.9%, -0.2%]	136
Improvements ✅ (secondary)	-2.6%	[-10.5%, -0.2%]	76
All ❌✅ (primary)	-3.4%	[-15.9%, 3.2%]	146

Max RSS (memory usage)

Results (primary -3.5%, secondary 1.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)	5.4%	[4.2%, 6.6%]	2
Regressions ❌ (secondary)	6.3%	[4.9%, 7.8%]	2
Improvements ✅ (primary)	-4.5%	[-11.2%, -2.1%]	17
Improvements ✅ (secondary)	-3.1%	[-3.2%, -3.0%]	2
All ❌✅ (primary)	-3.5%	[-11.2%, 6.6%]	19

Cycles

Results (primary -5.4%, secondary -3.7%)

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)	4.1%	[1.9%, 6.5%]	4
Improvements ✅ (primary)	-5.4%	[-14.5%, -0.8%]	72
Improvements ✅ (secondary)	-4.4%	[-9.4%, -1.5%]	45
All ❌✅ (primary)	-5.4%	[-14.5%, -0.8%]	72

Binary size

Results (primary 0.6%, secondary 1.5%)

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.9%	[0.0%, 2.4%]	45
Regressions ❌ (secondary)	1.7%	[0.6%, 2.4%]	78
Improvements ✅ (primary)	-0.1%	[-0.4%, -0.0%]	17
Improvements ✅ (secondary)	-5.0%	[-9.4%, -0.6%]	2
All ❌✅ (primary)	0.6%	[-0.4%, 2.4%]	62

Bootstrap: 702.485s -> 689.195s (-1.89%)
Artifact size: 328.89 MiB -> 333.03 MiB (1.26%)

[nikic]

tests/run-make/wasm-abi on wasm-wasip1 fails with:

Caused by:
    0: failed to instantiate "foo.wasm"
    1: incompatible import type for `host::two_i32`
    2: function types incompatible: expected func of type `(i32) -> ()`, found func of type `() -> (i32, i32)`

I believe this is caused by llvm/llvm-project#88492, which now seems to ignore the +multivalue feature unless the experimental-mv target ABI is used.

I'm a bit unclear on what the implications of that change on Rust are. Are we supposed to switch all our wasm targets to experimental-mv so that the extern "wasm" ABI isn't ignored? I don't really get this change, given that +multivalue is a per function target feature while the target ABI is global.

cc @alexcrichton

[CryZe]

Are we supposed to switch all our wasm targets to experimental-mv so that the extern "wasm" ABI isn't ignored?

That sounds like a mistake, because the plan is for extern "C" to match clang soon, which would mean that experimental-mv would have to be off.

Another point to add is that +multivalue is now also active for all functions. This to me means that extern "wasm" currently serves not much purpose anymore, especially with wasm-bindgen not having ended up using it. So to me this either means breaking (and or removing) extern "wasm" for now, or patching LLVM to support experimental-mv on a per function basis.

I guess a third solution would be to -multivalue,+experimental-mv, which would bring us essentially back to what the previous LLVM did, at the cost of losing multivalue for non extern "wasm" functions.

[nikic]

Another point to add is that +multivalue is now also active for all functions. This to me means that extern "wasm" currently serves not much purpose anymore, especially with wasm-bindgen not having ended up using it. So to me this either means breaking (and or removing) extern "wasm" for now, or patching LLVM to support experimental-mv on a per function basis.

To clarify my previous comment, because I think it didn't get across right: If I understand correctly, if experimental-mv is used, then it's still necessary to use +multivalue on individual functions. Multivalue will not be automatically enabled for everything just because the experimental-mv ABI is used. (Which is why I am so confused as to the purpose of that ABI.)

[CryZe]

Yes, but +multivalue is active by default now (which I assume also means it's on all functions? I wasn't even aware there's per function features until you mentioned it). https://github.com/llvm/llvm-project/pull/96584/files#diff-0580bdcbbd17bc30025ee4bb132537f58004b39bb7a86f0a8c9e6a12d96c8dd5R113

So this would mean if you added +experimental-mv that it would trigger the ABI for all functions, not just extern "wasm".

[nikic]

Yes, but +multivalue is active by default now (which I assume also means it's on all functions? I wasn't even aware there's per function features until you mentioned it). https://github.com/llvm/llvm-project/pull/96584/files#diff-0580bdcbbd17bc30025ee4bb132537f58004b39bb7a86f0a8c9e6a12d96c8dd5R113

Ah, I see what you mean now. So I guess if we wanted to keep the status quo, we'd have to a) use experimental-mv ABI and b) add -multivalue to all non-extern-wasm functions to opt out again, which is kind of convoluted.

For the record, if I set llvm_abiname to experimental-mv without doing anything else, then the run-make test passes and this ui test fails instead:

---- [ui] tests/ui/numbers-arithmetic/u128.rs stdout ----

error: test run failed!
status: exit status: 134
command: cd "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/numbers-arithmetic/u128" && RUST_TEST_THREADS="64" "/wasmtime-v19.0.0-x86_64-linux/wasmtime" "run" "-C" "cache=n" "--dir" "." "--env" "RUSTC_BOOTSTRAP" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/numbers-arithmetic/u128/a.wasm"
stdout: none
--- stderr -------------------------------
Error: failed to run main module `/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/numbers-arithmetic/u128/a.wasm`

Caused by:
    0: failed to invoke command default
    1: error while executing at wasm backtrace:
           0: 0x7f36 - a.wasm!memset
           1: 0xaf6b - a.wasm!core::fmt::num::fmt_u128::h1ec55f0ea0d172dc
           2: 0xae26 - a.wasm!core::fmt::num::<impl core::fmt::Display for u128>::fmt::h700225ef905cc701
           3: 0x9721 - a.wasm!core::fmt::write::ha0695b3bfa74a24d
           4: 0x8c7f - a.wasm!alloc::fmt::format::format_inner::h00360545e4341607
           5:  0x80d - a.wasm!u128::main::h1267772770aec22e
           6:  0x318 - a.wasm!std::sys::backtrace::__rust_begin_short_backtrace::hd80d215d9cabb8e6
           7:  0x30b - a.wasm!std::rt::lang_start::{{closure}}::h676c49074f69fa27
           8: 0x2a2a - a.wasm!std::rt::lang_start_internal::h7a5c1daedfda3666
           9: 0x156f - a.wasm!__main_void
          10:  0x2e3 - a.wasm!_start
    2: wasm trap: out of bounds memory access
------------------------------------------

That's possibly related to #127318.

[CryZe]

Is this with wasm32-unknown-unknown or WASI? Because there's a big difference between those two as well in their C ABI. I believe the unknown target's default C ABI is incompatible with multivalue anyway.

[alexcrichton]

I would agree with @CryZe that the best solution is to basically "do nothing" and disable any tests related to multivalue. (and/or just comment out the multivalue parts of the tests). There's been various discussions in a bunch of places with varying degrees of depth and complexity but the basic conclusion, in my opinion, is that multivalue just isn't feasible with LLVM. The "wasm" ABI definitely never materialized and should be removed and preexisting multivalue experiments were fraught with issues.

The goal at this point is to match Clang's C ABI on all targets, whether or not they're wasm32-unknown-unknown or not. Having a per-compilation-unit ABI setting is not usable from Rust since we're not going to add an entire target just for multi-value. I would still like to see a per-function setting for multi-value one day but there's not much interest in doing so from the current wasm backend maintainers since I don't think they have use cases to motivate it and aren't too interested into digging into other use cases.

[nikic]

@alexcrichton To check my understanding here, we have following ABIs:

1. The C ABI on wasm, in the sense of -Z wasm-c-abi=spec.
2. The legacy rustc wasm ABI in the sense of -Z wasm-c-abi=legacy (which is the default).
3. The extern "wasm" ABI, which is -Z wasm-c-abi=legacy plus the +multivalue feature.

Is that correct? In particular I want to confirm that 2 and 3 are not the same thing. The suggestion here is that we can go ahead and remove extern "wasm" (and thus our only direct use of +multivalue), while leaving the "normal" legacy wasm ABI alone?

If that's the case, I'd rather just go ahead and do that rather than silently break it on LLVM 19.

[alexcrichton]

I believe that's correct yeah, and I'll cc @daxpedda here as well since they added -Zwasm-c-abi I believe. The exact definition of the extern "wasm" ABI in (3) I think it's quite "just" legacy+multivalue, but regardless it's distinct from (2). Furthermore the extern "wasm" ABI has always been unstable and as far as I'm aware nothing is relying on it so it should be a suitable candidate for removal.

Put another way the only way to keep extern "wasm" would be to have -target-abi=experimental-mv on a per-function basis. That doesn't exist in LLVM (AFAIK) so there's just no way to keep extern "wasm". The implementation using +multivalue on functions that use extern "wasm" is broken with LLVM 19+ as well.

[nikic]

Okay, thanks for walking me through it! In that case I'll work on a PR to remove extern "wasm" / the wasm_abi feature.

For future reference, wasm_abi was added in #83763 and -Z wasm-c-abi in #117919.

[daxpedda]

Thank you for pinging me!

Removing extern "wasm" should be fine indeed, I can confirm that wasm-bindgen is not using it.
Of course it would be nice if we could get multivalue support in Rust, but as was already said, the motivation to fix LLVM to support it is quite low.

However, I believe we should not enable the multivalue and reference types target features by default, as both are unimplemented and it would be wrong for us to advertise that they are required to read the Wasm module.

I'm unsure exactly how to do that, but I believe we might want to patch our fork here and here or use the -target-cpu=mvp by default and enable our own set of default features, though I believe this would make users unable to use the MVP target CPU to actually disable all target features (which might be useless anyway because it requires build-std to fully work). Happy to make a PR after some input.

Or maybe we might consider stabilizing the multivalue feature without the ability to export functions using it. This would still be useful for non-exported functions. But probably not because its still exhibiting bugs.

[nikic]

PR for removing extern "wasm" is up at #127605.

I'm unsure exactly how to do that, but I believe we might want to patch our fork here and here or use the -target-cpu=mvp by default and enable our own set of default features, though I believe this would make users unable to use the MVP target CPU to actually disable all target features (which might be useless anyway because it requires build-std to fully work). Happy to make a PR after some input.

We cannot patch our LLVM fork to make functional changes. We can either use the mvp cpu with default features +mutable-globals,+sign-ext or keep the generic cpu with default features -multivalue,-reference-types.

[alexcrichton]

Ah yeah I'll highlight @CryZe's point above in that +multivalue is now on-by-default for LLVM at all times, but it has no actually affect on anything unless -target-abi=experimental-mv is also passed. I believe the maintainers wanted an up-to-date list of wasm proposals-as-features in terms of what's active, but it's acknowledged that the multivalue support in LLVM is not complete at this time.

[Kobzol]

Improvements far outweigh the regressions.

@rustbot label: +perf-regression-triaged

[alexcrichton]

For the relnotes label here could the wasm-related changes be removed from this PR? There's been a bit of confusion on what exactly the impact of this change was on wasm targets and such confusion should be addressed and resolved in #128511, notably with updated release notes that look like this. In the spirit of not duplicating release notes across PRs it might be best to leave the wasm-specific impact to that PR perhaps? (or alternatively rewrite this PR's release notes to match that PR's and remove relnotes from that PR, either way is fine by me)

[jieyouxu]

For the relnotes label here could the wasm-related changes be removed from this PR? There's been a bit of confusion on what exactly the impact of this change was on wasm targets and such confusion should be addressed and resolved in #128511, notably with updated release notes that look like this. In the spirit of not duplicating release notes across PRs it might be best to leave the wasm-specific impact to that PR perhaps? (or alternatively rewrite this PR's release notes to match that PR's and remove relnotes from that PR, either way is fine by me)

FYI @rust-lang/release as this is easy to miss.