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.

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

Use C-unwind ABI for __rust_start_panic in panic_abort #87787

Merged
merged 1 commit into from Aug 6, 2021
Merged

Use C-unwind ABI for __rust_start_panic in panic_abort #87787

merged 1 commit into from Aug 6, 2021

Conversation

ghost
Copy link

@ghost ghost commented Aug 5, 2021

The function originally has C ABI but is called using C-unwind ABI in std:

extern "C-unwind" {
/// `payload` is passed through another layer of raw pointers as `&mut dyn Trait` is not
/// FFI-safe. `BoxMeUp` lazily performs allocation only when needed (this avoids allocations
/// when using the "abort" panic runtime).
fn __rust_start_panic(payload: *mut &mut dyn BoxMeUp) -> u32;
}

Which might be problematic and triggers this Miri error:

error: Undefined Behavior: calling a function with ABI C using caller ABI C-unwind
   --> /home/hyd-dev/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/panicking.rs:672:9
    |
672 |         __rust_start_panic(obj)
    |         ^^^^^^^^^^^^^^^^^^^^^^^ calling a function with ABI C using caller ABI C-unwind
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information

Changing the ABI of the function to C-unwind fixes the error above.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 5, 2021
@RalfJung
Copy link
Member

RalfJung commented Aug 6, 2021

This sounds like Miri could accept it (though I am not sure of that), but I also don't think there is any reason to use a different ABI string on the two sides here. Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 6, 2021

📌 Commit 7520ea9 has been approved by RalfJung

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 6, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 6, 2021
Rollup of 7 pull requests

Successful merges:

 - rust-lang#85807 (bootstrap: Disable initial-exec TLS model on powerpc)
 - rust-lang#87761 (Fix overflow in rustc happening if the `err_count()` is reduced in a stage.)
 - rust-lang#87775 (Add hint for unresolved associated trait items if the trait has a single item)
 - rust-lang#87779 (Remove special case for statement `NodeId` assignment)
 - rust-lang#87787 (Use `C-unwind` ABI for `__rust_start_panic` in `panic_abort`)
 - rust-lang#87809 (Fix typo in the ptr documentation)
 - rust-lang#87816 (Sync rustc_codegen_cranelift)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c5202b3 into rust-lang:master Aug 6, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 6, 2021
@ghost ghost deleted the c-unwind branch August 12, 2021 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants