Guard against unwinding in cleanup code

[nbdd0121]

Currently the only safe guard we have against double unwind is the panic count (which is local to Rust). When double unwinds indeed happen (e.g. C++ exception + Rust panic, or two C++ exceptions), then the second unwind actually goes through and the first unwind is leaked. This can cause UB. cc rust-lang/project-ffi-unwind#6

E.g. given the following C++ code:

extern "C" void foo() {
    throw "A";
}

extern "C" void execute(void (*fn)()) {
    try {
        fn();
    } catch(...) {
    }
}

This program is well-defined to terminate:

struct dtor {
    ~dtor() noexcept(false) {
        foo();
    }
};

void a() {
    dtor a;
    dtor b;
}

int main() {
    execute(a);
    return 0;
}

But this Rust code doesn't catch the double unwind:

extern "C-unwind" {
    fn foo();
    fn execute(f: unsafe extern "C-unwind" fn());
}

struct Dtor;

impl Drop for Dtor {
    fn drop(&mut self) {
        unsafe { foo(); }
    }
}

extern "C-unwind" fn a() {
    let _a = Dtor;
    let _b = Dtor;
}

fn main() {
    unsafe { execute(a) };
}

To address this issue, this PR adds an unwind edge to an abort block, so that the Rust example aborts. This is similar to how clang guards against double unwind (except clang calls terminate per C++ spec and we abort).

The cost should be very small; it's an additional trap instruction (well, two for now, since we use TrapUnreachable, but that's a different issue) for each function with landing pads; if LLVM gains support to encode "abort/terminate" info directly in LSDA like GCC does, then it'll be free. It's an additional basic block though so compile time may be worse, so I'd like a perf run.

r? @ghost
@rustbot label: F-c_unwind

[Aaron1011]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 65202db0841627c4b93a331282a9e113a19233f4 with merge 7541ce347ff42c0458f27af1c6d1b57e97b9e96d...

[Amanieu]

In #92828 I am changing TerminatorKind::Abort to call the panic handler again to print a message instead of aborting directly with an obscure "Illegal Instruction" error. It would make sense to do the same here.

[nbdd0121]

I am a bit concerned about code bloat if this was a call to another function instead.

[fee1-dead]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 65202db0841627c4b93a331282a9e113a19233f4 with merge bfda8be488689971b86a2ad9297918630b80322b...

[bors]

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

[rust-timer]

Queued bfda8be488689971b86a2ad9297918630b80322b with parent b13a5bf, future comparison URL.

[rust-timer]

Finished benchmarking commit (bfda8be488689971b86a2ad9297918630b80322b): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

• Very large improvement in instruction counts (up to -6.5% on full builds of tokio-webpush-simple)
• Large regression in instruction counts (up to 3.4% on full builds of ripgrep)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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 led 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-review -S-waiting-on-perf +perf-regression

[bjorn3]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 42b41da with merge 34771111a4c6dac685e4f3443c622740bea51472...

[bors]

☀️ Try build successful - checks-actions
Build commit: 34771111a4c6dac685e4f3443c622740bea51472 (34771111a4c6dac685e4f3443c622740bea51472)

[rust-timer]

Queued 34771111a4c6dac685e4f3443c622740bea51472 with parent c26fbf8, future comparison URL.

[Amanieu]

@bors r+

[bors]

📌 Commit 7d683f5 has been approved by Amanieu

[bors]

⌛ Testing commit 7d683f5 with merge 2690468...

[bors]

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 2690468 to master...

[rust-timer]

Finished benchmarking commit (2690468): comparison url.

Summary: This benchmark run shows 19 relevant improvements 🎉 but 80 relevant regressions 😿 to instruction counts.

• Average relevant regression: 0.9%
• Average relevant improvement: -1.6%
• Largest improvement in instruction counts: -3.4% on incr-full builds of syn opt
• Largest regression in instruction counts: 3.7% on full builds of ripgrep opt

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

[rylev]

@nbdd0121 @Amanieu - this produces somewhat significant regressions in compiler performance. While this is a correctness fix, the situation it's guarding against (double unwind) seems like a relatively rare occurrence. Is the performance regression here justified?

[nbdd0121]

We've discussed about the perf regression briefly in https://rust-lang.zulipchat.com/#narrow/stream/210922-project-ffi-unwind/topic/weekly.20meeting, and our conclusion is that compile time regression can't be avoided because LLVM has more work to do. This is just compile time perf regression and it wouldn't affect runtime (this is cold path of cold path).

Although currently double unwinding can be be trigger via FFI unwinding, there are some discussions about allowing Rust code to perform double unwinding too - in that case this guard will be essential.

In the short term we might have to bear with this overhead in compile time, but if we made destructors nounwind by default then the overhead should go away.

[Amanieu]

In the short term we might have to bear with this overhead in compile time, but if we made destructors nounwind by default then the overhead should go away.

For more details see rust-lang/lang-team#97.

[riking]

Shouldn't this be ! grep unreachable ? -v inverts the match, so if the program writes any output grep reports success.

We want instead to search for "unreachable" and fail the build if it is found.

[nbdd0121]

$(CGREP) is not grep.

[RalfJung]

Given that this landed without t-lang involvement, I assume this guard is not a guarantee, it is just us being helpful? In particular, the Rust code in the OP is still UB, we just choose to not exploit that UB?

[RalfJung]

(Follow-up discussion happened on Zulip.)