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

[AIX] Add option -X32_64 to the "strip" command #133217

Merged
merged 1 commit into from
Nov 23, 2024

Conversation

xingxue-ibm
Copy link
Contributor

@xingxue-ibm xingxue-ibm commented Nov 19, 2024

The AIX strip utility requires option -X to specify the object mode. This patch adds the -X32_64 option to the strip command so that it can handle both 32-bit and 64-bit objects. The parameter option of function strip_symbols_with_external_utility, previously a single string, has been changed to options, an array of string slices, to accommodate multiple strip options.

@rustbot
Copy link
Collaborator

rustbot commented Nov 19, 2024

r? @compiler-errors

rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 19, 2024
@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@mustartt mustartt left a comment

Choose a reason for hiding this comment

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

nit: consider changing Option<&[String]> to maybe &[&str] so we don't need to use to_string.

@xingxue-ibm
Copy link
Contributor Author

nit: consider changing Option<&[String]> to maybe &[&str] so we don't need to use to_string.

Good point, thanks!

@lqd
Copy link
Member

lqd commented Nov 20, 2024

Similarly you don't need the vec.

@rust-log-analyzer

This comment has been minimized.

@xingxue-ibm
Copy link
Contributor Author

Similarly you don't need the vec.

Thanks for pointing this out, @lqd! This makes the code much better!

@lqd
Copy link
Member

lqd commented Nov 20, 2024

Sure, no problem.

Also, from a quick read:

@xingxue-ibm
Copy link
Contributor Author

Sure, no problem.

Also, from a quick read:

* you probably don't need the Option, and could use empty slices for no options

* can likely use https://doc.rust-lang.org/nightly/nightly-rustc/rustc_codegen_ssa/back/command/struct.Command.html#method.args in `strip_symbols_with_external_utility`

* (also, it'd be better to squash the commits)

Done. Thank you so much for the guidance, @lqd!

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 21, 2024
@xingxue-ibm
Copy link
Contributor Author

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 21, 2024
@compiler-errors compiler-errors added the O-aix OS: Big Blue's Advanced Interactive eXecutive.. label Nov 21, 2024
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 21, 2024

📌 Commit 02f51ec has been approved by compiler-errors

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 21, 2024
jhpratt added a commit to jhpratt/rust that referenced this pull request Nov 22, 2024
…errors

[AIX] Add option -X32_64 to the "strip" command

The AIX `strip` utility requires option `-X` to specify the object mode. This patch adds the `-X32_64` option to the `strip` command so that it can handle both 32-bit and 64-bit objects. The parameter `option` of function `strip_symbols_with_external_utility`, previously a single string, has been changed to `options`, an array of string slices, to accommodate multiple `strip` options.
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 22, 2024
Rollup of 13 pull requests

Successful merges:

 - rust-lang#130867 (distinguish overflow and unimplemented in Step::steps_between)
 - rust-lang#131859 (Update TRPL to add new Chapter 17: Async and Await)
 - rust-lang#132090 (Stop being so bail-y in candidate assembly)
 - rust-lang#132658 (Detect const in pattern with typo)
 - rust-lang#133041 (Print name of env var in `--print=deployment-target`)
 - rust-lang#133102 (aarch64 softfloat target: always pass floats in int registers)
 - rust-lang#133159 (Don't allow `-Zunstable-options` to take a value )
 - rust-lang#133217 ([AIX] Add option -X32_64 to the "strip" command)
 - rust-lang#133237 (Minimally constify `Add`)
 - rust-lang#133238 (re-export `is_loongarch_feature_detected`)
 - rust-lang#133286 (Re-delay a resolve `bug` related to `Self`-ctor in patterns)
 - rust-lang#133301 (Add code example for `wrapping_neg` method for signed integers)
 - rust-lang#133313 (Use arc4random of libc for RTEMS target)

Failed merges:

 - rust-lang#133215 (Fix missing submodule in `./x vendor`)

r? `@ghost`
`@rustbot` modify labels: rollup
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Nov 22, 2024
…errors

[AIX] Add option -X32_64 to the "strip" command

The AIX `strip` utility requires option `-X` to specify the object mode. This patch adds the `-X32_64` option to the `strip` command so that it can handle both 32-bit and 64-bit objects. The parameter `option` of function `strip_symbols_with_external_utility`, previously a single string, has been changed to `options`, an array of string slices, to accommodate multiple `strip` options.
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Nov 23, 2024
…errors

[AIX] Add option -X32_64 to the "strip" command

The AIX `strip` utility requires option `-X` to specify the object mode. This patch adds the `-X32_64` option to the `strip` command so that it can handle both 32-bit and 64-bit objects. The parameter `option` of function `strip_symbols_with_external_utility`, previously a single string, has been changed to `options`, an array of string slices, to accommodate multiple `strip` options.
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 23, 2024
Rollup of 4 pull requests

Successful merges:

 - rust-lang#133217 ([AIX] Add option -X32_64 to the "strip" command)
 - rust-lang#133237 (Minimally constify `Add`)
 - rust-lang#133355 (Add language tests for aggregate types)
 - rust-lang#133374 (show abi_unsupported_vector_types lint in future breakage reports)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 92f5144 into rust-lang:master Nov 23, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Nov 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 23, 2024
Rollup merge of rust-lang#133217 - xingxue-ibm:fix-strip, r=compiler-errors

[AIX] Add option -X32_64 to the "strip" command

The AIX `strip` utility requires option `-X` to specify the object mode. This patch adds the `-X32_64` option to the `strip` command so that it can handle both 32-bit and 64-bit objects. The parameter `option` of function `strip_symbols_with_external_utility`, previously a single string, has been changed to `options`, an array of string slices, to accommodate multiple `strip` options.
@mustartt
Copy link
Contributor

This fixes a lot of test cases on AIX and allows the strip to function properly without OBJECT_MODE=64.

@rustbot label beta-nominate

@rustbot
Copy link
Collaborator

rustbot commented Nov 26, 2024

Error: Label beta-nominate can only be set by Rust team members

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@mustartt
Copy link
Contributor

@rustbot label beta-nominated

@rustbot rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 26, 2024
@apiraino
Copy link
Contributor

Beta backport declined as per compiler team on Zulip. As far as we can see this is not fixing a regression, so we think it can wait the next release cycle

@rustbot label -beta-nominated

@rustbot rustbot removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 28, 2024
@davidtwco
Copy link
Member

Thanks for taking the time to fix this bug. Just to add to what @apiraino has said, the project's backport policy describes how we typically only backport regressions, and as far as we can tell, this isn't a regression, it's always been an issue. We also weigh up the risk vs reward for a backport, how likely a change is to break something vs how critical the issue is (can it be worked around, is it on a tier one platform, etc).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-aix OS: Big Blue's Advanced Interactive eXecutive.. 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
Development

Successfully merging this pull request may close these issues.

9 participants