[libc++] Disallow volatile types in std::allocator

[jloser]

Summary

LWG 2447 is marked as Complete, but there is no static_assert to
reject volatile types in std::allocator. See the discussion at
https://reviews.llvm.org/D108856.

Add static_assert in std::allocator to disallow volatile types. Since this
is an implementation choice, mark the binding test as libc++ only.

Remove tests that use containers backed by std::allocator that test
the container when used with a volatile type.

[ldionne]

Instead, would it be possible to add a static_assert in std::allocator that we're not passing a volatile type, and leave this as completed? Technically, I think this requires no change at all (and so we are correct in saying that we implement it) since the program is free to be ill-formed if one passes a volatile type to std::allocator (well, the resulting std::allocator<T> is not required to be a Cpp17Allocator). But I think it's good to explicitly error-out.

We could also add a libc++ specific test for that behavior.

[jloser]

Add static_assert in std::allocator to reject cv volatile types.
Add libc++ specific test

[jloser]

In D109056#2987735, @ldionne wrote:

Instead, would it be possible to add a static_assert in std::allocator that we're not passing a volatile type, and leave this as completed? Technically, I think this requires no change at all (and so we are correct in saying that we implement it) since the program is free to be ill-formed if one passes a volatile type to std::allocator (well, the resulting std::allocator<T> is not required to be a Cpp17Allocator). But I think it's good to explicitly error-out.

We could also add a libc++ specific test for that behavior.

That works for me. I just added a static_assert in std::allocator along with a libc++ verify test. Let me know what you think.

We can gate the static_assert on C++17 and later if you prefer (since it was raised as a C++17 issue); right now I left it to apply to all standards.

[jloser]

We have some tests that show we can use std::make_shared<T> when T is volatile type (see make_shared.volatile.pass.cpp). If we want this to work still, we can't have the static_assert in std::allocator.

@ldionne do you still think we should reject volatile types in std::allocator?

[jloser]

Update std::make_shared tests to not worry about testing support for volatile types.
Use __is_volatile so std::allocator rejects volatile types correctly on all standard modes (including C++03).

[• Quuxplusone]

libstdc++ actually broke and then deliberately restored support for make_shared<volatile T> in GCC 8, circa September 2018.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87278
Therefore I think we need very strong justification for libc++ to stop supporting make_shared<volatile T>.

If the only problem is that make_shared<T> has an unnecessary dependency on allocator<T>, maybe we could do something like

--- a/libcxx/include/__memory/shared_ptr.h
+++ b/libcxx/include/__memory/shared_ptr.h
@@ -1109,7 +1109,7 @@ template<class _Tp, class ..._Args, class = __enable_if_t<!is_array<_Tp>::value>
 _LIBCPP_HIDE_FROM_ABI
 shared_ptr<_Tp> make_shared(_Args&& ...__args)
 {
-    return _VSTD::allocate_shared<_Tp>(allocator<_Tp>(), _VSTD::forward<_Args>(__args)...);
+    return _VSTD::allocate_shared<_Tp>(allocator<int>(), _VSTD::forward<_Args>(__args)...);
 }

to work around that. The allocator gets rebound anyway, so there's no real reason to bother instantiating allocator<_Tp> in particular... and all allocator<T>s happen to have the same layout, so it wouldn't even break ABI to do this IMHO.

FWIW I also think the status quo is fine: so allocator<volatile T> is IFNDR instead of a static_assert; does anyone really care?

[ldionne]

In D109056#2995692, @Quuxplusone wrote:

libstdc++ actually broke and then deliberately restored support for make_shared<volatile T> in GCC 8, circa September 2018.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87278
Therefore I think we need very strong justification for libc++ to stop supporting make_shared<volatile T>.

If the only problem is that make_shared<T> has an unnecessary dependency on allocator<T>, maybe we could do something like

--- a/libcxx/include/__memory/shared_ptr.h
+++ b/libcxx/include/__memory/shared_ptr.h
@@ -1109,7 +1109,7 @@ template<class _Tp, class ..._Args, class = __enable_if_t<!is_array<_Tp>::value>
 _LIBCPP_HIDE_FROM_ABI
 shared_ptr<_Tp> make_shared(_Args&& ...__args)
 {
-    return _VSTD::allocate_shared<_Tp>(allocator<_Tp>(), _VSTD::forward<_Args>(__args)...);
+    return _VSTD::allocate_shared<_Tp>(allocator<int>(), _VSTD::forward<_Args>(__args)...);
 }

to work around that. The allocator gets rebound anyway, so there's no real reason to bother instantiating allocator<_Tp> in particular... and all allocator<T>s happen to have the same layout, so it wouldn't even break ABI to do this IMHO.

FWIW I also think the status quo is fine: so allocator<volatile T> is IFNDR instead of a static_assert; does anyone really care?

Simply put, I disagree with Jonathan's call on https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87278. As he said there, the Committee is moving away from supporting volatile in the standard library, and I think that sort of benign breakage is acceptable. After all, the Standard says that allocator<volatile T> is IFNDR, so we shouldn't encourage people to use that type.

libcxx/include/__memory/allocator.h

83

You can use the is_volatile type trait, since we support that trait even in C++03 mode. Generally speaking, libc++ supports all C++11 type traits in C++03 language mode as well.

[jloser]

Remove tests that use containers with a (const) volatile type since they would trigger the static_assert.
Use is_volatile since the type trait exists even in C++03 mode.

[ldionne]

LGTM once CI passes.

libcxx/test/libcxx/memory/allocator_volatile.verify.cpp

15–16

So this works in C++03 mode.

[jloser]

Fix test to not use auto so it works in C++03 mode.

[jloser]

In D109056#3010910, @ldionne wrote:

LGTM once CI passes.

Do we want to land this if CI is passing shortly or do internal builds at Apple/Google first for this breakage?

[jloser]

Rebase to fix CI failures fixed on main now

[ldionne]

Let's land this, I don't foresee any issues, and we can always revert in case something bad happens. I'll run a build in the meantime.

Please apply my suggested change just for consistency.

libcxx/test/libcxx/memory/allocator_volatile.verify.cpp

12

libcxx/TODO.TXT

 * Improve the quality and portability of the locale test data.
 * Convert failure tests to use Clang Verify.
 
 Misc Tasks
 ==========
 * Find all sequences of >2 underscores and eradicate them.
 * run clang-tidy on libc++
 * Document the "conditionally-supported" bits of libc++
-* Put a static_assert in std::allocator to deny const/volatile types (LWG 2447)

libcxx/include/__memory/allocator.h

 //
 // Note: For ABI compatibility between C++20 and previous standards, we make
 //       allocator<void> trivial in C++20.
 
 template <class _Tp>
 class _LIBCPP_TEMPLATE_VIS allocator
     : private __non_trivial_if<!is_void<_Tp>::value, allocator<_Tp> >
 {
+    static_assert(!is_volatile<_Tp>::value, "std::allocator does not support volatile types");
 public:
     typedef size_t      size_type;
     typedef ptrdiff_t   difference_type;
     typedef _Tp         value_type;
     typedef true_type   propagate_on_container_move_assignment;
     typedef true_type   is_always_equal;
 
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
#if _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS)
     }
 #endif
 };
 
 template <class _Tp>
 class _LIBCPP_TEMPLATE_VIS allocator<const _Tp>
     : private __non_trivial_if<!is_void<_Tp>::value, allocator<const _Tp> >
 {
+    static_assert(!is_volatile<_Tp>::value, "std::allocator does not support volatile types");
 public:
     typedef size_t      size_type;
     typedef ptrdiff_t   difference_type;
     typedef const _Tp   value_type;
     typedef true_type   propagate_on_container_move_assignment;
     typedef true_type   is_always_equal;
 
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17

libcxx/test/libcxx/memory/allocator_volatile.verify.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
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: libc++
+
+// LWG 2447 gives implementors freedom to reject volatile types in `std::allocator`.
+
+#include <memory>
+
+auto A1 = std::allocator<volatile int>{}; // expected-error@*:* {{std::allocator does not support volatile types}}
+auto A2 = std::allocator<const volatile int>{}; // expected-error@*:* {{std::allocator does not support volatile types}}

libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp

void test()
     test_not_const<std::array<               int, 0>>();
     test_not_const<std::array<               int, 1>>();
     test_false    <std::array<const          int, 1>>();
     test_not_const<std::array<      volatile int, 1>>();
     test_false    <std::array<const volatile int, 1>>();
     test_true     <std::deque<               int>>();
 #ifdef _LIBCPP_VERSION
     test_true     <std::deque<const          int>>();
-    test_true     <std::deque<      volatile int>>();
-    test_true     <std::deque<const volatile int>>();
 #endif // _LIBCPP_VERSION
     test_true     <std::forward_list<int>>();
     test_true     <std::list<int>>();
     test_true     <std::vector<int>>();
 
     // Associative containers
     test_true     <std::set<int>>();
     test_true     <std::map<int, int>>();
     test_true     <std::multiset<int>>();
     test_true     <std::multimap<int, int>>();
 
     // Unordered associative containers
     test_true     <std::unordered_set<int>>();
     test_true     <std::unordered_map<int, int>>();
     test_true     <std::unordered_multiset<int>>();
     test_true     <std::unordered_multimap<int, int>>();
 
     // Container adaptors
     test_true     <std::stack<               int>>();
 #ifdef _LIBCPP_VERSION
     test_true     <std::stack<const          int>>();
-    test_true     <std::stack<      volatile int>>();
-    test_true     <std::stack<const volatile int>>();
 #endif // _LIBCPP_VERSION
     test_true     <std::queue<int>>();
     test_true     <std::priority_queue<int>>();
 
     test_true     <std::span<               int>>();
     test_true     <std::span<const          int>>();
     test_true     <std::span<      volatile int>>();
     test_true     <std::span<const volatile int>>();

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp

void test_pointer_to_function() {
       std::shared_ptr<Result()> y(theFunction, resultDeletor);
     }
     assert(resultDeletorCount == 2);
 }
 #else // _LIBCPP_VERSION
 void test_pointer_to_function() {}
 #endif // _LIBCPP_VERSION
 
+template <typename T>
+void test(const T &t0)
+{
+    {
+      T t1 = t0;
+      std::shared_ptr<T> p0 = std::make_shared<T>(t0);
+      std::shared_ptr<T> p1 = std::make_shared<T>(t1);
+      assert(*p0 == t0);
+      assert(*p1 == t1);
+    }
+
+    {
+      const T t1 = t0;
+      std::shared_ptr<const T> p0 = std::make_shared<const T>(t0);
+      std::shared_ptr<const T> p1 = std::make_shared<const T>(t1);
+      assert(*p0 == t0);
+      assert(*p1 == t1);
+    }
+}
+
 int main(int, char**)
 {
     int nc = globalMemCounter.outstanding_new;
     {
     int i = 67;
     char c = 'e';
     std::shared_ptr<A> p = std::make_shared<A>(i, c);
     assert(globalMemCounter.checkOutstandingNewEq(nc+1));
#if TEST_STD_VER >= 11
     assert(globalMemCounter.checkOutstandingNewEq(nc+1));
     assert(A::count == 1);
     assert(p->get_int() == 67);
     assert(p->get_char() == 'e');
     }
 #endif
     assert(A::count == 0);
 
+    test<bool>(true);
+    test<int>(3);
+    test<double>(5.0);
+
   return 0;
 }

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.volatile.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
-//
-//===----------------------------------------------------------------------===//
-
-// <memory>
-
-// shared_ptr
-
-// template<class T, class... Args> shared_ptr<T> make_shared(Args&&... args);
-
-#include <memory>
-#include <cassert>
-
-#include "test_macros.h"
-
-template <typename T>
-void test(const T &t0)
-{
-    {
-    T t1 = t0;
-    std::shared_ptr<T> p0 = std::make_shared<T>(t0);
-    std::shared_ptr<T> p1 = std::make_shared<T>(t1);
-    assert(*p0 == t0);
-    assert(*p1 == t1);
-    }
-
-    {
-    const T t1 = t0;
-    std::shared_ptr<const T> p0 = std::make_shared<const T>(t0);
-    std::shared_ptr<const T> p1 = std::make_shared<const T>(t1);
-    assert(*p0 == t0);
-    assert(*p1 == t1);
-    }
-
-    {
-    volatile T t1 = t0;
-    std::shared_ptr<volatile T> p0 = std::make_shared<volatile T>(t0);
-    std::shared_ptr<volatile T> p1 = std::make_shared<volatile T>(t1);
-    assert(*p0 == t0);
-    assert(*p1 == t1);
-    }
-
-    {
-    const volatile T t1 = t0;
-    std::shared_ptr<const volatile T> p0 = std::make_shared<const volatile T>(t0);
-    std::shared_ptr<const volatile T> p1 = std::make_shared<const volatile T>(t1);
-    assert(*p0 == t0);
-    assert(*p1 == t1);
-    }
-
-}
-
-int main(int, char**)
-{
-    test<bool>(true);
-    test<int>(3);
-    test<double>(5.0);
-
-  return 0;
-}