[PATCH][fr30] Fix PR 34174 movdi mem->reg wrong-code bug

Rask Ingemann Lambertsen rask@sygehus.dk
Fri Nov 23 22:16:00 GMT 2007


   The bug is in fr30_move_double()'s mem->reg case. This code is amazing.
It detects when it is necessary to avoid clobbering the register holding the
memory address, then starts out by clobbering it anyway. And the
reverse/!reverse trickery is applied twice and therefore cancels itself out.
It has been broken this way ever since it was added seven and a half years
ago. The broken instruction sequence looks like this

 ld   @rN, r1
 mov  rN,  r2
 addn 4,   r2
 ld   @r2, r2

where rN holds the address. If N == 1, it breaks. This patch changes it to

 mov  rN,  r2
 ld   @rN, r1
 addn 4,   r2
 ld   @r2, r2

which fixes the bug. I've tested it by building a cross compiler for
fr30-unknown-elf and running the testsuite using the simulator from GDB 5.2.
There were no new failures. Ok for trunk?

   Note that I expect the testcase to fail on the AVR because of PR
target/27386 where this testcase originates. Should I XFAIL it on the AVR?

   Mark, would it be OK to fix this for 4.2.3 also? While clearly not a
regression, it is a wrong-code bug which is easy to trigger and causes
374 testsuite failures, all execution failures:

                === gcc Summary ===

-# of expected passes           43562
-# of unexpected failures       509
+# of expected passes           43901
+# of unexpected failures       182
 # of unexpected successes      3
 # of expected failures         84
 # of unresolved testcases      71

                === g++ Summary ===

-# of expected passes           16296
-# of unexpected failures       118
+# of expected passes           16343
+# of unexpected failures       71
 # of unexpected successes      2
 # of expected failures         81
 # of unresolved testcases      51

:ADDPATCH fr30:

23-11-2007  Rask Ingemann Lambertsen  <rask@sygehus.dk>

	PR target/34174
	* config/fr30/fr30.c (fr30_move_double): Sanitize mem->reg case. Copy
	the address before it is clobbered.

testsuite/
	* gcc.dg/torture/pr34174-1.c: New.


Index: gcc/config/fr30/fr30.c
===================================================================
--- gcc/config/fr30/fr30.c	(revision 130319)
+++ gcc/config/fr30/fr30.c	(working copy)
@@ -828,47 +828,23 @@ fr30_move_double (rtx * operands)
 	{
 	  rtx addr = XEXP (src, 0);
 	  int dregno = REGNO (dest);
-	  rtx dest0;
-	  rtx dest1;
+	  rtx dest0 = operand_subword (dest, 0, TRUE, mode);;
+	  rtx dest1 = operand_subword (dest, 1, TRUE, mode);;
 	  rtx new_mem;
 	  
-	  /* If the high-address word is used in the address, we
-	     must load it last.  Otherwise, load it first.  */
-	  int reverse = (refers_to_regno_p (dregno, dregno + 1, addr, 0) != 0);
-
 	  gcc_assert (GET_CODE (addr) == REG);
 	  
-	  dest0 = operand_subword (dest, reverse, TRUE, mode);
-	  dest1 = operand_subword (dest, !reverse, TRUE, mode);
-
-	  if (reverse)
-	    {
-	      emit_insn (gen_rtx_SET (VOIDmode, dest1,
-				      adjust_address (src, SImode, 0)));
-	      emit_insn (gen_rtx_SET (SImode, dest0,
-				      gen_rtx_REG (SImode, REGNO (addr))));
-	      emit_insn (gen_rtx_SET (SImode, dest0,
-				      plus_constant (dest0, UNITS_PER_WORD)));
-
-	      new_mem = gen_rtx_MEM (SImode, dest0);
-	      MEM_COPY_ATTRIBUTES (new_mem, src);
-	      
-	      emit_insn (gen_rtx_SET (VOIDmode, dest0, new_mem));
-	    }
-	  else
-	    {
-	      emit_insn (gen_rtx_SET (VOIDmode, dest0,
-				      adjust_address (src, SImode, 0)));
-	      emit_insn (gen_rtx_SET (SImode, dest1,
-				      gen_rtx_REG (SImode, REGNO (addr))));
-	      emit_insn (gen_rtx_SET (SImode, dest1,
-				      plus_constant (dest1, UNITS_PER_WORD)));
+	  /* Copy the address before clobbering it.  See PR 34174.  */
+	  emit_insn (gen_rtx_SET (SImode, dest1, addr));
+	  emit_insn (gen_rtx_SET (VOIDmode, dest0,
+				  adjust_address (src, SImode, 0)));
+	  emit_insn (gen_rtx_SET (SImode, dest1,
+				  plus_constant (dest1, UNITS_PER_WORD)));
 
-	      new_mem = gen_rtx_MEM (SImode, dest1);
-	      MEM_COPY_ATTRIBUTES (new_mem, src);
+	  new_mem = gen_rtx_MEM (SImode, dest1);
+	  MEM_COPY_ATTRIBUTES (new_mem, src);
 	      
-	      emit_insn (gen_rtx_SET (VOIDmode, dest1, new_mem));
-	    }
+	  emit_insn (gen_rtx_SET (VOIDmode, dest1, new_mem));
 	}
       else if (src_code == CONST_INT || src_code == CONST_DOUBLE)
 	{
Index: gcc/testsuite/gcc.dg/torture/pr34174-1.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr34174-1.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr34174-1.c	(revision 0)
@@ -0,0 +1,44 @@
+/* { dg-do run } */
+/* Based on PR target/27386 testcase by Joerg Wunsch.  */
+
+extern void abort (void);
+extern void exit (int);
+
+#if __INT_MAX__ >= 9223372036854775807LL
+typedef unsigned int uint64_t;
+#elif __LONG_MAX__ >= 9223372036854775807LL
+typedef unsigned long int uint64_t;
+#elif __LONG_LONG_MAX__ >= 9223372036854775807LL
+typedef unsigned long long int uint64_t;
+#else
+int
+main (void)
+{
+  exit (0);
+}
+#endif
+
+uint64_t a, b, c;
+
+int
+foo (uint64_t x, uint64_t y, uint64_t z, int i)
+{
+  a = x;
+  b = y;
+  c = z;
+  return 2 * i;
+}
+
+int
+main (void)
+{
+  if (foo (1234512345123ull, 3456734567345ull, 7897897897897ull, 42) != 84)
+    abort ();
+  if (a != 1234512345123ull)
+    abort ();
+  if (b != 3456734567345ull)
+    abort ();
+  if (c != 7897897897897ull)
+    abort ();
+  exit (0);
+}

-- 
Rask Ingemann Lambertsen
Danish law requires addresses in e-mail to be logged and stored for a year



More information about the Gcc-patches mailing list