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
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;
+}
