[libcxx] Remove shared_ptr::make_shared

[zoecarver]

Summary

This patch removes shared_ptr::make_shared as it is not part of the standard. This patch also adds __create_with_cntrl_block, which is a helper function that can be used in std::allocate_shared and std::make_shared. This is the third patch (out of 4) from D66178.

[ldionne]

Can you confirm this is the bit of the patch that we suspect had caused a SEGFAULT for @phosek?

Also, is there a reason why we can't just rename shared_ptr<_Tp>::make_shared to shared_ptr<_Tp>::__make_shared, without changing the logic and the signature? Do you have other changes pending that depend on that?

Note: Requesting changes so it shows up properly in my review queue.

[zoecarver]

@ldionne, yes. I suspect this is the patch that will cause a SEGFAULT. It could also be a similar part of the patch where I remove std::shared_ptr::allocate_shared, though. If it does cause a SEGFAULT we will know exactly where the issue is and it will be easier to debug.

We could mangle the function. But, part of the reason for this change is removing as much code from make_shared as possible so, later, when I add more overloads, there will be less duplication. Additionally, I don't think it makes sense to add an extra layer of abstraction when it gives us no benefit. If we don't want __create_with_cntrl_block I would still like to remove std::shared_ptr::make_shared (and std::shared_ptr::__make_shared).

D62641 depends on this functionality but, it shouldn't be too hard to update that patch (and I wouldn't mind doing it if you want me to remove __create_with_cntrl_block).

[ldionne]

libcxx/include/memory

3876

Can we call this __create_with_control_block instead? We're not saving many characters.

4421

This just jumped in my face -- it's obvious now.

We're calling a function f(p.get()->foobar(), p.release()) where p is a unique_ptr. If the second argument (p.release()) is evaluated before the first one, p.get() will be nullptr and we'll dereference that, causing the segfault. @phosek must have been testing on a target where the compiler evaluates arguments from right to left.

I think we pinned down the order of evaluation in C++20 (I'm not sure whether it actually got in), but it's definitely a problem for earlier standards anyway.

[zoecarver]

libcxx/include/memory

3876

Will do.

4421

That would explain it! I'll change this to make __hold2.get()->get() a variable.

I am surprised that compilers are allowed to mess with order of evaluation. Glad it's fixed in C++20, though.

[zoecarver]

• rename __create_with_cntrl_block to __create_with_control_block.
• pull __ptr argument into a variable so that the arguments can be evaluated from right to left without a segfault.

[ldionne]

libcxx/include/memory

4421

I am surprised that compilers are allowed to mess with order of evaluation.

So, actually, I just got confirmation from @Bigcheese that there was a proposal to make evaluation of arguments be from left to right, but it didn't go through. Instead, the order moved from unsequenced to indeterminately sequenced in C++17.

This means that evaluation of arguments can't be interleaved, but you still can't rely on any given order.

[zoecarver]

I'll commit this in the morning.

libcxx/include/memory

4421

Good to know; thanks for sharing :)

[zoecarver]

Resolved by rL375504.

libcxx/include/memory

#ifndef _LIBCPP_NO_RTTI
     template <class _Dp>
         _LIBCPP_INLINE_VISIBILITY
         _Dp* __get_deleter() const _NOEXCEPT
             {return static_cast<_Dp*>(__cntrl_
                     ? const_cast<void *>(__cntrl_->__get_deleter(typeid(_Dp)))
                       : nullptr);}
 #endif  // _LIBCPP_NO_RTTI
 
-    template<class ..._Args>
-        static
-        shared_ptr<_Tp>
-        make_shared(_Args&& ...__args);
+    template<class _Yp, class _CntrlBlk>
+    static shared_ptr<_Tp>
+    __create_with_cntrl_block(_Yp* __p, _CntrlBlk* __cntrl)
+    {
+        shared_ptr<_Tp> __r;
+        __r.__ptr_ = __p;
+        __r.__cntrl_ = __cntrl;
+        __r.__enable_weak_this(__r.__ptr_, __r.__ptr_);
+        return __r;
+    }
 
     template<class _Alloc, class ..._Args>
         static
         shared_ptr<_Tp>
         allocate_shared(const _Alloc& __a, _Args&& ...__args);
 
 private:
     template <class _Yp, bool = is_function<_Yp>::value>
#endif
                                      _AllocT > _CntrlBlk;
         __cntrl_ = new _CntrlBlk(__r.get(), ref(__r.get_deleter()), _AllocT());
         __enable_weak_this(__r.get(), __r.get());
     }
     __r.release();
 }
 
 template<class _Tp>
-template<class ..._Args>
-shared_ptr<_Tp>
-shared_ptr<_Tp>::make_shared(_Args&& ...__args)
-{
-    static_assert( is_constructible<_Tp, _Args...>::value, "Can't construct object in make_shared" );
-    typedef __shared_ptr_emplace<_Tp, allocator<_Tp> > _CntrlBlk;
-    typedef allocator<_CntrlBlk> _A2;
-    typedef __allocator_destructor<_A2> _D2;
-    _A2 __a2;
-    unique_ptr<_CntrlBlk, _D2> __hold2(__a2.allocate(1), _D2(__a2, 1));
-    ::new(__hold2.get()) _CntrlBlk(__a2, _VSTD::forward<_Args>(__args)...);
-    shared_ptr<_Tp> __r;
-    __r.__ptr_ = __hold2.get()->get();
-    __r.__cntrl_ = __hold2.release();
-    __r.__enable_weak_this(__r.__ptr_, __r.__ptr_);
-    return __r;
-}
-
-template<class _Tp>
 template<class _Alloc, class ..._Args>
 shared_ptr<_Tp>
 shared_ptr<_Tp>::allocate_shared(const _Alloc& __a, _Args&& ...__args)
 {
     static_assert( is_constructible<_Tp, _Args...>::value, "Can't construct object in allocate_shared" );
     typedef __shared_ptr_emplace<_Tp, _Alloc> _CntrlBlk;
     typedef typename __allocator_traits_rebind<_Alloc, _CntrlBlk>::type _A2;
     typedef __allocator_destructor<_A2> _D2;

 inline _LIBCPP_INLINE_VISIBILITY
 typename enable_if
 <
     !is_array<_Tp>::value,
     shared_ptr<_Tp>
 >::type
 make_shared(_Args&& ...__args)
 {
-    return shared_ptr<_Tp>::make_shared(_VSTD::forward<_Args>(__args)...);
+    static_assert(is_constructible<_Tp, _Args...>::value, "Can't construct object in make_shared");
+    typedef __shared_ptr_emplace<_Tp, allocator<_Tp> > _CntrlBlk;
+    typedef allocator<_CntrlBlk> _A2;
+    typedef __allocator_destructor<_A2> _D2;
+
+    _A2 __a2;
+    unique_ptr<_CntrlBlk, _D2> __hold2(__a2.allocate(1), _D2(__a2, 1));
+    ::new(__hold2.get()) _CntrlBlk(__a2, _VSTD::forward<_Args>(__args)...);
+
+    return shared_ptr<_Tp>::__create_with_cntrl_block(__hold2.get()->get(),
+                                                      __hold2.release());
 }
 
 template<class _Tp, class _Alloc, class ..._Args>
 inline _LIBCPP_INLINE_VISIBILITY
 typename enable_if
 <
     !is_array<_Tp>::value,
     shared_ptr<_Tp>