[PATCH] correct fix to avoid false positives for vectorized stores (PR 96963, 94655)

[PATCH] correct fix to avoid false positives for vectorized stores (PR 96963, 94655)

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 966 bytes --]

The hack I put in compute_objsize() last January for pr93200 isn't
quite correct.  It happened to suppress the false positive there
but, due to what looks like a thinko on my part, not in some other
test cases involving vectorized stores.

The attached change adjusts the hack to have compute_objsize() give
up on all MEM_REFs with a vector type.  This effectively disables
-Wstringop-{overflow,overread} for vector accesses (either written
by the user or synthesized by GCC from ordinary accesses).  It
doesn't affect -Warray-bounds because this warning doesn't use
compute_objsize() yet.  When it does (it should considerably
simplify the code) some additional changes will be needed to
preserve -Warray-bounds for out of bounds vector accesses.
The test this patch adds should serve as a reminder to make
it if we forget.

Tested on x86_64-linux.  Since PR 94655 was reported against GCC
10 I'd like to apply this fix to both the trunk and the 10 branch.

Martin

[-- Attachment #2: gcc-96963.diff --]
[-- Type: text/x-patch, Size: 8384 bytes --]

PR middle-end/96963 - -Wstringop-overflow false positive with -ftree-vectorize when assigning consecutive char struct members

gcc/ChangeLog:

	PR middle-end/96963
	* builtins.c (compute_objsize_r): Correct a workaround for vectorized
	assignments.

gcc/testsuite/ChangeLog:

	PR middle-end/96963
	* gcc.dg/Wstringop-overflow-65.c: New test.
	* gcc.dg/Warray-bounds-69.c: Same.

diff --git a/gcc/builtins.c b/gcc/builtins.c
index 0aed008687c..2ffe472d4ea 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -5425,24 +5425,33 @@ compute_objsize_r (tree ptr, int ostype, access_ref *pref,
       ++pref->deref;
 
       tree ref = TREE_OPERAND (ptr, 0);
-      tree reftype = TREE_TYPE (ref);
-      if (!addr && code == ARRAY_REF
-	  && TREE_CODE (TREE_TYPE (reftype)) == POINTER_TYPE)
-	/* Avoid arrays of pointers.  FIXME: Hande pointers to arrays
-	   of known bound.  */
-	return false;
+      {
+	tree reftype = TREE_TYPE (ref);
+	if (!addr && code == ARRAY_REF
+	    && TREE_CODE (TREE_TYPE (reftype)) == POINTER_TYPE)
+	  /* Avoid arrays of pointers.  FIXME: Hande pointers to arrays
+	     of known bound.  */
+	  return false;
+      }
+      {
+	tree ptrtype = TREE_TYPE (ptr);
+	if (POINTER_TYPE_P (ptrtype))
+	  ptrtype = TREE_TYPE (ptrtype);
 
-      if (code == MEM_REF && TREE_CODE (reftype) == POINTER_TYPE)
-	{
-	  /* Give up for MEM_REFs of vector types; those may be synthesized
-	     from multiple assignments to consecutive data members.  See PR
-	     93200.
-	     FIXME: Deal with this more generally, e.g., by marking up such
-	     MEM_REFs at the time they're created.  */
-	  reftype = TREE_TYPE (reftype);
-	  if (TREE_CODE (reftype) == VECTOR_TYPE)
+	if (code == MEM_REF && TREE_CODE (ptrtype) == VECTOR_TYPE)
+	  {
+	    /* Hack: Give up for MEM_REFs of vector types; those may be
+	       synthesized from multiple assignments to consecutive data
+	       members (see PR 93200 and 96963).
+	       FIXME: Vectorized assignments should only be present after
+	       vectorization so this hack is only necessary after it has
+	       run and could be avoided in calls from prior passes (e.g.,
+	       tree-ssa-strlen.c).
+	       FIXME: Deal with this more generally, e.g., by marking up
+	       such MEM_REFs at the time they're created.  */
 	    return false;
-	}
+	  }
+      }
 
       if (!compute_objsize_r (ref, ostype, pref, snlim, qry))
 	return false;
diff --git a/gcc/testsuite/gcc.dg/Warray-bounds-69.c b/gcc/testsuite/gcc.dg/Warray-bounds-69.c
new file mode 100644
index 00000000000..5a955774124
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Warray-bounds-69.c
@@ -0,0 +1,74 @@
+/* Verify that storing a bigger vector into smaller space is diagnosed.
+   { dg-do compile }
+   { dg-options "-O2 -Warray-bounds" } */
+
+typedef __INT16_TYPE__                         int16_t;
+typedef __attribute__ ((__vector_size__ (32))) char C32;
+
+typedef __attribute__ ((__vector_size__ (64))) int16_t I16_64;
+
+void sink (void*);
+
+
+void nowarn_c32 (char c)
+{
+  extern char nowarn_a32[32];
+
+  void *p = nowarn_a32;
+  *(C32*)p = (C32){ c };
+  sink (p);
+
+  char a32[32];
+  p = a32;
+  *(C32*)p = (C32){ c };
+  sink (p);
+}
+
+/* The invalid stores below are diagnosed by -Warray-bounds only
+   because it doesn't use compute_objsize().  If/when that changes
+   the function might need adjusting to avoid the hack put in place
+   to avoid false positives due to vectorization.  */
+
+void warn_c32 (char c)
+{
+  extern char warn_a32[32];   // { dg-message "'warn_a32'" "note" }
+
+  void *p = warn_a32 + 1;
+  *(C32*)p = (C32){ c };      // { dg-warning "\\\[-Warray-bounds" }
+
+  /* Verify a local variable too. */
+  char a32[32];               // { dg-message "'a32'" }
+  p = a32 + 1;
+  *(C32*)p = (C32){ c };      // { dg-warning "\\\[-Warray-bounds" }
+  sink (p);
+}
+
+
+void nowarn_i16_64 (int16_t i)
+{
+  extern char nowarn_a64[64];
+
+  void *p = nowarn_a64;
+  I16_64 *q = (I16_64*)p;
+  *q = (I16_64){ i };
+
+  char a64[64];
+  q = (I16_64*)a64;
+  *q = (I16_64){ i };
+  sink (q);
+}
+
+void warn_i16_64 (int16_t i)
+{
+  extern char warn_a64[64];   // { dg-message "'warn_a64'" }
+
+  void *p = warn_a64 + 1;
+  I16_64 *q = (I16_64*)p;
+  *q = (I16_64){ i };         // { dg-warning "\\\[-Warray-bounds" }
+
+  char a64[64];               // { dg-message "'a64'" }
+  p = a64 + 1;
+  q = (I16_64*)p;
+  *q = (I16_64){ i };         // { dg-warning "\\\[-Warray-bounds" }
+  sink (p);
+}
diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-47.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-47.c
index cb2c329aa84..cc28d22864c 100644
--- a/gcc/testsuite/gcc.dg/Wstringop-overflow-47.c
+++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-47.c
@@ -24,17 +24,22 @@ void nowarn_c32 (char c)
   sink (p);
 }
 
+/* The tests below fail as a result of the hack for PR 96963.  However,
+   with -Wall, the invalid stores are diagnosed by -Warray-bounds which
+   runs before vectorization and so doesn't need the hack.  If/when
+   -Warray changes to use compute_objsize() this will need adjusting.  */
+
 void warn_c32 (char c)
 {
-  extern char warn_a32[32];   // { dg-message "at offset 32 into destination object 'warn_a32' of size 32" "note" }
+  extern char warn_a32[32];   // { dg-message "at offset 32 into destination object 'warn_a32' of size 32" "note" "pr97027" { xfail *-*-* } }
 
   void *p = warn_a32 + 1;
-  *(C32*)p = (C32){ c };      // { dg-warning "writing 1 byte into a region of size 0" }
+  *(C32*)p = (C32){ c };      // { dg-warning "writing 1 byte into a region of size 0" "pr97027" { xfail *-*-* } }
 
   /* Verify a local variable too. */
   char a32[32];
   p = a32 + 1;
-  *(C32*)p = (C32){ c };      // { dg-warning "writing 1 byte into a region of size 0" }
+  *(C32*)p = (C32){ c };      // { dg-warning "writing 1 byte into a region of size 0"  "pr97027" { xfail *-*-* } }
   sink (p);
 }
 
diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-65.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-65.c
new file mode 100644
index 00000000000..d3e301dbc2f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-65.c
@@ -0,0 +1,98 @@
+/* PR middle-end/96963 - -Wstringop-overflow false positive with
+   -ftree-vectorize when assigning consecutive char struct members
+   { dg-do compile }
+   { dg-options "-O2 -Wall -ftree-vectorize" } */
+
+void sink (void*);
+
+struct Char
+{
+  int i;
+  char c, d, e, f;
+  char a[2], b[2];
+};
+
+void nowarn_char_assign (struct Char *p)
+{
+  sink (&p->c);
+
+  /* Verify the bogus warning triggered by the tree-ssa-strlen.c pass
+     is not issued.  */
+  p->c = 1;         // { dg-bogus "\\\[-Wstringop-overflow"] }
+  p->d = 2;
+  p->e = 3;
+  p->f = 4;
+}
+
+void nowarn_char_array_assign (struct Char *p)
+{
+  sink (p->a);
+
+  p->a[0] = 1;      // { dg-bogus "\\\[-Wstringop-overflow"] }
+  p->a[1] = 2;
+  p->b[0] = 3;
+  p->b[1] = 4;
+}
+
+void warn_char_array_assign_interior (struct Char *p)
+{
+  sink (p->a);
+
+  p->a[0] = 1;
+  p->a[1] = 2;
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Warray-bounds"
+  /* Warnings are only suppressed for trailing arrays.  Verify
+     one is issued for an interior array.  */
+  p->a[2] = 5;      // { dg-warning "\\\[-Wstringop-overflow" }
+#pragma GCC diagnostic pop
+}
+
+void warn_char_array_assign_trailing (struct Char *p)
+{
+  /* This is separated from warn_char_array_assign_interior because
+     otherwise GCC removes the store to p->a[2] as dead since it's
+     overwritten by p->b[0].  */
+  sink (p->b);
+
+  p->b[0] = 3;
+  p->b[1] = 4;
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Warray-bounds"
+  /* Warnings are only suppressed for trailing arrays with at most
+     one element.  Verify one is issued for a two-element array.  */
+  p->b[2] = 5;      // { dg-warning "\\\[-Wstringop-overflow" }
+#pragma GCC diagnostic pop
+}
+
+
+/* Also verify there's no warning for other types than char (even though
+   the problem was limited to chars and -Wstringop-overflow should only
+   trigger for character accesses).  */
+
+struct Short
+{
+  int i;
+  short c, d, e, f;
+  short a[2], b[2];
+};
+
+void nowarn_short_assign (struct Short *p)
+{
+  sink (&p->c);
+
+  p->c = 1;
+  p->d = 2;
+  p->e = 3;
+  p->f = 4;
+}
+
+void nowarn_short_array_assign (struct Short *p)
+{
+  sink (p->a);
+
+  p->a[0] = 1;
+  p->a[1] = 2;
+  p->b[0] = 3;
+  p->b[1] = 4;
+}

PING [PATCH] correct fix to avoid false positives for vectorized stores (PR 96963, 94655)

[Martin Sebor]

Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564059.html

On 1/21/21 4:38 PM, Martin Sebor wrote:
> The hack I put in compute_objsize() last January for pr93200 isn't
> quite correct.  It happened to suppress the false positive there
> but, due to what looks like a thinko on my part, not in some other
> test cases involving vectorized stores.
> 
> The attached change adjusts the hack to have compute_objsize() give
> up on all MEM_REFs with a vector type.  This effectively disables
> -Wstringop-{overflow,overread} for vector accesses (either written
> by the user or synthesized by GCC from ordinary accesses).  It
> doesn't affect -Warray-bounds because this warning doesn't use
> compute_objsize() yet.  When it does (it should considerably
> simplify the code) some additional changes will be needed to
> preserve -Warray-bounds for out of bounds vector accesses.
> The test this patch adds should serve as a reminder to make
> it if we forget.
> 
> Tested on x86_64-linux.  Since PR 94655 was reported against GCC
> 10 I'd like to apply this fix to both the trunk and the 10 branch.
> 
> Martin

PING 2 [PATCH] correct fix to avoid false positives for vectorized stores (PR 96963, 94655)

[Martin Sebor]

Ping 2:
   https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564059.html

On 1/29/21 10:20 AM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564059.html
> 
> On 1/21/21 4:38 PM, Martin Sebor wrote:
>> The hack I put in compute_objsize() last January for pr93200 isn't
>> quite correct.  It happened to suppress the false positive there
>> but, due to what looks like a thinko on my part, not in some other
>> test cases involving vectorized stores.
>>
>> The attached change adjusts the hack to have compute_objsize() give
>> up on all MEM_REFs with a vector type.  This effectively disables
>> -Wstringop-{overflow,overread} for vector accesses (either written
>> by the user or synthesized by GCC from ordinary accesses).  It
>> doesn't affect -Warray-bounds because this warning doesn't use
>> compute_objsize() yet.  When it does (it should considerably
>> simplify the code) some additional changes will be needed to
>> preserve -Warray-bounds for out of bounds vector accesses.
>> The test this patch adds should serve as a reminder to make
>> it if we forget.
>>
>> Tested on x86_64-linux.  Since PR 94655 was reported against GCC
>> 10 I'd like to apply this fix to both the trunk and the 10 branch.
>>
>> Martin
> 

PING 3 [PATCH] correct fix to avoid false positives for vectorized stores (PR 96963, 94655)

[Martin Sebor]

Ping 3:
https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564059.html

This is a fix for two P2 bugs (false positives).

On 2/6/21 10:13 AM, Martin Sebor wrote:
> Ping 2:
>    https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564059.html
> 
> On 1/29/21 10:20 AM, Martin Sebor wrote:
>> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564059.html
>>
>> On 1/21/21 4:38 PM, Martin Sebor wrote:
>>> The hack I put in compute_objsize() last January for pr93200 isn't
>>> quite correct.  It happened to suppress the false positive there
>>> but, due to what looks like a thinko on my part, not in some other
>>> test cases involving vectorized stores.
>>>
>>> The attached change adjusts the hack to have compute_objsize() give
>>> up on all MEM_REFs with a vector type.  This effectively disables
>>> -Wstringop-{overflow,overread} for vector accesses (either written
>>> by the user or synthesized by GCC from ordinary accesses).  It
>>> doesn't affect -Warray-bounds because this warning doesn't use
>>> compute_objsize() yet.  When it does (it should considerably
>>> simplify the code) some additional changes will be needed to
>>> preserve -Warray-bounds for out of bounds vector accesses.
>>> The test this patch adds should serve as a reminder to make
>>> it if we forget.
>>>
>>> Tested on x86_64-linux.  Since PR 94655 was reported against GCC
>>> 10 I'd like to apply this fix to both the trunk and the 10 branch.
>>>
>>> Martin
>>
> 

PING 4 [PATCH] correct fix to avoid false positives for vectorized stores (PR 96963, 94655)

[Martin Sebor]

Ping 4:
https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564059.html

On 2/14/21 5:43 PM, Martin Sebor wrote:
> Ping 3:
> https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564059.html
> 
> This is a fix for two P2 bugs (false positives).
> 
> On 2/6/21 10:13 AM, Martin Sebor wrote:
>> Ping 2:
>>    https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564059.html
>>
>> On 1/29/21 10:20 AM, Martin Sebor wrote:
>>> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564059.html
>>>
>>> On 1/21/21 4:38 PM, Martin Sebor wrote:
>>>> The hack I put in compute_objsize() last January for pr93200 isn't
>>>> quite correct.  It happened to suppress the false positive there
>>>> but, due to what looks like a thinko on my part, not in some other
>>>> test cases involving vectorized stores.
>>>>
>>>> The attached change adjusts the hack to have compute_objsize() give
>>>> up on all MEM_REFs with a vector type.  This effectively disables
>>>> -Wstringop-{overflow,overread} for vector accesses (either written
>>>> by the user or synthesized by GCC from ordinary accesses).  It
>>>> doesn't affect -Warray-bounds because this warning doesn't use
>>>> compute_objsize() yet.  When it does (it should considerably
>>>> simplify the code) some additional changes will be needed to
>>>> preserve -Warray-bounds for out of bounds vector accesses.
>>>> The test this patch adds should serve as a reminder to make
>>>> it if we forget.
>>>>
>>>> Tested on x86_64-linux.  Since PR 94655 was reported against GCC
>>>> 10 I'd like to apply this fix to both the trunk and the 10 branch.
>>>>
>>>> Martin
>>>
>>
> 

PING 5 [PATCH] correct fix to avoid false positives for vectorized stores (PR 96963, 94655)

[Martin Sebor]

Ping 5:
https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564059.html

On 2/22/21 5:20 PM, Martin Sebor wrote:
> Ping 4:
> https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564059.html
> 
> On 2/14/21 5:43 PM, Martin Sebor wrote:
>> Ping 3:
>> https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564059.html
>>
>> This is a fix for two P2 bugs (false positives).
>>
>> On 2/6/21 10:13 AM, Martin Sebor wrote:
>>> Ping 2:
>>>    https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564059.html
>>>
>>> On 1/29/21 10:20 AM, Martin Sebor wrote:
>>>> Ping: 
>>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564059.html
>>>>
>>>> On 1/21/21 4:38 PM, Martin Sebor wrote:
>>>>> The hack I put in compute_objsize() last January for pr93200 isn't
>>>>> quite correct.  It happened to suppress the false positive there
>>>>> but, due to what looks like a thinko on my part, not in some other
>>>>> test cases involving vectorized stores.
>>>>>
>>>>> The attached change adjusts the hack to have compute_objsize() give
>>>>> up on all MEM_REFs with a vector type.  This effectively disables
>>>>> -Wstringop-{overflow,overread} for vector accesses (either written
>>>>> by the user or synthesized by GCC from ordinary accesses).  It
>>>>> doesn't affect -Warray-bounds because this warning doesn't use
>>>>> compute_objsize() yet.  When it does (it should considerably
>>>>> simplify the code) some additional changes will be needed to
>>>>> preserve -Warray-bounds for out of bounds vector accesses.
>>>>> The test this patch adds should serve as a reminder to make
>>>>> it if we forget.
>>>>>
>>>>> Tested on x86_64-linux.  Since PR 94655 was reported against GCC
>>>>> 10 I'd like to apply this fix to both the trunk and the 10 branch.
>>>>>
>>>>> Martin
>>>>
>>>
>>
> 

Re: [PATCH] correct fix to avoid false positives for vectorized stores (PR 96963, 94655)

[Richard Biener]

On Fri, Jan 22, 2021 at 12:39 AM Martin Sebor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> The hack I put in compute_objsize() last January for pr93200 isn't
> quite correct.  It happened to suppress the false positive there
> but, due to what looks like a thinko on my part, not in some other
> test cases involving vectorized stores.
>
> The attached change adjusts the hack to have compute_objsize() give
> up on all MEM_REFs with a vector type.  This effectively disables
> -Wstringop-{overflow,overread} for vector accesses (either written
> by the user or synthesized by GCC from ordinary accesses).  It
> doesn't affect -Warray-bounds because this warning doesn't use
> compute_objsize() yet.  When it does (it should considerably
> simplify the code) some additional changes will be needed to
> preserve -Warray-bounds for out of bounds vector accesses.
> The test this patch adds should serve as a reminder to make
> it if we forget.
>
> Tested on x86_64-linux.  Since PR 94655 was reported against GCC
> 10 I'd like to apply this fix to both the trunk and the 10 branch.

The proposed code reads even more confusingly than before.
What is called 'ptr' is treated as a reference and what you
then name ptrtype is the type of the reference.

That leads to code like

+       if (code == MEM_REF && TREE_CODE (ptrtype) == VECTOR_TYPE)

what?  the pointer type is a VECTOR_TYPE?!

I think you are looking for whether the reference type is a VECTOR_TYPE.

Part of the confusion is likely because the code commons

  if (code == ARRAY_REF || code == MEM_REF)

but in one case operand zero is a pointer to the object (MEM_REF) and
in one case
it is the object (ARRAY_REF).

Please refactor this code so one can actually read it literally
without second-guessing proper variable names.

Thanks,
Richard.



> Martin

Re: [PATCH] correct fix to avoid false positives for vectorized stores (PR 96963, 94655)

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 2567 bytes --]

On 3/2/21 3:39 AM, Richard Biener wrote:
> On Fri, Jan 22, 2021 at 12:39 AM Martin Sebor via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> The hack I put in compute_objsize() last January for pr93200 isn't
>> quite correct.  It happened to suppress the false positive there
>> but, due to what looks like a thinko on my part, not in some other
>> test cases involving vectorized stores.
>>
>> The attached change adjusts the hack to have compute_objsize() give
>> up on all MEM_REFs with a vector type.  This effectively disables
>> -Wstringop-{overflow,overread} for vector accesses (either written
>> by the user or synthesized by GCC from ordinary accesses).  It
>> doesn't affect -Warray-bounds because this warning doesn't use
>> compute_objsize() yet.  When it does (it should considerably
>> simplify the code) some additional changes will be needed to
>> preserve -Warray-bounds for out of bounds vector accesses.
>> The test this patch adds should serve as a reminder to make
>> it if we forget.
>>
>> Tested on x86_64-linux.  Since PR 94655 was reported against GCC
>> 10 I'd like to apply this fix to both the trunk and the 10 branch.
> 
> The proposed code reads even more confusingly than before.
> What is called 'ptr' is treated as a reference and what you
> then name ptrtype is the type of the reference.
> 
> That leads to code like
> 
> +       if (code == MEM_REF && TREE_CODE (ptrtype) == VECTOR_TYPE)
> 
> what?  the pointer type is a VECTOR_TYPE?!
> 
> I think you are looking for whether the reference type is a VECTOR_TYPE.
> 
> Part of the confusion is likely because the code commons
> 
>    if (code == ARRAY_REF || code == MEM_REF)
> 
> but in one case operand zero is a pointer to the object (MEM_REF) and
> in one case
> it is the object (ARRAY_REF).
> 
> Please refactor this code so one can actually read it literally
> without second-guessing proper variable names.

I agree that handling both REFs in the same block makes the code
more difficult to follow than it needs to be.

In the attached patch I've factored the code out into two helper
functions, one for ARRAY_REF and another for MEM_REF, and chosen
(hopefully) clearer names for the local variables.

A similar refactoring could be done for COMPONENT_REF and also
for SSA_NAME to reduce the size of the function and make it much
easier to follow.  I stopped short of doing that but I can do it
for GCC 12 when I move the whole compute_objsize() implementation
into a more appropriate  place (e.g., its own file).

Martin

> 
> Thanks,
> Richard.
> 
> 
> 
>> Martin


[-- Attachment #2: gcc-96963.diff --]
[-- Type: text/x-patch, Size: 14234 bytes --]

PR middle-end/96963 - -Wstringop-overflow false positive with -ftree-vectorize when assigning consecutive char struct members

gcc/ChangeLog:

	PR middle-end/96963
	* builtins.c (compute_objsize_r): Factor out ARRAY_REF and MEM_REF
	handling into helper functions.	 Correct a workaround for vectorized
	assignments.

gcc/testsuite/ChangeLog:

	PR middle-end/96963
	* gcc.dg/Wstringop-overflow-47.c: Xfail tests.
	* gcc.dg/Wstringop-overflow-65.c: New test.
	* gcc.dg/Warray-bounds-69.c: Same.

@@ -5210,7 +5209,7 @@ gimple_call_return_array (gimple *stmt, offset_int offrng[2],
   return NULL_TREE;
 }
 
-/* A helper of compute_objsize() to determine the size from an assignment
+/* A helper of compute_objsize_r() to determine the size from an assignment
    statement STMT with the RHS of either MIN_EXPR or MAX_EXPR.  */
 
 static bool
@@ -5288,6 +5287,129 @@ handle_min_max_size (gimple *stmt, int ostype, access_ref *pref,
   return true;
 }
 
+/* A helper of compute_objsize_r() to determine the size from ARRAY_REF
+   AREF.  ADDR is true if PTR is the operand of ADDR_EXPR.  Return true
+   on success and false on failure.  */
+
+static bool
+handle_array_ref (tree aref, bool addr, int ostype, access_ref *pref,
+		  ssa_name_limit_t &snlim, pointer_query *qry)
+{
+  gcc_assert (TREE_CODE (aref) == ARRAY_REF);
+
+  ++pref->deref;
+
+  tree arefop = TREE_OPERAND (aref, 0);
+  tree reftype = TREE_TYPE (arefop);
+  if (!addr && TREE_CODE (TREE_TYPE (reftype)) == POINTER_TYPE)
+    /* Avoid arrays of pointers.  FIXME: Hande pointers to arrays
+       of known bound.  */
+    return false;
+
+  if (!compute_objsize_r (arefop, ostype, pref, snlim, qry))
+    return false;
+
+  offset_int orng[2];
+  tree off = pref->eval (TREE_OPERAND (aref, 1));
+  range_query *const rvals = qry ? qry->rvals : NULL;
+  if (!get_offset_range (off, NULL, orng, rvals))
+    {
+      /* Set ORNG to the maximum offset representable in ptrdiff_t.  */
+      orng[1] = wi::to_offset (TYPE_MAX_VALUE (ptrdiff_type_node));
+      orng[0] = -orng[1] - 1;
+    }
+
+  /* Convert the array index range determined above to a byte
+     offset.  */
+  tree lowbnd = array_ref_low_bound (aref);
+  if (!integer_zerop (lowbnd) && tree_fits_uhwi_p (lowbnd))
+    {
+      /* Adjust the index by the low bound of the array domain
+	 (normally zero but 1 in Fortran).  */
+      unsigned HOST_WIDE_INT lb = tree_to_uhwi (lowbnd);
+      orng[0] -= lb;
+      orng[1] -= lb;
+    }
+
+  tree eltype = TREE_TYPE (aref);
+  tree tpsize = TYPE_SIZE_UNIT (eltype);
+  if (!tpsize || TREE_CODE (tpsize) != INTEGER_CST)
+    {
+      pref->add_max_offset ();
+      return true;
+    }
+
+  offset_int sz = wi::to_offset (tpsize);
+  orng[0] *= sz;
+  orng[1] *= sz;
+
+  if (ostype && TREE_CODE (eltype) == ARRAY_TYPE)
+    {
+      /* Except for the permissive raw memory functions which use
+	 the size of the whole object determined above, use the size
+	 of the referenced array.  Because the overall offset is from
+	 the beginning of the complete array object add this overall
+	 offset to the size of array.  */
+      offset_int sizrng[2] =
+	{
+	 pref->offrng[0] + orng[0] + sz,
+	 pref->offrng[1] + orng[1] + sz
+	};
+      if (sizrng[1] < sizrng[0])
+	std::swap (sizrng[0], sizrng[1]);
+      if (sizrng[0] >= 0 && sizrng[0] <= pref->sizrng[0])
+	pref->sizrng[0] = sizrng[0];
+      if (sizrng[1] >= 0 && sizrng[1] <= pref->sizrng[1])
+	pref->sizrng[1] = sizrng[1];
+    }
+
+  pref->add_offset (orng[0], orng[1]);
+  return true;
+}
+
+/* A helper of compute_objsize_r() to determine the size from MEM_REF
+   MREF.  Return true on success and false on failure.  */
+
+static bool
+handle_mem_ref (tree mref, int ostype, access_ref *pref,
+		ssa_name_limit_t &snlim, pointer_query *qry)
+{
+  gcc_assert (TREE_CODE (mref) == MEM_REF);
+
+  ++pref->deref;
+
+  if (TREE_CODE (TREE_TYPE (mref)) == VECTOR_TYPE)
+    {
+      /* Hack: Give up for MEM_REFs of vector types; those may be
+	 synthesized from multiple assignments to consecutive data
+	 members (see PR 93200 and 96963).
+	 FIXME: Vectorized assignments should only be present after
+	 vectorization so this hack is only necessary after it has
+	 run and could be avoided in calls from prior passes (e.g.,
+	 tree-ssa-strlen.c).
+	 FIXME: Deal with this more generally, e.g., by marking up
+	 such MEM_REFs at the time they're created.  */
+      return false;
+    }
+
+  tree mrefop = TREE_OPERAND (mref, 0);
+  if (!compute_objsize_r (mrefop, ostype, pref, snlim, qry))
+    return false;
+
+  offset_int orng[2];
+  tree off = pref->eval (TREE_OPERAND (mref, 1));
+  range_query *const rvals = qry ? qry->rvals : NULL;
+  if (!get_offset_range (off, NULL, orng, rvals))
+    {
+      /* Set ORNG to the maximum offset representable in ptrdiff_t.  */
+      orng[1] = wi::to_offset (TYPE_MAX_VALUE (ptrdiff_type_node));
+      orng[0] = -orng[1] - 1;
+    }
+
+  pref->add_offset (orng[0], orng[1]);
+  return true;
+}
+
 /* Helper to compute the size of the object referenced by the PTR
    expression which must have pointer type, using Object Size type
    OSTYPE (only the least significant 2 bits are used).
@@ -5420,92 +5542,11 @@ compute_objsize_r (tree ptr, int ostype, access_ref *pref,
       return true;
     }
 
-  if (code == ARRAY_REF || code == MEM_REF)
-    {
-      ++pref->deref;
-
-      tree ref = TREE_OPERAND (ptr, 0);
-      tree reftype = TREE_TYPE (ref);
-      if (!addr && code == ARRAY_REF
-	  && TREE_CODE (TREE_TYPE (reftype)) == POINTER_TYPE)
-	/* Avoid arrays of pointers.  FIXME: Hande pointers to arrays
-	   of known bound.  */
-	return false;
-
-      if (code == MEM_REF && TREE_CODE (reftype) == POINTER_TYPE)
-	{
-	  /* Give up for MEM_REFs of vector types; those may be synthesized
-	     from multiple assignments to consecutive data members.  See PR
-	     93200.
-	     FIXME: Deal with this more generally, e.g., by marking up such
-	     MEM_REFs at the time they're created.  */
-	  reftype = TREE_TYPE (reftype);
-	  if (TREE_CODE (reftype) == VECTOR_TYPE)
-	    return false;
-	}
-
-      if (!compute_objsize_r (ref, ostype, pref, snlim, qry))
-	return false;
-
-      offset_int orng[2];
-      tree off = pref->eval (TREE_OPERAND (ptr, 1));
-      if (!get_offset_range (off, NULL, orng, rvals))
-	{
-	  /* Set ORNG to the maximum offset representable in ptrdiff_t.  */
-	  orng[1] = wi::to_offset (TYPE_MAX_VALUE (ptrdiff_type_node));
-	  orng[0] = -orng[1] - 1;
-	}
-
-      if (TREE_CODE (ptr) == ARRAY_REF)
-	{
-	  /* Convert the array index range determined above to a byte
-	     offset.  */
-	  tree lowbnd = array_ref_low_bound (ptr);
-	  if (!integer_zerop (lowbnd) && tree_fits_uhwi_p (lowbnd))
-	    {
-	      /* Adjust the index by the low bound of the array domain
-		 (normally zero but 1 in Fortran).  */
-	      unsigned HOST_WIDE_INT lb = tree_to_uhwi (lowbnd);
-	      orng[0] -= lb;
-	      orng[1] -= lb;
-	    }
-
-	  tree eltype = TREE_TYPE (ptr);
-	  tree tpsize = TYPE_SIZE_UNIT (eltype);
-	  if (!tpsize || TREE_CODE (tpsize) != INTEGER_CST)
-	    {
-	      pref->add_max_offset ();
-	      return true;
-	    }
-
-	  offset_int sz = wi::to_offset (tpsize);
-	  orng[0] *= sz;
-	  orng[1] *= sz;
-
-	  if (ostype && TREE_CODE (eltype) == ARRAY_TYPE)
-	    {
-	      /* Except for the permissive raw memory functions which use
-		 the size of the whole object determined above, use the size
-		 of the referenced array.  Because the overall offset is from
-		 the beginning of the complete array object add this overall
-		 offset to the size of array.  */
-	      offset_int sizrng[2] =
-		{
-		 pref->offrng[0] + orng[0] + sz,
-		 pref->offrng[1] + orng[1] + sz
-		};
-	      if (sizrng[1] < sizrng[0])
-		std::swap (sizrng[0], sizrng[1]);
-	      if (sizrng[0] >= 0 && sizrng[0] <= pref->sizrng[0])
-		pref->sizrng[0] = sizrng[0];
-	      if (sizrng[1] >= 0 && sizrng[1] <= pref->sizrng[1])
-		pref->sizrng[1] = sizrng[1];
-	    }
-	}
+  if (code == ARRAY_REF)
+    return handle_array_ref (ptr, addr, ostype, pref, snlim, qry);
 
-      pref->add_offset (orng[0], orng[1]);
-      return true;
-    }
+  if (code == MEM_REF)
+    return handle_mem_ref (ptr, ostype, pref, snlim, qry);
 
   if (code == TARGET_MEM_REF)
     {
diff --git a/gcc/testsuite/gcc.dg/Warray-bounds-69.c b/gcc/testsuite/gcc.dg/Warray-bounds-69.c
new file mode 100644
index 00000000000..5a955774124
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Warray-bounds-69.c
@@ -0,0 +1,74 @@
+/* Verify that storing a bigger vector into smaller space is diagnosed.
+   { dg-do compile }
+   { dg-options "-O2 -Warray-bounds" } */
+
+typedef __INT16_TYPE__                         int16_t;
+typedef __attribute__ ((__vector_size__ (32))) char C32;
+
+typedef __attribute__ ((__vector_size__ (64))) int16_t I16_64;
+
+void sink (void*);
+
+
+void nowarn_c32 (char c)
+{
+  extern char nowarn_a32[32];
+
+  void *p = nowarn_a32;
+  *(C32*)p = (C32){ c };
+  sink (p);
+
+  char a32[32];
+  p = a32;
+  *(C32*)p = (C32){ c };
+  sink (p);
+}
+
+/* The invalid stores below are diagnosed by -Warray-bounds only
+   because it doesn't use compute_objsize().  If/when that changes
+   the function might need adjusting to avoid the hack put in place
+   to avoid false positives due to vectorization.  */
+
+void warn_c32 (char c)
+{
+  extern char warn_a32[32];   // { dg-message "'warn_a32'" "note" }
+
+  void *p = warn_a32 + 1;
+  *(C32*)p = (C32){ c };      // { dg-warning "\\\[-Warray-bounds" }
+
+  /* Verify a local variable too. */
+  char a32[32];               // { dg-message "'a32'" }
+  p = a32 + 1;
+  *(C32*)p = (C32){ c };      // { dg-warning "\\\[-Warray-bounds" }
+  sink (p);
+}
+
+
+void nowarn_i16_64 (int16_t i)
+{
+  extern char nowarn_a64[64];
+
+  void *p = nowarn_a64;
+  I16_64 *q = (I16_64*)p;
+  *q = (I16_64){ i };
+
+  char a64[64];
+  q = (I16_64*)a64;
+  *q = (I16_64){ i };
+  sink (q);
+}
+
+void warn_i16_64 (int16_t i)
+{
+  extern char warn_a64[64];   // { dg-message "'warn_a64'" }
+
+  void *p = warn_a64 + 1;
+  I16_64 *q = (I16_64*)p;
+  *q = (I16_64){ i };         // { dg-warning "\\\[-Warray-bounds" }
+
+  char a64[64];               // { dg-message "'a64'" }
+  p = a64 + 1;
+  q = (I16_64*)p;
+  *q = (I16_64){ i };         // { dg-warning "\\\[-Warray-bounds" }
+  sink (p);
+}
diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-47.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-47.c
index cb2c329aa84..9bfc84af569 100644
--- a/gcc/testsuite/gcc.dg/Wstringop-overflow-47.c
+++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-47.c
@@ -24,17 +24,22 @@ void nowarn_c32 (char c)
   sink (p);
 }
 
+/* The tests below fail as a result of the hack for PR 96963.  However,
+   with -Wall, the invalid stores are diagnosed by -Warray-bounds which
+   runs before vectorization and so doesn't need the hack.  If/when
+   -Warray changes to use compute_objsize() this will need adjusting.  */
+
 void warn_c32 (char c)
 {
-  extern char warn_a32[32];   // { dg-message "at offset 32 into destination object 'warn_a32' of size 32" "note" }
+  extern char warn_a32[32];   // { dg-message "at offset 32 into destination object 'warn_a32' of size 32" "pr97027" { xfail *-*-* } }
 
   void *p = warn_a32 + 1;
-  *(C32*)p = (C32){ c };      // { dg-warning "writing 1 byte into a region of size 0" }
+  *(C32*)p = (C32){ c };      // { dg-warning "writing 1 byte into a region of size 0" "pr97027" { xfail *-*-* } }
 
   /* Verify a local variable too. */
   char a32[32];
   p = a32 + 1;
-  *(C32*)p = (C32){ c };      // { dg-warning "writing 1 byte into a region of size 0" }
+  *(C32*)p = (C32){ c };      // { dg-warning "writing 1 byte into a region of size 0" "pr97027" { xfail *-*-* } }
   sink (p);
 }
 
diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-65.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-65.c
new file mode 100644
index 00000000000..9f82d73e311
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-65.c
@@ -0,0 +1,98 @@
+/* PR middle-end/96963 - -Wstringop-overflow false positive with
+   -ftree-vectorize when assigning consecutive char struct members
+   { dg-do compile }
+   { dg-options "-O2 -Wall -ftree-vectorize" } */
+
+void sink (void*);
+
+struct Char
+{
+  int i;
+  char c, d, e, f;
+  char a[2], b[2];
+};
+
+void nowarn_char_assign (struct Char *p)
+{
+  sink (&p->c);
+
+  /* Verify the bogus warning triggered by the tree-ssa-strlen.c pass
+     is not issued.  */
+  p->c = 1;         // { dg-bogus "\\\[-Wstringop-overflow" }
+  p->d = 2;
+  p->e = 3;
+  p->f = 4;
+}
+
+void nowarn_char_array_assign (struct Char *p)
+{
+  sink (p->a);
+
+  p->a[0] = 1;      // { dg-bogus "\\\[-Wstringop-overflow" }
+  p->a[1] = 2;
+  p->b[0] = 3;
+  p->b[1] = 4;
+}
+
+void warn_char_array_assign_interior (struct Char *p)
+{
+  sink (p->a);
+
+  p->a[0] = 1;
+  p->a[1] = 2;
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Warray-bounds"
+  /* Warnings are only suppressed for trailing arrays.  Verify
+     one is issued for an interior array.  */
+  p->a[2] = 5;      // { dg-warning "\\\[-Wstringop-overflow" }
+#pragma GCC diagnostic pop
+}
+
+void warn_char_array_assign_trailing (struct Char *p)
+{
+  /* This is separated from warn_char_array_assign_interior because
+     otherwise GCC removes the store to p->a[2] as dead since it's
+     overwritten by p->b[0].  */
+  sink (p->b);
+
+  p->b[0] = 3;
+  p->b[1] = 4;
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Warray-bounds"
+  /* Warnings are only suppressed for trailing arrays with at most
+     one element.  Verify one is issued for a two-element array.  */
+  p->b[2] = 5;      // { dg-warning "\\\[-Wstringop-overflow" }
+#pragma GCC diagnostic pop
+}
+
+
+/* Also verify there's no warning for other types than char (even though
+   the problem was limited to chars and -Wstringop-overflow should only
+   trigger for character accesses).  */
+
+struct Short
+{
+  int i;
+  short c, d, e, f;
+  short a[2], b[2];
+};
+
+void nowarn_short_assign (struct Short *p)
+{
+  sink (&p->c);
+
+  p->c = 1;
+  p->d = 2;
+  p->e = 3;
+  p->f = 4;
+}
+
+void nowarn_short_array_assign (struct Short *p)
+{
+  sink (p->a);
+
+  p->a[0] = 1;
+  p->a[1] = 2;
+  p->b[0] = 3;
+  p->b[1] = 4;
+}

Re: [PATCH] correct fix to avoid false positives for vectorized stores (PR 96963, 94655)

[Richard Biener]

On Tue, Mar 2, 2021 at 9:23 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 3/2/21 3:39 AM, Richard Biener wrote:
> > On Fri, Jan 22, 2021 at 12:39 AM Martin Sebor via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> The hack I put in compute_objsize() last January for pr93200 isn't
> >> quite correct.  It happened to suppress the false positive there
> >> but, due to what looks like a thinko on my part, not in some other
> >> test cases involving vectorized stores.
> >>
> >> The attached change adjusts the hack to have compute_objsize() give
> >> up on all MEM_REFs with a vector type.  This effectively disables
> >> -Wstringop-{overflow,overread} for vector accesses (either written
> >> by the user or synthesized by GCC from ordinary accesses).  It
> >> doesn't affect -Warray-bounds because this warning doesn't use
> >> compute_objsize() yet.  When it does (it should considerably
> >> simplify the code) some additional changes will be needed to
> >> preserve -Warray-bounds for out of bounds vector accesses.
> >> The test this patch adds should serve as a reminder to make
> >> it if we forget.
> >>
> >> Tested on x86_64-linux.  Since PR 94655 was reported against GCC
> >> 10 I'd like to apply this fix to both the trunk and the 10 branch.
> >
> > The proposed code reads even more confusingly than before.
> > What is called 'ptr' is treated as a reference and what you
> > then name ptrtype is the type of the reference.
> >
> > That leads to code like
> >
> > +       if (code == MEM_REF && TREE_CODE (ptrtype) == VECTOR_TYPE)
> >
> > what?  the pointer type is a VECTOR_TYPE?!
> >
> > I think you are looking for whether the reference type is a VECTOR_TYPE.
> >
> > Part of the confusion is likely because the code commons
> >
> >    if (code == ARRAY_REF || code == MEM_REF)
> >
> > but in one case operand zero is a pointer to the object (MEM_REF) and
> > in one case
> > it is the object (ARRAY_REF).
> >
> > Please refactor this code so one can actually read it literally
> > without second-guessing proper variable names.
>
> I agree that handling both REFs in the same block makes the code
> more difficult to follow than it needs to be.
>
> In the attached patch I've factored the code out into two helper
> functions, one for ARRAY_REF and another for MEM_REF, and chosen
> (hopefully) clearer names for the local variables.
>
> A similar refactoring could be done for COMPONENT_REF and also
> for SSA_NAME to reduce the size of the function and make it much
> easier to follow.  I stopped short of doing that but I can do it
> for GCC 12 when I move the whole compute_objsize() implementation
> into a more appropriate  place (e.g., its own file).

+  if (!addr && TREE_CODE (TREE_TYPE (reftype)) == POINTER_TYPE)

POINTER_TYPE_P ()

+    /* Avoid arrays of pointers.  FIXME: Hande pointers to arrays
+       of known bound.  */
+    return false;

+  if (TREE_CODE (TREE_TYPE (mref)) == VECTOR_TYPE)
+    {
+      /* Hack: Give up for MEM_REFs of vector types; those may be
+        synthesized from multiple assignments to consecutive data
+        members (see PR 93200 and 96963).

VECTOR_TYPE_P (TREE_TYPE (mref))

+        FIXME: Vectorized assignments should only be present after
+        vectorization so this hack is only necessary after it has
+        run and could be avoided in calls from prior passes (e.g.,
+        tree-ssa-strlen.c).

You could gate this on cfun->curr_properties & PROP_gimple_lvec
which is only set after vectorization.  Users can write vector
accesses using intrinsics or GCCs vector extension and I guess
warning for those is useful (and they less likely will cover multiple
fields).

Note the vectorizer also uses integer types for vector accesses
either when vectorizing using 'generic' vectors (for loads, stores
and bit operations we don't need any vector ISA) or when
composing vectors.

Note store-merging has the same issue (but fortunately runs later?)

OK with the above changes.

Thanks,
Richard.

> Martin
>
> >
> > Thanks,
> > Richard.
> >
> >
> >
> >> Martin
>

Re: [PATCH] correct fix to avoid false positives for vectorized stores (PR 96963, 94655)

[Martin Sebor]

On 3/3/21 3:31 AM, Richard Biener wrote:
> On Tue, Mar 2, 2021 at 9:23 PM Martin Sebor <msebor@gmail.com> wrote:
>>
>> On 3/2/21 3:39 AM, Richard Biener wrote:
>>> On Fri, Jan 22, 2021 at 12:39 AM Martin Sebor via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>
>>>> The hack I put in compute_objsize() last January for pr93200 isn't
>>>> quite correct.  It happened to suppress the false positive there
>>>> but, due to what looks like a thinko on my part, not in some other
>>>> test cases involving vectorized stores.
>>>>
>>>> The attached change adjusts the hack to have compute_objsize() give
>>>> up on all MEM_REFs with a vector type.  This effectively disables
>>>> -Wstringop-{overflow,overread} for vector accesses (either written
>>>> by the user or synthesized by GCC from ordinary accesses).  It
>>>> doesn't affect -Warray-bounds because this warning doesn't use
>>>> compute_objsize() yet.  When it does (it should considerably
>>>> simplify the code) some additional changes will be needed to
>>>> preserve -Warray-bounds for out of bounds vector accesses.
>>>> The test this patch adds should serve as a reminder to make
>>>> it if we forget.
>>>>
>>>> Tested on x86_64-linux.  Since PR 94655 was reported against GCC
>>>> 10 I'd like to apply this fix to both the trunk and the 10 branch.
>>>
>>> The proposed code reads even more confusingly than before.
>>> What is called 'ptr' is treated as a reference and what you
>>> then name ptrtype is the type of the reference.
>>>
>>> That leads to code like
>>>
>>> +       if (code == MEM_REF && TREE_CODE (ptrtype) == VECTOR_TYPE)
>>>
>>> what?  the pointer type is a VECTOR_TYPE?!
>>>
>>> I think you are looking for whether the reference type is a VECTOR_TYPE.
>>>
>>> Part of the confusion is likely because the code commons
>>>
>>>     if (code == ARRAY_REF || code == MEM_REF)
>>>
>>> but in one case operand zero is a pointer to the object (MEM_REF) and
>>> in one case
>>> it is the object (ARRAY_REF).
>>>
>>> Please refactor this code so one can actually read it literally
>>> without second-guessing proper variable names.
>>
>> I agree that handling both REFs in the same block makes the code
>> more difficult to follow than it needs to be.
>>
>> In the attached patch I've factored the code out into two helper
>> functions, one for ARRAY_REF and another for MEM_REF, and chosen
>> (hopefully) clearer names for the local variables.
>>
>> A similar refactoring could be done for COMPONENT_REF and also
>> for SSA_NAME to reduce the size of the function and make it much
>> easier to follow.  I stopped short of doing that but I can do it
>> for GCC 12 when I move the whole compute_objsize() implementation
>> into a more appropriate  place (e.g., its own file).
> 
> +  if (!addr && TREE_CODE (TREE_TYPE (reftype)) == POINTER_TYPE)
> 
> POINTER_TYPE_P ()

I think this is intentional: POINTER_TYPE_P() includes C++ references
and although they don't come up here (there's no such thing as arrays
of references) it didn't seem appropriate to be including them in
the test even if it's benign.

> 
> +    /* Avoid arrays of pointers.  FIXME: Hande pointers to arrays
> +       of known bound.  */
> +    return false;
> 
> +  if (TREE_CODE (TREE_TYPE (mref)) == VECTOR_TYPE)
> +    {
> +      /* Hack: Give up for MEM_REFs of vector types; those may be
> +        synthesized from multiple assignments to consecutive data
> +        members (see PR 93200 and 96963).
> 
> VECTOR_TYPE_P (TREE_TYPE (mref))

Okay.

> 
> +        FIXME: Vectorized assignments should only be present after
> +        vectorization so this hack is only necessary after it has
> +        run and could be avoided in calls from prior passes (e.g.,
> +        tree-ssa-strlen.c).
> 
> You could gate this on cfun->curr_properties & PROP_gimple_lvec
> which is only set after vectorization.  Users can write vector
> accesses using intrinsics or GCCs vector extension and I guess
> warning for those is useful (and they less likely will cover multiple
> fields).

Thanks, that's useful to know!  I'll keep it in mind for GCC 12.

> 
> Note the vectorizer also uses integer types for vector accesses
> either when vectorizing using 'generic' vectors (for loads, stores
> and bit operations we don't need any vector ISA) or when
> composing vectors.
> 
> Note store-merging has the same issue (but fortunately runs later?)

Yes, it's the same issue (as is FRE/PRE using one member to write
to the next) and it does cause false positives depending on where
the code is called from.  They're on my to do list for GCC 12 to
avoid as we discussed, by transform those into MEM_REFs with
the enclosing object + offset instead of the member.

> 
> OK with the above changes.

I've committed the patch without the POINTER_TYPE_P change (I'm
okay with it if you feel it necessary.)

Martin

> 
> Thanks,
> Richard.
> 
>> Martin
>>
>>>
>>> Thanks,
>>> Richard.
>>>
>>>
>>>
>>>> Martin
>>