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

[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

merged 1 commit into from
Nov 15, 2024

Conversation

xingxue-ibm
Copy link
Contributor

@xingxue-ibm xingxue-ibm commented Nov 11, 2024 •
edited by bjorn3
Loading

The Rust on IBM AIX uses LLVM's libunwind. Since crate unwind is a dependency of crate std and #![no_std] is specified in the test case, libunwind is not included in the link command by default. As a result, the test case fails to link with the error "Undefined symbol: ._Unwind_Resume" on AIX. This PR explicitly adds crate unwind for AIX, along with feature panic_unwind, which is required to include the unwind crate.

@rustbot
Copy link
Collaborator

rustbot commented Nov 11, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cjgillot (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 11, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@xingxue-ibm
Copy link
Contributor Author

r? @bjorn3

@rustbot rustbot assigned bjorn3 and unassigned cjgillot Nov 12, 2024
bjorn3
bjorn3 reviewed Nov 12, 2024
View reviewed changes
tests/ui/extern-flag/auxiliary/panic_handler.rs
// `#![no_std]`, `libunwind` is not included in the link command on AIX
// by default. We need to include crate `unwind` manually.
#[cfg(target_os = "aix")]
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!

@bjorn3
Copy link
Member

bjorn3 commented Nov 14, 2024

@bors try

@bjorn3
Copy link
Member

bjorn3 commented Nov 14, 2024

@bors ping

@bors
Copy link
Contributor

bors commented Nov 14, 2024

😪 I'm awake I'm awake

@bors
Copy link
Contributor

bors commented Nov 14, 2024

⌛ Trying commit fc1c3d9 with merge 30a3068...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 14, 2024
@bors
Auto merge of rust-lang#132905 - xingxue-ibm:link-unwind, r=<try>
30a3068 
[AIX] Add crate "unwind" to link with libunwind

The Rust on IBM AIX uses LLVM's `libunwind`. Since crate `unwind` is a dependency of crate `std` and `#![no_std]` is specified in the test case, `libunwind` is not included in the link command by default. As a result, the test case fails to link with the error "Undefined symbol: ._Unwind_Resume" on AIX. This PR explicitly adds crate `unwind` for AIX, along with feature `panic_unwind`, which is required to include the `unwind` crate.

try-job: test-various
@bors
Copy link
Contributor

bors commented Nov 14, 2024

☀️ Try build successful - checks-actions
Build commit: 30a3068 (30a3068b755de413b034ef554317273e2df7e3a1)

bjorn3
bjorn3 approved these changes Nov 14, 2024
View reviewed changes
Copy link
Member

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

LGTM and tests pass for wasm at least which is a panic=abort target. Please squash your changes and I will r+.

@xingxue-ibm
Copy link
Contributor Author

LGTM and tests pass for wasm at least which is a panic=abort target. Please squash your changes and I will r+.

Done. Thanks so much, @bjorn3!

@bjorn3
Copy link
Member

bjorn3 commented Nov 14, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Nov 14, 2024

📌 Commit 467ce26 has been approved by bjorn3

It is now in the queue for this repository.

@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 Nov 14, 2024
@workingjubilee workingjubilee mentioned this pull request Nov 15, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 15, 2024
@bors
Auto merge of rust-lang#133059 - workingjubilee:rollup-rc5kvr1, r=wor…
251dc8a 
…kingjubilee

Rollup of 8 pull requests

Successful merges:

 - rust-lang#132790 (Add as_slice/into_slice for IoSlice/IoSliceMut.)
 - rust-lang#132905 ([AIX] Add crate "unwind" to link with libunwind)
 - rust-lang#132977 (Fix compilation error on Solaris due to flock usage)
 - rust-lang#132984 ([illumos] use pipe2 to create anonymous pipes)
 - rust-lang#133019 (docs: Fix missing period and colon in methods for primitive types)
 - rust-lang#133048 (use `&raw` in `{read, write}_unaligned` documentation)
 - rust-lang#133050 (Always inline functions signatures containing `f16` or `f128`)
 - rust-lang#133053 (tests: Fix the SIMD FFI tests with certain x86 configuration)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fcc0845 into rust-lang:master Nov 15, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 15, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 15, 2024
@rust-timer
Unrolled build for rust-lang#132905
cfc257d 
Rollup merge of rust-lang#132905 - xingxue-ibm:link-unwind, r=bjorn3

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

The Rust on IBM AIX uses LLVM's `libunwind`. Since crate `unwind` is a dependency of crate `std` and `#![no_std]` is specified in the test case, `libunwind` is not included in the link command by default. As a result, the test case fails to link with the error "Undefined symbol: ._Unwind_Resume" on AIX. This PR explicitly adds crate `unwind` for AIX, along with feature `panic_unwind`, which is required to include the `unwind` crate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 approved these changes

Assignees

@bjorn3 bjorn3

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.84.0
Development

Successfully merging this pull request may close these issues.
6 participants
@xingxue-ibm @rustbot @rust-log-analyzer @bjorn3 @bors @cjgillot