From patchwork Tue Feb 5 14:45:06 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 1036802 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-495302-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="cu9IAFNr"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 43v6nc5KBwz9s4Z for ; Wed, 6 Feb 2019 01:45:40 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:mime-version:content-type; q=dns; s=default; b=yHyVGxD/erHl8vEarQ1JlHPouwPXfvlPwGPyGCaCIagNG/IJ/h m3NFTfmwf0+tlTxiYSIIftB0hJPrEFb2b0N71XAgoPRvPzYPentI2AYcyNn4Rjva nkMdiotrwDAfP6L35uv13uvbuuNncFRJiSkCwX/6tPVBUboSyunt5Zf3c= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:mime-version:content-type; s= default; bh=DxbxmEd8LLvIZ+J+hTH46E7Nv0I=; b=cu9IAFNr3IT2kCXhL48k csjXKsJjytUugBDmlspaXiwirzEfVtMV2f5W7GF62w9tZ5a2r7VgNNC1c1gCbD1w KW1zY5pgIMB/BUTfnwfBV7KDasUl6Ef2iO9Pod3jqNx9DYT550ijtBTlMBadVbVu Mir0QhfQb3ykSgnx9yOC4XY= Received: (qmail 16954 invoked by alias); 5 Feb 2019 14:45:31 -0000 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 Received: (qmail 16666 invoked by uid 89); 5 Feb 2019 14:45:22 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, KAM_SHORT, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=utilities, relocate X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 05 Feb 2019 14:45:15 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 45FA280E7B; Tue, 5 Feb 2019 14:45:08 +0000 (UTC) Received: from localhost (unknown [10.33.36.88]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9DED96B497; Tue, 5 Feb 2019 14:45:07 +0000 (UTC) Date: Tue, 5 Feb 2019 14:45:06 +0000 From: Jonathan Wakely To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Cc: Marc Glisse Subject: [PATCH] libstdc++/89130 and libstdc++/89090 fixes for vector relocation Message-ID: <20190205144506.GL4162@redhat.com> MIME-Version: 1.0 Content-Disposition: inline X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.10.1 (2018-07-13) This fixes two PRs, one trivial (don't use C++17 features in C++11 mode) and one more serious (don't require MoveInsertable when we should only need CopyInsertable). It would be nice to rely on if-constexpr in C++11 mode, but it causes clang warnings, complicates testcase bisection/reduction, and causes users to file bogus bug reports. So let's just avoid it. Tested powerpc64le-linux, committed to trunk. commit 51908e56bd32b5f89bc909193c3da957de01c3e0 Author: Jonathan Wakely Date: Tue Feb 5 11:50:18 2019 +0000 PR libstdc++/89130 restore support for non-MoveConstructible types The changes to "relocate" std::vector elements can lead to new errors outside the immediate context, because moving the elements to new storage no longer makes use of the move-if-noexcept utilities. This means that types with deleted moves no longer degenerate to copies, but are just ill-formed. The errors happen while instantiating the noexcept-specifier for __relocate_object_a, when deciding whether to try to relocate. This patch introduces indirections to avoid the ill-formed instantiations of std::__relocate_object_a. In order to avoid using if-constexpr prior to C++17 this is done by tag dispatching. After this patch all uses of std::__relocate_a are guarded by checks that will support sensible code (i.e. code not using custom allocators that fool the new checks). PR libstdc++/89130 * include/bits/alloc_traits.h (__is_copy_insertable_impl): Rename to __is_alloc_insertable_impl. Replace single type member with two members, one for each of copy and move insertable. (__is_move_insertable): New trait for internal use. * include/bits/stl_vector.h (vector::_S_nothrow_relocate(true_type)) (vector::_S_nothrow_relocate(true_type)): New functions to conditionally check if __relocate_a can throw. (vector::_S_use_relocate()): Dispatch to _S_nothrow_relocate based on __is_move_insertable. (vector::_S_do_relocate): New overloaded functions to conditionally call __relocate_a. (vector::_S_relocate): New function that dispatches to _S_do_relocate based on _S_use_relocate. * include/bits/vector.tcc (vector::reserve, vector::_M_realloc_insert) (vector::_M_default_append): Call _S_relocate instead of __relocate_a. * testsuite/23_containers/vector/modifiers/push_back/89130.cc: New. diff --git a/libstdc++-v3/include/bits/alloc_traits.h b/libstdc++-v3/include/bits/alloc_traits.h index ed61ce845f8..3b0c16fbf64 100644 --- a/libstdc++-v3/include/bits/alloc_traits.h +++ b/libstdc++-v3/include/bits/alloc_traits.h @@ -577,14 +577,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } template - class __is_copy_insertable_impl + class __is_alloc_insertable_impl { - typedef allocator_traits<_Alloc> _Traits; + using _Traits = allocator_traits<_Alloc>; + using value_type = typename _Traits::value_type; - template, + typename = decltype(_Traits::construct(std::declval<_Alloc&>(), - std::declval<_Up*>(), - std::declval()))> + std::declval<_Tp*>(), + std::declval<_Up>()))> static true_type _M_select(int); @@ -593,13 +595,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _M_select(...); public: - typedef decltype(_M_select(0)) type; + using copy = decltype(_M_select(0)); + using move = decltype(_M_select(0)); }; // true if _Alloc::value_type is CopyInsertable into containers using _Alloc template struct __is_copy_insertable - : __is_copy_insertable_impl<_Alloc>::type + : __is_alloc_insertable_impl<_Alloc>::copy { }; // std::allocator<_Tp> just requires CopyConstructible @@ -608,6 +611,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION : is_copy_constructible<_Tp> { }; + // true if _Alloc::value_type is MoveInsertable into containers using _Alloc + template + struct __is_move_insertable + : __is_alloc_insertable_impl<_Alloc>::move + { }; + + // std::allocator<_Tp> just requires MoveConstructible + template + struct __is_move_insertable> + : is_move_constructible<_Tp> + { }; + // Trait to detect Allocator-like types. template struct __is_allocator : false_type { }; diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h index 43debda54f1..10bf4fac62e 100644 --- a/libstdc++-v3/include/bits/stl_vector.h +++ b/libstdc++-v3/include/bits/stl_vector.h @@ -425,14 +425,47 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER private: #if __cplusplus >= 201103L static constexpr bool - _S_use_relocate() + _S_nothrow_relocate(true_type) { return noexcept(std::__relocate_a(std::declval(), std::declval(), std::declval(), std::declval<_Tp_alloc_type&>())); } -#endif + + static constexpr bool + _S_nothrow_relocate(false_type) + { return false; } + + static constexpr bool + _S_use_relocate() + { + // Instantiating std::__relocate_a might cause an error outside the + // immediate context (in __relocate_object_a's noexcept-specifier), + // so only do it if we know the type can be move-inserted into *this. + return _S_nothrow_relocate(__is_move_insertable<_Tp_alloc_type>{}); + } + + static pointer + _S_do_relocate(pointer __first, pointer __last, pointer __result, + _Tp_alloc_type& __alloc, true_type) noexcept + { + return std::__relocate_a(__first, __last, __result, __alloc); + } + + static pointer + _S_do_relocate(pointer, pointer, pointer __result, + _Tp_alloc_type&, false_type) noexcept + { return __result; } + + static pointer + _S_relocate(pointer __first, pointer __last, pointer __result, + _Tp_alloc_type& __alloc) noexcept + { + using __do_it = __bool_constant<_S_use_relocate()>; + return _S_do_relocate(__first, __last, __result, __alloc, __do_it{}); + } +#endif // C++11 protected: using _Base::_M_allocate; diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc index 54c09774b15..497d9f72247 100644 --- a/libstdc++-v3/include/bits/vector.tcc +++ b/libstdc++-v3/include/bits/vector.tcc @@ -76,9 +76,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER if _GLIBCXX17_CONSTEXPR (_S_use_relocate()) { __tmp = this->_M_allocate(__n); - std::__relocate_a(this->_M_impl._M_start, - this->_M_impl._M_finish, - __tmp, _M_get_Tp_allocator()); + _S_relocate(this->_M_impl._M_start, this->_M_impl._M_finish, + __tmp, _M_get_Tp_allocator()); } else #endif @@ -459,17 +458,13 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER #if __cplusplus >= 201103L if _GLIBCXX17_CONSTEXPR (_S_use_relocate()) { - __new_finish - = std::__relocate_a - (__old_start, __position.base(), - __new_start, _M_get_Tp_allocator()); + __new_finish = _S_relocate(__old_start, __position.base(), + __new_start, _M_get_Tp_allocator()); ++__new_finish; - __new_finish - = std::__relocate_a - (__position.base(), __old_finish, - __new_finish, _M_get_Tp_allocator()); + __new_finish = _S_relocate(__position.base(), __old_finish, + __new_finish, _M_get_Tp_allocator()); } else #endif @@ -650,9 +645,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER _M_deallocate(__new_start, __len); __throw_exception_again; } - std::__relocate_a(this->_M_impl._M_start, - this->_M_impl._M_finish, - __new_start, _M_get_Tp_allocator()); + _S_relocate(this->_M_impl._M_start, this->_M_impl._M_finish, + __new_start, _M_get_Tp_allocator()); } else { diff --git a/libstdc++-v3/testsuite/23_containers/vector/modifiers/push_back/89130.cc b/libstdc++-v3/testsuite/23_containers/vector/modifiers/push_back/89130.cc new file mode 100644 index 00000000000..54b3f53069b --- /dev/null +++ b/libstdc++-v3/testsuite/23_containers/vector/modifiers/push_back/89130.cc @@ -0,0 +1,63 @@ +// Copyright (C) 2019 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 +// . + +// { dg-options "-std=gnu++2a" } +// { dg-do compile { target c++2a } } + +#include + +struct T +{ + T() { } + T(const T&) { } + T(T&&) = delete; // this means T is not MoveInsertable into std::vector +}; + +void f() +{ + const T val; + std::vector x; + // push_back(const T&) only requires T is CopyInsertable into std::vector: + x.push_back(val); +} + +template +struct Alloc +{ + using value_type = U; + Alloc() = default; + Alloc(const Alloc&) = default; + template + Alloc(const Alloc&) { } + + U* allocate(unsigned n) { return std::allocator().allocate(n); } + void deallocate(U* p, unsigned n) { std::allocator().deallocate(p, n); } + + void construct(Alloc*, U* p, U&& u) + { + // construct from const lvalue instead of rvalue: + ::new(p) U(const_cast(u)); + } +}; + +void g() +{ + const T val; + std::vector> x; + // push_back(const T&) only requires T is CopyInsertable into std::vector: + x.push_back(val); +} commit 45094752353b4c49fb1bc8dc2f2b5254235a7948 Author: Jonathan Wakely Date: Mon Jan 28 21:35:33 2019 +0000 PR libstdc++/89090 avoid C++17 features in C++11/C++14 code Although GCC and Clang both allow these features pre-C++17 in system headers, Clang does issue warnings with -Wsystem-headers. It can also complicate bisection and/or testcase reduction if # line markers are stripped, because the code won't be known to come from system headers. PR libstdc++/89090 * include/bits/stl_uninitialized.h (__relocate_a_1): Make unused parameter unnamed. Add message to static assertion. * include/bits/vector.tcc (vector::reserve, vector::_M_realloc_insert) (vector::_M_default_append): Use _GLIBCXX17_CONSTEXPR for if constexpr in C++11 code. diff --git a/libstdc++-v3/include/bits/stl_uninitialized.h b/libstdc++-v3/include/bits/stl_uninitialized.h index 03ed16b8c1a..0d42b253df1 100644 --- a/libstdc++-v3/include/bits/stl_uninitialized.h +++ b/libstdc++-v3/include/bits/stl_uninitialized.h @@ -904,7 +904,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION template inline __enable_if_t::value, _Tp*> __relocate_a_1(_Tp* __first, _Tp* __last, - _Tp* __result, allocator<_Up>& __alloc) noexcept + _Tp* __result, allocator<_Up>&) noexcept { ptrdiff_t __count = __last - __first; if (__count > 0) @@ -925,7 +925,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _ValueType; typedef typename iterator_traits<_ForwardIterator>::value_type _ValueType2; - static_assert(std::is_same<_ValueType, _ValueType2>::value); + static_assert(std::is_same<_ValueType, _ValueType2>::value, + "relocation is only possible for values of the same type"); _ForwardIterator __cur = __result; for (; __first != __last; ++__first, (void)++__cur) std::__relocate_object_a(std::__addressof(*__cur), diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc index 4cf0e809fe9..54c09774b15 100644 --- a/libstdc++-v3/include/bits/vector.tcc +++ b/libstdc++-v3/include/bits/vector.tcc @@ -73,7 +73,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER const size_type __old_size = size(); pointer __tmp; #if __cplusplus >= 201103L - if constexpr (_S_use_relocate()) + if _GLIBCXX17_CONSTEXPR (_S_use_relocate()) { __tmp = this->_M_allocate(__n); std::__relocate_a(this->_M_impl._M_start, @@ -457,7 +457,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER __new_finish = pointer(); #if __cplusplus >= 201103L - if constexpr (_S_use_relocate()) + if _GLIBCXX17_CONSTEXPR (_S_use_relocate()) { __new_finish = std::__relocate_a @@ -498,7 +498,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER __throw_exception_again; } #if __cplusplus >= 201103L - if constexpr (!_S_use_relocate()) + if _GLIBCXX17_CONSTEXPR (!_S_use_relocate()) #endif std::_Destroy(__old_start, __old_finish, _M_get_Tp_allocator()); _GLIBCXX_ASAN_ANNOTATE_REINIT; @@ -638,8 +638,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER const size_type __len = _M_check_len(__n, "vector::_M_default_append"); pointer __new_start(this->_M_allocate(__len)); -#if __cplusplus >= 201103L - if constexpr (_S_use_relocate()) + if _GLIBCXX17_CONSTEXPR (_S_use_relocate()) { __try { @@ -656,7 +655,6 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER __new_start, _M_get_Tp_allocator()); } else -#endif { pointer __destroy_from = pointer(); __try