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

rustc 1.30.0's linker flavor inference is a non-backwards compat change to -Clinker #55396

Closed
jsgf opened this issue Oct 26, 2018 · 8 comments
Closed
Assignees
Labels
P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jsgf
Copy link
Contributor

jsgf commented Oct 26, 2018

In our environment we end up setting -Clinker to a script called ld.sh; this script is implemented in terms of gcc as rustc used to expect. Now it treats it as a direct invocation of ld.

This was introduced in #52101.

I think rather than inferring the linker flavor from the name of the executable, rustc should either:

  1. invoke the binary with --version to try to work out what its dealing with, and/or
  2. have a stable -Clinker-flavor option to set it explicitly

There's an unstable -Zlinker-flavor option, which I think should have been stabilized at the same time as flavor inference. Ideally inference should be opt-in with -Clinker-flavor=infer, with the default being the old default of -Clinker-flavor=gcc (at least for Linux platforms).

@alexcrichton
Copy link
Member

Oh dear, sorry for the breakage! I think there's probably a few things we can do to help mitigate this:

  • For backwards compatibility, inference should only chop off *.exe and not other extensions when inferring a flavor. I think we'll still want to infer the flavor by default as it fixed a number of issues and empowered a number of embedded use cases to become stable. I also think we want to avoid running --version as running a binary has quite a bit of overhead unfortunately
  • Simultaneously I think we should likely stabilize -C linker-flavor as mentioned here to allow explicit configuration via the command line.

@jsgf does it work for now for y'all to rename to gcc.sh? Do the above steps sound ok to you?

Also, cc @japaric

@jsgf
Copy link
Contributor Author

jsgf commented Oct 26, 2018

The script is currently called cxx-gcc-5-glibc-2.23-clang-ld.sh so I wonder if the inference heuristic should be tweaked a bit. I think in the meantime I'll try adding -Zlinker-flavor=gcc rather than adjusting the name of the script.

@nagisa nagisa added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label Oct 27, 2018
@alexcrichton
Copy link
Member

@jsgf ah yeah the heuristic includes looking for -ld at the end, and we chop off the file extension to handle .exe on Windows. You should be able to work around it locally by renaming to -gcc.sh.

@pnkfelix pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 8, 2018
@pnkfelix
Copy link
Member

triage: P-high.

@pnkfelix pnkfelix added the P-high High priority label Nov 29, 2018
@pnkfelix
Copy link
Member

assigning to @nagisa to take point on moving forward on how we're going to address this

@davidtwco davidtwco self-assigned this Nov 29, 2018
pietroalbini added a commit to pietroalbini/rust that referenced this issue Nov 29, 2018
…ension, r=nagisa

rustc 1.30.0's linker flavor inference is a non-backwards compat change to -Clinker

Part of rust-lang#55396.

This commit modifies linker flavor inference to only remove the extension
to the linker when performing inference if that extension is a 'exe'.

r? @nagisa
cc @alexcrichton @japaric
kennytm added a commit to kennytm/rust that referenced this issue Nov 30, 2018
…ension, r=nagisa

rustc 1.30.0's linker flavor inference is a non-backwards compat change to -Clinker

Part of rust-lang#55396.

This commit modifies linker flavor inference to only remove the extension
to the linker when performing inference if that extension is a 'exe'.

r? @nagisa
cc @alexcrichton @japaric
kennytm added a commit to kennytm/rust that referenced this issue Nov 30, 2018
…ension, r=nagisa

rustc 1.30.0's linker flavor inference is a non-backwards compat change to -Clinker

Part of rust-lang#55396.

This commit modifies linker flavor inference to only remove the extension
to the linker when performing inference if that extension is a 'exe'.

r? @nagisa
cc @alexcrichton @japaric
@pnkfelix
Copy link
Member

T-compiler triage: this has one work-item PR (#56349) that landed, and then the final step is in PR #56351, which has gotten approval via rfcbot so I think all that remains is to bors r+ PR #56351 ?

kennytm added a commit to kennytm/rust that referenced this issue Dec 13, 2018
…ker-flavor, r=nagisa

Stabilize `linker-flavor` flag.

Part of rust-lang#55396.

This commit moves the linker-flavor flag from a debugging option to a
codegen option, thus stabilizing it. There are no feature flags
associated with this flag.
bors added a commit that referenced this issue Dec 14, 2018
… r=nagisa

Stabilize `linker-flavor` flag.

Part of #55396.

This commit moves the linker-flavor flag from a debugging option to a
codegen option, thus stabilizing it. There are no feature flags
associated with this flag.

r? @nagisa
@pnkfelix
Copy link
Member

T-compiler triage. PR #56351 landed, so closing this as fixed.

@jsgf
Copy link
Contributor Author

jsgf commented Dec 28, 2018

Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants