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

add support for the l4-bender linker on the x86_64-unknown-l4re-uclibc tier 3 target #85967

Merged
merged 3 commits into from
Jan 22, 2022

Conversation

atopia
Copy link
Contributor

@atopia atopia commented Jun 3, 2021

This PR contains the work by @humenda to update support for the x86_64-unknown-l4re-uclibc tier 3 target (published at humenda/rust), rebased and adapted to current rust in follow up commits by myself. The publishing of the rebased changes is authorized and preferred by the original author. As the goal was to distort the original work as little as possible, individual commits introduce changes that are incompatible to the newer code base that the changes were rebased on. These incompatibilities have been remedied in follow up commits, so that the PR as a whole should result in a clean update of the target.
If you prefer another strategy to mainline these changes while preserving attribution, please let me know.

@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

@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 @matthewjasper (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 Jun 3, 2021
@bors

This comment has been minimized.

fn control_flow_guard(&mut self) {}

fn no_crt_objects(&mut self) {}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The interface looks like more or less like an ld linker, I'd suggest to avoid adding a new linker flavor for a start and reuse GccLinker (especially given that the set of linker flavors accepted by -C linker-flavor is a stable public interface).

@humenda
Copy link
Contributor

humenda commented Jun 10, 2021 via email

@crlf0710 crlf0710 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 Jun 26, 2021
@bors

This comment has been minimized.

@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 Jul 25, 2021
@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 Aug 15, 2021
@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021
@pnkfelix
Copy link
Member

pnkfelix commented Sep 9, 2021

It is impossible to pass in arguments with spaces to the linker due to the way how this is implemented for linkers in Rustc. I talked to Alex Crichton @alexcrichton (hopefully the correct nick) a long time ago and the suggested way has been to add a new variant. L4-bender is indeed internally using ld but it bends the arguments and adds quite some things and custom options. If I recall correctly, even the order of the arguments matters, something that cannot be influenced from outside. One notable one is the -- that separates l4-bender arguments from ld arguments that are passed through.

I think the points made here seem to give reasonable support for adding a new linker flavor for this case.

@pnkfelix pnkfelix assigned pnkfelix and unassigned matthewjasper Sep 9, 2021
@humenda
Copy link
Contributor

humenda commented Sep 9, 2021 via email

@pnkfelix
Copy link
Member

pnkfelix commented Sep 9, 2021

I think the points made here seem to give reasonable support for adding a new linker flavor for this case.

Could we get some more support, e.g. by a rustc CI, to detect breakage earlier? Or is this something that should be done on the L4Re side due to the tier 3 support level?

I think the latter. My understanding of the tier 3 support level is that it is not meant to impose new burdens on the existing infrastructure.

@pnkfelix
Copy link
Member

pnkfelix commented Sep 9, 2021

We discussed this in today's T-compiler weekly triage meeting.

Some further review of the target tier policy led us to conclude that an MCP is not strictly required here.

I'm going to double-check the points that @petrochenkov made about the linker, but I'm currently inclined to trust @humenda 's judgement here.

Question for @humenda and @atopia : should either or both of you be recorded as maintainers for the x86_64-unknown-l4re-uclibc target? Is there anyone else who should also be on such a list of maintainers for the target?

@petrochenkov petrochenkov self-assigned this Sep 10, 2021
@petrochenkov
Copy link
Contributor

I'll look in more details, this all still look pretty suspicious to me.
I'm not even sure the cmd quoting and env reading stuff belongs to rustc?
There's approximately zero documentation about l4-bender on the net (if you have some info about what it does, please share), so it seems niche enough to generally adapt to rustc and not otherwise.

@petrochenkov
Copy link
Contributor

petrochenkov commented Sep 11, 2021

Here's the version of this PR, but without introducing a new linker flavor - petrochenkov@d3b02f3
It highlights the differences from a regular gcc-style linker, I left questions about them in code.
Most of them are related to the comma vs whitespace story which is still not clear to me.
If l4-bender simply passes the arguments after -- to the underlying linker, then why does e.g. -z relro needs to be changed to -z,relro?

Also, if there are some issues with passing arguments with whitespaces, then it's better to make a separate issue (with examples from practice, showing how they behave now, and how they should behave instead) and solve it once and for all, instead of introducing more hacks for specific targets.

r? @petrochenkov

@petrochenkov
Copy link
Contributor

(I'd also suggest submitting the library changes separately, in that case they will get a more appropriate reviewer familiar with library conventions and will likely land faster.)

@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 Sep 11, 2021
@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 Jan 20, 2022
humenda and others added 3 commits January 21, 2022 16:28
- Fix style errors.

- L4-bender does not yet support dynamic linking.

- Stack unwinding is not yet supported for x86_64-unknown-l4re-uclibc.
  For now, just abort on panics.

- Use GNU-style linker options where possible. As suggested by review:
    - Use standard GNU-style ld syntax for relro flags.
    - Use standard GNU-style optimization flags and logic.
    - Use standard GNU-style ld syntax for --subsystem.

- Don't read environment variables in L4Bender linker. Thanks to
  CARGO_ENCODED_RUSTFLAGS introduced in rust-lang#9601, l4-bender's arguments can
  now be passed from the L4Re build system without resorting to custom
  parsing of environment variables.
@atopia
Copy link
Contributor Author

atopia commented Jan 21, 2022

@rustbot ready

@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 Jan 21, 2022
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 22, 2022

📌 Commit 29d6235 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 Jan 22, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2022
…henkov

add support for the l4-bender linker on the x86_64-unknown-l4re-uclibc tier 3 target

This PR contains the work by `@humenda` to update support for the `x86_64-unknown-l4re-uclibc` tier 3 target (published at [humenda/rust](https://github.com/humenda/rust)), rebased and adapted to current rust in follow up commits by myself. The publishing of the rebased changes is authorized and preferred by the original author. As the goal was to distort the original work as little as possible, individual commits introduce changes that are incompatible to the newer code base that the changes were rebased on. These incompatibilities have been remedied in follow up commits, so that the PR as a whole should result in a clean update of the target.
If you prefer another strategy to mainline these changes while preserving attribution, please let me know.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2022
…henkov

add support for the l4-bender linker on the x86_64-unknown-l4re-uclibc tier 3 target

This PR contains the work by ``@humenda`` to update support for the `x86_64-unknown-l4re-uclibc` tier 3 target (published at [humenda/rust](https://github.com/humenda/rust)), rebased and adapted to current rust in follow up commits by myself. The publishing of the rebased changes is authorized and preferred by the original author. As the goal was to distort the original work as little as possible, individual commits introduce changes that are incompatible to the newer code base that the changes were rebased on. These incompatibilities have been remedied in follow up commits, so that the PR as a whole should result in a clean update of the target.
If you prefer another strategy to mainline these changes while preserving attribution, please let me know.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 22, 2022
…askrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#85967 (add support for the l4-bender linker on the x86_64-unknown-l4re-uclibc tier 3 target)
 - rust-lang#92828 (Print a helpful message if unwinding aborts when it reaches a nounwind function)
 - rust-lang#93012 (Update pulldown-cmark version to fix markdown list issue)
 - rust-lang#93116 (Simplify use of `map_or`)
 - rust-lang#93132 (Increase the format version of rustdoc-json-types)
 - rust-lang#93147 (Interner cleanups)
 - rust-lang#93153 (Reject unsupported naked functions)
 - rust-lang#93170 (Add missing GUI test explanations)
 - rust-lang#93172 (rustdoc: remove dashed underline under main heading)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ffd199d into rust-lang:master Jan 22, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 22, 2022
@atopia atopia deleted the update-l4re-target branch January 24, 2022 10:10
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 19, 2022
Update stdlib for the l4re target

This PR contains the work by `@humenda` and myself to update standard library support for the x86_64-unknown-l4re-uclibc tier 3 target, split out from  humenda/rust as requested in rust-lang#85967. The changes have been rebased on current master and updated in follow up commits by myself. The publishing of the changes is authorized and preferred by the original author. To preserve attribution, when standard library changes were introduced as part of other changes to the compiler, I have kept the changes concerning the standard library and altered the commit messages as indicated. Any incompatibilities have been remedied in follow up commits, so that the PR as a whole should result in a clean update of the target.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 19, 2022
Update stdlib for the l4re target

This PR contains the work by ``@humenda`` and myself to update standard library support for the x86_64-unknown-l4re-uclibc tier 3 target, split out from  humenda/rust as requested in rust-lang#85967. The changes have been rebased on current master and updated in follow up commits by myself. The publishing of the changes is authorized and preferred by the original author. To preserve attribution, when standard library changes were introduced as part of other changes to the compiler, I have kept the changes concerning the standard library and altered the commit messages as indicated. Any incompatibilities have been remedied in follow up commits, so that the PR as a whole should result in a clean update of the target.
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. 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.