[Bug middle-end/98465] New: Bogus warning stringop-overread wuth -std=gnu++20 -O2 and std::string::insert

[Bug middle-end/98465] New: Bogus warning stringop-overread wuth -std=gnu++20 -O2 and std::string::insert

[romain.geissler at amadeus dot com]

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

            Bug ID: 98465
           Summary: Bogus warning stringop-overread wuth -std=gnu++20 -O2
                    and std::string::insert
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: romain.geissler at amadeus dot com
  Target Milestone: ---

Hi,

The following C++ code compiled with -std=gnu++20 -O2 -Werror=stringop-overread
emits a bogus warning/error with gcc trunk, while it works without problem when
using gnu++17 instead. Tested on compiler explorer on x64 Linux:

#include <string>

const char constantString[] = {42, 53};

void f(std::string& s)
{
    s.insert(0, static_cast<const char*>(constantString), 2);
}



In file included from
/opt/compiler-explorer/gcc-trunk-20201228/include/c++/11.0.0/string:40,
                 from <source>:1:
In static member function 'static constexpr std::char_traits<char>::char_type*
std::char_traits<char>::copy(std::char_traits<char>::char_type*, const
char_type*, std::size_t)',
    inlined from 'static void std::__cxx11::basic_string<_CharT, _Traits,
_Alloc>::_S_copy(_CharT*, const _CharT*, std::__cxx11::basic_string<_CharT,
_Traits, _Alloc>::size_type) [with _CharT = char; _Traits =
std::char_traits<char>; _Alloc = std::allocator<char>]' at
/opt/compiler-explorer/gcc-trunk-20201228/include/c++/11.0.0/bits/basic_string.h:351:21,
    inlined from 'static void std::__cxx11::basic_string<_CharT, _Traits,
_Alloc>::_S_copy(_CharT*, const _CharT*, std::__cxx11::basic_string<_CharT,
_Traits, _Alloc>::size_type) [with _CharT = char; _Traits =
std::char_traits<char>; _Alloc = std::allocator<char>]' at
/opt/compiler-explorer/gcc-trunk-20201228/include/c++/11.0.0/bits/basic_string.h:346:7,
    inlined from 'std::__cxx11::basic_string<_CharT, _Traits, _Allocator>&
std::__cxx11::basic_string<_CharT, _Traits,
_Alloc>::_M_replace(std::__cxx11::basic_string<_CharT, _Traits,
_Alloc>::size_type, std::__cxx11::basic_string<_CharT, _Traits,
_Alloc>::size_type, const _CharT*, std::__cxx11::basic_string<_CharT, _Traits,
_Alloc>::size_type) [with _CharT = char; _Traits = std::char_traits<char>;
_Alloc = std::allocator<char>]' at
/opt/compiler-explorer/gcc-trunk-20201228/include/c++/11.0.0/bits/basic_string.tcc:481:20,
    inlined from 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&
std::__cxx11::basic_string<_CharT, _Traits,
_Alloc>::replace(std::__cxx11::basic_string<_CharT, _Traits,
_Alloc>::size_type, std::__cxx11::basic_string<_CharT, _Traits,
_Alloc>::size_type, const _CharT*, std::__cxx11::basic_string<_CharT, _Traits,
_Alloc>::size_type) [with _CharT = char; _Traits = std::char_traits<char>;
_Alloc = std::allocator<char>]' at
/opt/compiler-explorer/gcc-trunk-20201228/include/c++/11.0.0/bits/basic_string.h:1946:19,
    inlined from 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&
std::__cxx11::basic_string<_CharT, _Traits,
_Alloc>::insert(std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::size_type,
const _CharT*, std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::size_type)
[with _CharT = char; _Traits = std::char_traits<char>; _Alloc =
std::allocator<char>]' at
/opt/compiler-explorer/gcc-trunk-20201228/include/c++/11.0.0/bits/basic_string.h:1693:29,
    inlined from 'void f(std::string&)' at <source>:7:60:
/opt/compiler-explorer/gcc-trunk-20201228/include/c++/11.0.0/bits/char_traits.h:402:56:
error: 'void* __builtin_memcpy(void*, const void*, long unsigned int)' reading
2 bytes from a region of size 0 [-Werror=stringop-overread]
  402 |         return static_cast<char_type*>(__builtin_memcpy(__s1, __s2,
__n));
      |                                       
~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
<source>: In function 'void f(std::string&)':
<source>:3:12: note: at offset 2 into source object 'constantString' of size 2
    3 | const char constantString[] = {42, 53};
      |            ^~~~~~~~~~~~~~
cc1plus: some warnings being treated as errors
Compiler returned: 1


Note that gcc 10 doesn't issue any warning even with -Wall -Wextra with
gnu++20.

Cheers,
Romain

[Bug middle-end/98465] Bogus warning stringop-overread wuth -std=gnu++20 -O2 and std::string::insert

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |msebor at gcc dot gnu.org
           Keywords|                            |diagnostic,
                   |                            |missed-optimization
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
             Blocks|                            |97048
   Last reconfirmed|                            |2020-12-28
      Known to fail|                            |10.2.0, 11.0

--- Comment #1 from Martin Sebor <msebor at gcc dot gnu.org> ---
I'm not sure why I don't see the warning for the test case in my build but I
can reproduce it (as well as a -Warray-bounds) by compiling the translation
unit obtained from the test case and with system header markers removed with
either GCC 10 or 11:

$ g++ -O2 -S -Wall -std=gnu++20 pr98465.C
pr98465.C: In function ‘void f(std::string&)’:
pr98465.C:22316:20: warning: offset ‘2’ outside bounds of constant string
[-Warray-bounds]
22316 |       this->_S_copy(__p, __s + __len2 - __len1, __len2);
      |       ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pr98465.C:22843:12: note: ‘constantString’ declared here
22843 | const char constantString[] = {42, 53};
      |            ^~~~~~~~~~~~~~
In static member function ‘static constexpr std::char_traits<char>::char_type*
std::char_traits<char>::copy(std::char_traits<char>::char_type*, const
char_type*, std::size_t)’,
    inlined from ‘void f(std::string&)’ at pr98465.C:19983:21:
pr98465.C:9051:49: warning: ‘void* __builtin_memcpy(void*, const void*, long
unsigned int)’ reading 2 bytes from a region of size 0 [-Wstringop-overread]
 9051 |  return static_cast<char_type*>(__builtin_memcpy(__s1, __s2, __n));
      |                                 ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
pr98465.C: In function ‘void f(std::string&)’:
pr98465.C:22843:12: note: at offset 2 into source object ‘constantString’ of
size 2
22843 | const char constantString[] = {42, 53};
      |            ^~~~~~~~~~~~~~

The -Warray-bounds instance comes from a call to c_strlen() made during the
ccp2 pass for the following IL:

void f (struct string & s)
{
  _14 = MEM[(const struct basic_string *)s_2(D)]._M_dataplus._M_p;
  ...
  <bb 22> [local count: 179779816]:
  if (_14 >= &MEM <const char[2]> [(void *)&constantString + 2B])
    goto <bb 23>; [50.00%]
  else
    goto <bb 24>; [50.00%]
  ...
  <bb 24> [local count: 44944954]:
  if (_14 <= &constantString)
    goto <bb 25>; [50.00%]
  else
    goto <bb 26>; [50.00%]

  <bb 25> [local count: 22472477]:
  __builtin_memcpy (_14, &MEM <const char[2]> [(void *)&constantString + 2B],
2);   <<< -Warray-bounds
  goto <bb 55>; [100.00%]
  ...
}

The warning is correct: the call is invalid.  It results from the following
transformations:

Folding statement: _45 = 2 - _6;
Queued stmt for removal.  Folds to: 2
Folding statement: _46 = &constantString + _45;
Queued stmt for removal.  Folds to: &MEM <const char[2]> [(void
*)&constantString + 2B]
Folding statement: __builtin_memcpy (__p_18, _46, 2);
Folded into: __builtin_memcpy (_14, &MEM <const char[2]> [(void
*)&constantString + 2B], 2);

The invalid call survives until expansion which is when the -Wstringop-overread
is issued.  I suspect the default -Wno-system-headers setting has something to
do with the warning being masked.

In the IL above, the pointer inequalities that lead to the transformation are
the result of the call to _M_disjunct(__s) in std::string::_M_replace,
presumably done to identify and cope with self-insertion.  Since constantString
is a distinct object, the invalid call is unreachable, but without more context
GCC can't figure that out.  Annotating the std::string::_M_dataplus._M_p
pointer as one that cannot alias a declared object (other than the _M_local_buf
member) would be one way to avoid the warning (and improve the codegen at the
same time).  IIRC, I suggested something like that in the past but no nothing
has materialized yet.


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97048
[Bug 97048] [meta-bug] bogus/missing -Wstringop-overread warnings

[Bug middle-end/98465] Bogus warning stringop-overread wuth -std=gnu++20 -O2 and std::string::insert

[romain.geissler at amadeus dot com]

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

--- Comment #2 from Romain Geissler <romain.geissler at amadeus dot com> ---
Hi Martin,

Thanks for your investigation.

I have a few questions:
 - Since the warning seems to be fully emitted by system headers, shouldn't it
be silenced by default ? Why isn't it the case here ? On compiler explorer,
"x86-64 gcc 10.2" and flags "-std=gnu++20 -O2" I get no warning, yet with flags
"-std=gnu++20 -O2 -Wsystem-headers" I get a -Wstringop-overflow warning about
the same problem. Meaning that for stringop-overflow system headers seems to be
taken into account, but not for stringop-overread, is that expected ?
 - I understood why you did not reproduce the bug with gcc 11, my test case,
and flags "-std=gnu++20 -O2". It looks like Compiler explorer forces "-g" by
default. And it seems debug output generation does affect the warning. With
flags "-std=gnu++20 -O2 -g0" (effectively disable debug information generation)
I get no warning, while with "-std=gnu++20 -O2 -g" I get the stringop-overread
warning. Again, gcc 10 doesn't seem to be impacted by debug output generation
to trigger this warning, is this expected ?

There seems to be a strange interaction between -Wsystem-headers and -g in gcc
11 which I don't understand. See the following matrix:
 - "-std=gnu++20 -O2 -Wno-system-headers -g0" --> no warning
 - "-std=gnu++20 -O2 -Wno-system-headers -g"  --> warning
 - "-std=gnu++20 -O2 -Wsystem-headers -g0"    --> warning
 - "-std=gnu++20 -O2 -Wsystem-headers -g"     --> warning

(this last matrix was tested on compiler explorer).

I am confused.

Cheers,
Romain

[Bug middle-end/98465] Bogus warning stringop-overread wuth -std=gnu++20 -O2 and std::string::insert

[romain.geissler at amadeus dot com]

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

--- Comment #3 from Romain Geissler <romain.geissler at amadeus dot com> ---
I have found another example in my code base raising this error:

#include <string>

std::string f1()
{
    std::string aString = "string";
    aString = "bigger str";

    return aString;
}

With: -O2 -std=gnu++20 -Werror=stringop-overread -Wno-system-headers -g
raises:

/opt/compiler-explorer/gcc-trunk-20201230/include/c++/11.0.0/bits/char_traits.h:402:56:
error: 'void* __builtin_memcpy(void*, const void*, long unsigned int)' reading
10 bytes from a region of size 7 [-Werror=stringop-overread]
  402 |         return static_cast<char_type*>(__builtin_memcpy(__s1, __s2,
__n));
      |                                       
~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~

Yet now what confuses me is that if I add copy of this function later using
bigger strings (so not exposing the bug), the new f2 function makes the warning
disappear form the f1 function:

#include <string>

std::string f1()
{
    std::string aString = "string";
    aString = "bigger str"; // <--- no more warning here when we have function
f2 !

    return aString;
}

std::string f2()
{
    std::string aString = "initial string with enough capacity";
    aString = "smaller str";

    return aString;
}

Why does having a function f2 affects warnings in function f1 ?

[Bug middle-end/98465] Bogus warning stringop-overread wuth -std=gnu++20 -O2 and std::string::insert

[msebor at gcc dot gnu.org]

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

--- Comment #4 from Martin Sebor <msebor at gcc dot gnu.org> ---
(In reply to Romain Geissler from comment #2)
> There seems to be a strange interaction between -Wsystem-headers and -g in
> gcc 11 which I don't understand.

Thanks for the -g hint; with it I can see it on my end as well.  The difference
seems to be in how the inlining information is encoded with -g vs without. 
With -g, the algorithm that looks for the location into which the call to
s.insert() has been inlined manages to uncover it.  Without -g, the algorithm
fails and falls back on the traditional approach that only considers macro
expansion but not inlining.  So without -g, the warning is not issued because
of a bug or limitation in the new algorithm.  (The algorithm was introduced in
r11-6028 to enable -Wfree-nonheap-object and other similar middle-end warnings
for invalid uses of C++ standard library functions.  The goal is to avoid
issuing warnings for deliberate abuses by system headers, but we want to issue
those for incidental misuses of system functions by user code.)

(In reply to Romain Geissler from comment #3)
> Why does having a function f2 affects warnings in function f1 ?

Because s.insert() is a trivial wrapper around replace(), calls to it end up
expanded inline into those to s._M_replace().  When there's just one caller of
s1.insert(), the latter is inlined into it as well.  But with two or more
callers, because _M_replace() is big, the inliner decides it's better not to
expand it inline.  That in turn prevents constant propagation from substituting
the constant array's address into the code, which then defeats the warning. 
The inlining limit is controlled by -finline-limit=n so suitably increasing it
will trigger the warning.

[Bug middle-end/98465] Bogus warning stringop-overread wuth -std=gnu++20 -O2 and std::string::insert

[msebor at gcc dot gnu.org]

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

--- Comment #5 from Martin Sebor <msebor at gcc dot gnu.org> ---
Comparing the code emitted by GCC 10 and the current trunk shows that the
former doesn't inline std::string::replace() regardless of the inlining limit. 
The translation unit obtained with the former shows declarations of explicit
instantiations of basic_string:

$ g++-10 -O2 -E -Wall -g t.C | grep template | grep basic_string
      template<typename, typename, typename> friend class basic_stringbuf;
  extern template class basic_string<char>;
  extern template class basic_string<wchar_t>;

Current trunk doesn't declare them which is what allows std::string::replace to
be inlined.

Removing the declaration of the explicit instantiation from the translation
unit lets GCC 10 inline string::replace but the warning is not issued because
the location of the call is in a system header.  But enabling warning for those
via -Wsystem-headers shows the warning even with GCC 10:

$ g++-10 -E pr98465.C | sed "/extern template class basic_string/d" >
pr98465.ii && g++-10 -O2 -S -Wall -Wsystem-headers pr98465.ii
In file included from
/build/gcc-10-branch/x86_64-pc-linux-gnu/libstdc++-v3/include/string:56,
                 from pr98465.C:1:
/build/gcc-10-branch/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:
In function ‘void f(std::string&)’:
/build/gcc-10-branch/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:456:20:
warning: offset ‘2’ outside bounds of constant string [-Warray-bounds]
  456 |       this->_S_copy(__p, __s + __len2 - __len1, __len2);
      |       ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pr98465.C:3:12: note: ‘constantString’ declared here
    3 | const char constantString[] = {42, 53};
      |            ^~~~~~~~~~~~~~
In file included from
/build/gcc-10-branch/x86_64-pc-linux-gnu/libstdc++-v3/include/string:40,
                 from pr98465.C:1:
In static member function ‘static std::char_traits<char>::char_type*
std::char_traits<char>::copy(std::char_traits<char>::char_type*, const
char_type*, std::size_t)’,
    inlined from ‘void f(std::string&)’ at
/build/gcc-10-branch/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.h:351:21:
/build/gcc-10-branch/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/char_traits.h:395:49:
warning: ‘void* __builtin_memcpy(void*, const void*, long unsigned int)’
reading 2 bytes from a region of size 0 [-Wstringop-overflow=]
  395 |  return static_cast<char_type*>(__builtin_memcpy(__s1, __s2, __n));
      |                                 ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~

[Bug middle-end/98465] Bogus warning stringop-overread wuth -std=gnu++20 -O2 and std::string::insert

[msebor at gcc dot gnu.org]

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

--- Comment #6 from Martin Sebor <msebor at gcc dot gnu.org> ---
Different -std= settings also affect the warning, again as a result of inlining
and the explicit instantiation declarations.  The basic_string.tcc file has the
following block toward the end which explains when implicit instantiation is
suppressed:

  // Inhibit implicit instantiations for required instantiations,
  // which are defined via explicit instantiations elsewhere.
#if _GLIBCXX_EXTERN_TEMPLATE
  // The explicit instantiation definitions in src/c++11/string-inst.cc and
  // src/c++17/string-inst.cc only instantiate the members required for C++17
  // and earlier standards (so not C++20's starts_with and ends_with).
  // Suppress the explicit instantiation declarations for C++20, so C++20
  // code will implicitly instantiate std::string and std::wstring as needed.
# if __cplusplus <= 201703L && _GLIBCXX_EXTERN_TEMPLATE > 0
  extern template class basic_string<char>;
# elif ! _GLIBCXX_USE_CXX11_ABI
  // Still need to prevent implicit instantiation of the COW empty rep,
  // to ensure the definition in libstdc++.so is unique (PR 86138).
  extern template basic_string<char>::size_type
    basic_string<char>::_Rep::_S_empty_rep_storage[];
# endif

[Bug middle-end/98465] Bogus warning stringop-overread wuth -std=gnu++20 -O2 and std::string::insert

[msebor at gcc dot gnu.org]

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

--- Comment #7 from Martin Sebor <msebor at gcc dot gnu.org> ---
Adding #pragma GCC diagnostic ignore "-Wstringop-overread" to
string::_M_replace(size_type, size_type, const _CharT*, const size_type)
doesn't suppress the warning, either on trunk, or (with "-Wstringop-overflow"
because GCC 10 doesn't have -Wstringop-overread) for GCC 10.

[Bug middle-end/98465] Bogus warning stringop-overread wuth -std=gnu++20 -O2 and std::string::insert

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=93971

--- Comment #8 from Martin Sebor <msebor at gcc dot gnu.org> ---
Pr93971 discusses the aliasing problem in general (all containers) and specific
to std::string.

[Bug middle-end/98465] Bogus warning stringop-overread wuth -std=gnu++20 -O2 and std::string::insert

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
   Target Milestone|---                         |11.0
           Priority|P3                          |P2
           Keywords|                            |alias
           Assignee|unassigned at gcc dot gnu.org      |msebor at gcc dot gnu.org

--- Comment #9 from Martin Sebor <msebor at gcc dot gnu.org> ---
Bumping up Priority to 2 since this is causing Fedora build failures (with
-Werror) in a number of packages.  I can't think of a better near term
fix/workaround (for GCC 11) than to revert the change enabling the
-Wstringop-xxx warnings for memcpy/memmove calls in system headers inlined into
user code.  This won't eliminate the warning completely, just suppress it when
-Wno-system-headers is in effect (which is by default), and restore things to
their pre-GCC 11 status.

My idea for a longer term solution is to add some source-level annotation to
containers like std::string to either let GCC eliminate the unreachable code
altogether, or let the warning determine the code is, in fact, unreachable. 
For reference, here's a brief exchange we had on this subject:
  https://gcc.gnu.org/pipermail/gcc/2021-January/234641.html

[Bug middle-end/98465] Bogus warning stringop-overread wuth -std=gnu++20 -O2 and std::string::insert

[law at redhat dot com]

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

Jeffrey A. Law <law at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |law at redhat dot com

--- Comment #10 from Jeffrey A. Law <law at redhat dot com> ---
What about fixing the -g interaction?  Much like how -g shouldn't affect code
generation, it probably shouldn't be affecting warnings like this.

There may be more we can/should do here, but that seems like a good place to
start.

[Bug middle-end/98465] Bogus warning stringop-overread wuth -std=gnu++20 -O2 and std::string::insert

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Yeah, -g shouldn't affect which warnings are emitted either.
If you now all newly rely on some BLOCKs being preserved, then you actually
need to make sure they are not thrown away - see tree-ssa-live.c.

[Bug middle-end/98465] Bogus warning stringop-overread wuth -std=gnu++20 -O2 and std::string::insert

[msebor at gcc dot gnu.org]

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

--- Comment #12 from Martin Sebor <msebor at gcc dot gnu.org> ---
(In reply to Jeffrey A. Law from comment #10)
> What about fixing the -g interaction?  Much like how -g shouldn't affect
> code generation, it probably shouldn't be affecting warnings like this.
> 
> There may be more we can/should do here, but that seems like a good place to
> start.

Agreed, I'm looking into that too.  Fixing it won't avoid the warning though,
just make it more consistent (see comment #4).

[Bug middle-end/98465] Bogus warning stringop-overread wuth -std=gnu++20 -O2 and std::string::insert

[jakub at gcc dot gnu.org]

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

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Note, it is important to be able for -g0 to be able to optimize as many BLOCKs
as possible especially for LTO memory consumptions, so it should be just the
BLOCKs at inline boundaries that could be significant to diagnostics.

[Bug middle-end/98465] Bogus warning stringop-overread wuth -std=gnu++20 -O2 and std::string::insert

[msebor at gcc dot gnu.org]

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

--- Comment #14 from Martin Sebor <msebor at gcc dot gnu.org> ---
Smallish test case independent of libstdc++ that reproduces both the false
positive (due to the missing aliasing constraint) and its absence (due to a
bug/limitation in tree_inlined_location).  With -Wsystem-headers GCC issues two
instances of the false positive, one for f() and the other for g():

$ cat pr98465.C && gcc -O2 -S -Wall -Wextra  pr98465.C
# 1 "pr98465.h" 1 3

typedef __SIZE_TYPE__    size_type;
typedef __UINTPTR_TYPE__ uintptr_t;

struct S {
  char *dta;
  size_type cap, siz;

  bool _M_disjunct (const char *s) const {
    return ((uintptr_t)s < (uintptr_t)dta
            || (uintptr_t)dta + siz < (uintptr_t)s);
  }

  void assign (const char *s, size_type n) {
    assign2 (s, n);
  }

  void assign2 (const char *s, size_type n) {
    _M_replace (0, siz, s, n);
  }

  void _M_replace (size_type pos, size_type len1, const char* s,
                   const size_type len2) {
      const size_type old_size = siz;
      const size_type new_size = old_size + len2 - len1;

      if (new_size <= cap)
        {
          char *p = dta + pos;

          const size_type how_much = old_size - pos - len1;
          if (_M_disjunct (s))
            {
              if (how_much && len1 != len2)
                __builtin_memmove (p + len2, p + len1, how_much);
              if (len2)
                __builtin_memcpy (p, s, len2);
            }
          else
            {
              if (len2 && len2 <= len1)
                __builtin_memmove (p, s, len2);
              if (how_much && len1 != len2)
                __builtin_memmove (p + len2, p + len1, how_much);
              if (len2 > len1)
                {
                  if (s + len2 <= p + len1)
                    __builtin_memmove (p, s, len2);
                  else if (s >= p + len1)
                    __builtin_memcpy (p, s + len2 - len1, len2);
                  else
                    {
                      const size_type nleft = (p + len1) - s;
                      __builtin_memmove (p, s, nleft);
                      __builtin_memcpy (p + nleft, p + len2,
                                        len2 - nleft);
                    }
                }
            }
        }
    }
};

# 1 "pr98465.C"

const char a[] = { 1, 2 };

void f (S &s)
{
  s.assign (a, 2);      // no warning
}

const char b[] = { 2, 3 };

void g (S &s)
{
  s.assign2 (b, 2);     // bogus -Wstringop-overread
}
In file included from pr98465.C:1:
In member function ‘void S::_M_replace(size_type, size_type, const char*,
size_type)’,
    inlined from ‘void S::assign2(const char*, size_type)’ at pr98465.h:19:16,
    inlined from ‘void g(S&)’ at pr98465.C:13:13:
pr98465.h:50:24: warning: ‘void* __builtin_memcpy(void*, const void*, long
unsigned int)’ reading 2 bytes from a region of size 1 [-Wstringop-overread]
In file included from pr98465.C:1:
pr98465.C: In function ‘void g(S&)’:
pr98465.C:9:12: note: at offset [1, 2] into source object ‘b’ of size 2
    9 | 
      |            ^

[Bug middle-end/98465] Bogus warning stringop-overread wuth -std=gnu++20 -O2 and std::string::insert

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=98664

--- Comment #15 from Martin Sebor <msebor at gcc dot gnu.org> ---
I created pr98664 to track just the -g interaction.

[Bug middle-end/98465] Bogus warning stringop-overread wuth -std=gnu++20 -O2 and std::string::insert

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch

--- Comment #16 from Martin Sebor <msebor at gcc dot gnu.org> ---
Patch to suppress the false positives in std::string:
https://gcc.gnu.org/pipermail/gcc-patches/2021-January/563862.html

[Bug middle-end/98465] [11 Regression] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Bogus warning               |[11 Regression] Bogus
                   |stringop-overread wuth      |-Wstringop-overread with
                   |-std=gnu++20 -O2 and        |-std=gnu++20 -O2 and
                   |std::string::insert         |std::string::insert

--- Comment #17 from Martin Sebor <msebor at gcc dot gnu.org> ---
Emitting the warning even with -Wno-system-headers is technically a regression.

[Bug middle-end/98465] [11 Regression] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #18 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So, if adding some attribute or whatever to std::string etc. members doesn't
look doable/desirable, can't we e.g. implement _M_disjunct using some builtin
that would allow gcc to optimize it better (and later fold it into how it is
implemented now)?

It is currently:

      // True if _Rep and source do not overlap.
      bool
      _M_disjunct(const _CharT* __s) const _GLIBCXX_NOEXCEPT
      {
        return (less<const _CharT*>()(__s, _M_data())
                || less<const _CharT*>()(_M_data() + this->size(), __s));
      }

and when std::basic_string uses the normal allocators, _M_dataplus._M_p
can point either to some heap allocated object or to _M_local_buf inside of
*this.
So, can some template stuff ensure that the builtin would be only used when
using a standard allocator and not something that can say have a global:
char buffer[0x10000000]; and allocate from that?
Pass to the builtin all the needed info (e.g.
return __builtin_whatever_disjunct (_M_data(), this->size(), _M_local_data(),
__s);
) and let it use points to info to fold it to false if the last pointer can't
point to either _M_local_data() pointed object or heap memory, otherwise fold
into pointer sized casts of the pointers and integral comparisons as before?

[Bug middle-end/98465] [11 Regression] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[redi at gcc dot gnu.org]

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

--- Comment #19 from Jonathan Wakely <redi at gcc dot gnu.org> ---
A new built-in seems like a very large hammer to solve this problem, which
should really be implementable in quite simple pure C++.

(In reply to Jakub Jelinek from comment #18)
> So, can some template stuff ensure that the builtin would be only used when
> using a standard allocator and not something that can say have a global:
> char buffer[0x10000000]; and allocate from that?

Yes, that's pretty easy. Just check it's std::allocator and the char_type is
one of char, wchar_t, char16_t etc.

Ugly, but easy.

> Pass to the builtin all the needed info (e.g.
> return __builtin_whatever_disjunct (_M_data(), this->size(),
> _M_local_data(), __s);
> ) and let it use points to info to fold it to false if the last pointer
> can't point to either _M_local_data() pointed object or heap memory,
> otherwise fold into pointer sized casts of the pointers and integral
> comparisons as before?

But isn't it going to be fairly common that __s points to heap memory? Or do we
not care about that case, because the false positive warning only happens for
non-heap memory?

Tangentially, maybe we could improve _M_disjunct by passing it the length of
__s, which all callers have available. If that length is larger than
this->size() then we don't need the pointer comparisons. If __s points into the
string, then it can't extend past the end of the string.

Would the built-in be simpler and/or more useful elsewhere if it took two
pointers and two lengths, and determined if one region is entirely contained
within the other?

[Bug middle-end/98465] [11 Regression] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[jakub at gcc dot gnu.org]

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

--- Comment #20 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
What I meant this as was a variant to Martin's
the https://gcc.gnu.org/pipermail/gcc/2021-January/234641.html
idea.
Or perhaps add an attribute on _M_dataplus._M_p member that would tell the
compiler about this special behavior (the containing class owns the pointer,
and it can point either to a heap allocated memory that is owned by the class,
or
can point into a local buffer within the same object as the pointer,
and let have the new *disjunct builtin be for now the only consumer of that
attribute.  It could then handle both the case where the _M_replace __s points
to a variable that clearly can't be owned by the string object, but perhaps
also when passed pointers to other string objects (it would see, the other
pointer is loaded from _M_dataplus._M_p that has this new magic attribute, so
the containing object owns that pointer, and if points-to analysis finds out it
can't alias with the other owning object, it could fold the disjunct builtin to
false too.

The problem is that the else part in if (_M_disjunct(...)) { ... } else { ... }
is large, complicated and prone to these false positive warnings, so it is
desirable to fold _M_disjunct to compile time false as much as possible.

I have really no idea how often in real-world code people actually pass parts
of the destination string as the source (i.e. how often _M_disjunct returns
true).
Maybe if it is very rare another alternative would be avoid inlining that
(either by moving it into a separate method with noinline, cold attributes or
perhaps to a helper function in libstdc++ library.

[Bug middle-end/98465] [11 Regression] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[redi at gcc dot gnu.org]

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

--- Comment #21 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Or we could just use #pragma to disable the warning around that function.

[Bug middle-end/98465] [11 Regression] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[redi at gcc dot gnu.org]

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

--- Comment #22 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I think it is rare for _M_disjunct to return false, most strings being
appended/inserted are disjunct from the string itself.

[Bug middle-end/98465] [11 Regression] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[msebor at gcc dot gnu.org]

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

--- Comment #23 from Martin Sebor <msebor at gcc dot gnu.org> ---
Having a built-in sit on top of a (for now internal?) attribute with the goal
of eventually exposing the attribute makes sense to me, although neither seems
suitable to me for stage 4.

The patch I posted can be used to suppress the warning until then.  The patch
is needed in any case to solve pr98512 and pr98871.  Without the patch the
#pragma in libstdc++ doesn't work.

As an aside, an attribute that constrains aliasing won't just help std::string.
 It will improve the codegen for any container (see pr93971).

[Bug middle-end/98465] [11 Regression] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[jakub at gcc dot gnu.org]

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

--- Comment #24 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Ok, so for GCC 11, can we just help the optimizers a little bit and at the same
time get rid of the warning?
Like:
--- libstdc++-v3/include/bits/basic_string.tcc.jj       2021-01-04
10:26:02.930960956 +0100
+++ libstdc++-v3/include/bits/basic_string.tcc  2021-02-04 17:44:10.592843195
+0100
@@ -477,7 +477,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
                {
                  if (__s + __len2 <= __p + __len1)
                    this->_S_move(__p, __s, __len2);
+#if _GLIBCXX_HAS_BUILTIN(__builtin_object_size) && defined(__OPTIMIZE__)
+                 /* Help the optimizers rule out impossible cases and
+                    get rid of false positive warnings at the same time.
+                    If we know the maximum size of the __s object and
+                    it is shorter than 2 * __len2 - __len1, then
+                    __s >= __p + __len1 case is impossible.  */
+                 else if (!(__builtin_constant_p((2 * __len2 - __len1)
+                                                 * sizeof(_CharT))
+                            && __builtin_object_size(__s, 0)
+                               < (2 * __len2 - __len1) * sizeof(_CharT))
+                          && __s >= __p + __len1)
+#else
                  else if (__s >= __p + __len1)
+#endif
                    this->_S_copy(__p, __s + __len2 - __len1, __len2);
                  else
                    {
I don't get the warning anymore and function size on #c0 testcase shrunk with
-O2 -std=gnu++20 -Wall from 614 bytes to 568.
This code is handling the non-disjunct case, and when __s >= __p + __len1 and
__len1 < __len2, we need to copy from (__s + __len2 - __len1) __len2, i.e. have
access up to __s + 2 * __len2 - __len1 - 1.  If we know that is certainly out
of bounds, most likely it isn't the non-disjunct case at all, but even if it
would, this copying wouldn't be a valid option.

[Bug middle-end/98465] [11 Regression] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[jakub at gcc dot gnu.org]

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

--- Comment #25 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #22)
> I think it is rare for _M_disjunct to return false, most strings being
> appended/inserted are disjunct from the string itself.

If it is really rare, then moving it out of line might be beneficial.  It will
slow down the non-disjunct case, not only because of a (possibly PLT) call, but
also because when it isn't inlined, e.g. for constant __len1 and/or __len2 the
various comparisons can't be constant folded.  But it will make code smaller.

As a proof of concept, I've hacked up the preprocessed source:
--- pr98465.ii  2021-02-04 14:09:29.230049287 +0100
+++ pr98465-5.ii        2021-02-04 18:08:19.784739149 +0100
@@ -26229,6 +26229,10 @@ namespace __cxx11 {
       basic_string&
       _M_append(const _CharT* __s, size_type __n);

+      [[gnu::noinline, gnu::cold]] void
+      _M_replace_cold(size_type __pos, size_type __len1, const _CharT* __s,
+   const size_type __len2);
+
     public:
 # 2285
"/opt/compiler-explorer/gcc-trunk-20210204/include/c++/11.0.0/bits/basic_string.h"
3
       size_type
@@ -28220,31 +28224,15 @@ namespace std __attribute__ ((__visibili
     }

   template<typename _CharT, typename _Traits, typename _Alloc>
-    basic_string<_CharT, _Traits, _Alloc>&
+    void
     basic_string<_CharT, _Traits, _Alloc>::
-    _M_replace(size_type __pos, size_type __len1, const _CharT* __s,
+    _M_replace_cold(size_type __pos, size_type __len1, const _CharT* __s,
         const size_type __len2)
     {
-      _M_check_length(__len1, __len2, "basic_string::_M_replace");
+      pointer __p = this->_M_data() + __pos;

       const size_type __old_size = this->size();
-      const size_type __new_size = __old_size + __len2 - __len1;
-
-      if (__new_size <= this->capacity())
- {
-   pointer __p = this->_M_data() + __pos;
-
-   const size_type __how_much = __old_size - __pos - __len1;
-   if (_M_disjunct(__s))
-     {
-       if (__how_much && __len1 != __len2)
-  this->_S_move(__p + __len2, __p + __len1, __how_much);
-       if (__len2)
-  this->_S_copy(__p, __s, __len2);
-     }
-   else
-     {
-
+      const size_type __how_much = __old_size - __pos - __len1;
        if (__len2 && __len2 <= __len1)
   this->_S_move(__p, __s, __len2);
        if (__how_much && __len1 != __len2)
@@ -28263,7 +28251,34 @@ namespace std __attribute__ ((__visibili
         __len2 - __nleft);
       }
   }
+  }
+
+  template<typename _CharT, typename _Traits, typename _Alloc>
+    basic_string<_CharT, _Traits, _Alloc>&
+    basic_string<_CharT, _Traits, _Alloc>::
+    _M_replace(size_type __pos, size_type __len1, const _CharT* __s,
+        const size_type __len2)
+    {
+      _M_check_length(__len1, __len2, "basic_string::_M_replace");
+
+      const size_type __old_size = this->size();
+      const size_type __new_size = __old_size + __len2 - __len1;
+
+      pointer __p = this->_M_data() + __pos;
+
+      const size_type __how_much = __old_size - __pos - __len1;
+
+      if (__new_size <= this->capacity())
+ {
+   if (_M_disjunct(__s))
+     {
+       if (__how_much && __len1 != __len2)
+  this->_S_move(__p + __len2, __p + __len1, __how_much);
+       if (__len2)
+  this->_S_copy(__p, __s, __len2);
      }
+   else
+       _M_replace_cold(__pos, __len1, __s, __len2);
  }
       else
  this->_M_mutate(__pos, __len1, __s, __len2);

Without this patch, _Z1fRNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE
has 614 bytes,
with the patch there is:
399 bytes of _Z1fRNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE + 31 of
_Z1fRNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE.cold (caller of
_M_replace_cold moved into cold section) and 317 bytes
_ZNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEE15_M_replace_coldEmmPKcm. 
But that one can be moved to libstdc++ and shared there.

[Bug middle-end/98465] [11 Regression] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[msebor at gcc dot gnu.org]

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

--- Comment #26 from Martin Sebor <msebor at gcc dot gnu.org> ---
I tried the __builtin_object_size patch and while it avoids the warning in the
reported test case it's not effective in others derived from it, such as:

#include <string>

const char constantString[] = {42, 53};

void f(std::string& s, int n)
{
  if (n < 2) n = 2;
  s.insert(0, static_cast<const char*>(constantString), n);
}

[Bug middle-end/98465] [11 Regression] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[jakub at gcc dot gnu.org]

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

--- Comment #27 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Martin Sebor from comment #26)
> I tried the __builtin_object_size patch and while it avoids the warning in
> the reported test case it's not effective in others derived from it, such as:
> 
> #include <string>
> 
> const char constantString[] = {42, 53};
> 
> void f(std::string& s, int n)
> {
>   if (n < 2) n = 2;
>   s.insert(0, static_cast<const char*>(constantString), n);
> }

That could be handled by doing:
__builtin_constant_p (__builtin_object_size (__s, 0) < (2 * __len2 - __len1) *
sizeof(_CharT))
&& __builtin_object_size (__s, 0) < (2 * __len2 - __len1) * sizeof(_CharT)
assuming VRP is able to determine the comparison is constant even when one of
the lengths are non-constant.

[Bug middle-end/98465] [11 Regression] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[jakub at gcc dot gnu.org]

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

--- Comment #28 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Actually tested version.

The above testcase with [2, INF] range doesn't make really much sense,
but adjusted testcase where n has [0, 2] range doesn't warn anymore like the
one with constant 2.

diff --git a/libstdc++-v3/include/bits/c++config
b/libstdc++-v3/include/bits/c++config
index b57ff339990..69336a32bc6 100644
--- a/libstdc++-v3/include/bits/c++config
+++ b/libstdc++-v3/include/bits/c++config
@@ -731,6 +731,10 @@ namespace std
 # define _GLIBCXX_HAVE_BUILTIN_LAUNDER 1
 #endif

+#if _GLIBCXX_HAS_BUILTIN(__builtin_object_size)
+# define _GLIBCXX_HAVE_BUILTIN_OBJECT_SIZE 1
+#endif
+
 #undef _GLIBCXX_HAS_BUILTIN

 #if _GLIBCXX_HAVE_BUILTIN_IS_CONSTANT_EVALUATED && __cplusplus >= 201402L
diff --git a/libstdc++-v3/include/bits/basic_string.tcc
b/libstdc++-v3/include/bits/basic_string.tcc
index 5beda8b829b..bc6e0b98186 100644
--- a/libstdc++-v3/include/bits/basic_string.tcc
+++ b/libstdc++-v3/include/bits/basic_string.tcc
@@ -477,7 +477,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
                {
                  if (__s + __len2 <= __p + __len1)
                    this->_S_move(__p, __s, __len2);
+#if defined(_GLIBCXX_HAVE_BUILTIN_OBJECT_SIZE) && defined(__OPTIMIZE__)
+                 /* Help the optimizers rule out impossible cases and
+                    get rid of false positive warnings at the same time.
+                    If we know the maximum size of the __s object and
+                    it is shorter than 2 * __len2 - __len1, then
+                    __s >= __p + __len1 case is impossible.  */
+                 else if (!(__builtin_constant_p(__builtin_object_size(__s, 0)
+                                                 < ((2 * __len2 - __len1)
+                                                    * sizeof(_CharT)))
+                            && (__builtin_object_size(__s, 0)
+                                < (2 * __len2 - __len1) * sizeof(_CharT)))
+                          && __s >= __p + __len1)
+#else
                  else if (__s >= __p + __len1)
+#endif
                    this->_S_copy(__p, __s + __len2 - __len1, __len2);
                  else
                    {

[Bug middle-end/98465] [11 Regression] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[jakub at gcc dot gnu.org]

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

--- Comment #29 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 50145
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50145&action=edit
gcc11-pr98465.patch

Yet another libstdc++ change that makes the warning go away, but doesn't
actually change the generated code - in GIMPLE we end up with more stmts than
before but RTL optimizations once we lose distinction on what is a pointer and
what is just an offset added to it optimize those differences away.

[Bug middle-end/98465] [11 Regression] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[cvs-commit at gcc dot gnu.org]

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

--- Comment #30 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:e14ea108faa6eba6a60a45ff0ca3099ce6ae45c2

commit r11-7146-ge14ea108faa6eba6a60a45ff0ca3099ce6ae45c2
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Feb 9 12:32:43 2021 +0100

    string: Add a workaround for -Wstringop-overread false positives [PR98465]

    In the PR there are several possibilities how to improve _M_disjunct at
    least in certain cases so that the compiler can figure out at least in some
    cases where __s is provably disjunct from _M_data() ... _M_data() +
this->size()
    but it is probably GCC 12 material.

    The false positive warning is on this particular copy, which is done for
    non-disjunct pointers when __len2 > __len1 and the __s >= __p + __len1,
    i.e. __s used to point to the characters moved through _S_move a few lines
earlier
    by __len2 - __len1 characters up to make space.  That is why the
    _S_copy source is __s + __len2 - __len1.  Unfortunately, when the compiler
    can't prove objects are disjunct, that copying from __s + __len2 - __len1
    of __len2 characters can very well mean accessing characters the source
    object (if it is not disjunct) provably can't have.

    The following patch works around that by making the _S_copy be a __p based
    pointer instead of __s based pointer.
    __s + __len2 - __len1
    and
    __p + (__s - __p) + (__len2 - __len1)
    have the same value and the latter may seem to be uselessly longer,
    but it seems at least currently in GIMPLE we keep it that way and so that
is
    what the warning code during expansion will see, and only actually
    optimize it to __s + __len2 - __len1 during RTL when we lose information
    on what is a pointer and what is a mere offset with the same mode.

    So, in the end we emit exactly the same assembly, just without the false
    positive warning.

    2021-02-09  Jakub Jelinek  <jakub@redhat.com>

            PR middle-end/98465
            * include/bits/basic_string.tcc (basic_string::_M_replace): When
__s
            points to the characters moved by earlier _S_move, compute the
source
            address using expression based on the __p pointer rather than __s
            pointer.

            * g++.dg/warn/Wstringop-overread-1.C: New test.

[Bug middle-end/98465] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[11 Regression] Bogus       |Bogus -Wstringop-overread
                   |-Wstringop-overread with    |with -std=gnu++20 -O2 and
                   |-std=gnu++20 -O2 and        |std::string::insert
                   |std::string::insert         |

--- Comment #31 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Workaround applied, keeping open for improving _M_disjunct in GCC 12.

[Bug middle-end/98465] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|11.0                        |12.0

--- Comment #32 from Martin Sebor <msebor at gcc dot gnu.org> ---
The solution in
https://gcc.gnu.org/pipermail/gcc-patches/2021-January/563862.html has been
deferred to GCC 12:
https://gcc.gnu.org/pipermail/gcc-patches/2021-February/565551.html

[Bug middle-end/98465] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|11.0                        |
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #33 from Martin Sebor <msebor at gcc dot gnu.org> ---
With the workaround in r11-7146 the warning should no longer be issued for
std::string::insert.  Thanks to r12-2087 suppression by #pragma GCC diagnostic
works reliably even with inlining in GCC 12, so the warning can also be
suppressed using it.  I'm not working on any other improvements related to the
underlying problem but I think this report can be resolved as fixed in GCC 11
and 12.

[Bug middle-end/98465] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[Randy at miningrigrentals dot com]

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

Randy <Randy at miningrigrentals dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |Randy at miningrigrentals dot com

--- Comment #34 from Randy <Randy at miningrigrentals dot com> ---
Is the patch for GCC 12 for this issue also applied to std_vstring?

[Bug middle-end/98465] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[redi at gcc dot gnu.org]

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

--- Comment #35 from Jonathan Wakely <redi at gcc dot gnu.org> ---
As you can see from the commit above, nothing was changed in __gnu_cxx::vstring
(there is no "std_vstring").

[Bug middle-end/98465] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[Randy at miningrigrentals dot com]

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

--- Comment #36 from Randy <Randy at miningrigrentals dot com> ---
(In reply to Jonathan Wakely from comment #35)
> As you can see from the commit above, nothing was changed in
> __gnu_cxx::vstring (there is no "std_vstring").

Ok then can someone look at __gnu_cxx::vstring, we use this instead as
std::string causes segfaults in our code. I'm getting the error as described
here but using __gnu_cxx::vstring in gcc-11, and if it can be fixed or is
fixed? in newer gcc versions then I can go through the process of upgrading.

[Bug middle-end/98465] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[redi at gcc dot gnu.org]

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

--- Comment #37 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Please report a separate bug for vstring then.

(Your segfaults are probably because you're using c_str() on a temporary
string, so accessing the pointer after the temporary goes out of scope.)

[Bug middle-end/98465] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[Randy at miningrigrentals dot com]

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

--- Comment #38 from Randy <Randy at miningrigrentals dot com> ---
(In reply to Jonathan Wakely from comment #37)
> Please report a separate bug for vstring then.
> 
> (Your segfaults are probably because you're using c_str() on a temporary
> string, so accessing the pointer after the temporary goes out of scope.)

std::string is not thread safe, this is why vstring is used (from my memory). I
don't use c_str out of scope. I lost the documentation that exclaimed why to
use vstring instead of std string to me. At the time, vstring solved all of our
segaults in the internals of the string. Exactly why this is the case I don't
remember or even if its still an issue in newer gcc versions. The bug with the
warning in this thread exists in vstring as well.

I will look at filing a separate bug report then...

In static member function âstatic constexpr std::char_traits<char>::char_type*
std::char_traits<char>::copy(std::char_traits<char>::char_type*, const
char_type*, std::size_t)â,
    inlined from âstatic void __gnu_cxx::__vstring_utility<_CharT, _Traits,
_Alloc>::_S_copy(_CharT*, const _CharT*, __gnu_cxx::__vstring_utility<_CharT,
_Traits, _Alloc>::size_type) [with _CharT = char; _Traits =
std::char_traits<char>; _Alloc = std::allocator<char>]â at
/usr/include/c++/11/ext/vstring_util.h:112:21,
    inlined from â__gnu_cxx::__versa_string<_CharT, _Traits, _Alloc, _Base>&
__gnu_cxx::__versa_string<_CharT, _Traits, _Alloc,
_Base>::_M_replace(__gnu_cxx::__versa_string<_CharT, _Traits, _Alloc,
_Base>::size_type, __gnu_cxx::__versa_string<_CharT, _Traits, _Alloc,
_Base>::size_type, const _CharT*, __gnu_cxx::__versa_string<_CharT, _Traits,
_Alloc, _Base>::size_type) [with _CharT = char; _Traits =
std::char_traits<char>; _Alloc = std::allocator<char>; _Base =
__gnu_cxx::__sso_string_base]â at /usr/include/c++/11/ext/vstring.tcc:160:20,
    inlined from â__gnu_cxx::__versa_string<_CharT, _Traits, _Alloc, _Base>&
__gnu_cxx::__versa_string<_CharT, _Traits, _Alloc,
_Base>::replace(__gnu_cxx::__versa_string<_CharT, _Traits, _Alloc,
_Base>::size_type, __gnu_cxx::__versa_string<_CharT, _Traits, _Alloc,
_Base>::size_type, const _CharT*, __gnu_cxx::__versa_string<_CharT, _Traits,
_Alloc, _Base>::size_type) [with _CharT = char; _Traits =
std::char_traits<char>; _Alloc = std::allocator<char>; _Base =
__gnu_cxx::__sso_string_base]â at /usr/include/c++/11/ext/vstring.h:1313:19,
    inlined from â__gnu_cxx::__versa_string<_CharT, _Traits, _Alloc, _Base>&
__gnu_cxx::__versa_string<_CharT, _Traits, _Alloc,
_Base>::insert(__gnu_cxx::__versa_string<_CharT, _Traits, _Alloc,
_Base>::size_type, const _CharT*, __gnu_cxx::__versa_string<_CharT, _Traits,
_Alloc, _Base>::size_type) [with _CharT = char; _Traits =
std::char_traits<char>; _Alloc = std::allocator<char>; _Base =
__gnu_cxx::__sso_string_base]â at /usr/include/c++/11/ext/vstring.h:1085:29,
    inlined from âBouncedConnection::ReturnAction
BouncedConnection::Handle_Notify(rapidjson::Document&,
FullThreadCommandLogInstance&, String&)â at
BouncedConnectionCmdProcessing.cpp:2258:54:
/usr/include/c++/11/bits/char_traits.h:409:56: warning: âvoid*
__builtin_memcpy(void*, const void*, long unsigned int)â reading 2 bytes from a
region of size 1 [-Wstringop-overread]
  409 |         return static_cast<char_type*>(__builtin_memcpy(__s1, __s2,
__n));
      |                                       
~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~

[Bug middle-end/98465] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[redi at gcc dot gnu.org]

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

--- Comment #39 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Randy from comment #38)
> std::string is not thread safe, this is why vstring is used (from my
> memory).

That's totally wrong, vstring has no more thread-safety than std::string has.
The same rules apply to both of them (it's OK to call const member functions
concurrently, but any non-const access must be manually synchronized by the
caller).

[Bug middle-end/98465] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[Randy at miningrigrentals dot com]

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

--- Comment #40 from Randy <Randy at miningrigrentals dot com> ---
(In reply to Jonathan Wakely from comment #39)
> (In reply to Randy from comment #38)
> > std::string is not thread safe, this is why vstring is used (from my
> > memory).
> 
> That's totally wrong, vstring has no more thread-safety than std::string
> has. The same rules apply to both of them (it's OK to call const member
> functions concurrently, but any non-const access must be manually
> synchronized by the caller).

I googled, I think I rember now, its due to reference counting. I would get
cases where its deallocating something internally due to the way I pass strings
around to my threads.

[Bug middle-end/98465] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[redi at gcc dot gnu.org]

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

--- Comment #41 from Jonathan Wakely <redi at gcc dot gnu.org> ---
So then basically the same as what I said:

(In reply to Jonathan Wakely from comment #37)
> (Your segfaults are probably because you're using c_str() on a temporary
> string, so accessing the pointer after the temporary goes out of scope.)

You're relying on the string contents outliving a specific object, which only
works if the contents are reference-counted and there is still another object
somewhere that owns it. Your code has a bug.

[Bug middle-end/98465] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[Randy at miningrigrentals dot com]

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

--- Comment #42 from Randy <Randy at miningrigrentals dot com> ---
(In reply to Jonathan Wakely from comment #41)
> So then basically the same as what I said:
> 
> (In reply to Jonathan Wakely from comment #37)
> > (Your segfaults are probably because you're using c_str() on a temporary
> > string, so accessing the pointer after the temporary goes out of scope.)
> 
> You're relying on the string contents outliving a specific object, which
> only works if the contents are reference-counted and there is still another
> object somewhere that owns it. Your code has a bug.
This isn't the place to discuss this.

vstring solved the problem I was having at the time and I don't know if its
still an issue. Might have been using gcc-4.8.x at the time and I think
std::string implementation changed since then. I have no reason to switch from
using vstring, I just want those who work on gcc to also consider updating
vstring with the same type of fixes for this warning.

[Bug middle-end/98465] Bogus -Wstringop-overread with -std=gnu++20 -O2 and std::string::insert

[redi at gcc dot gnu.org]

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

--- Comment #43 from Jonathan Wakely <redi at gcc dot gnu.org> ---
And updating vstring is exactly what I'd like to avoid. It's pretty much on
life support as far as I'm concerned.