[LLD] Allow configuring default ld.lld backend

[mati865]

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>.

[mstorsjo]

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]

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]

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.

[MaskRay]

lld/tools/lld/lld.cpp

100

return StringRef(LLD_DEFAULT_LD_LLD_BACKEND) == "MINGW";

[mati865]

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]

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]

Thanks for the explanation.

[MaskRay]

lld/CMakeLists.txt

196

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]

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

196

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.

[mstorsjo]

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]

lld/CMakeLists.txt

196

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.

[mati865]

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

196

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]

lld/CMakeLists.txt

196

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 the diff.

[mstorsjo]

Ping @MaskRay

[MaskRay]

LGTM.

[mstorsjo]

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.

[MaskRay]

In D87418#2273236, @mstorsjo wrote:

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.

lld/CMakeLists.txt

if (VTUNE_FOUND)
     list(APPEND LLVM_COMMON_LIBS ${VTune_LIBRARIES})
     add_definitions(-DLLD_HAS_VTUNE)
   endif()
 endif()
 
 option(LLD_BUILD_TOOLS
   "Build the lld tools. If OFF, just generate build targets." ON)
 
+option(LLD_DEFAULT_LD_LLD_IS_MINGW
+    "Use MinGW as the default backend for ld.lld. If OFF, ELF will be used." OFF)
+if (LLD_DEFAULT_LD_LLD_IS_MINGW)
+  add_definitions("-DLLD_DEFAULT_LD_LLD_IS_MINGW=1")
+endif()
+
 if (MSVC)
   add_definitions(-wd4530) # Suppress 'warning C4530: C++ exception handler used, but unwind semantics are not enabled.'
   add_definitions(-wd4062) # Suppress 'warning C4062: enumerator X in switch of enum Y is not handled' from system header.
 endif()
 
 include_directories(BEFORE
   ${CMAKE_CURRENT_BINARY_DIR}/include
   ${CMAKE_CURRENT_SOURCE_DIR}/include

lld/tools/lld/lld.cpp

static bool isPETarget(std::vector<const char *> &v) {
   // to allow detecting the -m argument from arguments in them.
   SmallVector<const char *, 256> expandedArgs(v.data(), v.data() + v.size());
   cl::ExpandResponseFiles(saver, getDefaultQuotingStyle(), expandedArgs);
   for (auto it = expandedArgs.begin(); it + 1 != expandedArgs.end(); ++it) {
     if (StringRef(*it) != "-m")
       continue;
     return isPETargetName(*(it + 1));
   }
+
+#ifdef LLD_DEFAULT_LD_LLD_IS_MINGW
+  return true;
+#else
   return false;
+#endif
 }
 
 static Flavor parseProgname(StringRef progname) {
   // Use GNU driver for "ld" by default.
   if (progname == "ld")
     return Gnu;
 
   // Progname may be something like "lld-gnu". Parse it.