This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] Set CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC_LIBRARY when building builtins standalone
ClosedPublic

Authored by mstorsjo on Nov 12 2020, 4:14 AM.

Details

Summary

When building builtins, the toolchain might not yet be at a stage when linking a test application works yet, as builtins aren't available. Therefore set CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC_LIBRARY, to avoid failing the compiler sanity check.

Setting CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC_LIBRARY has the risk of making checks for library availability succeed falsely (e.g. indicating that libs would be available that really aren't, as the tests don't do any linking), but the builtins library doesn't try to link against any external libraries (and only produces static libraries anyway), so it should be safe here.

This avoids having to set CMAKE_C_COMPILER_WORKS when bootstrapping a cross toolchain, when building the builtins.

Diff Detail

Event Timeline

mstorsjo created this revision.Nov 12 2020, 4:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2020, 4:14 AM
Herald added subscribers: Restricted Project, mgorny, dberris. · View Herald Transcript
mstorsjo requested review of this revision.Nov 12 2020, 4:14 AM

Based on your description and rationale I *think* this makes sense and should be safe. I'll leave it to others to explicitly approve the patch, though.

@abidh has a presentation where he was manually setting this to the same value for building a pure LLVM toolchain, for the same reasons. I suppose the advantage of patching the CMake file is that this issue will be taken care of automatically, and that perhaps it wouldn't be convenient, obvious or possible to manually set this when building Compiler-RT as part of an integrated LLVM build (LLVM_ENABLE_RUNTIMES/PROJECTS)?

phosek accepted this revision.Mar 10 2021, 11:35 PM

LGTM

This revision is now accepted and ready to land.Mar 10 2021, 11:35 PM
smeenai accepted this revision.Mar 10 2021, 11:37 PM

This will already work as-is for the runtimes build (since it builds the builtins standalone), but builtins-config-ix already sets an equivalent option for compiler-rt's custom macros, so it should be fine to apply this one universally to the builtins as well (either in this file or in builtins-config-ix).

Hi, I believe this patch may have indirectly led to a failure in our two-stage clang builder (https://luci-milo.appspot.com/p/fuchsia/builders/prod/clang-linux-x64/b8853033259975286848?):

[89/302] Building C object CMakeFiles/clang_rt.builtins-i386.dir/floatuntidf.c.o
FAILED: CMakeFiles/clang_rt.builtins-i386.dir/floatuntidf.c.o 
/b/s/w/ir/x/w/staging/llvm_build/./bin/clang --target=x86_64-unknown-linux-gnu --sysroot=/b/s/w/ir/x/w/cipd/linux-amd64 -DVISIBILITY_HIDDEN  -O3 -DNDEBUG  -m32 -std=c11 -fPIC -fno-builtin -fvisibility=hidden -fomit-frame-pointer -MD -MT CMakeFiles/clang_rt.builtins-i386.dir/floatuntidf.c.o -MF CMakeFiles/clang_rt.builtins-i386.dir/floatuntidf.c.o.d -o CMakeFiles/clang_rt.builtins-i386.dir/floatuntidf.c.o -c /b/s/w/ir/x/w/llvm-project/compiler-rt/lib/builtins/floatuntidf.c
In file included from /b/s/w/ir/x/w/llvm-project/compiler-rt/lib/builtins/floatuntidf.c:13:
In file included from /b/s/w/ir/x/w/llvm-project/compiler-rt/lib/builtins/int_lib.h:93:
In file included from /b/s/w/ir/x/w/staging/llvm_build/lib/clang/13.0.0/include/limits.h:21:
In file included from /b/s/w/ir/x/w/cipd/linux-amd64/usr/include/limits.h:25:
/b/s/w/ir/x/w/cipd/linux-amd64/usr/include/features.h:374:12: fatal error: 'sys/cdefs.h' file not found
#  include <sys/cdefs.h>
           ^~~~~~~~~~~~~

So I don't think the error is so much that we're missing this header, but rather this patch is leads to CMakeFiles/clang_rt.builtins-i386.dir/floatuntidf.c.o getting generated as a target. Before this patch, the cmake invocation used in this directory is:

/usr/bin/cmake -DCMAKE_C_COMPILER=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-two-stage-fuchsia-toolchain/./bin/clang -DCMAKE_CXX_COMPILER=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-two-stage-fuchsia-toolchain/./bin/clang++ -DCMAKE_ASM_COMPILER=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-two-stage-fuchsia-toolchain/./bin/clang -DCMAKE_LINKER=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-two-stage-fuchsia-toolchain/./bin/ld.lld -DCMAKE_AR=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-two-stage-fuchsia-toolchain/./bin/llvm-ar -DCMAKE_RANLIB=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-two-stage-fuchsia-toolchain/./bin/llvm-ranlib -DCMAKE_NM=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-two-stage-fuchsia-toolchain/./bin/llvm-nm -DCMAKE_OBJDUMP=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-two-stage-fuchsia-toolchain/./bin/llvm-objdump -DCMAKE_OBJCOPY=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-two-stage-fuchsia-toolchain/./bin/llvm-objcopy -DCMAKE_STRIP=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-two-stage-fuchsia-toolchain/./bin/llvm-strip -DCMAKE_C_COMPILER_TARGET=x86_64-unknown-linux-gnu -DCMAKE_CXX_COMPILER_TARGET=x86_64-unknown-linux-gnu -DCMAKE_ASM_COMPILER_TARGET=x86_64-unknown-linux-gnu -DCMAKE_INSTALL_PREFIX= -DCMAKE_SYSROOT=/usr/local/google/home/leonardchan/sysroot/linux-amd64 -DLLVM_BINARY_DIR=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-two-stage-fuchsia-toolchain -DLLVM_CONFIG_PATH=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-two-stage-fuchsia-toolchain/bin/llvm-config -DLLVM_ENABLE_WERROR=OFF -DLLVM_HOST_TRIPLE=x86_64-unknown-linux-gnu -DLLVM_HAVE_LINK_VERSION_SCRIPT=1 -DLLVM_USE_RELATIVE_PATHS_IN_DEBUG_INFO=OFF -DLLVM_USE_RELATIVE_PATHS_IN_FILES=OFF -DLLVM_LIT_ARGS=-sv -DLLVM_SOURCE_PREFIX= -DPACKAGE_VERSION=13.0.0git -DCMAKE_BUILD_TYPE=Release -DCMAKE_MAKE_PROGRAM=/usr/bin/ninja -DCMAKE_EXPORT_COMPILE_COMMANDS=1 -DLLVM_LIBRARY_OUTPUT_INTDIR=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-two-stage-fuchsia-toolchain/./lib -DLLVM_RUNTIME_OUTPUT_INTDIR=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-two-stage-fuchsia-toolchain/./bin -DLLVM_DEFAULT_TARGET_TRIPLE=x86_64-unknown-linux-gnu -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON -DCMAKE_C_COMPILER_WORKS=ON -DCMAKE_ASM_COMPILER_WORKS=ON -GNinja /usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-3/llvm/runtimes/../../compiler-rt/lib/builtins

which (ignoring some of the unecessary flags) is invoked by the higher-level two-stage cmake invocation to create builtins for x86_64, but with this patch, the same cmake invocation generates many targets for i386 builtins (seen in compile_commands.json). I'm unsure if perhaps this is directly related to your patch. It could be possible there's an underlying cmake issue this patch discovered. Do you know if this is something expected or might be directly caused by this patch?

I don't think this is an issue with this change, it's an issue on our side.

I don't think this is an issue with this change, it's an issue on our side.

Ok, let's hope so. This one has a bit of a risk of accidentally succdeeding in tests that were supposed to fail, but your commands look like they have i386 mixed up with x86_64.

A different potential culprit would be D98173, but I'd really hope that one wouldn't mix up i386 with x86_64.

I don't think this is an issue with this change, it's an issue on our side.

Did you guys manage to sort this one out, or is there still some lingering issue potentially related to this?

I don't think this is an issue with this change, it's an issue on our side.

Did you guys manage to sort this one out, or is there still some lingering issue potentially related to this?

I believe Petr was able to resolve this issue on our side, so no further changes needed. Thanks.