[libcxx-commits] [PATCH] D80927: [libc++] Always generate a __config_site header

[Louis Dionne via Phabricator via libcxx-commits]

ldionne created this revision.
Herald added subscribers: libcxx-commits, dexonsmith, jkorous, mgorny.
Herald added a project: libc++.
Herald added a reviewer: libc++.

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 <https://reviews.llvm.org/rG2464d8135e2a4dbcf821ff254012995c05e20fa0> by setting
LIBCXX_HAS_MERGED_TYPEINFO_NAMES_DEFAULT explicitly in the Apple
CMake cache and then in d0fcdcd28f95 <https://reviews.llvm.org/rGd0fcdcd28f95d699b27d2026ede964a7f9cff9dd> 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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80927

Files:
  libcxx/CMakeLists.txt
  libcxx/benchmarks/CMakeLists.txt
  libcxx/cmake/Modules/HandleLibcxxFlags.cmake
  libcxx/include/CMakeLists.txt

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D80927.267621.patch
Type: text/x-patch
Size: 6268 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20200601/4789accd/attachment-0001.bin>