Page MenuHomePhabricator

[libcxx] Support for shared_ptr<T()>
ClosedPublic

Authored by erik.pilkington on Mar 10 2017, 10:27 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

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.

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

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 requested changes to this revision.Mar 10 2017, 3:07 PM

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.

This revision now requires changes to proceed.Mar 10 2017, 3:07 PM
erik.pilkington edited edge metadata.

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

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?

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 added inline comments.Mar 27 2017, 2:34 PM
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 added inline comments.Mar 27 2017, 5:06 PM
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 marked 2 inline comments as done.

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

  • 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.

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.

Rebase n' ping!

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

EricWF accepted this revision.May 25 2017, 12:18 AM

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.

This revision is now accepted and ready to land.May 25 2017, 12:18 AM
This revision was automatically updated to reflect the committed changes.