Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<xmemory>: allocate_at_least optimization broken by private inheritance from std::allocator #3890

Closed
StephanTLavavej opened this issue Jul 19, 2023 · 1 comment · Fixed by #3891
Labels
bug Something isn't working high priority Important!

Comments

@StephanTLavavej
Copy link
Member

After #3864, VSO-1852860 / AB#1852860 "[RWC][prod/fe][std:c++latest][Regression] Catch2 failed with error C2247 and error C2248" was reported, due to a custom allocator privately inheriting from std::allocator (see https://github.com/catchorg/Catch2/blob/4acc51828f7f93f3b2058a63f54d112af4034503/tests/SelfTest/UsageTests/Matchers.tests.cpp#L229-L254 ):

D:\GitHub\STL\out\x64>type meow.cpp
#include <cassert>
#include <memory>
#include <vector>
using namespace std;

template <typename T>
struct CustomAllocator : private allocator<T> {
    using value_type = T;

    CustomAllocator() = default;

    template <typename U>
    CustomAllocator(const CustomAllocator<U>&) {}

    using allocator<T>::allocate;
    using allocator<T>::deallocate;

    template <typename U>
    bool operator==(const CustomAllocator<U>&) const {
        return true;
    }
};

int main() {
    vector<int, CustomAllocator<int>> v;
    v.push_back(1729);
    assert(v.back() == 1729);
}
D:\GitHub\STL\out\x64>cl /EHsc /nologo /W4 /std:c++20 /MTd /Od meow.cpp && meow
meow.cpp

D:\GitHub\STL\out\x64>cl /EHsc /nologo /W4 /std:c++latest /MTd /Od meow.cpp && meow
meow.cpp
D:\GitHub\STL\out\x64\out\inc\xmemory(2197): error C2247: 'std::allocator<int>::allocate_at_least' not accessible because 'CustomAllocator<int>' uses 'private' to inherit from 'std::allocator<int>'
D:\GitHub\STL\out\x64\out\inc\xmemory(978): note: see declaration of 'std::allocator<int>::allocate_at_least'
meow.cpp(25): note: see declaration of 'CustomAllocator<int>'
D:\GitHub\STL\out\x64\out\inc\xmemory(1049): note: see declaration of 'std::allocator<int>'
D:\GitHub\STL\out\x64\out\inc\vector(831): note: see reference to function template instantiation 'int *std::_Allocate_at_least_helper<CustomAllocator<int>>(_Alloc &,unsigned __int64 &)' being compiled
        with
        [
            _Alloc=CustomAllocator<int>
        ]
D:\GitHub\STL\out\x64\out\inc\vector(785): note: see reference to function template instantiation 'int *std::vector<int,CustomAllocator<int>>::_Emplace_reallocate<_Ty>(int *const ,_Ty &&)' being compiled
        with
        [
            _Ty=int
        ]
D:\GitHub\STL\out\x64\out\inc\vector(878): note: see reference to function template instantiation '_Ty &std::vector<_Ty,CustomAllocator<int>>::_Emplace_one_at_back<int>(int &&)' being compiled
        with
        [
            _Ty=int
        ]
D:\GitHub\STL\out\x64\out\inc\vector(876): note: while compiling class template member function 'void std::vector<int,CustomAllocator<int>>::push_back(_Ty &&)'
        with
        [
            _Ty=int
        ]
meow.cpp(26): note: see the first reference to 'std::vector<int,CustomAllocator<int>>::push_back' in 'main'
meow.cpp(25): note: see reference to class template instantiation 'std::vector<int,CustomAllocator<int>>' being compiled
D:\GitHub\STL\out\x64\out\inc\xmemory(2197): error C2248: 'std::allocator<int>::allocate_at_least': cannot access private member declared in class 'CustomAllocator<int>'
D:\GitHub\STL\out\x64\out\inc\xmemory(978): note: see declaration of 'std::allocator<int>::allocate_at_least'
meow.cpp(25): note: see declaration of 'CustomAllocator<int>'

D:\GitHub\STL\out\x64>clang-cl /EHsc /nologo /W4 /std:c++20 /MTd /Od meow.cpp && meow

D:\GitHub\STL\out\x64>clang-cl /EHsc /nologo /W4 /std:c++latest /MTd /Od meow.cpp && meow

D:\GitHub\STL\out\x64>

This appears to involve an MSVC compiler bug in SFINAE, because Clang accepts this code. For MSVC, it appears that classic SFINAE is properly detecting that typename _Ty::_From_primary is inaccessible, but then Expression SFINAE is incorrectly detecting that allocate_at_least is accessible:

STL/stl/inc/xmemory

Lines 473 to 480 in f51733c

#if _HAS_CXX23
template <class _Alloc, class _SizeTy, class = void>
inline constexpr bool _Has_member_allocate_at_least = false;
template <class _Alloc, class _SizeTy>
inline constexpr bool _Has_member_allocate_at_least<_Alloc, _SizeTy,
void_t<decltype(_STD declval<_Alloc&>().allocate_at_least(_STD declval<const _SizeTy&>()))>> = true;
#endif // _HAS_CXX23

STL/stl/inc/xmemory

Lines 2177 to 2190 in f51733c

#if _HAS_CXX23
template <class _Ty, class = void>
inline constexpr bool _Has_member_from_primary = false;
template <class _Ty>
inline constexpr bool _Has_member_from_primary<_Ty, void_t<typename _Ty::_From_primary>> = true;
// Avoid using allocate_at_least when the allocator publicly derives from std::allocator:
// "old" allocators might hide allocate and deallocate but fail to hide allocate_at_least.
// Also avoid using allocate_at_least from std::allocator itself because it currently doesn't do anything useful.
template <class _Alloc>
inline constexpr bool _Should_allocate_at_least =
!_Has_member_from_primary<_Alloc>
&& _Has_member_allocate_at_least<_Alloc, typename allocator_traits<_Alloc>::size_type>;
#endif // _HAS_CXX23

We should report this compiler bug, and then we may need to work around it with is_base_of to bluntly forbid all std::allocator inheritance (is_base_of sees through private and ambiguous bases).

@StephanTLavavej StephanTLavavej added bug Something isn't working high priority Important! labels Jul 19, 2023
@CaseyCarter CaseyCarter linked a pull request Jul 19, 2023 that will close this issue
@CaseyCarter
Copy link
Member

I've filed the compiler bug as DevCom-10419218 "Expression SFINAE sometimes ignores access with two-phase lookup".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working high priority Important!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants