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

Preserve metadata w/ Solaris-like linkers. #85772

Merged
merged 1 commit into from
Jun 6, 2021

Conversation

luqmana
Copy link
Member

@luqmana luqmana commented May 28, 2021

#84468 moved the -zignore linker flag from the gc_sections method to add_as_needed which is more accurate but Solaris-style linkers will also end up removing an unreferenced ELF sections [1]. This had the unfortunate side effect of causing the .rustc section (which has the metada) to be removed which could cause issues when trying to link against the resulting crates or use proc macros.

Since the -zignore is positional, we fix this by moving the metadata objects to before the flag.

[1] Specifically a section is considered unreferenced if:

  • The section is allocatable
  • No other sections bind to (relocate) to this section
  • The section provides no global symbols

https://docs.oracle.com/cd/E19683-01/817-3677/6mj8mbtbs/index.html#chapter4-19

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 28, 2021
@luqmana
Copy link
Member Author

luqmana commented May 28, 2021

r? @petrochenkov

@petrochenkov
Copy link
Contributor

petrochenkov commented May 29, 2021

@luqmana
Do you have access to a Solaris machine for testing? (Which version?)
The link in the original post is very old, and more up-to-date documentation (Solaris 11.4, https://docs.oracle.com/cd/E37838_01/html/E36783/man-rum.html) says that -z ignore/-z record only affect dynamic libraries ( –z discard-unused=dependencies, --as-needed-style), but not sections (–z discard-unused=sections, --gc-sections-style).

UPD: Looks like the old doc also talks about dynamic libraries

An object built with the -z ignore option, will have no unused dependencies recorded in it.

, it's just clarified better in later versions.

@petrochenkov
Copy link
Contributor

Some relevant PRs:

@petrochenkov
Copy link
Contributor

If -z ignore acts as --gc-sections and have effect on unused sections (to which I find no confirmation in docs), then perhaps it's better to wrap metadata only into -z record instead?

@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 May 29, 2021
@luqmana
Copy link
Member Author

luqmana commented May 29, 2021

Not solaris exactly but I do have access to an illumos system which is where I ran into the issue. From man ld:

       -z ignore | record

           Ignores, or records, dynamic dependencies that are not referenced
           as part of the link-edit. Ignores, or records, unreferenced ELF
           sections from the relocatable objects that are read as part of the
           link-edit. By default, -z record is in effect.

           If an ELF section is ignored, the section is eliminated from the
           output file being generated. A section is ignored when three
           conditions are true. The eliminated section must contribute to an
           allocatable segment. The eliminated section must provide no global
           symbols. No other section from any object that contributes to the
           link-edit, must reference an eliminated section.

@luqmana
Copy link
Member Author

luqmana commented May 29, 2021

I'm fine with alternatively wrapping the metadata object with -z record <metadata.o> -z ignore but I just initially moved it a bit above to minimize the linker invocation a bit.

@petrochenkov
Copy link
Contributor

Looks like I've been looking at a wrong doc all this time, the illumos doc (https://illumos.org/man/1/ld) explicitly says that -z ignore means both --as-needed and --gc-sections, which is different from classic Solaris.

@petrochenkov petrochenkov 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 May 30, 2021
@luqmana
Copy link
Member Author

luqmana commented May 30, 2021

Definitely sure this applies to illumos, but I'm pretty sure this applies to Solaris itself too:

https://github.com/kofemann/opensolaris/blob/80192cd83bf665e708269dae856f9145f7190f74/usr/src/cmd/sgs/libld/common/sections.c#L164-L182

If -zignore has been in effect, scan all input files to determine if the file, or sections from the file, have been referenced. If not, the file or some of the files sections can be discarded.

@petrochenkov
Copy link
Contributor

cc @jclulow

@petrochenkov
Copy link
Contributor

One more idea (from #84449) is to mark the sections somehow (e.g. as debuginfo sections) so they are not thrown away when necessary.
(Given that #84449 is probably going to land, its compatibility with Solaris/illumos needs to be considered anyway.)

@luqmana
Copy link
Member Author

luqmana commented May 30, 2021

One more idea (from #84449) is to mark the sections somehow (e.g. as debuginfo sections) so they are not thrown away when necessary.
(Given that #84449 is probably going to land, its compatibility with Solaris/illumos needs to be considered anyway.)

I can definitely give #84449 a try to make sure it works fine on illumos. But I don't think it addresses the issue in this PR as it seems to be focused on metadata in rlibs whereas this issue specifically is for dylibs/proc-macros. (See write_compressed_metadata which hasn't been updated in that PR. Edit: I see @bjorn3 has brought that up already.)

@petrochenkov
Copy link
Contributor

Sorry for the delay, busy week.
@bors r+

I made myself an OpenIndiana virtual machine today and it seems to work, so I'll be able to experiment with fine-grained -z ignore/-z record to properly support as-needed modifier and -Clink-dead-code on Solaris (while being compatible with linker argument ordering from #85086).
In the meantime let's land this to fix dylibs/proc-macros.

@bors
Copy link
Contributor

bors commented Jun 5, 2021

📌 Commit cffef33 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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 6, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#85436 (Avoid cloning cache key)
 - rust-lang#85772 (Preserve metadata w/ Solaris-like linkers.)
 - rust-lang#85920 (Tweak wasm_base target spec to indicate linker is not GNU and update linker inferring logic for wasm-ld.)
 - rust-lang#85930 (Update standard library for IntoIterator implementation of arrays )
 - rust-lang#85972 (Rustdoc html fixes)
 - rust-lang#86028 (Drop an `if let` that will always succeed)
 - rust-lang#86043 (don't clone attrs)
 - rust-lang#86047 (Don't fire `invalid_doc_attributes` on `extern crate` items)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d69cd46 into rust-lang:master Jun 6, 2021
@rustbot rustbot added this to the 1.54.0 milestone Jun 6, 2021
@luqmana luqmana deleted the ignored-metadata branch June 6, 2021 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants