[Bug tree-optimization/109353] New: FAIL: 23_containers/vector/bool/allocator/copy.cc (test for excess errors)

[Bug tree-optimization/109353] New: FAIL: 23_containers/vector/bool/allocator/copy.cc (test for excess errors)

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109353

            Bug ID: 109353
           Summary: FAIL: 23_containers/vector/bool/allocator/copy.cc
                    (test for excess errors)
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rguenth at gcc dot gnu.org
  Target Milestone: ---

After the last ranger changes this test now FAILs with

In file included from
/home/rguenther/obj-trunk-g/x86_64-pc-linux-gnu/libstdc++-v3/include/vector:62,
                 from
/home/rguenther/src/trunk/libstdc++-v3/testsuite/23_containers/vector/bool/allocator/copy.cc:20:
In static member function 'static _Up* std::__copy_move<_IsMove, true,
std::random_access_iterator_tag>::__copy_m(_Tp*, _Tp*, _Up*) [with _Tp = long
unsigned int; _Up = long unsigned int; bool _IsMove = false]',
    inlined from '_OI std::__copy_move_a2(_II, _II, _OI) [with bool _IsMove =
false; _II = long unsigned int*; _OI = long unsigned int*]' at
/home/rguenther/obj-trunk-g/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:506,
    inlined from '_OI std::__copy_move_a1(_II, _II, _OI) [with bool _IsMove =
false; _II = long unsigned int*; _OI = long unsigned int*]' at
/home/rguenther/obj-trunk-g/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:533,
    inlined from '_OI std::__copy_move_a(_II, _II, _OI) [with bool _IsMove =
false; _II = long unsigned int*; _OI = long unsigned int*]' at
/home/rguenther/obj-trunk-g/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:540,
    inlined from '_OI std::copy(_II, _II, _OI) [with _II = long unsigned int*;
_OI = long unsigned int*]' at
/home/rguenther/obj-trunk-g/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:633,
    inlined from 'std::vector<bool, _Alloc>::iterator std::vector<bool,
_Alloc>::_M_copy_aligned(const_iterator, const_iterator, iterator) [with _Alloc
= __gnu_test::propagating_allocator<bool, false>]' at
/home/rguenther/obj-trunk-g/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_bvector.h:1303,
    inlined from 'void std::vector<bool, _Alloc>::_M_insert_aux(iterator, bool)
[with _Alloc = __gnu_test::propagating_allocator<bool, false>]' at
/home/rguenther/obj-trunk-g/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/vector.tcc:945,
    inlined from 'void std::vector<bool, _Alloc>::push_back(bool) [with _Alloc
= __gnu_test::propagating_allocator<bool, false>]' at
/home/rguenther/obj-trunk-g/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_bvector.h:1121,
    inlined from 'void test01()' at
/home/rguenther/src/trunk/libstdc++-v3/testsuite/23_containers/vector/bool/allocator/copy.cc:33:
/home/rguenther/obj-trunk-g/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:437:
warning: 'void* __builtin_memmove(void*, const void*, long unsigned int)'
writing between 9 and 9223372036854775807 bytes into a region of size 8
overflows the destination [-Wstringop-overflow=]
In file included from
/home/rguenther/obj-trunk-g/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu/bits/c++allocator.h:33,
                 from
/home/rguenther/obj-trunk-g/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/allocator.h:46,
                 from
/home/rguenther/obj-trunk-g/x86_64-pc-linux-gnu/libstdc++-v3/include/vector:63:
In member function '_Tp* std::__new_allocator<_Tp>::allocate(size_type, const
void*) [with _Tp = long unsigned int]',
    inlined from 'static _Tp* std::allocator_traits<std::allocator<_Tp1>
>::allocate(allocator_type&, size_type) [with _Tp = long unsigned int]' at
/home/rguenther/obj-trunk-g/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/alloc_traits.h:482,
    inlined from '__gnu_test::uneq_allocator<Tp, Alloc>::pointer
__gnu_test::uneq_allocator<Tp, Alloc>::allocate(size_type, const void*) [with
Tp = long unsigned int; Alloc = std::allocator<long unsigned int>]' at
/home/rguenther/src/trunk/libstdc++-v3/testsuite/util/testsuite_allocator.h:360,
    inlined from 'static std::allocator_traits< <template-parameter-1-1>
>::pointer std::allocator_traits< <template-parameter-1-1> >::allocate(_Alloc&,
size_type) [with _Alloc = __gnu_test::propagating_allocator<long unsigned int,
false, std::allocator<long unsigned int> >]' at
/home/rguenther/obj-trunk-g/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/alloc_traits.h:333,
    inlined from 'std::_Bvector_base<_Alloc>::_Bit_pointer
std::_Bvector_base<_Alloc>::_M_allocate(std::size_t) [with _Alloc =
__gnu_test::propagating_allocator<bool, false>]' at
/home/rguenther/obj-trunk-g/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_bvector.h:643,
    inlined from 'void std::vector<bool, _Alloc>::_M_insert_aux(iterator, bool)
[with _Alloc = __gnu_test::propagating_allocator<bool, false>]' at
/home/rguenther/obj-trunk-g/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/vector.tcc:943,
    inlined from 'void std::vector<bool, _Alloc>::push_back(bool) [with _Alloc
= __gnu_test::propagating_allocator<bool, false>]' at
/home/rguenther/obj-trunk-g/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_bvector.h:1121,
    inlined from 'void test01()' at
/home/rguenther/src/trunk/libstdc++-v3/testsuite/23_containers/vector/bool/allocator/copy.cc:33:
/home/rguenther/obj-trunk-g/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/new_allocator.h:147:
note: destination object of size 8 allocated by 'operator new'

[Bug tree-optimization/109353] FAIL: 23_containers/vector/bool/allocator/copy.cc (test for excess errors)

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109353

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
           Keywords|                            |missed-optimization
                 CC|                            |jwakely.gcc at gmail dot com
   Last reconfirmed|                            |2023-03-31
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
<bb 8> [local count: 86938296]:
D.70713 ={v} {CLOBBER(eol)};
_74 = v1.D.60930._M_impl.D.60395._M_start.D.16824._M_p;
_638 = (long int) _74;
_261 = -_638;
_383 = (long unsigned int) _261;
if (_638 < -8)
  goto <bb 11>; [90.00%]
else
  goto <bb 12>; [10.00%]

<bb 11> [local count: 78244465]:
__builtin_memmove (_216, _74, _383);

so _383 is > 8, the reported access size looks good here.  We determine
the destination size as 8 which is also good since

_216 = operator new (8);

so the diagnostic seems legit.  The alternative code taken isn't very much
better:

<bb 12> [local count: 8693830]:
if (_74 == -8B)
  goto <bb 13>; [34.00%]
else
  goto <bb 14>; [66.00%]

<bb 13> [local count: 2955901]:
_266 = MEM[(long unsigned int *)-8B];
*_216 = _266;

<bb 14> [local count: 86938296]:
_268 = _216 + _383;
_324 = MEM[(_Bit_type *)_268];
_327 = _324 & 18446744073709551614;

so definitely bb13 looks to be better unreachable as well.

I have difficulties in tracing libstdc++ back to the ultimate caller but
the above control flow is from stl_algobase.h:435 which is

  template<bool _IsMove>
    struct __copy_move<_IsMove, true, random_access_iterator_tag>
    {
      template<typename _Tp, typename _Up>
        _GLIBCXX20_CONSTEXPR
        static _Up* 
        __copy_m(_Tp* __first, _Tp* __last, _Up* __result) 
        {
          const ptrdiff_t _Num = __last - __first;
          if (__builtin_expect(_Num > 1, true)) 
            __builtin_memmove(__result, __first, sizeof(_Tp) * _Num);
          else if (_Num == 1) 
            std::__copy_move<_IsMove, false, random_access_iterator_tag>::
              __assign_one(__result, __first);
          return __result + _Num; 
        }
    };

but the assign-one seems to be odd.  So I suppose this all happens before

   v1.push_back(T());

and thus _Num == 0.

v1.D.60930._M_impl.D.60395._M_start.D.16824._M_p is probably NULL at this
point but 'v1' escapes to _M_deallocate so the intervening
operator new calls and an __atomic_load_1 / __cxa_guard_acquire prevent
CSE of this value.

It might be possible (again) to CSE this manually in libstdc++ to help
code generation.

[Bug tree-optimization/109353] FAIL: 23_containers/vector/bool/allocator/copy.cc (test for excess errors)

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109353

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
So it seems to be

    _M_insert_aux(iterator __position, bool __x)
    {
      if (this->_M_impl._M_finish._M_p != this->_M_impl._M_end_addr())
        {
          std::copy_backward(__position, this->_M_impl._M_finish,
                             this->_M_impl._M_finish + 1);
          *__position = __x;
          ++this->_M_impl._M_finish;
        }
      else
        {
          const size_type __len =
            _M_check_len(size_type(1), "vector<bool>::_M_insert_aux");
          _Bit_pointer __q = this->_M_allocate(__len);
          iterator __start(std::__addressof(*__q), 0);
          iterator __i = _M_copy_aligned(begin(), __position, __start); // <--
          *__i++ = __x;

where the compiler fails to optimize the case of no existing allocation.
Since that's from the push_back () itself no caching of the null start
in libstdc++ is possible.

Disabling early threading avoids the diagnostic because we then inline
very differently, not exposing most of the above.

[Bug tree-optimization/109353] FAIL: 23_containers/vector/bool/allocator/copy.cc (test for excess errors)

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109353

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
So if it's possible to refactor things as to key the copy size dispatching on
the destination size (that seems to be known here) rather than the source
that might help (unless we then diagnose overread from the source in other
cases ...)

[Bug tree-optimization/109353] FAIL: 23_containers/vector/bool/allocator/copy.cc (test for excess errors)

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109353

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
This doesn't help:

--- a/libstdc++-v3/include/bits/vector.tcc
+++ b/libstdc++-v3/include/bits/vector.tcc
@@ -936,15 +936,25 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
          *__position = __x;
          ++this->_M_impl._M_finish;
        }
+      else if (!this->_M_impl._M_start._M_p)
+       {
+         _Bit_pointer __q = this->_M_allocate(1);
+         iterator __i(std::__addressof(*__q), 0);
+         this->_M_impl._M_end_of_storage = __q + 1;
+         this->_M_impl._M_start = __i;
+         *__i = __x;
+         this->_M_impl._M_finish = ++__i;
+       }
       else
        {
          const size_type __len =
            _M_check_len(size_type(1), "vector<bool>::_M_insert_aux");
+         const iterator __begin = begin(), __end = end();
          _Bit_pointer __q = this->_M_allocate(__len);
          iterator __start(std::__addressof(*__q), 0);
-         iterator __i = _M_copy_aligned(begin(), __position, __start);
+         iterator __i = _M_copy_aligned(__begin, __position, __start);
          *__i++ = __x;
-         iterator __finish = std::copy(__position, end(), __i);
+         iterator __finish = std::copy(__position, __end, __i);
          this->_M_deallocate();
          this->_M_impl._M_end_of_storage = __q + _S_nword(__len);
          this->_M_impl._M_start = __start;

[Bug tree-optimization/109353] FAIL: 23_containers/vector/bool/allocator/copy.cc (test for excess errors)

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109353

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #4)
> This doesn't help:
> 
> --- a/libstdc++-v3/include/bits/vector.tcc
> +++ b/libstdc++-v3/include/bits/vector.tcc
> @@ -936,15 +936,25 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>           *__position = __x;
>           ++this->_M_impl._M_finish;
>         }
> +      else if (!this->_M_impl._M_start._M_p)

yes, we can't CSE this load from the = nullptr assignment so we'd still
diagnose the else case ...

[Bug tree-optimization/109353] FAIL: 23_containers/vector/bool/allocator/copy.cc (test for excess errors)

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109353

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |seurer at gcc dot gnu.org

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
*** Bug 111686 has been marked as a duplicate of this bug. ***