Re: [committed] Re: PATCH: fine-tuning for can_store_by_pieces

[Jakub Jelinek <jakub@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3273 bytes --]

On Sat, Aug 25, 2007 at 02:59:54AM -0400, Jakub Jelinek wrote:
> *************** store_expr (tree exp, rtx target, int ca                                                                         
> *** 4507,4513 ****                                                                                                               
>                                   str_copy_len, builtin_strncpy_read_str,                                                        
>                                   (void *) TREE_STRING_POINTER (exp),                                                            
>                                   MEM_ALIGN (target),                                                                            
> !                                 exp_len > str_copy_len ? 1 : 0);                                                               
>         if (exp_len > str_copy_len)                                                                                              
>         clear_storage (dest_mem, GEN_INT (exp_len - str_copy_len),                                                               
>                        BLOCK_OP_NORMAL);                                                                                         
> --- 4520,4527 ----                                                                                                               
>                                   str_copy_len, builtin_strncpy_read_str,                                                        
>                                   (void *) TREE_STRING_POINTER (exp),                                                            
>                                   MEM_ALIGN (target),                                                                            
> !                                 exp_len > str_copy_len ? 1 : 0,                                                                
> !                                 false);                                                                                        
>         if (exp_len > str_copy_len)                                                                                              
>         clear_storage (dest_mem, GEN_INT (exp_len - str_copy_len),                                                               
>                        BLOCK_OP_NORMAL);                                                                                         
> 
> This is wrong.  You added the memsetp argument to store_by_pieces
> as the 6th, before the endp argument, but you are passing
> exp_len > str_copy_len ? 1 : 0 as memsetp and false as endp.
> This will certainly screw up say
> struct A { char a[20]; };
> void foo (void)
> {
>   struct A a = { "abc" };
>   bar (&a);
> }
> because in that case exp_len (20) is bigger than str_copy_len
> and so clear_storage is needed for the rest of the array.
> If we pass false == 0 as endp, it means the returned endp
> will point to the beginning of the array (&a.a[0]), rather
> than where store_by_pieces stopped.

Surprised this wasn't caught up by make check, I have committed following as
obvious.  This new testcase at least on x86_64-linux and i686-linux
fails after your patch and no longer does after swapping the arguments.

	Jakub

[-- Attachment #2: O --]
[-- Type: text/plain, Size: 1815 bytes --]

2007-08-25  Jakub Jelinek  <jakub@redhat.com>

	* expr.c (store_expr): Fix order of store_by_pieces arguments.

	* gcc.dg/array-init-2.c: New test.

--- gcc/expr.c.jj	2007-08-25 09:06:46.000000000 +0200
+++ gcc/expr.c	2007-08-25 09:24:07.000000000 +0200
@@ -4519,9 +4519,8 @@ store_expr (tree exp, rtx target, int ca
       dest_mem = store_by_pieces (dest_mem,
 				  str_copy_len, builtin_strncpy_read_str,
 				  (void *) TREE_STRING_POINTER (exp),
-				  MEM_ALIGN (target),
-				  exp_len > str_copy_len ? 1 : 0,
-				  false);
+				  MEM_ALIGN (target), false,
+				  exp_len > str_copy_len ? 1 : 0);
       if (exp_len > str_copy_len)
 	clear_storage (dest_mem, GEN_INT (exp_len - str_copy_len),
 		       BLOCK_OP_NORMAL);
--- gcc/testsuite/gcc.dg/array-init-2.c.jj	2007-08-25 09:23:19.000000000 +0200
+++ gcc/testsuite/gcc.dg/array-init-2.c	2007-08-25 09:17:39.000000000 +0200
@@ -0,0 +1,51 @@
+/* Test array initializion by store_by_pieces.  */
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+struct A { char c[10]; };
+extern void abort (void);
+
+void
+__attribute__((noinline))
+check (struct A * a, int b)
+{
+  const char *p;
+  switch (b)
+    {
+    case 0:
+      p = "abcdefghi";
+      break;
+    case 1:
+      p = "j\0\0\0\0\0\0\0\0";
+      break;
+    case 2:
+      p = "kl\0\0\0\0\0\0\0";
+      break;
+    case 3:
+      p = "mnop\0\0\0\0\0";
+      break;
+    case 4:
+      p = "qrstuvwx\0";
+      break;
+    default:
+      abort ();
+    }
+  if (__builtin_memcmp (a->c, p, 10) != 0)
+    abort ();
+}
+
+int
+main (void)
+{
+  struct A a = { "abcdefghi" };
+  check (&a, 0);
+  struct A b = { "j" };
+  check (&b, 1);
+  struct A c = { "kl" };
+  check (&c, 2);
+  struct A d = { "mnop" };
+  check (&d, 3);
+  struct A e = { "qrstuvwx" };
+  check (&e, 4);
+  return 0;
+}