On Wed, Sep 22, 2021 at 10:21 PM Martin Sebor <[email protected]> wrote:
>
> On 9/21/21 7:38 PM, Hongtao Liu wrote:
> > On Mon, Sep 20, 2021 at 4:13 AM Martin Sebor <[email protected]> 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);
}
the last 2 warnings are missing, and there's new warning on the line
*struct A1 a = { 0, { 1 } };
>
> Martin
--
BR,
Hongtao
