Use the linker directly on darwin,ios

[tamird]

See individual commits. r? @alexcrichton cc @japaric

(pnkfelix adds: This is taking steps towards addressing #11937)

[kennytm]

This caused several errors on the CI which is running Linux 😕

[00:59:08] failures:
[00:59:08] [run-make] run-make/fpic
[00:59:08] [run-make] run-make/print-cfg
[00:59:08] [run-make] run-make/simd-ffi

[tamird]

Yep! Luckily I am able to reproduce these locally on macOS, so I'm investigating now.

…

On Sat, Nov 25, 2017 at 9:47 AM, kennytm ***@***.***> wrote: This caused several errors on the CI which is running Linux 😕 [00:59:08] failures: [00:59:08] [run-make] run-make/fpic [00:59:08] [run-make] run-make/print-cfg [00:59:08] [run-make] run-make/simd-ffi — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#46255 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsPLc6Xd2otGu9Fs-FjtVovaHLbEelks5s6CiSgaJpZM4QqauE> .

[tamird]

I was able to repro run-make/fpic locally, and the other two were caused by an errant unwrap call which assumed that xcrun is always available (which is not the case when cross-compiling). All three should be fixed now.

[tamird]

cc @TimNN

[alexcrichton]

Thanks for this, it looks pretty cool!

My main worry here would be around the area of regressions. For example this would break anyone using -C link-arg on OSX as the link arguments would need to be passed differently. I think it may be best to do this as a trial run instead of a "immediately change the defaults".

For example I think we should enable -Z linker-flavor=ld to "just work" on OSX. We'd leave the default linker flavor as gcc but we could then have the necessary code in the compiler to switch to ld if necessary.

Switching the iOS targets seems reasonable but we may wish to pursue the same strategy for now and then post on internals about how everyone's using -C link-args and OSX-related things.

[TimNN]

Hey @tamird, do you need anything particular from me or was the cc just an FYI?

[tamird]

@TimNN nah, just thought you might have some opinions on iOS stuff (as I recall you've done some work in that area in the past).

@alexcrichton sounds totally reasonable! one thing that was attractive about making ld the default was that it allowed me to build stage1 and confirm that linking actually worked, but perhaps there is a better way to do this? Specifically, how do I get bootstrap to pass custom flags to rustc (in this case I'd want -Z --linker-flavor=ld -C linker=ld)?

[alexcrichton]

@tamird yeah I'm quite surprised this worked to bootstrap! (that's awesome!)

We could always tweak our own bootstrap to use this linker by default and avoid cc (seems like a good idea to me) but I think we'll want to leave the default for everyone else as cc for now (until we at least message it out and canvas opinions).

I was also thinking that right now the default linker is cc sort of unconditionally but we can probably infer it based on the linker flavor. That is if you only specify -Z linker-flavor=ld then that should imply, on OSX, that you use a default linker of ld

[tamird]

So this is kind of a bummer. In order to make ld "just work" on osx, we need to teach all the targets the correct ld args, which in general are just the cc args with some stuff removed.

I say it's a bummer because it makes me think that "linker flavor", as implemented in rustc, is not a particularly good abstraction because generally (always?) there is only one linker on each platform, which we currently always front with cc. However, nearly all the flags we pass to cc have a direct mapping to an ld flag.

Here's my proposal: we should remove linker flavor and go back to having a list of linker args per target as before, only this time the linker args will all be the ld variant, rather than the cc variant. Then, we can check at link time if the executable we're calling "looks like" cc or ld (or lld, or whatever) and map the ld flags to cc flags. Unfortunately, this will be a breaking change, as you say, with respect to how link args are pass on the command line today (since they'll need to be in ld form).

We could also do this in the reverse direction (mapping cc flags to ld flags), I think, but I imagine it'll be somewhat uglier.

What do you think?

[alexcrichton]

Sorry I don't currently have the time to evaluate what appears to be major refactorings of the backends and how we pass arguments around, it would be good to consult other compiler team members with these sorts of questions right now.

[kennytm]

Reassigning to another person randomly on the compiler team as @alexcrichton doesn't have time to evaluate this.

r? @pnkfelix

[carols10cents]

Review ping for you @pnkfelix !

[pnkfelix]

This PR looks fine to me, but I'm not an expert in this domain (in terms of the issues that need to be addressed for #46255

[pnkfelix]

Reading the dialogue on this PR more carefully, I now realize I was a little too blasé in my pseudo-review (aka rubberstamp) above.

Namely, addressing #46255 is not sufficiently high priority to justify injecting the kinds of breaking changes that @alexcrichtonhas described.

@alexcrichton gave some suggestions for how to revise the PR so that it would not inject a breaking- change to stable code.

@tamird responded, but it seemed like their alternative approach is still going to inject breakage onto stable clients? (Did I misread their comment that outlined an alternative strategy to what @alexcrichton had suggested?)

So, high-level points + questions:

• Clearly one should not r+ this PR as it is written currently.
• The question is what to do next.
• Do we actually care about addressing Link using the linker directly #11937? It seems like @tamird has a personal investment in addressing it (I'm not clear what actual problem of theirs it is addressing, but I'll assume they have a good reason beyond just architectural cleanliness...)
• (Related to the previous bullet: Why is the strategy of using -Z --linker-flavor=ld -C linker=ld to accomplish the goal of Link using the linker directly #11937 not acceptable? Because we want to ensure that stable clients do get some way to opt into this?)
• If we are serious about actually addressing Link using the linker directly #11937, how can we do so without inject a breaking-change for clients of the stable channel who are are today already using -C link-arg?
  • Specifically: I don't think the strategy outlined by@tamird would get r+'ed by the compiler team, because of it being a breaking change.

Update: sigh, everywhere that I wrote #46255 I had meant to write #11937; sorry all...

[kennytm]

Triage ping @tamird. Do you have answers to @pnkfelix's questions above?

[tamird]

Why is the strategy of using -Z --linker-flavor=ld -C linker=ld to accomplish the goal of #46255 not acceptable?

Because it doesn't always work - if the target platform requires more flags, the user has to specify them all. The (reduced) goal is to be able to pass exactly those flags and have most platforms just work.

The other questions aren't for me to answer.

[kennytm]

Should this be raised to the team for discussion? (Due to holiday this will likely need to wait until January)

[tamird]

Seems that way to me.

…

On Wed, Dec 20, 2017 at 10:18 AM, kennytm ***@***.***> wrote: Should this be raised to the team for discussion? (Due to holiday this will likely need to wait until January) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#46255 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsPOZz8TelY85OagZy1T12Iqofox72ks5tCSUwgaJpZM4QqauE> .

[kennytm]

Nominated @rust-lang/compiler.

[kennytm]

Triage ping @rust-lang/compiler! Just to remind you all about this PR 😁.

[nikomatsakis]

We discussed this in the @rust-lang/compiler meeting. Conclusion was:

• We can land this PR, but only when it is made opt-in and feature-gated.
• We should open a tracking issue for the relevant feature.
  • I personally think that before stabilization, I'd like to see an RFC, or at least some strong evidence that we've covered all the use cases people might want here.

[shepmaster]

Triage ping for @tamird — looks like there is some feedback the team would like to see before this PR lands; will you have time to implement them?

[shepmaster]

Thanks for the contribution, @tamird ! Unfortunately, since we haven't heard from you in a few weeks, I'm going to go ahead and close this to keep the queue tidy. Please fee free to re-open it when you have time to address the most recent feedback!

[alexcrichton]

@tamird#48125 is doing most of the refactoring that I was thinking about, would you be up for rebasing this once that lands? I think with that we shoudl be able to get all of the following working by default:

$ rustc foo.rs
$ rustc foo.rs -Z linker-flavor=ld
$ rustc foo.rs -Z linker-flavor=ld64.ll

where there we're using the system linker, the ld linker itself, and then finally the bundled version of lld. I'd imagine that with the fixes here those could all work! (and the latter would be great for benchmarking when needed)

[tamird]

@alexcrichton sure! Thanks for making me aware of that PR; I'll follow along and pick this up when that's merged.