[Bug tree-optimization/104789] New: [12 Regression] New -Wstringop-overflow false positive since r12-5863-g9354a7d70caef1c9

[Bug tree-optimization/104789] New: [12 Regression] New -Wstringop-overflow false positive since r12-5863-g9354a7d70caef1c9

[marxin at gcc dot gnu.org]

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

            Bug ID: 104789
           Summary: [12 Regression] New -Wstringop-overflow false positive
                    since r12-5863-g9354a7d70caef1c9
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: marxin at gcc dot gnu.org
                CC: msebor at gcc dot gnu.org
  Target Milestone: ---

It's originally reported here:
https://github.com/godotengine/godot/issues/58747

$ cat godot-stringop.cpp
#include <cstdint>
#include <cstdio>

uint32_t some_func(const uint32_t components) {
        uint8_t header[8];
        uint32_t header_bytes = 0;
        for (uint32_t i = 0; i < components; i++) {
                header_bytes += 2;
        }
        header_bytes += 2;
        // This works it around, but shouldn't be needed AFAICT.
        //while (header_bytes != 8 && header_bytes % 4 != 0) {
        while (header_bytes % 4 != 0) {
                header[header_bytes++] = 0;
        }
        for (uint32_t i = 0; i < header_bytes; i++) {
                printf("%d\n", header[i]);
        }
        return header_bytes;
}

int main() {
        some_func(1);
        some_func(3);
        return 0;
}

$ g++ godot-stringop.cpp -c -Werror=all -O3
godot-stringop.cpp: In function ‘uint32_t some_func(uint32_t)’:
godot-stringop.cpp:14:40: error: writing 1 byte into a region of size 0
[-Werror=stringop-overflow=]
   14 |                 header[header_bytes++] = 0;
      |                 ~~~~~~~~~~~~~~~~~~~~~~~^~~
godot-stringop.cpp:5:17: note: at offset 8 into destination object ‘header’ of
size 8
    5 |         uint8_t header[8];
      |                 ^~~~~~
cc1plus: some warnings being treated as errors

[Bug tree-optimization/104789] [12 Regression] New -Wstringop-overflow false positive since r12-5863-g9354a7d70caef1c9

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2022-03-04
     Ever confirmed|0                           |1
   Target Milestone|---                         |12.0

[Bug tree-optimization/104789] [12 Regression] New -Wstringop-overflow false positive since r12-5863-g9354a7d70caef1c9

[marxin at gcc dot gnu.org]

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

--- Comment #1 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to Martin Liška from comment #0)
> It's originally reported here:
> https://github.com/godotengine/godot/issues/58747

Sorry, here:
https://github.com/godotengine/godot/pull/58755

[Bug tree-optimization/104789] [12 Regression] New -Wstringop-overflow false positive since r12-5863-g9354a7d70caef1c9

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Note I think the reduced testcase is reduced too much.

[Bug tree-optimization/104789] [12 Regression] New -Wstringop-overflow false positive since r12-5863-g9354a7d70caef1c9

[pinskia at gcc dot gnu.org]

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

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Note the first loop is bounded roo
Because of
encode_uint16(temp_packets[0].data[i], &header[header_bytes]);

Which is not in the reduced testcase.

[Bug tree-optimization/104789] [12 Regression] New -Wstringop-overflow false positive since r12-5863-g9354a7d70caef1c9

[rverschelde at gmail dot com]

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

Rémi Verschelde <rverschelde at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rverschelde at gmail dot com

--- Comment #4 from Rémi Verschelde <rverschelde at gmail dot com> ---
I'm not familiar with the term "bounded roo". I reduced the test case as much
as I could to focus on the minimal code that would trigger the warning which
seems to me like a false positive - is it not one?

I can try to include all the relevant code to do marshalling but it seems to me
that it will just make the test case more complex - while the minimal one does
reproduce an issue with the same symptoms.

[Bug tree-optimization/104789] [12 Regression] New -Wstringop-overflow false positive since r12-5863-g9354a7d70caef1c9

[pinskia at gcc dot gnu.org]

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

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Rémi Verschelde from comment #4)
> I'm not familiar with the term "bounded roo". I reduced the test case as
> much as I could to focus on the minimal code that would trigger the warning
> which seems to me like a false positive - is it not one?

roo was a typo for too. The r and t are next to each other on the standard
qwerty keyboard.

Anyways I think the warning should be reworded to say might be writing instead
of a definite. There is another bug talking about the wording too.

Also the way I read the code in both the original and reduced testcase, without
knowing how components' range, there could be a buffer overflow.

[Bug tree-optimization/104789] [12 Regression] New -Wstringop-overflow false positive since r12-5863-g9354a7d70caef1c9

[rverschelde at gmail dot com]

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

--- Comment #6 from Rémi Verschelde <rverschelde at gmail dot com> ---
Indeed if the warning was reworded to make it clear that it's a risk and not a
certainty would solve this.

Both the reduced test case and the original code are both risky: `components`
is an `uint32_t` that is set to either 1 or 3, so in the context of that code
`header_bytes` would never exceed 8.

But if this was changed to anything higher than 3 then yes, there would be a
buffer overflow. If this is what this warning aims to warn about (the risk of
buffer overflow and not the certainty of one), then there's probably no false
positive.

Adding:
```
if (components > 3) {
        return 0;
}
```
to the reduced testcase does prevent the warning.

On the other hand, the same kind of check doesn't seem to solve the warning in
the original Godot case, so you're right about the reduced testcase being too
reduced. I'll dig further and see if I can make a better testcase, or
understand why a check on `components` (or even `header_bytes > 7`) isn't
sufficient in Godot.

[Bug tree-optimization/104789] [12 Regression] New -Wstringop-overflow false positive since r12-5863-g9354a7d70caef1c9

[rverschelde at gmail dot com]

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

--- Comment #7 from Rémi Verschelde <rverschelde at gmail dot com> ---
Took me a while, but I was able to make a proper reduced testcase by cutting
down the Godot case to the very minimal.

It's a bit convoluted (and the code doesn't make any sense anymore), but the
complexity seems to trigger the false positive I intended to report originally.

```
$ cat godot-stringop.cpp
#include <cstdint>
#include <vector>

uint32_t components = 3;

void commit_temp_packets() {
        // Should be sufficient?
        if (components > 3) { return; }

        uint8_t header[8];
        uint32_t header_bytes = 0;
        for (uint32_t i = 0; i < components; i++) {
                header_bytes += 2;
        }

        uint32_t max_shifts[3] = { 0, 0, 0 };

        for (uint32_t i = 1; i < 2; i++) {
                for (uint32_t j = 0; j < components; j++) {
                        max_shifts[j] = 1;
                }
        }
        header_bytes += 2;

        // Should definitely be sufficient.
        if (header_bytes > 7) { return; }

        while (header_bytes % 4 != 0) {
                header[header_bytes++] = 0;
        }

        std::vector<uint8_t> data;

        for (uint32_t i = 0; i < header_bytes; i++) {
                data.push_back(header[i]);
        }

        uint32_t bit_buffer = 0;
        uint32_t bits_used = 0;

        for (uint32_t i = 1; i < 2; i++) {
                for (uint32_t j = 0; j < components; j++) {
                        bit_buffer |= 1 << bits_used;
                        bits_used += max_shifts[j] + 1;
                        while (bits_used >= 8) {
                                uint8_t byte = bit_buffer & 0xFF;
                                data.push_back(byte);
                                bit_buffer >>= 8;
                                bits_used -= 8;
                        }
                }
        }
}

$ g++ godot-stringop.cpp -c -Werror=all -O3
godot-stringop.cpp: In function ‘void commit_temp_packets()’:
godot-stringop.cpp:29:40: error: writing 1 byte into a region of size 0
[-Werror=stringop-overflow=]
   29 |                 header[header_bytes++] = 0;
      |                 ~~~~~~~~~~~~~~~~~~~~~~~^~~
godot-stringop.cpp:10:17: note: at offset 8 into destination object ‘header’ of
size 8
   10 |         uint8_t header[8];
      |                 ^~~~~~
godot-stringop.cpp:29:40: error: writing 1 byte into a region of size 0
[-Werror=stringop-overflow=]
   29 |                 header[header_bytes++] = 0;
      |                 ~~~~~~~~~~~~~~~~~~~~~~~^~~
godot-stringop.cpp:10:17: note: at offset [9, 11] into destination object
‘header’ of size 8
   10 |         uint8_t header[8];
      |                 ^~~~~~
cc1plus: some warnings being treated as errors
```

Interestingly, there are no changes to `header_bytes` or `components` after
line 29, yet the code that follows seems needed to trigger the warning.

[Bug tree-optimization/104789] [12 Regression] New -Wstringop-overflow false positive since r12-5863-g9354a7d70caef1c9

[msebor at gcc dot gnu.org]

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

--- Comment #8 from Martin Sebor <msebor at gcc dot gnu.org> ---
Created attachment 52574
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52574&action=edit
Output of debug_ranger() for the affected function.

The IL the first warning triggers for in the test case in comment #7 is below. 
The access to header[header_bytes_152] where header_bytes_152's value is in [8,
8][10, 10] is out of bounds for unsigned char[8].  So the warning is correctly
pointing out the invalid store.  The store is the result of GCC unrolling the
first six iterations of the while loop, starting with index 4 (stores at index
2 and 3 are hoisted above the unrolled loop).  The last two iterations store
past the end of the header array.  GCC 11 also unrolls six iterations of the
loop but it starts with index 2 (it doesn't hoist the first two stores above
it).  The attachment shows the full output of calling debug_ranger() on the
function.

=========== BB 13 ============
Imports: header_bytes_192  
Exports: _147  header_bytes_152  header_bytes_192  
         _147 : header_bytes_152  header_bytes_192(I)  
         header_bytes_152 : header_bytes_192(I)  
header_bytes_192        unsigned int [4, 4][6, 6]
Relational : (header_bytes_152 > header_bytes_192)
    <bb 13> [local count: 21246984]:
    header_bytes_152 = header_bytes_192 + 4;
    header[header_bytes_65] = 0;
    _147 = header_bytes_152 & 3;
    if (_147 != 0)
      goto <bb 14>; [85.71%]
    else
      goto <bb 20>; [14.29%]

_147 : unsigned int [0, 0][2, 2]
header_bytes_152 : uint32_t [8, 8][10, 10]
13->14  (T) _147 :      unsigned int [2, 2]
13->14  (T) header_bytes_152 :  uint32_t [8, 8][10, 10]
13->14  (T) header_bytes_192 :  unsigned int [4, 4][6, 6]
13->20  (F) _147 :      unsigned int [0, 0]
13->20  (F) header_bytes_152 :  uint32_t [8, 8][10, 10]
13->20  (F) header_bytes_192 :  unsigned int [4, 4][6, 6]

=========== BB 14 ============
Imports: header_bytes_192  
Exports: _131  header_bytes_133  header_bytes_192  
         _131 : header_bytes_133  header_bytes_192(I)  
         header_bytes_133 : header_bytes_192(I)  
header_bytes_192        unsigned int [4, 4][6, 6]
Relational : (header_bytes_133 > header_bytes_192)
    <bb 14> [local count: 18210790]:
    header_bytes_133 = header_bytes_192 + 5;
    header[header_bytes_152] = 0;    <<< -Wstringop-overflow
    _131 = header_bytes_133 & 3;
    if (_131 != 0)
      goto <bb 15>; [85.71%]
    else
      goto <bb 21>; [14.29%]

_131 : unsigned int [1, 1][3, 3]
header_bytes_133 : uint32_t [9, 9][11, 11]
14->15  (T) _131 :      unsigned int [1, 1][3, 3]
14->15  (T) header_bytes_133 :  uint32_t [9, 9][11, 11]
14->15  (T) header_bytes_192 :  unsigned int [4, 4][6, 6]
14->21  (F) _131 :      UNDEFINED
14->21  (F) header_bytes_133 :  UNDEFINED
14->21  (F) header_bytes_192 :  UNDEFINED

[Bug tree-optimization/104789] [12 Regression] -Wstringop-overflow false positive at -O3 for an unrolled loop

[msebor at gcc dot gnu.org]

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

--- Comment #9 from Martin Sebor <msebor at gcc dot gnu.org> ---
A much simplified test case that reproduces the same warning (with both GCC 12
and 11) is below.  The underlying problem is that although GCC does have a way
to represent simple disjoint ranges of variable values, it's not used here, and
even if it were, very little code makes uses of this representation beyond a
single contiguous range (or, at most, a converse of it).  Even the disjoint
representation isn't sufficiently flexible to capture arbitrarily complex
ranges (capturing those in their full generality would require a constraint
solver).

$ cat z.c && gcc -O3 -S -Wall z.c
char a[8];

void f (unsigned n)
{
  unsigned i = 0;

  for (unsigned j = 0; i != n; ++j)
    i += 2;

  if (i > 7)
    return;

  while (i % 4)
    a[i++] = 0;
}
z.c: In function ‘f’:
z.c:14:12: warning: writing 1 byte into a region of size 0
[-Wstringop-overflow=]
   14 |     a[i++] = 0;
      |     ~~~~~~~^~~
z.c:1:6: note: at offset 8 into destination object ‘a’ of size 8
    1 | char a[8];
      |      ^

The warning can be avoided (and the emitted object code improved) by changing
the while loop like so:

  while (i % 4)
    {
      if (i > 7) __builtin_unreachable (); 
      a[i++] = 0;
    }

The same suppression works in the test case in comment #7 but GCC then issues
another warning, this one pointing out that an element of the header array is
used uninitialized.  That warning looks valid to me:

/x86_64-pc-linux-gnu/bits/c++allocator.h:33,
                 from
/build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/allocator.h:46,
                 from
/build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/vector:61,
                 from pr104789.C:2:
In member function ‘void std::__new_allocator<_Tp>::construct(_Up*, _Args&&
...) [with _Up = unsigned char; _Args = {const unsigned char&}; _Tp = unsigned
char]’,
    inlined from ‘static void std::allocator_traits<std::allocator<_Tp1>
>::construct(allocator_type&, _Up*, _Args&& ...) [with _Up = unsigned char;
_Args = {const unsigned char&}; _Tp = unsigned char]’ at
/build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/alloc_traits.h:516:17,
    inlined from ‘void std::vector<_Tp, _Alloc>::push_back(const value_type&)
[with _Tp = unsigned char; _Alloc = std::allocator<unsigned char>]’ at
/build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_vector.h:1280:30,
    inlined from ‘void commit_temp_packets()’ at pr104789.C:37:17:
/build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/new_allocator.h:175:11:
warning: ‘header’ may be used uninitialized [-Wmaybe-uninitialized]
  175 |         { ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pr104789.C: In function ‘void commit_temp_packets()’:
pr104789.C:10:17: note: ‘header’ declared here
   10 |         uint8_t header[8];
      |                 ^~~~~~

[Bug tree-optimization/104789] [12 Regression] -Wstringop-overflow false positive at -O3 for an unrolled loop

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |diagnostic,
                   |                            |missed-optimization

--- Comment #10 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think the -Wstringop-overflow warnings are prone to false positives with loop
unrolling just like the -Warray-bound warnings were which is why those were
moved to before loop optimizations.  Now all of those cases will re-appear as
-Wstringop-overflow which happens too late since the strlen pass runs quite
late.

Unrolling sometimes "speculatively" peels iterations that will be never
reachable at runtime and that we have difficulty to statically remove.  That's
simply hard to avoid.

For GCC 13 we might want to experiment with moving the strlen pass before
loop optimizations.  I'd put it somewhere inbetween PRE and the DOM preceeding
it.

[Bug tree-optimization/104789] [12 Regression] -Wstringop-overflow false positive at -O3 for an unrolled loop

[msebor at gcc dot gnu.org]

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

--- Comment #11 from Martin Sebor <msebor at gcc dot gnu.org> ---
The direct store subset of -Wstringop-overflow that runs in the strlen pass
(i.e., those handled in strlen_pass::handle_store) might be better handled in
VRP and issued under -Warray-bounds.  The challenge there is that unlike this
-Wstringop-overflow subset which respects subobject boundaries, -Warray-bounds
intentionally considers complete objects (this was done to avoid false
positives).

So before moving all of strlen a solution to consider is to merge these two
sets of warnings while preserving the subobject sensitivity.

Another (indirectly related) improvement is to also move -Warray-bounds out of
VRP and into the access warning pass, and run it at the same times as most
other warnings there (that would also enable -Warray-bounds at -O0, which might
of course trigger some new false positives).

[Bug tree-optimization/104789] [12/13 Regression] -Wstringop-overflow false positive at -O3 for an unrolled loop

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 12.1 is being released, retargeting bugs to GCC 12.2.

[Bug tree-optimization/104789] [12/13 Regression] -Wstringop-overflow false positive at -O3 for an unrolled loop

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2

[Bug tree-optimization/104789] [12/13/14 Regression] -Wstringop-overflow false positive at -O3 for an unrolled loop

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|12.3                        |12.4

--- Comment #14 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 12.3 is being released, retargeting bugs to GCC 12.4.