diff mbox series

Fix up wrong-code caused by store_expr (PR middle-end/90025)

Message ID 20190410072010.GR21066@tucnak
State New
Headers show
Series Fix up wrong-code caused by store_expr (PR middle-end/90025) | expand

Commit Message

Jakub Jelinek April 10, 2019, 7:20 a.m. UTC
Hi!

In r268957 aka PR66152 fix I've slightly changed the store_expr
code which does store_by_pieces for just a part of an array and
clear_storage for the rest and since then the following testcase
is miscompiled on s390x-linux.  I believe one could construct other testcase
that would be miscompiled before as well though.

The problem is that store_by_pieces with RETURN_END returns a MEM object
with QImode and thus also MEM_SIZE of 1, we then adjust_address it to
BLKmode which doesn't change MEM_SIZE and nothing at least in the generic
clear_storage code tweaks that MEM_SIZE later on (and while perhaps some
targets do, e.g. s390 does not).  Because in this case the size is
really known and constant, the following patch makes sure we do
what adjust_address does, but in addition make sure to set MEM_SIZE
properly.  When MEM_SIZE is 1 and in the testcase there is a store after
it to the first few bytes of the remainder, RTL DSE happily removes that
memory clear insn emitted for clear_storage, because if it was really size
1, it would be indeed redundant, but there are 20 bytes that are not
redundant after it.

Bootstrapped/regtested on {x86_64,i686,s390x,powerpc64le}-linux, ok for
trunk?

2019-04-10  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/90025
	* expr.c (store_expr): Set properly size on the MEM passed to
	clear_storage.

	* gcc.c-torture/execute/pr90025.c: New test.


	Jakub

Comments

Richard Biener April 10, 2019, 7:24 a.m. UTC | #1
On Wed, 10 Apr 2019, Jakub Jelinek wrote:

> Hi!
> 
> In r268957 aka PR66152 fix I've slightly changed the store_expr
> code which does store_by_pieces for just a part of an array and
> clear_storage for the rest and since then the following testcase
> is miscompiled on s390x-linux.  I believe one could construct other testcase
> that would be miscompiled before as well though.
> 
> The problem is that store_by_pieces with RETURN_END returns a MEM object
> with QImode and thus also MEM_SIZE of 1, we then adjust_address it to
> BLKmode which doesn't change MEM_SIZE and nothing at least in the generic
> clear_storage code tweaks that MEM_SIZE later on (and while perhaps some
> targets do, e.g. s390 does not).  Because in this case the size is
> really known and constant, the following patch makes sure we do
> what adjust_address does, but in addition make sure to set MEM_SIZE
> properly.  When MEM_SIZE is 1 and in the testcase there is a store after
> it to the first few bytes of the remainder, RTL DSE happily removes that
> memory clear insn emitted for clear_storage, because if it was really size
> 1, it would be indeed redundant, but there are 20 bytes that are not
> redundant after it.
> 
> Bootstrapped/regtested on {x86_64,i686,s390x,powerpc64le}-linux, ok for
> trunk?

OK.

Richard.

> 2019-04-10  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/90025
> 	* expr.c (store_expr): Set properly size on the MEM passed to
> 	clear_storage.
> 
> 	* gcc.c-torture/execute/pr90025.c: New test.
> 
> --- gcc/expr.c.jj	2019-02-21 00:01:59.418057433 +0100
> +++ gcc/expr.c	2019-04-09 12:49:20.704457867 +0200
> @@ -5658,7 +5658,8 @@ store_expr (tree exp, rtx target, int ca
>        dest_mem = store_by_pieces (target, str_copy_len, string_cst_read_str,
>  				  (void *) str, MEM_ALIGN (target), false,
>  				  RETURN_END);
> -      clear_storage (adjust_address (dest_mem, BLKmode, 0),
> +      clear_storage (adjust_address_1 (dest_mem, BLKmode, 0, 1, 1, 0,
> +				       exp_len - str_copy_len),
>  		     GEN_INT (exp_len - str_copy_len), BLOCK_OP_NORMAL);
>        return NULL_RTX;
>      }
> --- gcc/testsuite/gcc.c-torture/execute/pr90025.c.jj	2019-04-09 12:59:35.876449686 +0200
> +++ gcc/testsuite/gcc.c-torture/execute/pr90025.c	2019-04-09 12:59:20.156705430 +0200
> @@ -0,0 +1,28 @@
> +/* PR middle-end/90025 */
> +
> +__attribute__((noipa)) void
> +bar (char *p)
> +{
> +  int i;
> +  for (i = 0; i < 6; i++)
> +    if (p[i] != "foobar"[i])
> +      __builtin_abort ();
> +  for (; i < 32; i++)
> +    if (p[i] != '\0')
> +      __builtin_abort ();
> +}
> +
> +__attribute__((noipa)) void
> +foo (unsigned int x)
> +{
> +  char s[32] = { 'f', 'o', 'o', 'b', 'a', 'r', 0 };
> +  ((unsigned int *) s)[2] = __builtin_bswap32 (x);
> +  bar (s);
> +}
> +
> +int
> +main ()
> +{
> +  foo (0);
> +  return 0;
> +}
> 
> 	Jakub
>
diff mbox series

Patch

--- gcc/expr.c.jj	2019-02-21 00:01:59.418057433 +0100
+++ gcc/expr.c	2019-04-09 12:49:20.704457867 +0200
@@ -5658,7 +5658,8 @@  store_expr (tree exp, rtx target, int ca
       dest_mem = store_by_pieces (target, str_copy_len, string_cst_read_str,
 				  (void *) str, MEM_ALIGN (target), false,
 				  RETURN_END);
-      clear_storage (adjust_address (dest_mem, BLKmode, 0),
+      clear_storage (adjust_address_1 (dest_mem, BLKmode, 0, 1, 1, 0,
+				       exp_len - str_copy_len),
 		     GEN_INT (exp_len - str_copy_len), BLOCK_OP_NORMAL);
       return NULL_RTX;
     }
--- gcc/testsuite/gcc.c-torture/execute/pr90025.c.jj	2019-04-09 12:59:35.876449686 +0200
+++ gcc/testsuite/gcc.c-torture/execute/pr90025.c	2019-04-09 12:59:20.156705430 +0200
@@ -0,0 +1,28 @@ 
+/* PR middle-end/90025 */
+
+__attribute__((noipa)) void
+bar (char *p)
+{
+  int i;
+  for (i = 0; i < 6; i++)
+    if (p[i] != "foobar"[i])
+      __builtin_abort ();
+  for (; i < 32; i++)
+    if (p[i] != '\0')
+      __builtin_abort ();
+}
+
+__attribute__((noipa)) void
+foo (unsigned int x)
+{
+  char s[32] = { 'f', 'o', 'o', 'b', 'a', 'r', 0 };
+  ((unsigned int *) s)[2] = __builtin_bswap32 (x);
+  bar (s);
+}
+
+int
+main ()
+{
+  foo (0);
+  return 0;
+}