[CMake] Check the builtins library value first

[phosek]

Summary

When the builtins library isn't found, find_compiler_rt_library
returns NOTFOUND so we'll end up linking against -lNOTFOUND. Check
the return value first.

[mstorsjo]

So this is for a case where a user has explicitly specified -DCOMPILER_RT_USE_BUILTINS_LIBRARY=YES but it turns out to not be found? Hmm, I'm not sure if it's better to just silently ignore it like this, or clearly error out . (The previous behaviour of erroring out due to -lNOTFOUND not found does error out, but not in a very understandable fashion I guess.)

[smeenai]

In D107627#2930630, @mstorsjo wrote:

So this is for a case where a user has explicitly specified -DCOMPILER_RT_USE_BUILTINS_LIBRARY=YES but it turns out to not be found? Hmm, I'm not sure if it's better to just silently ignore it like this, or clearly error out . (The previous behaviour of erroring out due to -lNOTFOUND not found does error out, but not in a very understandable fashion I guess.)

Agreed.

[phosek]

This was discovered after D107501 broke our build, which uncovered several CMake check failures:

Performing C++ SOURCE FILE Test COMPILER_RT_HAS_G_FLAG failed with the following output:
Change Dir: /Users/phosek/llvm/build/fuchsia/runtimes/runtimes-bins/CMakeFiles/CMakeTmp

Run Build Command(s):/Users/phosek/.brew/bin/ninja cmTC_85a84 && [1/2] Building CXX object CMakeFiles/cmTC_85a84.dir/src.cxx.o
[2/2] Linking CXX executable cmTC_85a84
FAILED: cmTC_85a84
: && /Users/phosek/llvm/build/fuchsia/./bin/clang++ --target=x86_64-apple-darwin20.6.0 -stdlib=libc++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -fdiagnostics-color -ffile-prefix-map=/Users/phosek/llvm/build/fuchsia/runtimes/runtimes-bins=../build/fuchsia/runtimes/runtimes-bins -ffile-prefix-map=/Users/phosek/llvm/llvm-project/= -no-canonical-prefixes  -nostdinc++ -nostdlib++ -nodefaultlibs -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk -Wl,-search_paths_first -Wl,-headerpad_max_install_names -stdlib=libc++ CMakeFiles/cmTC_85a84.dir/src.cxx.o -o cmTC_85a84  -lc  -lNOTFOUND && :
clang++: warning: argument unused during compilation: '-nostdinc++' [-Wunused-command-line-argument]
ld: library not found for -lNOTFOUND
clang++: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

The problem is that find_compiler_rt_library we use to find builtins fails on Darwin because the library isn't called libclang_rt.builtins.a there, rather it's called libclang_rt.$os.a where $os can be osx, ios, etc. Therefore, find_compiler_rt_library returns NOTFOUND but we don't validate the result before appending it to CMAKE_REQUIRED_LIBRARIES.

This is a latent bug that was just uncovered by D107501. The reason why this hasn't caused issues before is because the -nodefaultlibs flag check is also broken so we never include that flag in CMAKE_REQUIRED_FLAGS and the compiler always uses the default builtins library.

I agree with you, we should report an error when user sets -DCOMPILER_RT_USE_BUILTINS_LIBRARY=YES and we fail to find builtins, but before we can do that, we also need to fix find_compiler_rt_library on Darwin and the -nodefaultlibs check. I'm looking into all of these issues, but addressing them is going to take a bit longer so I was hoping to unbreak our build with this (hopefully temporary) workaround.

[smeenai]

That sounds good to me. Thanks for the additional context!

[phosek]

In D107627#2931672, @smeenai wrote:

That sounds good to me. Thanks for the additional context!

Thanks, I filed bugs for this and left a TODO in the code to make sure this doesn't get lost.

[smeenai]

compiler-rt/cmake/config-ix.cmake

38–41

Typo.

[mstorsjo]

Thanks for the extra comments! With that context and those extra comments, I also agree it's totally fine (and quite harmless) to use this as a workaround.

compiler-rt/cmake/config-ix.cmake

function(compiler_rt_check_linker_flag flag out_var)
   check_cxx_compiler_flag("" ${out_var})
   cmake_pop_check_state()
 endfunction()
 
 check_library_exists(c fopen "" COMPILER_RT_HAS_LIBC)
 if (COMPILER_RT_USE_BUILTINS_LIBRARY)
   include(HandleCompilerRT)
   find_compiler_rt_library(builtins "" COMPILER_RT_BUILTINS_LIBRARY)
+  # TODO(PR51389): We should check COMPILER_RT_BUILTINS_LIBRARY and report an
+  # error if the value is NOTFOUND rather than silenty continuing but we first
+  # need to fix find_compiler_rt_library on Darwin.
 else()
   if (ANDROID)
     check_library_exists(gcc __gcc_personality_v0 "" COMPILER_RT_HAS_GCC_LIB)
   else()
     check_library_exists(gcc_s __gcc_personality_v0 "" COMPILER_RT_HAS_GCC_S_LIB)
   endif()
 endif()
 
 check_c_compiler_flag(-nodefaultlibs COMPILER_RT_HAS_NODEFAULTLIBS_FLAG)
 if (COMPILER_RT_HAS_NODEFAULTLIBS_FLAG)
   set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -nodefaultlibs")
   if (COMPILER_RT_HAS_LIBC)
     list(APPEND CMAKE_REQUIRED_LIBRARIES c)
   endif ()
   if (COMPILER_RT_USE_BUILTINS_LIBRARY)
+    # TODO: remote this check once we address PR51389.
+    if (${COMPILER_RT_BUILTINS_LIBRARY})
       list(APPEND CMAKE_REQUIRED_LIBRARIES "${COMPILER_RT_BUILTINS_LIBRARY}")
+    endif()
   elseif (COMPILER_RT_HAS_GCC_S_LIB)
     list(APPEND CMAKE_REQUIRED_LIBRARIES gcc_s)
   elseif (COMPILER_RT_HAS_GCC_LIB)
     list(APPEND CMAKE_REQUIRED_LIBRARIES gcc)
   endif ()
   if (MINGW)
     # Mingw64 requires quite a few "C" runtime libraries in order for basic
     # programs to link successfully with -nodefaultlibs.