[Bug libstdc++/113841] New: Can't swap two std::hash<T*>

[Bug libstdc++/113841] New: Can't swap two std::hash<T*>

[ostash at ostash dot kiev.ua]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113841

            Bug ID: 113841
           Summary: Can't swap two std::hash<T*>
           Product: gcc
           Version: 12.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: ostash at ostash dot kiev.ua
  Target Milestone: ---

For the following code snippet:
---
#include <array>
#include <vector>

class Storage;

template <typename T>
class MyAllocator 
{
public:
  using value_type = T;
  using pointer = T*;

  MyAllocator(Storage* s);

  template <typename U>
  MyAllocator(MyAllocator<U> const& other) noexcept;

  T* allocate( std::size_t n );
  void deallocate(T* p, std::size_t n );

private:
  Storage* s_;
};

class Foo{
public:
    Foo(int, int);
};

void x() {
  using std::swap;

  using MyVec = std::vector<int, MyAllocator<int>>;
  using MyArrVec = std::array<MyVec, 1>;
  using MyHash = std::hash<std::pair<const int, MyArrVec>*>;

  MyHash h1, h2;
  swap(h1, h2);
}
---

GCC 12 reports:
In file included from
/opt/compiler-explorer/gcc-12.1.0/include/c++/12.1.0/vector:64,
                 from <source>:2:
/opt/compiler-explorer/gcc-12.1.0/include/c++/12.1.0/bits/stl_vector.h: In
instantiation of 'constexpr std::_Vector_base<_Tp,
_Alloc>::_Vector_impl::_Vector_impl() [with _Tp = int; _Alloc =
MyAllocator<int>]':
/opt/compiler-explorer/gcc-12.1.0/include/c++/12.1.0/bits/stl_vector.h:312:7:  
required by substitution of 'template<class _Tp> static std::true_type
std::__do_is_implicitly_default_constructible_impl::__test(const _Tp&, decltype
(__helper<const _Tp&>(<brace-enclosed initializer list>()))*) [with _Tp =
std::array<std::vector<int, MyAllocator<int> >, 1>]'
/opt/compiler-explorer/gcc-12.1.0/include/c++/12.1.0/type_traits:1249:30:  
required from 'struct
std::__is_implicitly_default_constructible_impl<std::array<std::vector<int,
MyAllocator<int> >, 1> >'
/opt/compiler-explorer/gcc-12.1.0/include/c++/12.1.0/type_traits:1253:12:  
required from 'struct
std::__is_implicitly_default_constructible_safe<std::array<std::vector<int,
MyAllocator<int> >, 1> >'
/opt/compiler-explorer/gcc-12.1.0/include/c++/12.1.0/type_traits:167:12:  
required from 'struct
std::__and_<std::__is_constructible_impl<std::array<std::vector<int,
MyAllocator<int> >, 1> >,
std::__is_implicitly_default_constructible_safe<std::array<std::vector<int,
MyAllocator<int> >, 1> > >'
/opt/compiler-explorer/gcc-12.1.0/include/c++/12.1.0/type_traits:1258:12:  
required from 'struct
std::__is_implicitly_default_constructible<std::array<std::vector<int,
MyAllocator<int> >, 1> >'
/opt/compiler-explorer/gcc-12.1.0/include/c++/12.1.0/type_traits:167:12:  
required from 'struct
std::__and_<std::__is_implicitly_default_constructible<const int>,
std::__is_implicitly_default_constructible<std::array<std::vector<int,
MyAllocator<int> >, 1> > >'
/opt/compiler-explorer/gcc-12.1.0/include/c++/12.1.0/type_traits:178:41:  
required from 'struct
std::__not_<std::__and_<std::__is_implicitly_default_constructible<const int>,
std::__is_implicitly_default_constructible<std::array<std::vector<int,
MyAllocator<int> >, 1> > > >'
/opt/compiler-explorer/gcc-12.1.0/include/c++/12.1.0/bits/stl_pair.h:226:16:  
required from 'struct std::pair<const int, std::array<std::vector<int,
MyAllocator<int> >, 1> >'
<source>:38:14:   required from here
/opt/compiler-explorer/gcc-12.1.0/include/c++/12.1.0/type_traits:1240:58:   in
'constexpr' expansion of 'std::vector<int, MyAllocator<int> >()'
/opt/compiler-explorer/gcc-12.1.0/include/c++/12.1.0/bits/stl_vector.h:526:7:  
in 'constexpr' expansion of '((std::vector<int, MyAllocator<int>
>*)this)->std::vector<int, MyAllocator<int>
>::<anonymous>.std::_Vector_base<int, MyAllocator<int> >::_Vector_base()'
/opt/compiler-explorer/gcc-12.1.0/include/c++/12.1.0/bits/stl_vector.h:139:26:
error: no matching function for call to 'MyAllocator<int>::MyAllocator()'
  139 |         : _Tp_alloc_type()
      |                          ^
<source>:16:3: note: candidate: 'template<class U>
MyAllocator<T>::MyAllocator(const MyAllocator<U>&) [with T = int]'
   16 |   MyAllocator(MyAllocator<U> const& other) noexcept;
      |   ^~~~~~~~~~~
<source>:16:3: note:   template argument deduction/substitution failed:
/opt/compiler-explorer/gcc-12.1.0/include/c++/12.1.0/bits/stl_vector.h:139:26:
note:   candidate expects 1 argument, 0 provided
  139 |         : _Tp_alloc_type()
      |                          ^
<source>:13:3: note: candidate: 'MyAllocator<T>::MyAllocator(Storage*) [with T
= int]'
   13 |   MyAllocator(Storage* s);
      |   ^~~~~~~~~~~
<source>:13:3: note:   candidate expects 1 argument, 0 provided
<source>:7:7: note: candidate: 'constexpr MyAllocator<int>::MyAllocator(const
MyAllocator<int>&)'
    7 | class MyAllocator
      |       ^~~~~~~~~~~
<source>:7:7: note:   candidate expects 1 argument, 0 provided
<source>:7:7: note: candidate: 'constexpr
MyAllocator<int>::MyAllocator(MyAllocator<int>&&)'
<source>:7:7: note:   candidate expects 1 argument, 0 provided
Compiler returned: 1


Clang 15, 16, 17 also fail to compile with similar error about missing default
constructor for custom allocator when they are using libstdc++ >=12

GCC 13 is able to compile this, same as Clang with libc++.

[Bug libstdc++/113841] Can't swap two std::hash<T*>

[ostash at ostash dot kiev.ua]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113841

--- Comment #1 from Viktor Ostashevskyi <ostash at ostash dot kiev.ua> ---
Issue is visible with -std=c++20, works fine for -std=c++17 (for both GCC12 and
Clang).

[Bug libstdc++/113841] Can't swap two std::hash<T*>

[ostash at ostash dot kiev.ua]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113841

--- Comment #2 from Viktor Ostashevskyi <ostash at ostash dot kiev.ua> ---
Compiler exporer link: https://godbolt.org/z/cPqsKq6nM

[Bug libstdc++/113841] Can't swap two std::hash<T*>

[ostash at ostash dot kiev.ua]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113841

--- Comment #3 from Viktor Ostashevskyi <ostash at ostash dot kiev.ua> ---
Additional information: everything works fine both for GCC12 and Clang if swap
call is fully qualified, i.e.:

std::swap(h1, h2);

[Bug libstdc++/113841] Can't swap two std::hash<T*>

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113841

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |12.3.1
      Known to work|                            |13.1.0
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2024-02-09
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=109030

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
This is a compiler bug not a library bug. The preprocessed source from GCC 12
compiles fine with GCC 13.

GCC started to accept it with r13-6716

c++: maybe_constant_init and unevaluated operands [PR109030]

This testcase in this PR (already fixed by r13-6526-ge4692319fd5fc7)
demonstrates that maybe_constant_init can be called on an unevaluated
operand (e.g. from massage_init_elt) so this entry point should also
limit constant evaluation in that case, like maybe_constant_value does.

PR c++/109030

gcc/cp/ChangeLog:

* constexpr.cc (maybe_constant_init_1): For an unevaluated
non-manifestly-constant operand, don't constant evaluate
and instead call fold_to_constant as in maybe_constant_value.

[Bug libstdc++/113841] Can't swap two std::hash<T*>

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113841

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|c++                         |libstdc++

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Actually, maybe we need this change in the library, so that it works in GCC 12
and also works with Clang:

--- a/libstdc++-v3/include/bits/stl_vector.h
+++ b/libstdc++-v3/include/bits/stl_vector.h
@@ -135,6 +135,9 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
        _GLIBCXX20_CONSTEXPR
        _Vector_impl() _GLIBCXX_NOEXCEPT_IF(
            is_nothrow_default_constructible<_Tp_alloc_type>::value)
+#if __cpp_lib_concepts
+       requires is_default_constructible_v<_Tp_alloc_type>
+#endif
        : _Tp_alloc_type()
        { }

[Bug libstdc++/113841] Can't swap two std::hash<T*>

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113841

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
IIRC the reason we don't just default that constructor is because we need to be
sure to value-initialize the allocator, not just default-initialize it.

[Bug libstdc++/113841] Can't swap two std::hash<T*>

[ostash at ostash dot kiev.ua]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113841

--- Comment #7 from Viktor Ostashevskyi <ostash at ostash dot kiev.ua> ---
I'm still wondering why for std::hash<T*>, the T type is checked for anything.
It shouldn't matter what T is, as we're hashing T*...

[Bug libstdc++/113841] Can't swap two std::hash<T*>

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113841

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Calling swap unqualified performs ADL, which has to find all the associated
namespaces and associated classes. To do that it has to complete all the types
involved, which means it tries to complete:
std::hash<std::pair<const int, MyArrVec>*>
std::pair<const int, MyArrVec>
MyArrVec
MyVec
MyAllocator<int>

Trying to complete MyVec hits an error outside the immediate context, because
its default constructor cannot be instantiated, because MyAllocator<int> is not
default constructible.

That's why comment 5 fixes it.

[Bug libstdc++/113841] Can't swap two std::hash<T*>

[ostash at ostash dot kiev.ua]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113841

--- Comment #9 from Viktor Ostashevskyi <ostash at ostash dot kiev.ua> ---
(In reply to Jonathan Wakely from comment #8)
> Calling swap unqualified performs ADL, which has to find all the associated
> namespaces and associated classes. To do that it has to complete all the
> types involved, which means it tries to complete:
> std::hash<std::pair<const int, MyArrVec>*>
> std::pair<const int, MyArrVec>
> MyArrVec
> MyVec
> MyAllocator<int>
> 
> Trying to complete MyVec hits an error outside the immediate context,
> because its default constructor cannot be instantiated, because
> MyAllocator<int> is not default constructible.
> 
> That's why comment 5 fixes it.

Thanks a lot for such a deep explanation. We applied patch from comment #5 to
gcc-13 and now clang-17 can compile our application using libstdc++.

Any plans to apply the patch to gcc-12/gcc-13/gcc-14/trunk?

[Bug libstdc++/113841] Can't swap two std::hash<T*>

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113841

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org
   Target Milestone|---                         |12.4

[Bug libstdc++/113841] Can't swap two std::hash<T*>

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113841

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
This one's much harder to fix:

#include <string>

template<typename T>
struct Alloc
{
  using value_type = T;

  Alloc(int) { }

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

  T* allocate(std::size_t n) { return std::allocator<T>().allocate(n); }
  void deallocate(T* p, std::size_t n) { std::allocator<T>().deallocate(p, n);
}
};

template<typename T> struct wrap { T t; };

template<typename T> void do_adl(T&) { }

void test_pr113841()
{
  using Tr = std::char_traits<char>;
  using test_type = std::basic_string<char, Tr, Alloc<char>>;
  std::pair<const int, wrap<test_type>>* h = nullptr;
  do_adl(h);
}

[Bug libstdc++/113841] Can't swap two std::hash<T*>

[ostash at ostash dot kiev.ua]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113841

--- Comment #11 from Viktor Ostashevskyi <ostash at ostash dot kiev.ua> ---
(In reply to Jonathan Wakely from comment #10)
> This one's much harder to fix:
> 
> #include <string>
> 
> template<typename T>
> struct Alloc
> {
>   using value_type = T;
> 
>   Alloc(int) { }
> 
>   template<typename U> Alloc(const Alloc<U>&) { }
> 
>   T* allocate(std::size_t n) { return std::allocator<T>().allocate(n); }
>   void deallocate(T* p, std::size_t n) { std::allocator<T>().deallocate(p,
> n); }
> };
> 
> template<typename T> struct wrap { T t; };
> 
> template<typename T> void do_adl(T&) { }
> 
> void test_pr113841()
> {
>   using Tr = std::char_traits<char>;
>   using test_type = std::basic_string<char, Tr, Alloc<char>>;
>   std::pair<const int, wrap<test_type>>* h = nullptr;
>   do_adl(h);
> }

Incremental approach? Fixing std::vector first, thinking about std::pair next..
:)

[Bug libstdc++/113841] Can't swap two std::hash<T*>

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113841

--- Comment #12 from Jonathan Wakely <redi at gcc dot gnu.org> ---
There's no problem with pair, it's basic_string that fails.

[Bug libstdc++/113841] Can't swap two std::hash<T*>

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113841

--- Comment #13 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:142cc4c223d695e515ed2504501b91d8a7ac6eb8

commit r14-9635-g142cc4c223d695e515ed2504501b91d8a7ac6eb8
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Feb 9 17:06:20 2024 +0000

    libstdc++: Constrain std::vector default constructor [PR113841]

    This is needed to avoid errors outside the immediate context when
    evaluating is_default_constructible_v<vector<T, A>> when A is not
    default constructible.

    To avoid diagnostic regressions for 23_containers/vector/48101_neg.cc we
    need to make the std::allocator<cv T> partial specializations default
    constructible, which they probably should have been anyway.

    libstdc++-v3/ChangeLog:

            PR libstdc++/113841
            * include/bits/allocator.h (allocator<cv T>): Add default
            constructor to partial specializations for cv-qualified types.
            * include/bits/stl_vector.h (_Vector_impl::_Vector_impl()):
            Constrain so that it's only present if the allocator is default
            constructible.
            * include/bits/stl_bvector.h (_Bvector_impl::_Bvector_impl()):
            Likewise.
            * testsuite/23_containers/vector/cons/113841.cc: New test.

[Bug libstdc++/113841] Can't swap two std::hash<T*>

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113841

--- Comment #14 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by Jonathan Wakely
<redi@gcc.gnu.org>:

https://gcc.gnu.org/g:87ec5b369eed205dfe6802afaaec3986b246ade9

commit r13-8569-g87ec5b369eed205dfe6802afaaec3986b246ade9
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Feb 9 17:06:20 2024 +0000

    libstdc++: Constrain std::vector default constructor [PR113841]

    This is needed to avoid errors outside the immediate context when
    evaluating is_default_constructible_v<vector<T, A>> when A is not
    default constructible.

    To avoid diagnostic regressions for 23_containers/vector/48101_neg.cc we
    need to make the std::allocator<cv T> partial specializations default
    constructible, which they probably should have been anyway.

    libstdc++-v3/ChangeLog:

            PR libstdc++/113841
            * include/bits/allocator.h (allocator<cv T>): Add default
            constructor to partial specializations for cv-qualified types.
            * include/bits/stl_vector.h (_Vector_impl::_Vector_impl()):
            Constrain so that it's only present if the allocator is default
            constructible.
            * include/bits/stl_bvector.h (_Bvector_impl::_Bvector_impl()):
            Likewise.
            * testsuite/23_containers/vector/cons/113841.cc: New test.

    (cherry picked from commit 142cc4c223d695e515ed2504501b91d8a7ac6eb8)