From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oo1-xc29.google.com (mail-oo1-xc29.google.com [IPv6:2607:f8b0:4864:20::c29]) by sourceware.org (Postfix) with ESMTPS id 12B2B3858D38; Thu, 17 Aug 2023 07:43:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 12B2B3858D38 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-oo1-xc29.google.com with SMTP id 006d021491bc7-56e691dc655so649784eaf.0; Thu, 17 Aug 2023 00:43:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1692258219; x=1692863019; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=NYIC3aPJooGaSVPkovS0WMaEbp/rNYi9hxdOWix2gpk=; b=BpvekIyiCI2vegBQK7valsaZK6W743FXp9sKsmy5p67pJlD7kOVuHg6OJZsGO5Qs07 zpZjnAm2a1Y4R9XvVJmgcavHLIug0ru8fZXaF1TnMx0zibYRg2/8tKIBDXDlxMkcleFl L1qs3DkGVy+7MAsP1yED5i0CLPWlnp9Mjbd1Myngk1yhk/V6OB7+pDCJDDMNvLpADXs3 8mSydujsfVaMQQkwRJFz0MxBsBeAbO7ndDcvQTX9Sc1TpGZpGTdK2odjbp6GCPfanUPO /hz7SJHxpJWcSa0//SoCZVB4F0Z2o+gjQYbODldJllcKjSsynE47b9RWY380JLtvORMv XLwQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1692258219; x=1692863019; 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=NYIC3aPJooGaSVPkovS0WMaEbp/rNYi9hxdOWix2gpk=; b=W5xYVz/zlo3CtteOmL7YR4UyNbk0ZKOtuVOkrF4NmdjPfNMhgoSzbw1wyBNclghJfR WgT7Ektncmn5EXGZeSuSONPamL/uEz6U4ufSV/3lEJ8eZm+FiatvbL/5jm445vzH60zm qAEqmRd5yu9NLCGeg53nL6MQpfh7W2y5AqGf3nKuElTFW8xckk6twQbveXJZJdazLaNb JgSm4k4dZHwA4u/CA8NLpMKl8Ra/gtAFThcm1uHPxWeBd66+oop4Q1vA4Z+m6nHw3Wk9 7v2zmDQDs6/8oS+bOcf8eZEAtOQcerE43uLYDxmbugEwuCDedc8c1iPYrGjxhVZULQYB ksPg== X-Gm-Message-State: AOJu0YwRDeUvdR57Ze85qQCad3JMNR2PWECH8e6VXanmVPw6tUam5469 0/efivVt5FLU69pwf8s8zwAOaLAazhvLnkoihQ== X-Google-Smtp-Source: AGHT+IFMv8GLmIofJkBBYY/C+4/pJopRSuPunbHhZ0NUI14CslZDZKlXIwGV5Ok/77LDZuwITfu684Gn/a6n8/kARZg= X-Received: by 2002:a4a:625b:0:b0:569:ac56:ca98 with SMTP id y27-20020a4a625b000000b00569ac56ca98mr4452854oog.3.1692258219007; Thu, 17 Aug 2023 00:43:39 -0700 (PDT) MIME-Version: 1.0 References: <20230808223405.35178-1-palevichva@gmail.com> In-Reply-To: From: Vladimir Palevich Date: Thu, 17 Aug 2023 10:43:27 +0300 Message-ID: Subject: Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879] To: Jonathan Wakely Cc: gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-7.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,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 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. > > >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 > > >