Re: Fixing the ARM interrupt attribute for 3.1-cvs

[Tobias Ringstrom <tori at ringstrom dot mine dot nu>]

On Mon, 25 Mar 2002, Tobias Ringstrom wrote:
> 
> A couple of days ago I filed two bug reports (6039 & 6040) regarding the
> broken code generation of interrupt functions for the ARM port for both
> version 3.0.4 and 3.1.  The two versions were broken in different ways.  
> I've never worked with the gcc code before, but I was curious, and there
> must be a first time for everything.  I now have what I believe to be a
> solution to the problems in the 3.1 version, but I need reviewing of my
> patch.  The patch fixes (at least) two problems:
> 
> 1. The IP register was restored with the old SP minus 4.
> 
> 2. The SP was never restored.  The ARM has a separate interrupt stack, but 
>    this limits the number of interrupts you can server before you crash 
>    quite substantially.  ;-)
> 
> I also fixed the case where a frame pointer was not needed but the IP was 
> clobbered which is probably very uncommon, but...
> 
> Again, this is for review only, and not (yet) for inclusion.  I will do
> real testing later this week.

It took longer than I expected, but I have now actually run the code, and   
as far as I can tell, the patch works.

To reiterate:  The code generated when using the attribute((interrupt))  
for ARM is 100% BROKEN for gcc 3.0.x and 3.1-cvs.  I'm pretty sure that
this patch fixes it for 3.1-cvs, but it would be nice to have a second
opinion.

/Tobias


Index: arm.c
===================================================================
RCS file: /cvsroot/gcc/gcc/gcc/config/arm/arm.c,v
retrieving revision 1.196.2.4
diff -u -r1.196.2.4 arm.c
--- arm.c	20 Mar 2002 15:06:03 -0000	1.196.2.4
+++ arm.c	12 Apr 2002 23:02:25 -0000
@@ -7181,6 +7181,7 @@
   int reg;
   unsigned long live_regs_mask;
   unsigned long func_type;
+  int isr_restore_ip = 0;
   
   func_type = arm_current_func_type ();
 
@@ -7229,16 +7230,17 @@
       else
 	return_reg = reg_names[LR_REGNUM];
 
-      if ((live_regs_mask & (1 << IP_REGNUM)) == (1 << IP_REGNUM))
-	/* There are two possible reasons for the IP register being saved.
-	   Either a stack frame was created, in which case IP contains the
-	   old stack pointer, or an ISR routine corrupted it.  If this in an
-	   ISR routine then just restore IP, otherwise restore IP into SP.  */
-	if (! IS_INTERRUPT (func_type))
-	  {
-	    live_regs_mask &= ~ (1 << IP_REGNUM);
-	    live_regs_mask |=   (1 << SP_REGNUM);
-	  }
+      if (frame_pointer_needed &&
+	  ((live_regs_mask & (1 << IP_REGNUM)) != 0))
+      {
+	/* If this is an ISR, IP must be restored, but since it was
+	   stored before the rest of the registers we must restore it
+	   separately. */
+	if (IS_INTERRUPT (func_type))
+	  isr_restore_ip = 1;
+	live_regs_mask &= ~ (1 << IP_REGNUM);
+	live_regs_mask |=   (1 << SP_REGNUM);
+      }
 
       /* On some ARM architectures it is faster to use LDR rather than
 	 LDM to load a single register.  On other architectures, the
@@ -7263,6 +7265,12 @@
 	  char *p;
 	  int first = 1;
 
+	  if (isr_restore_ip)
+	    {
+	      sprintf (instr, "ldr%s\t%%|ip, [%%|fp, #4]", conditional);
+	      output_asm_insn (instr, & operand);
+	    }
+
 	  /* Generate the load multiple instruction to restore the registers.  */
 	  if (frame_pointer_needed)
 	    sprintf (instr, "ldm%sea\t%%|fp, {", conditional);
@@ -8161,6 +8169,7 @@
 	     Creating a frame pointer however, corrupts the IP
 	     register, so we must push it first.  */
 	  insn = emit_multi_reg_push (1 << IP_REGNUM);
+	  fp_offset = 4;
 
 	  /* Do not set RTX_FRAME_RELATED_P on this insn.
 	     The dwarf stack unwinding code only wants to see one