[CMake] Use -isystem flag to access libc++ headers

[phosek]

Summary

This is a partial revert of D62155. Rather than copying libc++ headers
into the build directory to be later overwritten by the final headers,
use -isystem flag to access libc++ headers during CMake checks. This
should address the occasional flake we've seen, especially on Windows
builders where CMake fails to overwrite __config with the final version.

[smeenai]

I was abusing this functionality to be able to get the C++ headers installed without having to build all the runtimes build prerequisites; we have a configuration where we only need the C++ headers installed (and not the library), and using the runtime-libcxx-headers was an easy way to get them without needing to first build Clang and all the other tools the runtimes build requires. I should be able to come up with something else for that use case though.

[phosek]

@beanz I've tested this on macOS and it seems to be working fine.

[ldionne]

I think that's a lot cleaner. Thanks!

[phosek]

@smeenai do you already have an alternative for your use case?

[smeenai]

In D88454#2304023, @phosek wrote:

@smeenai do you already have an alternative for your use case?

Yup. Thanks for checking!

[uabelho]

Hi!
I don't understand why, but with this commit I start seeing several warnings like this when compiling builtins:

/data/repo/master/compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:45:7: error: '__sanitizer::FlagHandler<bool>' has virtual functions but non-virtual destructor [-Werror,-Wnon-virtual-dtor]
class FlagHandler : public FlagHandlerBase {
      ^
/data/repo/master/compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:71:13: note: in instantiation of template class '__sanitizer::FlagHandler<bool>' requested here
inline bool FlagHandler<bool>::Parse(const char *value) {
            ^

[phosek]

In D88454#2307806, @uabelho wrote:

Hi!
I don't understand why, but with this commit I start seeing several warnings like this when compiling builtins:

/data/repo/master/compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:45:7: error: '__sanitizer::FlagHandler<bool>' has virtual functions but non-virtual destructor [-Werror,-Wnon-virtual-dtor]
class FlagHandler : public FlagHandlerBase {
      ^
/data/repo/master/compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:71:13: note: in instantiation of template class '__sanitizer::FlagHandler<bool>' requested here
inline bool FlagHandler<bool>::Parse(const char *value) {
            ^

That's a bit surprising. What's your build configuration? Do you use runtimes build? I haven't managed to reproduce it locally so far.

[bjope]

In D88454#2308086, @phosek wrote:

In D88454#2307806, @uabelho wrote:

Hi!
I don't understand why, but with this commit I start seeing several warnings like this when compiling builtins:

/data/repo/master/compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:45:7: error: '__sanitizer::FlagHandler<bool>' has virtual functions but non-virtual destructor [-Werror,-Wnon-virtual-dtor]
class FlagHandler : public FlagHandlerBase {
      ^
/data/repo/master/compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:71:13: note: in instantiation of template class '__sanitizer::FlagHandler<bool>' requested here
inline bool FlagHandler<bool>::Parse(const char *value) {
            ^

That's a bit surprising. What's your build configuration? Do you use runtimes build? I haven't managed to reproduce it locally so far.

Sorry if there are some local paths in there, but this is our cmake command (answering for @uabelho ):

CC='/proj/flexasic/app/llvm/8.0/bin/clang -march=corei7' CXX='/proj/flexasic/app/llvm/8.0/bin/clang++ -march=corei7' LDFLAGS='-L/proj/flexasic/app/llvm/8.0/lib64 -Wl,-R/proj/flexasic/app/llvm/8.0/lib64:/proj/bbi_twh/wh_bbi/x86_64-Linux3/z3/4.8.8-1/lib64' PATH=/proj/flexasic/app/ninja/1.8.2/SLED11-64/bin:$PATH  /app/vbuild/RHEL6-x86_64/cmake/3.16.4/bin/cmake /repo/uabbpet/llvm-upstream/llvm --debug-trycompile -G Ninja -DCMAKE_MAKE_PROGRAM=/proj/flexasic/app/ninja/1.8.2/SLED11-64/bin/ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_INSTALL_PREFIX=/compiler-clang -DLLVM_APPEND_VC_REV=OFF -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_ENABLE_WERROR=ON -DLLVM_ENABLE_PROJECTS='clang;clang-tools-extra' -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON -DLLVM_BUILTIN_TARGETS=default -DLLVM_ENABLE_RUNTIMES=all -DRUNTIMES_x86_64-unknown-linux-gnu_SANITIZER_USE_STATIC_LLVM_UNWINDER=ON -DRUNTIMES_x86_64-unknown-linux-gnu_SANITIZER_USE_STATIC_CXX_ABI=ON -DRUNTIMES_x86_64-unknown-linux-gnu_COMPILER_RT_USE_BUILTINS_LIBRARY=ON -DRUNTIMES_x86_64-unknown-linux-gnu_COMPILER_RT_CAN_EXECUTE_TESTS=OFF -DRUNTIMES_x86_64-unknown-linux-gnu_LIBCXX_HAS_ATOMIC_LIB=0 -DLLVM_RUNTIME_TARGETS='x86_64-unknown-linux-gnu' -DRUNTIMES_x86_64-unknown-linux-gnu_COMPILER_RT_BUILD_XRAY=OFF -DLLVM_ENABLE_LIBPFM=OFF

When it gets to the runtime build I start seeing some diffs in the config output, e.g. these used to be sucess, now we get "not found" and "Failed":

...
-- Looking for fopen in c - not found
-- Found compiler-rt builtins library: /repo/uabbpet/llvm-upstream/llvm/build-all-builtins/lib/clang/12.0.0/lib/x86_64-unknown-linux-gnu/libclang_rt.builtins.a
-- Performing Test COMPILER_RT_HAS_NODEFAULTLIBS_FLAG
-- Performing Test COMPILER_RT_HAS_NODEFAULTLIBS_FLAG - Success
-- Performing Test COMPILER_RT_HAS_FFREESTANDING_FLAG
-- Performing Test COMPILER_RT_HAS_FFREESTANDING_FLAG - Failed
-- Performing Test COMPILER_RT_HAS_STD_C11_FLAG
-- Performing Test COMPILER_RT_HAS_STD_C11_FLAG - Failed
-- Performing Test COMPILER_RT_HAS_FPIC_FLAG
-- Performing Test COMPILER_RT_HAS_FPIC_FLAG - Failed
-- Performing Test COMPILER_RT_HAS_FPIE_FLAG
-- Performing Test COMPILER_RT_HAS_FPIE_FLAG - Failed
-- Performing Test COMPILER_RT_HAS_FNO_BUILTIN_FLAG
-- Performing Test COMPILER_RT_HAS_FNO_BUILTIN_FLAG - Failed
-- Performing Test COMPILER_RT_HAS_FNO_EXCEPTIONS_FLAG
-- Performing Test COMPILER_RT_HAS_FNO_EXCEPTIONS_FLAG - Failed
-- Performing Test COMPILER_RT_HAS_FOMIT_FRAME_POINTER_FLAG
-- Performing Test COMPILER_RT_HAS_FOMIT_FRAME_POINTER_FLAG - Failed
-- Performing Test COMPILER_RT_HAS_FUNWIND_TABLES_FLAG
-- Performing Test COMPILER_RT_HAS_FUNWIND_TABLES_FLAG - Failed
-- Performing Test COMPILER_RT_HAS_FNO_STACK_PROTECTOR_FLAG
-- Performing Test COMPILER_RT_HAS_FNO_STACK_PROTECTOR_FLAG - Failed
-- Performing Test COMPILER_RT_HAS_FNO_SANITIZE_SAFE_STACK_FLAG
-- Performing Test COMPILER_RT_HAS_FNO_SANITIZE_SAFE_STACK_FLAG - Failed
...

Maybe that is expected and unrelated.
Also, the warnings we get now seem correct afaict (those structs have virtual functions but non-virtual dtor), even though it feels strange that they popped up when mergine this patch.

[bjope]

Looking at build.ninja we used to get

FLAGS = -fPIC -fvisibility-inlines-hidden -Werror -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 -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -Werror -std=c++14 -Wno-unused-parameter -O3     -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fno-sanitize=safe-stack -fvisibility=hidden -fno-lto -O3 -gline-tables-only -Wno-gnu -Wno-variadic-macros -Wno-c99-extensions -Wno-non-virtual-dtor -fno-rtti -Wframe-larger-than=570 -Wglobal-constructors -UNDEBUG

but now we get

FLAGS = -fPIC -fvisibility-inlines-hidden -Werror -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 -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -O3     -O3 -UNDEBUG

I haven't tracked down why we for example no longer get -Wno-non-virtual-dtor when building the sanitizers. But it looks like before this patch those warnings were suppressed, but not with this patch.

[phosek]

In D88454#2309261, @bjope wrote:

Looking at build.ninja we used to get

FLAGS = -fPIC -fvisibility-inlines-hidden -Werror -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 -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -Werror -std=c++14 -Wno-unused-parameter -O3     -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fno-sanitize=safe-stack -fvisibility=hidden -fno-lto -O3 -gline-tables-only -Wno-gnu -Wno-variadic-macros -Wno-c99-extensions -Wno-non-virtual-dtor -fno-rtti -Wframe-larger-than=570 -Wglobal-constructors -UNDEBUG

but now we get

FLAGS = -fPIC -fvisibility-inlines-hidden -Werror -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 -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -O3     -O3 -UNDEBUG

I haven't tracked down why we for example no longer get -Wno-non-virtual-dtor when building the sanitizers. But it looks like before this patch those warnings were suppressed, but not with this patch.

I think I figured it out, D88756 should address the issue.

[bjope]

In D88454#2309456, @phosek wrote:

I think I figured it out, D88756 should address the issue.

Thanks! I tried it, and that seems to resolve the problem for us!

libcxx/include/CMakeLists.txt

COMMAND ${Python3_EXECUTABLE} ${LIBCXX_SOURCE_DIR}/utils/cat_files.py
     -o ${LIBCXX_BINARY_DIR}/__generated_config
   DEPENDS ${LIBCXX_SOURCE_DIR}/include/__config
           ${LIBCXX_BINARY_DIR}/__config_site
 )
 # Add a target that executes the generation commands.
 add_custom_target(cxx-generated-config ALL
   DEPENDS ${LIBCXX_BINARY_DIR}/__generated_config)
 
-# In some build configurations (like bootstrapping clang), we need to be able to
-# install the libcxx headers before the CMake configuration for libcxx runs. Making
-# the name of this target configurable allows LLVM/runtimes/CMakeLists.txt to
-# add this subdirectory to the LLVM build to put libcxx's headers in place
-# before libcxx's build configuration is run.
-if (NOT CXX_HEADER_TARGET)
-  set(CXX_HEADER_TARGET cxx-headers)
-endif()
 if(LIBCXX_HEADER_DIR)
   set(output_dir ${LIBCXX_HEADER_DIR}/include/c++/v1)
 
   set(out_files)
   foreach(f ${files})
     set(src ${CMAKE_CURRENT_SOURCE_DIR}/${f})
     set(dst ${output_dir}/${f})
     add_custom_command(OUTPUT ${dst}
       DEPENDS ${src}
       COMMAND ${CMAKE_COMMAND} -E copy_if_different ${src} ${dst}
       COMMENT "Copying CXX header ${f}")
     list(APPEND out_files ${dst})
   endforeach()
 
   # Copy the generated header as __config into build directory.
   set(src ${LIBCXX_BINARY_DIR}/__generated_config)
   set(dst ${output_dir}/__config)
   add_custom_command(OUTPUT ${dst}
       DEPENDS ${src} cxx-generated-config
       COMMAND ${CMAKE_COMMAND} -E copy_if_different ${src} ${dst}
       COMMENT "Copying CXX __config")
   list(APPEND out_files ${dst})
   add_custom_target(generate-cxx-headers DEPENDS ${out_files})
 
-  add_library(${CXX_HEADER_TARGET} INTERFACE)
-  add_dependencies(${CXX_HEADER_TARGET} generate-cxx-headers ${LIBCXX_CXX_ABI_HEADER_TARGET})
+  add_library(cxx-headers INTERFACE)
+  add_dependencies(cxx-headers generate-cxx-headers ${LIBCXX_CXX_ABI_HEADER_TARGET})
   # TODO: Use target_include_directories once we figure out why that breaks the runtimes build
   if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC" OR "${CMAKE_CXX_SIMULATE_ID}" STREQUAL "MSVC")
-    target_compile_options(${CXX_HEADER_TARGET} INTERFACE /I "${output_dir}")
+    target_compile_options(cxx-headers INTERFACE /I "${output_dir}")
   else()
-    target_compile_options(${CXX_HEADER_TARGET} INTERFACE -I "${output_dir}")
+    target_compile_options(cxx-headers INTERFACE -I "${output_dir}")
   endif()
 
   # Make sure the generated __config_site header is included when we build the library.
   if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC" OR "${CMAKE_CXX_SIMULATE_ID}" STREQUAL "MSVC")
-    target_compile_options(${CXX_HEADER_TARGET} INTERFACE /FI "${LIBCXX_BINARY_DIR}/__config_site")
+    target_compile_options(cxx-headers INTERFACE /FI "${LIBCXX_BINARY_DIR}/__config_site")
   else()
-    target_compile_options(${CXX_HEADER_TARGET} INTERFACE -include "${LIBCXX_BINARY_DIR}/__config_site")
+    target_compile_options(cxx-headers INTERFACE -include "${LIBCXX_BINARY_DIR}/__config_site")
   endif()
 else()
-  add_library(${CXX_HEADER_TARGET} INTERFACE)
+  add_library(cxx-headers INTERFACE)
 endif()
 
 if (LIBCXX_INSTALL_HEADERS)
   foreach(file ${files})
     get_filename_component(dir ${file} DIRECTORY)
     install(FILES ${file}
       DESTINATION ${LIBCXX_INSTALL_HEADER_PREFIX}include/c++/v1/${dir}
-      COMPONENT ${CXX_HEADER_TARGET}
+      COMPONENT cxx-headers
       PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
     )
   endforeach()
 
   # Install the generated header as __config.
   install(FILES ${LIBCXX_BINARY_DIR}/__generated_config
     DESTINATION ${LIBCXX_INSTALL_HEADER_PREFIX}include/c++/v1
     PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
     RENAME __config
-    COMPONENT ${CXX_HEADER_TARGET})
+    COMPONENT cxx-headers)
 
   if (NOT CMAKE_CONFIGURATION_TYPES)
-    add_custom_target(install-${CXX_HEADER_TARGET}
-                      DEPENDS ${CXX_HEADER_TARGET} cxx-generated-config
+    add_custom_target(install-cxx-headers
+                      DEPENDS cxx-headers cxx-generated-config
                       COMMAND "${CMAKE_COMMAND}"
-                              -DCMAKE_INSTALL_COMPONENT=${CXX_HEADER_TARGET}
+                              -DCMAKE_INSTALL_COMPONENT=cxx-headers
                               -P "${CMAKE_BINARY_DIR}/cmake_install.cmake")
     # Stripping is a no-op for headers
-    add_custom_target(install-${CXX_HEADER_TARGET}-stripped DEPENDS install-${CXX_HEADER_TARGET})
+    add_custom_target(install-cxx-headers-stripped DEPENDS install-cxx-headers)
   endif()
 endif()

llvm/runtimes/CMakeLists.txt

if(${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_CURRENT_SOURCE_DIR})
   set(LLVM_PATH ${LLVM_BUILD_MAIN_SRC_DIR})
 
   if(APPLE)
     set(LLVM_ENABLE_LIBCXX ON CACHE BOOL "")
   endif()
 
   include(CheckLibraryExists)
   include(CheckCCompilerFlag)
+  include(CMakePushCheckState)
 
-  # We don't have libc++ (yet).
-  set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -nostdlib++")
+  cmake_push_check_state()
+
+  # We don't have libc++ (yet)...
+  set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -nostdinc++ -nostdlib++")
+
+  # ...but we need access to libc++ headers for CMake checks to succeed.
+  if (LLVM_EXTERNAL_LIBCXX_SOURCE_DIR AND "libcxx" IN_LIST LLVM_ENABLE_RUNTIMES)
+    set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -isystem ${LLVM_EXTERNAL_LIBCXX_SOURCE_DIR}/include")
+  endif()
 
   # Avoid checking whether the compiler is working.
   set(LLVM_COMPILER_CHECKED ON)
 
   # Handle common options used by all runtimes.
   include(AddLLVM)
   include(HandleLLVMOptions)
   include(FindPythonInterp)
 
-  # Remove the -nostdlib++ option we've added earlier.
-  string(REPLACE "-nostdlib++" "" CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS}")
+  cmake_pop_check_state()
 
   # Use libtool instead of ar if you are both on an Apple host, and targeting Apple.
   if(CMAKE_HOST_APPLE AND APPLE)
     include(UseLibtool)
   endif()
 
   # This can be used to detect whether we're in the runtimes build.
   set(RUNTIMES_BUILD ON)
else()
       configure_file(
         ${CMAKE_CURRENT_SOURCE_DIR}/Components.cmake.in
         ${LLVM_BINARY_DIR}/runtimes/Components.cmake)
     endif()
   endif()
 
 else() # if this is included from LLVM's CMake
   include(LLVMExternalProjectUtils)
-  if (LLVM_EXTERNAL_LIBCXX_SOURCE_DIR AND "libcxx" IN_LIST LLVM_ENABLE_RUNTIMES)
-    # This looks wrong, but libcxx's build actually wants the header dir to be
-    # the root build dir, not the include directory.
-    set(LIBCXX_BINARY_DIR ${LLVM_BINARY_DIR})
-    set(LIBCXX_SOURCE_DIR ${LLVM_EXTERNAL_LIBCXX_SOURCE_DIR})
-    set(LIBCXX_HEADER_DIR ${LLVM_BINARY_DIR})
-    set(CXX_HEADER_TARGET runtime-libcxx-headers)
-    add_subdirectory(${LLVM_EXTERNAL_LIBCXX_SOURCE_DIR}/include ${CXX_HEADER_TARGET})
-  endif()
 
   if(NOT LLVM_BUILD_RUNTIMES)
     set(EXTRA_ARGS EXCLUDE_FROM_ALL)
   endif()
 
   function(check_apple_target triple builtin_or_runtime)
     set(error "\
 compiler-rt for Darwin builds for all platforms and architectures using a \
function(runtime_default_target)
     set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default ON)
     # AIX should fold 32-bit & 64-bit arch libraries into a single archive.
     if (TARGET_TRIPLE MATCHES "aix")
       set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default OFF)
     endif()
 
     llvm_ExternalProject_Add(runtimes
                              ${CMAKE_CURRENT_SOURCE_DIR}
-                             DEPENDS ${ARG_DEPENDS} ${CXX_HEADER_TARGET}
+                             DEPENDS ${ARG_DEPENDS}
                              # Builtins were built separately above
                              CMAKE_ARGS -DCOMPILER_RT_BUILD_BUILTINS=Off
                                         -DLLVM_INCLUDE_TESTS=${LLVM_INCLUDE_TESTS}
                                         -DLLVM_DEFAULT_TARGET_TRIPLE=${TARGET_TRIPLE}
                                         -DLLVM_ENABLE_PROJECTS_USED=${LLVM_ENABLE_PROJECTS_USED}
                                         -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=${LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default}
                                         -DCMAKE_C_COMPILER_TARGET=${TARGET_TRIPLE}
                                         -DCMAKE_CXX_COMPILER_TARGET=${TARGET_TRIPLE}
function(runtime_register_target name target)
     endif()
 
     if(target IN_LIST LLVM_RUNTIME_BUILD_ID_LINK_TARGETS)
       list(APPEND EXTRA_ARGS STRIP_TOOL ${CMAKE_CURRENT_BINARY_DIR}/llvm-strip-link)
     endif()
 
     llvm_ExternalProject_Add(runtimes-${name}
                              ${CMAKE_CURRENT_SOURCE_DIR}
-                             DEPENDS ${${name}_deps} ${CXX_HEADER_TARGET}
+                             DEPENDS ${${name}_deps}
                              # Builtins were built separately above
                              CMAKE_ARGS -DCOMPILER_RT_BUILD_BUILTINS=Off
                                         -DLLVM_INCLUDE_TESTS=${LLVM_INCLUDE_TESTS}
                                         -DLLVM_DEFAULT_TARGET_TRIPLE=${target}
                                         -DLLVM_ENABLE_PROJECTS_USED=${LLVM_ENABLE_PROJECTS_USED}
                                         -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON
                                         -DCMAKE_C_COMPILER_TARGET=${target}
                                         -DCMAKE_CXX_COMPILER_TARGET=${target}