Stabilise c_unwind

[nbdd0121]

Fix #74990
Fix #115285 (that's also where FCP is happening)

Marking as draft PR for now due to compiler_builtins issues

r? @Amanieu

[kornelski]

I've updated the mozjpeg crate to use the "C-unwind" ABI, so the stabilization of aborting in "C" ABI is fine by me.

[Amanieu]

compiler_builtins is not being compiled with panic=abort, and __umodti4 calls specialized_div_rem which is extern "Rust" and defined in a separate codegen unit.

I think #113923 should resolve this, but I'm not 100% sure.

Alternatively we should ensure that -Cpanic=abort -Coverflow-checks=off is always passed when building compiler-builtins since this isn't currently the case.

[dianqk]

I think #113923 should resolve this, but I'm not 100% sure.

From the error log, yes.

[nbdd0121]

Panic strategy and overflow checks are currently defined on Session, and if we want to force them for compiler builtins then we'll need to make them queries. Maybe instead of ad-hoc special cases we should just wait for #113923 to land first.

[bors]

☔ The latest upstream changes (presumably #116260) made this pull request unmergeable. Please resolve the merge conflicts.

[bjorn3]

Panic strategy and overflow checks are currently defined on Session, and if we want to force them for compiler builtins then we'll need to make them queries.

We also need to force an unlimited amount of codegen units for compiler-builtins to be compiled correctly. -Zbuild-std doesn't set all these things correctly for compiler-builtins, so it would be nice to have rustc handle this. Would also allow the panic_abort crate to always be built with panic=abort even without a rustc wrapper matching on the crate name.

[Dylan-DPC]

@nbdd0121 any updates on this?

[nbdd0121]

@nbdd0121 any updates on this?

Still blocked by #113923

[bors]

☔ The latest upstream changes (presumably #117472) made this pull request unmergeable. Please resolve the merge conflicts.

[tgross35]

@nbdd0121 #113923 is in the process of being merged now

[nbdd0121]

I spent way too much time debugging the error today...

I couldn't reproduce the issue when compiling directly (with downloaded LLVM), but the issue is reproducible with both x86_64-gnu-llvm-16 and x86_64-gnu-llvm-17 containers.

This error looks identical to #116976 (llvm/llvm-project#70578), and fix of which is to upgrade LLVM to 17.0.4. But the CI is using LLVM versions without the fix (I checked that the CI containers uses Ubuntu shipped LLVM 16.0.0 and 17.0.2, both of which I believe does not have the fix).

What should be done to get the CI to work? Perhaps we should disable noalias when LLVM version < 17.0.4?

cc @dianqk

[dianqk]

I spent way too much time debugging the error today...

I couldn't reproduce the issue when compiling directly (with downloaded LLVM), but the issue is reproducible with both x86_64-gnu-llvm-16 and x86_64-gnu-llvm-17 containers.

This error looks identical to #116976 (llvm/llvm-project#70578), and fix of which is to upgrade LLVM to 17.0.4. But the CI is using LLVM versions without the fix (I checked that the CI containers uses Ubuntu shipped LLVM 16.0.0 and 17.0.2, both of which I believe does not have the fix).

What should be done to get the CI to work? Perhaps we should disable noalias when LLVM version < 17.0.4?

cc @dianqk

Could you try to revert this commit locally to verify that it is indeed the same issue? We may need to build stage 2.
Removing noalias seems like a big change, and we may need to talk about confirming it in wg-llvm or even more generally.

[RalfJung]

Cc @nikic
what are we usually doing when unpatched LLVM leads to segfaults due to miscompilation?

[nikic]

Hm, this is a tricky case. Disabling noalias on older versions is a possibility, but it comes with it's own risks -- it's a big change in optimization behavior, which may surface other latent issues. We've seen this e.g. with the attempt to drop inbounds on getelementptr. I'd rather not rock the boat if we can avoid it, especially for old versions that receive little testing.

Something we should consider, especially to account for future cases where there might not be any workaround like "disable noalias", is to explicitly document additional patches that Rust requires if a certain system LLVM version is used. In that case we should change our llvm-N CI to build LLVM with the necessary patches ourselves.

At least for Fedora's use case (cc @cuviper) I think we wouldn't mind if additional patches are required, as long as they're clearly documented.

[nbdd0121]

Could you try to revert this commit locally to verify that it is indeed the same issue?

Yes, I can confirm that reverting [MemCpyOpt] Combine alias metadatas when replacing byval arguments (#70580) will cause the sigsegv to trigger with locally built LLVM.

---

Hm, this is a tricky case. Disabling noalias on older versions is a possibility, but it comes with it's own risks -- it's a big change in optimization behavior, which may surface other latent issues. We've seen this e.g. with the attempt to drop inbounds on getelementptr. I'd rather not rock the boat if we can avoid it, especially for old versions that receive little testing.

I think noalias probably is one of the safer attribute to disable, consider that we already disable it for &!Freeze and &mut !Unpin and clang rarely generates them.

Something we should consider, especially to account for future cases where there might not be any workaround like "disable noalias", is to explicitly document additional patches that Rust requires if a certain system LLVM version is used. In that case we should change our llvm-N CI to build LLVM with the necessary patches ourselves.

This would be great, would it then require cooperation from distributions to ensure miscompilation bugs that affect us are backported to LLVM releases that they support?

---

I found a workaround which is to change extern "C" in library/proc_macro/src/bridge/buffer.rs to extern "C-unwind", which inhibits some optimisation opportunity for LLVM that happens to mask the bug. However it feels very hacky to tweak source code so that LLVM happens to not miscompile, and I don't have the confidence that code in the wild wouldn't be misoptimised.

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[bors]

⌛ Testing commit bb2716e with merge 1aaab8b...

[bors]

☀️ Test successful - checks-actions
Approved by: Amanieu,RalfJung
Pushing 1aaab8b to master...

[rust-timer]

Finished benchmarking commit (1aaab8b): 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 (primary -4.8%)

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

Cycles

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

Binary size

Results (primary -0.2%, secondary -1.0%)

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.1%	[0.1%, 0.1%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.5%, -0.1%]	15
Improvements ✅ (secondary)	-1.0%	[-1.5%, -0.6%]	6
All ❌✅ (primary)	-0.2%	[-0.5%, 0.1%]	19

Bootstrap: 691.418s -> 690.905s (-0.07%)
Artifact size: 323.81 MiB -> 323.81 MiB (-0.00%)

[tgross35]

About time :) thanks for all the work on this @nbdd0121!!