[libcxx] Fix allocator<void>::pointer in C++20 with removed members

[ilya-biryukov]

Summary

When compiled with -D_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS
uses of allocator<void>::pointer resulted in compiler errors after D104323.
If we instantiate the primary template, allocator<void>::reference produces
an error 'cannot form references to void'.

To workaround this, allow to bring back the allocator<void> specialization by defining the new _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION macro.

To make sure the code that uses allocator<void> and the removed members does not break,
both _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS and _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS have to be defined.

[philnik]

libcxx/include/__memory/allocator.h

30

I think using _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS is misleading. This doesn't just re-enable members, but an entire class specialization. Adding something like _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION would be the correct way IMO.

libcxx/test/std/utilities/memory/default.allocator/allocator_types.void.compile.cxx20_with_removed_members.pass.cpp

1 ↗

(On Diff #431381)

This is a libc++ extension, so it should go in libcxx/test/libcxx and not libcxx/test/std.

18–23 ↗

(On Diff #431381)

You can just make this a .compile.pass.cpp and remove the main().

[ilya-biryukov]

libcxx/include/__memory/allocator.h

30

That keeps allocator<void>::pointer broken with _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS defined and _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION not defined. Are we okay with that?

This specialization was there in the REMOVED_ALLOCATOR_MEMBERS mode before D104323 too, this change restores this behavior.

[philnik]

libcxx/include/__memory/allocator.h

30

I think it's Ok to have two flags for this, since the two changes are orthogonal IMO. But let's see what @ldionne thinks.

[ilya-biryukov]

• Move test to test/libcxx
• Switch test to 'compile.pass' and remove 'main'

[ilya-biryukov]

@ldionne friendly ping. Just want to make sure this does not fall off your radar.
Let me know if you need more time, no rush here from our side.

[ldionne]

In D126210#3537030, @ilya-biryukov wrote:

@ldionne friendly ping. Just want to make sure this does not fall off your radar.
Let me know if you need more time, no rush here from our side.

Thanks for the ping.

Let's also add a release note and document the option in libcxx/docs/UsingLibcxx.rst.

libcxx/include/__memory/allocator.h

30

Yes, I agree with using _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION for this.

[ilya-biryukov]

• Add _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION
• Document the new MACRO and add a release note

[ilya-biryukov]

Thanks for the quick response. I have addressed the comments, please take another look.

[ldionne]

libcxx/test/libcxx/utilities/memory/default.allocator/allocator_types.void.cxx20_allocator_void_no_members.fail.cpp

9–10

Can you explain what this test does? This sentence seems to be copy-pasted from the below test, but they are not checking the same thing.

[ilya-biryukov]

libcxx/test/libcxx/utilities/memory/default.allocator/allocator_types.void.cxx20_allocator_void_no_members.fail.cpp

9–10

It is copy-pasted indeed, thanks for catching that. I'll update the comment.
This test attempts to verify that the void specialization actually does not contain the removed members if the _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS is not specified.

[ilya-biryukov]

• Update test comment, only check in C++20 and beyond, add expected-errors

[ilya-biryukov]

I've update change description and the added test. This should fix failures on buildbots and address the comments.
PTAL.

Side question: could you also point me to the docs that show how to run the test with clang in -verify mode?
I have tried passing --param=cxx_under_test=/usr/bin/clang++ to lit, but that did not even change the compiler that runs under tests.
I ended up running clang directly on the test with -Xclang -verify, but that's definitely not how other people do it :)

[ilya-biryukov]

• Fix test failure

[ilya-biryukov]

Answering my own question here: I ended up pointing CC and CXX to Clang instead of GCC when running CMake. This makes lit add the -verify flag to tests.

[ilya-biryukov]

@ldionne @philnik friendly ping, could you please take another look?

[ilya-biryukov]

@ldionne @philnik another friendly ping, please take a look when you have a spare minute.

[philnik]

LGTM % nit.

libcxx/test/libcxx/utilities/memory/default.allocator/allocator_types.void.cxx20_allocator_void_no_members.fail.cpp

2

Could you make this a .verify.cpp?

[ldionne]

LGTM with some comments. Please pay attention to the suggestions, some of them contain more than one fix per line and it's easy to miss!

Thanks!

libcxx/docs/ReleaseNotes.rst

112–116

libcxx/docs/UsingLibcxx.rst

328–330

libcxx/test/libcxx/utilities/memory/default.allocator/allocator_types.void.cxx20_allocator_void_no_members.fail.cpp

11

[ilya-biryukov]

• Change test from .fail.cpp to .verify.cpp

[ilya-biryukov]

• Fix typos and apply suggestions in documentation and comments

libcxx/docs/ReleaseNotes.rst

 
 - Setting a custom debug handler with ``std::__libcpp_debug_function`` is not
   supported anymore. Please migrate to using the new support for
   :ref:`assertions <assertions-mode>` instead.
 
 - ``vector<bool>::const_reference``, ``vector<bool>::const_iterator::reference``
   and ``bitset::const_reference`` are now aliases for `bool` in the unstable ABI.
 
+- The ``_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION`` has been added to allow
+  re-enabling ``allocator<void>`` specialization. When used in conjuction with
+  ``_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS``, this ensures accesses to members of
+  ``allocator<void>`` do not cause compiler errors.
+
 ABI Changes
 -----------
 
 - The ``_LIBCPP_ABI_USE_CXX03_NULLPTR_EMULATION`` macro controlling whether we use an
   emulation for ``std::nullptr_t`` in C++03 mode has been removed. After this change,
   ``_LIBCPP_ABI_USE_CXX03_NULLPTR_EMULATION`` will not be honoured anymore and there
   will be no way to opt back into the C++03 emulation of ``std::nullptr_t``.
 

libcxx/docs/UsingLibcxx.rst

**_LIBCPP_ENABLE_CXX20_REMOVED_FEATURES**:
   This macro is used to re-enable all the features removed in C++20. The effect
   is equivalent to manually defining each macro listed below.
 
 **_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS**:
   This macro is used to re-enable redundant members of `allocator<T>`,
   including `pointer`, `reference`, `rebind`, `address`, `max_size`,
   `construct`, `destroy`, and the two-argument overload of `allocate`.
 
+**_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION**:
+  This macro is used to re-enable the library-provided specializations of
+  `allocator<void>` and `allocator<const void>`.
+  Use it in conjuction with `_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS`
+  to ensure uses of removed members inside `allocator<void>` do not cause
+  compile errors.
+
 **_LIBCPP_ENABLE_CXX20_REMOVED_BINDER_TYPEDEFS**:
   This macro is used to re-enable the `argument_type`, `result_type`,
   `first_argument_type`, and `second_argument_type` members of class
   templates such as `plus`, `logical_not`, `hash`, and `owner_less`.
 
 **_LIBCPP_ENABLE_CXX20_REMOVED_NEGATORS**:
   This macro is used to re-enable `not1`, `not2`, `unary_negate`,
   and `binary_negate`.

libcxx/include/__config

 #define _LIBCPP_ENABLE_CXX17_REMOVED_AUTO_PTR
 #define _LIBCPP_ENABLE_CXX17_REMOVED_BINDERS
 #define _LIBCPP_ENABLE_CXX17_REMOVED_RANDOM_SHUFFLE
 #define _LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS
 #endif // _LIBCPP_ENABLE_CXX17_REMOVED_FEATURES
 
 #if defined(_LIBCPP_ENABLE_CXX20_REMOVED_FEATURES)
 #define _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS
+#define _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION
 #define _LIBCPP_ENABLE_CXX20_REMOVED_BINDER_TYPEDEFS
 #define _LIBCPP_ENABLE_CXX20_REMOVED_NEGATORS
 #define _LIBCPP_ENABLE_CXX20_REMOVED_RAW_STORAGE_ITERATOR
 #define _LIBCPP_ENABLE_CXX20_REMOVED_TYPE_TRAITS
 #endif // _LIBCPP_ENABLE_CXX20_REMOVED_FEATURES
 
 #if !defined(__cpp_impl_coroutine) || __cpp_impl_coroutine < 201902L
 #define _LIBCPP_HAS_NO_CXX20_COROUTINES

libcxx/include/__memory/allocator.h

 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 template <class _Tp> class allocator;
 
-#if _LIBCPP_STD_VER <= 17
+#if _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION)
 template <>
 class _LIBCPP_TEMPLATE_VIS allocator<void>
 {
+#if _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS)
 public:
     _LIBCPP_DEPRECATED_IN_CXX17 typedef void*             pointer;
     _LIBCPP_DEPRECATED_IN_CXX17 typedef const void*       const_pointer;
     _LIBCPP_DEPRECATED_IN_CXX17 typedef void              value_type;
 
     template <class _Up> struct _LIBCPP_DEPRECATED_IN_CXX17 rebind {typedef allocator<_Up> other;};
+#endif
 };
 
 template <>
 class _LIBCPP_TEMPLATE_VIS allocator<const void>
 {
+#if _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS)
 public:
     _LIBCPP_DEPRECATED_IN_CXX17 typedef const void*       pointer;
     _LIBCPP_DEPRECATED_IN_CXX17 typedef const void*       const_pointer;
     _LIBCPP_DEPRECATED_IN_CXX17 typedef const void        value_type;
 
     template <class _Up> struct _LIBCPP_DEPRECATED_IN_CXX17 rebind {typedef allocator<_Up> other;};
+#endif
 };
 #endif
 
 // This class provides a non-trivial default constructor to the class that derives from it
 // if the condition is satisfied.
 //
 // The second template parameter exists to allow giving a unique type to __non_trivial_if,
 // which makes it possible to avoid breaking the ABI when making this a base class of an

libcxx/test/libcxx/utilities/memory/default.allocator/allocator_types.void.cxx20_allocator_void_no_members.fail.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
+//
+//===----------------------------------------------------------------------===//
+
+// Check that the nested types of std::allocator<void> are provided in C++20
+// with a flag that keeps the removed members.
+
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION
+
+#include <memory>
+#include <type_traits>
+
+static_assert((std::is_same<std::allocator<void>::pointer, void*>::value), "");
+static_assert((std::is_same<std::allocator<void>::const_pointer, const void*>::value), "");
+static_assert((std::is_same<std::allocator<void>::value_type, void>::value), "");
+static_assert((std::is_same<std::allocator<void>::rebind<int>::other, std::allocator<int> >::value), "");

libcxx/test/libcxx/utilities/memory/default.allocator/allocator_types.void.cxx20_with_removed_members.compile.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
+//
+//===----------------------------------------------------------------------===//
+
+// Check that the nested types of std::allocator<void> are provided in C++20
+// with a flag that keeps the removed members.
+
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION
+
+#include <memory>
+#include <type_traits>
+
+static_assert((std::is_same<std::allocator<void>::pointer, void*>::value), "");
+static_assert((std::is_same<std::allocator<void>::const_pointer, const void*>::value), "");
+static_assert((std::is_same<std::allocator<void>::value_type, void>::value), "");
+static_assert((std::is_same<std::allocator<void>::rebind<int>::other, std::allocator<int> >::value), "");