Propagate -fdiagnostics-color to LLD.

[ruiu]

Summary

Some build systems such as Ninja sets -fdiagnostics-color to Clang
command line so that errors are displayed in color. However, linker
errors were displayed without color because the compiler driver
didn't pass that parameter to linkers.

This is a patch to set -color-diagnostics=all if a linker is LLD.
LLD supports that option and displays errors in color.

I'm not totally sure if this is the right place to add new code,
but it seems we have many copy-and-pasted code to construct a linker
command line in this file, and if I fixed each of them, I'd have had
to update more than 10 locations.

[hans]

lib/Driver/Tools.cpp

234

Yes, this doesn't seem like exactly the right place, but maybe there is no better one. Perhaps we could add an "addCommonLinkerFlags" function?

286

This needs a test.

And can we get color diagnostics from lld-link.exe too? That would be nice.

[rnk]

lib/Driver/Tools.cpp

283

nit: Clang's original spelling was -fcolor-diagnostics, which reads better in English. GCC made -fdiagnostics-color, which we added as an alias. It makes sense ontologically, but it doesn't read as well in a comment.

[ruiu]

lib/Driver/Tools.cpp

234

The other way of propagating this is to use an environment variable. Because we have control of both the compiler and the linker, we can make a change to LLD to interpret something like LLD_COLOR_DIAGNOSTICS=1 and set it in Clang.

The point is that in that way we can still have the exact same command line for bfd/gold/lld. What do you think?

[hans]

lib/Driver/Tools.cpp

234

I don't think Clang passes things via the environment to any other tool. I think passing a flag is better.

[rsmith]

lib/Driver/Tools.cpp

284

I don't think this will work for -fuse-ld=$BINDIR/lld and the like. We also have code around Tools.cpp doing things like this:

const char *Exec = Args.MakeArgString(ToolChain.GetLinkerPath());
if (llvm::sys::path::filename(Exec) == "lld") {

and

const char *Exec = Args.MakeArgString(ToolChain.GetLinkerPath());
if (llvm::sys::path::stem(Exec).equals_lower("lld")) {

so detecting ld.lld here seems inconsistent. (Meanwhile, the above checks won't fire for -fuse-ld=lld, since that sets the linker path to .../ld.lld.)

I think it's time to factor out an isLinkerLLD function and use that to detect whether we're using lld.

[ruiu]

lib/Driver/Tools.cpp

284

What if a linker is just /usr/bin/ld? I want to detect if it's LLD or not, but it's not doable without running that command. (If we pass it via the environment, we can always set LLD_COLOR_DIAGNOSTICS=1 though.)

[rsmith]

lib/Driver/Tools.cpp

284

Maybe the best we can do is have each target tell us what it expects /usr/bin/ld to be, and/or readlink it and see if it is a symlink to a binary named lld. This doesn't seem like a very satisfying answer, though :(

We already pass some flags if we think we're running lld (for instance, -flavor gnu -target $TARGET); passing those through the environment doesn't seem ideal.

[ruiu]

Nico suggested I make change to CMake instead of Clang so that CMake adds -Wl,-color-diagnostics if it detects the linker can accept that option. I think that makes sense, so I'll look into it.

include/clang/Basic/Diagnostic.h

public:
   /// \brief Set tree printing, to outputting the template difference in a
   /// tree format.
   void setPrintTemplateTree(bool Val = false) { PrintTemplateTree = Val; }
   bool getPrintTemplateTree() { return PrintTemplateTree; }
  
   /// \brief Set color printing, so the type diffing will inject color markers
   /// into the output.
   void setShowColors(bool Val = false) { ShowColors = Val; }
-  bool getShowColors() { return ShowColors; }
+  bool getShowColors() const { return ShowColors; }
 
   /// \brief Specify which overload candidates to show when overload resolution
   /// fails.
   ///
   /// By default, we show all candidates.
   void setShowOverloads(OverloadsShown Val) {
     ShowOverloads = Val;
   }

lib/Driver/Tools.cpp

if (CombinedArg) {
       CmdArgs.push_back(ArgName);
       CmdArgs.push_back(Args.MakeArgString(Dirs));
     }
   }
 }
 
 static void AddLinkerInputs(const ToolChain &TC, const InputInfoList &Inputs,
                             const ArgList &Args, ArgStringList &CmdArgs,
                             const JobAction &JA) {
   const Driver &D = TC.getDriver();
 
   // Add extra linker input arguments which are not treated as inputs
   // (constructed via -Xarch_).
   Args.AddAllArgValues(CmdArgs, options::OPT_Zlinker_input);
 
   for (const auto &II : Inputs) {
     // If the current tool chain refers to an OpenMP offloading host, we should
else if (A.getOption().matches(options::OPT_z)) {
       A.renderAsInput(Args, CmdArgs);
     }
   }
 
   // LIBRARY_PATH - included following the user specified library paths.
   //                and only supported on native toolchains.
   if (!TC.isCrossCompiling())
     addDirectoryList(Args, CmdArgs, "-L", "LIBRARY_PATH");
+
+  // Add an extra parameter for LLD. If the linker is LLD, we want to
+  // propagate -fdiagnostics-color.
+  if (StringRef(TC.GetLinkerPath()).endswith("ld.lld") &&
+      D.getDiags().getShowColors())
+    CmdArgs.push_back("-color-diagnostics=always");
 }
 
 /// Add OpenMP linker script arguments at the end of the argument list so that
 /// the fat binary is built by embedding each of the device images into the
 /// host. The linker script also defines a few symbols required by the code
 /// generation so that the images can be easily retrieved at runtime by the
 /// offloading library. This should be used only in tool chains that support
 /// linker scripts.