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

[Jonathan Wakely <jwakely.gcc@gmail.com>]

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