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

[AIX] Add crate "unwind" to link with libunwind #132905

Merged
merged 1 commit into from
Nov 15, 2024
Merged

[AIX] Add crate "unwind" to link with libunwind #132905

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 5 additions & 9 deletions tests/ui/extern-flag/auxiliary/panic_handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,10 @@
#![feature(lang_items)]
#![feature(lang_items, panic_unwind)]
#![no_std]

// Since `rustc` generally passes `-nodefaultlibs` to the linker,
// Rust programs link necessary system libraries via `#[link()]`
// attributes in the `libc` crate. `libc` is a dependency of `std`,
// but as we are `#![no_std]`, we need to include it manually.
// Except on windows-msvc.
#![feature(rustc_private)]
#[cfg(not(all(windows, target_env = "msvc")))]
extern crate libc;
// Since the `unwind` crate is a dependency of the `std` crate, and we have
// `#![no_std]`, the unwinder is not included in the link command by default.
// We need to include crate `unwind` manually.
extern crate unwind;

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could unconditionally link against the unwind crate and remove usage of the libc crate?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if that will work for wasm or some other panic=abort targets though.

Copy link
Contributor Author

@xingxue-ibm xingxue-ibm Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know if that will work for wasm or some other panic=abort targets though.

Yeah, unconditionally including the panic_unwind feature failed on CI target x86_64-gnu-llvm-18 with error "unknown feature 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.

The reason CI failed before the last commit was because you enabled the panic_unwind feature unconditionally without including the unwind crate unconditionally. The panic_unwind crate is what defines the unwind feature. If you remove the #[cfg(target_os = "aix")] on extern crate unwind;, enabling the panic_unwind feature outside of AIX should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see. Should we try to include the panic_unwind feature and specify the unwind crate both unconditionally and see how CI goes?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Will probably need a couple of try builds for coverage of panic=abort targets. And if it doesn't work on panic=abort targets, maybe adding //@ needs-unwind to the tests that use this auxiliary file would be acceptable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that makes sense, thanks!

#[panic_handler]
pub fn begin_panic_handler(_info: &core::panic::PanicInfo<'_>) -> ! {
Expand Down
Loading