Bug 102462

Summary: vectorization breaks diagnostic for array out of bound detect.
Product: gcc Reporter: Hongtao.liu <crazylht>
Component: tree-optimizationAssignee: Not yet assigned to anyone <unassigned>
Status: NEW ---    
Severity: normal CC: msebor, rguenth, sjames
Priority: P3 Keywords: diagnostic
Version: 12.0   
Target Milestone: ---   
See Also: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113026
Host: Target:
Build: Known to work:
Known to fail: Last reconfirmed: 2021-09-23 00:00:00
Bug Depends on:    
Bug Blocks: 88443, 102697    

Description Hongtao.liu 2021-09-23 03:07:49 UTC 
> > 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're 3 cases.

1. All accesses are out of bound, and after vectorization, there are
some warnings missing.(Because there only 1 access after vectorization, 2 accesses w/o vectorization, and diagnostic is based on access).
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.


below is case3:

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


Related testcases.

            * c-c++-common/Wstringop-overflow-2.c
            * gcc.dg/Warray-bounds-51.c: Ditto.
            * gcc.dg/Wstringop-overflow-14.c: Ditto.
            * gcc.dg/Wstringop-overflow-21.c: Ditto.
Comment 1 Hongtao.liu 2021-09-23 03:52:30 UTC 
The issue also exists for O3
Comment 2 Richard Biener 2021-09-23 06:46:28 UTC 
Can you reproduce compilable small testcases for all three cases here?  I can't find 'gali_1' and esp. how struct A1 is laid out.
Comment 3 Hongtao.liu 2021-09-23 07:01:29 UTC 
case3:
struct A1
{
  char n;
  char a[1];                    // { dg-message "destination object" "note" }
};
void sink (void*);
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 } };
  a.a[0] = 1;
  a.a[1] = 2;                   // { dg-warning "\\\[-Wstringop-overflow" }
  a.a[2] = 3;                   // { dg-warning "\\\[-Wstringop-overflow" }
  sink (&a);
}

I think i was wrong, there's no case3, case3 is just case2, part of access inbound, part is not, now the warning message is from struct A1 a = { 0, { 1 } }; which is recorded as the vectorized stmt lineno

  [case3.c:15:13] MEM <vector(4) char> [(char *)&a] = { 0, 1, 2, 3 };
Comment 4 Hongtao.liu 2021-09-23 07:03:42 UTC 
case1

struct A1
{
  char n;
  char a[1];                    // { dg-message "destination object" "note" }
};

struct A1 a1__ = { 0 };

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 = { 1 };
  a.a[0] = 0;
  a.a[1] = 1;                    // { dg-warning "\\\[-Wstringop-overflow" }
  a.a[2] = 2;                    // { dg-warning "\\\[-Wstringop-overflow" }
  sink (&a);
}
Comment 5 Hongtao.liu 2021-09-23 07:05:03 UTC 
It looks like vectorized stmt is always marked as the first access lineno.
Comment 6 Hongtao.liu 2021-09-23 07:38:18 UTC 
Move pass_strlen before loop passes

@@ -261,6 +261,7 @@ along with GCC; see the file COPYING3.  If not see
       NEXT_PASS (pass_tsan);
       NEXT_PASS (pass_dse);
       NEXT_PASS (pass_dce);
+      NEXT_PASS (pass_strlen);
       /* Pass group that runs when 1) enabled, 2) there are loops
         in the function.  Make sure to run pass_fix_loops before
         to discover/remove loops before running the gate function
@@ -334,7 +335,6 @@ along with GCC; see the file COPYING3.  If not see
          form if possible.  */
       NEXT_PASS (pass_thread_jumps);
       NEXT_PASS (pass_dominator, false /* may_peel_loop_headers_p */);
-      NEXT_PASS (pass_strlen);
       NEXT_PASS (pass_thread_jumps);
       NEXT_PASS (pass_vrp, false /* warn_array_bounds_p */);
       /* Threading can leave many const/copy propagations in the IL.

causes 54 new fails

c-c++-common/Wstringop-overflow.c  -Wc++-compat   (test for warnings, line 93)
c-c++-common/Wstringop-overflow.c  -Wc++-compat   (test for warnings, line 94)
c-c++-common/Wstringop-overflow.c  -std=gnu++14  (test for warnings, line 93)
c-c++-common/Wstringop-overflow.c  -std=gnu++14  (test for warnings, line 94)
c-c++-common/Wstringop-overflow.c  -std=gnu++17  (test for warnings, line 93)
c-c++-common/Wstringop-overflow.c  -std=gnu++17  (test for warnings, line 94)
c-c++-common/Wstringop-overflow.c  -std=gnu++2a  (test for warnings, line 93)
c-c++-common/Wstringop-overflow.c  -std=gnu++2a  (test for warnings, line 94)
c-c++-common/Wstringop-overflow.c  -std=gnu++98  (test for warnings, line 93)
c-c++-common/Wstringop-overflow.c  -std=gnu++98  (test for warnings, line 94)
g++.dg/tree-ssa/calloc.C  -std=gnu++14  scan-tree-dump-not optimized "malloc"
g++.dg/tree-ssa/calloc.C  -std=gnu++14  scan-tree-dump-not optimized "memset"
g++.dg/tree-ssa/calloc.C  -std=gnu++14  scan-tree-dump-times optimized "calloc" 1
g++.dg/tree-ssa/calloc.C  -std=gnu++17  scan-tree-dump-not optimized "malloc"
g++.dg/tree-ssa/calloc.C  -std=gnu++17  scan-tree-dump-not optimized "memset"
g++.dg/tree-ssa/calloc.C  -std=gnu++17  scan-tree-dump-times optimized "calloc" 1
g++.dg/tree-ssa/calloc.C  -std=gnu++2a  scan-tree-dump-not optimized "malloc"
g++.dg/tree-ssa/calloc.C  -std=gnu++2a  scan-tree-dump-not optimized "memset"
g++.dg/tree-ssa/calloc.C  -std=gnu++2a  scan-tree-dump-times optimized "calloc" 1
g++.dg/tree-ssa/calloc.C  -std=gnu++98  scan-tree-dump-not optimized "malloc"
g++.dg/tree-ssa/calloc.C  -std=gnu++98  scan-tree-dump-not optimized "memset"
g++.dg/tree-ssa/calloc.C  -std=gnu++98  scan-tree-dump-times optimized "calloc" 1
gcc.dg/Wstringop-overflow-17.c  (test for warnings, line 16)
gcc.dg/Wstringop-overflow-17.c  (test for warnings, line 9)
gcc.dg/Wstringop-overflow-70.c  (test for warnings, line 22)
gcc.dg/tree-ssa/pr95731.c scan-tree-dump-times optimized " >= 0| < 0" 6
gcc.dg/tree-ssa/pr96480.c scan-tree-dump optimized " = _[0-9]* <= 3;"
unix/-m32: c-c++-common/Wstringop-overflow.c  -Wc++-compat   (test for warnings, line 93)
unix/-m32: c-c++-common/Wstringop-overflow.c  -Wc++-compat   (test for warnings, line 94)
unix/-m32: c-c++-common/Wstringop-overflow.c  -std=gnu++14  (test for warnings, line 93)
unix/-m32: c-c++-common/Wstringop-overflow.c  -std=gnu++14  (test for warnings, line 94)
unix/-m32: c-c++-common/Wstringop-overflow.c  -std=gnu++17  (test for warnings, line 93)
unix/-m32: c-c++-common/Wstringop-overflow.c  -std=gnu++17  (test for warnings, line 94)
unix/-m32: c-c++-common/Wstringop-overflow.c  -std=gnu++2a  (test for warnings, line 93)
unix/-m32: c-c++-common/Wstringop-overflow.c  -std=gnu++2a  (test for warnings, line 94)
unix/-m32: c-c++-common/Wstringop-overflow.c  -std=gnu++98  (test for warnings, line 93)
unix/-m32: c-c++-common/Wstringop-overflow.c  -std=gnu++98  (test for warnings, line 94)
unix/-m32: g++.dg/tree-ssa/calloc.C  -std=gnu++14  scan-tree-dump-not optimized "malloc"
unix/-m32: g++.dg/tree-ssa/calloc.C  -std=gnu++14  scan-tree-dump-not optimized "memset"
unix/-m32: g++.dg/tree-ssa/calloc.C  -std=gnu++14  scan-tree-dump-times optimized "calloc" 1
unix/-m32: g++.dg/tree-ssa/calloc.C  -std=gnu++17  scan-tree-dump-not optimized "malloc"
unix/-m32: g++.dg/tree-ssa/calloc.C  -std=gnu++17  scan-tree-dump-not optimized "memset"
unix/-m32: g++.dg/tree-ssa/calloc.C  -std=gnu++17  scan-tree-dump-times optimized "calloc" 1
unix/-m32: g++.dg/tree-ssa/calloc.C  -std=gnu++2a  scan-tree-dump-not optimized "malloc"
unix/-m32: g++.dg/tree-ssa/calloc.C  -std=gnu++2a  scan-tree-dump-not optimized "memset"
unix/-m32: g++.dg/tree-ssa/calloc.C  -std=gnu++2a  scan-tree-dump-times optimized "calloc" 1
unix/-m32: g++.dg/tree-ssa/calloc.C  -std=gnu++98  scan-tree-dump-not optimized "malloc"
unix/-m32: g++.dg/tree-ssa/calloc.C  -std=gnu++98  scan-tree-dump-not optimized "memset"
unix/-m32: g++.dg/tree-ssa/calloc.C  -std=gnu++98  scan-tree-dump-times optimized "calloc" 1
unix/-m32: gcc.dg/Wstringop-overflow-17.c  (test for warnings, line 16)
unix/-m32: gcc.dg/Wstringop-overflow-17.c  (test for warnings, line 9)
unix/-m32: gcc.dg/Wstringop-overflow-70.c  (test for warnings, line 22)
unix/-m32: gcc.dg/tree-ssa/pr95731.c scan-tree-dump-times optimized " >= 0| < 0" 6
unix/-m32: gcc.dg/tree-ssa/pr96480.c scan-tree-dump optimized " = _[0-9]* <= 3;"
Comment 7 Martin Sebor 2021-09-23 14:44:41 UTC 
One thing to note here is that the three Wstringop-overflow tests mentioned in comment #0 disable -Warray-bounds.  With the warning enabled the affected cases will (should) continue to trigger the expected diagnostics on the expected lines (and not on the in bounds accesses).

I.e., the default GCC invocation (with no special codegen or warning suppression options) should be unaffected by the -O2 -> -O3 change, and so the regression in the quality of these diagnostics can be viewed as only minor.

That -Warray-bounds is issued on the correct lines for these cases also confirms the viability of the idea of moving the strlen subset -Wstringop-overflow warnings into the -Warray-bounds pass.  (As comment #6 implies, moving the whole strlen pass would likely have bigger repercussions and is not a suitable change just to maintain warning locations).

Alternatively, since -Wstringop-overflow is documented to "warn for calls to string manipulation functions" it might make sense to consider disabling the -Wstringop-overflow warnings issued from the strlen pass as long as they're all handled by -Warray-bounds.  (I'm not sure they are at present: I think out-of-bounds subobject accesses are not detected.)
Comment 8 Hongtao.liu 2021-09-26 02:19:11 UTC 
I got a case3 from Wstringop-overflow-76.c

#define MAX(p, q) ((p) > (q) ? (p) : (q))
struct B4_B6
{
  char b4[4];
  char b6[6];       // { dg-message "at offset 6 into destination object 'b6' of size 6" "note" }
};

void max_B6_B4 (int i, struct B4_B6 *pb4_b6)
{
char *p = pb4_b6->b6 + i;
char *q = pb4_b6->b4 + i;
  char *d = MAX (p, q);

  d[3] = 0;
  d[4] = 0;
  d[5] = 0;
  d[6] = 0;         // { dg-warning "writing 1 byte into a region of size 0 " }

After vectorization, no warning message output, option: -O2 -Wno-array-bounds

Expected output warning message

test.c: In function ‘max_B6_B4’:
test.c:17:8: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
   17 |   d[6] = 0;         // { dg-warning "writing 1 byte into a region of size 0 "" { xfail vect_int }" }
      |   ~~~~~^~~
test.c:5:8: note: at offset 6 into destination object ‘b6’ of size 6
    5 |   char b6[6];       // { dg-message "at offset \[^a-zA-Z\n\r\]*6\[^a-zA-Z0-9\]* into destination object 'b6' of size 6" "note" { xfail vect_int } }
      |        ^~
Comment 9 Hongtao.liu 2021-10-13 02:30:06 UTC 
case 1: All accesses are out of bound, and after vectorization, there are
some warnings missing.(Because there only 1 access after vectorization, 2 accesses w/o vectorization, and diagnostic is based on access).

from c-c++-common/Wstringop-overflow-2.c
struct A1 a1__ = { 0 };

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 = { 1 };
  a.a[0] = 0;
// After vectorization, below codes are optimized to
// vector(2) char = { 1, 2}, there's only 1 access remained, so add xfail
// to a.a[2] = 2, refer to pr102462.
  a.a[1] = 1;                    // { dg-warning "\\\[-Wstringop-overflow" }
  a.a[2] = 2;                    // { dg-warning "\\\[-Wstringop-overflow" "" { xfail { i?86-*-* x86_64-*-* } } }
  sink (&a);
}

struct A1 a1_0 = { 0, { } };

void ga1_0_ (void)
{
  a1_0.a[0] = 0;
  a1_0.a[1] = 1;                // { dg-warning "\\\[-Wstringop-overflow" }
  a1_0.a[2] = 2;                // { dg-warning "\\\[-Wstringop-overflow" }

  struct A1 a = { 1, { } };
  a.a[0] = 0;
  a.a[1] = 1;                   // { dg-warning "\\\[-Wstringop-overflow" }
  a.a[2] = 2;                   // { dg-warning "\\\[-Wstringop-overflow" "" { xfail { i?86-*-* x86_64-*-* } } }
  sink (&a);
}

struct A1i a1i__ = { 0 };

void ga1i__ (void)
{
  a1i__.a[0] = 0;
  a1i__.a[1] = 1;                // { dg-warning "\\\[-Wstringop-overflow" }
  a1i__.a[2] = 2;                // { dg-warning "\\\[-Wstringop-overflow" }

  struct A1i a = { 0 };
  a.a[0] = 0;
  a.a[1] = 1;                    // { dg-warning "\\\[-Wstringop-overflow" }
  a.a[2] = 2;                    // { dg-warning "\\\[-Wstringop-overflow" "" { xfail { i?86-*-* x86_64-*-* } } }
  sink (&a);
}

struct A1 a1i_0 = { 0, { } };

void ga1i_0_ (void)
{
  a1i_0.a[0] = 0;
  a1i_0.a[1] = 1;               // { dg-warning "\\\[-Wstringop-overflow" }
  a1i_0.a[2] = 2;               // { dg-warning "\\\[-Wstringop-overflow" }

  struct A1 a = { 0, { } };
  a.a[0] = 0;
  a.a[1] = 1;                   // { dg-warning "\\\[-Wstringop-overflow" }
  a.a[2] = 2;                   // { dg-warning "\\\[-Wstringop-overflow" "" { xfail { i?86-*-* x86_64-*-* } } }
  sink (&a);
}
Comment 10 GCC Commits 2021-10-20 02:13:35 UTC 
The master branch has been updated by hongtao Liu <liuhongt@gcc.gnu.org>:

https://gcc.gnu.org/g:3c8d8c0be95e99dc0cba7f6fad2429243582119f

commit r12-4523-g3c8d8c0be95e99dc0cba7f6fad2429243582119f
Author: liuhongt <hongtao.liu@intel.com>
Date:   Thu Oct 14 09:31:03 2021 +0800

    Adjust testcase for O2 vectorization.
    
    As discussed in [1], this patch add xfail/target selector to those
    testcases, also make a copy of them so that they can be tested w/o
    vectorization.
    
    Newly added xfail/target selectors are used to check the vectorization
    capability of continuous byte/double bytes storage, these scenarios
    are exactly the part of the testcases that regressed after O2
    vectorization.
    
    [1] https://gcc.gnu.org/pipermail/gcc-patches/2021-October/581456.html.
    
    2021-10-19  Hongtao Liu  <hongtao.liu@intel.com>
                Kewen Lin  <linkw@linux.ibm.com>
    
    gcc/ChangeLog
    
            * doc/sourcebuild.texi (Effective-Target Keywords): Document
            vect_slp_v2qi_store, vect_slp_v4qi_store, vect_slp_v8qi_store,
            vect_slp_v16qi_store, vect_slp_v2hi_store,
            vect_slp_v4hi_store, vect_slp_v2si_store, vect_slp_v4si_store.
    
    gcc/testsuite/ChangeLog
    
            PR middle-end/102722
            PR middle-end/102697
            PR middle-end/102462
            PR middle-end/102706
            PR middle-end/102744
            * c-c++-common/Wstringop-overflow-2.c: Adjust testcase with new
            xfail/target selector.
            * gcc.dg/Warray-bounds-51.c: Ditto.
            * gcc.dg/Warray-parameter-3.c: Ditto.
            * gcc.dg/Wstringop-overflow-14.c: Ditto.
            * gcc.dg/Wstringop-overflow-21.c: Ditto.
            * gcc.dg/Wstringop-overflow-68.c: Ditto.
            * gcc.dg/Wstringop-overflow-76.c: Ditto.
            * gcc.dg/Warray-bounds-48.c: Ditto.
            * gcc.dg/Wzero-length-array-bounds-2.c: Ditto.
            * lib/target-supports.exp (check_vect_slp_aligned_store_usage):
            New function.
            (check_effective_target_vect_slp_v2qi_store): Ditto.
            (check_effective_target_vect_slp_v4qi_store): Ditto.
            (check_effective_target_vect_slp_v8qi_store): Ditto.
            (check_effective_target_vect_slp_v16qi_store): Ditto.
            (check_effective_target_vect_slp_v2hi_store): Ditto.
            (check_effective_target_vect_slp_v4hi_store): Ditto.
            (check_effective_target_vect_slp_v2si_store): Ditto.
            (check_effective_target_vect_slp_v4si_store): Ditto.
            * c-c++-common/Wstringop-overflow-2-novec.c: New test.
            * gcc.dg/Warray-bounds-51-novec.c: New test.
            * gcc.dg/Warray-bounds-48-novec.c: New test.
            * gcc.dg/Warray-parameter-3-novec.c: New test.
            * gcc.dg/Wstringop-overflow-14-novec.c: New test.
            * gcc.dg/Wstringop-overflow-21-novec.c: New test.
            * gcc.dg/Wstringop-overflow-76-novec.c: New test.
            * gcc.dg/Wzero-length-array-bounds-2-novec.c: New test.