Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

regression: compiler had non-unwinding abort #123286

Closed
Mark-Simulacrum opened this issue Mar 31, 2024 · 6 comments
Closed

regression: compiler had non-unwinding abort #123286

Mark-Simulacrum opened this issue Mar 31, 2024 · 6 comments
Assignees
@saethlin
Labels
A-proc-macros Area: Procedural macros I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team
Milestone
1.78.0

Comments

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Mar 31, 2024 •
edited
Loading 

[INFO] [stderr] thread caused non-unwinding panic. aborting.
[INFO] [stderr] error: could not compile `garbage_collector2` (bin "garbage_collector2")
[INFO] [stderr] 
[INFO] [stderr] Caused by:
[INFO] [stderr]   process didn't exit successfully: `/opt/rustwide/rustup-home/toolchains/beta-2024-03-30-x86_64-unknown-linux-gnu/bin/rustc --crate-name garbage_collector2 --edition=2021 src/main.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type bin --emit=dep-info,link -C embed-bitcode=no -C debuginfo=2 -C metadata=857ded4589b0897a -C extra-filename=-857ded4589b0897a --out-dir /opt/rustwide/target/debug/deps -L dependency=/opt/rustwide/target/debug/deps --extern anyhow=/opt/rustwide/target/debug/deps/libanyhow-4ba56e5ce709e35c.rlib --extern bidivec=/opt/rustwide/target/debug/deps/libbidivec-03555ce9cb9a7e42.rlib --extern console_error_panic_hook=/opt/rustwide/target/debug/deps/libconsole_error_panic_hook-0f6527b0603b2eb8.rlib --extern getrandom=/opt/rustwide/target/debug/deps/libgetrandom-b4d6e3ffd9dfac91.rlib --extern instant=/opt/rustwide/target/debug/deps/libinstant-11857eb893fd2c6b.rlib --extern laps=/opt/rustwide/target/debug/deps/liblaps-f7a69f6626767932.rlib --extern map_macro=/opt/rustwide/target/debug/deps/libmap_macro-71bc2494c68bef75.rlib --extern rand=/opt/rustwide/target/debug/deps/librand-0a6d206a676e4578.rlib --extern ron=/opt/rustwide/target/debug/deps/libron-3d88a0ce6119ff9c.rlib --extern serde=/opt/rustwide/target/debug/deps/libserde-e7a2b4f62285ba72.rlib --extern speedy2d=/opt/rustwide/target/debug/deps/libspeedy2d-93be5c445316c833.rlib --cap-lints=warn` (signal: 6, SIGABRT: process abort signal)
@Mark-Simulacrum Mark-Simulacrum added I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Mar 31, 2024
@Mark-Simulacrum Mark-Simulacrum added this to the 1.78.0 milestone Mar 31, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 31, 2024
@Mark-Simulacrum Mark-Simulacrum added the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label Mar 31, 2024
@Mark-Simulacrum Mark-Simulacrum mentioned this issue Mar 31, 2024
Closed
@saethlin saethlin self-assigned this Apr 1, 2024
@saethlin
Copy link
Member

saethlin commented Apr 1, 2024 •
edited
Loading

The regression in InfiniteCoder01/GarbageCollector2 is a failing precondition check in a library used in a proc macro:

#11 0x00007af93a1375c5 in core::panicking::panic_nounwind () at library/core/src/panicking.rs:215
#12 0x00007af93a258e38 in core::char::convert::from_u32_unchecked::precondition_check ()
   from /tmp/GarbageCollector2/target/debug/deps/liblaps_macros-ee19be92fda0179c.so
#13 0x00007af93a258e50 in core::char::methods::<impl char>::from_u32_unchecked ()
   from /tmp/GarbageCollector2/target/debug/deps/liblaps_macros-ee19be92fda0179c.so
#14 0x00007af93a25a140 in <char as laps_regex::mir::SymbolOp>::next ()
   from /tmp/GarbageCollector2/target/debug/deps/liblaps_macros-ee19be92fda0179c.so
#15 0x00007af93a1f93aa in laps_regex::mir::Mir<S,T>::symbol_set ()
   from /tmp/GarbageCollector2/target/debug/deps/liblaps_macros-ee19be92fda0179c.so
#16 0x00007af93a2022bb in laps_regex::mir::Mir<S,T>::optimize ()
   from /tmp/GarbageCollector2/target/debug/deps/liblaps_macros-ee19be92fda0179c.so
#17 0x00007af93a22a620 in laps_regex::re::RegexBuilder<T>::build_impl ()
   from /tmp/GarbageCollector2/target/debug/deps/liblaps_macros-ee19be92fda0179c.so
#18 0x00007af93a22b0bd in laps_regex::re::RegexBuilder<T>::build ()

The regression in regexp2rust_macro is a failing precondition check in wasmer:

#11 0x000070261e30d3a5 in core::panicking::panic_nounwind () at library/core/src/panicking.rs:215
#12 0x000070261ecb66ad in core::intrinsics::copy::precondition_check (src=0x70261cd5f000, dst=0x70261c64aaec, align=8)
    at /rustc/805813650248c1a2f6f271460d728d1bb852d2a7/library/core/src/ub_checks.rs:66
#13 0x000070261eca33c4 in core::intrinsics::copy<wasmer_vm::vmcontext::VMFunctionImport> (src=0x70261cd5f000, 
    dst=0x70261c64aaec, count=10) at /rustc/805813650248c1a2f6f271460d728d1bb852d2a7/library/core/src/intrinsics.rs:2975
#14 wasmer_vm::instance::InstanceHandle::new (allocator=..., module=..., context=0x70262c94d900, finished_functions=..., 
    finished_function_call_trampolines=..., finished_memories=..., finished_tables=..., finished_globals=..., imports=..., 
    vmshared_signatures=...) at src/instance/mod.rs:899
#15 0x000070261ec57d48 in wasmer_compiler::engine::artifact::Artifact::instantiate (self=0x70261c663810, tunables=..., 
    imports=..., context=0x70262c94d900) at src/engine/artifact.rs:376
#16 0x000070261e4738d2 in wasmer::sys::module::Module::instantiate<wasmer::sys::store::Store> (self=0x70262d3defe8, 
    store=0x70262d3defc0, imports=...)
    at /home/ben/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wasmer-3.0.2/src/sys/module.rs:361
#17 0x000070261e325516 in wasmer::sys::instance::Instance::new<wasmer::sys::store::Store> (store=0x70262d3defc0, 
    module=0x70262d3defe8, imports=0x70262d3df160)
    at /home/ben/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wasmer-3.0.2/src/sys/instance.rs:121
#18 0x000070261e31b62a in jsexec::runjs (source=...) at src/lib.rs:21
#19 0x000070261e3115e0 in regexp2rust_macro::run ()

At most this is asking for proc-macros to be able to report panic messages, but really these are just more of #123285. People were executing UB, we now prevent that and try to tell them about it.

@saethlin saethlin added A-proc-macros Area: Procedural macros T-opsem Relevant to the opsem team and removed E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Apr 1, 2024
@Mark-Simulacrum
Copy link
Member Author

Why doesn't the panic message get printed somewhere? That feels surprising to me.

@saethlin
Copy link
Member

saethlin commented Apr 1, 2024

proc-macros deliberately remove the default panic hook, and the panic hook is responsible for printing the panic message:

rust/library/proc_macro/src/bridge/client.rs

Lines 280 to 287 in 3d5528c

// Hide the default panic output within `proc_macro` expansions.
// NB. the server can't do this because it may use a different std.
static HIDE_PANICS_DURING_EXPANSION: Once = Once::new();
HIDE_PANICS_DURING_EXPANSION.call_once(|| {
let prev = panic::take_hook();
panic::set_hook(Box::new(move |info| {
if force_show_panics || !is_available() {
prev(info)

I think this is done because proc-macros want to use catch_unwind to do their own panic reporting, but of course that doesn't work on a non-unwinding panic. There's no unwind to catch, so the proc-macro just loses track of the PanicInfo entirely.

@saethlin
Copy link
Member

saethlin commented Apr 1, 2024

I'm working on a change to let the compiler report panics even when proc-macros are compiled with -Cpanic=abort. But I also noticed that we could change this:

         if force_show_panics || !is_available() { 
             prev(info)

To this: (UB-preventing panics always have can_unwind: false)

         if force_show_panics || !is_available() || !info.can_unwind() { 
             prev(info)

The experience isn't optimal; you're forced into a very verbose backtrace because the ICE setup uses set_var to configure RUST_BACKTRACE=full, so the context anyone actually wants is at the top of about 100 rustc frames. But at least the panic message and backtrace are printed.

I'm only suggesting this because it's such a tiny change that it's a plausible beta backport. @rustbot label +I-compiler-nominated

@rustbot rustbot added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Apr 1, 2024
@apiraino
Copy link
Contributor

T-compiler discussed the suggestion during the triage meeting (on zulip). Seems reasonable! thanks @saethlin

@rustbot label -I-compiler-nominated

@rustbot rustbot removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Apr 11, 2024
@saethlin saethlin mentioned this issue Apr 11, 2024
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 12, 2024
@matthiaskrgr
Rollup merge of rust-lang#123825 - saethlin:report-nounwind-panics, r…
8c86920 
…=petrochenkov

Call the panic hook for non-unwind panics in proc-macros

As I suggested in rust-lang#123286 (comment).

If a proc macro causes a non-unwinding panic, `proc_macro` isn't able to catch the unwind and report the panic as a compile error by passing control back to the compiler. Our only chance to produce any diagnostic is the panic hook, so we should call it.

This scenario has already existed, but has become a lot more interesting now that we're adding more UB-detecting panics to the standard library, and such panics do not unwind.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 12, 2024
@rust-timer
Unrolled build for rust-lang#123825
10b568e 
Rollup merge of rust-lang#123825 - saethlin:report-nounwind-panics, r=petrochenkov

Call the panic hook for non-unwind panics in proc-macros

As I suggested in rust-lang#123286 (comment).

If a proc macro causes a non-unwinding panic, `proc_macro` isn't able to catch the unwind and report the panic as a compile error by passing control back to the compiler. Our only chance to produce any diagnostic is the panic hook, so we should call it.

This scenario has already existed, but has become a lot more interesting now that we're adding more UB-detecting panics to the standard library, and such panics do not unwind.
@saethlin
Copy link
Member

Since #123825 was merged and backported I think we should close this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@saethlin saethlin

Labels
A-proc-macros Area: Procedural macros I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team
Projects
None yet
Milestone
1.78.0
Development

No branches or pull requests
4 participants
@Mark-Simulacrum @apiraino @saethlin @rustbot