[libcxx-commits] [libcxx] 53623d4 - [libc++] Always generate a __config_site header
Louis Dionne via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Jun 25 21:48:45 PDT 2020
Author: Louis Dionne
Date: 2020-06-26T00:47:48-04:00
New Revision: 53623d4aa7105dd431b388c6fddea3a71b52ab14
URL: https://github.com/llvm/llvm-project/commit/53623d4aa7105dd431b388c6fddea3a71b52ab14
DIFF: https://github.com/llvm/llvm-project/commit/53623d4aa7105dd431b388c6fddea3a71b52ab14.diff
LOG: [libc++] Always generate a __config_site header
Before this patch, the __config_site header was only generated when at
least one __config_site macro needed to be defined. This lead to two
different code paths in how libc++ is configured, depending on whether
a __config_site header was generated or not. After this patch, the
__config_site is always generated, but it can be empty in case there
are no macros to define in it.
More context on why this change is important
--------------------------------------------
In addition to being confusing, this double-code-path situation lead to
broken code being checked in undetected in 2405bd689815, which introduced
the LIBCXX_HAS_MERGED_TYPEINFO_NAMES_DEFAULT CMake setting. Specifically,
the _LIBCPP_HAS_MERGED_TYPEINFO_NAMES_DEFAULT <__config_site> macro was
supposed NOT to be defined unless LIBCXX_HAS_MERGED_TYPEINFO_NAMES_DEFAULT
was specified explicitly on the CMake command line. Instead, what happened
is that it was defined to 0 if it wasn't specified explicitly and a
<__config_site> header was generated. And defining that macro to 0 had
the important effect of using the non-unique RTTI comparison implementation,
which changes the ABI.
This change in behavior wasn't noticed because the <__config_site> header
is not generated by default. However, the Apple configuration does cause
a <__config_site> header to be generated, which lead to the wrong RTTI
implementation being used, and to https://llvm.org/PR45549. We came close
to an ABI break in the dylib, but were saved due to a downstream-only
change that overrode the decision of the <__config_site> for the purpose
of RTTI comparisons in libc++abi. This is an incredible luck that we should
not rely on ever again.
While the problem itself was fixed with 2464d8135e2a by setting
LIBCXX_HAS_MERGED_TYPEINFO_NAMES_DEFAULT explicitly in the Apple
CMake cache and then in d0fcdcd28f95 by making the setting less
brittle, the point still is that we should have had a single code
path from the beginning. Unlike most normal libraries, the macros
that configure libc++ are really complex, there's a lot of them and
they control important properties of the C++ runtime. There must be
a single code path for that, and it must be simple and robust.
Differential Revision: https://reviews.llvm.org/D80927
Added:
Modified:
libcxx/CMakeLists.txt
libcxx/benchmarks/CMakeLists.txt
libcxx/cmake/Modules/HandleLibcxxFlags.cmake
libcxx/include/CMakeLists.txt
Removed:
################################################################################
diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
index 75088403f029..f884456b1d9e 100644
--- a/libcxx/CMakeLists.txt
+++ b/libcxx/CMakeLists.txt
@@ -874,22 +874,15 @@ if (DEFINED WIN32 AND LIBCXX_ENABLE_STATIC AND NOT LIBCXX_ENABLE_SHARED)
endif()
set(site_config_path "${LIBCXX_BINARY_DIR}/__config_site")
-if (LIBCXX_NEEDS_SITE_CONFIG)
- configure_file("include/__config_site.in"
- "${site_config_path}"
- @ONLY)
-elseif(EXISTS "${site_config_path}")
- message(STATUS "Removing stale site configuration ${site_config_path}")
- file(REMOVE "${site_config_path}")
-endif()
+configure_file("include/__config_site.in"
+ "${site_config_path}"
+ @ONLY)
function(cxx_add_config_site target)
- if (LIBCXX_NEEDS_SITE_CONFIG)
- if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC" OR "${CMAKE_CXX_SIMULATE_ID}" STREQUAL "MSVC")
- target_compile_options(${target} PUBLIC /FI "${site_config_path}")
- else()
- target_compile_options(${target} PUBLIC -include "${site_config_path}")
- endif()
+ if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC" OR "${CMAKE_CXX_SIMULATE_ID}" STREQUAL "MSVC")
+ target_compile_options(${target} PUBLIC /FI "${site_config_path}")
+ else()
+ target_compile_options(${target} PUBLIC -include "${site_config_path}")
endif()
endfunction()
diff --git a/libcxx/benchmarks/CMakeLists.txt b/libcxx/benchmarks/CMakeLists.txt
index bd38de97d7a9..f012cccb696e 100644
--- a/libcxx/benchmarks/CMakeLists.txt
+++ b/libcxx/benchmarks/CMakeLists.txt
@@ -18,9 +18,7 @@ if (DEFINED LIBCXX_CXX_ABI_LIBRARY_PATH)
-L${LIBCXX_CXX_ABI_LIBRARY_PATH}
-Wl,-rpath,${LIBCXX_CXX_ABI_LIBRARY_PATH})
endif()
-if (LIBCXX_NEEDS_SITE_CONFIG)
- list(APPEND BENCHMARK_LIBCXX_COMPILE_FLAGS -include "${LIBCXX_BINARY_DIR}/__config_site")
-endif()
+list(APPEND BENCHMARK_LIBCXX_COMPILE_FLAGS -include "${LIBCXX_BINARY_DIR}/__config_site")
split_list(BENCHMARK_LIBCXX_COMPILE_FLAGS)
ExternalProject_Add(google-benchmark-libcxx
diff --git a/libcxx/cmake/Modules/HandleLibcxxFlags.cmake b/libcxx/cmake/Modules/HandleLibcxxFlags.cmake
index b0fb98b9866e..a5b4df6600ee 100644
--- a/libcxx/cmake/Modules/HandleLibcxxFlags.cmake
+++ b/libcxx/cmake/Modules/HandleLibcxxFlags.cmake
@@ -88,20 +88,17 @@ endmacro()
macro(config_define_if condition def)
if (${condition})
set(${def} ON)
- set(LIBCXX_NEEDS_SITE_CONFIG ON)
endif()
endmacro()
macro(config_define_if_not condition def)
if (NOT ${condition})
set(${def} ON)
- set(LIBCXX_NEEDS_SITE_CONFIG ON)
endif()
endmacro()
macro(config_define value def)
set(${def} ${value})
- set(LIBCXX_NEEDS_SITE_CONFIG ON)
endmacro()
# Add a list of flags to all of 'CMAKE_CXX_FLAGS', 'CMAKE_C_FLAGS',
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index 3625dd9f6286..c913ef9eb771 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -184,27 +184,20 @@ if(LIBCXX_INSTALL_SUPPORT_HEADERS)
)
endif()
-if (LIBCXX_NEEDS_SITE_CONFIG)
- # Generate a custom __config header. The new header is created
- # by prepending __config_site to the current __config header.
- add_custom_command(OUTPUT ${LIBCXX_BINARY_DIR}/__generated_config
- COMMAND ${Python3_EXECUTABLE} ${LIBCXX_SOURCE_DIR}/utils/cat_files.py
- ${LIBCXX_BINARY_DIR}/__config_site
- ${LIBCXX_SOURCE_DIR}/include/__config
- -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)
- set(generated_config_deps cxx-generated-config)
-else()
- set(files
- ${files}
- __config
- )
-endif()
+# Generate a custom __config header. The new header is created
+# by prepending __config_site to the current __config header.
+add_custom_command(OUTPUT ${LIBCXX_BINARY_DIR}/__generated_config
+ COMMAND ${Python3_EXECUTABLE} ${LIBCXX_SOURCE_DIR}/utils/cat_files.py
+ ${LIBCXX_BINARY_DIR}/__config_site
+ ${LIBCXX_SOURCE_DIR}/include/__config
+ -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)
+set(generated_config_deps cxx-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
@@ -228,16 +221,14 @@ if(NOT LIBCXX_USING_INSTALLED_LLVM AND LIBCXX_HEADER_DIR)
list(APPEND out_files ${dst})
endforeach()
- if (LIBCXX_NEEDS_SITE_CONFIG)
- # 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} ${generated_config_deps}
- COMMAND ${CMAKE_COMMAND} -E copy_if_
diff erent ${src} ${dst}
- COMMENT "Copying CXX __config")
- list(APPEND out_files ${dst})
- endif()
+ # 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} ${generated_config_deps}
+ COMMAND ${CMAKE_COMMAND} -E copy_if_
diff erent ${src} ${dst}
+ COMMENT "Copying CXX __config")
+ list(APPEND out_files ${dst})
add_custom_target(${CXX_HEADER_TARGET} ALL DEPENDS ${out_files} ${LIBCXX_CXX_ABI_HEADER_TARGET})
else()
@@ -255,14 +246,12 @@ if (LIBCXX_INSTALL_HEADERS)
)
endforeach()
- if (LIBCXX_NEEDS_SITE_CONFIG)
- # 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})
- endif()
+ # 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})
if (NOT CMAKE_CONFIGURATION_TYPES)
add_custom_target(install-${CXX_HEADER_TARGET}
More information about the libcxx-commits
mailing list