[compiler-rt] respect CMAKE_EXE_LINKER_FLAGS in compiler and library tests

[inglorion]

Summary

check_cxx_compiler_flag and check_library_exists could fail because they ignored CMAKE_EXE_LINKER_FLAGS and therefore would always fail to produce executables. Cmake policy CMP0056 fixes this, but was explicitly set to OLD in our CMakeLists because it caused problems with test_target_arch. This change sets the policy to NEW to fix the problem with the compiler and library tests, and temporarily clears CMAKE_EXE_LINKER_FLAGS inside test_target_arch to emulate the old behavior there. This allows, for example, LTO builds that require lld to succeed.

[inglorion]

@beanz, @davidxl : It isn't clear to me from the comment that was in CMakeLists.txt what exact problem CMP0056 NEW caused with test_target_arch, so I'm not completely sure what I did avoids the problem. Could one of you check if my change seems ok or if there is a better way?

[inglorion]

Some more background on this change:

I tried to build clang with lld and LTO on Linux. The build spit out a lot of warnings about non-virtual destructors and a bunch of errors like undefined reference to dlsym. This turned out to be because we were not passing flags that we were supposed to be passing. The parts of config-ix.cmake that are supposed to be testing for these flags were all failing, because they try to produce executables with the flag, and if that fails, we conclude that the compiler or linker does not support that flag. In my case, we failed to create executables because we were trying to link object files created with -flto with a linker that doesn't support LLVM bitcode - despite the build using LLVM_USE_LINKER=lld. So we would conclude that things like -Wno-non-virtual-dtor and -ldl are not supported, not pass those flags, and fail the build.

[davidxl]

Can you do a 'ninja check-profile' on a x86_64 linux box and tell me the number of tests done? With and without your patch, the number should remain the same.

[inglorion]

@davidxl: My ninja doesn't know about check-profile. Did you mean check-clang-profile? That gives me 32 tests with and without this change.

[davidxl]

I meant 'check-profile' target. You should see 150 tests successfully run.

[davidxl]

how did you configure your build?

[inglorion]

I configured the build using

cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DCMAKE_AR=$LLVM_BIN/llvm-ar -DCMAKE_RANLIB=$LLVM_BIN/llvm-ranlib -DCMAKE_C_COMPILER=$LLVM_BIN/clang -DCMAKE_CXX_COMPILER=$LLVM_BIN/clang++ -DLLVM_USE_LINKER=lld -DCMAKE_LINKER=$LLVM_BIN/ld.lld -DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_BINUTILS_INCDIR=$HOME/binutils/include -DLLVM_ENABLE_ASSERTIONS=1 -DLLVM_ENABLE_LTO=thin  $HOME/llvm

where LLVM_BIN points at the bin directory of a previous build, and $HOME/llvm is where my sources live. Ninja doesn't know about any check-profile target:

$ ninja check-profile                                                                             
ninja: error: unknown target 'check-profile'

This is on r298110, with llvm, compiler-rt, clang, and lld checked out.

[davidxl]

I just did a source sync and rerun cmake in a fresh dir, and verified check-profile target is there (lto is set to Off and type is Release)

env DYLD_LIBRARY_PATH=${build_dir}/lib/ cmake -G Ninja \
${src_root} \
-DLLVM_BUILD_EXAMPLES=Off \
-DLLVM_ENABLE_LTO=${lto} \
-DLLVM_PARALLEL_LINK_JOBS=1 \
-DCMAKE_C_COMPILER=${build_dir}/bin/clang \
-DCMAKE_C_FLAGS=-g0 \
-DCMAKE_CXX_FLAGS=-g0 \
-DCMAKE_CXX_COMPILER=${build_dir}/bin/clang++ \
-DCMAKE_EXE_LINKER_FLAGS=-fuse-ld=gold \
-DCMAKE_MODULE_LINKER_FLAGS=-fuse-ld=gold \
-DCMAKE_SHARED_LINKER_FLAGS=-fuse-ld=gold \
-DCMAKE_RANLIB=${build_dir}/bin/llvm-ranlib \
-DCMAKE_AR=${build_dir}/bin/llvm-ar \
-DLLVM_BINUTILS_INCDIR=${bin_utils_dir} \
-DCMAKE_BUILD_TYPE=${type} \
-DLLVM_ENABLE_ASSERTIONS=Off \
-DCOMPILER_RT_BUILD_BUILTINS=Off \
'-DLLVM_LIT_ARGS=--xunit-xml-output=testresults.xunit.xml -v'

[inglorion]

Thanks, davidxl. With your command line I do get the check-profile target.

[inglorion]

I'm seeing 100 tests passed, 2 unsupported without this change, and 50 passed, 1 unsupported with the change. That doesn't seem right.

[beanz]

@davidxl, do you recall how the failure that caused you to make the policy change exhibited itself? It seems to me like we were working around the behavior of the old policy setting by passing the linker options through manually. I wonder if the reason for the error was CMake masking those liker flags under the new policy.

Thoughts?

CMakeLists.txt

17 ↗

(On Diff #92188)

You shouldn't need to set this to NEW, it should default to NEW based on the cmake_minimum_version being set above.

cmake/Modules/CompilerRTUtils.cmake

129 ↗

(On Diff #92188)

Pretty sure this will actually change behavior. Look down below, we were reading this value and passing it through to the try_compile call.

[inglorion]

cmake/Modules/CompilerRTUtils.cmake

129 ↗

(On Diff #92188)

Yes. It looks like the code we have in this function explicitly passes CMAKE_EXE_LINKER_FLAGS. I'm still running the tests, but it looks like just setting CMP0056 and removing that code makes things work like they're supposed to. New diff coming shortly.

[inglorion]

Changed test_target_arch to pass CMAKE_EXE_LINKER_FLAGS and argstring, which matches what it did before.

[inglorion]

With this new diff, check-profile runs the same number of tests as it does in the base revision (102, in my case). Unfortunately, when building with thinlto, they all fail (error message: llvm-build-clang/lib/clang/5.0.0/lib/linux/libclang_rt.profile-i386.a: error adding symbols: File format not recognized).

It's still an improvement over the base revision, which will fail to build all of compiler-rt when using thinlto.

[inglorion]

The "error adding symbols" message means we're still not using the right linker (it comes from ld, in a build where lld should be used as the linker).

[beanz]

cmake/Modules/CompilerRTUtils.cmake

146 ↗

(On Diff #92384)

Yes, this is the right way to make this change. Can you make a similar change to darwin_test_archs in CompilerRTDarwinUtils.cmake?

It should be a mechanical transformation.

[inglorion]

cmake/Modules/CompilerRTUtils.cmake

146 ↗

(On Diff #92384)

Per @fjricci's comment on D31144, I think this patch already fixes the issue for them. Note that darwin_test_archs isn't currently explicitly passing in CMAKE_EXE_LINKER_FLAGS. D31144 would have done it, and this patch should accomplish the same effect by passing the flags implicitly because CMP0056 is NEW.

[fjricci]

I think @beanz is correct, I actually end up seeing a few of my architectures disappear with the current version of this patch, although I didn't look into it yet.

[beanz]

@inglorion I believe the reason we had to set the policy to OLD is because the auto-passed variable masks the explicitly passed one in try_compile. If that is the case, we need the same change in the Darwin code path too.

[inglorion]

apply similar change to darwin_test_archs

[inglorion]

cmake/Modules/CompilerRTUtils.cmake

146 ↗

(On Diff #92384)

Ah, right. I had somehow missed that darwin_test_archs also overrides CMAKE_EXE_LINKER_FLAGS. @fjricci, does it work for you with this change to darwin_test_archs?

[fjricci]

Works for me now, thanks.

[beanz]

Cool! Seems to me like this patch addresses everyones issues, and I think the patch looks good. Thanks for iterating on this!

[inglorion]

Thanks for reviewing!

compiler-rt/trunk/CMakeLists.txt

 # CMake build for CompilerRT.
 #
 # This build assumes that CompilerRT is checked out into the
 # 'projects/compiler-rt' or 'runtimes/compiler-rt' inside of an LLVM tree.
 # Standalone build system for CompilerRT is not yet ready.
 #
 # An important constraint of the build is that it only produces libraries
 # based on the ability of the host toolchain to target various platforms.
 
 # Check if compiler-rt is built as a standalone project.
 if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR OR COMPILER_RT_STANDALONE_BUILD)
   project(CompilerRT C CXX ASM)
   set(COMPILER_RT_STANDALONE_BUILD TRUE)
 endif()
 
 cmake_minimum_required(VERSION 3.4.3)
-# FIXME:
-# The OLD behavior (pre 3.2) for this policy is to not set the value of the 
-# CMAKE_EXE_LINKER_FLAGS variable in the generated test project. The NEW behavior
-# for this policy is to set the value of the CMAKE_EXE_LINKER_FLAGS variable 
-# in the test project to the same as it is in the calling project. The new 
-# behavior cause the compiler_rt test to fail during try_compile: see
-# projects/compiler-rt/cmake/Modules/CompilerRTUtils.cmake:121 such that
-# CAN_TARGET_${arch} is not set properly. This results in COMPILER_RT_SUPPORTED_ARCH
-# not being updated properly leading to poblems.
-cmake_policy(SET CMP0056 OLD)
 
 # Add path for custom compiler-rt modules.
 list(INSERT CMAKE_MODULE_PATH 0
   "${CMAKE_CURRENT_SOURCE_DIR}/cmake"
   "${CMAKE_CURRENT_SOURCE_DIR}/cmake/Modules"
   )
 
 include(base-config-ix)

compiler-rt/trunk/cmake/Modules/CompilerRTDarwinUtils.cmake

function(darwin_test_archs os valid_archs)
 
   set(working_archs)
   foreach(arch ${archs})
    
     set(arch_linker_flags "-arch ${arch} ${os_linker_flags}")
     if(TEST_COMPILE_ONLY)
       try_compile_only(CAN_TARGET_${os}_${arch} -v -arch ${arch} ${DARWIN_${os}_CFLAGS})
     else()
+      set(SAVED_CMAKE_EXE_LINKER_FLAGS ${CMAKE_EXE_LINKER_FLAGS})
+      set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${arch_linker_flags}")
       try_compile(CAN_TARGET_${os}_${arch} ${CMAKE_BINARY_DIR} ${SIMPLE_C}
                   COMPILE_DEFINITIONS "-v -arch ${arch}" ${DARWIN_${os}_CFLAGS}
-                  CMAKE_FLAGS "-DCMAKE_EXE_LINKER_FLAGS=${arch_linker_flags}"
                   OUTPUT_VARIABLE TEST_OUTPUT)
+      set(CMAKE_EXE_LINKER_FLAGS ${SAVED_CMAKE_EXE_LINKER_FLAGS})
     endif()
     if(${CAN_TARGET_${os}_${arch}})
       list(APPEND working_archs ${arch})
     else()
       file(APPEND ${CMAKE_BINARY_DIR}${CMAKE_FILES_DIRECTORY}/CMakeError.log
         "Testing compiler for supporting ${os}-${arch}:\n"
         "${TEST_OUTPUT}\n")
     endif()

compiler-rt/trunk/cmake/Modules/CompilerRTUtils.cmake

macro(test_target_arch arch def)
   endforeach()
   check_compile_definition("${def}" "${argstring}" HAS_${arch}_DEF)
   if(NOT DEFINED CAN_TARGET_${arch})
     if(NOT HAS_${arch}_DEF)
       set(CAN_TARGET_${arch} FALSE)
     elseif(TEST_COMPILE_ONLY)
       try_compile_only(CAN_TARGET_${arch} ${TARGET_${arch}_CFLAGS})
     else()
-      set(argstring "${CMAKE_EXE_LINKER_FLAGS} ${argstring}")
       set(FLAG_NO_EXCEPTIONS "")
       if(COMPILER_RT_HAS_FNO_EXCEPTIONS_FLAG)
         set(FLAG_NO_EXCEPTIONS " -fno-exceptions ")
       endif()
+      set(SAVED_CMAKE_EXE_LINKER_FLAGS ${CMAKE_EXE_LINKER_FLAGS})
+      set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${argstring}")
       try_compile(CAN_TARGET_${arch} ${CMAKE_BINARY_DIR} ${SIMPLE_SOURCE}
                   COMPILE_DEFINITIONS "${TARGET_${arch}_CFLAGS} ${FLAG_NO_EXCEPTIONS}"
-                  OUTPUT_VARIABLE TARGET_${arch}_OUTPUT
-                  CMAKE_FLAGS "-DCMAKE_EXE_LINKER_FLAGS:STRING=${argstring}")
+                  OUTPUT_VARIABLE TARGET_${arch}_OUTPUT)
+      set(CMAKE_EXE_LINKER_FLAGS ${SAVED_CMAKE_EXE_LINKER_FLAGS})
     endif()
   endif()
   if(${CAN_TARGET_${arch}})
     list(APPEND COMPILER_RT_SUPPORTED_ARCH ${arch})
   elseif("${COMPILER_RT_DEFAULT_TARGET_ARCH}" STREQUAL "${arch}" AND
          COMPILER_RT_HAS_EXPLICIT_DEFAULT_TARGET_TRIPLE)
     # Bail out if we cannot target the architecture we plan to test.
     message(FATAL_ERROR "Cannot compile for ${arch}:\n${TARGET_${arch}_OUTPUT}")