* [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).