[Bug tree-optimization/63841] New: Incorrect strlen optimization after complete unroll

[tejohnson at google dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63841

            Bug ID: 63841
           Summary: Incorrect strlen optimization after complete unroll
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: tejohnson at google dot com

The following test fails with trunk:

$ cat bug_test.cc
#include <cstdio>
#include <cassert>
#include <string>

std::string __attribute__ ((noinline)) comp_test_write() {
  std::string data;

  for (int i = 0; i < 2; ++i) {
    char b = 1 >> (i * 8);
    data.append(&b, 1);
  }

  return data;
}

std::string __attribute__ ((noinline)) comp_test_write_good() {
  std::string data;

  char b;
  for (int i = 0; i < 2; ++i) {
    b = 1 >> (i * 8);
    data.append(&b, 1);
  }

  return data;
}

int main() {
  std::string bad = comp_test_write();
  printf("BAD: %hx\n", *(short*)bad.c_str());

  std::string good = comp_test_write_good();
  printf("GOOD: %hx\n", *(short*)good.c_str());

  return good != bad;
}

% g++ bug_test.cc -O2 
% a.out
BAD: 101
GOOD: 1

Notice that the difference between comp_test_write_good (GOOD) and
comp_test_write (BAD) is that declaration "char b" is moved outside the loop in
the good case.

In both cases cunrolli completely unrolls the loop. The main difference is that
there is a clobber at the start of the second unrolled iteration in the bad
case:

   # .MEM_26 = VDEF <.MEM_25>
  bD.13054 ={v} {CLOBBER};
  [./bug_test2.cc : 8:3] # DEBUG iD.13053 => 1
  # DEBUG iD.13053 => 1
  [./bug_test2.cc : 9:25] # .MEM_33 = VDEF <.MEM_26>
  bD.13054 = 0;
  [./bug_test2.cc : 10:23] [LP 2] # .MEM_34 = VDEF <.MEM_33>
  # USE = nonlocal null { D.12110 D.13054 D.13371 } (glob)
  # CLB = nonlocal null { D.12110 D.13054 D.13371 } (glob)
  _ZNSs6appendEPKcmD.11551 (data_4(D), &bD.13054, 1);

Above is the code just before tree-strlen. The tree-strlen removes the
"bD.13054 = 0" incorrectly. If I compile with -fdisable-tree-strlen the test
passes.

I tracked this down to the code in handle_char_store in tree-ssa-strlen.c with
the following comment:

              /* When storing '\0', the store can be removed
                 if we know it has been stored in the current function.  */

When we hit this routine for the store "bD.13054 = 0" there is a saved strinfo
struct that indicates there was a store to this string with length 0, and it
thinks it is safe to remove this null termination store.

The strinfo struct was created during the prior call to handle_char_store for
store "bD.13054 ={v} {CLOBBER}". It is created with length zero because
initializer_zerop returns true for the RHS. The reason initializer_zerop
returns true is that the RHS is a constructor with 0 length. Therefore this
code in initializer_zerop:
        FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt)
          if (!initializer_zerop (elt))
            return false;
        return true;
falls through to the "return true" without executing any loop iterations.

A couple possible solutions that all work:
1) handle_char_store does not consider the store RHS to be a zero
initialization if the statement is a clobber.
2) initializer_zerop returns false if the constructor is a clobber
3) initializer_zerop returns false if there are no elements in the constructor
array.

It seems to me like #3 is the correct solution - I don't think an empty
constructor should be considered a zero initialization. The regression tests
pass with that change so I will send it for review