On Thu, 30 May 2024 at 06:11, François Dumont <frs.dum...@gmail.com> wrote: > > Looks like this new version works the same to fix the warning without > the issues reported here. > > All 23_containers/vector tests run in C++98/14/20 so far. > > Ok to commit once I've complete the testsuite (or some bot did it for me > !) ?
There seem to be two unrelated changes here. One is to make the local variables usable in both branches, but I don't understand why that matters because the first branch doesn't reallocate, so there's no call to operator new that would confuse the compiler. The second change is to add the __builtin_unreachable() to tell the compiler that __len is correct and did not wrap around. Which should not be needed because Are both changes necessary to fix the FAIL for c++98 mode? I've just done a quick check, and it seems that this smaller change fixes the FAIL: --- a/libstdc++-v3/include/bits/vector.tcc +++ b/libstdc++-v3/include/bits/vector.tcc @@ -933,6 +933,9 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER const size_type __len = _M_check_len(__n, "vector::_M_range_insert"); + if (__len < (__n + (__old_finish - __old_start))) + __builtin_unreachable(); + pointer __new_start(this->_M_allocate(__len)); pointer __new_finish(__new_start); __try We don't need the rest of the change, which makes sense because the first branch doesn't reallocate so the compiler doesn't think the pointers can be invalidated. > I'll look for a PR to associate, if you have one in mind do not hesitate > to tell me. It's discussed in PR 109849. > > François > > > On 28/05/2024 12:28, Jonathan Wakely wrote: > > On 27/05/24 22:07 +0200, François Dumont wrote: > >> In C++98 this test fails with: > >> > >> Excess errors: > >> /home/fdumont/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:452: > >> warning: 'void* __builtin_memcpy(void*, const void*, long unsigned > >> int)' writing between 2 and 9223372036854775806 bytes into a region > >> of size 0 overflows the destination [-Wstringop-overflow=] > >> > >> The attached patch avoids this warning. > >> > >> libstdc++: Fix -Wstringop-overflow warning coming from std::vector > >> > >> Make vector<>::_M_range_insert implementation more transparent to > >> the compiler checks. > >> > >> Extend local copies of members to the whole method scope so that > >> all branches benefit > >> from those. > >> > >> libstdc++-v3/ChangeLog: > >> > >> * include/bits/vector.tcc > >> (std::vector<>::_M_range_insert(iterator, _FwdIt, _FwdIt, > >> forward_iterator_tag)): > >> Use local copies of members to call the different > >> algorithms. > >> > >> Ok to commit if all tests passes ? > >> > >> François > > > >> diff --git a/libstdc++-v3/include/bits/vector.tcc > >> b/libstdc++-v3/include/bits/vector.tcc > >> index 36b27dce7b9..671929dee55 100644 > >> --- a/libstdc++-v3/include/bits/vector.tcc > >> +++ b/libstdc++-v3/include/bits/vector.tcc > >> @@ -885,83 +885,80 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER > >> { > >> if (__first != __last) > >> { > >> + // Make local copies of these members because the compiler > >> + // thinks the allocator can alter them if 'this' is globally > >> + // reachable. > >> + pointer __start = this->_M_impl._M_start; > >> + pointer __end = this->_M_impl._M_end_of_storage; > >> + pointer __finish = this->_M_impl._M_finish; > >> + pointer __pos = __position.base(); > >> + _Tp_alloc_type& __allocator = _M_get_Tp_allocator(); > >> + > >> + if (__pos < __start || __finish < __pos) > >> + __builtin_unreachable(); > > > > I don't think we should use __builtin_unreachable for something which > > is not an invariant of the class. The __position argument is supplied > > by the user, so we should not make promises about it being valid, > > because we can't know that. > > > > We can promise that __start <= __finish, and that __finish <= end, > > because we control those. We can't promise the user won't pass in a > > bad __position. Although it's undefined for the user to do that, using > > __builtin_unreachable() here makes the effects worse, and makes it > > harder to debug. > > > > Also, (__pos < __start) might already trigger undefined behaviour for > > fancy pointers, if they don't point to the same memory region. > > > > So this change is not OK. > > > > > >> + > >> const size_type __n = std::distance(__first, __last); > >> - if (size_type(this->_M_impl._M_end_of_storage > >> - - this->_M_impl._M_finish) >= __n) > >> + if (size_type(__end - __finish) >= __n) > >> { > >> - const size_type __elems_after = end() - __position; > >> - pointer __old_finish(this->_M_impl._M_finish); > >> + const size_type __elems_after = __end - __pos; > >> + pointer __old_finish(__finish); > >> if (__elems_after > __n) > >> { > >> _GLIBCXX_ASAN_ANNOTATE_GROW(__n); > >> - std::__uninitialized_move_a(this->_M_impl._M_finish - __n, > >> - this->_M_impl._M_finish, > >> - this->_M_impl._M_finish, > >> - _M_get_Tp_allocator()); > >> - this->_M_impl._M_finish += __n; > >> + __finish = std::__uninitialized_move_a > >> + (__finish - __n, __finish, __finish, __allocator); > >> _GLIBCXX_ASAN_ANNOTATE_GREW(__n); > >> - _GLIBCXX_MOVE_BACKWARD3(__position.base(), > >> - __old_finish - __n, __old_finish); > >> - std::copy(__first, __last, __position); > >> + _GLIBCXX_MOVE_BACKWARD3 > >> + (__pos, __old_finish - __n, __old_finish); > >> + std::copy(__first, __last, __pos); > >> } > >> else > >> { > >> _ForwardIterator __mid = __first; > >> std::advance(__mid, __elems_after); > >> _GLIBCXX_ASAN_ANNOTATE_GROW(__n); > >> - std::__uninitialized_copy_a(__mid, __last, > >> - this->_M_impl._M_finish, > >> - _M_get_Tp_allocator()); > >> - this->_M_impl._M_finish += __n - __elems_after; > >> + __finish = std::__uninitialized_copy_a > >> + (__mid, __last, __finish, __allocator); > >> _GLIBCXX_ASAN_ANNOTATE_GREW(__n - __elems_after); > >> - std::__uninitialized_move_a(__position.base(), > >> - __old_finish, > >> - this->_M_impl._M_finish, > >> - _M_get_Tp_allocator()); > >> - this->_M_impl._M_finish += __elems_after; > >> + __finish = std::__uninitialized_move_a > >> + (__pos, __old_finish, __finish, __allocator); > >> _GLIBCXX_ASAN_ANNOTATE_GREW(__elems_after); > >> - std::copy(__first, __mid, __position); > >> + std::copy(__first, __mid, __pos); > >> } > >> + > >> + this->_M_impl._M_finish = __finish; > >> } > >> else > >> { > >> - // Make local copies of these members because the compiler > >> - // thinks the allocator can alter them if 'this' is globally > >> - // reachable. > >> - pointer __old_start = this->_M_impl._M_start; > >> - pointer __old_finish = this->_M_impl._M_finish; > >> - > >> + const size_type __size = size_type(__finish - __start); > >> const size_type __len = > >> _M_check_len(__n, "vector::_M_range_insert"); > >> + if (__len < __n + __size) > >> + __builtin_unreachable(); > >> + > >> pointer __new_start(this->_M_allocate(__len)); > >> pointer __new_finish(__new_start); > >> __try > >> { > >> __new_finish > >> = std::__uninitialized_move_if_noexcept_a > >> - (__old_start, __position.base(), > >> - __new_start, _M_get_Tp_allocator()); > >> + (__start, __pos, __new_start, __allocator); > >> __new_finish > >> - = std::__uninitialized_copy_a(__first, __last, > >> - __new_finish, > >> - _M_get_Tp_allocator()); > >> + = std::__uninitialized_copy_a > >> + (__first, __last, __new_finish, __allocator); > >> __new_finish > >> = std::__uninitialized_move_if_noexcept_a > >> - (__position.base(), __old_finish, > >> - __new_finish, _M_get_Tp_allocator()); > >> + (__pos, __finish, __new_finish, __allocator); > >> } > >> __catch(...) > >> { > >> - std::_Destroy(__new_start, __new_finish, > >> - _M_get_Tp_allocator()); > >> + std::_Destroy(__new_start, __new_finish, __allocator); > >> _M_deallocate(__new_start, __len); > >> __throw_exception_again; > >> } > >> - std::_Destroy(__old_start, __old_finish, > >> - _M_get_Tp_allocator()); > >> + std::_Destroy(__start, __finish, __allocator); > >> _GLIBCXX_ASAN_ANNOTATE_REINIT; > >> - _M_deallocate(__old_start, > >> - this->_M_impl._M_end_of_storage - __old_start); > >> + _M_deallocate(__start, __end - __start); > >> this->_M_impl._M_start = __new_start; > >> this->_M_impl._M_finish = __new_finish; > >> this->_M_impl._M_end_of_storage = __new_start + __len; > >