[libc++] Refactor allocate_shared to use an allocation guard

[ldionne]

Summary

This commit is a step towards making it easier to add support for arrays
in allocate_shared. Adding support for arrays will require writing multiple
functions, and the current complexity of writing allocate_shared is
prohibitive for understanding.

[ldionne]

Rebase

[saugustine]

This change appears to have dropped a constructor, or maybe two. I'm not familiar enough with all the requirements to know if that is correct or not. Here is a somewhat redacted version of the errors I get building a private application.

In module 'xyz':
.../include/c++/v1/memory:3341:48: error: no matching constructor for initialization of 'allocation_guard<_Foo>' (aka 'allocation_guard<Allocator<std::u::shared_ptr_emplace<Bar::Bat, Baz::Ball::Allocator<Bar::Bat>>>>')

__allocation_guard<_Foo> __guard(__a, 1);
                                           ^       ~~~~~~

source.cc:57:15: note: in instantiation of function template specialization 'std::__u::allocate_shared<Bar::Bat, Baz::Ball::Allocator<Bar::Bat>, Bar::Bat::ConstructorToken, std::string_view &, Bar::BatOptions &, void>' requested here

return std::allocate_shared<MemoryTracker>(Alloc(), ConstructorToken(), name,
            ^

.../include/c++/v1/__memory/utilities.h:52:14: note: candidate constructor not viable: no known conversion from 'const Allocator<Bar::Bat>' to 'Allocator<std::u::shared_ptr_emplace<Bar::Bat, Baz::Ball::Allocator<Bar::Bat>>>' for 1st argument

explicit __allocation_guard(_Alloc __alloc, _Size __n)
         ^

.../include/c++/v1/__memory/utilities.h:47:8: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 2 were provided
struct __allocation_guard {

^

In module 'xyz':
.../include/c++/v1/memory:3341:48: error: no matching constructor for initialization of 'allocation_guard<_Foo>' (aka 'allocation_guard<Allocator<std::u::shared_ptr_emplace<Bar::Bat, Baz::Ball::Allocator<Bar::Bat>>>>')

__allocation_guard<_Foo> __guard(__a, 1);
                                           ^       ~~~~~~

source.cc:229:16: note: in instantiation of function template specialization 'std::__u::allocate_shared<Bar::Bat, Baz::Ball::Allocator<Bar::Bat>, Bar::Bat::ConstructorToken, char const (&)[7], Bar::BatOptions, void>' requested here

std::allocate_shared<MemoryTracker>(
     ^

.../include/c++/v1/__memory/utilities.h:52:14: note: candidate constructor not viable: no known conversion from 'const Allocator<Bar::Bat>' to 'Allocator<std::u::shared_ptr_emplace<Bar::Bat, Baz::Ball::Allocator<Bar::Bat>>>' for 1st argument

explicit __allocation_guard(_Alloc __alloc, _Size __n)
         ^

.../include/c++/v1/__memory/utilities.h:47:8: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 2 were provided
struct __allocation_guard {

[ldionne]

In D93130#2453963, @saugustine wrote:

This change appears to have dropped a constructor, or maybe two. I'm not familiar enough with all the requirements to know if that is correct or not. Here is a somewhat redacted version of the errors I get building a private application.
...

Is it possible that your custom Allocator has its converting copy constructor (from other specializations of Allocator) marked as explicit? We don't seem to have a test for that, I'll add one and fix this, but please confirm that it's the case.

[saugustine]

In D93130#2455145, @ldionne wrote:

In D93130#2453963, @saugustine wrote:

This change appears to have dropped a constructor, or maybe two. I'm not familiar enough with all the requirements to know if that is correct or not. Here is a somewhat redacted version of the errors I get building a private application.
...

Is it possible that your custom Allocator has its converting copy constructor (from other specializations of Allocator) marked as explicit? We don't seem to have a test for that, I'll add one and fix this, but please confirm that it's the case.

Yes, in fact it does. Thanks for the quick follow up.

[ldionne]

In D93130#2455196, @saugustine wrote:

In D93130#2455145, @ldionne wrote:

In D93130#2453963, @saugustine wrote:

This change appears to have dropped a constructor, or maybe two. I'm not familiar enough with all the requirements to know if that is correct or not. Here is a somewhat redacted version of the errors I get building a private application.
...

Is it possible that your custom Allocator has its converting copy constructor (from other specializations of Allocator) marked as explicit? We don't seem to have a test for that, I'll add one and fix this, but please confirm that it's the case.

Yes, in fact it does. Thanks for the quick follow up.

Ok, that was it. Fixed by:

commit a00290ed10a6b4e9f6e9be44ceec367562f270c6 (HEAD -> main, github-llvm/main)
Author: Louis Dionne <[e-mail]>
Date:   Tue Dec 15 11:45:53 2020 -0500

    [libc++] Fix allocate_shared when used with an explicitly convertible allocator

    When the allocator is only explicitly convertible from other specializations
    of itself, the new version of std::allocate_shared would not work because
    it would try to do an implicit conversion. This patch fixes the problem
    and adds a test so that we don't fall into the same trap in the future.

libcxx/include/CMakeLists.txt

 set(files
   __availability
   __bit_reference
   __bsd_locale_defaults.h
   __bsd_locale_fallbacks.h
   __debug
   __errc
   __functional_03
   __functional_base
   __functional_base_03
   __hash_table
   __libcpp_version
   __locale
   __memory/allocator_traits.h
   __memory/base.h
   __memory/pointer_traits.h
+  __memory/utilities.h
   __mutex_base
   __node_handle
   __nullptr
   __split_buffer
   __sso_allocator
   __std_stream
   __string
   __threading_support

libcxx/include/__memory/utilities.h

+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
+//
+// 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___MEMORY_UTILITIES_H
+#define _LIBCPP___MEMORY_UTILITIES_H
+
+#include <__config>
+#include <__memory/allocator_traits.h>
+#include <cstddef>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#pragma GCC system_header
+#endif
+
+_LIBCPP_PUSH_MACROS
+#include <__undef_macros>
+
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+// Helper class to allocate memory using an Allocator in an exception safe
+// manner.
+//
+// The intended usage of this class is as follows:
+//
+// 0
+// 1     __allocation_guard<SomeAllocator> guard(alloc, 10);
+// 2     do_some_initialization_that_may_throw(guard.__get());
+// 3     save_allocated_pointer_in_a_noexcept_operation(guard.__release_ptr());
+// 4
+//
+// If line (2) throws an exception during initialization of the memory, the
+// guard's destructor will be called, and the memory will be released using
+// Allocator deallocation. Otherwise, we release the memory from the guard on
+// line (3) in an operation that can't throw -- after that, the guard is not
+// responsible for the memory anymore.
+//
+// This is similar to a unique_ptr, except it's easier to use with a
+// custom allocator.
+template<class _Alloc>
+struct __allocation_guard {
+    using _Pointer = typename allocator_traits<_Alloc>::pointer;
+    using _Size = typename allocator_traits<_Alloc>::size_type;
+
+    _LIBCPP_HIDE_FROM_ABI
+    explicit __allocation_guard(_Alloc __alloc, _Size __n)
+        : __alloc_(_VSTD::move(__alloc))
+        , __n_(__n)
+        , __ptr_(allocator_traits<_Alloc>::allocate(__alloc_, __n_)) // initialization order is important
+    { }
+
+    _LIBCPP_HIDE_FROM_ABI
+    ~__allocation_guard() _NOEXCEPT {
+        if (__ptr_ != nullptr) {
+            allocator_traits<_Alloc>::deallocate(__alloc_, __ptr_, __n_);
+        }
+    }
+
+    _LIBCPP_HIDE_FROM_ABI
+    _Pointer __release_ptr() _NOEXCEPT { // not called __release() because it's a keyword in objective-c++
+        _Pointer __tmp = __ptr_;
+        __ptr_ = nullptr;
+        return __tmp;
+    }
+
+    _LIBCPP_HIDE_FROM_ABI
+    _Pointer __get() const _NOEXCEPT {
+        return __ptr_;
+    }
+
+private:
+    _Alloc __alloc_;
+    _Size __n_;
+    _Pointer __ptr_;
+};
+
+_LIBCPP_END_NAMESPACE_STD
+
+_LIBCPP_POP_MACROS
+
+#endif  // _LIBCPP___MEMORY_UTILITIES_H

libcxx/include/memory

 #include <__functional_base>
 #include <iosfwd>
 #include <tuple>
 #include <stdexcept>
 #include <cstring>
 #include <__memory/allocator_traits.h>
 #include <__memory/base.h>
 #include <__memory/pointer_traits.h>
+#include <__memory/utilities.h>
 #if !defined(_LIBCPP_HAS_NO_ATOMIC_HEADER)
 #  include <atomic>
 #endif
 #include <version>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
<
     __compatible_with<_Yp, typename shared_ptr<_Tp>::element_type>::value,
     void
 >::type
 shared_ptr<_Tp>::reset(_Yp* __p, _Dp __d, _Alloc __a)
 {
     shared_ptr(__p, __d, __a).swap(*this);
 }
 
-template<class _Tp, class _Alloc, class ..._Args>
-inline _LIBCPP_INLINE_VISIBILITY
-typename enable_if
-<
-    !is_array<_Tp>::value,
-    shared_ptr<_Tp>
->::type
-allocate_shared(const _Alloc& __a, _Args&& ...__args)
+//
+// std::allocate_shared and std::make_shared
+//
+template<class _Tp, class _Alloc, class ..._Args, class = _EnableIf<!is_array<_Tp>::value> >
+_LIBCPP_HIDE_FROM_ABI
+shared_ptr<_Tp> allocate_shared(const _Alloc& __a, _Args&& ...__args)
 {
     static_assert(is_constructible<_Tp, _Args...>::value,
         "allocate_shared/make_shared: the type is not constructible from the provided arguments");
-
-    typedef __shared_ptr_emplace<_Tp, _Alloc> _CntrlBlk;
-    typedef typename __allocator_traits_rebind<_Alloc, _CntrlBlk>::type _A2;
-    typedef __allocator_destructor<_A2> _D2;
-
-    _A2 __a2(__a);
-    unique_ptr<_CntrlBlk, _D2> __hold2(__a2.allocate(1), _D2(__a2, 1));
-    ::new(static_cast<void*>(_VSTD::addressof(*__hold2.get())))
-        _CntrlBlk(__a, _VSTD::forward<_Args>(__args)...);
-
-    typename shared_ptr<_Tp>::element_type *__p = __hold2->__get_elem();
-    return shared_ptr<_Tp>::__create_with_control_block(__p, _VSTD::addressof(*__hold2.release()));
+    using _ControlBlock = __shared_ptr_emplace<_Tp, _Alloc>;
+    using _ControlBlockAllocator = typename __allocator_traits_rebind<_Alloc, _ControlBlock>::type;
+    __allocation_guard<_ControlBlockAllocator> __guard(__a, 1);
+    ::new ((void*)_VSTD::addressof(*__guard.__get())) _ControlBlock(__a, _VSTD::forward<_Args>(__args)...);
+    auto __control_block = __guard.__release_ptr();
+    return shared_ptr<_Tp>::__create_with_control_block((*__control_block).__get_elem(), _VSTD::addressof(*__control_block));
 }
 
 template<class _Tp, class ..._Args, class = _EnableIf<!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)...);
 }