[cfe-dev] [RFC] Recognize -fuse-ld={bfd, gold, lld} but don't prepend "ld." or "ld64." for other values

[David Blaikie via cfe-dev]

On Tue, Jun 2, 2020 at 12:24 PM Fangrui Song <maskray at google.com> wrote:
>
> On 2020-06-02, David Blaikie wrote:
> >On Tue, Jun 2, 2020 at 11:47 AM Fangrui Song <maskray at google.com> wrote:
> >>
> >> On 2020-06-02, David Blaikie wrote:
> >> >On Mon, Jun 1, 2020 at 9:47 PM James Y Knight via cfe-dev
> >> ><cfe-dev at lists.llvm.org> wrote:
> >> >>
> >> >>
> >> >>
> >> >> On Wed, May 20, 2020 at 3:10 PM Fangrui Song via cfe-dev <cfe-dev at lists.llvm.org> wrote:
> >> >>>
> >> >>> In https://reviews.llvm.org/D80225 I intended to update the semantics of -fuse-ld=
> >> >>> Some context (from my archaeology) about the option
> >> >>>
> >> >>> * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55470 added support for -fuse-ld=bfd to mean ld.bfd and -fuse-ld=gold to mean ld.gold
> >> >>> * rL194328 ported the feature and made -fuse-ld= available for other values (with the ld. prefix)
> >> >>> * D78290 changed the prefix to ld64. on Darwin.
> >> >>> * GCC added support for -fuse-ld=lld. No other value than bfd,gold,lld is allowed (so our
> >> >>>    behavior change here will not be incompatible with GCC).
> >> >>>
> >> >>> Our existing behavior is cumbersome:
> >> >>>
> >> >>> * Inconsistency between an absolute
> >> >>>    path (which does not get a ld.  prefix) and a relative path (which gets
> >> >>>    a ld. prefix) For a relative path, -fuse-ld=dir/foo currently tries to
> >> >>>    access x86_64-unknown-linux-gnu-ld.dir/foo (if
> >> >>>    LLVM_DEFAULT_TARGET_TRIPLE is x86_64-unknown-linux-gnu).
> >> >>> * lld's new Mach-O port needs to pick an executable name. It would be
> >> >>>    nice if -fuse-ld= simply accepts a program name, not necessarily prefixed by `ld64.`
> >> >>>
> >> >>> I suggest we just hard code the currently used values which intend to get
> >> >>> a prefix: bfd, gold, lld. For all other values, don't add a prefix.
> >> >>> (PS4 seems to use -fuse-ld=ps4 but it overrides ToolChain::GetLinkerPath, thus unaffected)
> >> >>>
> >> >>> Thoughts?
> >> >>
> >> >>
> >> >> It concerns me that we currently claim to accept arbitrary file paths for -fuse-ld=, yet, we sometimes hardcode special behavior for certain argument values. (Special behavior beyond simply changing which executable name to search for, that is). If we're going to have such conditionals, GCC's behavior of allowing only certain enumerated values would seem to me to be better.
> >> >>
> >> >> However, currently, all of the cases in llvm appear to be either related to Windows toolchains or PS4 toolchain. E.g. here https://github.com/llvm/llvm-project/blob/3bb0d95fdc2fffd193d39d14f2ef421d4b468617/clang/lib/Driver/Driver.cpp#L4890 and https://github.com/llvm/llvm-project/blob/3bb0d95fdc2fffd193d39d14f2ef421d4b468617/clang/lib/Driver/ToolChains/MinGW.cpp#L424 (just search for OPT_fuse_ld_EQ to find others.)
> >> >
> >> >Sounds like fixing those is independent of the change you're
> >> >proposing, though? I'm not sure I see it as related to this proposal.
> >>
> >> It is independent.
> >>
> >> >> And the PS4 toolchain driver code already prohibits any -fuse-ld= other than the exact strings "ps4" or "gold", so there's no weird behavior changes, only a clear (yet perhaps unexpected) error message. So, maybe we should do the same for windows? But it's rather unfortunate for clang to inconsistently sometimes allow paths as the argument to -fuse-ld= and sometimes not...
> >> >
> >> >Not sure I follow these pieces. Re: PS4, it's good to know it's not an
> >> >issue/wouldn't be affected by this change. But that doesn't mean other
> >> >users out there aren't benefiting from/relying on the ld. prefix -
> >> >it's functionality LLVM has shipped, users can use, etc, and we would
> >> >be changing that out from under them which I think is always something
> >> >that should be considered carefully as to what we gain by changing
> >> >user-facing behavior.
> >>
> >> If they use a relative path, they can't be benefiting from ld. prefix
> >>
> >> With the magic ld. prefix,
> >>
> >>    -fuse-ld=path1/path2/foo -> ld.path1/path2/foo
> >
> >OK, I'm confused - it seems we've gone back and forth on this, or at
> >least I have, in understanding what happens with relative paths.
> >
> >You're saying, currently relative paths always get the prefix? Or does
> >clang test if "ld.path1/path2/foo" exists, and if it doesn't exist it
> >falls back to "path1/path2/foo"?
>
> No fallback. A relative path does not really work. See below.

Ah, OK - thanks for the clarification.

> >I'd say that's "fairly supported",
> >but not ideal - we could test if the path has any path separators in
> >it, then never use the prefix if it does. That way it doesn't
> >accidentally pick up a weird path if it does exist. Yes, this is a
> >break in functionality for some potential users - but I think the
> >possibility of a user intentionally using "ld.path1/path2/foo" is
> >small compared to the possibility of a user using "ld.foo"
> >intentionally (since that's already shown as intentional with bfd,
> >gold, and lld - so a user might see that and add their own in a
> >similar way).
>
> -fuse-ld=/abs/path only stats the absolute path /abs/path
>
> -fuse-ld=word stats x86_64-unknown-linux-gnu-ld.word , then
> ld.word .
>
> -fuse-ld=dir/path stats x86_64-unknown-linux-gnu-ld.dir/path
>
> x86_64-unknown-linux-gnu-ld.dir is unlikely to be a directory.

More interested in which binary it runs - but I guess when you say
"stats" you mean "Checks if it exists, and if it does, runs it as the
linker"?

> >> If they use an absolute path, they are unaffected.
> >>
> >> If they use a bare word, e.g. -fuse-ld=meow , they need to change it to
> >> -fuse-ld=ld.meow now.
> >>
> >> (1) GCC does not support arbitrary -fuse-ld=bare-word
> >> (2) absolute/relative paths are not affected
> >
> >^ this confuses me. You said above that " -fuse-ld=path1/path2/foo ->
> >ld.path1/path2/foo" is happening, so removing the prefix seems like it
> >would affect relative paths. But you're saying it won't?
>
> An absolute path is unaffected by the patch.
>
> A relative path does not reall work (diverged a lot from users' expection). Changing its behavior should not matter.
>
> >> (3) bare word can easily be adapted
> >>
> >> => conclusion: if this (ever) affects any user, the troublesome should be small enough.
> >
> >My concern would be mostly for users where it might silently pick a
> >different linker or get different linker semantics. Yes, that's
> >probably a fairly small set of users, if any. (a user with "ld.foo"
> >being a different version than "foo" would be surprising, and a user
> >with "foo" providing different semantics than "ld.foo" might be more
> >likely but still small (a busyboxed linker that doesn't default to the
> >semantics of the system they're on - eg: imagine if "lld2" did COFF By
> >default, but did ELF when using "ld.lld2"  - changing "-fuse-ld=lld2"
> >from running "ld.lld2" to running COFF lld would be pretty
> >surprising/confusing/etc)) But, yes, if you got the wrong linker
> >semantics, it'd probably be broken enough you wouldn't get far/would
> >have to go figure out what went wrong and add the "ld." to your
> >-fuse-ld command.
>
> Getting different linker semantics will fail immediately. The magic is
> incorrect -> the linker should immediately report an error.
> It is difficult to cause the linker to silently accept invalid input.

Fair point - though still seems like a somewhat confusing thing for
users, but probably a fairly small slice of users.

> >> The RFC serves the purpose seeking for any such (proprietary linker)
> >> user. Such objection hasn't emerged yet. If they don't speak up, I am
> >> not sure we should wait indefinitely making -fuse-ld= sane and
> >> consistent.
> >
> >I don't think it's not sane, really.
> >
> >And there are many more LLVM users than read these mailing lists,
> >unfortunately - some amount of "this feature is in the wild and may be
> >used by any number of users in interesting ways" is something we
> >should consider both when creating features and changing them.
> >
> >- David
>
> I think I have done anything I can to make the impact as small as
> possible. We can't indefinitely support hypothetical use cases because
> we simply can't.

This feature doesn't seem particularly costly to support, though?

If the original goal was to support relative paths - I think a
narrower fix of testing whether the path contains path separators &
only adding the prefix if it doesn't have path separators would be
workable. (but, sure, possibly not worth bothering with)
If one of the goals is to get consistency with GCC and GCC does not
want to implement the generalized prefix support (was that proposed to
and rejected by GCC?), yeah - I'd be OK with breaking the
small/hypothetical use case to gain compatibility with GCC, for
instance.

Perhaps you could summarize the motivations/goals/path/tradeoffs?
(where'd you start, what proposals did you make to what communities &
how did that inform/change the direction, if at all, etc)

> Last but not the least, https://reviews.llvm.org/D80225 does not deprive
> their rights to contribute their own -fuse-ld=word . They can still make
> the change
>
> http://clang.llvm.org/get_involved.html

My concern would be that would be another potential break/change for
users that could make things more confusing. It'd be good to make
choices we can be fairly committed to going forward. (which, I don't
think what your proposing is impossible to commit to - but I have some
doubts/want to check what the alternatives might be)

>
> > A specific need to reside within the Clang tree: There are some
> > extensions that would be better expressed as a separate tool, and should
> > remain as separate tools even if they end up being hosted as part of the
> > LLVM umbrella project.