On 10/8/21 4:49 AM, Aldy Hernandez via Gcc-patches wrote:
On Thu, Sep 23, 2021 at 8:32 AM Richard Biener via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
On Thu, 23 Sep 2021, Hongtao Liu wrote:
On Thu, Sep 23, 2021 at 9:48 AM Hongtao Liu <crazy...@gmail.com> wrote:
On Wed, Sep 22, 2021 at 10:21 PM Martin Sebor <mse...@gmail.com> wrote:
On 9/21/21 7:38 PM, Hongtao Liu wrote:
On Mon, Sep 20, 2021 at 4:13 AM Martin Sebor <mse...@gmail.com> wrote:
...
diff --git a/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c
b/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c
index 1d79930cd58..9351f7e7a1a 100644
--- a/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c
+++ b/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c
@@ -1,7 +1,7 @@
/* PR middle-end/91458 - inconsistent warning for writing past the end
of an array member
{ dg-do compile }
- { dg-options "-O2 -Wall -Wno-array-bounds -fno-ipa-icf" } */
+ { dg-options "-O2 -Wall -Wno-array-bounds -fno-ipa-icf -fno-tree-vectorize"
} */
The testcase is large - what part requires this change? Given the
testcase was added for inconsistent warnings do they now become
inconsistent again as we enable vectorization at -O2?
That said, the testcase adjustments need some explaining - I suppose
you didn't just slap -fno-tree-vectorize to all of those changing
behavior?
void ga1_ (void)
{
a1_.a[0] = 0;
a1_.a[1] = 1; // { dg-warning "\\\[-Wstringop-overflow" }
a1_.a[2] = 2; // { dg-warning "\\\[-Wstringop-overflow" }
struct A1 a;
a.a[0] = 0;
a.a[1] = 1; // { dg-warning "\\\[-Wstringop-overflow" }
a.a[2] = 2; // { dg-warning "\\\[-Wstringop-overflow" }
sink (&a);
}
It's supposed to be 2 warning for a.a[1] = 1 and a.a[2] = 1 since
there are 2 accesses, but after enabling vectorization, there's only
one access, so one warning is missing which causes the failure.
With the stores vectorized, is the warning on the correct line or
does it point to the first store, the one that's in bounds, as
it does with -O3? The latter would be a regression at -O2.
For the upper case, It points to the second store which is out of
bounds, the third store warning is missing.
I would find it preferable to change the test code over disabling
optimizations that are on by default. My concern is that the test
would no longer exercise the default behavior. (The same goes for
the -fno-ipa-icf option.)
Hmm, it's a middle-end test, for some backend, it may not do
vectorization(it depends on TARGET_VECTOR_MODE_SUPPORTED_P and
relative cost model).
Yes, there are quite a few warning tests like that. Their main
purpose is to verify that in common GCC invocations (i.e., without
any special options) warnings are a) issued when expected and b)
not issued when not expected. Otherwise, middle end warnings are
known to have both false positives and false negatives in some
invocations, depending on what optimizations are in effect.
Indiscriminately disabling common optimizations for these large
tests and invoking them under artificial conditions would
compromise this goal and hide the problems.
If enabling vectorization at -O2 causes regressions in the quality
of diagnostics (as the test failure above indicates seems to be
happening) we should investigate these and open bugs for them so
they can be fixed. We can then tweak the specific failing test
cases to avoid the failures until they are fixed.
There are indeed cases of false positives and false negatives
.i.e.
// Verify warning for access to a definition with an initializer that
// initializes the one-element array member.
struct A1 a1i_1 = { 0, { 1 } };
void ga1i_1 (void)
{
a1i_1.a[0] = 0;
a1i_1.a[1] = 1; // { dg-warning "\\\[-Wstringop-overflow" }
a1i_1.a[2] = 2; // { dg-warning "\\\[-Wstringop-overflow" }
struct A1 a = { 0, { 1 } }; --- false positive here.
a.a[0] = 1;
a.a[1] = 2; // { dg-warning
"\\\[-Wstringop-overflow" } false negative here.
a.a[2] = 3; // { dg-warning
"\\\[-Wstringop-overflow" } false negative here.
sink (&a);
}
Similar for
* gcc.dg/Warray-bounds-51.c.
* gcc.dg/Warray-parameter-3.c
* gcc.dg/Wstringop-overflow-14.c
* gcc.dg/Wstringop-overflow-21.c
So there're 3 situations.
1. All accesses are out of bound, and after vectorization, there are
some warnings missing.
2. Part of accesses are inbound, part of accesses are out of bound,
and after vectorization, the warning goes from out of bound line to
inbound line.
3. All access are out of bound, and after vectoriation, all warning
are missing, and goes to a false-positive line.
I remember some of the warning code explicitely excuses itself from
even trying to deal with vectorized loads/stores, that might need to
be revisited. It would also be useful to verify whether the line
info on the vectorized loads/stores is sensible (if you dump with
-lineno you get stmts with line numbers).
It is of course impossible to preserve the location of all original
scalar accesses exactly - it _might_ be possible to create a new
location range (not sure how they are exactly represented), but then
if we vectorize say
a[0] = b[0];
a[1] = b[1];
we'd somehow get a multi-line location range that covers unrelated
bits (the intermediate load or store).
So currently the vectorizer simply chooses the location of one of
the scalar loads or stores for the vectorized access (and it might do
so quite randomly).
Note I don't think it's feasible to not vectorize out-of-bound loads
or stores for the same reason that you'll say you can't do the
warnings all before vectorizing because the might expose themselves
only later.
So we simply have to cope with the reality that GCC is optimizing and
that the later we perform analysis for warnings the more the original
code is mangled. As we moved array-bound warnings before unrolling
so we should move this particular warning to before vectorization
[loop optimization].
Sorry I'm late to the party here...
Couldn't we run warnings earlier?
Yes, we could.
A while back, I converted the array bounds checker to the generic
range_query API, so it could be used either with the vr_values object
from the VRP pass, or with a ranger. There's no reason it should be
tied to VRP. We could make it its own pass and move it somewhere more
sensible.
The warning here (-Wstringop-overflow) comes from the strlen pass
which runs after vectorization which is why we lose the accurate
location for individual stores. VRP runs before it, so
-Warray-bounds doesn't suffer from the same problem.
Running the strlen subset of -Wstringop-overflow earlier will avoid
the problem:
https://gcc.gnu.org/pipermail/gcc-patches/2021-September/580040.html
It shouldn't even be necessary to move it -- -Warray-bounds should
be enough. I think the only difference is that -Wstringop-overflow
for plain char assignments respects subobject boundaries while
-Warray-bounds does not (for accesses of any type). That's a hack
to avoid some false positives so if we can find some other way to
avoid those we can improve -Warray-bounds and get rid of
-Wstringop-overflow for direct char stores (the warning was
meant for built-in string functions but it has grown to encompass
direct stores as well).
That said, it is useful to distinguish out-of-bounds reads from
out-of-bounds writes. -Wstringop-overflow and -Wstringop-overread
make that possible. -Warray-bounds doesn't. So maybe we also need
to enhance -Warry-bounds and add the corresponding modes.
Martin
Aldy
