Revert Break into the debugger on panic (129019)

[ChrisDenton]

This was talked about a bit at a recent libs meeting. While I think experimenting with this is worthwhile, I am nervous about this new behaviour reaching stable. We've already reverted on one tier 1 platform (Linux, #130810) which means we have differing semantics on different tier 1 platforms. Also the fact it triggers even when catch_unwind is used to catch the panic means it can be very noisy in some projects.

At the very least I think it could use some more discussion before being instantly stable. I think this could maybe be re-landed with an environment variable to control/override the behaviour. But that part would likely need a libs-api decision.

cc @workingjubilee @kromych

[rustbot]

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Noratrieb]

↩️

[bors]

📌 Commit d3e59a5 has been approved by Noratrieb

It is now in the queue for this repository.

[cuviper]

#129019 is headed for 1.83, so let's consider a backport:

@rustbot label beta-nominated

[cuviper]

Oh, nevermind, I must be in a time bubble -- beta is 1.82 right now.

[kromych]

This was talked about a bit at a recent libs meeting. While I think experimenting with this is worthwhile, I am nervous about this new behaviour reaching stable.

Appreciate taking great care of this! To make experimentation easier, I've published https://crates.io/crates/dbg_breakpoint that has all that there has been in the PR being reverted, and also some more.

[khuey]

fwiw (as an rr maintainer) I don't think breaking rr is necessarily a showstopper. rr is powerful enough that we could hide ourselves from the tracee (by faking the reads from /proc/<pid>/status and removing the TracerPid line) if this feature were really valuable. I do think that breaking strace is a showstopper ...

The one big question I have about this feature is why the interactive debugger needs the standard library to cooperate at all? Why can't it just set a breakpoint on __rust_start_panic? For example, catch throw is implemented in gdb for C++ by setting a breakpoint on __cxa_throw. Why won't the equivalent work here?

[kromych]

The one big question I have about this feature is why the interactive debugger needs the standard library to cooperate at all? Why can't it just set a breakpoint on __rust_start_panic? For example, catch throw is implemented in gdb for C++ by setting a breakpoint on __cxa_throw. Why won't the equivalent work here?

I believe it can. One needs to know the name of the panic function, and if it ever changes, that causes pain to the folks who do that. On one side there is a workflow of just starting the debugger and running the program and waiting for the panic, on the other side, knowing the name of the function and entering the command. The former seems as fewer steps or less setting up.

[Noratrieb]

Fewer steps, but less flexible and more fragile, as we're seeing. For example, if the standard library does it it's harder and weirder to turn off at runtime and as we're seeing, just not a decision the standard library is on a position to make. "debuggers" like strace really don't want this, and it's up to the debugger to decide if they want breakpoints or not.

[Noratrieb]

more specifically, I think the way to progress here is to document/stabilize the existence of a symbol suffix/substring¹ (__rust_start_panic or whatever) (I say suffix/substring specifically because the unmangled global symbols are problematic and we shouldn't guarantee the existence of one) and then make debuggers break on that by default. Whether debuggers want to implement that is up to them of course.
This way breakpointing happens in the right place (the debugger, not the standard library) and therefore sidesteps a lot of problems here.
strace would obviously not do anything, rr would do it in a way that works, etc.
It's more effort because it involves changing the debuggers but it's the right solution I think.

Footnotes

1. There is some symbol with the intention of this already but I forgot the name and I don't think it's documented stable ↩

[bors]

⌛ Testing commit d3e59a5 with merge 8d93c8d...

[khuey]

To be clear, I'm not saying "tell end users to break on __rust_start_panic", I'm saying teach gdb/lldb/Visual Studio/whatever that their "break on exception" knobs should translate to "break on __rust_start_panic" for Rust code. I believe all three of those debuggers have the capability to break on C++ exceptions.

Teaching the debuggers what to do instead of just SIGTRAPing also has the not-insignificant benefit that not every user of a debugger will be bombarded with panics regardless of their interest in them.

[rust-log-analyzer]

The job x86_64-msvc-ext failed! Check out the build log: (web) (plain)

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

    failures:
    
    ---- subtyping.md - Subtyping_and_Variance (line 26) stdout ----
    thread 'rustc' panicked at C:\a\rust\rust\compiler\rustc_codegen_ssa\src\base.rs:139:17:
    assertion `left == right` failed: NOP unsize vtable changed principal trait ref: for<'a> Fn(&'a i32) -> &i32 -> Fn(&i32) -> &i32
      left: Some(Binder { value: std::ops::Fn<(&i32,)>, bound_vars: [Region(BrAnon)] })
     right: Some(Binder { value: std::ops::Fn<(&i32,)>, bound_vars: [] })
       0:     0x7fffc2ba570e - <std::sys::backtrace::BacktraceLock::print::DisplayBacktrace as core::fmt::Display>::fmt::h195e23b22215e401
       1:     0x7fffc2bec98d - core::fmt::write::hfc773eccb0a97902
       2:     0x7fffc2b9bb11 - <std::io::IoSlice as core::fmt::Debug>::fmt::h724283129e24c78c
       3:     0x7fffc2ba5565 - std::sys::backtrace::lock::h159eb17c5b5323a5
       3:     0x7fffc2ba5565 - std::sys::backtrace::lock::h159eb17c5b5323a5
       4:     0x7fffc2ba8e26 - std::panicking::default_hook::h57d16b56db9b3ca5
       5:     0x7fffc2ba8b03 - std::panicking::default_hook::h57d16b56db9b3ca5
       6:     0x7fffbae78125 - RINvXs9_NtCsl6uhUqFnDm4_9hashbrown3setINtB6_7HashSetNtNtNtCsikYzPLV2Ve8_18rustc_query_system9dep_graph5graph12DepNodeIndexINtNtCsimXziBF6WLI_4core4hash18BuildHasherDefaultNtCsb7Nx8i8j1Nd_10rustc_hash8FxHasherEEINtNtNtNtB21_4iter6traits7collect6ExtendBO_E6e
       8:     0x7fffc2ba9499 - <std::panicking::begin_panic_handler::StaticStrPayload as core::fmt::Display>::fmt::h7083180d217c84d7
       9:     0x7fffc2ba60af - <std::sys::backtrace::BacktraceLock::print::DisplayBacktrace as core::fmt::Display>::fmt::h195e23b22215e401
      10:     0x7fffc2ba901e - rust_begin_unwind
      11:     0x7fffc2f8e641 - core::panicking::panic_fmt::h2f0e478414e6f2fc
      11:     0x7fffc2f8e641 - core::panicking::panic_fmt::h2f0e478414e6f2fc
      12:     0x7fffc2f8eb37 - core::panicking::assert_failed_inner::h8241f4bf374fbfaa
      13:     0x7fffc2c7caf7 - RINvNtCsimXziBF6WLI_4core9panicking13assert_failedINtNtB4_6option6OptionINtNtCscPc3GTN8RCV_13rustc_type_ir6binder6BinderNtNtNtCs5igMnZa9kXE_12rustc_middle2ty7context6TyCtxtINtNtB1d_9predicate19ExistentialTraitRefB1U_EEEBM_ECsl8lNb53GvKx_18rustc_codegen_llv
      14:     0x7fffbb4beae5 - rustc_codegen_ssa[6d86451db8c56afd]::base::unsize_ptr::<rustc_codegen_llvm[f62b3fccd9090843]::builder::Builder>
      15:     0x7fffbb4be265 - rustc_codegen_ssa[6d86451db8c56afd]::base::unsize_ptr::<rustc_codegen_llvm[f62b3fccd9090843]::builder::Builder>
      16:     0x7fffbb4a5439 - RINvXs_NtNtNtCsimXziBF6WLI_4core4iter8adapters6clonedINtB5_6ClonedINtNtNtBb_5slice4iter4IterINtCscH01iEiXtmH_8indexmap6BucketNtNtCsh7PM0AXQJal_5alloc6string6StringB1Z_EEENtNtNtB9_6traits8iterator8Iterator4folduNCINvNvB2I_8for_each4callB1s_NCINvMsi_NtB23_3v
      17:     0x7fffbb498596 - rustc_codegen_ssa[6d86451db8c56afd]::mir::codegen_mir::<rustc_codegen_llvm[f62b3fccd9090843]::builder::Builder>
      18:     0x7fffbb4c0994 - rustc_codegen_ssa[6d86451db8c56afd]::base::codegen_instance::<rustc_codegen_llvm[f62b3fccd9090843]::builder::Builder>
      19:     0x7fffbb327377 - <rustc_middle[3daba63accbd803c]::mir::mono::MonoItem as rustc_codegen_ssa[6d86451db8c56afd]::mono_item::MonoItemExt>::define::<rustc_codegen_llvm[f62b3fccd9090843]::builder::Builder>
      20:     0x7fffbb361242 - rustc_codegen_llvm[f62b3fccd9090843]::base::compile_codegen_unit
      21:     0x7fffbb35fd57 - rustc_codegen_llvm[f62b3fccd9090843]::base::compile_codegen_unit
      22:     0x7fffbb4bf939 - rustc_codegen_ssa[6d86451db8c56afd]::base::codegen_crate::<rustc_codegen_llvm[f62b3fccd9090843]::LlvmCodegenBackend>
      23:     0x7fffbb2f0508 - <rustc_codegen_llvm[f62b3fccd9090843]::LlvmCodegenBackend as rustc_codegen_ssa[6d86451db8c56afd]::traits::backend::CodegenBackend>::codegen_crate
      25:     0x7fffbb0bf5eb - <rustc_interface[929501187f762e2f]::queries::Linker>::codegen_and_build_linker
      25:     0x7fffbb0bf5eb - <rustc_interface[929501187f762e2f]::queries::Linker>::codegen_and_build_linker
      26:     0x7fffbad7c346 - RINvMsh_NtNtCs5igMnZa9kXE_12rustc_middle2ty7contextNtB6_10GlobalCtxt5enterNCNCNCNvCshZFjmIeFu3n_17rustc_driver_impl12run_compiler0s_0s4_0INtNtCsimXziBF6WLI_4core6result6ResultINtNtB2g_6option6OptionNtNtCscAfFLKbRsdf_15rustc_interface7queries6LinkerENtCs2qc
      27:     0x7fffbae637c9 - RINvMs1_NtCscAfFLKbRsdf_15rustc_interface7queriesINtB6_11QueryResultRNtNtNtCs5igMnZa9kXE_12rustc_middle2ty7context10GlobalCtxtE5enterINtNtCsimXziBF6WLI_4core6result6ResultINtNtB2c_6option6OptionNtB6_6LinkerENtCs2qcV2bIPlyf_10rustc_span15ErrorGuaranteedENCN
      28:     0x7fffbadf7a2d - RINvMs4_NtCscAfFLKbRsdf_15rustc_interface7queriesNtNtB8_9interface8Compiler5enterNCNCNvCshZFjmIeFu3n_17rustc_driver_impl12run_compiler0s_0INtNtCsimXziBF6WLI_4core6result6ResultINtNtB2h_6option6OptionNtB6_6LinkerENtCs2qcV2bIPlyf_10rustc_span15ErrorGuarantee
      29:     0x7fffbae202b1 - RINvMs_Cs4fJrD3BwdRP_10scoped_tlsINtB5_9ScopedKeyNtCs2qcV2bIPlyf_10rustc_span14SessionGlobalsE3setNCNCNCINvNtCscAfFLKbRsdf_15rustc_interface4util26run_in_thread_with_globalsNCINvB1H_31run_in_thread_pool_with_globalsNCINvNtB1J_9interface12run_compilerINtNtC
      30:     0x7fffbad8aa6a - RINvCs2qcV2bIPlyf_10rustc_span27create_session_globals_thenINtNtCsimXziBF6WLI_4core6result6ResultuNtB2_15ErrorGuaranteedENCNCNCINvNtCscAfFLKbRsdf_15rustc_interface4util26run_in_thread_with_globalsNCINvB24_31run_in_thread_pool_with_globalsNCINvNtB26_9interf
      31:     0x7fffbad73ebb - RINvNtNtCs739BlfsWpLm_3std3sys9backtrace28___rust_begin_short_backtraceNCNCINvNtCscAfFLKbRsdf_15rustc_interface4util26run_in_thread_with_globalsNCINvB1e_31run_in_thread_pool_with_globalsNCINvNtB1g_9interface12run_compilerINtNtCsimXziBF6WLI_4core6result6Res
      32:     0x7fffbae2e9f7 - <std[522042d3ef23ccae]::hash::random::RandomState as core[d5f87d18b0881838]::hash::BuildHasher>::hash_one::<&(intl_pluralrules[212dda65410eaf10]::PluralRuleType,)>
      34:     0x7fffffc14cb0 - BaseThreadInitThunk
      35:     0x7ff8012becdb - RtlUserThreadStart
    
    error: the compiler unexpectedly panicked. this is a bug.
    error: the compiler unexpectedly panicked. this is a bug.
    
    note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md
    
    note: please make sure that you have updated to the latest nightly
    
    note: please attach the file at `C:\a\_temp\msys64\tmp\mdbook-oTTTub\rustc-ice-2024-09-26T07_13_05-3040.txt` to your bug report
    query stack during panic:
    end of query stack
    Couldn't compile the test.
    
---
[RUSTC-TIMING] miri test:false 5.406
error: failed to remove file `C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1-tools\x86_64-pc-windows-msvc\release\miri.exe`

Caused by:
  Access is denied. (os error 5)
Command has failed. Rerun with -v to see more details.
  local time: Thu, Sep 26, 2024  7:28:47 AM
  network time: Thu, 26 Sep 2024 07:28:47 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

[bors]

💔 Test failed - checks-actions

[ChrisDenton]

@bors retry spurious "Access is denied" on x86_64-msvc-ext

[kromych]

Until __rust_start_panic is guaranteed to preserve its name and meaning, that'd be another can of worms to change the debuggers.

As there is no solution presently that satisfies everyone, a separate crate is where this belongs rather than in std. The only obstacle would be the trap instruction intrinsic available in nightly only, yet that is easily solvable with some elbow grease. These considerations led to the crate above.

[RalfJung]

Until __rust_start_panic is guaranteed to preserve its name and meaning, that'd be another can of worms to change the debuggers.

That seems like a pretty actionable next step then -- figure out if __rust_start_panic can be made a guaranteed symbol name, have that blessed by t-libs-api, and add suitable comments in the sources.

[bjorn3]

I think we could guarantee that the symbol has that substring, but I don't think we can guarantee that that exact symbol will be used without multiple standard library versions in a single binary conflicting with each other. For example #127173 will mangle the name, but keep it as substring. Also we should probably provide guarantees about rust_panic, not __rust_start_panic. rust_panic is documented as being intended to put breakpoints on:

rust/library/std/src/panicking.rs

Lines 856 to 861 in f2becdf

	/// An unmangled function (through `rustc_std_internal_symbol`) on which to slap
	/// yer breakpoints.
	#[inline(never)]
	#[cfg_attr(not(test), rustc_std_internal_symbol)]
	#[cfg(not(feature = "panic_immediate_abort"))]
	fn rust_panic(msg: &mut dyn PanicPayload) -> ! {

[GrigorenkoPV]

Until __rust_start_panic is guaranteed to preserve its name and meaning, that'd be another can of worms to change the debuggers.

That seems like a pretty actionable next step then -- figure out if __rust_start_panic can be made a guaranteed symbol name, have that blessed by t-libs-api, and add suitable comments in the sources.

A bit of a silly idea, but what if we just provide something like

#[inline(never)]
#[unsafe(no_mangle)]
fn __rust_panic_breakpoint() {}

? An empty function that is called from the actual rust_panic implementations and serves as a place "on which to slap yer breakpoints". This way we are free to change rust_panic however we see fit, as long as it calls __rust_panic_breakpoint at some point. And it is quite easy to preserve __rust_panic_breakpoint the way it is, since it literally does nothing.

Or, maybe, if we want some extensibility, we could pass a pointer to it,

#[inline(never)]
#[unsafe(no_mangle)]
extern "C" fn __rust_panic_breakpoint(_payload: *const core::ffi::c_void) {}

which, for now, will always be null, but in future can point to some payload with valuable info (presumably this payload should start with a version number for its format).

I think this way there will be no conflicts between the std version.

[bjorn3]

I think this way there will be no conflicts between the std version.

Linkers tend to not like multiple definitions of a symbol even if they are identical.

[cuviper]

Isn't that what COMDAT is for?

[RalfJung]

Linkers tend to not like multiple definitions of a symbol even if they are identical.

How does C++ deal with that?

[bjorn3]

COMDAT or weak symbols I think. Rustc doesn't currently support COMDAT though.

[workingjubilee]

Violating the One Definition Rule leaves a program ill-formed, "no diagnostic required".

Or: Declare it UB.

[workingjubilee]

C++ implementations try to use those, yes.

[GrigorenkoPV]

Violating the One Definition Rule leaves a program ill-formed, "no diagnostic required".

Or: Declare it UB.

Not if you use the "inline" keyword https://en.cppreference.com/w/cpp/language/inline

There may be more than one definition of an inline function or variable in the program as long as each definition appears in a different translation unit and […] all definitions are identical.

[bjorn3]

Using inline will prevent the symbol from getting exported from the object file. I guess marking rust_panic as internal to the object file would work.

[workingjubilee]

Yeah this is quite specifically "we want the symbol in the object file so the debugger can find it" territory.

[ChrisDenton]

There are some really great ideas here but it might be better to move this to a new issue if someone would be willing to summarise. Or an API Change Proposal if someone wants to make a concrete proposal.

It'd be shame if this were left buried in a closed PR.