This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Always generate a __config_site header
ClosedPublic

Authored by ldionne on Jun 1 2020, 8:00 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG53623d4aa710: [libc++] Always generate a __config_site header
Summary

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.

Diff Detail

Event Timeline

ldionne created this revision.Jun 1 2020, 8:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2020, 8:00 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Jun 25 2020, 9:45 PM

Got LGTM from EricWF offline. We've been talking about this for a while.

This revision is now accepted and ready to land.Jun 25 2020, 9:45 PM
This revision was automatically updated to reflect the committed changes.