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.dum...@gmail.com> wrote:
Looks like this new version works the same to fix the warning without
the issues reported here.

All 23_containers/vector tests run in C++98/14/20 so far.

Ok to commit once I've complete the testsuite (or some bot did it for me
!) ?
There seem to be two unrelated changes here. One is to make the local
variables usable in both branches, but I don't understand why that
matters because the first branch doesn't reallocate, so there's no
call to operator new that would confuse the compiler.

The second change is to add the __builtin_unreachable() to tell the
compiler that __len is correct and did not wrap around. Which should
not be needed because

Are both changes necessary to fix the FAIL for c++98 mode?

I've just done a quick check, and it seems that this smaller change
fixes the FAIL:

--- a/libstdc++-v3/include/bits/vector.tcc
+++ b/libstdc++-v3/include/bits/vector.tcc
@@ -933,6 +933,9 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER

                const size_type __len =
                  _M_check_len(__n, "vector::_M_range_insert");
+               if (__len < (__n + (__old_finish - __old_start)))
+                 __builtin_unreachable();
+
                pointer __new_start(this->_M_allocate(__len));
                pointer __new_finish(__new_start);
                __try

We don't need the rest of the change, which makes sense because the
first branch doesn't reallocate so the compiler doesn't think the
pointers can be invalidated.



I'll look for a PR to associate, if you have one in mind do not hesitate
to tell me.
It's discussed in PR 109849.

François


On 28/05/2024 12:28, Jonathan Wakely wrote:
On 27/05/24 22:07 +0200, François Dumont wrote:
In C++98 this test fails with:

Excess errors:
/home/fdumont/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:452:
warning: 'void* __builtin_memcpy(void*, const void*, long unsigned
int)' writing between 2 and 9223372036854775806 bytes into a region
of size 0 overflows the destination [-Wstringop-overflow=]

The attached patch avoids this warning.

     libstdc++: Fix -Wstringop-overflow warning coming from std::vector

     Make vector<>::_M_range_insert implementation more transparent to
the compiler checks.

     Extend local copies of members to the whole method scope so that
all branches benefit
     from those.

     libstdc++-v3/ChangeLog:

             * include/bits/vector.tcc
             (std::vector<>::_M_range_insert(iterator, _FwdIt, _FwdIt,
forward_iterator_tag)):
             Use local copies of members to call the different
algorithms.

Ok to commit if all tests passes ?

François
diff --git a/libstdc++-v3/include/bits/vector.tcc
b/libstdc++-v3/include/bits/vector.tcc
index 36b27dce7b9..671929dee55 100644
--- a/libstdc++-v3/include/bits/vector.tcc
+++ b/libstdc++-v3/include/bits/vector.tcc
@@ -885,83 +885,80 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       {
     if (__first != __last)
       {
+        // Make local copies of these members because the compiler
+        // thinks the allocator can alter them if 'this' is globally
+        // reachable.
+        pointer __start = this->_M_impl._M_start;
+        pointer __end = this->_M_impl._M_end_of_storage;
+        pointer __finish = this->_M_impl._M_finish;
+        pointer __pos = __position.base();
+        _Tp_alloc_type& __allocator = _M_get_Tp_allocator();
+
+        if (__pos < __start || __finish < __pos)
+          __builtin_unreachable();
I don't think we should use __builtin_unreachable for something which
is not an invariant of the class. The __position argument is supplied
by the user, so we should not make promises about it being valid,
because we can't know that.

We can promise that __start <= __finish, and that __finish <= end,
because we control those. We can't promise the user won't pass in a
bad __position. Although it's undefined for the user to do that, using
__builtin_unreachable() here makes the effects worse, and makes it
harder to debug.

Also, (__pos < __start) might already trigger undefined behaviour for
fancy pointers, if they don't point to the same memory region.

So this change is not OK.


+
         const size_type __n = std::distance(__first, __last);
-        if (size_type(this->_M_impl._M_end_of_storage
-              - this->_M_impl._M_finish) >= __n)
+        if (size_type(__end - __finish) >= __n)
           {
-        const size_type __elems_after = end() - __position;
-        pointer __old_finish(this->_M_impl._M_finish);
+        const size_type __elems_after = __end - __pos;
+        pointer __old_finish(__finish);
         if (__elems_after > __n)
           {
             _GLIBCXX_ASAN_ANNOTATE_GROW(__n);
- std::__uninitialized_move_a(this->_M_impl._M_finish - __n,
-                        this->_M_impl._M_finish,
-                        this->_M_impl._M_finish,
-                        _M_get_Tp_allocator());
-            this->_M_impl._M_finish += __n;
+            __finish = std::__uninitialized_move_a
+              (__finish - __n, __finish, __finish, __allocator);
             _GLIBCXX_ASAN_ANNOTATE_GREW(__n);
-            _GLIBCXX_MOVE_BACKWARD3(__position.base(),
-                        __old_finish - __n, __old_finish);
-            std::copy(__first, __last, __position);
+            _GLIBCXX_MOVE_BACKWARD3
+              (__pos, __old_finish - __n, __old_finish);
+            std::copy(__first, __last, __pos);
           }
         else
           {
             _ForwardIterator __mid = __first;
             std::advance(__mid, __elems_after);
             _GLIBCXX_ASAN_ANNOTATE_GROW(__n);
-            std::__uninitialized_copy_a(__mid, __last,
-                        this->_M_impl._M_finish,
-                        _M_get_Tp_allocator());
-            this->_M_impl._M_finish += __n - __elems_after;
+            __finish = std::__uninitialized_copy_a
+              (__mid, __last, __finish, __allocator);
             _GLIBCXX_ASAN_ANNOTATE_GREW(__n - __elems_after);
-            std::__uninitialized_move_a(__position.base(),
-                        __old_finish,
-                        this->_M_impl._M_finish,
-                        _M_get_Tp_allocator());
-            this->_M_impl._M_finish += __elems_after;
+            __finish = std::__uninitialized_move_a
+              (__pos, __old_finish, __finish, __allocator);
             _GLIBCXX_ASAN_ANNOTATE_GREW(__elems_after);
-            std::copy(__first, __mid, __position);
+            std::copy(__first, __mid, __pos);
           }
+
+        this->_M_impl._M_finish = __finish;
           }
         else
           {
-        // Make local copies of these members because the compiler
-        // thinks the allocator can alter them if 'this' is globally
-        // reachable.
-        pointer __old_start = this->_M_impl._M_start;
-        pointer __old_finish = this->_M_impl._M_finish;
-
+        const size_type __size = size_type(__finish - __start);
         const size_type __len =
           _M_check_len(__n, "vector::_M_range_insert");
+        if (__len < __n + __size)
+          __builtin_unreachable();
+
         pointer __new_start(this->_M_allocate(__len));
         pointer __new_finish(__new_start);
         __try
           {
             __new_finish
               = std::__uninitialized_move_if_noexcept_a
-              (__old_start, __position.base(),
-               __new_start, _M_get_Tp_allocator());
+              (__start, __pos, __new_start, __allocator);
             __new_finish
-              = std::__uninitialized_copy_a(__first, __last,
-                            __new_finish,
-                            _M_get_Tp_allocator());
+              = std::__uninitialized_copy_a
+              (__first, __last, __new_finish, __allocator);
             __new_finish
               = std::__uninitialized_move_if_noexcept_a
-              (__position.base(), __old_finish,
-               __new_finish, _M_get_Tp_allocator());
+              (__pos, __finish, __new_finish, __allocator);
           }
         __catch(...)
           {
-            std::_Destroy(__new_start, __new_finish,
-                  _M_get_Tp_allocator());
+            std::_Destroy(__new_start, __new_finish, __allocator);
             _M_deallocate(__new_start, __len);
             __throw_exception_again;
           }
-        std::_Destroy(__old_start, __old_finish,
-                  _M_get_Tp_allocator());
+        std::_Destroy(__start, __finish, __allocator);
         _GLIBCXX_ASAN_ANNOTATE_REINIT;
-        _M_deallocate(__old_start,
-                  this->_M_impl._M_end_of_storage - __old_start);
+        _M_deallocate(__start, __end - __start);
         this->_M_impl._M_start = __new_start;
         this->_M_impl._M_finish = __new_finish;
         this->_M_impl._M_end_of_storage = __new_start + __len;
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

Reply via email to