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

Fix#84467 linker_args with --target=sparcv9-sun-solaris #84468

Merged
merged 1 commit into from
May 6, 2021

Conversation

iladin
Copy link
Contributor

@iladin iladin commented Apr 23, 2021

Trying to cross-compile for sparcv9-sun-solaris
getting a error message for -zignore

Introduced when -z -ignore was seperated here
22d0ab0

No formatting done

Reproduce

rustup target add sparcv9-sun-solaris
cargo new --bin hello && cd hello && cargo run --target=sparcv9-sun-solaris

config.toml

[target.sparcv9-sun-solaris]
linker = "gcc"

@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 @varkor (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 Apr 23, 2021
@iladin iladin force-pushed the iladin/fix-84467 branch from 8590958 to e2b5087 Compare April 30, 2021 04:51
@petrochenkov
Copy link
Contributor

r? @petrochenkov

@rust-highfive rust-highfive assigned petrochenkov and unassigned varkor Apr 30, 2021
@petrochenkov
Copy link
Contributor

This is something I had questions about.
The doc (https://docs.oracle.com/cd/E88353_01/html/E37839/ld-1.html) says that ld on Solaris supports GNU-compatible –as-needed too, so why not use it?
(I never worked with Solaris or its derivatives, so I may be missing something.)

Also this logic needs to be moved from fn gc_sections to fn add_as_needed.

@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 Apr 30, 2021
@iladin
Copy link
Contributor Author

iladin commented Apr 30, 2021

This is something I had questions about.
The doc (https://docs.oracle.com/cd/E88353_01/html/E37839/ld-1.html) says that ld on Solaris supports GNU-compatible –as-needed too, so why not use it?
(I never worked with Solaris or its derivatives, so I may be missing something.)

Also this logic needs to be moved from fn gc_sections to fn add_as_needed.

I agree that this should be moved from gc_sections (unrelated; not mutually exclusive to --gc-sections) and had only kept it there since it was a revert.

However, --as-needed only functions as -z ignore on Solaris 11.4, not on prior versions (https://docs.oracle.com/cd/E26502_01/html/E29030/ld-1.html)

@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 Apr 30, 2021
Moved -z ignore to add_as_needed

Trying to cross-compile for sparcv9-sun-solaris
getting an error message for -zignore

Introduced when -z -ignore was separated here
22d0ab0

No formatting done

Reproduce

``` bash
rustup target add sparcv9-sun-solaris
cargo new --bin hello && cd hello && cargo run --target=sparcv9-sun-solaris
```

config.toml

[target.sparcv9-sun-solaris]
linker = "gcc"
@iladin iladin force-pushed the iladin/fix-84467 branch from e2b5087 to fe68b1a Compare April 30, 2021 22:56
@iladin
Copy link
Contributor Author

iladin commented Apr 30, 2021

@petrochenkov I have made the changes you had requested in fe68b1a, let me know if you have any further questions.

@petrochenkov
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 3, 2021

📌 Commit fe68b1a 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 May 3, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 4, 2021
…nkov

Fix#84467 linker_args with --target=sparcv9-sun-solaris

Trying to cross-compile for sparcv9-sun-solaris
getting a error message for -zignore

Introduced when -z -ignore was seperated here
22d0ab0

No formatting done

Reproduce

``` bash
rustup target add sparcv9-sun-solaris
cargo new --bin hello && cd hello && cargo run --target=sparcv9-sun-solaris
```

config.toml

[target.sparcv9-sun-solaris]
linker = "gcc"
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 4, 2021
…nkov

Fix#84467 linker_args with --target=sparcv9-sun-solaris

Trying to cross-compile for sparcv9-sun-solaris
getting a error message for -zignore

Introduced when -z -ignore was seperated here
22d0ab0

No formatting done

Reproduce

``` bash
rustup target add sparcv9-sun-solaris
cargo new --bin hello && cd hello && cargo run --target=sparcv9-sun-solaris
```

config.toml

[target.sparcv9-sun-solaris]
linker = "gcc"
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 4, 2021
…nkov

Fix#84467 linker_args with --target=sparcv9-sun-solaris

Trying to cross-compile for sparcv9-sun-solaris
getting a error message for -zignore

Introduced when -z -ignore was seperated here
22d0ab0

No formatting done

Reproduce

``` bash
rustup target add sparcv9-sun-solaris
cargo new --bin hello && cd hello && cargo run --target=sparcv9-sun-solaris
```

config.toml

[target.sparcv9-sun-solaris]
linker = "gcc"
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 4, 2021
…nkov

Fix#84467 linker_args with --target=sparcv9-sun-solaris

Trying to cross-compile for sparcv9-sun-solaris
getting a error message for -zignore

Introduced when -z -ignore was seperated here
22d0ab0

No formatting done

Reproduce

``` bash
rustup target add sparcv9-sun-solaris
cargo new --bin hello && cd hello && cargo run --target=sparcv9-sun-solaris
```

config.toml

[target.sparcv9-sun-solaris]
linker = "gcc"
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 4, 2021
…nkov

Fix#84467 linker_args with --target=sparcv9-sun-solaris

Trying to cross-compile for sparcv9-sun-solaris
getting a error message for -zignore

Introduced when -z -ignore was seperated here
22d0ab0

No formatting done

Reproduce

``` bash
rustup target add sparcv9-sun-solaris
cargo new --bin hello && cd hello && cargo run --target=sparcv9-sun-solaris
```

config.toml

[target.sparcv9-sun-solaris]
linker = "gcc"
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 4, 2021
…nkov

Fix#84467 linker_args with --target=sparcv9-sun-solaris

Trying to cross-compile for sparcv9-sun-solaris
getting a error message for -zignore

Introduced when -z -ignore was seperated here
22d0ab0

No formatting done

Reproduce

``` bash
rustup target add sparcv9-sun-solaris
cargo new --bin hello && cd hello && cargo run --target=sparcv9-sun-solaris
```

config.toml

[target.sparcv9-sun-solaris]
linker = "gcc"
RalfJung added a commit to RalfJung/rust that referenced this pull request May 5, 2021
…nkov

Fix#84467 linker_args with --target=sparcv9-sun-solaris

Trying to cross-compile for sparcv9-sun-solaris
getting a error message for -zignore

Introduced when -z -ignore was seperated here
22d0ab0

No formatting done

Reproduce

``` bash
rustup target add sparcv9-sun-solaris
cargo new --bin hello && cd hello && cargo run --target=sparcv9-sun-solaris
```

config.toml

[target.sparcv9-sun-solaris]
linker = "gcc"
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 5, 2021
…nkov

Fix#84467 linker_args with --target=sparcv9-sun-solaris

Trying to cross-compile for sparcv9-sun-solaris
getting a error message for -zignore

Introduced when -z -ignore was seperated here
22d0ab0

No formatting done

Reproduce

``` bash
rustup target add sparcv9-sun-solaris
cargo new --bin hello && cd hello && cargo run --target=sparcv9-sun-solaris
```

config.toml

[target.sparcv9-sun-solaris]
linker = "gcc"
@RalfJung
Copy link
Member

RalfJung commented May 5, 2021

Could this have caused the failures in #84899, #84924, #84932, #84951? (It's always the same failure.)
@bors rollup=iffy

@iladin
Copy link
Contributor Author

iladin commented May 5, 2021

I really don't see how this could affect a different target, this is a small change for Solaris.

One common thread is that each run has also include #84797 It looks like this might be an issue with a deleted makefile not being included correctly?

I think what I will try to do is run the test locally without his change to show that it passes, merge it in to show that it fails and then use remake to figure out why his change is failing but it may take me some time to figure out how to do all that.

@bors
Copy link
Contributor

bors commented May 6, 2021

⌛ Testing commit fe68b1a with merge 1d99508...

@RalfJung
Copy link
Member

RalfJung commented May 6, 2021

Well, bors will soon tell us if it's this PR or the other one. :)

@bors
Copy link
Contributor

bors commented May 6, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 1d99508 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 6, 2021
@bors bors merged commit 1d99508 into rust-lang:master May 6, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 6, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 18, 2021
…r=petrochenkov

Only pass --[no-]gc-sections if linker is GNU ld.

Fixes a regression from rust-lang#84468 where linking now fails with solaris linkers. LinkerFlavor::Gcc does not always mean GNU ld specifically. And in the case of at least the solaris ld in illumos, that flag is unrecognized and will cause the linking step to fail.

Even though removing the `is_like_solaris` branch from `gc_sections` in rust-lang#84468 made sense as `-z ignore/record` are more analogous to the `--[no-]-as-needed` flags, it inadvertently caused solaris linkers to be passed the `--gc-sections` flag. So let's just change it to be more explicit about when we pass those flags.
jackh726 added a commit to jackh726/rust that referenced this pull request May 19, 2021
…r=petrochenkov

Only pass --[no-]gc-sections if linker is GNU ld.

Fixes a regression from rust-lang#84468 where linking now fails with solaris linkers. LinkerFlavor::Gcc does not always mean GNU ld specifically. And in the case of at least the solaris ld in illumos, that flag is unrecognized and will cause the linking step to fail.

Even though removing the `is_like_solaris` branch from `gc_sections` in rust-lang#84468 made sense as `-z ignore/record` are more analogous to the `--[no-]-as-needed` flags, it inadvertently caused solaris linkers to be passed the `--gc-sections` flag. So let's just change it to be more explicit about when we pass those flags.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 6, 2021
…enkov

Preserve metadata w/ Solaris-like linkers.

rust-lang#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

8 participants