[libcxx] Support for shared_ptr<T()>

[erik.pilkington]

Summary

This patch adds support for shared_ptr<T()> types, so that the following works:

void Func();
void Del(void (*)())
std::shared_ptr<void()> x(Func, Del);

Where previously this would fail to compile. In PR27566, the use case described for this was a shared pointer to a dynamically loaded function, which seems reasonable enough to me. Both libstdc++ and MSVC support this use case as well, and I didn't notice any wording in the standard that disallowed it. This patch has 2 parts:

1. Don't try to instantiate an allocator<T()>
2. Fix __enable_weak_this() dummy overload to work with function pointers.

Number 1 is accomplished by passing in allocator<void> into __shared_ptr_pointer or __shared_ptr_emplace when no allocator was passed to the constructor of the shared_ptr, I believe this is correct because in this case the allocator is only used for a rebind, so any instantiation of std::allocator would work fine.

This is my first libc++ patch, so take it with a grain of salt! Thanks for taking a look,
Erik

[halyavin]

But std::allocator<void> is deprecated in C++17. I don't know a good solution, I just used int as an arbitrary type when I faced similar problem.

[erik.pilkington]

This new patch replaces the allocator from allocator<void> to allocator<int>, I didn't realize allocator<void> was deprecated.
Thanks,
Erik

[• Quuxplusone]

I notice that the Standard implies that std::unique_ptr<T> will work only with *object types* T, and yet the "object" wording is missing from shared_ptr.

A unique pointer is an object that owns another *object* and manages that other *object* through a pointer.

The shared_ptr class template stores a *pointer*, usually obtained via new. shared_ptr implements semantics of shared ownership; the last remaining owner of the pointer is responsible for destroying the object, *or otherwise releasing the resources associated with the stored pointer.*

So it does seem like shared_ptr<T()> should work and should store a T(*)().
And it does seem that unique_ptr<T(), custom_deleter> should continue to not-work... but maybe there's a useful vendor extension or standards proposal to be made in that area.

include/memory

4209 ↗

(On Diff #91385)

IIUC, you don't need to touch make_shared. I doubt these changes are truly harmful, but I don't understand what meaning we're trying to assign to things like

auto a = std::make_shared<void()>();
auto b = std::make_shared<void()>(myFunction);

It seems like the effect of this change is just to mess with anyone who's explicitly specializing std::allocator<MyObjectType> to make its ::rebind member do something wacky. In which case I'd say they've got it coming to them... but the effect of this change doesn't seem relevant to the purpose described in your commit message. :)

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

49 ↗

(On Diff #91385)

Could you plausibly make this

void resultDeleter(Result (*f)()) { assert(f == theFunction); }

for extra sanity checking?
For super-duper checking, touch a global variable in resultDeleter so that you can verify that the deleter was actually called. This might reasonably be perceived as overkill. :)

72 ↗

(On Diff #91385)

Presumably this should also compile without the explicit &s, right? Might be worth adding

std::shared_ptr<Result()> y(theFunction, resultDeleter);

in addition to your existing test case, just to make sure that omitting the & doesn't mess up the template deduction somehow.
IIUC, it would never make sense to write something like

auto z = std::make_shared<Result()>(theFunction);

so that part doesn't need testing, right?

[EricWF]

We can't just use an arbitrary allocator type for a number of reasons:

• You just changed the type of the control block. That's ABI breaking.
• allocator<int> allocates ints, nothing else.
• It may mean we don't select a valid user specialization of allocator<Yp>.

I'll play around to see if I can come up with another path forward to fix this.

[erik.pilkington]

In this new patch:

• We only select allocator<int> when _Yp is a function type, fixing the ABI break @EricWF pointed out.
• Only try to select a different allocator if we're using the __shared_ptr_pointer layout; make_shared<void()> doesn't make sense.
• Improve the testcase

Thanks for the feedback!
Erik

[erik.pilkington]

In D30837#698305, @EricWF wrote:

We can't just use an arbitrary allocator type for a number of reasons:

• You just changed the type of the control block. That's ABI breaking.

Ah, I didn't think of that. This new patch only selects allocator<int> when _Yp is a function type, which is only permitted as of this patch.

• allocator<int> allocates ints, nothing else.

Since we only use allocator<int> for a rebind, we don't violate this rule.

• It may mean we don't select a valid user specialization of allocator<Yp>.

In the new patch, allocator<int> is only selected if is_function<_Yp>, I don't believe that there is any valid user specialization of std::allocator<FunctionType>. This is because [namespace.std] p1:

A program may add a template specialization for any standard library template to namespace std only if [...] the specialization meets the standard library requirements for the original template [...]

In this case, the original template is the default allocator, which is supposed to have overloads of address taking a const_reference and a reference ([allocator.members] p2-3), which will be ambiguous for function types with the given definitions of const_reference and reference. Is this a valid argument?

[erik.pilkington]

Ping!

[EricWF]

This patch seems to support constructing a shared_ptr<FuncType> without providing a non-default deleter. I don't think this should work because the default deleter will attempt to free a function pointer, which is never valid. (Although I think this case will still cause a compile error).

include/memory

3896 ↗

(On Diff #91660)

I would just handle this case with the primary template rather than a complete specialization.

3903 ↗

(On Diff #91660)

Using an arbitrary and unrelated allocator type std::allocator<int> still makes me nervous. I would rather use a custom allocator type written only for this use case.

[• Quuxplusone]

include/memory

3933 ↗

(On Diff #91660)

EricWF wrote:

This patch seems to support constructing a shared_ptr<FuncType> without providing a non-default deleter. I don't think this should work because the default deleter will attempt to free a function pointer, which is never valid. (Although I think this case will still cause a compile error).

Good point. But then are you suggesting that this constructor should be SFINAEd in that case, or just static_assert, or leave the existing behavior (which is indeed a hard compile error *inside* a static-assert)?

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:2514:27: error: invalid application of
      'sizeof' to a function type
            static_assert(sizeof(_Tp) > 0, "default_delete can not delete incomplete type");
                          ^~~~~~~~~~~
x.cc:6:2: note: in instantiation of member function 'std::__1::default_delete<int ()>::operator()' requested here

(Technically-technically, arguably the user is allowed to fully specialize std::default_delete<T()> for some user-defined type T, right? Not that libc++ ought to be catering to such people.)

[EricWF]

include/memory

3933 ↗

(On Diff #91660)

SFINAE is definitely the wrong approach, since we don't want to affect overload resolution.

On second thought it's probably better to fix the allocator type here anyway, since the instantiation of default_delete<T> will lead to a more readable compile error.

Also you're right about user defined specializations of default_delete, you're also right that libc++ ought not care about such craziness.

[erik.pilkington]

In this new patch, use an explicit specialization of std::allocator that specifically only performs a rebind. This needs to be a specialization of std::allocator so we can use allocator's allocator(const allocator<U> &) ctor from shared_ptr_pointer::__on_zero_shared_weak().
Thanks,
Erik

[erik.pilkington]

Ping!

[EricWF]

• Please apply this patch on top of yours. It adds tests for the diagnostics produced when the default deleter is used.

LGTM other than the inline comments. I'll want to run this by @mclow.lists before giving it gets committed though.

include/memory

3606 ↗

(On Diff #93488)

I would prefer using an entirely different allocator type, not a specialization of std::allocator.

Re-naming this class to __shared_ptr_dummy_rebind_allocator and moving it closer to __shared_ptr_default_allocator would be good.

[erik.pilkington]

This new patch includes @EricWF's static_assert & test.
Thanks,
Erik

include/memory

3606 ↗

(On Diff #93488)

Unfortunately, for this approach to work we have to specialize std::allocator because we use the template <class _Up> allocator(const allocator<_Up> &) constructor at __shared_ptr_pointer::__on_zero_shared_weak.

If specializing std::allocator is a problem, I suppose we could pass in a special entirely empty struct as an "allocator" to __shared_ptr_pointer when necessary, then metaprogram our way out of using the converting constructor at __shared_ptr_ptr::__on_zero_shared_weak. I think the current approach is simpler though.

[erik.pilkington]

Rebase n' ping!

[erik.pilkington]

Ping! @mclow.lists: Do you have any thoughts here?

[erik.pilkington]

Ping!

[EricWF]

LGTM after addressing inline comments. Sorry for the delay.

include/memory

3663 ↗

(On Diff #97313)

This rebind isn't getting selected because it's private, but std::allocator_traits is doing the rebinding instead.

Please make the rebinding public.

libcxx/trunk/include/memory

_NOEXCEPT_(__is_nothrow_swappable<_T1>::value &&
                __is_nothrow_swappable<_T2>::value) {
   __x.swap(__y);
 }
 
 // default_delete
 
 template <class _Tp>
 struct _LIBCPP_TEMPLATE_VIS default_delete {
+    static_assert(!is_function<_Tp>::value,
+                  "default_delete cannot be instantiated for function types");
 #ifndef _LIBCPP_CXX03_LANG
   _LIBCPP_INLINE_VISIBILITY constexpr default_delete() noexcept = default;
 #else
   _LIBCPP_INLINE_VISIBILITY default_delete() {}
 #endif
   template <class _Up>
   _LIBCPP_INLINE_VISIBILITY
   default_delete(const default_delete<_Up>&,
__shared_ptr_emplace<_Tp, _Alloc>::__on_zero_shared_weak() _NOEXCEPT
     typedef typename __allocator_traits_rebind<_Alloc, __shared_ptr_emplace>::type _Al;
     typedef allocator_traits<_Al> _ATraits;
     typedef pointer_traits<typename _ATraits::pointer> _PTraits;
     _Al __a(__data_.first());
     __data_.first().~_Alloc();
     __a.deallocate(_PTraits::pointer_to(*this), 1);
 }
 
+struct __shared_ptr_dummy_rebind_allocator_type;
+template <>
+class _LIBCPP_TEMPLATE_VIS allocator<__shared_ptr_dummy_rebind_allocator_type>
+{
+public:
+    template <class _Other>
+    struct rebind
+    {
+        typedef allocator<_Other> other;
+    };
+};
+
 template<class _Tp> class _LIBCPP_TEMPLATE_VIS enable_shared_from_this;
 
 template<class _Tp>
 class _LIBCPP_TEMPLATE_VIS shared_ptr
 {
 public:
     typedef _Tp element_type;
 
#else  // _LIBCPP_HAS_NO_VARIADICS
 
     template<class _Alloc, class _A0, class _A1, class _A2>
         static shared_ptr<_Tp>
         allocate_shared(const _Alloc& __a, _A0& __a0, _A1& __a1, _A2& __a2);
 
 #endif  // _LIBCPP_HAS_NO_VARIADICS
 
 private:
+    template <class _Yp, bool = is_function<_Yp>::value>
+        struct __shared_ptr_default_allocator
+        {
+            typedef allocator<_Yp> type;
+        };
+
+    template <class _Yp>
+        struct __shared_ptr_default_allocator<_Yp, true>
+        {
+            typedef allocator<__shared_ptr_dummy_rebind_allocator_type> type;
+        };
 
     template <class _Yp, class _OrigPtr>
         _LIBCPP_INLINE_VISIBILITY
         typename enable_if<is_convertible<_OrigPtr*,
                                           const enable_shared_from_this<_Yp>*
         >::value,
             void>::type
         __enable_weak_this(const enable_shared_from_this<_Yp>* __e,
                            _OrigPtr* __ptr) _NOEXCEPT
         {
             typedef typename remove_cv<_Yp>::type _RawYp;
             if (__e && __e->__weak_this_.expired())
             {
                 __e->__weak_this_ = shared_ptr<_RawYp>(*this,
                     const_cast<_RawYp*>(static_cast<const _Yp*>(__ptr)));
             }
         }
 
-    _LIBCPP_INLINE_VISIBILITY
-    void __enable_weak_this(const volatile void*, const volatile void*) _NOEXCEPT {}
+    _LIBCPP_INLINE_VISIBILITY void __enable_weak_this(...) _NOEXCEPT {}
 
     template <class _Up> friend class _LIBCPP_TEMPLATE_VIS shared_ptr;
     template <class _Up> friend class _LIBCPP_TEMPLATE_VIS weak_ptr;
 };
 
 
 template<class _Tp>
 inline

 
 template<class _Tp>
 template<class _Yp>
 shared_ptr<_Tp>::shared_ptr(_Yp* __p,
                             typename enable_if<is_convertible<_Yp*, element_type*>::value, __nat>::type)
     : __ptr_(__p)
 {
     unique_ptr<_Yp> __hold(__p);
-    typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, allocator<_Yp> > _CntrlBlk;
-    __cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), allocator<_Yp>());
+    typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
+    typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, _AllocT > _CntrlBlk;
+    __cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), _AllocT());
     __hold.release();
     __enable_weak_this(__p, __p);
 }
 
 template<class _Tp>
 template<class _Yp, class _Dp>
 shared_ptr<_Tp>::shared_ptr(_Yp* __p, _Dp __d,
                             typename enable_if<is_convertible<_Yp*, element_type*>::value, __nat>::type)
     : __ptr_(__p)
 {
 #ifndef _LIBCPP_NO_EXCEPTIONS
     try
     {
 #endif  // _LIBCPP_NO_EXCEPTIONS
-        typedef __shared_ptr_pointer<_Yp*, _Dp, allocator<_Yp> > _CntrlBlk;
-        __cntrl_ = new _CntrlBlk(__p, __d, allocator<_Yp>());
+        typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
+        typedef __shared_ptr_pointer<_Yp*, _Dp, _AllocT > _CntrlBlk;
+        __cntrl_ = new _CntrlBlk(__p, __d, _AllocT());
         __enable_weak_this(__p, __p);
 #ifndef _LIBCPP_NO_EXCEPTIONS
     }
     catch (...)
     {
         __d(__p);
         throw;
     }
 #endif  // _LIBCPP_NO_EXCEPTIONS
 }
 
 template<class _Tp>
 template<class _Dp>
 shared_ptr<_Tp>::shared_ptr(nullptr_t __p, _Dp __d)
     : __ptr_(0)
 {
 #ifndef _LIBCPP_NO_EXCEPTIONS
     try
     {
 #endif  // _LIBCPP_NO_EXCEPTIONS
-        typedef __shared_ptr_pointer<nullptr_t, _Dp, allocator<_Tp> > _CntrlBlk;
-        __cntrl_ = new _CntrlBlk(__p, __d, allocator<_Tp>());
+        typedef typename __shared_ptr_default_allocator<_Tp>::type _AllocT;
+        typedef __shared_ptr_pointer<nullptr_t, _Dp, _AllocT > _CntrlBlk;
+        __cntrl_ = new _CntrlBlk(__p, __d, _AllocT());
 #ifndef _LIBCPP_NO_EXCEPTIONS
     }
     catch (...)
     {
         __d(__p);
         throw;
     }
 #endif  // _LIBCPP_NO_EXCEPTIONS
#endif
     : __ptr_(__r.get())
 {
 #if _LIBCPP_STD_VER > 11
     if (__ptr_ == nullptr)
         __cntrl_ = nullptr;
     else
 #endif
     {
-        typedef __shared_ptr_pointer<_Yp*, _Dp, allocator<_Yp> > _CntrlBlk;
-        __cntrl_ = new _CntrlBlk(__r.get(), __r.get_deleter(), allocator<_Yp>());
+        typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
+        typedef __shared_ptr_pointer<_Yp*, _Dp, _AllocT > _CntrlBlk;
+        __cntrl_ = new _CntrlBlk(__r.get(), __r.get_deleter(), _AllocT());
         __enable_weak_this(__r.get(), __r.get());
     }
     __r.release();
 }
 
 template<class _Tp>
 template <class _Yp, class _Dp>
 #ifndef _LIBCPP_HAS_NO_RVALUE_REFERENCES
#endif
     : __ptr_(__r.get())
 {
 #if _LIBCPP_STD_VER > 11
     if (__ptr_ == nullptr)
         __cntrl_ = nullptr;
     else
 #endif
     {
+        typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
         typedef __shared_ptr_pointer<_Yp*,
                                      reference_wrapper<typename remove_reference<_Dp>::type>,
-                                     allocator<_Yp> > _CntrlBlk;
-        __cntrl_ = new _CntrlBlk(__r.get(), ref(__r.get_deleter()), allocator<_Yp>());
+                                     _AllocT > _CntrlBlk;
+        __cntrl_ = new _CntrlBlk(__r.get(), ref(__r.get_deleter()), _AllocT());
         __enable_weak_this(__r.get(), __r.get());
     }
     __r.release();
 }
 
 #ifndef _LIBCPP_HAS_NO_VARIADICS
 
 template<class _Tp>

libcxx/trunk/test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp

+#include <memory>
+
+template <int> struct Tag {};
+
+template <int ID>
+using SPtr = std::shared_ptr<void(Tag<ID>)>;
+
+template <int ID>
+using FnType = void(Tag<ID>);
+
+template <int ID>
+void TestFn(Tag<ID>) {}
+
+template <int ID>
+FnType<ID>* getFn() {
+  return &TestFn<ID>;
+}
+
+struct Deleter {
+  template <class Tp>
+  void operator()(Tp) const {
+    using RawT = typename std::remove_pointer<Tp>::type;
+    static_assert(std::is_function<RawT>::value ||
+                  std::is_null_pointer<RawT>::value, "");
+  }
+};
+
+int main() {
+  {
+    SPtr<0> s; // OK
+    SPtr<1> s1(nullptr); // OK
+    SPtr<2> s2(getFn<2>(), Deleter{}); // OK
+    SPtr<3> s3(nullptr, Deleter{}); // OK
+  }
+  // expected-error@memory:* 2 {{static_assert failed "default_delete cannot be instantiated for function types"}}
+  {
+    SPtr<4> s4(getFn<4>()); // expected-note {{requested here}}
+    SPtr<5> s5(getFn<5>(), std::default_delete<FnType<5>>{}); // expected-note {{requested here}}
+  }
+}

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

 
 
 struct Foo
 {
     Foo() = default;
     virtual ~Foo() = default;
 };
 
+struct Result {};
+static Result theFunction() { return Result(); }
+static int resultDeletorCount;
+static void resultDeletor(Result (*pf)()) {
+  assert(pf == theFunction);
+  ++resultDeletorCount;
+}
 
 int main()
 {
     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));
     assert(A::count == 1);
     assert(p->get_int() == 67);
     assert(p->get_char() == 'e');
     }
 
     { // https://bugs.llvm.org/show_bug.cgi?id=24137
     std::shared_ptr<Foo> p1       = std::make_shared<Foo>();
     assert(p1.get());
     std::shared_ptr<const Foo> p2 = std::make_shared<const Foo>();
     assert(p2.get());
     }
-
+    { // https://bugs.llvm.org/show_bug.cgi?id=27566
+      std::shared_ptr<Result()> x(&theFunction, &resultDeletor);
+      std::shared_ptr<Result()> y(theFunction, resultDeletor);
+    }
+    assert(resultDeletorCount == 2);
 #if TEST_STD_VER >= 11
     nc = globalMemCounter.outstanding_new;
     {
     char c = 'e';
     std::shared_ptr<A> p = std::make_shared<A>(67, c);
     assert(globalMemCounter.checkOutstandingNewEq(nc+1));
     assert(A::count == 1);
     assert(p->get_int() == 67);
     assert(p->get_char() == 'e');
     }
 #endif
     assert(A::count == 0);
 }