[PATCH] constrain one character optimization to one character stores (PR 90989)

[Jakub Jelinek]

On Thu, Jun 27, 2019 at 09:15:41AM -0600, Jeff Law wrote:
> Actually it was trivial to create with store merging.
> 
> char x[20];
> foo()
> {
>   x[0] = 0x41;
>   x[1] = 0x42;
> }
> 
>   MEM <unsigned short> [(char *)&x] = 16961;
> 
> So clearly we can get this in gimple.  So we need a check of some kind,
> either on the LHS or the RHS to ensure that we actually have a single
> character store as opposed to something like the above.

handle_char_store is only called if:
          tree type = TREE_TYPE (lhs);
          if (TREE_CODE (type) == ARRAY_TYPE)
            type = TREE_TYPE (type);
          if (TREE_CODE (type) == INTEGER_TYPE
              && TYPE_MODE (type) == TYPE_MODE (char_type_node)
              && TYPE_PRECISION (type) == TYPE_PRECISION (char_type_node))
            {
              if (! handle_char_store (gsi))
                return false;
            }
so the above case will not trigger, the only way to call it for multiple
character stores is if you have an array.  And so far I've succeeded
creating that just with strcpy builtin.
E.g. the
      if (storing_all_zeros_p && cmp == 0 && si->full_string_p)
        {
          /* When overwriting a '\0' with a '\0', the store can be removed
             if we know it has been stored in the current function.  */
          if (!stmt_could_throw_p (cfun, stmt) && si->writable)
optimization looks unsafe if we are actually storing more than one zero
because then the first zero in there is redundant, but there could be
non-zero chars after that and removing the larger all zeros store will
keep those other chars with incorrect values; I've tried to reproduce it with:
__attribute__((noipa)) void
bar (char *x, int l1, int l2)
{
  if (__builtin_memcmp (x, "abcd\0", 6) != 0 || l1 != 4 || l2 != 4)
    __builtin_abort ();
}

int
main ()
{
  char x[64];
  __builtin_memset (x, -1, sizeof (x));
  asm volatile ("" : : "g" (&x[0]) : "memory");
  __builtin_strcpy (x, "abcd");
  int l1 = __builtin_strlen (x);
#ifdef ONE
  short __attribute__((may_alias)) *p = (void *) &x[0];
  *(p + 2) = 0;
#else
  short *p = (short *) (&x[0] + 4);
  __builtin_memcpy (p, "\0\0", 2);
#endif
  int l2 = __builtin_strlen (x);
  bar (x, l1, l2);
}

but the first case creates a short store, so handle_char_store isn't
called, and second for some reason isn't folded from memcpy to a char[2]
store.

Fundamentally, there is no reason to guard handle_char_store with
char or array of char stores, as long as CHAR_BIT == 8 && BITS_PER_UNIT == 8
and native_encode_expr can handle the rhs expression - then we can analyze
exactly where is the first '\0' character in there and in the code take it
into account.

	Jakub