[PATCH] register variable miscompilation

[Paul Brook <paul at codesourcery dot com>]

The attached patch fixes miscompilation which occurs when register variables 
are used in asm statements.

The problematic code uses a local register variable to assign a value to the 
register usually used as the frame pointer. It then passes this as an input to 
an asm statement (this is the only time GCC is guaranteed to honor the 
register specifier).

When GCC requires a frame pointer for this function then this is obviously 
user error.  However if the frame pointer is eliminated then IMO the code 
should be valid.  I believe glibc contains code like this.

The frame pointer is special because the dataflow analsys ignores it until 
after reload.  This effectively means that any uses are invisible to the 
register allocator.  There is already code in ira.c to scan though inline asms 
to detect explicit frame pointer uses. If a real frame pointer is required 
then this generates an error, otherwise it prevents the register allocator 
using that register.

However this only catches modifications that actually happen in the asm 
statement. In this case the modification occurs earlier, and the asm just uses 
the modified value. At this point it's impossible to distinguish between a use 
that comes from a register variable and a compiler generated use (e.g. the 
address of a local variable that happens to coincide with the frame pointer). 
Scanning all instructions fails because some of the EH/setjmp related 
constructs access/modify the frame pointer.

I've fixed this by recording uses of hard registers during RTL expansion, and 
combining that information into the existing asm clobbers. I suspect the ira.c 
asm scanning code may actually be redundant, but I've yet to convince myself 
that asm clobbers will work correctly without it.

This failure was observed on arm-linux in Thumb mode, where the frame pointer 
is used to hold a syscall arguments.  However I believe the bug is present on 
all targets.

Tested on arm-linux, arm-eabi, x86_64-linux and minimal testing on i386-linux
Most of the cases mentioned above are already covered by the testsuite.

Ok?

2010-05-19  Paul Brook  <paul@codesourcery.com>

	gcc/
	* gengtype-lex.l: Add HARD_REG_SET.
	* expr.c (expand_expr_real_1): Record writes to hard registers.
	* function.c (rtl_data): Add asm_clobbers.
	* ira.c (compute_regs_asm_clobbered): Use crtl->asm_clobbers.
	(ira_setup_eliminable_regset): Remove regs_asm_clobbered.
	Use crtl->asm_clobbers.

	gcc/testsuite/
	* gcc.target/arm/frame-pointer-1.c: New test.
	* gcc.target/i386/pr9771-1.c: Move code out of main to allow frame
	pointer elimination.

Index: gcc/testsuite/gcc.target/arm/frame-pointer-1.c
===================================================================
--- gcc/testsuite/gcc.target/arm/frame-pointer-1.c	(revision 0)
+++ gcc/testsuite/gcc.target/arm/frame-pointer-1.c	(revision 0)
@@ -0,0 +1,42 @@
+/* Check local register variables using a register conventionally 
+   used as the frame pointer aren't clobbered under high register pressure.  */
+/* { dg-do run } */
+/* { dg-options "-Os -mthumb -fomit-frame-pointer" } */
+
+#include <stdlib.h>
+
+int global=5;
+
+void __attribute__((noinline)) foo(int p1, int p2, int p3, int p4)
+{
+  if (global != 5 || p1 != 1 || p2 != 2 || p3 != 3 || p4 != 4)
+    abort();
+}
+
+int __attribute__((noinline)) test(int a, int b, int c, int d)
+{
+  register unsigned long r __asm__("r7") = 0xdeadbeef;
+  int e;
+
+  /* ABCD are live after the call which should be enough
+     to cause r7 to be used if it weren't for the register variable.  */
+  foo(a,b,c,d);
+
+  e = 0;
+  __asm__ __volatile__ ("mov %0, %2"
+			: "=r" (e)
+			: "0" (e), "r" (r));
+
+  global = a+b+c+d;
+
+  return e;
+}
+
+int main()
+{
+  if (test(1, 2, 3, 4) != 0xdeadbeef)
+    abort();
+  if (global != 10)
+    abort();
+  return 0;
+}
Index: gcc/testsuite/gcc.target/i386/pr9771-1.c
===================================================================
--- gcc/testsuite/gcc.target/i386/pr9771-1.c	(revision 159479)
+++ gcc/testsuite/gcc.target/i386/pr9771-1.c	(working copy)
@@ -28,7 +28,10 @@ void foo()
   *adr = save;
 }
 
-int main()
+/* This must not be inlined becuase main() requires the frame pointer
+   for stack alignment.  */
+void test(void) __attribute__((noinline));
+void test(void)
 {
   B = &x;
 
@@ -42,3 +45,9 @@ int main()
   exit(0);
 }
 
+int main()
+{
+  test();
+  return 0;
+
+}
Index: gcc/gengtype-lex.l
===================================================================
--- gcc/gengtype-lex.l	(revision 159479)
+++ gcc/gengtype-lex.l	(working copy)
@@ -49,7 +49,7 @@ update_lineno (const char *l, size_t len
 ID	[[:alpha:]_][[:alnum:]_]*
 WS	[[:space:]]+
 HWS	[ \t\r\v\f]*
-IWORD	short|long|(un)?signed|char|int|HOST_WIDE_INT|HOST_WIDEST_INT|bool|size_t|BOOL_BITFIELD|CPPCHAR_SIGNED_T|ino_t|dev_t
+IWORD	short|long|(un)?signed|char|int|HOST_WIDE_INT|HOST_WIDEST_INT|bool|size_t|BOOL_BITFIELD|CPPCHAR_SIGNED_T|ino_t|dev_t|HARD_REG_SET
 ITYPE	{IWORD}({WS}{IWORD})*
 EOID	[^[:alnum:]_]
 
Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(revision 159479)
+++ gcc/expr.c	(working copy)
@@ -8424,6 +8424,19 @@ expand_expr_real_1 (tree exp, rtx target
     expand_decl_rtl:
       gcc_assert (decl_rtl);
       decl_rtl = copy_rtx (decl_rtl);
+      /* Record writes to register variables.  */
+      if (modifier == EXPAND_WRITE && REG_P (decl_rtl)
+	  && REGNO (decl_rtl) < FIRST_PSEUDO_REGISTER)
+	{
+	    int i = REGNO (decl_rtl);
+	    int nregs = hard_regno_nregs[i][GET_MODE (decl_rtl)];
+	    while (nregs)
+	      {
+		SET_HARD_REG_BIT (crtl->asm_clobbers, i);
+		i++;
+		nregs--;
+	      }
+	}
 
       /* Ensure variable marked as used even if it doesn't go through
 	 a parser.  If it hasn't be used yet, write out an external
Index: gcc/function.h
===================================================================
--- gcc/function.h	(revision 159479)
+++ gcc/function.h	(working copy)
@@ -25,6 +25,7 @@ along with GCC; see the file COPYING3.
 #include "tree.h"
 #include "hashtab.h"
 #include "vecprim.h"
+#include "hard-reg-set.h"
 
 /* Stack of pending (incomplete) sequences saved by `start_sequence'.
    Each element describes one pending sequence.
@@ -422,6 +423,12 @@ struct GTY(()) rtl_data {
      TREE_NOTHROW (current_function_decl) it is set even for overwritable
      function where currently compiled version of it is nothrow.  */
   bool nothrow;
+
+  /* Like regs_ever_live, but 1 if a reg is set or clobbered from an
+     asm.  Unlike regs_ever_live, elements of this array corresponding
+     to eliminable regs (like the frame pointer) are set if an asm
+     sets them.  */
+  HARD_REG_SET asm_clobbers;
 };
 
 #define return_label (crtl->x_return_label)
Index: gcc/ira.c
===================================================================
--- gcc/ira.c	(revision 159479)
+++ gcc/ira.c	(working copy)
@@ -1385,14 +1385,12 @@ insn_contains_asm (rtx insn)
   return for_each_rtx (&insn, insn_contains_asm_1, NULL);
 }
 
-/* Set up regs_asm_clobbered.  */
+/* Add register clobbers from asm statements.  */
 static void
-compute_regs_asm_clobbered (char *regs_asm_clobbered)
+compute_regs_asm_clobbered (void)
 {
   basic_block bb;
 
-  memset (regs_asm_clobbered, 0, sizeof (char) * FIRST_PSEUDO_REGISTER);
-
   FOR_EACH_BB (bb)
     {
       rtx insn;
@@ -1413,7 +1411,7 @@ compute_regs_asm_clobbered (char *regs_a
 		      + hard_regno_nregs[dregno][mode] - 1;
 
 		    for (i = dregno; i <= end; ++i)
-		      regs_asm_clobbered[i] = 1;
+		      SET_HARD_REG_BIT(crtl->asm_clobbers, i);
 		  }
 	      }
 	}
@@ -1425,12 +1423,6 @@ compute_regs_asm_clobbered (char *regs_a
 void
 ira_setup_eliminable_regset (void)
 {
-  /* Like regs_ever_live, but 1 if a reg is set or clobbered from an
-     asm.  Unlike regs_ever_live, elements of this array corresponding
-     to eliminable regs (like the frame pointer) are set if an asm
-     sets them.  */
-  char *regs_asm_clobbered
-    = (char *) alloca (FIRST_PSEUDO_REGISTER * sizeof (char));
 #ifdef ELIMINABLE_REGS
   int i;
   static const struct {const int from, to; } eliminables[] = ELIMINABLE_REGS;
@@ -1454,7 +1446,8 @@ ira_setup_eliminable_regset (void)
   COPY_HARD_REG_SET (ira_no_alloc_regs, no_unit_alloc_regs);
   CLEAR_HARD_REG_SET (eliminable_regset);
 
-  compute_regs_asm_clobbered (regs_asm_clobbered);
+  compute_regs_asm_clobbered ();
+
   /* Build the regset of all eliminable registers and show we can't
      use those that we already know won't be eliminated.  */
 #ifdef ELIMINABLE_REGS
@@ -1464,7 +1457,7 @@ ira_setup_eliminable_regset (void)
 	= (! targetm.can_eliminate (eliminables[i].from, eliminables[i].to)
 	   || (eliminables[i].to == STACK_POINTER_REGNUM && need_fp));
 
-      if (! regs_asm_clobbered[eliminables[i].from])
+      if (!TEST_HARD_REG_BIT (crtl->asm_clobbers, eliminables[i].from))
 	{
 	    SET_HARD_REG_BIT (eliminable_regset, eliminables[i].from);
 
@@ -1478,7 +1471,7 @@ ira_setup_eliminable_regset (void)
 	df_set_regs_ever_live (eliminables[i].from, true);
     }
 #if FRAME_POINTER_REGNUM != HARD_FRAME_POINTER_REGNUM
-  if (! regs_asm_clobbered[HARD_FRAME_POINTER_REGNUM])
+  if (!TEST_HARD_REG_BIT (crtl->asm_clobbers, HARD_FRAME_POINTER_REGNUM))
     {
       SET_HARD_REG_BIT (eliminable_regset, HARD_FRAME_POINTER_REGNUM);
       if (need_fp)
@@ -1492,7 +1485,7 @@ ira_setup_eliminable_regset (void)
 #endif
 
 #else
-  if (! regs_asm_clobbered[FRAME_POINTER_REGNUM])
+  if (!TEST_HARD_REG_BIT (crtl->asm_clobbers, HARD_FRAME_POINTER_REGNUM))
     {
       SET_HARD_REG_BIT (eliminable_regset, FRAME_POINTER_REGNUM);
       if (need_fp)