Page MenuHomePhabricator

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

Authored by sidneym on Dec 2 2019, 12:16 PM.

Details

Summary

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

Diff Detail

Event Timeline

sidneym created this revision.Dec 2 2019, 12:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2019, 12:16 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
kparzysz added inline comments.Dec 2 2019, 12:25 PM
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 updated this revision to Diff 231765.Dec 2 2019, 12:45 PM

Update testcase.

sidneym marked an inline comment as done.Dec 5 2019, 8:49 AM
bcain added inline comments.Dec 5 2019, 8:59 AM
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 updated this revision to Diff 232374.Dec 5 2019, 10:02 AM

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 added a comment.Dec 11 2019, 8:44 PM

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 added inline comments.
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 updated this revision to Diff 233687.Dec 12 2019, 2:51 PM

make check for lld more generic.

Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2019, 2:51 PM
sidneym updated this revision to Diff 233864.Dec 13 2019, 1:24 PM

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

kparzysz added inline comments.Dec 16 2019, 10:30 AM
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 updated this revision to Diff 234174.Dec 16 2019, 3:30 PM

Remove quotes.

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

This revision was not accepted when it landed; it landed in state Needs Review.Dec 20 2019, 12:23 PM
This revision was automatically updated to reflect the committed changes.

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.

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

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