Skip to content

Add __CxxFrameHandler3 in panic_abort #83607

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

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions library/panic_abort/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,19 @@ pub mod personalities {
)))]
pub extern "C" fn rust_eh_personality() {}

// On *-pc-windows-msvc we need such a symbol to make linker happy.
#[allow(non_snake_case)]
#[no_mangle]
#[cfg(all(target_os = "windows", target_env = "msvc"))]
pub extern "C" fn __CxxFrameHandler3(
_record: usize,
_frame: usize,
_context: usize,
_dispatcher: usize,
) -> u32 {
1
}

// On x86_64-pc-windows-gnu we use our own personality function that needs
// to return `ExceptionContinueSearch` as we're passing on all our frames.
#[rustc_std_internal_symbol]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// run-fail
// compile-flags: -C panic=abort -C target-feature=+crt-static
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly confused about -C target-feature=+crt-static. We're statically linking the crt but also dynamically linking libcmt. Shouldn't we be testing on or the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libcmt is the static C runtime library. It is a static library, though it should be linked dynamically. I just cannot tell why it needs dynamic linking. It should make no difference between dynamic linking and static linking.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+crt-static will already link libcmt so there should be no reason to declare it manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right when using libc crate, but here we need to test under no_std.

// aux-build:exit-success-if-unwind-msvc-no-std.rs
// only-msvc
// Test that `no_std` with `panic=abort` under MSVC toolchain
// doesn't cause error when linking to libcmt.
// We don't run this executable because it will hang in `rust_begin_unwind`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we expect that this should actually run successfully? If the MSCRT is linked in statically, then we should be able to handle linking another crate which is compiled with -Cpanic=unwind, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first, I expect it fail because panic! is in a panic=abort crate, and unwind won't occur; but it hangs in rust_begin_unwind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I solved this problem now. It should be able to run and fail.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should be changed then.


#![no_std]
#![no_main]

extern crate exit_success_if_unwind_msvc_no_std;

#[link(name = "libcmt")]
extern "C" {}

#[no_mangle]
pub extern "C" fn main() -> i32 {
exit_success_if_unwind_msvc_no_std::bar(do_panic);
0
}

fn do_panic() {
panic!();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// run-fail
// compile-flags: -C panic=abort
// aux-build:exit-success-if-unwind-msvc-no-std.rs
// only-msvc
// Test that `no_std` with `panic=abort` under MSVC toolchain
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also expect this to run successfully (even though it doesn't currently)? Since MSCRT is linked in?

// doesn't cause error when linking to msvcrt.
// We don't run this executable because it will hang in `rust_begin_unwind`

#![no_std]
#![no_main]

extern crate exit_success_if_unwind_msvc_no_std;

#[link(name = "msvcrt")]
extern "C" {}

#[no_mangle]
pub extern "C" fn main() -> i32 {
exit_success_if_unwind_msvc_no_std::bar(do_panic);
0
}

fn do_panic() {
panic!();
}
52 changes: 52 additions & 0 deletions src/test/ui/panic-runtime/abort-link-to-unwind-msvc-no-std.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// run-fail
// compile-flags:-C panic=abort
// aux-build:exit-success-if-unwind-msvc-no-std.rs
// no-prefer-dynamic
// only-msvc
// We don't run this executable because it will hang in `rust_begin_unwind`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is wrong now


#![no_std]
#![no_main]
#![windows_subsystem = "console"]
#![feature(panic_abort)]

extern crate exit_success_if_unwind_msvc_no_std;
extern crate panic_abort;

#[no_mangle]
pub unsafe extern "C" fn memcpy(dest: *mut u8, _src: *const u8, _n: usize) -> *mut u8 {
dest
}

#[no_mangle]
pub unsafe extern "C" fn memmove(dest: *mut u8, _src: *const u8, _n: usize) -> *mut u8 {
dest
}

#[no_mangle]
pub unsafe extern "C" fn memset(mem: *mut u8, _val: i32, _n: usize) -> *mut u8 {
mem
}

#[no_mangle]
pub unsafe extern "C" fn memcmp(_mem1: *const u8, _mem2: *const u8, _n: usize) -> i32 {
0
}

// Used by compiler_builtins
#[no_mangle]
#[used]
static _fltused: i32 = 0;

// MSVC always assumes that some functions are avaliable
#[link(name = "ntdll")]
extern "system" {}

#[no_mangle]
pub extern "C" fn mainCRTStartup() {
exit_success_if_unwind_msvc_no_std::bar(main);
}

fn main() {
panic!();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// compile-flags:-C panic=unwind
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to add a comment explaining what this helper crate does at a high level.

// no-prefer-dynamic

#![no_std]
#![crate_type = "rlib"]
#![feature(core_intrinsics)]

struct Bomb;

impl Drop for Bomb {
fn drop(&mut self) {
#[link(name = "kernel32")]
extern "system" {
fn ExitProcess(code: u32) -> !;
}
unsafe {
ExitProcess(0);
}
}
}

pub fn bar(f: fn()) {
let _bomb = Bomb;
f();
}

use core::panic::PanicInfo;

#[panic_handler]
fn handle_panic(_: &PanicInfo) -> ! {
core::intrinsics::abort();
}
47 changes: 47 additions & 0 deletions src/test/ui/panic-runtime/unwind-msvc-no-std-link-to-libcmt.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// build-pass
// compile-flags: -C panic=unwind -C target-feature=+crt-static
// only-msvc
// Test that `no_std` with `panic=unwind` under MSVC toolchain
// doesn't cause error when linking to libcmt.

#![no_std]
#![no_main]
#![feature(alloc_error_handler)]
#![feature(panic_unwind)]

use core::alloc::{GlobalAlloc, Layout};

struct DummyAllocator;

unsafe impl GlobalAlloc for DummyAllocator {
unsafe fn alloc(&self, _layout: Layout) -> *mut u8 {
core::ptr::null_mut()
}

unsafe fn dealloc(&self, _ptr: *mut u8, _layout: Layout) {}
}

#[global_allocator]
static ALLOC: DummyAllocator = DummyAllocator;

#[alloc_error_handler]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for bringing in alloc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

panic_unwind needs that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we're using the panic_unwind feature and not #[panic_handler] with an eh_personality item? This would remove the need for the panic_unwind and alloc_error_handler features. I believe that's the more common and less verbose way to handle panics in a custom way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm only testing a no_std crate with panic_unwind feature. Do you mean that panic_unwind doesn't need a panic_handler?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'm asking if testing the panic_unwind feature is the right test here. We want to test custom panic handling, and the panic_unwind feature is one way to do that. Another is simply adding a panic_handler and eh_personality. Perhaps we should add another test for that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean adding a test with panic=unwind feature and a custom panic handler? It seems somehow complicated to write a robust unwind panic handler...

fn rust_oom(_layout: Layout) -> ! {
panic!()
}

extern crate panic_unwind;

use core::panic::PanicInfo;

#[panic_handler]
fn handle_panic(_: &PanicInfo) -> ! {
loop {}
}

#[link(name = "libcmt")]
extern "C" {}

#[no_mangle]
pub extern "C" fn main() -> i32 {
panic!();
}
47 changes: 47 additions & 0 deletions src/test/ui/panic-runtime/unwind-msvc-no-std-link-to-msvcrt.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// build-pass
// compile-flags: -C panic=unwind
// only-msvc
// Test that `no_std` with `panic=unwind` under MSVC toolchain
// doesn't cause error when linking to msvcrt.

#![no_std]
#![no_main]
#![feature(alloc_error_handler)]
#![feature(panic_unwind)]

use core::alloc::{GlobalAlloc, Layout};

struct DummyAllocator;

unsafe impl GlobalAlloc for DummyAllocator {
unsafe fn alloc(&self, _layout: Layout) -> *mut u8 {
core::ptr::null_mut()
}

unsafe fn dealloc(&self, _ptr: *mut u8, _layout: Layout) {}
}

#[global_allocator]
static ALLOC: DummyAllocator = DummyAllocator;

#[alloc_error_handler]
fn rust_oom(_layout: Layout) -> ! {
panic!()
}

extern crate panic_unwind;

use core::panic::PanicInfo;

#[panic_handler]
fn handle_panic(_: &PanicInfo) -> ! {
loop {}
}

#[link(name = "msvcrt")]
extern "C" {}

#[no_mangle]
pub extern "C" fn main() -> i32 {
panic!();
}