Message ID | 3e1c6283-ddd9-0607-695b-361aea880a0b@gmail.com |
---|---|
State | New |
Headers | show |
Series | avoid warning on vectorized past-the-end stores (PR 93200) | expand |
On Wed, 2020-01-08 at 17:23 +0000, Martin Sebor wrote: > A recent improvement to the vectorizer (r278334 if my bisection > is right) can transform multiple stores to adjacent struct members > into single vectorized assignments that write over all the members > in a single MEM_REF. These are then flagged by -Wstringop-overflow > thanks to its also recently enhanced past-the-end store detection. > The warnings have been causing failures in some of Jeff's periodic > builds (e.g., in cjdns-v20.4). > > Reliably distinguishing these transformed, multi-member, MEM_REF > stores from accidental bugs the warning is designed to detect will > require annotating them somehow at the time they are introduced. > Until that happens, the attached patch simply tweaks the logic that > determines the size of the destination objects to punt on these > vectorized MEM_REFs. I thought we had other code which could combine stores into consecutive memory locations that might run afoul of this warning as well. But I can't seem to find that code -- we may well have throttled it a while back because of data store races. OK. Thanks for taking care of this so quickly. jeff
On Wed, Jan 08, 2020 at 09:24:48PM -0700, Jeff Law wrote: > On Wed, 2020-01-08 at 17:23 +0000, Martin Sebor wrote: > > A recent improvement to the vectorizer (r278334 if my bisection > > is right) can transform multiple stores to adjacent struct members > > into single vectorized assignments that write over all the members > > in a single MEM_REF. These are then flagged by -Wstringop-overflow > > thanks to its also recently enhanced past-the-end store detection. > > The warnings have been causing failures in some of Jeff's periodic > > builds (e.g., in cjdns-v20.4). > > > > Reliably distinguishing these transformed, multi-member, MEM_REF > > stores from accidental bugs the warning is designed to detect will > > require annotating them somehow at the time they are introduced. > > Until that happens, the attached patch simply tweaks the logic that > > determines the size of the destination objects to punt on these > > vectorized MEM_REFs. > I thought we had other code which could combine stores into consecutive > memory locations that might run afoul of this warning as well. But I > can't seem to find that code -- we may well have throttled it a while > back because of data store races. E.g. store-merging does that, but that runs fairly late. And as it is mentioned in other PRs, if the warning code determines anything e.g. from pointers used in MEM_REFs, then any kind of value numbering can cause that, as for pointers the optimizers only care about the value and not access path. So all those cases e.g. with union where we access one union member at one point and another one later in the same function and the two pointers through which the access is made have the same value. Jakub
On 1/9/20 9:24 PM, Jeff Law wrote: > On Wed, 2020-01-08 at 17:23 +0000, Martin Sebor wrote: >> A recent improvement to the vectorizer (r278334 if my bisection >> is right) can transform multiple stores to adjacent struct members >> into single vectorized assignments that write over all the members >> in a single MEM_REF. These are then flagged by -Wstringop-overflow >> thanks to its also recently enhanced past-the-end store detection. >> The warnings have been causing failures in some of Jeff's periodic >> builds (e.g., in cjdns-v20.4). >> >> Reliably distinguishing these transformed, multi-member, MEM_REF >> stores from accidental bugs the warning is designed to detect will >> require annotating them somehow at the time they are introduced. >> Until that happens, the attached patch simply tweaks the logic that >> determines the size of the destination objects to punt on these >> vectorized MEM_REFs. > I thought we had other code which could combine stores into consecutive > memory locations that might run afoul of this warning as well. But I > can't seem to find that code -- we may well have throttled it a while > back because of data store races. The only other case I can think of is stores into consecutive array elements. Those can get merged into MEM_REF <T> where T is either a bigger scalar type than the array element type or an array of such a type (or, apparently only at -O3, a vector). But (AFAIK) those are handled correctly (both by the strlen pass and by compute_objsize and the warning). For example this: struct S { char a[4]; } s; void f (void) { s.a[0] = '1'; s.a[1] = '2'; s.a[2] = '2'; s.a[3] = 0; } at -O2 results in: MEM <unsigned int> [(char *)&s] = 3289649; and at -O3 in: MEM <vector(4) char> [(char *)&s] = { 49, 50, 50, 0 }; (For reasons I don't yet understand it doesn't happen when the struct object is local to the function or referenced by a pointer passed to the function.) What I hadn't seen or anticipate is the merging of stores into distinct members. It's possible that some valid cases involving unions could trigger warnings. I haven't seen GCC introduce such transformations for "clean" code but as I said above, I will look into avoiding warnings for such code if/when I come across it. Other than that, I'm not concerned about issuing warnings for user code that abuses unions to get around the type system. Authors of code that skirts the rules or good practices need to be prepared to deal with warnings. Martin > > OK. Thanks for taking care of this so quickly. > > jeff > >
PR middle-end/93200 - spurious -Wstringop-overflow due to assignment vectorization to multiple members gcc/testsuite/ChangeLog: PR middle-end/93200 * gcc.dg/Wstringop-overflow-30.c: New test. gcc/ChangeLog: PR middle-end/93200 * builtins.c (compute_objsize): Avoid handling MEM_REFs of vector type. Index: gcc/builtins.c =================================================================== --- gcc/builtins.c (revision 280012) +++ gcc/builtins.c (working copy) @@ -3966,6 +3966,18 @@ || TREE_CODE (dest) == MEM_REF) { tree ref = TREE_OPERAND (dest, 0); + tree reftype = TREE_TYPE (ref); + if (TREE_CODE (dest) == 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 NULL_TREE; + } tree off = TREE_OPERAND (dest, 1); if (tree size = compute_objsize (ref, ostype, pdecl, poff)) { Index: gcc/testsuite/gcc.dg/Wstringop-overflow-30.c =================================================================== --- gcc/testsuite/gcc.dg/Wstringop-overflow-30.c (nonexistent) +++ gcc/testsuite/gcc.dg/Wstringop-overflow-30.c (working copy) @@ -0,0 +1,80 @@ +/* PR middle-end/93200 - spurious -Wstringop-overflow due to assignment + vectorization to multiple members + { dg-do compile } + { dg-options "-O3 -Wall" } */ + +typedef __INT8_TYPE__ int8_t; +typedef __INT16_TYPE__ int16_t; +typedef __INT32_TYPE__ int32_t; +typedef __INT64_TYPE__ int64_t; + +struct A { char b, c; }; +struct B1A { int8_t i8; struct A a; }; +struct B2A { int16_t i16; struct A a; }; +struct B3A { int16_t i16; int8_t i8; struct A a; }; +struct B4A { int64_t i64; struct A a; }; + +void ba1 (struct B1A *p) +{ + p->a.b = 0; p->a.c = 1; +} + +void b2a (struct B2A *p) +{ + /* This results in: + vector(2) char *vectp.14_6 = &p_2(D)->a.b; + MEM <vector(2) char> [(char *)vectp.14_6] = { 4, 5 }; */ + + p->a.b = 4; // { dg-bogus "-Wstringop-overflow" } + p->a.c = 5; +} + +void b3a (struct B3A *p) +{ + p->a.b = 4; p->a.c = 5; +} + +void b4a (struct B4A *p) +{ + /* This results in: + vector(2) char *vectp.22_6 = &p_2(D)->a.b; + MEM <vector(2) char> [(char *)vectp.22_6] = { 6, 7 }; */ + + p->a.b = 6; // { dg-bogus "-Wstringop-overflow" } + p->a.c = 7; +} + + +struct Aa { char a[2], b[2]; }; +struct B1Aa { int8_t i8; struct Aa a; }; +struct B2Aa { int16_t i16; struct Aa a; }; +struct B3Aa { int16_t i16; int8_t i8; struct Aa a; }; +struct B4Aa { int64_t i64; struct Aa a; }; + +void b1aa (struct B1Aa *p) +{ + p->a.a[0] = 0; p->a.a[1] = 1; + p->a.b[0] = 0; p->a.b[1] = 1; +} + +void b2aa (struct B2Aa *p) +{ + p->a.a[0] = 2; p->a.a[1] = 3; + p->a.b[0] = 2; p->a.b[1] = 3; +} + +void b3aa (struct B3Aa *p) +{ + p->a.a[0] = 4; p->a.a[1] = 5; + p->a.b[0] = 4; p->a.b[1] = 5; +} + +void b4aa (struct B4Aa *p) +{ + /* This results in: + vector(4) char *vectp.36_8 = &p_2(D)->a.a[0]; + MEM <vector(4) char> [(char *)vectp.36_8] = { 6, 7, 6, 7 }; */ + + p->a.a[0] = 6; p->a.a[1] = 7; + p->a.b[0] = 6; p->a.b[1] = 7; +}