Re: [Boost-bugs] [Boost C++ Libraries] #11229: vector incorrectly copies move-only objects using memcpy

Subject: Re: [Boost-bugs] [Boost C++ Libraries] #11229: vector incorrectly copies move-only objects using memcpy
From: Boost C++ Libraries (noreply_at_[hidden])
Date: 2015-12-02 17:05:17

#11229: vector incorrectly copies move-only objects using memcpy
-------------------------------+------------------------
  Reporter:  joseph.thomson@…  |      Owner:  igaztanaga
      Type:  Bugs              |     Status:  new
 Milestone:  To Be Determined  |  Component:  move
   Version:  Boost 1.58.0      |   Severity:  Problem
Resolution:                    |   Keywords:
-------------------------------+------------------------

Comment (by joseph.thomson@…):

 Following the trail:

 `boost/container/detail/copy_move_algo.hpp`
 {{{
 template <typename I, typename O>
 struct is_memtransfer_copy_constructible
    : boost::move_detail::and_
       < are_contiguous_and_same<I, O>
       , container_detail::is_trivially_copy_constructible< typename
 ::boost::container::iterator_traits<I>::value_type >
       >
 {};
 }}}

 `boost/container/detail/type_traits.hpp`
 {{{
 using ::boost::move_detail::is_trivially_copy_constructible;
 }}}

 `boost/move/detail/type_traits.hpp`
 {{{
 template<class T>
 struct is_trivially_copy_constructible
 {
    //In several compilers BOOST_MOVE_IS_TRIVIALLY_COPY_CONSTRUCTIBLE
 return true even with
    //deleted copy constructors so make sure the type is copy
 constructible.
    static const bool value = ::boost::move_detail::is_pod<T>::value ||
                              (
 ::boost::move_detail::is_copy_constructible<T>::value &&
 BOOST_MOVE_IS_TRIVIALLY_COPY_CONSTRUCTIBLE(T) );
 };
 }}}

 {{{
 #ifdef BOOST_MOVE_HAS_TRIVIAL_COPY
    #define BOOST_MOVE_IS_TRIVIALLY_COPY_CONSTRUCTIBLE(T)
 BOOST_MOVE_HAS_TRIVIAL_COPY(T)
 #else
    #define BOOST_MOVE_IS_TRIVIALLY_COPY_CONSTRUCTIBLE(T)
 ::boost::move_detail::is_pod<T>::value
 #endif
 }}}

 ''MSVC''
 {{{
 #   define BOOST_MOVE_HAS_TRIVIAL_COPY(T)
 (__has_trivial_copy(T)|| ::boost::move_detail::is_pod<T>::value)
 }}}

 ''Clang''
 {{{
 #   if __has_feature(has_trivial_copy)
 #     //There are problems with deleted copy constructors detected as
 trivially copyable.
 #     //http://stackoverflow.com/questions/12754886/has-trivial-copy-

 behaves-differently-in-clang-and-gcc-whos-right
 #     define BOOST_MOVE_HAS_TRIVIAL_COPY(T) (__has_trivial_copy(T) &&
 ::boost::move_detail::is_copy_constructible<T>::value)
 #   endif
 }}}

 GCC
 {{{
 #   define BOOST_MOVE_HAS_TRIVIAL_COPY(T) ((__has_trivial_copy(T)
 BOOST_MOVE_INTEL_TT_OPTS))
 }}}

 ''Note that Boost.Container no longer depends on Boost.TypeTraits, so the
 code you cited is not actually being used.''

 The original bug existed because Boost.Container only required
 `boost::has_trivial_copy` to be true to enable the `memcpy` optimization,
 and in GCC 4.9 the behaviour of `__has_trivial_copy` was fixed, meaning
 that `boost::has_trivial_copy` worked correctly in Boost 1.58 with GCC
 4.9, meaning that Boost.Containers thought that `std::unique_ptr` was
 `memcpy`-able.

 As you can see above, Boost.Container fixed the bug by depending on
 `boost::move_detail::is_trivially_copy_constructible` instead, which is
 the correct thing to do (`std::unique_ptr` should give false for this). As
 you can see, Boost.Move implements this by explicitly checking whether the
 class is copy constructible, so this bug should be gone on all compilers
 in Boost 1.59.

 But looking at Boost.Move, `BOOST_MOVE_IS_TRIVIALLY_COPY_CONSTRUCTIBLE` is
 incorrectly implemented as `BOOST_MOVE_HAS_TRIVIAL_COPY`. However,
 `BOOST_MOVE_HAS_TRIVIAL_COPY` behaves incorrectly on GCC 4.8 and MSVC, due
 to `__has_trivial_move` being broken, which means that
 `BOOST_MOVE_IS_TRIVIALLY_COPY_CONSTRUCTIBLE` actually behaves correctly.
 This is also the case on Clang, but only because that "fix" has been
 applied to make it behave like GCC 4.8. Conversely,
 `BOOST_MOVE_HAS_TRIVIAL_COPY` behaves correctly on GCC 4.9, while
 `BOOST_MOVE_IS_TRIVIALLY_COPY_CONSTRUCTIBLE` behaves incorrectly.

 Thus, Boost.Move needs to do the following to fix these problems:

 * Remove the "fix" for Clang
 * Implement `BOOST_MOVE_IS_TRIVIALLY_COPY_CONSTRUCTIBLE` correctly
 * Document that `BOOST_MOVE_HAS_TRIVIAL_COPY` doesn't work on GCC 4.8 or
 MSVC

 It is possible that they may be able to use SFINAE on GCC 4.8 to detect
 the lack of copy constructor, but I have already tried this on MSVC, and
 the bug exists with the SFINAE (alas, this may be the case with GCC 4.8 as
 well).

 ''p.s. I have no access to GCC or Clang, so I was unable to test by
 conclusions. Perhaps you could?''

-- 
Ticket URL: <https://svn.boost.org/trac/boost/ticket/11229#comment:13>
Boost C++ Libraries <http://www.boost.org/>
Boost provides free peer-reviewed portable C++ source libraries.