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.129.124]) by sourceware.org (Postfix) with ESMTPS id 0D5C43858414 for ; Fri, 1 Sep 2023 15:13:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0D5C43858414 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=1693581180; 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=tXkddcDgHVRbRNyQUSpwenG+LDvE6xNc/n4+wv5cCAY=; b=cient3ti3wwszGGHGIvfCRMf0emXcFaEqHEiRmIxbu7SyiBdgl43F2DRAX7WLSCjOA6W6n g7TUAMGPHrJRacRFoidjOTbOTRhilhSMSl6pFh6tmkYXxwwvOlBgn4JECD0Pi+4ELHNV2S FFJKzivA2Xd6utcjn35yjudXMixX/HM= Received: from mail-lj1-f199.google.com (mail-lj1-f199.google.com [209.85.208.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-631-55c05VA5MteFwWxRxrWwSw-1; Fri, 01 Sep 2023 11:12:59 -0400 X-MC-Unique: 55c05VA5MteFwWxRxrWwSw-1 Received: by mail-lj1-f199.google.com with SMTP id 38308e7fff4ca-2bcc5098038so26775851fa.2 for ; Fri, 01 Sep 2023 08:12:58 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693581177; x=1694185977; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=tXkddcDgHVRbRNyQUSpwenG+LDvE6xNc/n4+wv5cCAY=; b=I4djjwhxUwKXB+wgYX0VP7sp5cn0hTVMOwLUETCPt4N3uzpcsvZicwrsBdq14ap73F 94FPyhqDZF/DEEfzx5EKB3wfsxXDKUN2NOhUF0DRZKYWO5XDwVRdL8AERYTBuDnPykfu +Fd1Bkm97m4dCReQclG0nL76cpcUzzTSx8/V6jYgpuyz+m3ReFDvIFWrDMofX8PgZ3ry MbfDTNY5m0hQ+UDY6xzbfq5/WehkpdJT5mOI1j+7c+6wHgacx9DMSlkZ0okWiPRJm9cP TPp/AGWDyKa3AGv3OngN4awBpuQpXLhT+Ai+yoUD4qmRV75CKbmAJPH7rGoCqMATrLHX DoUA== X-Gm-Message-State: AOJu0YzcEFqO/MrUFUhR2k14i1SFXtXAMvD++fpuElPE4oTXlxxA01SX 4htfG0ddEXkxMlwQj7EiYG2Mk1CKE3A+NP/6AUZv5M14Y/zsvcjS6Q/q/RPYiKKvR0PK9ZYYdj1 PUTN7HZQRz9vsLMWq2uW+j9dP4Lmwcpg= X-Received: by 2002:a2e:998e:0:b0:2b4:5cad:f246 with SMTP id w14-20020a2e998e000000b002b45cadf246mr2021708lji.7.1693581177702; Fri, 01 Sep 2023 08:12:57 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHtKwdyi5vaYaXmL8Sf+7iTGuujPTq5L92au7/m2m9R2va3c8ppjMX8teMYigGvGBaCq3W+yaqX8q9KSDTmgk8= X-Received: by 2002:a2e:998e:0:b0:2b4:5cad:f246 with SMTP id w14-20020a2e998e000000b002b45cadf246mr2021687lji.7.1693581177344; Fri, 01 Sep 2023 08:12:57 -0700 (PDT) MIME-Version: 1.0 References: <20230808223405.35178-1-palevichva@gmail.com> In-Reply-To: From: Jonathan Wakely Date: Fri, 1 Sep 2023 16:12:46 +0100 Message-ID: Subject: Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879] To: Vladimir Palevich Cc: gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-12.0 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=ham 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 Thu, 17 Aug 2023 at 08:43, Vladimir Palevich wrote: > > On Thu, 17 Aug 2023 at 01:51, Jonathan Wakely wrote: > > > > 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. > > I've just copypasted this boilerplate from other test without thinking. > You are correct, I don't have a FSF copyright assignment. I'm contributing > under the terms of the DCO. > You can just remove this boilerplate. > > > > > > > >+// 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. > > Sure, you can move this test to a more appropriate place. OK thanks - I've pushed it to trunk now. Thanks again for the analysis of the problem and the patch to fix it! > > > > > >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 > > > > > >