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

[WIP] Stabilize -Z pre-link-arg(s) as -C pre-link-arg(s) #70505

Closed
wants to merge 1 commit into from

Conversation

petrochenkov
Copy link
Contributor

The option is the same as -C link-arg(s), except that the arguments are inserted at the start of the linker option list, rather than at the end.

The feature was introduced in 2017 (#41971) to facilitate discussion about passing linker arguments (rust-embedded/wg#24). The conclusion seems to be "yep, .cargo/config with rustc flags in it is the way to go".

This PR also accompanies #70501.

@rust-highfive
Copy link
Collaborator

r? @estebank

(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 Mar 28, 2020
@petrochenkov petrochenkov added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 28, 2020
@petrochenkov
Copy link
Contributor Author

cc @japaric

@jonas-schievink jonas-schievink added A-linkage Area: linking into static, shared libraries and binaries needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. labels Mar 28, 2020
@jonas-schievink jonas-schievink added this to the 1.44 milestone Mar 28, 2020
@ehuss
Copy link
Contributor

ehuss commented Mar 28, 2020

Would you mind also updating the documentation at https://github.com/rust-lang/rust/blob/master/src/doc/rustc/src/codegen-options/index.md? It might be useful to clarify what it means by "prepend". I presume it means "arguments added before the arguments generated by rustc"? So the result is [pre-link-args] …generated args… [link-args] ?.

@Centril
Copy link
Contributor

Centril commented Mar 30, 2020

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned estebank Mar 30, 2020
@nagisa
Copy link
Member

nagisa commented Mar 30, 2020

Is there a tracking issue for this feature? I have no concerns re implementation, but I think it should still go through the usual process (such as the FCP?)

@Centril
Copy link
Contributor

Centril commented Mar 30, 2020

Are there docs for the flag? Would be good to add some user facing ones if not.

@petrochenkov
Copy link
Contributor Author

There's no tracking issue and I'll add docs.

@petrochenkov
Copy link
Contributor Author

I'll add docs.

Ha-ha, looks like the option parsing for both stable link-args and unstable pre-link-args is terribly broken if multiple options are passed. Some of them are lost, some are reordered.

rustc -C link-arg="a" -C link-args="b c" -C link-args="d e" -C link-arg="f" mytest.rs
... "d" "e" "a" "f" ...

I'm not even surprised.

I'll fix this stuff before the stabilization.

@petrochenkov petrochenkov changed the title Stabilize -Z pre-link-arg(s) as -C pre-link-arg(s) [WIP] Stabilize -Z pre-link-arg(s) as -C pre-link-arg(s) Mar 31, 2020
@petrochenkov
Copy link
Contributor Author

Closing for now.
Upon closer investigation it turned out it's a mess (as usual), and it's risky to stabilize as is.
I'll try to work on some cleanup and documentation and then perhaps resurrect this PR in some form.

Centril added a commit to Centril/rust that referenced this pull request Apr 5, 2020
Do not lose or reorder user-provided linker arguments

Linker arguments are potentially order-dependent, so the order in which `-C link-arg` and `-C link-args` options are passed to `rustc` should be preserved when they are passed further to the linker.

Also, multiple `-C link-args` options are now appended to each other rather than overwrite each other.

In other words, `-C link-arg=a -C link-args="b c" -C link-args="d e" -C link-arg=f` is now passed as `"a" "b" "c" "d" "e" "f"` and not as `"d" "e" "a" "f"`.

Addresses rust-lang#70505 (comment).
Centril added a commit to Centril/rust that referenced this pull request Apr 9, 2020
…ti865

rustc_codegen_ssa: Refactor construction of linker arguments

And add comments.

This PR doesn't reorder any linker arguments and therefore shouldn't contain any observable changes.

The next goal here is to
- Factor out order-independent linker arguments in the compiler code and in target specifications and pass them together. Such arguments generally apply to the whole linking session or the produced linking result rather to individual object files or libraries.
- Figure out where exactly among the remaining order-dependent arguments we should place customization points like `-C link-args` and `-Z pre-link-args`.
- Possibly provide command line opt-outs for options that are currently passed unconditionally (like CRT objects or arguments defined by the target spec).
- Document and stabilize the customization points that are not yet stable (rust-lang#70505).
@crlf0710 crlf0710 removed this from the 1.44 milestone Apr 27, 2020
@wmmc88
Copy link

wmmc88 commented Jul 5, 2023

@petrochenkov is there any plan to stabilize these features?

@petrochenkov
Copy link
Contributor Author

@wmmc88
No immediate plans for -Z pre-link-arg.

You may also be interested in #99427 (-l link-arg) which has better chances at stabilization in the near future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linkage Area: linking into static, shared libraries and binaries needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

9 participants