try to infer linker flavor from linker name and vice versa #52101
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
alexcrichton
reviewed
Jul 6, 2018
|
|
||
| Ok((linker, flavor)) | ||
| }, | ||
| (None, None) => otherwise(), |
There was a problem hiding this comment.
Stylistically I might recommend instead of a cps style to instead do something like:
- Emit only fatal errors here via
sess(it doesn't look like the error is ever "caught"?) - Return
Optioninstead ofResult - Instead do something like:
if let Some(ret) = locate_linker(&sess.opts) {
return Some(ret)
}
if let Some(ret) = locate_linker(&sess.target.target) {
return Some(ret)
}
sess.fatal("...")| LinkerFlavor::Em => "emcc", | ||
| LinkerFlavor::Gcc => "gcc", | ||
| LinkerFlavor::Ld => "ld", | ||
| LinkerFlavor::Msvc => "link.exe", |
There was a problem hiding this comment.
I think this is losing the logic of locating link.exe on MSVC which may cause issues there?
There was a problem hiding this comment.
The logic is in get_linker which is called with the values this function returns. I think that function will locate link.exe even with these changes.
| (Some(linker), Some(flavor)) => Ok((linker, flavor)), | ||
| // only the linker flavor is known; use the default linker for the selected flavor | ||
| (None, Some(flavor)) => Ok((PathBuf::from(match flavor { | ||
| LinkerFlavor::Em => "emcc", |
There was a problem hiding this comment.
I think this may have accidentally lost the logic of emcc.bat on Windows?
| LinkerFlavor::Lld(f) => Command::lld(linker, f), | ||
| _ => Command::new(linker), | ||
|
|
||
| } | ||
| }; | ||
| })(); |
There was a problem hiding this comment.
You can probably remove the intermediate closure here, I don't think it's serving much purpose any more
| sess | ||
| .struct_err(&format!("couldn't infer linker flavor from specified linker")) | ||
| .emit(); | ||
| return Err(()); |
There was a problem hiding this comment.
This runs the risk of being a breaking change by accident if someone's passing in a "flavorful" linker because currently we can't error today I think? Perhaps this could fall back to the target spec's linker flavor and if that's not present emit an error?
TimNN
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #52268) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Ping from triage, @japaric: There are some review comments on your PR. |
|
It'll take me a while to get back to this so I'm going to close the PR in the meantime. |
|
re-r? @alexcrichton I believe I have addressed all your comments. |
japaric
commented
Aug 9, 2018
| return ret; | ||
| } | ||
|
|
||
| sess.fatal("Not enough information provided to determine how to invoke the linker"); |
There was a problem hiding this comment.
I believe, today, this is effectively unreachable because linker flavor is a mandatory field of target specifications. Should this be bug instead of fatal?
|
I realized (too late) that these changes are not enough to use rustc LLD with the What we could do to correctly support |
|
Ok looks great to me! I agree yeah changing the |
this field defaults to the LD / GNU flavor
|
@alexcrichton OK, done. r? |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
japaric
added
S-waiting-on-review
|
@bors: r+ 👍 |
|
📌 Commit 4bbedd7 has been approved by |
|
@bors r=alexcrichton |
|
📌 Commit 98e4cd5 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 98e4cd5 with merge 403324fa238c45b1739855f384b945c2c31045e4... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
expands logs
What? What happened? cc @rust-lang/infra |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 98e4cd5 with merge e2fdc33267255d50a849a7eae7aa87b56f38b6b8... |
|
💔 Test failed - status-appveyor |
bors
added
S-waiting-on-review
|
@bors r=alexcrichton |
|
📌 Commit a6f4ae8 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Aug 20, 2018
try to infer linker flavor from linker name and vice versa This is a second take on PR #50359 that implements the logic proposed in #50359 (review) With this change it would become possible to link `thumb*` binaries using GNU's LD on stable as `-C linker=arm-none-eabi-ld` would be enough to change both the linker and the linker flavor from their default values of `arm-none-eabi-gcc` and `gcc`. To link `thumb*` binaries using rustc's LLD on stable `-Z linker-flavor` would need to be stabilized as `-C linker=rust-lld -Z linker-flavor=ld.lld` are both required to change the linker and the linker flavor, but this PR doesn't propose that. We would probably need some sort of stability guarantee around `rust-lld`'s name and availability to make linking with rustc's LLD truly stable. With this change it would also be possible to link `thumb*` binaries using a system installed LLD on stable using the `-C linker=ld.lld` flag (provided that `ld.lld` is a symlink to the system installed LLD). r? @alexcrichton
|
☀️ Test successful - status-appveyor, status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a second take on PR #50359 that implements the logic proposed in #50359 (review)
With this change it would become possible to link
thumb*binaries using GNU's LD on stable as-C linker=arm-none-eabi-ldwould be enough to change both the linker and the linker flavor from their default values ofarm-none-eabi-gccandgcc.To link
thumb*binaries using rustc's LLD on stable-Z linker-flavorwould need to be stabilized as-C linker=rust-lld -Z linker-flavor=ld.lldare both required to change the linker and the linker flavor, but this PR doesn't propose that. We would probably need some sort of stability guarantee aroundrust-lld's name and availability to make linking with rustc's LLD truly stable.With this change it would also be possible to link
thumb*binaries using a system installed LLD on stable using the-C linker=ld.lldflag (provided thatld.lldis a symlink to the system installed LLD).r? @alexcrichton