This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by inglorion on Mar 17 2017, 1:30 PM.

Details

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.

Event Timeline

inglorion created this revision.Mar 17 2017, 1:30 PM

@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?

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 edited edge metadata.Mar 17 2017, 4:25 PM

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.

@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.

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

how did you configure your build?

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.

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'

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

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 edited edge metadata.Mar 20 2017, 2:06 PM

@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
16–17

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

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 added inline comments.Mar 20 2017, 2:34 PM
cmake/Modules/CompilerRTUtils.cmake
129

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 updated this revision to Diff 92384.Mar 20 2017, 2:51 PM

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

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.

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 added inline comments.Mar 20 2017, 3:25 PM
cmake/Modules/CompilerRTUtils.cmake
146

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 added inline comments.Mar 20 2017, 3:41 PM
cmake/Modules/CompilerRTUtils.cmake
146

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.

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 added a comment.Mar 20 2017, 3:45 PM

@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 updated this revision to Diff 92394.Mar 20 2017, 4:05 PM

apply similar change to darwin_test_archs

inglorion added inline comments.Mar 20 2017, 4:06 PM
cmake/Modules/CompilerRTUtils.cmake
146

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?

Works for me now, thanks.

beanz accepted this revision.Mar 21 2017, 10:40 AM

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

This revision is now accepted and ready to land.Mar 21 2017, 10:40 AM
This revision was automatically updated to reflect the committed changes.

Thanks for reviewing!