[Hexagon] Avoid passing unsupported options to lld when -fuse-ld=lld is used

[sidneym]

Summary

Avoid passing unsupported options to lld when -fuse-ld=lld is used.

[kparzysz]

clang/test/Driver/hexagon-toolchain-elf.c

560

I don't think there will ever be "-mcpu=hexagon" (including the quotation marks). Maybe this should just check for any -march and -mcpu.

[sidneym]

Update testcase.

[bcain]

clang/lib/Driver/ToolChains/Hexagon.cpp

212

Does this still work when -fuse-ld=ld.lld ? How about absolute paths -fuse-ld=/path/to/lld?

[sidneym]

Remove quotes around check-not.

-fuse-ld=lld is the correct usage. -fuse-ld=ld.lld results in an error message:
error: invalid linker name in argument '-fuse-ld=ld.lld'

[ruiu]

In D70919#1771198, @sidneym wrote:

Remove quotes around check-not.

-fuse-ld=lld is the correct usage. -fuse-ld=ld.lld results in an error message:
error: invalid linker name in argument '-fuse-ld=ld.lld'

-fuse-ld accepts an absolute path to a linker, so you can pass for example -fuse-ld=/absolute/path/to/ld.lld.

[MaskRay]

clang/lib/Driver/ToolChains/Hexagon.cpp

212

Fuchsia uses:

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

[sidneym]

make check for lld more generic.

[sidneym]

OK, Yes Fuchsia is a good example. Using that pattern

[bcain]

LGTM

[kparzysz]

clang/test/Driver/hexagon-toolchain-elf.c

560

This is still wrong. Assume that we are passing -mcpu=hexagonv60 to lld. The options will show up with quotation marks, and so it will be "-mcpu=hexagonv60". The testcase is checking for "-mcpu" which will not match "-mcpu=hexagonv60", and the testcase will pass even though it shouldn't.

[sidneym]

Remove quotes.

Also add ld.lld file so that configurations that don't enable lld will still pass this test.

[leonardchan]

Hi! This seems to be causing some test failures on our bots: https://ci.chromium.org/p/fuchsia/builders/ci/clang-linux-x64/b8893521019883752048

: 'RUN: at line 553';   /b/s/w/ir/k/recipe_cleanup/clangciZYdn/llvm_build_dir/bin/clang -### -target hexagon-unknown-elf    -ccc-install-dir /b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/hexagon_tree/Tools/bin    -mcpu=hexagonv60    -fuse-ld=lld    /b/s/w/ir/k/llvm-project/clang/test/Driver/hexagon-toolchain-elf.c 2>&1    | /b/s/w/ir/k/recipe_cleanup/clangciZYdn/llvm_build_dir/bin/FileCheck -check-prefix=CHECK082 /b/s/w/ir/k/llvm-project/clang/test/Driver/hexagon-toolchain-elf.c
--
Exit Code: 1

Command Output (stderr):
--
/b/s/w/ir/k/llvm-project/clang/test/Driver/hexagon-toolchain-elf.c:548:14: error: CHECK081: expected string not found in input
// CHECK081: "-march=hexagon"
             ^
<stdin>:1:1: note: scanning from here
Fuchsia clang version 10.0.0 (https://fuchsia.googlesource.com/a/third_party/llvm-project ddf897fc80499ece298bc33201db6b697d2af50e)
^
<stdin>:5:193: note: possible intended match here
 "/b/s/w/ir/k/recipe_cleanup/clangciZYdn/llvm_build_dir/bin/clang-10" "-cc1" "-triple" "hexagon-unknown-unknown-elf" "-emit-obj" "-mrelax-all" "--mrelax-relocations" "-disable-free" "-main-file-name" "hexagon-toolchain-elf.c" "-mrelocation-model" "static" "-mthread-model" "posix" "-mframe-pointer=all" "-fmath-errno" "-fno-rounding-math" "-masm-verbose" "-mconstructor-aliases" "-target-cpu" "hexagonv60" "-target-feature" "-long-calls" "-mqdsp6-compat" "-Wreturn-type" "-fshort-enums" "-mllvm" "-machine-sink-split=0" "-dwarf-column-info" "-fno-split-dwarf-inlining" "-debugger-tuning=gdb" "-resource-dir" "/b/s/w/ir/k/recipe_cleanup/clangciZYdn/llvm_build_dir/lib/clang/10.0.0" "-internal-externc-isystem" "/b/s/w/ir/k/llvm-project/clang/test/Driver/Inputs/hexagon_tree/Tools/bin/../target/hexagon/include" "-fdebug-compilation-dir" "/b/s/w/ir/k/recipe_cleanup/clangciZYdn/llvm_build_dir/tools/clang/test/Driver" "-ferror-limit" "19" "-fmessage-length" "0" "-fshort-enums" "-fno-signed-char" "-fgnuc-version=4.2.1" "-fobjc-runtime=gcc" "-fdiagnostics-show-option" "-faddrsig" "-o" "/b/s/w/ir/tmp/t/hexagon-toolchain-elf-2999dd.o" "-x" "c" "/b/s/w/ir/k/llvm-project/clang/test/Driver/hexagon-toolchain-elf.c"
                                                                                                                                                                                                ^

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 

Testing Time: 60.03s
********************
Failing Tests (1):
    Clang :: Driver/hexagon-toolchain-elf.c

Would you mind taking a look and either fixing or reverting this patch? Thanks.

[nathanchance]

I am seeing the same failure that @leonardchan reported above. This is related to -DCLANG_DEFAULT_LINKER=lld:

$ cmake -G Ninja \
      -Wno-dev \
      -DCMAKE_BUILD_TYPE=Release \
      -DCLANG_DEFAULT_LINKER=lld \
      -DPYTHON_EXECUTABLE=$(command -v python3) \
      -DLLVM_ENABLE_PROJECTS="clang;lld" \
      -DCMAKE_C_COMPILER=clang \
      -DCMAKE_CXX_COMPILER=clang++ \
      ../llvm && \
ninja lld check-clang
...
Failing Tests (1):
    Clang :: Driver/hexagon-toolchain-elf.c

  Expected Passes    : 16530
  Expected Failures  : 21
  Unsupported Tests  : 98
  Unexpected Failures: 1
...
$ cmake -G Ninja \
      -Wno-dev \
      -DCMAKE_BUILD_TYPE=Release \
      -DPYTHON_EXECUTABLE=$(command -v python3) \
      -DLLVM_ENABLE_PROJECTS="clang;lld" \
      -DCMAKE_C_COMPILER=clang \
      -DCMAKE_CXX_COMPILER=clang++ \
      ../llvm && \
ninja lld check-clang
...
  Expected Passes    : 16531
  Expected Failures  : 21
  Unsupported Tests  : 98
...

The test assumes that if -fuse-ld is not passed, we are not using ld.lld but that is clearly false when -DCLANG_DEFAULT_LINKER=lld is set. It seems like an explicit -fuse-ld=ld should be used so we are guaranteed to hit https://elixir.bootlin.com/llvm/llvmorg-10-init/source/clang/lib/Driver/ToolChain.cpp#L501?

diff --git a/clang/test/Driver/hexagon-toolchain-elf.c b/clang/test/Driver/hexagon-toolchain-elf.c
index cf7d8bdb393..c9b5e20c41a 100644
--- a/clang/test/Driver/hexagon-toolchain-elf.c
+++ b/clang/test/Driver/hexagon-toolchain-elf.c
@@ -538,11 +538,12 @@
 // CHECK080:      "-Wreturn-type"

 // -----------------------------------------------------------------------------
-// Default, not passing -fuse-ld
+// Default, using system linker
 // -----------------------------------------------------------------------------
 // RUN: %clang -### -target hexagon-unknown-elf \
 // RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
 // RUN:   -mcpu=hexagonv60 \
+// RUN:   -fuse-ld=ld \
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK081 %s
 // REQUIRES: hexagon-registered-target

[MaskRay]

In D70919#1818355, @nathanchance wrote:

I am seeing the same failure that @leonardchan reported above. This is related to -DCLANG_DEFAULT_LINKER=lld:

$ cmake -G Ninja \
      -Wno-dev \
      -DCMAKE_BUILD_TYPE=Release \
      -DCLANG_DEFAULT_LINKER=lld \
      -DPYTHON_EXECUTABLE=$(command -v python3) \
      -DLLVM_ENABLE_PROJECTS="clang;lld" \
      -DCMAKE_C_COMPILER=clang \
      -DCMAKE_CXX_COMPILER=clang++ \
      ../llvm && \
ninja lld check-clang
...
Failing Tests (1):
    Clang :: Driver/hexagon-toolchain-elf.c

  Expected Passes    : 16530
  Expected Failures  : 21
  Unsupported Tests  : 98
  Unexpected Failures: 1
...
$ cmake -G Ninja \
      -Wno-dev \
      -DCMAKE_BUILD_TYPE=Release \
      -DPYTHON_EXECUTABLE=$(command -v python3) \
      -DLLVM_ENABLE_PROJECTS="clang;lld" \
      -DCMAKE_C_COMPILER=clang \
      -DCMAKE_CXX_COMPILER=clang++ \
      ../llvm && \
ninja lld check-clang
...
  Expected Passes    : 16531
  Expected Failures  : 21
  Unsupported Tests  : 98
...

The test assumes that if -fuse-ld is not passed, we are not using ld.lld but that is clearly false when -DCLANG_DEFAULT_LINKER=lld is set. It seems like an explicit -fuse-ld=ld should be used so we are guaranteed to hit https://elixir.bootlin.com/llvm/llvmorg-10-init/source/clang/lib/Driver/ToolChain.cpp#L501?

diff --git a/clang/test/Driver/hexagon-toolchain-elf.c b/clang/test/Driver/hexagon-toolchain-elf.c
index cf7d8bdb393..c9b5e20c41a 100644
--- a/clang/test/Driver/hexagon-toolchain-elf.c
+++ b/clang/test/Driver/hexagon-toolchain-elf.c
@@ -538,11 +538,12 @@
 // CHECK080:      "-Wreturn-type"

 // -----------------------------------------------------------------------------
-// Default, not passing -fuse-ld
+// Default, using system linker
 // -----------------------------------------------------------------------------
 // RUN: %clang -### -target hexagon-unknown-elf \
 // RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
 // RUN:   -mcpu=hexagonv60 \
+// RUN:   -fuse-ld=ld \
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK081 %s
 // REQUIRES: hexagon-registered-target

Looking

[MaskRay]

https://reviews.llvm.org/D72668

clang/lib/Driver/ToolChains/Hexagon.cpp

constructHexagonLinkArgs(Compilation &C, const JobAction &JA,
   //----------------------------------------------------------------------------
   bool IsStatic = Args.hasArg(options::OPT_static);
   bool IsShared = Args.hasArg(options::OPT_shared);
   bool IsPIE = Args.hasArg(options::OPT_pie);
   bool IncStdLib = !Args.hasArg(options::OPT_nostdlib);
   bool IncStartFiles = !Args.hasArg(options::OPT_nostartfiles);
   bool IncDefLibs = !Args.hasArg(options::OPT_nodefaultlibs);
   bool UseG0 = false;
+  const char *Exec = Args.MakeArgString(HTC.GetLinkerPath());
+  bool UseLLD = (llvm::sys::path::filename(Exec).equals_lower("ld.lld") ||
+                 llvm::sys::path::stem(Exec).equals_lower("ld.lld"));
   bool UseShared = IsShared && !IsStatic;
+  StringRef CpuVer = toolchains::HexagonToolChain::GetTargetCPUVersion(Args);
 
   //----------------------------------------------------------------------------
   // Silence warnings for various options
   //----------------------------------------------------------------------------
   Args.ClaimAllArgs(options::OPT_g_Group);
   Args.ClaimAllArgs(options::OPT_emit_llvm);
   Args.ClaimAllArgs(options::OPT_w); // Other warning options are already
                                      // handled somewhere else.
   Args.ClaimAllArgs(options::OPT_static_libgcc);
 
   //----------------------------------------------------------------------------
   //
   //----------------------------------------------------------------------------
   if (Args.hasArg(options::OPT_s))
     CmdArgs.push_back("-s");
 
   if (Args.hasArg(options::OPT_r))
     CmdArgs.push_back("-r");
 
   for (const auto &Opt : HTC.ExtraOpts)
     CmdArgs.push_back(Opt.c_str());
 
+  if (!UseLLD) {
     CmdArgs.push_back("-march=hexagon");
-  StringRef CpuVer = toolchains::HexagonToolChain::GetTargetCPUVersion(Args);
     CmdArgs.push_back(Args.MakeArgString("-mcpu=hexagon" + CpuVer));
+  }
 
   if (IsShared) {
     CmdArgs.push_back("-shared");
     // The following should be the default, but doing as hexagon-gcc does.
     CmdArgs.push_back("-call_shared");
   }
 
   if (IsStatic)

clang/test/Driver/hexagon-toolchain-elf.c

 // -----------------------------------------------------------------------------
 // RUN: %clang -### -target hexagon-unknown-elf \
 // RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
 // RUN:   -mcpu=hexagonv60 \
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK080 %s
 // CHECK080:      "-cc1"
 // CHECK080:      "-Wreturn-type"
+
+// -----------------------------------------------------------------------------
+// Default, not passing -fuse-ld
+// -----------------------------------------------------------------------------
+// RUN: %clang -### -target hexagon-unknown-elf \
+// RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
+// RUN:   -mcpu=hexagonv60 \
+// RUN:   %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK081 %s
+// CHECK081:      "-march=hexagon"
+// CHECK081:      "-mcpu=hexagonv60"
+// -----------------------------------------------------------------------------
+// Passing -fuse-ld=lld
+// -----------------------------------------------------------------------------
+// RUN: %clang -### -target hexagon-unknown-elf \
+// RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
+// RUN:   -mcpu=hexagonv60 \
+// RUN:   -fuse-ld=lld \
+// RUN:   %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK082 %s
+// CHECK082-NOT:      -march=
+// CHECK082-NOT:      -mcpu=