Bug 106904 - [12 Regression] Incorrect -Wstringop-overflow with partial memcpy() into a nested structure
Summary: [12 Regression] Incorrect -Wstringop-overflow with partial memcpy() into a ne...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 12.2.0
: P3 normal
Target Milestone: 12.3
Assignee: Richard Biener
URL:
Keywords: diagnostic
Depends on:
Blocks: Wstringop-overflow
  Show dependency treegraph
 
Reported: 2022-09-11 22:47 UTC by Zeb Figura
Modified: 2023-03-15 10:05 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Known to work: 12.2.1, 13.0
Known to fail: 12.2.0
Last reconfirmed: 2022-12-07 00:00:00


Attachments
minimal test case (161 bytes, text/plain)
2022-09-11 22:47 UTC, Zeb Figura
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Zeb Figura 2022-09-11 22:47:11 UTC
Created attachment 53562 [details]
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;
      |         ^~~~
Comment 1 Andrew Pinski 2022-09-11 22:51:16 UTC
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.
Comment 2 Zeb Figura 2022-09-11 22:52:26 UTC
(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?
Comment 3 Zeb Figura 2022-09-11 22:54:19 UTC
From the warning, it seems like it thinks I wrote

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

but that's not what I wrote.
Comment 4 Andrew Pinski 2022-09-13 15:30:55 UTC
(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.
Comment 5 Richard Biener 2022-12-07 13:42:14 UTC
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.
Comment 6 GCC Commits 2022-12-11 13:35:19 UTC
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.
Comment 7 Richard Biener 2022-12-11 13:35:54 UTC
Fixed on trunk sofar.
Comment 8 Zeb Figura 2022-12-11 17:40:25 UTC
Thanks!
Comment 9 GCC Commits 2023-03-15 09:47:43 UTC
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)
Comment 10 Richard Biener 2023-03-15 10:05:14 UTC
Fixed.