Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings

[Jonathan Wakely <jwakely@redhat.com>]

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!
> >