[Bug c/106904] New: Incorrect -Wstringop-overflow with partial memcpy() into a nested structure

[Bug c/106904] New: Incorrect -Wstringop-overflow with partial memcpy() into a nested structure

[zfigura at codeweavers dot com]

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

            Bug ID: 106904
           Summary: Incorrect -Wstringop-overflow with partial memcpy()
                    into a nested structure
           Product: gcc
           Version: 12.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: zfigura at codeweavers dot com
  Target Milestone: ---

Created attachment 53562
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53562&action=edit
minimal test case

I encountered a warning while trying to compile 32-bit wine 7.17 with gcc 12.2,
specificially at this line here:

https://source.winehq.org/git/wine.git/blob/wine-7.17:/dlls/win32u/message.c#l359

The relevant code copies a smaller structure into a larger one of a different
type. (This may be a violation of aliasing rules, but adding
-fno-strict-aliasing doesn't change anything.)

I was able to reproduce this with a minimal test case. This is a very weird set
of conditions, but I couldn't seem to reduce this test case any further.
Changing the type of "ps" to "struct packed_windowpos" makes the error go away;
so does changing the first argument of the memcpy to "ps".

leslie@terabithia:~$ gcc --version
gcc (Debian 12.2.0-1) 12.2.0
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

leslie@terabithia:~$ gcc -m32 test.c -c -o test.o -Wall -O2
test.c: In function ‘func’:
test.c:26:5: warning: writing 8 bytes into a region of size 4
[-Wstringop-overflow=]
   26 |     __builtin_memcpy(&ps->wp, &wp, sizeof(wp));
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test.c:9:9: note: destination object ‘hwnd’ of size 4
    9 |     int hwnd;
      |         ^~~~

[Bug tree-optimization/106904] Incorrect -Wstringop-overflow with partial memcpy() into a nested structure

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The warning is correct for the reduced testcase as we warning that you are
copying the wrong size for the field 

Now I have not looked at the non reduced testcase.

[Bug tree-optimization/106904] Incorrect -Wstringop-overflow with partial memcpy() into a nested structure

[zfigura at codeweavers dot com]

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

--- Comment #2 from Zebediah Figura <zfigura at codeweavers dot com> ---
(In reply to Andrew Pinski from comment #1)
> The warning is correct for the reduced testcase as we warning that you are
> copying the wrong size for the field 

The field "&ps->wp" is of size 16 (4 ints), whereas the source "wp" is of size
8 (2 ints). Or did I make a mistake somewhere?

[Bug tree-optimization/106904] Incorrect -Wstringop-overflow with partial memcpy() into a nested structure

[zfigura at codeweavers dot com]

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

--- Comment #3 from Zebediah Figura <zfigura at codeweavers dot com> ---
From the warning, it seems like it thinks I wrote

memcpy(&ps->wp.hwnd, &wp, sizeof(wp));

but that's not what I wrote.

[Bug tree-optimization/106904] [12/13 Regression] Incorrect -Wstringop-overflow with partial memcpy() into a nested structure

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.3
            Summary|Incorrect                   |[12/13 Regression]
                   |-Wstringop-overflow with    |Incorrect
                   |partial memcpy() into a     |-Wstringop-overflow with
                   |nested structure            |partial memcpy() into a
                   |                            |nested structure

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Zebediah Figura from comment #3)
> From the warning, it seems like it thinks I wrote
> 
> memcpy(&ps->wp.hwnd, &wp, sizeof(wp));
> 
> but that's not what I wrote.

Oh I read the code wrong sorry about that.

[Bug tree-optimization/106904] [12/13 Regression] Incorrect -Wstringop-overflow with partial memcpy() into a nested structure

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2022-12-07
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org
             Status|UNCONFIRMED                 |ASSIGNED

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
Note we diagnose

MEM <unsigned char[8]> [(char * {ref-all})vectp.4_10] = MEM <unsigned char[8]>
[(char * {ref-all})&wp];

where vectp.4_10 == &ps_5(D)->mp.hwnd;

that happens because SLP vectorization produces

  vectp.4_10 = &ps_5(D)->wp.hwnd;
  vect__1.5_11 = MEM[(int *)vectp.4_10];
  vectp.4_12 = vectp.4_10 + 4;
  vectp.4_14 = vectp.4_10 + 8;
  vect__1.7_15 = MEM[(int *)vectp.4_14];

and we then CSE the memcpy address in the following code to vectp.4_10:

  _3 = &ps_5(D)->wp;
  __builtin_memcpy (_3, &wp, 8);

the access diagnostics have the issue that they mis-interpret addresses
as more than just pointer arithmetic.  Eventually part of this could be
avoided by not introducing any non-invariant ADDR_EXPRs at least but
use POINTER_PLUS_EXPR where possible (like in the above case).  Alternatively
we could strip zero-offset components at these points.

[Bug tree-optimization/106904] [12/13 Regression] Incorrect -Wstringop-overflow with partial memcpy() into a nested structure

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

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

commit r13-4598-gf8d136e50e6f82cba793483d910a2b2643108508
Author: Richard Biener <rguenther@suse.de>
Date:   Wed Dec 7 14:42:24 2022 +0100

    tree-optimization/106904 - bogus -Wstringopt-overflow with vectors

    The following avoids CSE of &ps->wp to &ps->wp.hwnd confusing
    -Wstringopt-overflow by making sure to produce addresses to the
    biggest container from vectorization.  For this I introduce
    strip_zero_offset_components which turns &ps->wp.hwnd into
    &(*ps) and use that to base the vector data references on.
    That will also work for addresses with variable components,
    alternatively emitting pointer arithmetic via calling
    get_inner_reference and gimplifying that would be possible
    but likely more intrusive.

    This is by no means a complete fix for all of those issues
    (avoiding ADDR_EXPRs in favor of pointer arithmetic might be).
    Other passes will have similar issues.

    In theory that might now cause false negatives.

            PR tree-optimization/106904
            * tree.h (strip_zero_offset_components): Declare.
            * tree.cc (strip_zero_offset_components): Define.
            * tree-vect-data-refs.cc (vect_create_addr_base_for_vector_ref):
            Strip zero offset components before building the address.

            * gcc.dg/Wstringop-overflow-pr106904.c: New testcase.

[Bug tree-optimization/106904] [12 Regression] Incorrect -Wstringop-overflow with partial memcpy() into a nested structure

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[12/13 Regression]          |[12 Regression] Incorrect
                   |Incorrect                   |-Wstringop-overflow with
                   |-Wstringop-overflow with    |partial memcpy() into a
                   |partial memcpy() into a     |nested structure
                   |nested structure            |
      Known to work|                            |13.0

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed on trunk sofar.

[Bug tree-optimization/106904] [12 Regression] Incorrect -Wstringop-overflow with partial memcpy() into a nested structure

[zfigura at codeweavers dot com]

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

--- Comment #8 from Zebediah Figura <zfigura at codeweavers dot com> ---
Thanks!

[Bug tree-optimization/106904] [12 Regression] Incorrect -Wstringop-overflow with partial memcpy() into a nested structure

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Richard Biener
<rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:97d599e09b0fd389a7cbac8867e56977ec97900f

commit r12-9254-g97d599e09b0fd389a7cbac8867e56977ec97900f
Author: Richard Biener <rguenther@suse.de>
Date:   Wed Dec 7 14:42:24 2022 +0100

    tree-optimization/106904 - bogus -Wstringopt-overflow with vectors

    The following avoids CSE of &ps->wp to &ps->wp.hwnd confusing
    -Wstringopt-overflow by making sure to produce addresses to the
    biggest container from vectorization.  For this I introduce
    strip_zero_offset_components which turns &ps->wp.hwnd into
    &(*ps) and use that to base the vector data references on.
    That will also work for addresses with variable components,
    alternatively emitting pointer arithmetic via calling
    get_inner_reference and gimplifying that would be possible
    but likely more intrusive.

    This is by no means a complete fix for all of those issues
    (avoiding ADDR_EXPRs in favor of pointer arithmetic might be).
    Other passes will have similar issues.

    In theory that might now cause false negatives.

            PR tree-optimization/106904
            * tree.h (strip_zero_offset_components): Declare.
            * tree.cc (strip_zero_offset_components): Define.
            * tree-vect-data-refs.cc (vect_create_addr_base_for_vector_ref):
            Strip zero offset components before building the address.

            * gcc.dg/Wstringop-overflow-pr106904.c: New testcase.

    (cherry picked from commit f8d136e50e6f82cba793483d910a2b2643108508)

[Bug tree-optimization/106904] [12 Regression] Incorrect -Wstringop-overflow with partial memcpy() into a nested structure

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED
      Known to fail|                            |12.2.0
      Known to work|                            |12.2.1

--- Comment #10 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed.