Prevent __rust_begin_short_backtrace frames from being tail-call optimised away

[eggyal]

I've stumbled across some situations where there (unexpectedly) was no __rust_begin_short_backtrace frame on the stack during unwinding.

On closer examination, it appeared that the calls to that function had been tail-call optimised away.

This PR follows @bjorn3's suggestion on Zulip, by adding calls to black_box that hint to rustc not to perform TCO.

Fixes #47429

[rust-highfive]

r? @dtolnay

(rust_highfive has picked a reviewer for you, use r? to override)

[Mark-Simulacrum]

Do we have evidence this actually fixes things? I would prefer to see this land alongside a UI test or so that actually verifies that we're seeing the behavior we expect here.

Alternatively, it may be that the right fix isn't in removing TCO from the short backtrace function but reworking how we determine where to start/stop short backtraces.

[eggyal]

It has certainly fixed the lack of __rust_begin_short_backtrace frame that I was running up against—but that was in a custom unwind routine so perhaps not the easiest way to test. Just thinking aloud: what would be an appropriate UI test? Checking that the output when panicking with RUST_BACKTRACE=1 stops before catch_unwind?

[eggyal]

An idea for an alternative approach could be to store the stack pointer somewhere (thread-local?), and compare against that when unwinding.

[Mark-Simulacrum]

For testing I think the best thing is probably to use a unit test in library/std/src/backtrace.rs that asserts a given number of frames and a start/stop frame when getting a short backtrace. We might need to extract the actual formatting invocation to do so, though, into a separate function not tied to panicking in particular.

I don't think we need thread locals to get good results. I experimented a bit locally, and was able to get things down to this:

thread 'main' panicked at 'Test', /home/mark/Build/rust/library/std/src/panicking.rs:457:65
stack backtrace:
   0: std::panicking::begin_panic
             at ./Build/rust/library/std/src/panicking.rs:456
   1: t::bar
             at ./t.rs:6
   2: t::main
             at ./t.rs:2
   3: core::ops::function::FnOnce::call_once
             at ./Build/rust/library/core/src/ops/function.rs:233
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

And with nightly:

thread 'main' panicked at 'Test', t.rs:6:5
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.46/src/backtrace/libunwind.rs:86
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.46/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at library/std/src/sys_common/backtrace.rs:78
   3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
             at library/std/src/sys_common/backtrace.rs:59
   4: core::fmt::write
             at library/core/src/fmt/mod.rs:1117
   5: std::io::Write::write_fmt
             at library/std/src/io/mod.rs:1510
   6: std::sys_common::backtrace::_print
             at library/std/src/sys_common/backtrace.rs:62
   7: std::sys_common::backtrace::print
             at library/std/src/sys_common/backtrace.rs:49
   8: std::panicking::default_hook::{{closure}}
             at library/std/src/panicking.rs:198
   9: std::panicking::default_hook
             at library/std/src/panicking.rs:217
  10: std::panicking::rust_panic_with_hook
             at library/std/src/panicking.rs:526
  11: std::panicking::begin_panic
             at /rustc/cfc572cae2d1fc381cce476b5c787fd7221af98c/library/std/src/panicking.rs:456
  12: t::bar
             at ./t.rs:6
  13: t::main
             at ./t.rs:2
  14: std::rt::lang_start::{{closure}}
             at /rustc/cfc572cae2d1fc381cce476b5c787fd7221af98c/library/std/src/rt.rs:67
  15: std::rt::lang_start_internal::{{closure}}
             at library/std/src/rt.rs:52
  16: std::panicking::try::do_call
             at library/std/src/panicking.rs:348
  17: std::panicking::try
             at library/std/src/panicking.rs:325
  18: std::panic::catch_unwind
             at library/std/src/panic.rs:394
  19: std::rt::lang_start_internal
             at library/std/src/rt.rs:51
  20: std::rt::lang_start
             at /rustc/cfc572cae2d1fc381cce476b5c787fd7221af98c/library/std/src/rt.rs:67
  21: main
  22: __libc_start_main
  23: _start
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

[erikdesjardins]

This likely fixes #47429

[Mark-Simulacrum]

I think we should try to add a UI test for this. My guess is we can do that with a run-fail test, like this one https://github.com/rust-lang/rust/blob/master/src/test/ui/test-panic-abort.rs

[eggyal]

I think we should try to add a UI test for this. My guess is we can do that with a run-fail test, like this one https://github.com/rust-lang/rust/blob/master/src/test/ui/test-panic-abort.rs

Agreed, I'll add one now.

[Mark-Simulacrum]

This looks great. Could you squash the commits into just the one to clean up git history a bit?

I suspect that this PR might cause problems if there's cases or platforms where our mini-functions get elided regardless of the black box, but we can iron out the kinks in nightly -- I don't think we'll find them by just looking in this PR, and it's small enough that I am comfortable just landing it.

r=me with commits squashed

[eggyal]

Sure, force-pushed a squash. Had thought bors would do that when merging.

[Mark-Simulacrum]

@bors r+ rollup=iffy -- not sure if the added test will have the exact same output on other platforms

[bors]

📌 Commit 18d4ba34907e7d0b9fc142a9d9a9c5bbdae4279f has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit 18d4ba34907e7d0b9fc142a9d9a9c5bbdae4279f with merge cca2c20c8b6cd0a71346bf7f85feb3c6291bd60d...

[bors]

💔 Test failed - checks-actions

[bjorn3]

spurious network error

[rust-log-analyzer]

The job i686-gnu of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

[Mark-Simulacrum]

@bors retry

[bors]

⌛ Testing commit 18d4ba34907e7d0b9fc142a9d9a9c5bbdae4279f with merge c70a4d6c76d010b569fa4d16b2ffb7eced04bc52...

[eggyal]

@Mark-Simulacrum could we give this another crack please? I spotted an error before bors got a chance to retry.

[Mark-Simulacrum]

@bors r+

[bors]

📌 Commit 5792840 has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit 5792840 with merge f3a9de9...

[bors]

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing f3a9de9 to master...

[rust-highfive]

📣 Toolstate changed by #75048!

Tested on commit f3a9de9.
Direct link to PR: #75048

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

[ecstatic-morse]

This PR resulted in moderate regressions in instruction counts. The increase is mostly concentrated in very small benchmarks with little generated code, and seems to be caused by some additional codegen query invocations.

Is there any way to mitigate this? This is more impactful for superficial benchmarks than for actual users, so I don't think it necessitates a revert if the regressions are unavoidable.

[Mark-Simulacrum]

My guess is no -- we're probably just paying the cost of having one more function to codegen. I suspect any fix (e.g., trying to locate the inlined call frame manually or so) is going to be more expensive than the current tiny regressions.

[eggyal]

We could limit the effect of this PR to debug builds only? Enforcing short backtraces in release is unlikely to be a requirement?

[bjorn3]

Libstd is always compiled without debug assertions. The optimization level of your program doesn't matter. You are always using the same compiled libstd.