Move EH personality functions to std

[Amanieu]

These were previously in the panic_unwind crate with dummy stubs in the
panic_abort crate. However it turns out that this is insufficient: we
still need a proper personality function even with -C panic=abort to
handle the following cases:

1. extern "C-unwind" still needs to catch foreign exceptions with -C
   panic=abort to turn them into aborts. This requires landing pads and a
   personality function.

2. ARM EHABI uses the personality function when creating backtraces.
   The dummy personality function in panic_abort was causing backtrace
   generation to get stuck in a loop since the personality function is
   responsible for advancing the unwind state to the next frame.

Fixes #41004

[Mark-Simulacrum]

It seems like the catching of foreign exceptions should work even if std isn't available - should this be in core perhaps? Similarly backtrace generation in theory isn't dependent on std, so if the function is needed for that it should be provided?

[Amanieu]

The eh_personality lang item is only required in no_std if extern "C-unwind" is used. Otherwise, we don't want to force no_std code to have a dependency on libunwind if it's not needed.

On ARM, a default personality function (provided by libunwind) is used for functions that don't specify a personality (which is the case for most code compiled with -C panic=abort). Again, this is only needed on no_std with extern "C-unwind" which uses rust_eh_personality.

[nbdd0121]

eh_personality lang item is also required if no_std crate uses panic=unwind. I have use cases where I supply a separate panic runtime and unwinding runtime (https://github.com/nbdd0121/unwinding). Currently I have to supply a copy of the personality function as well, but ideally that can be provided by some lib bundled with Rust.

[Amanieu]

I think that in the future we could move the unwinding-related functionality to its own crate that is independent of std. This crate would then only be pulled in by panic_unwind and if any codegen within a crate requires a personality function (-Cpanic=unwind or any use of extern "C-unwind"). However I feel that this should be done in a separate PR.

[joshtriplett]

One of the major reasons people use panic=abort is code size. This change seems likely to produce a regression on that front.

[bjorn3]

The personality function isn't all that big. In addition if you truly care about code size you will compile every crate including the standard library with -Cpanic=abort in which case there won't be any references to the personality function and thus it will be removed by --gc-sections.

[joshtriplett]

@bjorn3 Recompiling the standard library is much more difficult than most other steps someone might take in optimization.

[Amanieu]

On a hello world program compiled with rustc -C panic=abort -C strip=symbols there is a slight increase in binary size from 313656 bytes to 317752 bytes. The difference is 4096 bytes but this is probably due to rounding, so the "real" difference is less than 4KB. I think this is perfectly acceptable relative to the overall size of std.

[nbdd0121]

I think that in the future we could move the unwinding-related functionality to its own crate that is independent of std. This crate would then only be pulled in by panic_unwind and if any codegen within a crate requires a personality function (-Cpanic=unwind or any use of extern "C-unwind"). However I feel that this should be done in a separate PR.

If we want to move it to its own crate eventually then we might just do so without first moving it into std. The codegen part could be done later and we can just let std unconditionally pull it in for now.

[joshtriplett]

@Amanieu Hello world pulls in all the format machinery, which embedded often tries to avoid. What's the delta for an empty main function?

[nbdd0121]

std panic handlers would already pull in all format machinery. Embedded would use no_std.

[Amanieu]

If we want to move it to its own crate eventually then we might just do so without first moving it into std. The codegen part could be done later and we can just let std unconditionally pull it in for now.

I think moving into std is a good first step since it separates the personality function from the choice of panic runtime. It should make it easier to move into a separate crate in the future. Note that it can't be placed in core itself since unwinding has OS dependencies (e.g. dl_iterate_phdr).

[nbdd0121]

I don't see why moving it into a separate crate requires it to go through std first. The personality function doesn't depend on std. The personality function also has no OS dependencies. It does depend on an unwinding runtime, but unwinding runtime can function without OS.

[Amanieu]

In this PR I'm just trying to fix two issues which require a personality function even with -Cpanic=abort. The minimal solution is to move the personality function into std.

Adding a new crate to support unwinding without std would require a lot more work which I have chosen to leave for a future PR.

[nbdd0121]

I don't see how adding a new crate is "a lot more work". This is all it takes:

diff --git a/Cargo.lock b/Cargo.lock
index ef9f91fdb43..1c9ce7f75e0 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1078,6 +1078,17 @@ dependencies = [
  "rustc-std-workspace-core",
 ]
 
+[[package]]
+name = "eh_personality"
+version = "0.0.0"
+dependencies = [
+ "cfg-if 0.1.10",
+ "compiler_builtins",
+ "core",
+ "libc",
+ "unwind",
+]
+
 [[package]]
 name = "either"
 version = "1.6.0"
@@ -5066,6 +5077,7 @@ dependencies = [
  "compiler_builtins",
  "core",
  "dlmalloc",
+ "eh_personality",
  "fortanix-sgx-abi",
  "hashbrown",
  "hermit-abi",
diff --git a/library/eh_personality/Cargo.toml b/library/eh_personality/Cargo.toml
new file mode 100644
index 00000000000..47cdf9ee7e9
--- /dev/null
+++ b/library/eh_personality/Cargo.toml
@@ -0,0 +1,19 @@
+[package]
+name = "eh_personality"
+version = "0.0.0"
+license = "MIT OR Apache-2.0"
+repository = "https://github.com/rust-lang/rust.git"
+description = "Personality function for unwinding Rust stack frames"
+edition = "2021"
+
+[lib]
+test = false
+bench = false
+doc = false
+
+[dependencies]
+core = { path = "../core" }
+libc = { version = "0.2", default-features = false }
+unwind = { path = "../unwind" }
+compiler_builtins = "0.1.0"
+cfg-if = "0.1.8"
diff --git a/library/eh_personality/src/lib.rs b/library/eh_personality/src/lib.rs
new file mode 100644
index 00000000000..0c9ac1ac8e4
--- /dev/null
+++ b/library/eh_personality/src/lib.rs
@@ -0,0 +1 @@
+#![no_std]
diff --git a/library/std/Cargo.toml b/library/std/Cargo.toml
index 232ccdf39d4..66c913f1911 100644
--- a/library/std/Cargo.toml
+++ b/library/std/Cargo.toml
@@ -21,6 +21,7 @@ profiler_builtins = { path = "../profiler_builtins", optional = true }
 unwind = { path = "../unwind" }
 hashbrown = { version = "0.11", default-features = false, features = ['rustc-dep-of-std'] }
 std_detect = { path = "../stdarch/crates/std_detect", default-features = false, features = ['rustc-dep-of-std'] }
+eh_personality = { path = "../eh_personality" }
 
 # Dependencies of the `backtrace` crate
 addr2line = { version = "0.16.0", optional = true, default-features = false }
diff --git a/library/std/src/lib.rs b/library/std/src/lib.rs
index 4ba4e2a528e..27704b7d880 100644
--- a/library/std/src/lib.rs
+++ b/library/std/src/lib.rs
@@ -368,6 +368,10 @@
 #[allow(unused_extern_crates)]
 extern crate unwind;
 
+#[doc(masked)]
+#[allow(unused_extern_crates)]
+extern crate eh_personality;
+
 // During testing, this crate is not actually the "real" std library, but rather
 // it links to the real std library, which was compiled from this same source
 // code. So any lang items std defines are conditionally excluded (or else they

[Amanieu]

I would prefer to leave splitting this into a separate crate to a future PR.

[Mark-Simulacrum]

I'm a little concerned by the coupling of changes and code movement that make reviewing those changes difficult - can you clarify whether those are intended? I noted a few cases...

[Mark-Simulacrum]

I'm a little concerned by the coupling of changes and code movement that make reviewing those changes difficult - can you clarify whether those are intended? I noted a few cases...

Sorry if it wasn't clear about this. My worry is that it's difficult to review this PR as-is, because it's coupling code movement and code changes. Can you split this out into separate commits at least? It might be easier to do that from scratch, but as-is it's essentially forcing me to scroll back and forth to reconstruct that separation manually, which seems like it's easy to miss something and not very reliable.

[Amanieu]

I split it up into separate commits.

[Amanieu]

@bors r=Mark-Simulacrum

[bors]

📌 Commit a7e4794 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

[bors]

⌛ Testing commit a7e4794 with merge 91f128b...

[bors]

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

[bjorn3]

This means that rust_eh_personality is now exported from cdylibs as it doesn't use SymbolExportLevel::Rust, right? That is going to give troubles when depending on two cdylibs compiled by different versions of rustc if we change the eh_personality implementation as one version of this symbol will override the other.

[Amanieu]

I think symbol_export_level still returns Rust for rust_eh_personality since it doesn't have any attribute such as no_mangle that marks it as externally visible.

[bjorn3]

I just checked it and you are indeed right. rust_eh_personality is not exported.

[rust-timer]

Finished benchmarking commit (91f128b): 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.

	mean1	range	count2
Regressions ❌ (primary)	2.2%	[2.2%, 2.2%]	1
Regressions ❌ (secondary)	4.0%	[1.1%, 7.0%]	2
Improvements ✅ (primary)	-2.5%	[-2.7%, -2.3%]	2
Improvements ✅ (secondary)	-3.3%	[-3.3%, -3.3%]	1
All ❌✅ (primary)	-0.9%	[-2.7%, 2.2%]	3

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.

	mean1	range	count2
Regressions ❌ (primary)	2.0%	[2.0%, 2.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.4%	[-3.4%, -3.4%]	1
Improvements ✅ (secondary)	-2.6%	[-3.0%, -2.2%]	2
All ❌✅ (primary)	-0.7%	[-3.4%, 2.0%]	2

Footnotes

1. the arithmetic mean of the percent change ↩ ↩²

2. number of relevant changes ↩ ↩²

[glandium]

This is causing problems building Firefox with *-windows-gnu targets, whereby staticlibs built with panic=abort now fail to link because of missing the symbols _GCC_specific_handler, _Unwind_GetLanguageSpecificData, _Unwind_GetIPInfo, _Unwind_GetRegionStart, _Unwind_SetGR, _Unwind_SetIP, _Unwind_GetDataRelBase, and _Unwind_GetTextRelBase. Interestingly, this doesn't happen on other targets Firefox is built for.

[Amanieu]

These symbols are provided by either libgcc_eh.a or libgcc_s-dw2-1.dll. Usually the former.

rust/compiler/rustc_target/src/spec/windows_gnu_base.rs

Lines 54 to 58 in ff479b1

	// If all of our crates are statically linked then we can get away
	// with statically linking the libgcc unwinding code. This allows
	// binaries to be redistributed without the libgcc_s-dw2-1.dll
	// dependency, but unfortunately break unwinding across DLL
	// boundaries when unwinding across FFI boundaries.

[glandium]

They shouldn't be needed though for panic=abort + -Clto (and weren't before).

[ChrisDenton]

This is perhaps related to the point made at the end of this comment:

It may just be because we build the standard library with panic=unwind so that it can be used with both panic=unwind and panic=abort.

[Amanieu]

Right, if you rebuild the standard library with -C panic=abort and -Zbuild-std then these symbols will go away.

[glandium]

Not sure about *-windows-gnu, but at least on *-linux-gnu, -Clto eliminates unused code from std. It doesn't do it for this, though...

Another problem that arises from this is that when building Firefox for aarch64-apple-darwin, linking fails with "ld64.lld: error: too many personalities (4) for compact unwind to encode"

[BatmanAoD]

@glandium @Amanieu Should these issues be moved to a new ticket? Are they related to #101134 at all?

Is this a stable-to-nightly regression?

[Amanieu]

Not sure about *-windows-gnu, but at least on *-linux-gnu, -Clto eliminates unused code from std. It doesn't do it for this, though...

Previously we had an additional LLVM pass inserted in LTO which removed all unwinding from functions when -C panic=abort was used. However this was removed because it broken extern "C-unwind" which needs to catch unwinds (to turn them into aborts) in -C panic=abort.