Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings
On Tue, 28 May 2024 at 21:55, François Dumont 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 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 > > + (__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 > >>> 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. >
Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings
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 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 + (__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 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()
Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings
On Mon, 27 May 2024 at 05:37, François Dumont 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 + (__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 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
Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings
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 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 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); -
Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings
On 24/05/2024 16:17, Jonathan Wakely wrote: On Thu, 23 May 2024 at 18:38, François Dumont 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 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. Already done in this initial patch proposal, see below. + const size_type __n = static_cast(__int_n); + _Guard
Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings
On Thu, 23 May 2024 at 18:38, François Dumont 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 > >> void > >> -_M_initialize_dispatch(_Integer __n, _Integer __value, __true_type) > >> +_M_initialize_dispatch(_Integer __int_n, _Integer __value, > >> __true_type) > >> { >
Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings
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 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. Already done in this initial patch proposal, see below. + 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
Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings
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),
[PATCH] Avoid vector -Wfree-nonheap-object warnings
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 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 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); + const size_type __n = static_cast(__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