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

src/secure_allocator.hpp: define missing 'rebind' type #4480

Merged
merged 1 commit into from
Dec 22, 2022
Merged

src/secure_allocator.hpp: define missing 'rebind' type #4480

merged 1 commit into from
Dec 22, 2022

Conversation

trofi
Copy link
Contributor

@trofi trofi commented Dec 20, 2022

gcc-13 added an assert to standard headers to make sure custom allocators have intended implementation of rebind type instead of inherited rebind. gcc change:
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=64c986b49558a7

Without the fix build fails on this week's gcc-13 as:

[ 92%] Building CXX object tests/CMakeFiles/test_security_curve.dir/test_security_curve.cpp.o
In file included from /<<NIX>>/gcc-13.0.0/include/c++/13.0.0/ext/alloc_traits.h:34,
                 from /<<NIX>>/gcc-13.0.0/include/c++/13.0.0/bits/stl_uninitialized.h:64,
                 from /<<NIX>>/gcc-13.0.0/include/c++/13.0.0/memory:69,
                 from tests/../src/secure_allocator.hpp:42,
                 from tests/../src/curve_client_tools.hpp:49,
                 from tests/test_security_curve.cpp:53:
/<<NIX>>/gcc-13.0.0/include/c++/13.0.0/bits/alloc_traits.h: In instantiation of 'struct std::__allocator_traits_base::__rebind<zmq::secure_allocator_t<unsigned char>, unsigned char, void>':
/<<NIX>>/gcc-13.0.0/include/c++/13.0.0/bits/alloc_traits.h:94:11:   required by substitution of 'template<class _Alloc, class _Up> using std::__alloc_rebind = typename std::__allocator_traits_base::__rebind<_Alloc, _Up>::type [with _Alloc = zmq::secure_allocator_t<unsigned char>; _Up = unsigned char]'
/<<NIX>>/gcc-13.0.0/include/c++/13.0.0/bits/alloc_traits.h:228:8:   required by substitution of 'template<class _Alloc> template<class _Tp> using std::allocator_traits< <template-parameter-1-1> >::rebind_alloc = std::__alloc_rebind<_Alloc, _Tp> [with _Tp = unsigned char; _Alloc = zmq::secure_allocator_t<unsigned char>]'
/<<NIX>>/gcc-13.0.0/include/c++/13.0.0/ext/alloc_traits.h:126:65:   required from 'struct __gnu_cxx::__alloc_traits<zmq::secure_allocator_t<unsigned char>, unsigned char>::rebind<unsigned char>'
/<<NIX>>/gcc-13.0.0/include/c++/13.0.0/bits/stl_vector.h:88:21:   required from 'struct std::_Vector_base<unsigned char, zmq::secure_allocator_t<unsigned char> >'
/<<NIX>>/gcc-13.0.0/include/c++/13.0.0/bits/stl_vector.h:423:11:   required from 'class std::vector<unsigned char, zmq::secure_allocator_t<unsigned char> >'
tests/../src/curve_client_tools.hpp:64:76:   required from here
/<<NIX>>/gcc-13.0.0/include/c++/13.0.0/bits/alloc_traits.h:70:31: error: static assertion failed: allocator_traits<A>::rebind_alloc<A::value_type> must be A
   70 |                         _Tp>::value,
      |                               ^~~~~

The change adds trivial rebind definition with expected return type.

@bluca
Copy link
Member

bluca commented Dec 20, 2022

Build fails on windows:

2>C:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\include\xutility(419) : error C2664: 'zmq::secure_allocator_t<T>::secure_allocator_t(const zmq::secure_allocator_t<T> &) throw()' : cannot convert parameter 1 from 'zmq::secure_allocator_t<T>' to 'const zmq::secure_allocator_t<T> &'
2>        with
2>        [
2>            T=std::_Aux_cont
2>        ]
2>        and
2>        [
2>            T=uint8_t
2>        ]
2>        and
2>        [
2>            T=std::_Aux_cont
2>        ]
2>        Reason: cannot convert from 'zmq::secure_allocator_t<T>' to 'const zmq::secure_allocator_t<T>'
2>        with
2>        [
2>            T=uint8_t
2>        ]
2>        and
2>        [
2>            T=std::_Aux_cont
2>        ]
2>        No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
2>        C:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\include\xutility(417) : while compiling class template member function 'std::_Container_base_aux_alloc_real<_Alloc>::_Container_base_aux_alloc_real(_Alloc)'
2>        with
2>        [
2>            _Alloc=zmq::secure_allocator_t<uint8_t>
2>        ]
2>        C:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\include\vector(421) : see reference to class template instantiation 'std::_Container_base_aux_alloc_real<_Alloc>' being compiled
2>        with
2>        [
2>            _Alloc=zmq::secure_allocator_t<uint8_t>
2>        ]
2>        C:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\include\vector(439) : see reference to class template instantiation 'std::_Vector_val<_Ty,_Alloc>' being compiled
2>        with
2>        [
2>            _Ty=uint8_t,
2>            _Alloc=zmq::secure_allocator_t<uint8_t>
2>        ]
2>        c:\projects\libzmq\src\curve_client_tools.hpp(65) : see reference to class template instantiation 'std::vector<_Ty,_Ax>' being compiled
2>        with
2>        [
2>            _Ty=uint8_t,
2>            _Ax=zmq::secure_allocator_t<uint8_t>
2>        ]
2>ws_decoder.cpp
2>ws_connecter.cpp

@trofi trofi marked this pull request as draft December 20, 2022 22:30
@trofi
Copy link
Contributor Author

trofi commented Dec 20, 2022

Thank you! Let's mark as draft. I'll need some time to understand why Reason: cannot convert from 'zmq::secure_allocator_t<T>' to 'const zmq::secure_allocator_t<T>' became special.

@@ -99,6 +99,10 @@ bool operator!= (const secure_allocator_t<T> &, const secure_allocator_t<U> &)
#else
template <typename T> struct secure_allocator_t : std::allocator<T>
{
template <class U> struct rebind
Copy link

@jwakely jwakely Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
template <class U> struct rebind
secure_allocator_t() { }
template<class U> secure_allocator_t(const secure_allocator_t<U>&) { }
template <class U> struct rebind

The error is because you need a converting constructor that can be used to construct Alloc<U> from Alloc<T> and vice versa. This adds it.

The other requirements for an allocator type are value_type, allocate and deallocate, and operator== which can both come from the std::allocator base class.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

became special

It didn't.

To rebind an allocator you need to be able to determine the rebound type, and construct an instance of the rebound type from the original type. With the missing rebind the rebound type is std::allocator<U> and that can be constructed from a secure_allocator_t<T> (via slicing). But that's broken.

With the new rebind member the rebound type is correct: secure_allocator_t<U>. But you can't construct that from secure_allocator_t<T>. So you need to add a converting constructor, and then you also need a user-declared default constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Taken default constructor and conversion constructor definitions from the non-no-op variant as well. I think it is equivalent to the definition you suggested.

`gcc-13` added an assert to standard headers to make sure custom
allocators have intended implementation of rebind type instead
of inherited rebind. gcc change:
    https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=64c986b49558a7

Without the fix build fails on this week's `gcc-13` as:

    [ 92%] Building CXX object tests/CMakeFiles/test_security_curve.dir/test_security_curve.cpp.o
    In file included from /<<NIX>>/gcc-13.0.0/include/c++/13.0.0/ext/alloc_traits.h:34,
                     from /<<NIX>>/gcc-13.0.0/include/c++/13.0.0/bits/stl_uninitialized.h:64,
                     from /<<NIX>>/gcc-13.0.0/include/c++/13.0.0/memory:69,
                     from tests/../src/secure_allocator.hpp:42,
                     from tests/../src/curve_client_tools.hpp:49,
                     from tests/test_security_curve.cpp:53:
    /<<NIX>>/gcc-13.0.0/include/c++/13.0.0/bits/alloc_traits.h: In instantiation of 'struct std::__allocator_traits_base::__rebind<zmq::secure_allocator_t<unsigned char>, unsigned char, void>':
    /<<NIX>>/gcc-13.0.0/include/c++/13.0.0/bits/alloc_traits.h:94:11:   required by substitution of 'template<class _Alloc, class _Up> using std::__alloc_rebind = typename std::__allocator_traits_base::__rebind<_Alloc, _Up>::type [with _Alloc = zmq::secure_allocator_t<unsigned char>; _Up = unsigned char]'
    /<<NIX>>/gcc-13.0.0/include/c++/13.0.0/bits/alloc_traits.h:228:8:   required by substitution of 'template<class _Alloc> template<class _Tp> using std::allocator_traits< <template-parameter-1-1> >::rebind_alloc = std::__alloc_rebind<_Alloc, _Tp> [with _Tp = unsigned char; _Alloc = zmq::secure_allocator_t<unsigned char>]'
    /<<NIX>>/gcc-13.0.0/include/c++/13.0.0/ext/alloc_traits.h:126:65:   required from 'struct __gnu_cxx::__alloc_traits<zmq::secure_allocator_t<unsigned char>, unsigned char>::rebind<unsigned char>'
    /<<NIX>>/gcc-13.0.0/include/c++/13.0.0/bits/stl_vector.h:88:21:   required from 'struct std::_Vector_base<unsigned char, zmq::secure_allocator_t<unsigned char> >'
    /<<NIX>>/gcc-13.0.0/include/c++/13.0.0/bits/stl_vector.h:423:11:   required from 'class std::vector<unsigned char, zmq::secure_allocator_t<unsigned char> >'
    tests/../src/curve_client_tools.hpp:64:76:   required from here
    /<<NIX>>/gcc-13.0.0/include/c++/13.0.0/bits/alloc_traits.h:70:31: error: static assertion failed: allocator_traits<A>::rebind_alloc<A::value_type> must be A
       70 |                         _Tp>::value,
          |                               ^~~~~

The change adds trivial `rebind` definition with expected return type
and satisfies conversion requirements.
@trofi trofi marked this pull request as ready for review December 20, 2022 23:03
@trofi trofi requested a review from jwakely December 20, 2022 23:03
Copy link

@jwakely jwakely left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me

@bluca bluca merged commit bdd471f into zeromq:master Dec 22, 2022
@trofi trofi deleted the gcc-13-rebind-fix branch December 22, 2022 11:16
@kloczek
Copy link

kloczek commented Jan 31, 2023

Is it possible to release new version with that fix? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants