* [PATCH] Fix -Wstringop-overflow warning in 23_containers/vector/types/1.cc @ 2024-05-27 20:07 François Dumont 2024-05-27 20:18 ` Sam James 2024-05-28 10:28 ` Jonathan Wakely 0 siblings, 2 replies; 10+ messages in thread From: François Dumont @ 2024-05-27 20:07 UTC (permalink / raw) To: libstdc++; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 992 bytes --] 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 [-- Attachment #2: vector_range_insert_patch.txt --] [-- Type: text/plain, Size: 4755 bytes --] 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(); + 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; ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix -Wstringop-overflow warning in 23_containers/vector/types/1.cc 2024-05-27 20:07 [PATCH] Fix -Wstringop-overflow warning in 23_containers/vector/types/1.cc François Dumont @ 2024-05-27 20:18 ` Sam James 2024-05-27 20:58 ` François Dumont 2024-05-28 10:28 ` Jonathan Wakely 1 sibling, 1 reply; 10+ messages in thread From: Sam James @ 2024-05-27 20:18 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches François Dumont <frs.dumont@gmail.com> writes: > In C++98 this test fails with: For this, and your other -Wfree-nonheap-object patches, could you see if it helps with any of the bugs reported for both -Wstringop-overflow and -Wfree-nonheap-object in libstdc++? There's a bunch of (possible) dupes that it'd be worth tagging if any of them are applicable. > > 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 > > [2. text/plain; vector_range_insert_patch.txt]... ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix -Wstringop-overflow warning in 23_containers/vector/types/1.cc 2024-05-27 20:18 ` Sam James @ 2024-05-27 20:58 ` François Dumont 0 siblings, 0 replies; 10+ messages in thread From: François Dumont @ 2024-05-27 20:58 UTC (permalink / raw) To: Sam James; +Cc: libstdc++, gcc-patches Sure, I'll try to have a look. But for the moment this patch introduces some regressions so I keep on working on it. FAIL: 23_containers/vector/modifiers/moveable.cc -std=gnu++14 execution test FAIL: 23_containers/vector/modifiers/moveable2.cc -std=gnu++14 execution test On 27/05/2024 22:18, Sam James wrote: > François Dumont <frs.dumont@gmail.com> writes: > >> In C++98 this test fails with: > For this, and your other -Wfree-nonheap-object patches, could you see if > it helps with any of the bugs reported for both -Wstringop-overflow and > -Wfree-nonheap-object in libstdc++? There's a bunch of (possible) dupes > that it'd be worth tagging if any of them are applicable. > >> 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 >> >> [2. text/plain; vector_range_insert_patch.txt]... ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix -Wstringop-overflow warning in 23_containers/vector/types/1.cc 2024-05-27 20:07 [PATCH] Fix -Wstringop-overflow warning in 23_containers/vector/types/1.cc François Dumont 2024-05-27 20:18 ` Sam James @ 2024-05-28 10:28 ` Jonathan Wakely 2024-05-30 5:11 ` François Dumont 1 sibling, 1 reply; 10+ messages in thread From: Jonathan Wakely @ 2024-05-28 10:28 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches 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; ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix -Wstringop-overflow warning in 23_containers/vector/types/1.cc 2024-05-28 10:28 ` Jonathan Wakely @ 2024-05-30 5:11 ` François Dumont 2024-05-30 11:07 ` Jonathan Wakely 0 siblings, 1 reply; 10+ messages in thread From: François Dumont @ 2024-05-30 5:11 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 9330 bytes --] 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 !) ? I'll look for a PR to associate, if you have one in mind do not hesitate to tell me. 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; > [-- Attachment #2: vector_range_insert_patch.txt --] [-- Type: text/plain, Size: 4834 bytes --] diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc index 36b27dce7b9..5b3c5c2ecd5 100644 --- a/libstdc++-v3/include/bits/vector.tcc +++ b/libstdc++-v3/include/bits/vector.tcc @@ -885,83 +885,78 @@ _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(); + 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; + this->_M_impl._M_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; + this->_M_impl._M_finish = __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; + this->_M_impl._M_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); } } 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); + _Guard_alloc __guard(__new_start, __len, *this); __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); + + __guard._M_storage = __start; + __guard._M_len = size_type(__end - __start); } __catch(...) - { - std::_Destroy(__new_start, __new_finish, - _M_get_Tp_allocator()); - _M_deallocate(__new_start, __len); - __throw_exception_again; - } - std::_Destroy(__old_start, __old_finish, - _M_get_Tp_allocator()); + { + std::_Destroy(__new_start, __new_finish, __allocator); + __throw_exception_again; + } + std::_Destroy(__start, __finish, __allocator); _GLIBCXX_ASAN_ANNOTATE_REINIT; - _M_deallocate(__old_start, - this->_M_impl._M_end_of_storage - __old_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; ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix -Wstringop-overflow warning in 23_containers/vector/types/1.cc 2024-05-30 5:11 ` François Dumont @ 2024-05-30 11:07 ` Jonathan Wakely 2024-06-03 4:56 ` [PATCH] Fix -Wstringop-overflow warning in 23_containers/vector/types/1.cc PR109849 François Dumont 0 siblings, 1 reply; 10+ messages in thread From: Jonathan Wakely @ 2024-05-30 11:07 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On Thu, 30 May 2024 at 06:11, François Dumont <frs.dumont@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; > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix -Wstringop-overflow warning in 23_containers/vector/types/1.cc PR109849 2024-05-30 11:07 ` Jonathan Wakely @ 2024-06-03 4:56 ` François Dumont 2024-06-03 16:20 ` Jonathan Wakely 0 siblings, 1 reply; 10+ messages in thread From: François Dumont @ 2024-06-03 4:56 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 10569 bytes --] I hadn't try to make my patch as limited as possible to fix the problem, indeed. libstdc++: Fix -Wstringop-overflow warning coming from std::vector [PR109849] libstdc++-v3/ChangeLog: PR libstdc++/109849 * include/bits/vector.tcc (std::vector<>::_M_range_insert(iterator, _FwdIt, _FwdIt, forward_iterator_tag)): Add __builtin_unreachable expression to tell the compiler that the allocated buffer is large enough to receive current elements plus the range to insert. Tested under Linux x64, ok to commit ? François On 30/05/2024 13:07, Jonathan Wakely wrote: > On Thu, 30 May 2024 at 06:11, François Dumont <frs.dumont@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; [-- Attachment #2: vector_range_insert_patch.txt --] [-- Type: text/plain, Size: 570 bytes --] diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc index 36b27dce7b9..e5a12c1d608 100644 --- a/libstdc++-v3/include/bits/vector.tcc +++ b/libstdc++-v3/include/bits/vector.tcc @@ -933,6 +933,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER const size_type __len = _M_check_len(__n, "vector::_M_range_insert"); + if (!__builtin_constant_p(__len) && + __len < (__n + (__old_start - __old_finish))) + __builtin_unreachable(); + pointer __new_start(this->_M_allocate(__len)); pointer __new_finish(__new_start); __try ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix -Wstringop-overflow warning in 23_containers/vector/types/1.cc PR109849 2024-06-03 4:56 ` [PATCH] Fix -Wstringop-overflow warning in 23_containers/vector/types/1.cc PR109849 François Dumont @ 2024-06-03 16:20 ` Jonathan Wakely 2024-06-03 17:46 ` François Dumont 0 siblings, 1 reply; 10+ messages in thread From: Jonathan Wakely @ 2024-06-03 16:20 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On Mon, 3 Jun 2024 at 05:56, François Dumont <frs.dumont@gmail.com> wrote: > > I hadn't try to make my patch as limited as possible to fix the problem, > indeed. > > libstdc++: Fix -Wstringop-overflow warning coming from std::vector > [PR109849] > > libstdc++-v3/ChangeLog: > > PR libstdc++/109849 > * include/bits/vector.tcc > (std::vector<>::_M_range_insert(iterator, _FwdIt, _FwdIt, > forward_iterator_tag)): Add __builtin_unreachable > expression to tell > the compiler that the allocated buffer is large enough to > receive current > elements plus the range to insert. > > Tested under Linux x64, ok to commit ? Does the !__builtin_constant_p(__len) in this version do anything? If it's a constant, then the compiler can already provide it's in range, so the __builtin_unreachable() is redundant, but doesn't do any harm. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix -Wstringop-overflow warning in 23_containers/vector/types/1.cc PR109849 2024-06-03 16:20 ` Jonathan Wakely @ 2024-06-03 17:46 ` François Dumont 2024-06-03 19:08 ` Jonathan Wakely 0 siblings, 1 reply; 10+ messages in thread From: François Dumont @ 2024-06-03 17:46 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 2047 bytes --] On 03/06/2024 18:20, Jonathan Wakely wrote: > On Mon, 3 Jun 2024 at 05:56, François Dumont <frs.dumont@gmail.com> wrote: >> I hadn't try to make my patch as limited as possible to fix the problem, >> indeed. >> >> libstdc++: Fix -Wstringop-overflow warning coming from std::vector >> [PR109849] >> >> libstdc++-v3/ChangeLog: >> >> PR libstdc++/109849 >> * include/bits/vector.tcc >> (std::vector<>::_M_range_insert(iterator, _FwdIt, _FwdIt, >> forward_iterator_tag)): Add __builtin_unreachable >> expression to tell >> the compiler that the allocated buffer is large enough to >> receive current >> elements plus the range to insert. >> >> Tested under Linux x64, ok to commit ? > Does the !__builtin_constant_p(__len) in this version do anything? > > If it's a constant, then the compiler can already provide it's in > range, so the __builtin_unreachable() is redundant, but doesn't do any > harm. > Yes, it prevents some constexpr test failure because __builtin_unreachable is not a constexpr (at least not for some C++ Standard versions). But it wasn't a nice way to avoid this regression. Here is another proposal that activate the __builtin_unreachable only for pre-c++11 modes. C++03 had no problem neither but I haven't found any occurrence of __cplusplus checks against the C++03 version so I prefer not to add any. libstdc++: Fix -Wstringop-overflow warning coming from std::vector [PR109849] libstdc++-v3/ChangeLog: PR libstdc++/109849 * include/bits/vector.tcc (std::vector<>::_M_range_insert(iterator, _FwdIt, _FwdIt, forward_iterator_tag))[__cplusplus < 2011103L]: Add __builtin_unreachable expression to tell the compiler that the allocated buffer is large enough to receive current elements plus the elements of the range to insert. Ok to commit ? François [-- Attachment #2: vector_range_insert_patch.txt --] [-- Type: text/plain, Size: 567 bytes --] diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc index 36b27dce7b9..c500aab9e56 100644 --- a/libstdc++-v3/include/bits/vector.tcc +++ b/libstdc++-v3/include/bits/vector.tcc @@ -933,6 +933,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER const size_type __len = _M_check_len(__n, "vector::_M_range_insert"); +#if __cplusplus < 201103LL + if (__len < (__n + (__old_start - __old_finish))) + __builtin_unreachable(); +#endif + pointer __new_start(this->_M_allocate(__len)); pointer __new_finish(__new_start); __try ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix -Wstringop-overflow warning in 23_containers/vector/types/1.cc PR109849 2024-06-03 17:46 ` François Dumont @ 2024-06-03 19:08 ` Jonathan Wakely 0 siblings, 0 replies; 10+ messages in thread From: Jonathan Wakely @ 2024-06-03 19:08 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++, gcc-patches On Mon, 3 Jun 2024 at 18:46, François Dumont <frs.dumont@gmail.com> wrote: > > > On 03/06/2024 18:20, Jonathan Wakely wrote: > > On Mon, 3 Jun 2024 at 05:56, François Dumont <frs.dumont@gmail.com> wrote: > >> I hadn't try to make my patch as limited as possible to fix the problem, > >> indeed. > >> > >> libstdc++: Fix -Wstringop-overflow warning coming from std::vector > >> [PR109849] > >> > >> libstdc++-v3/ChangeLog: > >> > >> PR libstdc++/109849 > >> * include/bits/vector.tcc > >> (std::vector<>::_M_range_insert(iterator, _FwdIt, _FwdIt, > >> forward_iterator_tag)): Add __builtin_unreachable > >> expression to tell > >> the compiler that the allocated buffer is large enough to > >> receive current > >> elements plus the range to insert. > >> > >> Tested under Linux x64, ok to commit ? > > Does the !__builtin_constant_p(__len) in this version do anything? > > > > If it's a constant, then the compiler can already provide it's in > > range, so the __builtin_unreachable() is redundant, but doesn't do any > > harm. > > > Yes, it prevents some constexpr test failure because > __builtin_unreachable is not a constexpr (at least not for some C++ > Standard versions). > > But it wasn't a nice way to avoid this regression. Here is another > proposal that activate the __builtin_unreachable only for pre-c++11 > modes. C++03 had no problem neither but I haven't found any occurrence > of __cplusplus checks against the C++03 version so I prefer not to add any. > > libstdc++: Fix -Wstringop-overflow warning coming from std::vector > [PR109849] > > libstdc++-v3/ChangeLog: > > PR libstdc++/109849 > * include/bits/vector.tcc > (std::vector<>::_M_range_insert(iterator, _FwdIt, _FwdIt, > forward_iterator_tag))[__cplusplus < 2011103L]: Add > __builtin_unreachable > expression to tell the compiler that the allocated buffer > is large enough to > receive current elements plus the elements of the range to > insert. > > Ok to commit ? OK for trunk and gcc-14, thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-06-03 19:08 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-05-27 20:07 [PATCH] Fix -Wstringop-overflow warning in 23_containers/vector/types/1.cc François Dumont 2024-05-27 20:18 ` Sam James 2024-05-27 20:58 ` François Dumont 2024-05-28 10:28 ` Jonathan Wakely 2024-05-30 5:11 ` François Dumont 2024-05-30 11:07 ` Jonathan Wakely 2024-06-03 4:56 ` [PATCH] Fix -Wstringop-overflow warning in 23_containers/vector/types/1.cc PR109849 François Dumont 2024-06-03 16:20 ` Jonathan Wakely 2024-06-03 17:46 ` François Dumont 2024-06-03 19:08 ` Jonathan Wakely
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).