[libc++] Remove _LIBCPP_ABI_UNSTABLE

[ldionne]

Summary

Previously, _LIBCPP_ABI_UNSTABLE would be used interchangeably with
_LIBCPP_ABI_VERSION >= 2. This was confusing and creating unnecessary
complexity.

This patch removes _LIBCPP_ABI_UNSTABLE -- instead, the LIBCXX_ABI_UNSTABLE
CMake option will result in the LIBCXX_ABI_VERSION being set to '2', the
current unstable ABI. As a result, in the code, we only have _LIBCPP_ABI_VERSION
to check in order to query the current ABI version.

As a fly-by, this also defines the ABI namespace during CMake configuration
to reduce complexity in config. I believe it was previously done this
way because we used to try to use config_site as seldom as possible.
Now that we always ship a config_site, it doesn't really matter and
I think being explicit about how the library is configured in the config_site
is actually a feature.

[• Quuxplusone]

LGTM mod nits.

libcxx/CMakeLists.txt

188

Here we fail to actually say what the ABI version is set to when LIBCXX_ABI_UNSTABLE is specified. (The answer, after last week's D118989, is "2".)

libcxx/docs/BuildingLibcxx.rst

446

libcxx/test/libcxx/utilities/utility/pairs/pairs.pair/non_trivial_copy_move_ABI.pass.cpp

18

Here and below, I suggest XFAIL: freebsd.

libcxx/utils/gdb/libcxx/printers.py

10

These should work for objects compiled with either the stable ABI or the unstable ABI. (Or both? Is "both" a physical possibility?)

[ldionne]

Address comments and hopefully fix CI.

libcxx/utils/gdb/libcxx/printers.py

10

Good suggestion. I think the intent of the comment is to say that the pretty printers are ABI independent. I think it would be physically possible to compile a single object file with two libc++s on different ABI versions, but IDK how the pretty printers would handle it.

[ldionne]

I'm also adding a release note, in case some users were setting the macro manually (even though they were not supposed to).

@thakis You might also want to tweak your BUILD.gn

[ronlieb]

Hi
Looks like hip buildbot breaks from this
https://lab.llvm.org/buildbot/#/builders/165/builds/15363

[paulkirth]

Hi,

We're seeing breakage in Fuchsia's Clang canary builders for Linux (aarch64 & X86_64) that seem related to this patch, (though it may be related to D119036 ? I can't tell).

Sorry for the late notification. This was hidden by a breakage in an different LLVM component, so we missed this for a bit.

We're seeing errors of this nature:

In file included from /b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/libcxx/containers/sequences/deque/incomplete.pass.cpp:16:
In file included from /b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/deque:163:
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__config:64:11: error: '_LIBCPP_ABI_INCOMPLETE_TYPES_IN_DEQUE' macro redefined [-Werror,-Wmacro-redefined]
#  define _LIBCPP_ABI_INCOMPLETE_TYPES_IN_DEQUE
          ^
<command line>:6:9: note: previous definition is here
#define _LIBCPP_ABI_INCOMPLETE_TYPES_IN_DEQUE 1

You can find the builders here: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-arm64/b8822761889490955361/overview
the failing test in Clang :: CodeGen/aarch64-mops.c has already been addressed and is unrelated.

and newer ones here: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8822749181186387249/overview

[lebedev.ri]

libcxx/CMakeLists.txt

190–192

This requires build dir purge and cmake rerun,
because LIBCXX_ABI_NAMESPACE would be "" (empty) before,
and you don't force-recover it to "__${LIBCXX_ABI_VERSION}".

[ldionne]

In D119173#3306641, @paulkirth wrote:

Hi,

We're seeing breakage in Fuchsia's Clang canary builders for Linux (aarch64 & X86_64) that seem related to this patch, (though it may be related to D119036 ? I can't tell).

Sorry for the late notification. This was hidden by a breakage in an different LLVM component, so we missed this for a bit.
[...]

Thanks for the heads up. It was indeed caused by D119036, and should be fixed by eceb40183cbb9442c7f42e7a4b8324abb50c32ab.

Can you folks move the Fuchsia bots to our pre-commit CI infrastructure?

In D119173#3305963, @ronlieb wrote:

Hi
Looks like hip buildbot breaks from this
https://lab.llvm.org/buildbot/#/builders/165/builds/15363

Indeed, you need to nuke your CMakeCache.txt. Generally speaking, a build bot that reuses an existing cache is brittle and you should be re-performing the CMake configure step each time. If you are trying to increase caching, IMO you're better off with something like ccache.

libcxx/CMakeLists.txt

190–192

Technically you only need to clear your CMakeCache.txt.

[paulkirth]

In D119173#3307636, @ldionne wrote:

In D119173#3306641, @paulkirth wrote:

Hi,

We're seeing breakage in Fuchsia's Clang canary builders for Linux (aarch64 & X86_64) that seem related to this patch, (though it may be related to D119036 ? I can't tell).

Sorry for the late notification. This was hidden by a breakage in an different LLVM component, so we missed this for a bit.
[...]

Thanks for the heads up. It was indeed caused by D119036, and should be fixed by eceb40183cbb9442c7f42e7a4b8324abb50c32ab.

Can you folks move the Fuchsia bots to our pre-commit CI infrastructure?

@ldionne let me loop @phosek in, since he will be better able to answer you about that.

[phosek]

In D119173#3307920, @paulkirth wrote:

In D119173#3307636, @ldionne wrote:

In D119173#3306641, @paulkirth wrote:

Hi,

We're seeing breakage in Fuchsia's Clang canary builders for Linux (aarch64 & X86_64) that seem related to this patch, (though it may be related to D119036 ? I can't tell).

Sorry for the late notification. This was hidden by a breakage in an different LLVM component, so we missed this for a bit.
[...]

Thanks for the heads up. It was indeed caused by D119036, and should be fixed by eceb40183cbb9442c7f42e7a4b8324abb50c32ab.

Can you folks move the Fuchsia bots to our pre-commit CI infrastructure?

@ldionne let me loop @phosek in, since he will be better able to answer you about that.

We cannot move existing bots over since we would loose integration into the rest of Fuchsia's CI infrastructure which lets us test every toolchain against the rest of the platform. We have been thinking about setting up additional upstream builders for libc++ though.

[ldionne]

In D119173#3308285, @phosek wrote:

We cannot move existing bots over since we would loose integration into the rest of Fuchsia's CI infrastructure which lets us test every toolchain against the rest of the platform. We have been thinking about setting up additional upstream builders for libc++ though.

Right -- I am not requesting that you "move" your bots, simply that you provide one additional bot that we can use in our pre-commit CI. I am very serious about this -- I know everyone is busy but this is simple to set up and will make our lives much easier. These unforeseen breakages are a waste of libc++ developers' time in the long run since they can be avoided with proper pre-commit CI.

libcxx/CMakeLists.txt

cmake_dependent_option(LIBCXX_INSTALL_STATIC_LIBRARY
   "LIBCXX_ENABLE_STATIC;LIBCXX_INSTALL_LIBRARY" OFF)
 cmake_dependent_option(LIBCXX_INSTALL_SHARED_LIBRARY
   "Install the shared libc++ library." ON
   "LIBCXX_ENABLE_SHARED;LIBCXX_INSTALL_LIBRARY" OFF)
 cmake_dependent_option(LIBCXX_INSTALL_EXPERIMENTAL_LIBRARY
         "Install libc++experimental.a" ON
         "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_UNSTABLE "Use the unstable ABI of libc++. This is equivalent to specifying LIBCXX_ABI_VERSION=n, where n is the not-yet-stable version." OFF)
+if (LIBCXX_ABI_UNSTABLE)
+  set(abi_version "2")
+else()
+  set(abi_version "1")
+endif()
+set(LIBCXX_ABI_VERSION "${abi_version}" CACHE STRING "ABI version of libc++. Can be either 1 or 2, where 2 is currently the unstable ABI. Defaults to 1 unless LIBCXX_ABI_UNSTABLE is specified, in which case this is 2.")
+set(LIBCXX_ABI_NAMESPACE "__${LIBCXX_ABI_VERSION}" CACHE STRING "The inline ABI namespace used by libc++. It defaults to __n where `n` is the current ABI version.")
+if (NOT LIBCXX_ABI_NAMESPACE MATCHES "__.*")
+  message(FATAL_ERROR "LIBCXX_ABI_NAMESPACE must be a reserved identifier.")
+endif()
 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 "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.
 
target_compile_definitions(${target} PRIVATE
                                  _CRT_SECURE_NO_WARNINGS
                                  # Use the ISO conforming behaviour for conversion
                                  # in printf, scanf.
                                  _CRT_STDIO_ISO_WIDE_SPECIFIERS)
   endif()
 endfunction()
 
 # Configuration file flags =====================================================
-if (NOT LIBCXX_ABI_VERSION EQUAL 1)
-  config_define(${LIBCXX_ABI_VERSION} _LIBCPP_ABI_VERSION)
-endif()
-if (NOT LIBCXX_ABI_NAMESPACE STREQUAL "")
-  if (NOT LIBCXX_ABI_NAMESPACE MATCHES "__.*")
-    message(FATAL_ERROR "LIBCXX_ABI_NAMESPACE must be a reserved identifier.")
-  endif()
-  if (LIBCXX_ABI_NAMESPACE MATCHES "__[0-9]+$")
-    message(FATAL_ERROR "LIBCXX_ABI_NAMESPACE '${LIBCXX_ABI_NAMESPACE}' is reserved for use by libc++.")
-  endif()
-  config_define(${LIBCXX_ABI_NAMESPACE} _LIBCPP_ABI_NAMESPACE)
-endif()
-config_define_if(LIBCXX_ABI_UNSTABLE _LIBCPP_ABI_UNSTABLE)
 config_define_if(LIBCXX_ABI_FORCE_ITANIUM _LIBCPP_ABI_FORCE_ITANIUM)
 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_THREADS _LIBCPP_HAS_NO_THREADS)
 config_define_if_not(LIBCXX_ENABLE_MONOTONIC_CLOCK _LIBCPP_HAS_NO_MONOTONIC_CLOCK)
 if (NOT LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION STREQUAL "default")
   config_define("${LIBCXX_TYPEINFO_COMPARISON_IMPLEMENTATION}" _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION)
 endif()

libcxx/docs/BuildingLibcxx.rst

.. option:: LIBCXX_ABI_NAMESPACE:STRING
 
   **Default**: ``__n`` where ``n`` is the current ABI version.
 
   This option defines the name of the inline ABI versioning namespace. It can be used for building
   custom versions of libc++ with unique symbol names in order to prevent conflicts or ODR issues
   with other libc++ versions.
 
   .. warning::
-    When providing a custom namespace, it's the users responsibility to ensure the name won't cause
+    When providing a custom namespace, it's the user's responsibility to ensure the name won't cause
     conflicts with other names defined by libc++, both now and in the future. In particular, inline
-    namespaces of the form ``__[0-9]+`` are strictly reserved by libc++ and may not be used by users.
-    Doing otherwise could cause conflicts and hinder libc++ ABI evolution.
+    namespaces of the form ``__[0-9]+`` could cause conflicts with future versions of the library,
+    and so should be avoided.
 
 .. 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.
 

libcxx/include/__config

 #ifndef _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER
 #  pragma GCC system_header
 #endif
 
 #ifdef __cplusplus
 
 #define _LIBCPP_VERSION 15000
 
-#ifdef _LIBCPP_ABI_UNSTABLE
-#  define _LIBCPP_ABI_VERSION 2
-#endif
-
-#ifndef _LIBCPP_ABI_VERSION
-#  define _LIBCPP_ABI_VERSION 1
-#endif
-
 #if __STDC_HOSTED__ == 0
 #  define _LIBCPP_FREESTANDING
 #endif
 
 #ifndef _LIBCPP_STD_VER
 #  if  __cplusplus <= 201103L
 #    define _LIBCPP_STD_VER 11
 #  elif __cplusplus <= 201402L

 # define _LIBCPP_ABI_ENABLE_ADDITIONAL_IOSTREAM_EXPLICIT_INSTANTIATIONS_1
 
 // Define a key function for `bad_function_call` in the library, to centralize
 // its vtable and typeinfo to libc++ rather than having all other libraries
 // using that class define their own copies.
 #  define _LIBCPP_ABI_BAD_FUNCTION_CALL_KEY_FUNCTION
 #endif
 
-#define _LIBCPP_CONCAT1(_LIBCPP_X,_LIBCPP_Y) _LIBCPP_X##_LIBCPP_Y
-#define _LIBCPP_CONCAT(_LIBCPP_X,_LIBCPP_Y) _LIBCPP_CONCAT1(_LIBCPP_X,_LIBCPP_Y)
-
-#ifndef _LIBCPP_ABI_NAMESPACE
-# define _LIBCPP_ABI_NAMESPACE _LIBCPP_CONCAT(__,_LIBCPP_ABI_VERSION)
-#endif
-
 #if __cplusplus < 201103L
 #define _LIBCPP_CXX03_LANG
 #endif
 
 #ifndef __has_attribute
 #define __has_attribute(__x) 0
 #endif
 

libcxx/include/__config_site.in

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP_CONFIG_SITE
 #define _LIBCPP_CONFIG_SITE
 
 #cmakedefine _LIBCPP_ABI_VERSION @_LIBCPP_ABI_VERSION@
-#cmakedefine _LIBCPP_ABI_UNSTABLE
+#cmakedefine _LIBCPP_ABI_NAMESPACE @_LIBCPP_ABI_NAMESPACE@
 #cmakedefine _LIBCPP_ABI_FORCE_ITANIUM
 #cmakedefine _LIBCPP_ABI_FORCE_MICROSOFT
 #cmakedefine _LIBCPP_HIDE_FROM_ABI_PER_TU_BY_DEFAULT
 #cmakedefine _LIBCPP_HAS_NO_THREADS
 #cmakedefine _LIBCPP_HAS_NO_MONOTONIC_CLOCK
 #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
 #cmakedefine _LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION @_LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION@
-#cmakedefine _LIBCPP_ABI_NAMESPACE @_LIBCPP_ABI_NAMESPACE@
 #cmakedefine _LIBCPP_HAS_NO_FILESYSTEM_LIBRARY
 #cmakedefine _LIBCPP_HAS_PARALLEL_ALGORITHMS
 #cmakedefine _LIBCPP_HAS_NO_RANDOM_DEVICE
 #cmakedefine _LIBCPP_HAS_NO_LOCALIZATION
 #cmakedefine _LIBCPP_HAS_NO_WIDE_CHARACTERS
 #cmakedefine _LIBCPP_HAS_NO_INCOMPLETE_FORMAT
 #cmakedefine _LIBCPP_HAS_NO_INCOMPLETE_RANGES
 
 @_LIBCPP_ABI_DEFINES@
 @_LIBCPP_EXTRA_SITE_DEFINES@
 
 #endif // _LIBCPP_CONFIG_SITE

libcxx/test/libcxx/utilities/utility/pairs/pairs.pair/non_trivial_copy_move_ABI.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // The test suite needs to define the ABI macros on the command line when
 // modules are enabled.
 // UNSUPPORTED: modules-build
 
 // <utility>
 
 // template <class T1, class T2> struct pair
 
-// Test that we properly provide the trivial copy operations by default.
-
-// FreeBSD provides the old ABI. This test checks the new ABI so we need
-// to manually turn it on.
-#undef _LIBCPP_ABI_UNSTABLE
-#undef _LIBCPP_ABI_VERSION
-#define _LIBCPP_ABI_VERSION 1
-#define _LIBCPP_DEPRECATED_ABI_DISABLE_PAIR_TRIVIAL_COPY_CTOR
+// Test that we provide the non-trivial copy operations when _LIBCPP_DEPRECATED_ABI_DISABLE_PAIR_TRIVIAL_COPY_CTOR
+// is specified.
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DEPRECATED_ABI_DISABLE_PAIR_TRIVIAL_COPY_CTOR
 
 #include <utility>
 #include <type_traits>
 #include <cstdlib>
 #include <cstddef>
 #include <cassert>
 
 #include "test_macros.h"
 
-#if !defined(_LIBCPP_DEPRECATED_ABI_DISABLE_PAIR_TRIVIAL_COPY_CTOR)
-#error trivial ctor ABI macro defined
-#endif
-
 template <class T>
 struct HasNonTrivialABI : std::integral_constant<bool,
     !std::is_trivially_destructible<T>::value
     || (std::is_copy_constructible<T>::value && !std::is_trivially_copy_constructible<T>::value)
 #if TEST_STD_VER >= 11
    || (std::is_move_constructible<T>::value && !std::is_trivially_move_constructible<T>::value)
 #endif
 > {};

libcxx/test/libcxx/utilities/utility/pairs/pairs.pair/trivial_copy_move_ABI.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // <utility>
 
 // template <class T1, class T2> struct pair
 
 // Test that we properly provide the trivial copy operations by default.
 
-// FreeBSD provides the old ABI. This test checks the new ABI so we need
-// to manually turn it on.
-#if defined(__FreeBSD__)
-#define _LIBCPP_ABI_UNSTABLE
-#endif
+// FreeBSD still provides the old ABI for std::pair.
+// XFAIL: freebsd
 
 #include <utility>
 #include <type_traits>
 #include <cstdlib>
 #include <cstddef>
 #include <cassert>
 
 #include "test_macros.h"
 
-#if defined(_LIBCPP_DEPRECATED_ABI_DISABLE_PAIR_TRIVIAL_COPY_CTOR)
-#error Non-trivial ctor ABI macro defined
-#endif
-
 template <class T>
 struct HasTrivialABI : std::integral_constant<bool,
     std::is_trivially_destructible<T>::value
     && (!std::is_copy_constructible<T>::value || std::is_trivially_copy_constructible<T>::value)
 #if TEST_STD_VER >= 11
    && (!std::is_move_constructible<T>::value || std::is_trivially_move_constructible<T>::value)
 #endif
 > {};

libcxx/utils/gdb/libcxx/printers.py

 #===----------------------------------------------------------------------===##
 #
 # Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 #
 #===----------------------------------------------------------------------===##
 """GDB pretty-printers for libc++.
 
-These should work for objects compiled when _LIBCPP_ABI_UNSTABLE is defined
-and when it is undefined.
+These should work for objects compiled with either the stable ABI or the unstable ABI.
 """
 
 from __future__ import print_function
 
 import math
 import re
 import gdb
 

libcxx/utils/libcxx/test/features.py

 # (e.g. `_LIBCPP_ABI_VERSION=2`), the feature is `libcpp-xxx-yyy-zzz=<value>`.
 macros = {
   '_LIBCPP_HAS_NO_MONOTONIC_CLOCK': 'libcpp-has-no-monotonic-clock',
   '_LIBCPP_HAS_NO_THREADS': 'libcpp-has-no-threads',
   '_LIBCPP_HAS_THREAD_API_EXTERNAL': 'libcpp-has-thread-api-external',
   '_LIBCPP_HAS_THREAD_API_PTHREAD': 'libcpp-has-thread-api-pthread',
   '_LIBCPP_NO_VCRUNTIME': 'libcpp-no-vcruntime',
   '_LIBCPP_ABI_VERSION': 'libcpp-abi-version',
-  '_LIBCPP_ABI_UNSTABLE': 'libcpp-abi-unstable',
   '_LIBCPP_HAS_NO_FILESYSTEM_LIBRARY': 'libcpp-has-no-filesystem-library',
   '_LIBCPP_HAS_NO_RANDOM_DEVICE': 'libcpp-has-no-random-device',
   '_LIBCPP_HAS_NO_LOCALIZATION': 'libcpp-has-no-localization',
   '_LIBCPP_HAS_NO_WIDE_CHARACTERS': 'libcpp-has-no-wide-characters',
   '_LIBCPP_HAS_NO_INCOMPLETE_FORMAT': 'libcpp-has-no-incomplete-format',
   '_LIBCPP_HAS_NO_INCOMPLETE_RANGES': 'libcpp-has-no-incomplete-ranges',
   '_LIBCPP_HAS_NO_UNICODE': 'libcpp-has-no-unicode',
 }