-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Make symbols stripping work on MacOS X #82037
Conversation
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. |
@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 |
@petrochenkov perhaps this is one for your review, given that you were involved in #72110 and #73138? |
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. |
Not really. I can help discuss it, but I don't have the time to be responsible here. |
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? |
self-assigning to act as extra eyeballs for @estebank . Un-nominating from team. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@JohnCSimon thanks for the ping. I've updated this PR. |
This comment has been minimized.
This comment has been minimized.
@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 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. |
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! |
@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. |
r? @petrochenkov |
r=me after squashing commits. |
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>
6d9203e
to
df0fc6d
Compare
@petrochenkov I've squashed the commits. GitHub Actions is waiting for a maintainer to approve the workflows before running. |
@bors r+ |
@bors r+ |
📌 Commit df0fc6d has been approved by |
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
Enable strip test on macos. This enables the `strip_works` test for macos, which was fixed a while ago in rust-lang/rust#82037.
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