This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Fix linker detection in AddLLVM.cmake
ClosedPublic

Authored by light2yellow on Oct 7 2017, 5:33 AM.

Details

Summary

Fix linker not being correctly detected when a custom one is specified
through LLVM_USE_LINKER CMake variable.

In particular,

cmake -DCMAKE_BUILD_TYPE=Release -DLLVM_USE_LINKER=gold ../llvm

resulted into

  • Linker detection: GNU ld

instead of

  • Linker detection: GNU Gold

due to the construction not accounting for such variable. It led to the general
confusion and prevented setting linker-specific flags inside functions defined
in AddLLVM.cmake.

Event Timeline

light2yellow created this revision.Oct 7 2017, 5:33 AM
light2yellow edited the summary of this revision. (Show Details)Oct 7 2017, 5:36 AM
light2yellow added a reviewer: chandlerc.
light2yellow set the repository for this revision to rL LLVM.
pmatos added a subscriber: pmatos.Oct 23 2017, 12:10 AM
lichray accepted this revision.Oct 24 2017, 12:01 PM
lichray added a subscriber: ruiu.
lichray added a subscriber: lichray.

Tested on FreeBSD; works for me.

This revision is now accepted and ready to land.Oct 24 2017, 12:01 PM
ruiu added inline comments.Oct 24 2017, 12:04 PM
cmake/modules/AddLLVM.cmake
153

Is this safe if a C compiler does not support the -fuse-ld option?

lichray added inline comments.Oct 24 2017, 3:22 PM
cmake/modules/AddLLVM.cmake
153

That may be too old to compile LLVM. I would say wait them come to us. After all, this option is to assist detection, and more users are specifying linkers directly.

ruiu added inline comments.Oct 24 2017, 3:24 PM
cmake/modules/AddLLVM.cmake
153

You can use compilers other than clang to build clang and llvm, and I believe only clang supports this -fuse-ld option.

lichray added inline comments.Oct 24 2017, 3:31 PM
cmake/modules/AddLLVM.cmake
153

No? gcc does https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55470 therefore Intel C++ does. Clang does, then so does XL on AIX and Oracle.

ruiu added a comment.Oct 24 2017, 3:38 PM

I believe it only recognizes -fuse-ld=bfd and -fuse-ld=gold. You cannot pass any other linker name.

In D38665#905826, @ruiu wrote:

I believe it only recognizes -fuse-ld=bfd and -fuse-ld=gold. You cannot pass any other linker name.

I think that's enough. If you worry about lld, there is another option LLVM_USE_LLD=ON.

ruiu added a comment.Oct 24 2017, 5:25 PM

I mean -DLLVM_USE_LINKER=lld is a valid option, so when it failed due to the lack compiler support of -fuse-ld=lld, it should be reported as a meaningful error, instead of a cryptic "CMake failed to compile this program blah blah." message. I think that is also true to the case when you pass -DLLVM_USE_LINKER=gold but gold was not installed to your system.

In D38665#905947, @ruiu wrote:

I mean -DLLVM_USE_LINKER=lld is a valid option, so when it failed due to the lack compiler support of -fuse-ld=lld, it should be reported as a meaningful error, instead of a cryptic "CMake failed to compile this program blah blah." message. I think that is also true to the case when you pass -DLLVM_USE_LINKER=gold but gold was not installed to your system.

Let me clarify this with refreshed information -- there is no LLVM_USE_LLD but LLVM_ENABLE_LLD, which builds lld and then set LLVM_USE_LINKER, and then in cmake/modules/HandleLLVMOptions.cmake line 197 detects -fuse-ld=. Which means, neither LLVM_USE_LINKER nor LLVM_ENABLE_LLD had magics to support compilers other than clang to use lld. That is to say, this patch doesn't make our support worse. The error message has room for improvement, but IMHO may not worth to work it out in this patch. The author can feel free to apply line 197's macro here, but unlikely we want to make even more errors from underlying compilers more meaningful.

ruiu accepted this revision.Oct 24 2017, 6:26 PM

CMake inherently runs on a variety of systems, so if your change could result in producing a head-scratching mysterious error message on some system, I think you need to address that from the beginning, even if that system is not supported. Saying that "this flag is not supported/your system is too old" is one of essential roles of our cmakefiles and that is not "good to have".

That said, since we already have a check for a LLVM_USE_LINKER value in llvm/cmake/modules/HandleLLVMOptions.cmake, it seems that we don't need add the same check to this file. So LGTM.

(Actually, the existing check in HandleLLVMOptions.cmake is not working because of a bug, so when you pass a bogus value to -DLLVM_USE_LINKER, it prints out a bad error message. I'll fix it separately.)

timshen closed this revision.Oct 30 2017, 2:13 PM
timshen added a subscriber: timshen.

Committed as r316956.