Page MenuHomePhabricator

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

Authored by ldionne on Dec 11 2020, 9:51 AM.

Details

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.

Diff Detail

Event Timeline

ldionne created this revision.Dec 11 2020, 9:51 AM
ldionne requested review of this revision.Dec 11 2020, 9:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 11 2020, 9:51 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Dec 14 2020, 2:09 PM
This revision is now accepted and ready to land.Dec 14 2020, 2:09 PM

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 {

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.

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.

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 <ldionne.2@gmail.com>
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.