* [PATCH] Avoid vector -Wfree-nonheap-object warnings @ 2024-05-23 4:55 François Dumont 2024-05-23 13:31 ` Jonathan Wakely 0 siblings, 1 reply; 9+ messages in thread From: François Dumont @ 2024-05-23 4:55 UTC (permalink / raw) To: libstdc++; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 1079 bytes --] 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. 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 [-- Attachment #2: vector_patch.txt --] [-- Type: text/plain, Size: 5932 bytes --] 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 + { + 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; + 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<typename _Integer> 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<size_type>(__n), _M_get_Tp_allocator())); - this->_M_impl._M_end_of_storage = - this->_M_impl._M_start + static_cast<size_type>(__n); - _M_fill_initialize(static_cast<size_type>(__n), __value); + const size_type __n = static_cast<size_type>(__int_n); + _Guard __guard(_M_allocate(_S_check_init_len( + __n, _M_get_Tp_allocator())), __n, *this); + 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); + 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings 2024-05-23 4:55 [PATCH] Avoid vector -Wfree-nonheap-object warnings François Dumont @ 2024-05-23 13:31 ` Jonathan Wakely 2024-05-23 17:37 ` François Dumont 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Wakely @ 2024-05-23 13:31 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches 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<typename _Integer> > 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<size_type>(__n), _M_get_Tp_allocator())); >- this->_M_impl._M_end_of_storage = >- this->_M_impl._M_start + static_cast<size_type>(__n); >- _M_fill_initialize(static_cast<size_type>(__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<size_type>(__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<typename _Integer> void _M_initialize_dispatch(_Integer __ni, _Integer __value, __true_type) { const size_type __n = static_cast<size_type>(__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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings 2024-05-23 13:31 ` Jonathan Wakely @ 2024-05-23 17:37 ` François Dumont 2024-05-24 14:17 ` Jonathan Wakely 0 siblings, 1 reply; 9+ messages in thread From: François Dumont @ 2024-05-23 17:37 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 7183 bytes --] On 23/05/2024 15:31, Jonathan Wakely wrote: > 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? 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=115016 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 = _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. 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 = 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<typename _Integer> >> 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<size_type>(__n), _M_get_Tp_allocator())); >> - this->_M_impl._M_end_of_storage = >> - this->_M_impl._M_start + static_cast<size_type>(__n); >> - _M_fill_initialize(static_cast<size_type>(__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 = static_cast<size_type>(__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<typename _Integer> > void > _M_initialize_dispatch(_Integer __ni, _Integer __value, __true_type) > { > const size_type __n = static_cast<size_type>(__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. 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 :-) Ok to commit ? François [-- Attachment #2: vector_patch.txt --] [-- Type: text/plain, Size: 6966 bytes --] diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h index 31169711a48..30d7201c613 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_alloc + { + pointer _M_storage; // Storage to deallocate + size_type _M_len; + _Base& _M_vect; + + _GLIBCXX20_CONSTEXPR + _Guard_alloc(pointer __s, size_type __l, _Base& __vect) + : _M_storage(__s), _M_len(__l), _M_vect(__vect) + { } + + _GLIBCXX20_CONSTEXPR + ~_Guard_alloc() + { + if (_M_storage) + _M_vect._M_deallocate(_M_storage, _M_len); + } + + _GLIBCXX20_CONSTEXPR + pointer + _M_release() + { + pointer __res = _M_storage; + _M_storage = pointer(); + return __res; + } + + private: + _Guard_alloc(const _Guard_alloc&); + }; + 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_alloc __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,17 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER // 438. Ambiguity in the "do the right thing" clause template<typename _Integer> 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<size_type>(__n), _M_get_Tp_allocator())); - this->_M_impl._M_end_of_storage = - this->_M_impl._M_start + static_cast<size_type>(__n); - _M_fill_initialize(static_cast<size_type>(__n), __value); + const size_type __n = static_cast<size_type>(__int_n); + pointer __start = + _M_allocate(_S_check_init_len(__n, _M_get_Tp_allocator())); + _Guard_alloc __guard(__start, __n, *this); + this->_M_impl._M_finish = std::__uninitialized_fill_n_a + (__start, __n, __value, _M_get_Tp_allocator()); + this->_M_impl._M_start = __start; + (void)__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 +1719,17 @@ _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()); + pointer __start = + this->_M_allocate(_S_check_init_len(__n, _M_get_Tp_allocator())); + _Guard_alloc __guard(__start, __n, *this); + this->_M_impl._M_finish = std::__uninitialized_copy_a + (__first, __last, __start, _M_get_Tp_allocator()); + this->_M_impl._M_start = __start; + (void) __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..36b27dce7b9 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_alloc __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_alloc __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 @@ -833,32 +785,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER _M_check_len(__n, "vector::_M_default_append"); pointer __new_start(this->_M_allocate(__len)); - // 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_alloc __guard(__new_start, __len, *this); std::__uninitialized_default_n_a(__new_start + __size, __n, _M_get_Tp_allocator()); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings 2024-05-23 17:37 ` François Dumont @ 2024-05-24 14:17 ` Jonathan Wakely 2024-05-25 9:59 ` François Dumont 2024-05-27 4:37 ` François Dumont 0 siblings, 2 replies; 9+ messages in thread From: Jonathan Wakely @ 2024-05-24 14:17 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On Thu, 23 May 2024 at 18:38, François Dumont <frs.dumont@gmail.com> wrote: > > > On 23/05/2024 15:31, Jonathan Wakely wrote: > > 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? > > 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=115016 > > 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 = _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. > > 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 = 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<typename _Integer> > >> 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<size_type>(__n), _M_get_Tp_allocator())); > >> - this->_M_impl._M_end_of_storage = > >> - this->_M_impl._M_start + static_cast<size_type>(__n); > >> - _M_fill_initialize(static_cast<size_type>(__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 = static_cast<size_type>(__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<typename _Integer> > > void > > _M_initialize_dispatch(_Integer __ni, _Integer __value, __true_type) > > { > > const size_type __n = static_cast<size_type>(__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. > > 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! ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings 2024-05-24 14:17 ` Jonathan Wakely @ 2024-05-25 9:59 ` François Dumont 2024-05-27 4:37 ` François Dumont 1 sibling, 0 replies; 9+ messages in thread From: François Dumont @ 2024-05-25 9:59 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc-patches On 24/05/2024 16:17, Jonathan Wakely wrote: > On Thu, 23 May 2024 at 18:38, François Dumont <frs.dumont@gmail.com> wrote: >> >> On 23/05/2024 15:31, Jonathan Wakely wrote: >>> 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? >> 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=115016 >> 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 = _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. >> 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 = 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<typename _Integer> >>>> 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<size_type>(__n), _M_get_Tp_allocator())); >>>> - this->_M_impl._M_end_of_storage = >>>> - this->_M_impl._M_start + static_cast<size_type>(__n); >>>> - _M_fill_initialize(static_cast<size_type>(__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 = static_cast<size_type>(__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<typename _Integer> >>> void >>> _M_initialize_dispatch(_Integer __ni, _Integer __value, __true_type) >>> { >>> const size_type __n = static_cast<size_type>(__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. >> Yes, this is why I called __uninitialized_fill_n_a instead and also to >> do so *before* assigning _M_impl._M_start. >> Ok to commit ? > OK for trunk, thanks! > There are test failures in C++98, working on it. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings 2024-05-24 14:17 ` Jonathan Wakely 2024-05-25 9:59 ` François Dumont @ 2024-05-27 4:37 ` François Dumont 2024-05-28 10:30 ` Jonathan Wakely 1 sibling, 1 reply; 9+ messages in thread From: François Dumont @ 2024-05-27 4:37 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 7505 bytes --] Here is a new version working also in C++98. Note that I have this failure: FAIL: 23_containers/vector/types/1.cc -std=gnu++98 (test for excess errors) but it's already failing on master, my patch do not change anything. Tested under Linux x64, still ok to commit ? François On 24/05/2024 16:17, Jonathan Wakely wrote: > On Thu, 23 May 2024 at 18:38, François Dumont <frs.dumont@gmail.com> wrote: >> >> On 23/05/2024 15:31, Jonathan Wakely wrote: >>> 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? >> 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=115016 >> 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 = _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. >> 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 = 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<typename _Integer> >>>> 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<size_type>(__n), _M_get_Tp_allocator())); >>>> - this->_M_impl._M_end_of_storage = >>>> - this->_M_impl._M_start + static_cast<size_type>(__n); >>>> - _M_fill_initialize(static_cast<size_type>(__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 = static_cast<size_type>(__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<typename _Integer> >>> void >>> _M_initialize_dispatch(_Integer __ni, _Integer __value, __true_type) >>> { >>> const size_type __n = static_cast<size_type>(__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. >> 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! > [-- Attachment #2: vector_patch.txt --] [-- Type: text/plain, Size: 8321 bytes --] diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h index 31169711a48..81fe3825064 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_alloc + { + pointer _M_storage; // Storage to deallocate + size_type _M_len; + _Base& _M_vect; + + _GLIBCXX20_CONSTEXPR + _Guard_alloc(pointer __s, size_type __l, _Base& __vect) + : _M_storage(__s), _M_len(__l), _M_vect(__vect) + { } + + _GLIBCXX20_CONSTEXPR + ~_Guard_alloc() + { + if (_M_storage) + _M_vect._M_deallocate(_M_storage, _M_len); + } + + _GLIBCXX20_CONSTEXPR + pointer + _M_release() + { + pointer __res = _M_storage; + _M_storage = pointer(); + return __res; + } + + private: + _Guard_alloc(const _Guard_alloc&); + }; + 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_alloc __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,18 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER // 438. Ambiguity in the "do the right thing" clause template<typename _Integer> 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<size_type>(__n), _M_get_Tp_allocator())); - this->_M_impl._M_end_of_storage = - this->_M_impl._M_start + static_cast<size_type>(__n); - _M_fill_initialize(static_cast<size_type>(__n), __value); + const size_type __n = static_cast<size_type>(__int_n); + pointer __start = + _M_allocate(_S_check_init_len(__n, _M_get_Tp_allocator())); + _Guard_alloc __guard(__start, __n, *this); + this->_M_impl._M_finish = + std::__uninitialized_fill_n_a<pointer, size_type, value_type> + (__start, __n, __value, _M_get_Tp_allocator()); + this->_M_impl._M_start = __start; + (void) __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 +1720,17 @@ _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()); + pointer __start = + this->_M_allocate(_S_check_init_len(__n, _M_get_Tp_allocator())); + _Guard_alloc __guard(__start, __n, *this); + this->_M_impl._M_finish = std::__uninitialized_copy_a + (__first, __last, __start, _M_get_Tp_allocator()); + this->_M_impl._M_start = __start; + (void) __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..36b27dce7b9 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_alloc __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_alloc __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 @@ -833,32 +785,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER _M_check_len(__n, "vector::_M_default_append"); pointer __new_start(this->_M_allocate(__len)); - // 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_alloc __guard(__new_start, __len, *this); std::__uninitialized_default_n_a(__new_start + __size, __n, _M_get_Tp_allocator()); diff --git a/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_1_neg.cc b/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_1_neg.cc index 6b3d48f0c92..d732ca8bed8 100644 --- a/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_1_neg.cc +++ b/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_1_neg.cc @@ -19,7 +19,7 @@ // { dg-do compile } // { dg-prune-output "cannot convert" } -// { dg-prune-output "no matching function .*_M_fill_initialize" } +// { dg-prune-output "no matching function .*__uninitialized_fill_n_a" } #include <vector> diff --git a/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_2_neg.cc b/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_2_neg.cc index 65ce2fd30e5..58c4c8f9a73 100644 --- a/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_2_neg.cc +++ b/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_2_neg.cc @@ -19,7 +19,7 @@ // { dg-do compile } // { dg-prune-output "cannot convert" } -// { dg-prune-output "no matching function .*_M_fill_initialize" } +// { dg-prune-output "no matching function .*__uninitialized_fill_n_a" } #include <vector> #include <utility> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings 2024-05-27 4:37 ` François Dumont @ 2024-05-28 10:30 ` Jonathan Wakely 2024-05-28 20:53 ` François Dumont 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Wakely @ 2024-05-28 10:30 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On Mon, 27 May 2024 at 05:37, François Dumont <frs.dumont@gmail.com> wrote: > > Here is a new version working also in C++98. Can we use a different solution that doesn't involve an explicit template argument list for that __uninitialized_fill_n_a call? -+ this->_M_impl._M_finish = std::__uninitialized_fill_n_a ++ this->_M_impl._M_finish = ++ std::__uninitialized_fill_n_a<pointer, size_type, value_type> + (__start, __n, __value, _M_get_Tp_allocator()); Using _M_fill_initialize solves the problem :-) > > Note that I have this failure: > > FAIL: 23_containers/vector/types/1.cc -std=gnu++98 (test for excess errors) > > but it's already failing on master, my patch do not change anything. Yes, that's been failing for ages. > > Tested under Linux x64, > > still ok to commit ? > > François > > On 24/05/2024 16:17, Jonathan Wakely wrote: > > On Thu, 23 May 2024 at 18:38, François Dumont <frs.dumont@gmail.com> wrote: > >> > >> On 23/05/2024 15:31, Jonathan Wakely wrote: > >>> 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? > >> 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=115016 > >> 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 = _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. > >> 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 = 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<typename _Integer> > >>>> 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<size_type>(__n), _M_get_Tp_allocator())); > >>>> - this->_M_impl._M_end_of_storage = > >>>> - this->_M_impl._M_start + static_cast<size_type>(__n); > >>>> - _M_fill_initialize(static_cast<size_type>(__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 = static_cast<size_type>(__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<typename _Integer> > >>> void > >>> _M_initialize_dispatch(_Integer __ni, _Integer __value, __true_type) > >>> { > >>> const size_type __n = static_cast<size_type>(__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. > >> 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! > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings 2024-05-28 10:30 ` Jonathan Wakely @ 2024-05-28 20:53 ` François Dumont 2024-05-29 9:35 ` Jonathan Wakely 0 siblings, 1 reply; 9+ messages in thread From: François Dumont @ 2024-05-28 20:53 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 9583 bytes --] I can indeed restore _M_initialize_dispatch as it was before. It was not fixing my initial problem. I simply kept the code simplification. 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 and rename. (_M_allocate_and_copy): Use latter. (_M_initialize_dispatch): Small code simplification. (_M_range_initialize): Likewise and set _M_finish first from the result of __uninitialize_fill_n_a that can throw. Tested under Linux x86_64. Ok to commit ? François On 28/05/2024 12:30, Jonathan Wakely wrote: > On Mon, 27 May 2024 at 05:37, François Dumont <frs.dumont@gmail.com> wrote: >> Here is a new version working also in C++98. > Can we use a different solution that doesn't involve an explicit > template argument list for that __uninitialized_fill_n_a call? > > -+ this->_M_impl._M_finish = std::__uninitialized_fill_n_a > ++ this->_M_impl._M_finish = > ++ std::__uninitialized_fill_n_a<pointer, size_type, value_type> > + (__start, __n, __value, _M_get_Tp_allocator()); > > Using _M_fill_initialize solves the problem :-) > > > >> Note that I have this failure: >> >> FAIL: 23_containers/vector/types/1.cc -std=gnu++98 (test for excess errors) >> >> but it's already failing on master, my patch do not change anything. > Yes, that's been failing for ages. > >> Tested under Linux x64, >> >> still ok to commit ? >> >> François >> >> On 24/05/2024 16:17, Jonathan Wakely wrote: >>> On Thu, 23 May 2024 at 18:38, François Dumont <frs.dumont@gmail.com> wrote: >>>> On 23/05/2024 15:31, Jonathan Wakely wrote: >>>>> 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? >>>> 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=115016 >>>> 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 = _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. >>>> 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 = 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<typename _Integer> >>>>>> 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<size_type>(__n), _M_get_Tp_allocator())); >>>>>> - this->_M_impl._M_end_of_storage = >>>>>> - this->_M_impl._M_start + static_cast<size_type>(__n); >>>>>> - _M_fill_initialize(static_cast<size_type>(__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 = static_cast<size_type>(__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<typename _Integer> >>>>> void >>>>> _M_initialize_dispatch(_Integer __ni, _Integer __value, __true_type) >>>>> { >>>>> const size_type __n = static_cast<size_type>(__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. >>>> 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! >>> [-- Attachment #2: vector_patch.txt --] [-- Type: text/plain, Size: 6607 bytes --] diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h index 31169711a48..182ad41ed94 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_alloc + { + pointer _M_storage; // Storage to deallocate + size_type _M_len; + _Base& _M_vect; + + _GLIBCXX20_CONSTEXPR + _Guard_alloc(pointer __s, size_type __l, _Base& __vect) + : _M_storage(__s), _M_len(__l), _M_vect(__vect) + { } + + _GLIBCXX20_CONSTEXPR + ~_Guard_alloc() + { + if (_M_storage) + _M_vect._M_deallocate(_M_storage, _M_len); + } + + _GLIBCXX20_CONSTEXPR + pointer + _M_release() + { + pointer __res = _M_storage; + _M_storage = pointer(); + return __res; + } + + private: + _Guard_alloc(const _Guard_alloc&); + }; + 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_alloc __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,14 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER // 438. Ambiguity in the "do the right thing" clause template<typename _Integer> 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<size_type>(__n), _M_get_Tp_allocator())); - this->_M_impl._M_end_of_storage = - this->_M_impl._M_start + static_cast<size_type>(__n); - _M_fill_initialize(static_cast<size_type>(__n), __value); + const size_type __n = static_cast<size_type>(__int_n); + pointer __start = + _M_allocate(_S_check_init_len(__n, _M_get_Tp_allocator())); + this->_M_impl._M_start = __start; + this->_M_impl._M_end_of_storage = __start + __n; + _M_fill_initialize(__n, __value); } // Called by the range constructor to implement [23.1.1]/9 @@ -1690,13 +1716,14 @@ _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()); + pointer __start = + this->_M_allocate(_S_check_init_len(__n, _M_get_Tp_allocator())); + _Guard_alloc __guard(__start, __n, *this); + this->_M_impl._M_finish = std::__uninitialized_copy_a + (__first, __last, __start, _M_get_Tp_allocator()); + this->_M_impl._M_start = __start; + (void) __guard._M_release(); + this->_M_impl._M_end_of_storage = __start + __n; } // Called by the first initialize_dispatch above and by the diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc index 25df060beee..36b27dce7b9 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_alloc __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_alloc __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 @@ -833,32 +785,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER _M_check_len(__n, "vector::_M_default_append"); pointer __new_start(this->_M_allocate(__len)); - // 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_alloc __guard(__new_start, __len, *this); std::__uninitialized_default_n_a(__new_start + __size, __n, _M_get_Tp_allocator()); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings 2024-05-28 20:53 ` François Dumont @ 2024-05-29 9:35 ` Jonathan Wakely 0 siblings, 0 replies; 9+ messages in thread From: Jonathan Wakely @ 2024-05-29 9:35 UTC (permalink / raw) To: François Dumont; +Cc: Jonathan Wakely, libstdc++, gcc-patches On Tue, 28 May 2024 at 21:55, François Dumont <frs.dumont@gmail.com> wrote: > > I can indeed restore _M_initialize_dispatch as it was before. It was not > fixing my initial problem. I simply kept the code simplification. > > 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 and rename. > (_M_allocate_and_copy): Use latter. > (_M_initialize_dispatch): Small code simplification. > (_M_range_initialize): Likewise and set _M_finish first > from the result > of __uninitialize_fill_n_a that can throw. > > Tested under Linux x86_64. > > Ok to commit ? OK, thanks > > François > > On 28/05/2024 12:30, Jonathan Wakely wrote: > > On Mon, 27 May 2024 at 05:37, François Dumont <frs.dumont@gmail.com> wrote: > >> Here is a new version working also in C++98. > > Can we use a different solution that doesn't involve an explicit > > template argument list for that __uninitialized_fill_n_a call? > > > > -+ this->_M_impl._M_finish = std::__uninitialized_fill_n_a > > ++ this->_M_impl._M_finish = > > ++ std::__uninitialized_fill_n_a<pointer, size_type, value_type> > > + (__start, __n, __value, _M_get_Tp_allocator()); > > > > Using _M_fill_initialize solves the problem :-) > > > > > > > >> Note that I have this failure: > >> > >> FAIL: 23_containers/vector/types/1.cc -std=gnu++98 (test for excess errors) > >> > >> but it's already failing on master, my patch do not change anything. > > Yes, that's been failing for ages. > > > >> Tested under Linux x64, > >> > >> still ok to commit ? > >> > >> François > >> > >> On 24/05/2024 16:17, Jonathan Wakely wrote: > >>> On Thu, 23 May 2024 at 18:38, François Dumont <frs.dumont@gmail.com> wrote: > >>>> On 23/05/2024 15:31, Jonathan Wakely wrote: > >>>>> 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? > >>>> 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=115016 > >>>> 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 = _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. > >>>> 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 = 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<typename _Integer> > >>>>>> 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<size_type>(__n), _M_get_Tp_allocator())); > >>>>>> - this->_M_impl._M_end_of_storage = > >>>>>> - this->_M_impl._M_start + static_cast<size_type>(__n); > >>>>>> - _M_fill_initialize(static_cast<size_type>(__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 = static_cast<size_type>(__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<typename _Integer> > >>>>> void > >>>>> _M_initialize_dispatch(_Integer __ni, _Integer __value, __true_type) > >>>>> { > >>>>> const size_type __n = static_cast<size_type>(__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. > >>>> 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! > >>> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-05-29 9:35 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-05-23 4:55 [PATCH] Avoid vector -Wfree-nonheap-object warnings François Dumont 2024-05-23 13:31 ` Jonathan Wakely 2024-05-23 17:37 ` François Dumont 2024-05-24 14:17 ` Jonathan Wakely 2024-05-25 9:59 ` François Dumont 2024-05-27 4:37 ` François Dumont 2024-05-28 10:30 ` Jonathan Wakely 2024-05-28 20:53 ` François Dumont 2024-05-29 9:35 ` Jonathan Wakely
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).