[libc++] Simplify how we pick the typeinfo comparison

[ldionne]

Summary

Selecting the right implementation for comparing typeinfos is not something
that we should configure in the config site. It's specific to the ABI in
use, and it should be detected automatically by the library. It has been
a source of confusion and bugs ever since we've introduced that.

Furthermore, it's really weird to set the comparison implementation
explicitly, yet for that choice not to be honored on arm64/Apple.

This commit removes the ability to explicitly select the implementation
of typeinfo comparison, and instead relies on the existing logic to select
the right one based on the ABI.

[smeenai]

We have a large internal codebase that I migrated from libstdc++ to libc++. libstdc++ uses relaxed typeinfo comparisons, and I found that just naively switching to libc++ produced hundreds of exception-related issues. Configuring libc++ to use the non-unique implementation was by far the easiest way to get this codebase working. I'm aware that in most cases, there is a proper fix for the exception issues, but (a) it's really hard to justify the large time investment required to fix all the different issues, and (b) there's some cases involving dlopen and RTLD_LOCAL that I believe are impossible to get right with the unique implementation (and I think that was the motivation for libstdc++ to switch to relaxed typeinfo comparisons).

Getting rid of the configurability seems problematic at least for ELF, where there's good reasons to want to use either the unique or non-unique implementations.

[ldionne]

In D91574#2398507, @smeenai wrote:

We have a large internal codebase that I migrated from libstdc++ to libc++. libstdc++ uses relaxed typeinfo comparisons, and I found that just naively switching to libc++ produced hundreds of exception-related issues. Configuring libc++ to use the non-unique implementation was by far the easiest way to get this codebase working. I'm aware that in most cases, there is a proper fix for the exception issues, but (a) it's really hard to justify the large time investment required to fix all the different issues, and (b) there's some cases involving dlopen and RTLD_LOCAL that I believe are impossible to get right with the unique implementation (and I think that was the motivation for libstdc++ to switch to relaxed typeinfo comparisons).

Getting rid of the configurability seems problematic at least for ELF, where there's good reasons to want to use either the unique or non-unique implementations.

Interesting, thanks for the information. This is the sort of feedback I was looking for when putting up this review.

In that case, what I'll do instead is clarify that LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION allows unconditionally overriding the implementation of typeinfo used, and have the non unique ARM implementation be _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION == 3 like in this patch. It'll get rid of most of the confusing-ness while still letting you override it.

[smeenai]

In D91574#2400066, @ldionne wrote:

In D91574#2398507, @smeenai wrote:

We have a large internal codebase that I migrated from libstdc++ to libc++. libstdc++ uses relaxed typeinfo comparisons, and I found that just naively switching to libc++ produced hundreds of exception-related issues. Configuring libc++ to use the non-unique implementation was by far the easiest way to get this codebase working. I'm aware that in most cases, there is a proper fix for the exception issues, but (a) it's really hard to justify the large time investment required to fix all the different issues, and (b) there's some cases involving dlopen and RTLD_LOCAL that I believe are impossible to get right with the unique implementation (and I think that was the motivation for libstdc++ to switch to relaxed typeinfo comparisons).

Getting rid of the configurability seems problematic at least for ELF, where there's good reasons to want to use either the unique or non-unique implementations.

Interesting, thanks for the information. This is the sort of feedback I was looking for when putting up this review.

In that case, what I'll do instead is clarify that LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION allows unconditionally overriding the implementation of typeinfo used, and have the non unique ARM implementation be _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION == 3 like in this patch. It'll get rid of most of the confusing-ness while still letting you override it.

Sounds great, thank you!

[ldionne]

Still allow customizing the implementation

[smeenai]

Thanks!

libcxx/docs/BuildingLibcxx.rst

381

Do you still wanna be removing this documentation?

[ldionne]

libcxx/docs/BuildingLibcxx.rst

381

That's a tough one. I kinda do.

I've been trying to avoid duplicate information in libc++, since that gets out of sync, etc. The set of options documented here does not reflect the real set of options provided in the CMake files, so I thought we might as well just have a single source of truth. For that reason, I tried to beef up the documentation of the option in libcxx/CMakeLists.txt itself.

Do you have an opinion about that?

[smeenai]

libcxx/docs/BuildingLibcxx.rst

381

That makes sense to me; the documentation in the CMakeLists.txt and the header looks good.

[shafik]

I think your CMakeLists.txt broke the green dragon build: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/24893/consoleFull#349140555e9a0fee5-ebcc-4238-a641-c5aa112c323e

It is not obvious to me what the problem is though.

[aheejin]

In D91574#2403954, @shafik wrote:

I think your CMakeLists.txt broke the green dragon build: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/24893/consoleFull#349140555e9a0fee5-ebcc-4238-a641-c5aa112c323e

It is not obvious to me what the problem is though.

I am experiencing the same error.

[ldionne]

You need to remove your build directory before running CMake. The issue is that CMake cache variables don't get overwritten with new values unless the cache is changed manually. If you don't remove the cache, it gets stale, and in this case, it's wrong.

Generally speaking, any CI job that doesn't either remove the whole build directory and start from scratch, or at least remove the CMake cache, is bound to be flaky and break in this way. This is a problem of the build job, not of this change.

Note that if you're trying to setup an incremental build and save on full rebuilds, I think it's better to use something like ccache than trying to keep the build directory around.

[ldionne]

So if you want to fix the Green Dragon issue, just remove the build directory. But I would advise fixing the root cause because this sort of failure may happen again, either with libcxx changes or any other change that falls in the same pattern.

[lebedev.ri]

This seems to have broken no-op rebuild w/ existing build dir:

CMake Error at /repositories/llvm-project/libcxx/CMakeLists.txt:187 (message):
  Value '' is not a valid value for

                         LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION


-- Configuring incomplete, errors occurred!

It seems to work after i manually specify -DLIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION=default, but still..

[fhahn]

In D91574#2404708, @lebedev.ri wrote:

This seems to have broken no-op rebuild w/ existing build dir:

CMake Error at /repositories/llvm-project/libcxx/CMakeLists.txt:187 (message):
  Value '' is not a valid value for

                         LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION


-- Configuring incomplete, errors occurred!

It seems to work after i manually specify -DLIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION=default, but still..

Same for me.

@ldionne would it be possible to adjust the default so that it doesn't break re-builds & works without explicitly having to set DLIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION?

[ldionne]

In D91574#2404957, @fhahn wrote:

In D91574#2404708, @lebedev.ri wrote:

This seems to have broken no-op rebuild w/ existing build dir:

CMake Error at /repositories/llvm-project/libcxx/CMakeLists.txt:187 (message):
  Value '' is not a valid value for

                         LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION


-- Configuring incomplete, errors occurred!

It seems to work after i manually specify -DLIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION=default, but still..

Same for me.

@ldionne would it be possible to adjust the default so that it doesn't break re-builds & works without explicitly having to set DLIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION?

Sorry, but no. The whole point of this patch is partly to make the default explicit. We've had bugs related to defining this as an empty string (which it was previously), which is what would appear to "satisfy" rebuilds.

I already said the bug was that these bots are not cleaning their build directory (or at least their build cache), and I stand by that.

[ldionne]

In D91574#2404708, @lebedev.ri wrote:

This seems to have broken no-op rebuild w/ existing build dir:

Please read two comments above yours. I explained the issue.

[JDevlieghere]

In D91574#2404340, @ldionne wrote:

You need to remove your build directory before running CMake. The issue is that CMake cache variables don't get overwritten with new values unless the cache is changed manually. If you don't remove the cache, it gets stale, and in this case, it's wrong.

Generally speaking, any CI job that doesn't either remove the whole build directory and start from scratch, or at least remove the CMake cache, is bound to be flaky and break in this way. This is a problem of the build job, not of this change.

Note that if you're trying to setup an incremental build and save on full rebuilds, I think it's better to use something like ccache than trying to keep the build directory around.

LLVM has a bunch of incremental bots and in addition to trying to be be fast, they also mirror what developers do when they pull. I can only speak for myself, but I certainly don't remove the CMake cache every time I do that. I've been involved in a bunch of CMake-related code reviews and as this is a pretty common problem, which I've seen people address by introducing a temporary CMake variable to let the bots (and developers) catch up. Is there any reason that couldn't have worked here?

FWIW if you disagree with how the bots deal with incremental builds, you're of course free to change that, but in the meantime the burden is still on the author of the patch and anyone could've reverted this in accordance with LLVM's "revert-to-green" policy.

[ldionne]

In D91574#2405856, @JDevlieghere wrote:

[...]

LLVM has a bunch of incremental bots and in addition to trying to be be fast, they also mirror what developers do when they pull. I can only speak for myself, but I certainly don't remove the CMake cache every time I do that. I've been involved in a bunch of CMake-related code reviews and as this is a pretty common problem, which I've seen people address by introducing a temporary CMake variable to let the bots (and developers) catch up. Is there any reason that couldn't have worked here?

The only trick I'm aware of is to temporarily use FORCE in the CMake set() command to force the cache to be overwritten. I've used that trick in the past to avoid exactly this kind of breakage, however in this case that would also break any build that explicitly sets that CMake variable (i.e. -DLIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION=XXX). So the reason why this broke bots is not that I didn't care about it, it's that I thought it was inevitable.

Now that you mention a temporary variable, I think I know where you're going. Do you mean:

1. Introduce a new cache variable with a different name, and make the code use it. That new variable should be initialized to the old one if it's set, to accommodate bots that set the old name explicitly.
2. set(FORCE) the old variable name to whatever you want.
3. Wait for the bots to catch up (like 1 day or so).
4. Move the code back to the original variable name, which will now have the correct default set by the set(FORCE).
5. Delete the temporary variable name, which is not useful anymore.

That sounds like it *might* work, however it's kind of complicated too, no? It seems to me like it would be better to acknowledge that CMake caches are invalidated from time to time, and make sure our infrastructure is robust against that.

FWIW if you disagree with how the bots deal with incremental builds, you're of course free to change that, but in the meantime the burden is still on the author of the patch and anyone could've reverted this in accordance with LLVM's "revert-to-green" policy.

I don't disagree with how they deal with incremental builds -- I just think we should be fine with a bit of human intervention from time to time if we're going to assume the cache never gets invalidated. And at the same time, I suggested a technical approach (full build + ccache) to make the bots more robust so that these issues don't happen again in the future.

[shafik]

In D91574#2405909, @ldionne wrote:

I don't disagree with how they deal with incremental builds -- I just think we should be fine with a bit of human intervention from time to time if we're going to assume the cache never gets invalidated. And at the same time, I suggested a technical approach (full build + ccache) to make the bots more robust so that these issues don't happen again in the future.

If you foresee the need for possible human intervention maybe the discussion about that should have happened before the change was landed? Then at least folks would have been prepared to fix the problem right after the it landed, assuming they agreed on that approach.

[ldionne]

In D91574#2405977, @shafik wrote:

In D91574#2405909, @ldionne wrote:

I don't disagree with how they deal with incremental builds -- I just think we should be fine with a bit of human intervention from time to time if we're going to assume the cache never gets invalidated. And at the same time, I suggested a technical approach (full build + ccache) to make the bots more robust so that these issues don't happen again in the future.

If you foresee the need for possible human intervention maybe the discussion about that should have happened before the change was landed? Then at least folks would have been prepared to fix the problem right after the it landed, assuming they agreed on that approach.

TBH, I didn't know which bots, if any, would break. We've moved some of the incremental builds to ccache + full build in the past, and I don't have a full mental map of all the bots in LLVM (I'm sure you don't, either). What I meant above is that I didn't purposefully break the build because I didn't care -- I thought it was inevitable (and it's still not clear to me it's not), and I wasn't sure whether it would actually break bots. When asked whether there was a technical way to unbreak the cache, I replied "no", which is the answer I thought was correct at the time (@JDevlieghere can LMK if I got his suggestion right).

Is it possible we're making this way bigger than it needs to be? If there's an agreed-upon way of making these CMake cache-affecting changes, then let's document it and be done with it. I'll be happy to do it in the future. If there's no such technical way, let's fix the bots to make sure they're robust to these changes, and somehow find a way to avoid breaking people's build as well when they git pull.

[JDevlieghere]

In D91574#2405909, @ldionne wrote:

In D91574#2405856, @JDevlieghere wrote:

[...]

LLVM has a bunch of incremental bots and in addition to trying to be be fast, they also mirror what developers do when they pull. I can only speak for myself, but I certainly don't remove the CMake cache every time I do that. I've been involved in a bunch of CMake-related code reviews and as this is a pretty common problem, which I've seen people address by introducing a temporary CMake variable to let the bots (and developers) catch up. Is there any reason that couldn't have worked here?

The only trick I'm aware of is to temporarily use FORCE in the CMake set() command to force the cache to be overwritten. I've used that trick in the past to avoid exactly this kind of breakage, however in this case that would also break any build that explicitly sets that CMake variable (i.e. -DLIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION=XXX). So the reason why this broke bots is not that I didn't care about it, it's that I thought it was inevitable.

Now that you mention a temporary variable, I think I know where you're going. Do you mean:

1. Introduce a new cache variable with a different name, and make the code use it. That new variable should be initialized to the old one if it's set, to accommodate bots that set the old name explicitly.
2. set(FORCE) the old variable name to whatever you want.
3. Wait for the bots to catch up (like 1 day or so).
4. Move the code back to the original variable name, which will now have the correct default set by the set(FORCE).
5. Delete the temporary variable name, which is not useful anymore.

That sounds like it *might* work, however it's kind of complicated too, no?

Yep, at least that's how we dealt with this for LLDB in the past.

It seems to me like it would be better to acknowledge that CMake caches are invalidated from time to time, and make sure our infrastructure is robust against that.

I totally agree. Some of the bots allow you to specify a variable (which admittedly is totally obscure) to request a clean build which is something else I've used in the past (when I had already landed something without the workaround and wanted to avoid reverting).

FWIW if you disagree with how the bots deal with incremental builds, you're of course free to change that, but in the meantime the burden is still on the author of the patch and anyone could've reverted this in accordance with LLVM's "revert-to-green" policy.

I don't disagree with how they deal with incremental builds -- I just think we should be fine with a bit of human intervention from time to time if we're going to assume the cache never gets invalidated. And at the same time, I suggested a technical approach (full build + ccache) to make the bots more robust so that these issues don't happen again in the future.

I think that's fair, but if you expect something like this to break, it might be nice to land it at a time where you can work with the bot owners to get it resolved quickly.

[JDevlieghere]

In D91574#2406048, @ldionne wrote:

In D91574#2405977, @shafik wrote:

In D91574#2405909, @ldionne wrote:

I don't disagree with how they deal with incremental builds -- I just think we should be fine with a bit of human intervention from time to time if we're going to assume the cache never gets invalidated. And at the same time, I suggested a technical approach (full build + ccache) to make the bots more robust so that these issues don't happen again in the future.

If you foresee the need for possible human intervention maybe the discussion about that should have happened before the change was landed? Then at least folks would have been prepared to fix the problem right after the it landed, assuming they agreed on that approach.

TBH, I didn't know which bots, if any, would break. We've moved some of the incremental builds to ccache + full build in the past, and I don't have a full mental map of all the bots in LLVM (I'm sure you don't, either). What I meant above is that I didn't purposefully break the build because I didn't care -- I thought it was inevitable (and it's still not clear to me it's not), and I wasn't sure whether it would actually break bots. When asked whether there was a technical way to unbreak the cache, I replied "no", which is the answer I thought was correct at the time (@JDevlieghere can LMK if I got his suggestion right).

Not that I know of, other than reverting and/or introducing the differently named (temporary) variable.

Is it possible we're making this way bigger than it needs to be? If there's an agreed-upon way of making these CMake cache-affecting changes, then let's document it and be done with it. I'll be happy to do it in the future. If there's no such technical way, let's fix the bots to make sure they're robust to these changes, and somehow find a way to avoid breaking people's build as well when they git pull.

It was not my intention to assign blame, I'm sorry if it came across that way. My goal was to make some suggestions on how this could've been avoided or at least less obtrusive in the future. I think documenting this might be worthwhile, although it doesn't happen that often, it can be annoying for a bunch of people.

libcxx/CMakeLists.txt

cmake_dependent_option(LIBCXX_INSTALL_EXPERIMENTAL_LIBRARY
         "LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY;LIBCXX_INSTALL_LIBRARY" OFF)
 
 set(LIBCXX_ABI_VERSION "1" CACHE STRING "ABI version of libc++. Can be either 1 or 2, where 2 is currently not stable. Defaults to 1.")
 set(LIBCXX_ABI_NAMESPACE "" CACHE STRING "The inline ABI namespace used by libc++. It defaults to __n where `n` is the current ABI version.")
 option(LIBCXX_ABI_UNSTABLE "Unstable ABI of libc++." OFF)
 option(LIBCXX_ABI_FORCE_ITANIUM "Ignore auto-detection and force use of the Itanium ABI.")
 option(LIBCXX_ABI_FORCE_MICROSOFT "Ignore auto-detection and force use of the Microsoft ABI.")
 
-
-set(LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION "" CACHE STRING "The implementation of typeinfo comparison to use.")
-set(TYPEINFO_COMPARISON_VALUES ";1;2")
-set_property(CACHE LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION PROPERTY STRINGS ${TYPEINFO_COMPARISON_VALUES})
-list(FIND TYPEINFO_COMPARISON_VALUES "${LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION}" IS_VALID_DEFAULT)
-if (${IS_VALID_DEFAULT} EQUAL -1)
+set(LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION "default" CACHE STRING
+  "Override the implementation to use for comparing typeinfos. By default, this
+   is detected automatically by the library, but this option allows overriding
+   which implementation is used unconditionally.
+
+   See the documentation in <libcxx/include/typeinfo> for details on what each
+   value means.")
+set(TYPEINFO_COMPARISON_VALUES "default;1;2;3")
+if (NOT ("${LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION}" IN_LIST TYPEINFO_COMPARISON_VALUES))
   message(FATAL_ERROR "Value '${LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION}' is not a valid value for
                        LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION")
 endif()
 
 option(LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT "Enable per TU ABI insulation by default. To be used by vendors." OFF)
 set(LIBCXX_ABI_DEFINES "" CACHE STRING "A semicolon separated list of ABI macros to define in the site config header.")
 option(LIBCXX_USE_COMPILER_RT "Use compiler-rt instead of libgcc" OFF)
 set(LIBCXX_LIBCPPABI_VERSION "2" CACHE STRING "Version of libc++abi's ABI to re-export from libc++ when re-exporting is enabled.
                                                Note that this is not related to the version of libc++'s ABI itself!")
 

 config_define_if(LIBCXX_ABI_FORCE_MICROSOFT _LIBCPP_ABI_FORCE_MICROSOFT)
 config_define_if(LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT _LIBCPP_HIDE_FROM_ABI_PER_TU_BY_DEFAULT)
 config_define_if_not(LIBCXX_ENABLE_GLOBAL_FILESYSTEM_NAMESPACE _LIBCPP_HAS_NO_GLOBAL_FILESYSTEM_NAMESPACE)
 config_define_if_not(LIBCXX_ENABLE_STDIN _LIBCPP_HAS_NO_STDIN)
 config_define_if_not(LIBCXX_ENABLE_STDOUT _LIBCPP_HAS_NO_STDOUT)
 config_define_if_not(LIBCXX_ENABLE_THREADS _LIBCPP_HAS_NO_THREADS)
 config_define_if_not(LIBCXX_ENABLE_MONOTONIC_CLOCK _LIBCPP_HAS_NO_MONOTONIC_CLOCK)
 config_define_if_not(LIBCXX_ENABLE_THREAD_UNSAFE_C_FUNCTIONS _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS)
-if (NOT LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION STREQUAL "")
+if (NOT LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION STREQUAL "default")
   config_define("${LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION}" _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION)
 endif()
-
 config_define_if(LIBCXX_HAS_PTHREAD_API _LIBCPP_HAS_THREAD_API_PTHREAD)
 config_define_if(LIBCXX_HAS_EXTERNAL_THREAD_API _LIBCPP_HAS_THREAD_API_EXTERNAL)
 config_define_if(LIBCXX_HAS_WIN32_THREAD_API _LIBCPP_HAS_THREAD_API_WIN32)
 config_define_if(LIBCXX_BUILD_EXTERNAL_THREAD_LIBRARY _LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL)
 config_define_if(LIBCXX_HAS_MUSL_LIBC _LIBCPP_HAS_MUSL_LIBC)
 config_define_if(LIBCXX_NO_VCRUNTIME _LIBCPP_NO_VCRUNTIME)
 config_define_if(LIBCXX_ENABLE_PARALLEL_ALGORITHMS _LIBCPP_HAS_PARALLEL_ALGORITHMS)
 config_define_if_not(LIBCXX_ENABLE_RANDOM_DEVICE _LIBCPP_HAS_NO_RANDOM_DEVICE)

libcxx/cmake/caches/Apple.cmake

 set(CMAKE_BUILD_TYPE MinSizeRel CACHE STRING "")
 set(CMAKE_POSITION_INDEPENDENT_CODE OFF CACHE BOOL "")
 
 set(LIBCXX_USE_COMPILER_RT ON CACHE BOOL "")
 set(LIBCXX_ENABLE_ASSERTIONS OFF CACHE BOOL "")
 set(LIBCXX_ABI_VERSION "1" CACHE STRING "")
 set(LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY OFF CACHE BOOL "")
 set(LIBCXX_ENABLE_STATIC OFF CACHE BOOL "")
 set(LIBCXX_ENABLE_SHARED ON CACHE BOOL "")
-set(LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION "1" CACHE STRING "")
 set(LIBCXX_CXX_ABI libcxxabi CACHE STRING "")
 set(LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT ON CACHE BOOL "")
 set(LIBCXX_ENABLE_DEBUG_MODE_SUPPORT OFF CACHE BOOL "")
 set(LIBCXX_ENABLE_VENDOR_AVAILABILITY_ANNOTATIONS ON CACHE BOOL "")
 
 set(LIBCXXABI_ENABLE_PIC OFF CACHE BOOL "")
 set(LIBCXXABI_ENABLE_ASSERTIONS OFF CACHE BOOL "")
 set(LIBCXXABI_ENABLE_FORGIVING_DYNAMIC_CAST ON CACHE BOOL "")

libcxx/docs/BuildingLibcxx.rst

 .. option:: LIBCXX_ABI_DEFINES:STRING
 
   **Default**: ``""``
 
   A semicolon-separated list of ABI macros to persist in the site config header.
   See ``include/__config`` for the list of ABI macros.
 
 
-.. option:: LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION
-
-  **Default**: ``None``, which lets the library figure out which implementation
-  to use based on the object format.
-
-  This setting defines what implementation to use for comparing typeinfo objects.
-  There are two main implementations, which differ on whether we make the assumption
-  that type info names for a type have been fully merged are unique across the entire
-  program. This may not be the case for libraries built with ``-Bsymbolic`` or due to
-  compiler or linker bugs (Ex. llvm.org/PR37398).
-
-
-  When the value is set to ``1``, we assume that typeinfos are unique across the
-  whole program, and typeinfo comparisons compare only the pointer value.
-
-  When the value is set to ``2``, we do not assume that typeinfos are unique across
-  the whole program. We first compare the pointers, and then use ``strcmp`` on the
-  typeinfo names as a fallback.
-
-
 .. _LLVM-specific variables:
 
 LLVM-specific options
 ---------------------
 
 .. option:: LLVM_LIBDIR_SUFFIX:STRING
 
   Extra suffix to append to the directory where libraries are to be

libcxx/include/__config

 #ifndef _LIBCPP_HIDE_FROM_ABI_PER_TU
 #  ifndef _LIBCPP_HIDE_FROM_ABI_PER_TU_BY_DEFAULT
 #    define _LIBCPP_HIDE_FROM_ABI_PER_TU 0
 #  else
 #    define _LIBCPP_HIDE_FROM_ABI_PER_TU 1
 #  endif
 #endif
 
-#ifndef _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION
-#  ifdef _LIBCPP_OBJECT_FORMAT_COFF // Windows binaries can't merge typeinfos.
-#    define _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION 2
-#  else
-     // TODO: This isn't strictly correct on ELF platforms due to llvm.org/PR37398
-     // And we should consider defaulting to OFF.
-#    define _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION 1
-#  endif
-#endif
-
 #ifndef _LIBCPP_HIDE_FROM_ABI
 #  if _LIBCPP_HIDE_FROM_ABI_PER_TU
 #    define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_INTERNAL_LINKAGE
 #  else
 #    define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION
 #  endif
 #endif
 

libcxx/include/__config_site.in

 #cmakedefine _LIBCPP_HAS_MUSL_LIBC
 #cmakedefine _LIBCPP_HAS_THREAD_API_PTHREAD
 #cmakedefine _LIBCPP_HAS_THREAD_API_EXTERNAL
 #cmakedefine _LIBCPP_HAS_THREAD_API_WIN32
 #cmakedefine _LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL
 #cmakedefine _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS
 #cmakedefine _LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS
 #cmakedefine _LIBCPP_NO_VCRUNTIME
-#ifndef _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION
 #cmakedefine _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION @_LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION@
-#endif
 #cmakedefine _LIBCPP_ABI_NAMESPACE @_LIBCPP_ABI_NAMESPACE@
 #cmakedefine _LIBCPP_HAS_PARALLEL_ALGORITHMS
 #cmakedefine _LIBCPP_HAS_NO_RANDOM_DEVICE
 #cmakedefine _LIBCPP_HAS_NO_LOCALIZATION
 
 @_LIBCPP_ABI_DEFINES@
 
 #endif // _LIBCPP_CONFIG_SITE

libcxx/include/typeinfo

 // This implementation of type_info does not assume there is always a unique
 // copy of the RTTI for a given type inside a program. For various reasons
 // the linker may have failed to merge every copy of a types RTTI
 // (For example: -Bsymbolic or llvm.org/PR37398). Under this assumption, two
 // type_infos are equal if their addresses are equal or if a deep string
 // comparison is equal.
 // -------------------------------------------------------------------------- //
 //                          NonUniqueARMRTTIBit
-// (selected on ARM64 regardless of _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION)
+//               (_LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION = 3)
 // -------------------------------------------------------------------------- //
 // This implementation of type_info does not assume always a unique copy of
 // the RTTI for a given type inside a program. It packs the pointer to the
 // type name into a uintptr_t and reserves the high bit of that pointer (which
 // is assumed to be free for use under the ABI in use) to represent whether
 // that specific copy of the RTTI can be assumed unique inside the program.
 // To implement equality-comparison of type_infos, we check whether BOTH
 // type_infos are guaranteed unique, and if so, we simply compare the addresses
 // of their type names instead of doing a deep string comparison, which is
 // faster. If at least one of the type_infos can't guarantee uniqueness, we
 // have no choice but to fall back to a deep string comparison.
 //
 // This implementation is specific to ARM64 on Apple platforms.
 //
 // Note that the compiler is the one setting (or unsetting) the high bit of
 // the pointer when it constructs the type_info, depending on whether it can
 // guarantee uniqueness for that specific type_info.
 
+// This value can be overriden in the __config_site. When it's not overriden,
+// we pick a default implementation based on the platform here.
+#ifndef _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION
+
+  // Windows binaries can't merge typeinfos, so use the NonUnique implementation.
+# ifdef _LIBCPP_OBJECT_FORMAT_COFF
+#   define _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION 2
+
+  // On arm64 on Apple platforms, use the special NonUniqueARMRTTIBit implementation.
+# elif defined(__APPLE__) && defined(__LP64__) && !defined(__x86_64__)
+#   define _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION 3
+
+  // On all other platforms, assume the Itanium C++ ABI and use the Unique implementation.
+# else
+#   define _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION 1
+# endif
+#endif
+
 struct __type_info_implementations {
   struct __string_impl_base {
     typedef const char* __type_name_t;
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_ALWAYS_INLINE
     _LIBCPP_CONSTEXPR static const char* __type_name_to_string(__type_name_t __v) _NOEXCEPT {
       return __v;
     }
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_ALWAYS_INLINE
private:
     }
     _LIBCPP_INLINE_VISIBILITY
     static bool __is_type_name_unique(__type_name_t __lhs, __type_name_t __rhs) _NOEXCEPT {
       return !((__lhs & __rhs) & __non_unique_rtti_bit::value);
     }
   };
 
   typedef
-#if defined(__APPLE__) && defined(__LP64__) && !defined(__x86_64__)
-    __non_unique_arm_rtti_bit_impl
-#elif _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION == 1
+#if _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION == 1
     __unique_impl
 #elif _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION == 2
     __non_unique_impl
+#elif _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION == 3
+    __non_unique_arm_rtti_bit_impl
 #else
 #   error invalid configuration for _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION
 #endif
      __impl;
 };
 
 class _LIBCPP_EXCEPTION_ABI type_info
 {