From patchwork Tue Aug 8 22:34:05 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Vladimir Palevich X-Patchwork-Id: 132927 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:c44e:0:b0:3f2:4152:657d with SMTP id w14csp2434880vqr; Tue, 8 Aug 2023 15:36:33 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEDK+X/3STeSkA216lIkjoIKJ4C3fGOK+de5yihMklstiMU1CGi12URAQrL/5arSg/JVVrF X-Received: by 2002:aa7:d745:0:b0:523:1eab:c47 with SMTP id a5-20020aa7d745000000b005231eab0c47mr820151eds.34.1691534193239; Tue, 08 Aug 2023 15:36:33 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1691534193; cv=none; d=google.com; s=arc-20160816; b=YF4F0b2J35lKyHtNHV3J2g6NGnK2JJymD/Y+Mt8OVq3ooz7mBRHXB2eIyp3EoR+AAu WfkM5ApFRuxpXPBjSFDLu8gYFvNqgko+snP+LqSey0lq9FhJzcFscu+XG7eMFiXb2E4y GNUGhVxn4VNW5dnTCPeVd0q+Uebn6x7JmEU2Ac8Zk5Nnm9g28Az0WuR+utZ/qI06gDaA OGsj04JKxren+JFRbDwZ1Y6HZaNZBHX7PnTe//jIhrsu8xMyWxANl2c++jIhdZ8CNcHA e60o6KDL+2I3UoCKxxJxJ5xFd7J9xBD4ILefutTgYY40BRA+hclBN2tUMYFYQ/QecZ5l zQsw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:reply-to:from:list-subscribe:list-help:list-post :list-archive:list-unsubscribe:list-id:precedence :content-transfer-encoding:mime-version:message-id:date:subject:cc :to:dmarc-filter:delivered-to:dkim-signature:dkim-filter; bh=mw6MimRgM9xGJIoL0Hpg8/LmwzzvMhnsP9GdvL2dxDw=; fh=SO9AlxjnH9YU6EOWmQrE3k1hU64lpEug4cCaD4AfiEk=; b=W2XSE/4UNmCIPffCLFgzvmiMIX4MfWYKJxPJX8tASfvBFrDn5DNH5I6KyPI5XTpRtm e7WEVlE2CpCak5Sxf6zBtXkzf6156BqmupAymFdn/cUVImo58bXD+JofkF0VA9jKekfs 9B46gl2cDB/MA9ZDOihkMSw3HyhTUFAEPGSWLAmg45Ss/SZwUapZ9e9R+L4Uv6rk7ZFd e8j39YP21HGp3wbhUo00ChNZs6pizNyHaXyMQUXzmfaVAYlGWdCGK/Rl0gSEzDmi7h8t bTH01BOkGKFtg7nTd0Q3wm/I4vFrWG82l2CnR0ISkVTKG+w5Q6SP3WSMr9eN7WMZG/tF nkXg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=Vkk7lERw; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 8.43.85.97 as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from server2.sourceware.org (ip-8-43-85-97.sourceware.org. [8.43.85.97]) by mx.google.com with ESMTPS id f18-20020a056402151200b005222c51fac1si8243611edw.659.2023.08.08.15.36.32 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 08 Aug 2023 15:36:33 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 8.43.85.97 as permitted sender) client-ip=8.43.85.97; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=Vkk7lERw; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 8.43.85.97 as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 16ED33858288 for ; Tue, 8 Aug 2023 22:36:32 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 16ED33858288 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1691534192; bh=mw6MimRgM9xGJIoL0Hpg8/LmwzzvMhnsP9GdvL2dxDw=; h=To:Cc:Subject:Date:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=Vkk7lERw/nAQdd2iq1OfOo0xhqr+/dz7gfNlXWbV/xAPeUm+BUWUIlDjrC0/SXr/M JmEQV3AmXIj9DRDNPdbq/ohh+eVcl+RjnloGJ58vrDy1DjwQIdH0REoM6dfihPh3nx XBeIuM59v/qhKJ6UE5GCAsE3f1zX/8aH2P35FZZ8= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com [IPv6:2a00:1450:4864:20::52e]) by sourceware.org (Postfix) with ESMTPS id E16593858D20 for ; Tue, 8 Aug 2023 22:35:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E16593858D20 Received: by mail-ed1-x52e.google.com with SMTP id 4fb4d7f45d1cf-522dd6b6438so7808393a12.0 for ; Tue, 08 Aug 2023 15:35:44 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1691534142; x=1692138942; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=mw6MimRgM9xGJIoL0Hpg8/LmwzzvMhnsP9GdvL2dxDw=; b=YtI38thBZiwdYnwfmZ99rQReRpQx/y6Hc/bNv7bZ0F3zybldtcKuo2C2MZh15PSvYy qvax372PQ5JeUmyaQ1WYDIGu9vuWGSRUbuARyT3eSG3opGhzg9ZI2pzeA+aLZIpNpUuA Rewnki8LZIyQNIoIhxylGD0XPqchQyKuHjJcmri9vCI8hRwAT/9n5JFPdCxaJKU0FeGs e0GPKWbrE7/49ScHVPHwH8iIuTILSEh5qmMRwyYyJz/q+eq4hOO1xmPs3P3K7wZz2Ycg KmxdcrNiPRfUjDB1Bc2fmZ+tfEFmeWmw6vcIw62CcAZMe+YsrcqlwLvJiXsWcrwF79xM En3Q== X-Gm-Message-State: AOJu0YxtdDgVxhAyOWjjGGttcF5oUOsthkeW28253MSA7bFG2EpYEoQp eS4TYEbR7D61qwMfSV56+uhTypJlRw== X-Received: by 2002:a17:906:3297:b0:966:17b2:5b0b with SMTP id 23-20020a170906329700b0096617b25b0bmr663564ejw.49.1691534141803; Tue, 08 Aug 2023 15:35:41 -0700 (PDT) Received: from localhost.localdomain ([176.232.59.112]) by smtp.gmail.com with ESMTPSA id o6-20020a17090611c600b0098d486d2bdfsm7289208eja.177.2023.08.08.15.35.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 08 Aug 2023 15:35:41 -0700 (PDT) To: gcc-patches@gcc.gnu.org Cc: Vladimir Palevich Subject: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879] Date: Wed, 9 Aug 2023 01:34:05 +0300 Message-Id: <20230808223405.35178-1-palevichva@gmail.com> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 X-Spam-Status: No, score=-10.2 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 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Vladimir Palevich via Gcc-patches From: Vladimir Palevich Reply-To: Vladimir Palevich Errors-To: gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org Sender: "Gcc-patches" X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1773702158317016491 X-GMAIL-MSGID: 1773702158317016491 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. +// +// 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" } } 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