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

Make symbols stripping work on MacOS X #82037

Merged
merged 1 commit into from
Jun 10, 2021
Merged

Make symbols stripping work on MacOS X #82037

merged 1 commit into from
Jun 10, 2021

Conversation

calavera
Copy link
Contributor

As reported in the stabilization issue, MacOS' linker doesn't support the -s and -S flags to strip symbols anymore. However, the os ships a separated tool to perform these operations.

This change allows the compiler to use that tool after a target has been compiled to strip symbols.

For rationale, see: #72110 (comment)
For option selection, see: https://www.unix.com/man-page/osx/1/strip/

Signed-off-by: David Calavera david.calavera@gmail.com

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 12, 2021
@calavera
Copy link
Contributor Author

@estebank can you let me know if this is a good direction to solve the problem on MacOS? I'll be happy to modify this patch as needed

@eggyal
Copy link
Contributor

eggyal commented Mar 3, 2021

@petrochenkov perhaps this is one for your review, given that you were involved in #72110 and #73138?

@estebank
Copy link
Contributor

estebank commented Mar 3, 2021 •
edited
Loading

I would feel better if someone with more understanding of codegen took over this PR. @petrochenkov or @ehuss, would you be up to it? The code itself looks reasonable, but this is not my area of expertise.

@ehuss
Copy link
Contributor

ehuss commented Mar 8, 2021

@ehuss, would you be up to it?

Not really. I can help discuss it, but I don't have the time to be responsible here.

@estebank
Copy link
Contributor

estebank commented Mar 8, 2021

I really don't want to block on this, so I will send a blast to the rest of the team. @rust-lang/compiler would any of you be a better fit for reviewing this PR?

@estebank estebank added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 8, 2021
@pnkfelix pnkfelix self-assigned this Mar 11, 2021
@pnkfelix
Copy link
Member

self-assigning to act as extra eyeballs for @estebank . Un-nominating from team.

@joshtriplett joshtriplett mentioned this pull request Mar 12, 2021
Closed
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 29, 2021
@bors

This comment has been minimized.

Sign in to view
@JohnCSimon
Copy link
Member

Ping from triage: @calavera can you please address the merge conflict? thank you

@rustbot label: -S-waiting-on-review +S-waiting-on-author

@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 Apr 18, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@calavera
Copy link
Contributor Author

@JohnCSimon thanks for the ping. I've updated this PR.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@calavera
Copy link
Contributor Author

@ehuss I've looked at adding another LinkerFlavor option, but I think it's going to over complicate this change.

That enum is used extensively to control other linker behaviors, much more important than this feature. Introducing an option that is not used like the rest of the flavors would only be more confusing for future contributors, in my opinion.

Moreover, there are other features that follow the same convention that I'm using here, which is checking the is_like_osx flag, and similar, to execute further commands for specific targets, see the stabilization PR for -Zrun-dsymutil as -Csplit-debuginfo #79570.

I do agree that it's not necessary to extend this logic between two files, so I moved everything into link.rs. Let me know what you think.

@calavera calavera requested a review from pnkfelix April 23, 2021 20:00
@ehuss
Copy link
Contributor

ehuss commented Apr 25, 2021

That's fine, it was just a thought. The idea would be to handle the case if you are not using ld64 on macos. In this particular case, I don't think it matters too much. If there is a desire to use more ld64-specific flags in the future, it might be worth considering more.

From a cursory glance, this looks good to me. I haven't tested this locally, though. I would still prefer for @pnkfelix or some other compiler team member to review.

Thanks for working on this!

@dbr dbr mentioned this pull request Apr 27, 2021
Closed
@joshtriplett
Copy link
Member

@calavera Looking over the codebase a second time, it looks like there are indeed a number of other places making similar assumptions about macOS targets, notably the handling of dsymutil. Given that, while I do think it'd be valuable to switch to a linker flavor, I also don't think this PR needs to do that. Consider the concern withdrawn, please; is_like_osx seems fine for now.

@petrochenkov
Copy link
Contributor

r? @petrochenkov
There's nothing wrong with using is_like_osx, and adding a new linker flavor is an orthogonal change (and it's not clear to me whether adding it is worth it).
I'll look at this PR this week.

@petrochenkov
Copy link
Contributor

r=me after squashing commits.

@petrochenkov petrochenkov 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 Jun 9, 2021
@calavera
Make symbols stripping work on MacOS X
df0fc6d 
As reported in the stabilization issue, MacOS' linker doesn't support the `-s` and `-S` flags to strip symbols anymore. However, the os ships a separated tool to perform these operations.

This change allows the compiler to use that tool after a target has been compiled to strip symbols.

For rationale, see: rust-lang#72110 (comment)
For option selection, see: https://www.unix.com/man-page/osx/1/strip/

Signed-off-by: David Calavera <david.calavera@gmail.com>
@calavera
Copy link
Contributor Author

calavera commented Jun 9, 2021

@petrochenkov I've squashed the commits. GitHub Actions is waiting for a maintainer to approve the workflows before running.

@petrochenkov
Copy link
Contributor

@bors r+

@petrochenkov petrochenkov reopened this Jun 9, 2021
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 9, 2021

📌 Commit df0fc6d has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 9, 2021
@JohnTitor JohnTitor mentioned this pull request Jun 10, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 10, 2021
@bors
Auto merge of rust-lang#86186 - JohnTitor:rollup-upaw6wx, r=JohnTitor
c5fbcd3 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#82037 (Make symbols stripping work on MacOS X)
 - rust-lang#84687 (Multiple improvements to RwLocks)
 - rust-lang#85997 (rustdoc: Print a warning if the diff when comparing to old nightlies is empty)
 - rust-lang#86051 (Updated code examples and wording in move keyword documentation )
 - rust-lang#86111 (fix off by one in `std::iter::Iterator` documentation)
 - rust-lang#86113 (build doctests with lld if use-lld = true)
 - rust-lang#86175 (update Miri)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 27e84b8 into rust-lang:master Jun 10, 2021
@rustbot rustbot added this to the 1.54.0 milestone Jun 10, 2021
@joshtriplett joshtriplett added relnotes Marks issues that should be documented in the release notes of the next release. and removed relnotes Marks issues that should be documented in the release notes of the next release. labels Jun 10, 2021
@calavera calavera deleted the strip_debuginfo_osx branch June 10, 2021 17:31
@ehuss ehuss mentioned this pull request Sep 9, 2021
Merged
bors added a commit to rust-lang/cargo that referenced this pull request Sep 9, 2021
@bors
Auto merge of #9889 - ehuss:macos-strip, r=alexcrichton
4168727 
Enable strip test on macos.

This enables the `strip_works` test for macos, which was fixed a while ago in rust-lang/rust#82037.
@nagisa nagisa mentioned this pull request Sep 28, 2022
Merged
@messense messense mentioned this pull request Mar 14, 2023
Closed
@Skgland Skgland mentioned this pull request Jul 29, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehuss ehuss ehuss left review comments

@estebank estebank estebank left review comments

@eggyal eggyal eggyal left review comments

@pnkfelix pnkfelix Awaiting requested review from pnkfelix

Assignees

@petrochenkov petrochenkov

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.54.0
Development

Successfully merging this pull request may close these issues.