[all-commits] [llvm/llvm-project] 53623d: [libc++] Always generate a __config_site header

[Louis Dionne via All-commits]

  Branch: refs/heads/master
  Home:   https://github.com/llvm/llvm-project
  Commit: 53623d4aa7105dd431b388c6fddea3a71b52ab14
      https://github.com/llvm/llvm-project/commit/53623d4aa7105dd431b388c6fddea3a71b52ab14
  Author: Louis Dionne <ldionne at apple.com>
  Date:   2020-06-26 (Fri, 26 Jun 2020)

  Changed paths:
    M libcxx/CMakeLists.txt
    M libcxx/benchmarks/CMakeLists.txt
    M libcxx/cmake/Modules/HandleLibcxxFlags.cmake
    M libcxx/include/CMakeLists.txt

  Log Message:
  -----------
  [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