LLVM 17 regression: tests/ui/backtrace.rs can't double-panic anymore

[durin42]

LLVM change 0d4a709 (https://reviews.llvm.org/D145210) caused LLVM to have enough smarts to figure out that double panics "can't" happen - we already adjusted some tests for this (eg e4a4064 where we fixed tests/codegen/vec-shrink-panik.rs) but this seems to be a bit of an actual regression, as the code asserts the double-panic happens, but instead the double panic fails like:

thread 'main' panicked at 'once', fake-test-src-base/backtrace.rs:33:5
stack backtrace:
   0: std::panicking::begin_panic
             at /rustc/FAKE_PREFIX/library/std/src/panicking.rs:611:12
   1: backtrace::double
             at fake-test-src-base/backtrace.rs:33:5
   2: backtrace::main
             at fake-test-src-base/backtrace.rs:127:9
   3: core::ops::function::FnOnce::call_once
             at /rustc/FAKE_PREFIX/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
fatal runtime error: failed to initiate panic, error 3

so it seems like maybe LLVM is incorrect about this double-panic not being reachable or something.

Filing this bug and disabling the test in our Rust/LLVM HEADs CI for now so we see other (more urgent) breakage reliably.

[clubby789]

@rustbot label +A-llvm

[jyn514]

so it seems like maybe LLVM is incorrect about this double-panic not being reachable or something.

this seems like a bug in LLVM itself? why did you file it against rust-lang/rust?

[durin42]

It certainly sounds like one, but the IR from the Rust test case is big enough that I feel bad just whining upstream, when I don't really understand what's going on.

Maybe @nikic has an idea, given they reviewed the upstream change?

[nikic]

Hm, the IR looks fine to me -- double() still has the check for double panics. Not sure why it doesn't work.

[nikic]

Looks like error code 3 is _URC_FATAL_PHASE1_ERROR.

[nikic]

rust_eh_personality returns _URC_CONTINUE_UNWIND for the backtrace::double frame and _URC_FATAL_PHASE1_ERROR for the bracktrace::main frame.

I think the problem here is that LLVM now realizes that double() can't unwind because it will always end up aborting due to double panic. This means that the invoke to double() in main() will be converted into a call and won't have an LSD entry. This then causes rust_eh_personality to signal an error.

This seems correct from an LLVM perspective, but not sure who/what is actually at fault here. Maybe @Amanieu knows.

[Amanieu]

_URC_FATAL_PHASE1_ERROR means that either:

• Exception handling metadata is invalid, and libgcc (which provides the unwind implementation on Linux) is complaining.
• The personality routine returned an error (anything except _URC_CONTINUE_UNWIND or _URC_HANDLER_FOUND).

Based on my reading of the personality function (library/std/src/peronality/gcc.rs), an error only occurs if exception handling metadata is invalid, or if there is an exception from a call site that doesn't have a corresponding entry in the LSDA call site table. This might happen if we are throwing an exception from a call site that LLVM assumes is noexcept.

Note that this is not something that LLVM should assume on its own: throwing an exception from a landing pad is well-defined and should propagate that exception up to the caller. If we (or LLVM) are something incorrectly marking a call as noexcept then this should be fixed.

Does this still reproduce if the test case is reduced to just the double() function? If so, could you post the IR and generated assembly code so I can have a look?

[nikic]

Hrm, just reducing it to double() makes it stop reproducing. Here's the smallest I was able to produce:

use std::env;
use std::process::Command;

#[inline(never)]
fn double() {
    struct Double;

    impl Drop for Double {
        fn drop(&mut self) { panic!("twice") }
    }

    let _d = Double;

    panic!("once");
}

fn main() {
    let args: Vec<String> = env::args().collect();
    if args.len() >= 2 && args[1] == "fail" {
    } else if args.len() >= 2 && args[1] == "double-fail" {
        double();
    } else {
        Command::new(&args[0]);
    }
}

The IR/asm for that file looks like this: https://gist.github.com/nikic/a3a4322442c8f63b507cbe5a4a32b0db

[nikic]

Based on my reading of the personality function (library/std/src/peronality/gcc.rs), an error only occurs if exception handling metadata is invalid, or if there is an exception from a call site that doesn't have a corresponding entry in the LSDA call site table. This might happen if we are throwing an exception from a call site that LLVM assumes is noexcept.

As I understand the problem, this is what happens. The call site is noexcept because the function is known to ultimately abort due to double panic.

Cleanup handlers get skipped during the phase 1 walk. Normally, we will always hit a catch handler eventually (in lang_start_internal if nothing else). In this case this doesn't happen because it is known that no exception can unwind that far up (once cleanup handlers have been executed).

[Amanieu]

Clang uses a catch handler for noexcept functions (https://godbolt.org/z/aYKGGdanq) which avoids this problem.

I remember we had a good reason for using cleanup instead of catch for TerminatorKind::Abort, but I can't remember what it was.

cc @nbdd0121

[nbdd0121]

We just don't have a reliable way to tell whether we'll be aborting or not, because TerminatorKind::Abort is a terminator that is part of cleanup blocks. I do plan to move away from cleanup (#104070, but to filter instead of catch, so our personality function can tell it apart from the try intrinsic), but that PR is currently blocked on having the MIR unwind refactor #102906 merged (which will make aborting action explicit).

[nbdd0121]

@nikic what will happen if you change the panic!("twice") to an abort()? Would LLVM still infer that double is indeed nounwind and thus omit the LSDA entry?

If so, then we have a bigger problem then just TerminatorKind::Abort.

[nikic]

I don't think that changing how we handle the double panic abort will fully fix, because we get the same behavior if we just explicitly abort inside the cleanup:

#[inline(never)]
fn double() {
    struct Double;

    impl Drop for Double {
        fn drop(&mut self) {
            std::process::abort();
        }
    }

    let _d = Double;

    panic!("once");
}

This wouldn't be affected by the proposed changes, right?