Page MenuHomePhabricator

[LLD] Allow configuring default ld.lld backend
ClosedPublic

Authored by mati865 on Sep 9 2020, 2:56 PM.

Details

Summary

The motivation for this is ld.lld --help targeting MinGW which currently prints help for the ELF backend unless -m i386pe{,p} is added. This confuses build systems that grep through linker help to find supported flags.
This matches LD from Binutils which always prints help for MinGW when configured to target it.

After this change, the backend can still be overridden to any supported ELF/MinGW target by using correct -m <arch>.

Diff Detail

Event Timeline

mati865 created this revision.Sep 9 2020, 2:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2020, 2:56 PM
mati865 requested review of this revision.Sep 9 2020, 2:56 PM
mati865 added a reviewer: lld.
mati865 edited the summary of this revision. (Show Details)

As a concept, this looks sensible to me, similar to how llvm can be configured with a default triple or how clang can have cmake-configurable built-in defaults for linker/stdlib.

lld/tools/lld/lld.cpp
100

Wrap either side in StringRef() to get proper behaviour for the == - or maybe evaluate it to an int/bool already in cmake to avoid any actual runtime code for this? (Sorry for not picking up on this before.)

mati865 added inline comments.Sep 10 2020, 3:10 AM
lld/tools/lld/lld.cpp
100

Sorry for not picking up on this before.

No problem. I have totally missed the fact this is C string not C++ string.

maybe evaluate it to an int/bool already in cmake to avoid any actual runtime code

Defining something like LLD_DEFAULT_LD_LLD_BACKEND_MINGW from CMake is an easy fix but there are other ways to keep generic define: https://godbolt.org/z/c7dvf5

Which one should I use? For me strcmp() seems like good choice here.

mstorsjo added inline comments.Sep 10 2020, 3:38 AM
lld/tools/lld/lld.cpp
100

As both gcc and clang seem to optimize out the redundant strcmp(), that sounds like a good option to me too.

mati865 updated this revision to Diff 290975.Sep 10 2020, 7:52 AM
mati865 marked 2 inline comments as done.
MaskRay added inline comments.Sep 10 2020, 9:19 AM
lld/tools/lld/lld.cpp
100

return StringRef(LLD_DEFAULT_LD_LLD_BACKEND) == "MINGW";

mati865 added inline comments.Sep 10 2020, 9:39 AM
lld/tools/lld/lld.cpp
100

I don't see why this (small) runtime penalty is preferred but I'll update the diff later.

mstorsjo added inline comments.Sep 10 2020, 11:50 AM
lld/tools/lld/lld.cpp
100

StringRef is both very lightweight and transparent (it's essentially a <const char*, size_t> tuple) so it turns out the compiler can evaluate the whole operator invocation at compile time (tested with GCC 7), boiling down to the same constant as before.

mati865 updated this revision to Diff 291055.Sep 10 2020, 12:11 PM
mati865 marked 2 inline comments as done.

Thanks for the explanation.

mati865 updated this revision to Diff 291056.Sep 10 2020, 12:12 PM
MaskRay added inline comments.Sep 10 2020, 1:12 PM
lld/CMakeLists.txt
177

How about a boolean variable LLD_DEFAULT_LD_LLD_IS_MINGW?

Then you can write #if LLD_DEFAULT_LD_LLD_IS_MINGW, a Bazel based build system which does not need to this variable (my users) does not need any change.
Additionally, the gn build will not break.

pcc added a subscriber: pcc.Sep 10 2020, 1:35 PM

How widespread are these build systems that parse help output? (Given that it took until now to discover them, I'd venture "not very".) Maybe it would be better to fix them to explicitly pass -m and/or do something that doesn't rely on parsing help output (e.g. just try the flag and see whether it fails).

lld/CMakeLists.txt
177

The other build systems aren't really supported though, so we should probably do whatever makes sense for the cmake build and let the other build systems deal with it.

In D87418#2266652, @pcc wrote:

How widespread are these build systems that parse help output? (Given that it took until now to discover them, I'd venture "not very".) Maybe it would be better to fix them to explicitly pass -m and/or do something that doesn't rely on parsing help output (e.g. just try the flag and see whether it fails).

Well, lld on mingw is not very widely used yet - this has been long known (and I think the primary build system that does this is libtool, which is essentially unmaintained at this point).

In my llvm-mingw distribution I use a small wrapper around lld that passes -m, but for e.g. msys2, it would make sense to have an ld.lld.exe that on its own, without any options, defaults to the local native target, and would work just like ld.bfd.exe (which afaik works without an -m option) in that sense. Although in practice, except for such probing, most use of the linker is via the compiler driver...

MaskRay added inline comments.Sep 10 2020, 2:03 PM
lld/CMakeLists.txt
177

If mingw is an anomaly, I think a boolean variable makes more sense than a string variable with multiple values. The convenience for other build systems is a byproduct.

How widespread are these build systems that parse help output? (Given that it took until now to discover them, I'd venture "not very".)

This was well known limitation but also seen as "not a big deal" because LLD wasn't used broadly.

Basically any package at https://github.com/msys2/MINGW-packages which uses autotools build system is more or less affected. It's possible that MSYS2 will soon get new subsystem that uses LLVM project as the main compiler.

Maybe it would be better to fix them to explicitly pass -m and/or do something that doesn't rely on parsing help output (e.g. just try the flag and see whether it fails).

Patching configure script of all affected packages every time they are updated is enormous task.
It basically comes to downstream vs upstream patches.

for e.g. msys2, it would make sense to have an ld.lld.exe that on its own, without any options, defaults to the local native target, and would work just like ld.bfd.exe (which afaik works without an -m option) in that sense

It also often confuses users who expect ld.lld --help to print MinGW help just like ld.bfd --help does.

lld/CMakeLists.txt
177

That was my initial attempt...

I thought the other targets in the future could need the same thing so I replaced it with generic solution.

I'm fine either way.

MaskRay added inline comments.Sep 10 2020, 2:41 PM
lld/CMakeLists.txt
177

This change only affects ld.lld (the Gnu flavor). AFAIK there are just two possible values: ELF and MinGW. So a boolean variable works the best.

mati865 updated this revision to Diff 291175.Sep 11 2020, 3:54 AM

Updated the diff.

mati865 marked 4 inline comments as done.Sep 11 2020, 3:54 AM
MaskRay accepted this revision.Sep 14 2020, 1:46 PM

LGTM.

This revision is now accepted and ready to land.Sep 14 2020, 1:46 PM
This revision was automatically updated to reflect the committed changes.

Pushed this one now.

Just FTR, keep in mind that if LLD is configured this way, most existing ELF tests will fail, as they don't pass any explicit -m parameter. Not sure if it's worth adding such an option to all of them, or just require building with the option in the default state if one wants to work on LLD and actually run tests.

Pushed this one now.

Just FTR, keep in mind that if LLD is configured this way, most existing ELF tests will fail, as they don't pass any explicit -m parameter. Not sure if it's worth adding such an option to all of them, or just require building with the option in the default state if one wants to work on LLD and actually run tests.

Not worth changing all RUN lines of all test/ELF/ tests. You can probably use lit.cfg.py to mark all test/ELF tests unsupported if LLD_DEFAULT_LD_LLD_IS_MINGW.