Message ID | 52BDC6AD.5030207@gmail.com |
---|---|
State | New |
Headers | show |
I saw on another thread that 4.9 release was getting closer. Applying this patch sounds like a good idear for this release. Any feedback ? On 12/27/2013 07:27 PM, François Dumont wrote: > Hi > > Here is a patch to fix an issue in normal mode during the move > assignment. The destination vector allocator instance is moved too > during the assignment which is wrong. > > As I discover this problem while working on issues with management > of safe iterators during move operations this patch also fix those > issues in the debug mode for the vector container. Fixes for other > containers in debug mode will come later. > > 2013-12-27 François Dumont <fdumont@gcc.gnu.org> > > * include/bits/stl_vector.h (std::vector<>::_M_move_assign): Pass > *this allocator instance when building temporary vector instance > so that *this allocator do not get moved. > * include/debug/safe_base.h > (_Safe_sequence_base(_Safe_sequence_base&&)): New. > * include/debug/vector (__gnu_debug::vector<>(vector&&)): Use > latter. > (__gnu_debug::vector<>(vector&&, const allocator_type&)): Swap > safe iterators if the instance is moved. > (__gnu_debug::vector<>::operator=(vector&&)): Likewise. > * testsuite/23_containers/vector/allocator/move.cc (test01): Add > check on a vector iterator. > * testsuite/23_containers/vector/allocator/move_assign.cc > (test02): Likewise. > (test03): New, test with a non-propagating allocator. > * testsuite/23_containers/vector/debug/move_assign_neg.cc: New. > > Tested under Linux x86_64 normal and debug modes. > > I will be in vacation for a week starting today so if you want to > apply it quickly do not hesitate to do it yourself. > > François >
On 7 January 2014 21:12, François Dumont wrote: > I saw on another thread that 4.9 release was getting closer. Applying this > patch sounds like a good idear for this release. > > Any feedback ? Yes, it's coming :-) I imagine most of us have been on holiday and are just getting back to speed, and I've also started a new job. I haven't forgotten about the patch (and if it's appropriate for 4.9 there's still enough time to get it in).
On 27 December 2013 18:27, François Dumont wrote: > Hi > > Here is a patch to fix an issue in normal mode during the move > assignment. The destination vector allocator instance is moved too during > the assignment which is wrong. Thanks for your patience, the normal-mode fix is definitely correct, and I've finished reviewing the other parts and they look good too. > As I discover this problem while working on issues with management of > safe iterators during move operations this patch also fix those issues in > the debug mode for the vector container. Fixes for other containers in debug > mode will come later. OK, great. In the new test you have: + VERIFY( it == v1.begin() ); // Error, it singular Please change this to "Error, it is singular" > 2013-12-27 François Dumont <fdumont@gcc.gnu.org> > > * include/bits/stl_vector.h (std::vector<>::_M_move_assign): Pass > *this allocator instance when building temporary vector instance > so that *this allocator do not get moved. Please change this to "does not get moved" > * include/debug/safe_base.h > (_Safe_sequence_base(_Safe_sequence_base&&)): New. > * include/debug/vector (__gnu_debug::vector<>(vector&&)): Use > latter. I don't think "latter" is clear here, please say something like "Use new move constructor for base class" or "... for _Safe_sequence_base". This is OK for trunk, thanks very much. We might also want to fix just the normal-mode part on the 4.8 branch, I'll think about that.
On Fri, Dec 27, 2013 at 10:27 AM, François Dumont <frs.dumont@gmail.com> wrote: > Hi > > Here is a patch to fix an issue in normal mode during the move > assignment. The destination vector allocator instance is moved too during > the assignment which is wrong. > > As I discover this problem while working on issues with management of > safe iterators during move operations this patch also fix those issues in > the debug mode for the vector container. Fixes for other containers in debug > mode will come later. > > 2013-12-27 François Dumont <fdumont@gcc.gnu.org> > > * include/bits/stl_vector.h (std::vector<>::_M_move_assign): Pass > *this allocator instance when building temporary vector instance > so that *this allocator do not get moved. > * include/debug/safe_base.h > (_Safe_sequence_base(_Safe_sequence_base&&)): New. > * include/debug/vector (__gnu_debug::vector<>(vector&&)): Use > latter. > (__gnu_debug::vector<>(vector&&, const allocator_type&)): Swap > safe iterators if the instance is moved. > (__gnu_debug::vector<>::operator=(vector&&)): Likewise. > * testsuite/23_containers/vector/allocator/move.cc (test01): Add > check on a vector iterator. > * testsuite/23_containers/vector/allocator/move_assign.cc > (test02): Likewise. > (test03): New, test with a non-propagating allocator. > * testsuite/23_containers/vector/debug/move_assign_neg.cc: New. > > Tested under Linux x86_64 normal and debug modes. > > I will be in vacation for a week starting today so if you want to apply it > quickly do not hesitate to do it yourself. > This caused: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59738
Index: include/bits/stl_vector.h =================================================================== --- include/bits/stl_vector.h (revision 206214) +++ include/bits/stl_vector.h (working copy) @@ -1433,7 +1433,7 @@ void _M_move_assign(vector&& __x, std::true_type) noexcept { - const vector __tmp(std::move(*this)); + const vector __tmp(std::move(*this), get_allocator()); this->_M_impl._M_swap_data(__x._M_impl); if (_Alloc_traits::_S_propagate_on_move_assign()) std::__alloc_on_move(_M_get_Tp_allocator(), Index: include/debug/safe_base.h =================================================================== --- include/debug/safe_base.h (revision 206214) +++ include/debug/safe_base.h (working copy) @@ -192,6 +192,12 @@ : _M_iterators(0), _M_const_iterators(0), _M_version(1) { } +#if __cplusplus >= 201103L + _Safe_sequence_base(_Safe_sequence_base&& __x) noexcept + : _Safe_sequence_base() + { _M_swap(__x); } +#endif + /** Notify all iterators that reference this sequence that the sequence is being destroyed. */ ~_Safe_sequence_base() Index: include/debug/vector =================================================================== --- include/debug/vector (revision 206214) +++ include/debug/vector (working copy) @@ -52,6 +52,7 @@ typedef __gnu_debug::_Equal_to<_Base_const_iterator> _Equal; #if __cplusplus >= 201103L + typedef __gnu_debug::_Safe_sequence<vector<_Tp, _Allocator> > _Safe_base; typedef __gnu_cxx::__alloc_traits<_Allocator> _Alloc_traits; #endif @@ -111,18 +112,16 @@ vector(const vector& __x) : _Base(__x), _M_guaranteed_capacity(__x.size()) { } - /// Construction from a release-mode vector + /// Construction from a normal-mode vector vector(const _Base& __x) : _Base(__x), _M_guaranteed_capacity(__x.size()) { } #if __cplusplus >= 201103L vector(vector&& __x) noexcept : _Base(std::move(__x)), + _Safe_base(std::move(__x)), _M_guaranteed_capacity(this->size()) - { - this->_M_swap(__x); - __x._M_guaranteed_capacity = 0; - } + { __x._M_guaranteed_capacity = 0; } vector(const vector& __x, const allocator_type& __a) : _Base(__x, __a), _M_guaranteed_capacity(__x.size()) { } @@ -131,7 +130,10 @@ : _Base(std::move(__x), __a), _M_guaranteed_capacity(this->size()) { - __x._M_invalidate_all(); + if (__x.get_allocator() == __a) + this->_M_swap(__x); + else + __x._M_invalidate_all(); __x._M_guaranteed_capacity = 0; } @@ -146,7 +148,7 @@ vector& operator=(const vector& __x) { - static_cast<_Base&>(*this) = __x; + _M_base() = __x; this->_M_invalidate_all(); _M_update_guaranteed_capacity(); return *this; @@ -157,8 +159,13 @@ operator=(vector&& __x) noexcept(_Alloc_traits::_S_nothrow_move()) { __glibcxx_check_self_move_assign(__x); - _Base::operator=(std::move(__x)); - this->_M_invalidate_all(); + bool xfer_memory = _Alloc_traits::_S_propagate_on_move_assign() + || __x.get_allocator() == this->get_allocator(); + _M_base() = std::move(__x._M_base()); + if (xfer_memory) + this->_M_swap(__x); + else + this->_M_invalidate_all(); _M_update_guaranteed_capacity(); __x._M_invalidate_all(); __x._M_guaranteed_capacity = 0; @@ -168,7 +175,7 @@ vector& operator=(initializer_list<value_type> __l) { - static_cast<_Base&>(*this) = __l; + _M_base() = __l; this->_M_invalidate_all(); _M_update_guaranteed_capacity(); return *this; Index: testsuite/23_containers/vector/allocator/move.cc =================================================================== --- testsuite/23_containers/vector/allocator/move.cc (revision 206214) +++ testsuite/23_containers/vector/allocator/move.cc (working copy) @@ -32,9 +32,11 @@ typedef std::vector<T, alloc_type> test_type; test_type v1(alloc_type(1)); v1 = { T() }; + auto it = v1.begin(); test_type v2(std::move(v1)); VERIFY(1 == v1.get_allocator().get_personality()); VERIFY(1 == v2.get_allocator().get_personality()); + VERIFY( it == v2.begin() ); } void test02() Index: testsuite/23_containers/vector/allocator/move_assign.cc =================================================================== --- testsuite/23_containers/vector/allocator/move_assign.cc (revision 206214) +++ testsuite/23_containers/vector/allocator/move_assign.cc (working copy) @@ -46,16 +46,35 @@ typedef std::vector<T, alloc_type> test_type; test_type v1(alloc_type(1)); v1.push_back(T()); + auto it = v1.begin(); test_type v2(alloc_type(2)); + v2.push_back(T()); v2 = std::move(v1); - v2.push_back(T()); + VERIFY( it == v2.begin() ); VERIFY(0 == v1.get_allocator().get_personality()); VERIFY(1 == v2.get_allocator().get_personality()); } +void test03() +{ + bool test __attribute__((unused)) = true; + typedef propagating_allocator<T, false> alloc_type; + typedef std::vector<T, alloc_type> test_type; + test_type v1(alloc_type(1)); + v1.push_back(T()); + auto it = v1.begin(); + test_type v2(alloc_type(1)); + v2.push_back(T()); + v2 = std::move(v1); + VERIFY( it == v2.begin() ); + VERIFY(1 == v1.get_allocator().get_personality()); + VERIFY(1 == v2.get_allocator().get_personality()); +} + int main() { test01(); test02(); + test03(); return 0; } Index: testsuite/23_containers/vector/debug/move_assign_neg.cc =================================================================== --- testsuite/23_containers/vector/debug/move_assign_neg.cc (revision 0) +++ testsuite/23_containers/vector/debug/move_assign_neg.cc (revision 0) @@ -0,0 +1,50 @@ +// Copyright (C) 2013 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. +// +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. +// +// { dg-options "-std=gnu++11" } +// { dg-do run { xfail *-*-* } } + +#include <debug/vector> + +#include <testsuite_allocator.h> +#include <testsuite_hooks.h> + +void test01() +{ + bool test __attribute__((unused)) = true; + + typedef __gnu_test::uneq_allocator<int> alloc_type; + typedef __gnu_debug::vector<int, alloc_type> test_type; + + test_type v1(alloc_type(1)); + v1 = { 0, 1, 2, 3 }; + + test_type v2(alloc_type(2)); + v2 = { 4, 5, 6, 7 }; + + auto it = v2.begin(); + + v1 = std::move(v2); + + VERIFY( it == v1.begin() ); // Error, it singular +} + +int main() +{ + test01(); + return 0; +}