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 B422A3858D38 for ; Thu, 23 May 2024 13:31:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B422A3858D38 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 B422A3858D38 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1716471100; cv=none; b=V7reGjg6r8Y3iCMsNlCr9trs1pipnVeKIHXsjaKdh1HFD/RJHbigbdGBf4m7pU1UxtW/rZpBj44rtqw9MkRPdafh5aHelV+wfIfXneQ7MAG5AVzJ/3aHW8lhs7/XB7YM6tI6hpSswIPsXZXkhiQUU5PiLH9r5iZk919LFMIpA7Y= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1716471100; c=relaxed/simple; bh=/CDCKJDbfg9C4WzF5ZhWuxsOLCA6skHFfZ/6uKNWi0A=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=dKX+7XTywinCYeSUZaBGHKb7DLgKdy9wPhhNm+DHJjZ29A03uGCmylEFViyCSr7x9htHwR7r4SDHMAmgO2gF8KgnsCxrRMzXatpfbW3K6J2J2f+loY461w7ICU97ZOXVD0W4S65aXmOVstxuuz/J8l4P/rW8o4Wf0DkXbMMJiZY= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1716471097; 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=PrhWDbY8lpKmRa0Fnig7NTFi71jiq016VXh5rNEvXk4=; b=R9lYi1Dzr2QliXsm9udEeiP4tct5gzJktnZDR/vznrPzt4ZNUH37dHgeC3ly2Dx3IaRWKa /rfuWM3IYhXIbNH+qHNs5ME9vhI5d+OFjX2ThtkcrmHJmUQ40HFDULOu4G7do/pCJsCcXN XQnW3C+jgOeH0xI1gO2F8imz0gUl19g= Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-591-buRQcIYbMxCZnBYjoRDjMw-1; Thu, 23 May 2024 09:31:28 -0400 X-MC-Unique: buRQcIYbMxCZnBYjoRDjMw-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 8BFB21C05149; Thu, 23 May 2024 13:31:26 +0000 (UTC) Received: from localhost (unknown [10.42.28.18]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4088536EC; Thu, 23 May 2024 13:31:26 +0000 (UTC) Date: Thu, 23 May 2024 14:31:25 +0100 From: Jonathan Wakely To: =?iso-8859-1?Q?Fran=E7ois?= Dumont Cc: libstdc++ , gcc-patches Subject: Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings Message-ID: References: <305e3002-4ebb-4f07-91f3-10a29ec7fc70@gmail.com> MIME-Version: 1.0 In-Reply-To: <305e3002-4ebb-4f07-91f3-10a29ec7fc70@gmail.com> X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.6 required=5.0 tests=BAYES_00,BODY_8BITS,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 23/05/24 06:55 +0200, François 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? 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=115016 >    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... >            * include/bits/stl_vector.h: ...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. > >Tested under Linux x86_64, ok to commit ? > >François > >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. >+ { >+ 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 = _M_storage; >+ _M_storage = 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 = pointer() instead. >+ 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 = 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_type) >+ _M_initialize_dispatch(_Integer __int_n, _Integer __value, __true_type) > { >- this->_M_impl._M_start = _M_allocate(_S_check_init_len( >- static_cast(__n), _M_get_Tp_allocator())); >- this->_M_impl._M_end_of_storage = >- 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. >+ const size_type __n = 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 = static_cast(__ni); pointer __start = _M_allocate(_S_check_init_len(__n), _M_get_Tp_allocator()); _Guard __guard(__start, __n, *this); this->_M_impl._M_start = __start; _M_fill_initialize(__n, __value); this->_M_impl._M_end_of_storage = __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. >+ this->_M_impl._M_finish = std::__uninitialized_fill_n_a >+ (__guard._M_storage, __n, __value, _M_get_Tp_allocator()); >+ pointer __start = this->_M_impl._M_start = __guard._M_release(); >+ this->_M_impl._M_end_of_storage = __start + __n; > } > > // Called by the range constructor to implement [23.1.1]/9 >@@ -1690,17 +1717,15 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER > std::forward_iterator_tag) > { > const size_type __n = std::distance(__first, __last); >- this->_M_impl._M_start >- = this->_M_allocate(_S_check_init_len(__n, _M_get_Tp_allocator())); >- this->_M_impl._M_end_of_storage = this->_M_impl._M_start + __n; >- this->_M_impl._M_finish = >- std::__uninitialized_copy_a(__first, __last, >- this->_M_impl._M_start, >- _M_get_Tp_allocator()); >+ _Guard __guard(this->_M_allocate(_S_check_init_len( >+ __n, _M_get_Tp_allocator())), __n, *this); Again, I think this would be easier to read if split up into two statements, rather than doing the _S_check_init_len call and the _M_allocate call and the _Guard initialization all at once. >+ this->_M_impl._M_finish = std::__uninitialized_copy_a >+ (__first, __last, __guard._M_storage, _M_get_Tp_allocator()); >+ pointer __start = this->_M_impl._M_start = __guard._M_release(); >+ this->_M_impl._M_end_of_storage = __start + __n; > } > >- // Called by the first initialize_dispatch above and by the >- // vector(n,value,a) constructor. >+ // Called by the vector(n,value,a) constructor. > _GLIBCXX20_CONSTEXPR > void > _M_fill_initialize(size_type __n, const value_type& __value) >diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc >index 25df060beee..e31da4f6c4c 100644 >--- a/libstdc++-v3/include/bits/vector.tcc >+++ b/libstdc++-v3/include/bits/vector.tcc >@@ -467,32 +467,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER > pointer __new_start(this->_M_allocate(__len)); > pointer __new_finish(__new_start); > >- // RAII guard for allocated storage. >- struct _Guard > { >- pointer _M_storage; // Storage to deallocate >- size_type _M_len; >- _Tp_alloc_type& _M_alloc; >- >- _GLIBCXX20_CONSTEXPR >- _Guard(pointer __s, size_type __l, _Tp_alloc_type& __a) >- : _M_storage(__s), _M_len(__l), _M_alloc(__a) >- { } >- >- _GLIBCXX20_CONSTEXPR >- ~_Guard() >- { >- if (_M_storage) >- __gnu_cxx::__alloc_traits<_Tp_alloc_type>:: >- deallocate(_M_alloc, _M_storage, _M_len); >- } >- >- private: >- _Guard(const _Guard&); >- }; >- >- { >- _Guard __guard(__new_start, __len, _M_impl); >+ _Guard __guard(__new_start, __len, *this); > > // The order of the three operations is dictated by the C++11 > // case, where the moves could alter a new element belonging >@@ -596,32 +572,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER > pointer __new_start(this->_M_allocate(__len)); > pointer __new_finish(__new_start); > >- // RAII guard for allocated storage. >- struct _Guard >- { >- pointer _M_storage; // Storage to deallocate >- size_type _M_len; >- _Tp_alloc_type& _M_alloc; >- >- _GLIBCXX20_CONSTEXPR >- _Guard(pointer __s, size_type __l, _Tp_alloc_type& __a) >- : _M_storage(__s), _M_len(__l), _M_alloc(__a) >- { } >- >- _GLIBCXX20_CONSTEXPR >- ~_Guard() >- { >- if (_M_storage) >- __gnu_cxx::__alloc_traits<_Tp_alloc_type>:: >- deallocate(_M_alloc, _M_storage, _M_len); >- } >- >- private: >- _Guard(const _Guard&); >- }; >- > { >- _Guard __guard(__new_start, __len, _M_impl); >+ _Guard __guard(__new_start, __len, *this); > > // The order of the three operations is dictated by the C++11 > // case, where the moves could alter a new element belonging