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 01DE33857C4F for ; Fri, 24 May 2024 14:18:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 01DE33857C4F Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 01DE33857C4F Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1716560293; cv=none; b=qHP7xD0ijRj1jACaTBRQxf9teCexhRGeS61EX7o1e9nld9BL7uqugcQJy4PGENoeg5mfQogcAuCxqFe7HUaCtjLyaJa3YOOrkej+fCC74oHdNp71Nd8SH1LB4JZ2uAF3+9cop7ik6kp4zkseBGTLemDSQY21GakO0pYHdCzw2wI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1716560293; c=relaxed/simple; bh=+6FHDaxTORr5UY/ahUbOXxyHqZ3IZpxFZeimkx7f2GI=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=Df1+aKEgaRDDGkgzFRoZejJYVYfhHRad+aq+kMhq1M0pHe0mAmvbQgpkvJ/6zDkJbX+LIOnk+hXU9qPTxJdTBDmd7qSicmEDoXmlyObEXFzsgf7SMyVZfwguKnIWa1aVADE0ii2D2u6DyuaKF9EuZ5IAjPr7isz2G9aVLiEPfHg= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1716560289; 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: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=syQsyxPpOxUebuQaxCD9LsNcl3Z7SY7s4o7PFNsh30I=; b=DCByMsBpnU2BGp3NN7czU+E8BPmOus1B/y6oV/lPAUOLpQEZjH4GzicBMx/APOuOlVtWM3 ctU88/CPB5nHJMoDNjslJgNCI9VuFC+cTXFDRPdUwbmFoxqHwn8tg/hvpe47jOgq1/JnUA tw93x28csT7B2Vy6lCPktRnv3Tg8AhE= Received: from mail-yb1-f197.google.com (mail-yb1-f197.google.com [209.85.219.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-564-GD1LCI9TOXa3spXtj3fbNQ-1; Fri, 24 May 2024 10:18:08 -0400 X-MC-Unique: GD1LCI9TOXa3spXtj3fbNQ-1 Received: by mail-yb1-f197.google.com with SMTP id 3f1490d57ef6-df52a2535d5so527566276.0 for ; Fri, 24 May 2024 07:18:08 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1716560288; x=1717165088; h=content-transfer-encoding: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=syQsyxPpOxUebuQaxCD9LsNcl3Z7SY7s4o7PFNsh30I=; b=ugUDC9avPtvzGXitWVGOKGboZP3+Y4dIlm3bUHz8AzrhXbpGFMwckdnZ0KS2D9I/R7 fzO8ogxbGGio4XyjfZh3NORtT71LZJam2qpjTNeIxFHRlMa/2NEqkoHGfLnqxrvtkHFJ fjPdoka5E/dYc2K2STPwf4b9a2eljJix8IJmFpFiMkEP8jomOBjyTPXAO56TlOXWPnM3 UvUrNBgA1mQI7FmikN8jCdk0Q/+gAvhWzu9amIpwLRnlAnaQRiNzBjeRz/Dpa++cGxz5 yS10FHISWwlYhks54FoH9DEO1uxcdeEruPBDsVtbvyEa8LJuEXcZOGYd8LrlnbHJskT0 mu/g== X-Gm-Message-State: AOJu0Yx5iv7hkQUqo5siXvucdtbWDgN/154r+6HGztV1YaluIekj5aWt 4ljvY/O4mxEHndUhiVcTLfrtW88ZYa1EuBnw6NtYzyiddKXmNvWbbXqWM3rp0Kz5BPYGZ4RC2m/ n+3rn7qBwNDH0gQd124ae+4YQ+sTIFQqVyz/D/raPHQY63IsTfjVHsiBLGKOGgYbknsgnWs0G4V sSgfxHvTjJSZdSTMbxR49LJ+mZKvReCmAXD2k= X-Received: by 2002:a25:848d:0:b0:dee:5d31:b95f with SMTP id 3f1490d57ef6-df77218a0c4mr2229844276.11.1716560286462; Fri, 24 May 2024 07:18:06 -0700 (PDT) X-Google-Smtp-Source: AGHT+IElxJtgJwXD+2uTtCPvaZ70+GK9NTRRwIMyRx/lVujasX3vUcmlm3sueLjbBIn2nwqvNcXKiW9fe6UGxW+V66E= X-Received: by 2002:a25:848d:0:b0:dee:5d31:b95f with SMTP id 3f1490d57ef6-df77218a0c4mr2229818276.11.1716560285894; Fri, 24 May 2024 07:18:05 -0700 (PDT) MIME-Version: 1.0 References: <305e3002-4ebb-4f07-91f3-10a29ec7fc70@gmail.com> In-Reply-To: From: Jonathan Wakely Date: Fri, 24 May 2024 15:17:50 +0100 Message-ID: Subject: Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings To: =?UTF-8?Q?Fran=C3=A7ois_Dumont?= Cc: "libstdc++" , gcc-patches X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-12.3 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 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 Thu, 23 May 2024 at 18:38, Fran=C3=A7ois Dumont w= rote: > > > On 23/05/2024 15:31, Jonathan Wakely wrote: > > On 23/05/24 06:55 +0200, Fran=C3=A7ois Dumont wrote: > >> As explained in this email: > >> > >> https://gcc.gnu.org/pipermail/libstdc++/2024-April/058552.html > >> > >> I experimented -Wfree-nonheap-object because of my enhancements on > >> algos. > >> > >> So here is a patch to extend the usage of the _Guard type to other > >> parts of vector. > > > > Nice, that fixes the warning you were seeing? > > Yes ! I indeed forgot to say so :-) > > > > > > We recently got a bug report about -Wfree-nonheap-object in > > std::vector, but that is coming from _M_realloc_append which already > > uses the RAII guard :-( > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D115016 > > Note that I also had to move call to __uninitialized_copy_a before > assigning this->_M_impl._M_start so get rid of the -Wfree-nonheap-object > warn. But _M_realloc_append is already doing potentially throwing > operations before assigning this->_M_impl so it must be something else. > > Though it made me notice another occurence of _Guard in this method. Now > replaced too in this new patch. > > libstdc++: Use RAII to replace try/catch blocks > > Move _Guard into std::vector declaration and use it to guard all > calls to > vector _M_allocate. > > Doing so the compiler has more visibility on what is done with the > pointers > and do not raise anymore the -Wfree-nonheap-object warning. > > libstdc++-v3/ChangeLog: > > * include/bits/vector.tcc (_Guard): Move all the nested > duplicated class... > * include/bits/stl_vector.h (_Guard_alloc): ...here. > (_M_allocate_and_copy): Use latter. > (_M_initialize_dispatch): Likewise and set _M_finish first > from the result > of __uninitialize_fill_n_a that can throw. > (_M_range_initialize): Likewise. > > >> diff --git a/libstdc++-v3/include/bits/stl_vector.h > >> b/libstdc++-v3/include/bits/stl_vector.h > >> index 31169711a48..4ea74e3339a 100644 > >> --- a/libstdc++-v3/include/bits/stl_vector.h > >> +++ b/libstdc++-v3/include/bits/stl_vector.h > >> @@ -1607,6 +1607,39 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER > >> clear() _GLIBCXX_NOEXCEPT > >> { _M_erase_at_end(this->_M_impl._M_start); } > >> > >> + private: > >> + // RAII guard for allocated storage. > >> + struct _Guard > > > > If it's being defined at class scope instead of locally in a member > > function, I think a better name would be good. Maybe _Ptr_guard or > > _Dealloc_guard or something. > _Guard_alloc chosen. > > > >> + { > >> + pointer _M_storage; // Storage to deallocate > >> + size_type _M_len; > >> + _Base& _M_vect; > >> + > >> + _GLIBCXX20_CONSTEXPR > >> + _Guard(pointer __s, size_type __l, _Base& __vect) > >> + : _M_storage(__s), _M_len(__l), _M_vect(__vect) > >> + { } > >> + > >> + _GLIBCXX20_CONSTEXPR > >> + ~_Guard() > >> + { > >> + if (_M_storage) > >> + _M_vect._M_deallocate(_M_storage, _M_len); > >> + } > >> + > >> + _GLIBCXX20_CONSTEXPR > >> + pointer > >> + _M_release() > >> + { > >> + pointer __res =3D _M_storage; > >> + _M_storage =3D 0; > > > > I don't think the NullablePointer requirements include assigning 0, > > only from nullptr, which isn't valid in C++98. > > > > https://en.cppreference.com/w/cpp/named_req/NullablePointer > > > > Please use _M_storage =3D pointer() instead. > > I forgot about user fancy pointer, fixed. > > > > > >> + return __res; > >> + } > >> + > >> + private: > >> + _Guard(const _Guard&); > >> + }; > >> + > >> protected: > >> /** > >> * Memory expansion handler. Uses the member allocation > >> function to > >> @@ -1618,18 +1651,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER > >> _M_allocate_and_copy(size_type __n, > >> _ForwardIterator __first, _ForwardIterator __last) > >> { > >> - pointer __result =3D this->_M_allocate(__n); > >> - __try > >> - { > >> - std::__uninitialized_copy_a(__first, __last, __result, > >> - _M_get_Tp_allocator()); > >> - return __result; > >> - } > >> - __catch(...) > >> - { > >> - _M_deallocate(__result, __n); > >> - __throw_exception_again; > >> - } > >> + _Guard __guard(this->_M_allocate(__n), __n, *this); > >> + std::__uninitialized_copy_a > >> + (__first, __last, __guard._M_storage, _M_get_Tp_allocator()); > >> + return __guard._M_release(); > >> } > >> > >> > >> @@ -1642,13 +1667,15 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER > >> // 438. Ambiguity in the "do the right thing" clause > >> template > >> void > >> - _M_initialize_dispatch(_Integer __n, _Integer __value, __true_typ= e) > >> + _M_initialize_dispatch(_Integer __int_n, _Integer __value, > >> __true_type) > >> { > >> - this->_M_impl._M_start =3D _M_allocate(_S_check_init_len( > >> - static_cast(__n), _M_get_Tp_allocator())); > >> - this->_M_impl._M_end_of_storage =3D > >> - this->_M_impl._M_start + static_cast(__n); > >> - _M_fill_initialize(static_cast(__n), __value); > > > > Please fix the comment on _M_fill_initialize if you're removing the > > use of it here. > > Already done in this initial patch proposal, see below. > > > > >> + const size_type __n =3D static_cast(__int_n); > >> + _Guard __guard(_M_allocate(_S_check_init_len( > >> + __n, _M_get_Tp_allocator())), __n, *this); > > > > I think this would be easier to read if the _S_check_init_len call was > > done first, and maybe the allocation too, since we are going to need a > > local __start later anyway. So maybe like this: > > > > template > > void > > _M_initialize_dispatch(_Integer __ni, _Integer __value, __true_type= ) > > { > > const size_type __n =3D static_cast(__ni); > > pointer __start =3D _M_allocate(_S_check_init_len(__n), > > _M_get_Tp_allocator()); > > _Guard __guard(__start, __n, *this); > > this->_M_impl._M_start =3D __start; > > _M_fill_initialize(__n, __value); > > this->_M_impl._M_end_of_storage =3D __start + __n; > > (void) __guard._M_release(); > > } > > > > Or inline the __uninitialized_fill_n_a call if you want to (but then > > fix the comment on _M_fill_initialize). Inlining it does make this > > function more consistent with the next one, which calls > > __uninitialized_copy_a directly. > > Yes, this is why I called __uninitialized_fill_n_a instead and also to > do so *before* assigning _M_impl._M_start. > > > >> - // Called by the first initialize_dispatch above and by the > >> - // vector(n,value,a) constructor. > >> + // Called by the vector(n,value,a) constructor. > > See, it's here :-) Doh! Sorry, I'm not sure how I missed that. > > Ok to commit ? OK for trunk, thanks!