Change entry point to 🛡️ against 💥 💥-payloads

[nagisa]

Guard against panic payloads panicking within entrypoints, where it is
UB to do so.

Note that there are a number of tradeoffs to consider. For instance, I
considered guarding against accidental panics inside the rt::init and
rt::cleanup code as well, as it is not all that obvious these may not
panic, but doing so would mean that we initialize certain thread-local
slots unconditionally, which has its own problems.

Fixes #86030
r? @m-ou-se

[bjorn3]

Maybe it would be safer to add a drop bomb that aborts the process when unwinding above the catch_unwind(main) call? That would also ensure an abort when C++ unwinds into rust code. We define that as UB anyway, but just in case.

[nagisa]

There are multiple implementation approaches that could be taken here, indeed. Each has their own considerations.

I don't think the choice should be made based on behaviour in case UB is invoked, however. This code handling a super niche corner-case in the first place; I think the simplicity of the implementation is probably outweighs in benefit the better UB resiliency.

That said, a drop-guard would allow to guard against panics in rt::init and rt::cleanup, which is quite desirable, yes.

[nagisa]

Alas, the drop guard approach does not work. That's because there isn't any catch pads the unwinding implementation could find, and thus you get the same old UB as if we did nothing at all.

I did adjust the implementation to use another catch_unwind, however, which should handle the issues with user code and certain runtime functions panicking.

[m-ou-se]

This PR guards against two things: 1. panics from rt::init/rt::cleanup, 2. panics from dropping a user-defined panic payload.

Maybe it's good to separate how those are handled.

For 1, we don't need to worry about dropping the payload, since we don't throw non-string payloads in the standard library (panic_any isn't used anywhere).

For 2, we should make sure the drop happens before cleanup because it executes user code. And we might want to show a different message, like "panic payload panicked" or something.

Regardless, this PR is already a big improvement over the current situation. So r=me either way.

[nagisa]

For 1, we don't need to worry about dropping the payload

I think this already holds true – we don't particularly worry much about the runtime panics, as they are bugs in libstd's implementation and the fix would be to not panic in the first place.

For 2, we should make sure the drop happens before cleanup because it executes user code. And we might want to show a different message, like "panic payload panicked" or something.

I adjusted the abort message to focus more on the panic-payload panics and less on the runtime side of the things. Ideally in absence of bugs in the libstd's implementation the only ways users will end up seeing the message is by panicking in the payload.

[nagisa]

@bors r=m-ou-se

[bors]

📌 Commit 594e4030b30749d496904438579f2e4ce6194563 has been approved by m-ou-se

[m-ou-se]

I think this can be really confusing when the standard library does somehow produce a panic outside of main. If you want to separate these cases and have a message that's specific to the panic payload drop, that'd be good, but it should really just be a separate catch_unwind. I'm afraid that like this, it's going to cause someone a lot of confusion and cost them a lot of time to debug a panic at some point.

[nagisa]

Well, IMO a backtrace would have revealed the problem regardless, but adjusted this anyway. That said, lang_start_internal has become a big pile of yucky assembly with this :(

[m-ou-se]

@bors r-

[nagisa]

This is ready for a re-review.

[m-ou-se]

There is a Result::into_ok for this: https://doc.rust-lang.org/nightly/std/result/enum.Result.html#method.into_ok

(I suppose you could also use that directly instead of the ?s above, to avoid changing the return type to Result.)

[m-ou-se]

I don't necessarily mind sorting the features here (though it might cause unnecessary merge conflicts), but these ones specifically were already sorted correctly. The _ comes before a.

[m-ou-se]

Ah, probably a locale thing:

$ LANG=C sort thing
#![feature(alloc_error_handler)]
#![feature(alloc_layout_extra)]
#![feature(allocator_api)]
#![feature(allocator_internals)]

$ LANG=en_GB.UTF-8 sort thing
#![feature(allocator_api)]
#![feature(allocator_internals)]
#![feature(alloc_error_handler)]
#![feature(alloc_layout_extra)]

rustfmt uses the first one for imports though, so let's follow that one.

[nagisa]

😔

[m-ou-se]

@bors r+

[bors]

⌛ Testing commit 9c9a0da with merge 400e7a1735c8350220fb96f51959d155c13eaabe...

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

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

[bors]

💔 Test failed - checks-actions

[nagisa]

Looks like a timeout in communication with the builders.

@bors retry

[bors]

⌛ Testing commit 9c9a0da with merge 6b354a1...

[bors]

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 6b354a1 to master...

[rust-highfive]

📣 Toolstate changed by #86034!

Tested on commit 6b354a1.
Direct link to PR: #86034

💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).

[pnkfelix]

While doing performance triage, I noticed a number of rustc benchmarks regressing their max-rss immediately after this PR: token-stream-stress by up to 10%, and many others in the 4% to 6% range.

@nagisa , Was this an expected outcome here? Do you have a hypothesis as to what might have caused it?

[nagisa]

I can't say it was expected. If I were to guess, it'd be because of different codegen unit distribution, perhaps.

[Mark-Simulacrum]

Yes, looking at the self profile results for the regressed benchmarks all have 1-2 additional LLVM modules being codegen'd. A simple fn main() {} generates some additional LLVM IR after this PR: https://gist.github.com/Mark-Simulacrum/896dfbf040073b9bffdffedc77a221b3 (generated without optimization, just --emit=llvm-ir).

Put up #88988 with a potential fix.

[Mark-Simulacrum]

Per results in #88988, that indeed seems to be sufficient to resolve the extra work this added. Spot checking a few benchmarks it removes the extra LLVM modules getting codegen'd so seems like mostly avoids the extra work introduced by this PR.

There's potential followup work here around removing some other extra bits that end up getting codegen'd into "empty main" binaries (e.g., the re-throwing of panics in __rust_begin_short_backtrace around the black_box), but those seem likely to have somewhat diminishing returns and are further work rather than direct fixes for a regression.

In the mean time, marking this as triaged. @rustbot label +perf-regression-triaged