From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 9CD7C3858C2F for ; Wed, 16 Aug 2023 22:51:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9CD7C3858C2F Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1692226318; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=sl/v9W3j731oQgzkgm0WlXprDHqimemfrEBKcCI/6l8=; b=Cnck7mbM6jOsww9/XUbmdF3cxzxbETA2Mw7Pebb/5BVVPCu4SFmZ3+wH8eYarDW6MMg2Le lcuZ7PVAD1tlqYM1/J+gEeTR7zvhLPS0w6lalKrGsl1rvNczv1wQNemPaCoGAQADkeEcBE O8SrEE66eufmIPsrh+1+l5v6n016LWw= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-564-4Gs8oPlVPiucu-RSe_XLpQ-1; Wed, 16 Aug 2023 18:51:55 -0400 X-MC-Unique: 4Gs8oPlVPiucu-RSe_XLpQ-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 0A0EF8DC660; Wed, 16 Aug 2023 22:51:55 +0000 (UTC) Received: from localhost (unknown [10.42.28.201]) by smtp.corp.redhat.com (Postfix) with ESMTP id ADF28C15BAD; Wed, 16 Aug 2023 22:51:54 +0000 (UTC) Date: Wed, 16 Aug 2023 23:51:54 +0100 From: Jonathan Wakely To: Vladimir Palevich Cc: gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org Subject: Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879] Message-ID: References: <20230808223405.35178-1-palevichva@gmail.com> MIME-Version: 1.0 In-Reply-To: <20230808223405.35178-1-palevichva@gmail.com> X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 3.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,URI_HEX autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 09/08/23 01:34 +0300, Vladimir Palevich wrote: >Because of the recent change in _M_realloc_insert and _M_default_append, call >to deallocate was ordered after assignment to class members of std::vector >(in the guard destructor), which is causing said members to be call-clobbered. >This is preventing further optimization, the compiler is unable to move memory >read out of a hot loop in this case. >This patch reorders the call to before assignments by putting guard in its own >block. Plus a new testsuite for this case. >I'm not very happy with the new testsuite, but I don't know how to properly >test this. > >Tested on x86_64-pc-linux-gnu. > >Maybe something could be done so that the compiler would be able to optimize >such cases anyway. Reads could be moved just after the clobbering calls in >unlikely branches, for example. This should be a fairly common case with >destructors at the end of a function. > >Note: I don't have write access. > >-- >8 -- > >Fix ordering to prevent clobbering of class members by a call to deallocate >in _M_realloc_insert and _M_default_append. > >libstdc++-v3/ChangeLog: > PR libstdc++/110879 > * include/bits/vector.tcc: End guard lifetime just before assignment to > class members. > * testsuite/libstdc++-dg/conformance.exp: Load scantree.exp. > * testsuite/23_containers/vector/110879.cc: New test. > >Signed-off-by: Vladimir Palevich >--- > libstdc++-v3/include/bits/vector.tcc | 220 +++++++++--------- > .../testsuite/23_containers/vector/110879.cc | 35 +++ > .../testsuite/libstdc++-dg/conformance.exp | 13 ++ > 3 files changed, 163 insertions(+), 105 deletions(-) > create mode 100644 libstdc++-v3/testsuite/23_containers/vector/110879.cc > >diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc >index ada396c9b30..80631d1e2a1 100644 >--- a/libstdc++-v3/include/bits/vector.tcc >+++ b/libstdc++-v3/include/bits/vector.tcc >@@ -488,78 +488,83 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER > private: > _Guard(const _Guard&); > }; >- _Guard __guard(__new_start, __len, _M_impl); > >- // The order of the three operations is dictated by the C++11 >- // case, where the moves could alter a new element belonging >- // to the existing vector. This is an issue only for callers >- // taking the element by lvalue ref (see last bullet of C++11 >- // [res.on.arguments]). >+ { >+ _Guard __guard(__new_start, __len, _M_impl); > >- // If this throws, the existing elements are unchanged. >+ // The order of the three operations is dictated by the C++11 >+ // case, where the moves could alter a new element belonging >+ // to the existing vector. This is an issue only for callers >+ // taking the element by lvalue ref (see last bullet of C++11 >+ // [res.on.arguments]). >+ >+ // If this throws, the existing elements are unchanged. > #if __cplusplus >= 201103L >- _Alloc_traits::construct(this->_M_impl, >- std::__to_address(__new_start + __elems_before), >- std::forward<_Args>(__args)...); >+ _Alloc_traits::construct(this->_M_impl, >+ std::__to_address(__new_start + __elems_before), >+ std::forward<_Args>(__args)...); > #else >- _Alloc_traits::construct(this->_M_impl, >- __new_start + __elems_before, >- __x); >+ _Alloc_traits::construct(this->_M_impl, >+ __new_start + __elems_before, >+ __x); > #endif > > #if __cplusplus >= 201103L >- if _GLIBCXX17_CONSTEXPR (_S_use_relocate()) >- { >- // Relocation cannot throw. >- __new_finish = _S_relocate(__old_start, __position.base(), >- __new_start, _M_get_Tp_allocator()); >- ++__new_finish; >- __new_finish = _S_relocate(__position.base(), __old_finish, >- __new_finish, _M_get_Tp_allocator()); >- } >- else >+ if _GLIBCXX17_CONSTEXPR (_S_use_relocate()) >+ { >+ // Relocation cannot throw. >+ __new_finish = _S_relocate(__old_start, __position.base(), >+ __new_start, _M_get_Tp_allocator()); >+ ++__new_finish; >+ __new_finish = _S_relocate(__position.base(), __old_finish, >+ __new_finish, _M_get_Tp_allocator()); >+ } >+ else > #endif >- { >- // RAII type to destroy initialized elements. >- struct _Guard_elts > { >- pointer _M_first, _M_last; // Elements to destroy >- _Tp_alloc_type& _M_alloc; >- >- _GLIBCXX20_CONSTEXPR >- _Guard_elts(pointer __elt, _Tp_alloc_type& __a) >- : _M_first(__elt), _M_last(__elt + 1), _M_alloc(__a) >- { } >- >- _GLIBCXX20_CONSTEXPR >- ~_Guard_elts() >- { std::_Destroy(_M_first, _M_last, _M_alloc); } >- >- private: >- _Guard_elts(const _Guard_elts&); >- }; >- >- // Guard the new element so it will be destroyed if anything throws. >- _Guard_elts __guard_elts(__new_start + __elems_before, _M_impl); >- >- __new_finish = std::__uninitialized_move_if_noexcept_a( >- __old_start, __position.base(), >- __new_start, _M_get_Tp_allocator()); >- >- ++__new_finish; >- // Guard everything before the new element too. >- __guard_elts._M_first = __new_start; >- >- __new_finish = std::__uninitialized_move_if_noexcept_a( >- __position.base(), __old_finish, >- __new_finish, _M_get_Tp_allocator()); >- >- // New storage has been fully initialized, destroy the old elements. >- __guard_elts._M_first = __old_start; >- __guard_elts._M_last = __old_finish; >- } >- __guard._M_storage = __old_start; >- __guard._M_len = this->_M_impl._M_end_of_storage - __old_start; >+ // RAII type to destroy initialized elements. >+ struct _Guard_elts >+ { >+ pointer _M_first, _M_last; // Elements to destroy >+ _Tp_alloc_type& _M_alloc; >+ >+ _GLIBCXX20_CONSTEXPR >+ _Guard_elts(pointer __elt, _Tp_alloc_type& __a) >+ : _M_first(__elt), _M_last(__elt + 1), _M_alloc(__a) >+ { } >+ >+ _GLIBCXX20_CONSTEXPR >+ ~_Guard_elts() >+ { std::_Destroy(_M_first, _M_last, _M_alloc); } >+ >+ private: >+ _Guard_elts(const _Guard_elts&); >+ }; >+ >+ // Guard the new element so it will be destroyed if anything throws. >+ _Guard_elts __guard_elts(__new_start + __elems_before, _M_impl); >+ >+ __new_finish = std::__uninitialized_move_if_noexcept_a( >+ __old_start, __position.base(), >+ __new_start, _M_get_Tp_allocator()); >+ >+ ++__new_finish; >+ // Guard everything before the new element too. >+ __guard_elts._M_first = __new_start; >+ >+ __new_finish = std::__uninitialized_move_if_noexcept_a( >+ __position.base(), __old_finish, >+ __new_finish, _M_get_Tp_allocator()); >+ >+ // New storage has been fully initialized, destroy the old elements. >+ __guard_elts._M_first = __old_start; >+ __guard_elts._M_last = __old_finish; >+ } >+ __guard._M_storage = __old_start; >+ __guard._M_len = this->_M_impl._M_end_of_storage - __old_start; >+ } >+ // deallocate should be called before assignments to _M_impl, >+ // to avoid call-clobbering > > this->_M_impl._M_start = __new_start; > this->_M_impl._M_finish = __new_finish; >@@ -728,49 +733,54 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER > private: > _Guard(const _Guard&); > }; >- _Guard __guard(__new_start, __len, _M_impl); >- >- std::__uninitialized_default_n_a(__new_start + __size, __n, >- _M_get_Tp_allocator()); >- >- if _GLIBCXX17_CONSTEXPR (_S_use_relocate()) >- { >- _S_relocate(__old_start, __old_finish, >- __new_start, _M_get_Tp_allocator()); >- } >- else >- { >- // RAII type to destroy initialized elements. >- struct _Guard_elts >+ >+ { >+ _Guard __guard(__new_start, __len, _M_impl); >+ >+ std::__uninitialized_default_n_a(__new_start + __size, __n, >+ _M_get_Tp_allocator()); >+ >+ if _GLIBCXX17_CONSTEXPR (_S_use_relocate()) > { >- pointer _M_first, _M_last; // Elements to destroy >- _Tp_alloc_type& _M_alloc; >- >- _GLIBCXX20_CONSTEXPR >- _Guard_elts(pointer __first, size_type __n, >- _Tp_alloc_type& __a) >- : _M_first(__first), _M_last(__first + __n), _M_alloc(__a) >- { } >- >- _GLIBCXX20_CONSTEXPR >- ~_Guard_elts() >- { std::_Destroy(_M_first, _M_last, _M_alloc); } >- >- private: >- _Guard_elts(const _Guard_elts&); >- }; >- _Guard_elts __guard_elts(__new_start + __size, __n, _M_impl); >- >- std::__uninitialized_move_if_noexcept_a( >- __old_start, __old_finish, __new_start, >- _M_get_Tp_allocator()); >- >- __guard_elts._M_first = __old_start; >- __guard_elts._M_last = __old_finish; >- } >- _GLIBCXX_ASAN_ANNOTATE_REINIT; >- __guard._M_storage = __old_start; >- __guard._M_len = this->_M_impl._M_end_of_storage - __old_start; >+ _S_relocate(__old_start, __old_finish, >+ __new_start, _M_get_Tp_allocator()); >+ } >+ else >+ { >+ // RAII type to destroy initialized elements. >+ struct _Guard_elts >+ { >+ pointer _M_first, _M_last; // Elements to destroy >+ _Tp_alloc_type& _M_alloc; >+ >+ _GLIBCXX20_CONSTEXPR >+ _Guard_elts(pointer __first, size_type __n, >+ _Tp_alloc_type& __a) >+ : _M_first(__first), _M_last(__first + __n), _M_alloc(__a) >+ { } >+ >+ _GLIBCXX20_CONSTEXPR >+ ~_Guard_elts() >+ { std::_Destroy(_M_first, _M_last, _M_alloc); } >+ >+ private: >+ _Guard_elts(const _Guard_elts&); >+ }; >+ _Guard_elts __guard_elts(__new_start + __size, __n, _M_impl); >+ >+ std::__uninitialized_move_if_noexcept_a( >+ __old_start, __old_finish, __new_start, >+ _M_get_Tp_allocator()); >+ >+ __guard_elts._M_first = __old_start; >+ __guard_elts._M_last = __old_finish; >+ } >+ _GLIBCXX_ASAN_ANNOTATE_REINIT; >+ __guard._M_storage = __old_start; >+ __guard._M_len = this->_M_impl._M_end_of_storage - __old_start; >+ } >+ // deallocate should be called before assignments to _M_impl, >+ // to avoid call-clobbering > > this->_M_impl._M_start = __new_start; > this->_M_impl._M_finish = __new_start + __size + __n; >diff --git a/libstdc++-v3/testsuite/23_containers/vector/110879.cc b/libstdc++-v3/testsuite/23_containers/vector/110879.cc >new file mode 100644 >index 00000000000..d38a5a0d1a3 >--- /dev/null >+++ b/libstdc++-v3/testsuite/23_containers/vector/110879.cc >@@ -0,0 +1,35 @@ >+// -*- C++ -*- >+ >+// Copyright (C) 2023 Free Software Foundation, Inc. Do you actually have a FSF copyright assignment for GCC? If not, then this should not be here, as you can't assign it to the FSF. You've already added a Signed-off-by tag, which I assume means you're contributing under the terms of the DCO: https://gcc.gnu.org/dco.html In which case, this isn't copyright FSF. I've stopped putting the copyright and license header in tests, I don't consider a 5 line function that does nothing interesting to be copyrightable, or worth adding all this boilerplate to. If you don't mind, I'll just remove this boilerplate from the test. >+// 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-do compile } >+// { dg-options "-O3 -fdump-tree-optimized" } >+ >+#include >+ >+std::vector f(std::size_t n) { >+ std::vector res; >+ for (std::size_t i = 0; i < n; ++i) { >+ res.push_back(i); >+ } >+ return res; >+} >+ >+// Reads of _M_finish should be optimized out. >+// This regex matches all reads from res variable except for _M_end_of_storage field. >+// { dg-final { scan-tree-dump-not "=\\s*\\S*res_(?!\\S*_M_end_of_storage;)" "optimized" } } If this was added to gcc/testsuite/g++.dg/ instead of libstdc++-v3/testsuite then we wouldn't need scandump.exp and scantree.exp in the library tests. >diff --git a/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp b/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp >index e8c6504a7f7..1d0588aeadc 100644 >--- a/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp >+++ b/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp >@@ -18,6 +18,19 @@ > > # libstdc++-v3 testsuite that uses the 'dg.exp' driver. > >+# Damn dejagnu for not having proper library search paths for load_lib. >+# We have to explicitly load everything that gcc-dg.exp wants to load. >+ >+proc load_gcc_lib { filename } { >+ global srcdir loaded_libs >+ >+ load_file $srcdir/../../gcc/testsuite/lib/$filename >+ set loaded_libs($filename) "" >+} >+ >+load_gcc_lib scandump.exp >+load_gcc_lib scantree.exp >+ > # Initialization. > dg-init >