[libcxx-commits] [PATCH] D80927: [libc++] Always generate a __config_site header
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Jun 1 08:00:52 PDT 2020
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.
rG LLVM Github Monorepo
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 6268 bytes
Desc: not available
More information about the libcxx-commits