[compiler-rt][CMAKE] Only add cmake link flags in standalone build

[jsji]

Summary

Only add CMAKE_EXE_LINKER_FLAGS when in a standalone bulid.
Or else CMAKE_EXE_LINKER_FLAGS contains flags for build compiler of Clang/llvm.
This might not be the same as what the COMPILER_RT_TEST_COMPILER supports.
eg: the build compiler use lld linker and we use it to build clang with
default ld linker then to be tested clang will complain about lld
options like --color-diagnostics.

[MaskRay]

Not adding CMAKE_EXE_LINKER_FLAGS to TEST_LINK_FLAGS looks reasonable to me, but why does COMPILER_RT_STANDALONE_BUILD add the flags?

[jsji]

In D78373#1989002, @MaskRay wrote:

Not adding CMAKE_EXE_LINKER_FLAGS to TEST_LINK_FLAGS looks reasonable to me, but why does COMPILER_RT_STANDALONE_BUILD add the flags?

In standalone build, the CMAKE_EXE_LINKER_FLAGS should contain the flags of to be tested compiler,
as we shouldn't be passing it down from llvm/clang anymore.

[jsji]

In D78373#1989016, @jsji wrote:

In D78373#1989002, @MaskRay wrote:

Not adding CMAKE_EXE_LINKER_FLAGS to TEST_LINK_FLAGS looks reasonable to me, but why does COMPILER_RT_STANDALONE_BUILD add the flags?

In standalone build, the CMAKE_EXE_LINKER_FLAGS should contain the flags of to be tested compiler,
as we shouldn't be passing it down from llvm/clang anymore.

In https://reviews.llvm.org/rL208896 the author mentioned:
This is particularly important when the flags direct Clang, even the just-built-Clang, toward the standard library, linker, and other tools to use.

So my understanding is this might happen in standalone build mode. (Although I am not 100% sure).

[MaskRay]

In D78373#1989018, @jsji wrote:

In D78373#1989016, @jsji wrote:

In D78373#1989002, @MaskRay wrote:

Not adding CMAKE_EXE_LINKER_FLAGS to TEST_LINK_FLAGS looks reasonable to me, but why does COMPILER_RT_STANDALONE_BUILD add the flags?

In standalone build, the CMAKE_EXE_LINKER_FLAGS should contain the flags of to be tested compiler,
as we shouldn't be passing it down from llvm/clang anymore.

In https://reviews.llvm.org/rL208896 the author mentioned:
This is particularly important when the flags direct Clang, even the just-built-Clang, toward the standard library, linker, and other tools to use.

So my understanding is this might happen in standalone build mode. (Although I am not 100% sure).

If CMAKE_EXE_LINKER_FLAGS contains -fuse-ld=lld, this change will drop -fuse-ld=lld for compiler-rt tests.

In your use case, how does CMAKE_CXX_COMPILER selects lld while COMPILER_RT_TEST_COMPILER selects a non-lld system linker?

[jsji]

If CMAKE_EXE_LINKER_FLAGS contains -fuse-ld=lld, this change will drop -fuse-ld=lld for compiler-rt tests.

In your use case, how does CMAKE_CXX_COMPILER selects lld while COMPILER_RT_TEST_COMPILER selects a non-lld system linker?

OK, so looks like still confusing to you.

There are two compilers:

1. compiler#1: Build compiler of clang/llvm, CMAKE_CXX_COMPILER in llvm CMakeFiles

The build compiler can be any compiler, gcc/clang, we really can NOT assume what linker it will use.

In our case, we are still using clang, and it was built to use lld as default linker.

2. compiler#2: the clang/llvm we want to build and test

and also COMPILER_RT_TEST_COMPILER in compiler_rt CMakefiles

We did not change linker settings in config.
So, the newly built clang/llvm will be using default ld linker.

After the build, we will need to build compilre_rt tests,
this now will use compiler#2 as test compiler.

During the config time of this build, CMAKE_EXE_LINKER_FLAGS will call check_linker_flag,
which will try running compiler#1 to detect whether it supports --color-diagnostics.
Since compiler#1 is using lld, so yes, we can add --color-diagnostics to CMAKE_EXE_LINKER_FLAGS,
to build all exe in clang/LLVM .

Since compilre_rt is sub-project of llvm,
the CMAKE_EXE_LINKER_FLAGS set by llvm will be passed down to project compiler_rt.
So when we are trying to call compiler#2 to generate tests,
we met the problem: ld die complaining about not supporting --color-diagnostics.

If we set -fuse-ld=lld during this build in CMAKE_EXE_LINKER_FLAGS,
then compiler#2 will use lld as linker.

Yes, with this change, we will drop -fuse-ld=lld in the command line of generating tests.
But that should NOT be a problem at all -- because compiler#2 is built to use lld as linker already,
you don't need to (and should not) specify the command line when building tests ..

Hope this make it clear.

[MaskRay]

In D78373#1989546, @jsji wrote:

If CMAKE_EXE_LINKER_FLAGS contains -fuse-ld=lld, this change will drop -fuse-ld=lld for compiler-rt tests.

In your use case, how does CMAKE_CXX_COMPILER selects lld while COMPILER_RT_TEST_COMPILER selects a non-lld system linker?

OK, so looks like still confusing to you.

There are two compilers:

1. compiler#1: Build compiler of clang/llvm, CMAKE_CXX_COMPILER in llvm CMakeFiles

The build compiler can be any compiler, gcc/clang, we really can NOT assume what linker it will use.

In our case, we are still using clang, and it was built to use lld as default linker.

2. compiler#2: the clang/llvm we want to build and test

and also COMPILER_RT_TEST_COMPILER in compiler_rt CMakefiles

We did not change linker settings in config.
So, the newly built clang/llvm will be using default ld linker.

After the build, we will need to build compilre_rt tests,
this now will use compiler#2 as test compiler.

During the config time of this build, CMAKE_EXE_LINKER_FLAGS will call check_linker_flag,
which will try running compiler#1 to detect whether it supports --color-diagnostics.
Since compiler#1 is using lld, so yes, we can add --color-diagnostics to CMAKE_EXE_LINKER_FLAGS,
to build all exe in clang/LLVM .

Since compilre_rt is sub-project of llvm,
the CMAKE_EXE_LINKER_FLAGS set by llvm will be passed down to project compiler_rt.
So when we are trying to call compiler#2 to generate tests,
we met the problem: ld die complaining about not supporting --color-diagnostics.

If we set -fuse-ld=lld during this build in CMAKE_EXE_LINKER_FLAGS,
then compiler#2 will use lld as linker.

Yes, with this change, we will drop -fuse-ld=lld in the command line of generating tests.
But that should NOT be a problem at all -- because compiler#2 is built to use lld as linker already,
you don't need to (and should not) specify the command line when building tests ..

Hope this make it clear.

Thanks for the explanation. Can you check whether CMAKE_CXX_FLAGS is threaded through compiler-rt test CXXFLAGS? If not, I feel that we should conservatively choose the flags that TEST_LINK_FLAGS should inherit. Instead of set(TEST_LINK_FLAGS ${CMAKE_EXE_LINKER_FLAGS} ${TEST_LINK_FLAGS}"), TEST_LINK_FLAGS can just inherit from LLVM_ENABLE_LLD and LLVM_USE_LINKER

[jsji]

Thanks for the explanation. Can you check whether CMAKE_CXX_FLAGS is threaded through compiler-rt test CXXFLAGS? If not, I feel that we should conservatively choose the flags that TEST_LINK_FLAGS should inherit. Instead of set(TEST_LINK_FLAGS ${CMAKE_EXE_LINKER_FLAGS} ${TEST_LINK_FLAGS}"), TEST_LINK_FLAGS can just inherit from LLVM_ENABLE_LLD and LLVM_USE_LINKER

Yes, they should be handled similarly, but I don't think how we currently deal with CMAKE_CXX_FLAGS is a reason to impact the decision about how to pass CMAKE_EXE_LINKER_FLAGS.

Only inheriting LLVM_ENABLE_LLD or LLVM_USE_LINKER won't break, however, I think it is still hacking -- we are still mixing flags for build compiler and test compiler.

[jsji]

Ping, any further feedback?

[jsji]

Ping..

[MaskRay]

I still don't understand why non-COMPILER_RT_STANDALONE_BUILD is differentiated and why -fuse-ld=lld has to be dropped. We can just remove -Wl,--color-diagnostics (added in cmake/modules/HandleLLVMOptions.cmake) here.

[jsji]

In D78373#2017751, @MaskRay wrote:

I still don't understand why non-COMPILER_RT_STANDALONE_BUILD is differentiated and why -fuse-ld=lld has to be dropped. We can just remove -Wl,--color-diagnostics (added in cmake/modules/HandleLLVMOptions.cmake) here.

In standalone build, the CMAKE_EXE_LINKER_FLAGS should contain the flags of to be tested compiler,
as we shouldn't be passing it down from llvm/clang anymore.

In non-stadalone build, we are configure for building clang/llvm,
so the flags in CMAKE_EXE_LINKER_FLAGS are for build compiler of clang/llvm, not for their tests (eg:compiler_rt tests).

Yes, removing -Wl,--color-diagnostics will not cause problem in current situation, but it is just a workaround to me.
We might meet similar issues if someone add another non-compatible options in llvm cmakefiles.

[MaskRay]

In D78373#2018140, @jsji wrote:

In D78373#2017751, @MaskRay wrote:

I still don't understand why non-COMPILER_RT_STANDALONE_BUILD is differentiated and why -fuse-ld=lld has to be dropped. We can just remove -Wl,--color-diagnostics (added in cmake/modules/HandleLLVMOptions.cmake) here.

In standalone build, the CMAKE_EXE_LINKER_FLAGS should contain the flags of to be tested compiler,
as we shouldn't be passing it down from llvm/clang anymore.

In non-stadalone build, we are configure for building clang/llvm,
so the flags in CMAKE_EXE_LINKER_FLAGS are for build compiler of clang/llvm, not for their tests (eg:compiler_rt tests).

If some system libraries are installed at non-standard locations, the user may pass some -L to make that work. -L will be needed by both the toolchain (llvm,clang,...) and tests (unless -nostdlib,-nodefaultlibs,etc).

Yes, removing -Wl,--color-diagnostics will not cause problem in current situation, but it is just a workaround to me.
We might meet similar issues if someone add another non-compatible options in llvm cmakefiles.

It may be the simplest approach and I don't think it can be broken easily. lld does not have many options not recognized by GNU ld. It is unlikely for users to specify those lld specific options (--warn-backrefs, --color-diagnostics, --no-rosegment). I don't see a problem blacklisting --color-diagnostics if the test compiler is different from CMAKE_CXX_COMPILER.

[jsji]

If some system libraries are installed at non-standard locations, the user may pass some -L to make that work. -L will be needed by both the toolchain (llvm,clang,...) and tests (unless -nostdlib,-nodefaultlibs,etc).

Yes, I agree there are situations that the flags for build compiler and to-be-tested compiler will use same flag. But this does not mean that they should be always the same.
We should try to avoid adding unnecessary options to compiler_rt test.

It may be the simplest approach and I don't think it can be broken easily. lld does not have many options not recognized by GNU ld. It is unlikely for users to specify those lld specific options (--warn-backrefs, --color-diagnostics, --no-rosegment). I don't see a problem blacklisting --color-diagnostics if the test compiler is different from CMAKE_CXX_COMPILER.

I believe there are other target linkers , more than just lld and ld .

[echristo]

Adding Petr.

[jsji]

Ping.. @phosek Petr, any comments? Thanks!

[phosek]

I agree with @MaskRay that this is fairly brittle, but so is the existing logic for picking up the COMPILER_RT_TEST_COMPILER and its flags, and I agree with @jsji that the current behavior may cause issues. Ideally, even when using the just built Clang we would rerun the CMake checks as in the case of the standalone build to make sure we have the right options, this is exactly how the runtimes build work, see http://lists.llvm.org/pipermail/llvm-dev/2020-April/140771.html for more details. However, I understand that it may not be feasible to switch to the runtimes build if you already have an existing build setup, and we should make sure that the existing build modes keep on working correctly, so LGTM from me.

[jsji]

Thank you @phosek ! So I will commit this first, and we will try to switch to the recommended standalone build mode.

compiler-rt/cmake/Modules/AddCompilerRT.cmake

function(add_compiler_rt_test test_suite test_name arch)
   list(APPEND TEST_LINK_FLAGS ${TARGET_LINK_FLAGS})
 
   # If we're not on MSVC, include the linker flags from CMAKE but override them
   # with the provided link flags. This ensures that flags which are required to
   # link programs at all are included, but the changes needed for the test
   # trump. With MSVC we can't do that because CMake is set up to run link.exe
   # when linking, not the compiler. Here, we hack it to use the compiler
   # because we want to use -fsanitize flags.
-  if(NOT MSVC)
+
+  # Only add CMAKE_EXE_LINKER_FLAGS when in a standalone bulid.
+  # Or else CMAKE_EXE_LINKER_FLAGS contains flags for build compiler of Clang/llvm.
+  # This might not be the same as what the COMPILER_RT_TEST_COMPILER supports.
+  # eg: the build compiler use lld linker and we build clang with default ld linker
+  # then to be tested clang will complain about lld options like --color-diagnostics.
+  if(NOT MSVC AND COMPILER_RT_STANDALONE_BUILD)
     set(TEST_LINK_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${TEST_LINK_FLAGS}")
     separate_arguments(TEST_LINK_FLAGS)
   endif()
   if(NOT COMPILER_RT_STANDALONE_BUILD AND COMPILER_RT_HAS_LLD AND "lld" IN_LIST LLVM_ENABLE_PROJECTS)
     # CMAKE_EXE_LINKER_FLAGS may contain -fuse=lld
     # FIXME: -DLLVM_ENABLE_LLD=ON and -DLLVM_ENABLE_PROJECTS without lld case.
     list(APPEND TEST_DEPS lld)
   endif()