From patchwork Thu Mar 7 21:28:57 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Fran=C3=A7ois_Dumont?= X-Patchwork-Id: 225954 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 5044F2C03A2 for ; Fri, 8 Mar 2013 08:29:19 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1363296560; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Message-ID:Date:From:User-Agent:MIME-Version:To:Subject: Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:Sender:Delivered-To; bh=5SNb2hT cdt7EeLpwlAMDE56gXG4=; b=nbAVJbqsCqlCHoIi93m0CrMRegc6+AXjUgehZbP NJHU+QHZrQJyfubIc6WYJ8rKiAn0zF0VCLRieyjCVPM3xvWP1oeFEdex6QrDsIzm 9A4Kzw9WQYJVaXf7J5fI9YRuRJPavQNfmGQ6iOfkkXa1R9C8sFPylDKUPUO5nGMe V/eI= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:X-Received:Received:Message-ID:Date:From:User-Agent:MIME-Version:To:Subject:Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=KD2dtsgUpapcFDXqqnh4c86Q92INvvIVqqwy6bgipxpKuo3wO+SjtXhcybA35B osR4RNYVyohCeaBbWv/6sTbeDYnmXarIiJbAw1EbOkP5eS3591C8iKNZWjFK1IzV UWOmr8tNNxwkVK+suX5RK6Udx8LfSubgyxx1jOaxzr6Iw=; Received: (qmail 16675 invoked by alias); 7 Mar 2013 21:29:10 -0000 Received: (qmail 16652 invoked by uid 22791); 7 Mar 2013 21:29:09 -0000 X-SWARE-Spam-Status: No, hits=-4.5 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, FREEMAIL_FROM, KHOP_RCVD_TRUST, RCVD_IN_DNSWL_LOW, RCVD_IN_HOSTKARMA_YE X-Spam-Check-By: sourceware.org Received: from mail-wi0-f171.google.com (HELO mail-wi0-f171.google.com) (209.85.212.171) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 07 Mar 2013 21:29:02 +0000 Received: by mail-wi0-f171.google.com with SMTP id hn17so1120178wib.16 for ; Thu, 07 Mar 2013 13:29:00 -0800 (PST) X-Received: by 10.194.109.136 with SMTP id hs8mr55082602wjb.8.1362691740248; Thu, 07 Mar 2013 13:29:00 -0800 (PST) Received: from localhost.localdomain (arf62-1-82-237-250-248.fbx.proxad.net. [82.237.250.248]) by mx.google.com with ESMTPS id fx5sm5596055wib.11.2013.03.07.13.28.58 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 07 Mar 2013 13:28:58 -0800 (PST) Message-ID: <51390699.9040904@gmail.com> Date: Thu, 07 Mar 2013 22:28:57 +0100 From: =?ISO-8859-1?Q?Fran=E7ois_Dumont?= User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120829 Thunderbird/15.0 MIME-Version: 1.0 To: "libstdc++@gcc.gnu.org" , gcc-patches Subject: Fix vector C++11 allocator bug Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Hi While working on unordered containers C++11 allocator integration I used forward_list tests you have done Jon. It reported some problems that should have been seen on forward_list or vector allocator tests too if those tests were indeed manipulating memory. But there weren't because no element was ever injected in the containers. So I have started injecting elements and the propagating_allocator issue shown up like in my unordered container tests. The problem is that there is an assertion in the allocator to check that memory is returned to the correct allocator instance thanks to the personality. But when forward_list or vector instances with non propagating allocators are swapped personnality is not swapped and the issue. So I have introduced a bool template parameter to disable all assertions regarding personality so that the allocator can still be used to check that personality hasn't been exchanged. Additional, making the vector tests more functional revealed a bug in vector implementation. 2013-03-08 François Dumont * include/bits/vector.tcc (vector<>operator=(const vector<>&): Reset pointers after deallocation when memory can't be reused. * testsuite/util/testsuite_allocator.h (uneq_allocator<>): Add IgnorePerson template parameter to disable assertions regarding allocator personality. * testsuite/23_containers/vector/allocator/minimal.cc: Insert element to really challenge C++11 allocator integration. * testsuite/23_containers/vector/allocator/copy.cc: Likewise. * testsuite/23_containers/vector/allocator/copy_assign.cc: Likewise. * testsuite/23_containers/vector/allocator/move_assign.cc: Likewise. * testsuite/23_containers/vector/allocator/swap.cc: Disable assertions regarding allocator personality when swapping vectors with not propagating allocators. * testsuite/23_containers/forward_list/allocator/minimal.cc: Insert element to really challenge C++11 allocator integration. * testsuite/23_containers/forward_list/allocator/copy.cc: Likewise. * testsuite/23_containers/forward_list/allocator/copy_assign.cc: Likewise. * testsuite/23_containers/forward_list/allocator/move_assign.cc: Likewise. * testsuite/23_containers/forward_list/allocator/swap.cc: Disable assertions regarding allocator personality when swapping vectors with not propagating allocators. Tested under Linux x86_64. Ok to commit ? François Index: include/bits/vector.tcc =================================================================== --- include/bits/vector.tcc (revision 196526) +++ include/bits/vector.tcc (working copy) @@ -173,6 +173,9 @@ _M_deallocate(this->_M_impl._M_start, this->_M_impl._M_end_of_storage - this->_M_impl._M_start); + this->_M_impl._M_start = nullptr; + this->_M_impl._M_finish = nullptr; + this->_M_impl._M_end_of_storage = nullptr; } std::__alloc_on_copy(_M_get_Tp_allocator(), __x._M_get_Tp_allocator()); Index: testsuite/23_containers/vector/allocator/minimal.cc =================================================================== --- testsuite/23_containers/vector/allocator/minimal.cc (revision 196526) +++ testsuite/23_containers/vector/allocator/minimal.cc (working copy) @@ -35,6 +35,7 @@ typedef std::allocator_traits traits_type; typedef std::vector test_type; test_type v(alloc_type{}); + v.push_back(T()); VERIFY( v.max_size() == traits_type::max_size(v.get_allocator()) ); } Index: testsuite/23_containers/vector/allocator/copy.cc =================================================================== --- testsuite/23_containers/vector/allocator/copy.cc (revision 196526) +++ testsuite/23_containers/vector/allocator/copy.cc (working copy) @@ -31,6 +31,7 @@ typedef propagating_allocator alloc_type; typedef std::vector test_type; test_type v1(alloc_type(1)); + v1.push_back(T()); test_type v2(v1); VERIFY(1 == v1.get_allocator().get_personality()); VERIFY(0 == v2.get_allocator().get_personality()); @@ -42,6 +43,7 @@ typedef propagating_allocator alloc_type; typedef std::vector test_type; test_type v1(alloc_type(1)); + v1.push_back(T()); test_type v2(v1); VERIFY(1 == v1.get_allocator().get_personality()); VERIFY(1 == v2.get_allocator().get_personality()); Index: testsuite/23_containers/vector/allocator/move_assign.cc =================================================================== --- testsuite/23_containers/vector/allocator/move_assign.cc (revision 196526) +++ testsuite/23_containers/vector/allocator/move_assign.cc (working copy) @@ -31,7 +31,9 @@ typedef propagating_allocator alloc_type; typedef std::vector test_type; test_type v1(alloc_type(1)); + v1.push_back(T()); test_type v2(alloc_type(2)); + v2.push_back(T()); v2 = std::move(v1); VERIFY(1 == v1.get_allocator().get_personality()); VERIFY(2 == v2.get_allocator().get_personality()); @@ -43,8 +45,10 @@ typedef propagating_allocator alloc_type; typedef std::vector test_type; test_type v1(alloc_type(1)); + v1.push_back(T()); test_type v2(alloc_type(2)); v2 = std::move(v1); + v2.push_back(T()); VERIFY(0 == v1.get_allocator().get_personality()); VERIFY(1 == v2.get_allocator().get_personality()); } Index: testsuite/23_containers/vector/allocator/swap.cc =================================================================== --- testsuite/23_containers/vector/allocator/swap.cc (revision 196526) +++ testsuite/23_containers/vector/allocator/swap.cc (working copy) @@ -25,30 +25,19 @@ using __gnu_test::propagating_allocator; -// It is undefined behaviour to swap() containers wth unequal allocators -// if the allocator doesn't propagate, so ensure the allocators compare -// equal, while still being able to test propagation via get_personality(). -bool -operator==(const propagating_allocator&, - const propagating_allocator&) -{ - return true; -} - -bool -operator!=(const propagating_allocator&, - const propagating_allocator&) -{ - return false; -} - void test01() { bool test __attribute__((unused)) = true; - typedef propagating_allocator alloc_type; + // It is undefined behaviour to swap() containers wth unequal allocators + // if the allocator doesn't propagate, so request allocator to ignore + // personality to have equivalent allocators, while still being able to test + // propagation via get_personality(). + typedef propagating_allocator alloc_type; typedef std::vector test_type; test_type v1(alloc_type(1)); + v1.push_back(T()); test_type v2(alloc_type(2)); + v2.push_back(T()); std::swap(v1, v2); VERIFY(1 == v1.get_allocator().get_personality()); VERIFY(2 == v2.get_allocator().get_personality()); @@ -60,7 +49,9 @@ typedef propagating_allocator alloc_type; typedef std::vector test_type; test_type v1(alloc_type(1)); + v1.push_back(T()); test_type v2(alloc_type(2)); + v2.push_back(T()); std::swap(v1, v2); VERIFY(2 == v1.get_allocator().get_personality()); VERIFY(1 == v2.get_allocator().get_personality()); Index: testsuite/23_containers/vector/allocator/copy_assign.cc =================================================================== --- testsuite/23_containers/vector/allocator/copy_assign.cc (revision 196526) +++ testsuite/23_containers/vector/allocator/copy_assign.cc (working copy) @@ -31,7 +31,9 @@ typedef propagating_allocator alloc_type; typedef std::vector test_type; test_type v1(alloc_type(1)); + v1.push_back(T()); test_type v2(alloc_type(2)); + v2.push_back(T()); v2 = v1; VERIFY(1 == v1.get_allocator().get_personality()); VERIFY(2 == v2.get_allocator().get_personality()); @@ -43,7 +45,9 @@ typedef propagating_allocator alloc_type; typedef std::vector test_type; test_type v1(alloc_type(1)); + v1.push_back(T()); test_type v2(alloc_type(2)); + v2.push_back(T()); v2 = v1; VERIFY(1 == v1.get_allocator().get_personality()); VERIFY(1 == v2.get_allocator().get_personality()); Index: testsuite/23_containers/forward_list/allocator/minimal.cc =================================================================== --- testsuite/23_containers/forward_list/allocator/minimal.cc (revision 196526) +++ testsuite/23_containers/forward_list/allocator/minimal.cc (working copy) @@ -37,6 +37,7 @@ typedef std::allocator_traits traits_type; typedef std::forward_list test_type; test_type v(alloc_type{}); + v.push_front(T()); VERIFY( v.max_size() == traits_type::max_size(v.get_allocator()) ); } Index: testsuite/23_containers/forward_list/allocator/copy.cc =================================================================== --- testsuite/23_containers/forward_list/allocator/copy.cc (revision 196526) +++ testsuite/23_containers/forward_list/allocator/copy.cc (working copy) @@ -31,6 +31,7 @@ typedef propagating_allocator alloc_type; typedef std::forward_list test_type; test_type v1(alloc_type(1)); + v1.push_front(T()); test_type v2(v1); VERIFY(1 == v1.get_allocator().get_personality()); VERIFY(0 == v2.get_allocator().get_personality()); @@ -42,6 +43,7 @@ typedef propagating_allocator alloc_type; typedef std::forward_list test_type; test_type v1(alloc_type(1)); + v1.push_front(T()); test_type v2(v1); VERIFY(1 == v1.get_allocator().get_personality()); VERIFY(1 == v2.get_allocator().get_personality()); Index: testsuite/23_containers/forward_list/allocator/move_assign.cc =================================================================== --- testsuite/23_containers/forward_list/allocator/move_assign.cc (revision 196526) +++ testsuite/23_containers/forward_list/allocator/move_assign.cc (working copy) @@ -31,7 +31,9 @@ typedef propagating_allocator alloc_type; typedef std::forward_list test_type; test_type v1(alloc_type(1)); + v1.push_front(T()); test_type v2(alloc_type(2)); + v2.push_front(T()); v2 = std::move(v1); VERIFY(1 == v1.get_allocator().get_personality()); VERIFY(2 == v2.get_allocator().get_personality()); @@ -43,7 +45,9 @@ typedef propagating_allocator alloc_type; typedef std::forward_list test_type; test_type v1(alloc_type(1)); + v1.push_front(T()); test_type v2(alloc_type(2)); + v2.push_front(T()); v2 = std::move(v1); VERIFY(0 == v1.get_allocator().get_personality()); VERIFY(1 == v2.get_allocator().get_personality()); Index: testsuite/23_containers/forward_list/allocator/swap.cc =================================================================== --- testsuite/23_containers/forward_list/allocator/swap.cc (revision 196526) +++ testsuite/23_containers/forward_list/allocator/swap.cc (working copy) @@ -25,30 +25,19 @@ using __gnu_test::propagating_allocator; -// It is undefined behaviour to swap() containers wth unequal allocators -// if the allocator doesn't propagate, so ensure the allocators compare -// equal, while still being able to test propagation via get_personality(). -bool -operator==(const propagating_allocator&, - const propagating_allocator&) -{ - return true; -} - -bool -operator!=(const propagating_allocator&, - const propagating_allocator&) -{ - return false; -} - void test01() { bool test __attribute__((unused)) = true; - typedef propagating_allocator alloc_type; + // It is undefined behaviour to swap() containers wth unequal allocators + // if the allocator doesn't propagate, so request allocator to ignore + // personality to have equivalent allocators, while still being able to test + // propagation via get_personality(). + typedef propagating_allocator alloc_type; typedef std::forward_list test_type; test_type v1(alloc_type(1)); + v1.push_front(T()); test_type v2(alloc_type(2)); + v2.push_front(T()); std::swap(v1, v2); VERIFY(1 == v1.get_allocator().get_personality()); VERIFY(2 == v2.get_allocator().get_personality()); @@ -60,7 +49,9 @@ typedef propagating_allocator alloc_type; typedef std::forward_list test_type; test_type v1(alloc_type(1)); + v1.push_front(T()); test_type v2(alloc_type(2)); + v2.push_front(T()); std::swap(v1, v2); VERIFY(2 == v1.get_allocator().get_personality()); VERIFY(1 == v2.get_allocator().get_personality()); Index: testsuite/23_containers/forward_list/allocator/copy_assign.cc =================================================================== --- testsuite/23_containers/forward_list/allocator/copy_assign.cc (revision 196526) +++ testsuite/23_containers/forward_list/allocator/copy_assign.cc (working copy) @@ -31,7 +31,9 @@ typedef propagating_allocator alloc_type; typedef std::forward_list test_type; test_type v1(alloc_type(1)); + v1.push_front(T()); test_type v2(alloc_type(2)); + v2.push_front(T()); v2 = v1; VERIFY(1 == v1.get_allocator().get_personality()); VERIFY(2 == v2.get_allocator().get_personality()); @@ -43,7 +45,9 @@ typedef propagating_allocator alloc_type; typedef std::forward_list test_type; test_type v1(alloc_type(1)); + v1.push_front(T()); test_type v2(alloc_type(2)); + v2.push_front(T()); v2 = v1; VERIFY(1 == v1.get_allocator().get_personality()); VERIFY(1 == v2.get_allocator().get_personality()); Index: testsuite/util/testsuite_allocator.h =================================================================== --- testsuite/util/testsuite_allocator.h (revision 196526) +++ testsuite/util/testsuite_allocator.h (working copy) @@ -243,7 +243,9 @@ } }; - template + // IgnorePerson can be used to disable internal checks on personality while + // still allowing to check it in the test. + template class uneq_allocator : private uneq_allocator_base { @@ -261,8 +263,8 @@ #endif template - struct rebind - { typedef uneq_allocator other; }; + struct rebind + { typedef uneq_allocator other; }; uneq_allocator() _GLIBCXX_USE_NOEXCEPT : personality(0) { } @@ -271,7 +273,8 @@ : personality(person) { } template - uneq_allocator(const uneq_allocator& b) _GLIBCXX_USE_NOEXCEPT + uneq_allocator(const uneq_allocator& b) + _GLIBCXX_USE_NOEXCEPT : personality(b.get_personality()) { } ~uneq_allocator() _GLIBCXX_USE_NOEXCEPT @@ -319,7 +322,7 @@ // Enforce requirements in Table 32 about deallocation vs // allocator equality. - VERIFY( it->second == personality ); + VERIFY( IgnorePerson || it->second == personality ); get_map().erase(it); ::operator delete(p); @@ -331,8 +334,8 @@ #if __cplusplus >= 201103L template - void - construct(U* p, Args&&... args) + void + construct(U* p, Args&&... args) { ::new((void *)p) U(std::forward(args)...); } template @@ -357,31 +360,32 @@ #endif private: - // ... yet swappable! friend inline void swap(uneq_allocator& a, uneq_allocator& b) { std::swap(a.personality, b.personality); } template - friend inline bool - operator==(const uneq_allocator& a, const uneq_allocator& b) - { return a.personality == b.personality; } + friend inline bool + operator==(const uneq_allocator& a, + const uneq_allocator& b) + { return IgnorePerson || a.personality == b.personality; } template - friend inline bool - operator!=(const uneq_allocator& a, const uneq_allocator& b) - { return !(a == b); } + friend inline bool + operator!=(const uneq_allocator& a, + const uneq_allocator& b) + { return !(a == b); } int personality; }; #if __cplusplus >= 201103L // An uneq_allocator which can be used to test allocator propagation. - template - class propagating_allocator : public uneq_allocator + template + class propagating_allocator : public uneq_allocator { - typedef uneq_allocator base_alloc; + typedef uneq_allocator base_alloc; base_alloc& base() { return *this; } const base_alloc& base() const { return *this; } void swap_base(base_alloc& b) { swap(b, this->base()); } @@ -392,15 +396,17 @@ // default allocator_traits::rebind_alloc would select // uneq_allocator::rebind so we must define rebind here template - struct rebind { typedef propagating_allocator other; }; + struct rebind + { typedef propagating_allocator other; }; propagating_allocator(int i) noexcept : base_alloc(i) { } template - propagating_allocator(const propagating_allocator& a) - noexcept + propagating_allocator(const propagating_allocator& a) + noexcept : base_alloc(a) { } @@ -417,13 +423,13 @@ } template - propagating_allocator& - operator=(const propagating_allocator& a) noexcept - { + propagating_allocator& + operator=(const propagating_allocator& a) noexcept + { static_assert(P2, "assigning propagating_allocator"); propagating_allocator(a).swap_base(*this); return *this; - } + } // postcondition: a.get_personality() == 0 propagating_allocator(propagating_allocator&& a) noexcept