Re: [PATCH]: ARM ISR stack frame fix

[Nick Clifton <nickc at cambridge dot redhat dot com>]

Hi Alan,

> This is a patch to fix some really annoying stack bugs that
> prevented me from writing using the
> __attribute__((interrupt("IRQ"))) extension of arm-gcc in any useful
> way.

Thanks very much for taking the time to track down and fix these
problems.

> Not included in this patch is a hack I used to make dwarf output
> work correctly for ISRs. I commented out the RTX_FRAME_RELATED_P
> line in 
> 
>       if (IS_INTERRUPT (func_type))
> 	{
> 	  /* Interrupt functions must not corrupt any registers.
> 	     Creating a frame pointer however, corrupts the IP
> 	     register, so we must push it first.  */
> 	  insn = emit_multi_reg_push (1 << IP_REGNUM);
> 	  RTX_FRAME_RELATED_P (insn) = 1;
> 	}
> 
> to make files compile with '-g'. Unfortunately, I don't understand the
> dwarf output sufficiently well to know what the right solution is.

It is.  The dwarf2 unwind code is sensitive to uses of the stack
pointer.  By labeling this instruction as being part of the frame
creation code it establishes the SP register as the frame pointer
register (for the purposes of dwarf2 frame unwinding) and so the
assignment of IP to FP later on the frame creation sequence creates an
abort.  (The stack unwinding code is expecting an assignment of SP to
FP...)


> Problem
> 
> 	In the prologue of an ISR with a frame pointer, ip is saved
> 	with 
> 		str     ip, [sp, #-4]!
> 	the equivalent of a stmfd. However, ip is restored with
>  		ldmea   sp, {ip}
> 	which results in both the incorrect value being restored in
> 	ip, as well as an improper stack adjustment (the stack leaks 1
> 	word per invocation).

I agree that the wrong restore instruction is being used, but where
does the stack leak come from ?  The LDMEA instruction will increment
the stack pointer.  It is just that it will do it before loading IP
instead of afterwards.

> 	arm_compute_initial_elimination_offset did not account for the
> 	possibility of saving r0-r3 or ip on the stack during interrupt
> 	handlers. This caused reloads to be allocated to the same stack
> 	slots as saved registers.

This is actually a second problem, but since it is closely related to
the first it is OK to include it in the same report.

> Testcases

Thanks for including this.  I have taken the time to convert this
testcase into a version that can be part of the GCC testsuite, so that
in the future this bug should not reoccur.

> 2001-12-20 Alan Shieh <ashieh@hkn.eecs.berkeley.edu>
> 
> 	* config/arm/arm.c (arm_output_epilogue): Changed IP restore
> 	  to use ldmfd instead of ldmea.
> 	* config/arm/arm.c (arm_compute_initial_elimination_offset):
> 	  Modified to reflect behavior of arm_expand_prologue when generating
> 	  interrupt handlers

I have taken your code and amended it in a couple of ways.  I will
check this amended version in.  The changes I made were:

  * Move the code you added to arm_compute_initial_elimination_offset
    into a separate function and arrange for it to be called both from
    here and from arm_compute_save_reg_mask.  This should eliminate
    future discrepancies like this.

  * Fix a spelling typo in a comment.

  * Removing the setting of RTX_FRAME_RELATED_P on the push of the IP
    register, as you suggested.

  * Creating a new, ARM specific test for this bug.

Cheers
        Nick

gcc/ChangeLog
2001-12-20 Alan Shieh <ashieh@hkn.eecs.berkeley.edu>

	* config/arm/arm.c (arm_output_epilogue): Changed IP restore
	to use ldmfd instead of ldmea.
	* config/arm/arm.c (arm_compute_initial_elimination_offset):
	Modified to reflect behavior of arm_expand_prologue when generating
	interrupt handlers

2001-12-20  Nick Clifton  <nickc@cambridge.redhat.com>

	* config/arm/arm.c (arm_compute_save_reg0_reg12_mask):  New
        function. Compute which of registers r0 through r12 should be
	saved onto the stack during a function's prologue.
        (arm_compute_save_reg_mask): Use
        arm_compute_save_reg0_reg12_mask. 
        (arm_compute_initial_elimination_offset): Use
        arm_compute_save_reg0_reg12_mask.

        (arm_expand_prologue): Do not mark as save of the IP register
        for an interrupt handler as being part of the frame creation
        code.

gcc/testsuite/ChangeLog
2001-12-20  Nick Clifton  <nickc@cambridge.redhat.com>

	* gcc.misc-test/arm-isr.exp: New file.  Control running of the
        arm-isr.c test.
        * gcc.misc-tests/arm-isr.c: New file.  Test prologue and
        epilogue generation for ARM ISR routines.

Index: config/arm/arm.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/arm/arm.c,v
retrieving revision 1.180
diff -p -c -r1.180 arm.c
*** arm.c	2001/12/17 15:05:27	1.180
--- arm.c	2001/12/20 18:41:23
*************** static void	 push_minipool_barrier	     
*** 104,109 ****
--- 104,110 ----
  static void	 push_minipool_fix		PARAMS ((rtx, Hint, rtx *, Mmode, rtx));
  static void	 note_invalid_constants	        PARAMS ((rtx, Hint));
  static int       current_file_function_operand	PARAMS ((rtx));
+ static Ulong arm_compute_save_reg0_reg12_mask   PARAMS ((void));
  static Ulong     arm_compute_save_reg_mask	PARAMS ((void));
  static Ulong     arm_isr_value 			PARAMS ((tree));
  static Ulong     arm_compute_func_type		PARAMS ((void));
*************** output_ascii_pseudo_op (stream, p, len)
*** 7001,7038 ****
    fputs ("\"\n", stream);
  }
  
! /* Compute a bit mask of which registers need to be
!    saved on the stack for the current function.  */
  
  static unsigned long
! arm_compute_save_reg_mask ()
  {
    unsigned int save_reg_mask = 0;
    unsigned int reg;
-   unsigned long func_type = arm_current_func_type ();
- 
-   if (IS_NAKED (func_type))
-     /* This should never really happen.  */
-     return 0;
  
-   /* If we are creating a stack frame, then we must save the frame pointer,
-      IP (which will hold the old stack pointer), LR and the PC.  */
-   if (frame_pointer_needed)
-     save_reg_mask |=
-       (1 << ARM_HARD_FRAME_POINTER_REGNUM)
-       | (1 << IP_REGNUM)
-       | (1 << LR_REGNUM)
-       | (1 << PC_REGNUM);
- 
-   /* Volatile functions do not return, so there
-      is no need to save any other registers.  */
-   if (IS_VOLATILE (func_type))
-     return save_reg_mask;
- 
    if (IS_INTERRUPT (func_type))
      {
        unsigned int max_reg;
-       
        /* Interrupt functions must not corrupt any registers,
  	 even call clobbered ones.  If this is a leaf function
  	 we can just examine the registers used by the RTL, but
--- 7016,7035 ----
    fputs ("\"\n", stream);
  }
  
! /* Compute the register sabe mask for registers 0 through 12
!    inclusive.  This code is used by both arm_compute_save_reg_mask
!    and arm_compute_initial_elimination_offset.  */
  
  static unsigned long
! arm_compute_save_reg0_reg12_mask ()
  {
+   unsigned long func_type = arm_current_func_type ();
    unsigned int save_reg_mask = 0;
    unsigned int reg;
  
    if (IS_INTERRUPT (func_type))
      {
        unsigned int max_reg;
        /* Interrupt functions must not corrupt any registers,
  	 even call clobbered ones.  If this is a leaf function
  	 we can just examine the registers used by the RTL, but
*************** arm_compute_save_reg_mask ()
*** 7042,7048 ****
        if (ARM_FUNC_TYPE (func_type) == ARM_FT_FIQ)
  	/* FIQ handlers have registers r8 - r12 banked, so
  	   we only need to check r0 - r7, Normal ISRs only
! 	   bank r14 and r15, so ew must check up to r12.
  	   r13 is the stack pointer which is always preserved,
  	   so we do not need to consider it here.  */
  	max_reg = 7;
--- 7039,7045 ----
        if (ARM_FUNC_TYPE (func_type) == ARM_FT_FIQ)
  	/* FIQ handlers have registers r8 - r12 banked, so
  	   we only need to check r0 - r7, Normal ISRs only
! 	   bank r14 and r15, so we must check up to r12.
  	   r13 is the stack pointer which is always preserved,
  	   so we do not need to consider it here.  */
  	max_reg = 7;
*************** arm_compute_save_reg_mask ()
*** 7077,7082 ****
--- 7074,7111 ----
  	save_reg_mask |= 1 << PIC_OFFSET_TABLE_REGNUM;
      }
  
+   return save_reg_mask;
+ }
+ 
+ /* Compute a bit mask of which registers need to be
+    saved on the stack for the current function.  */
+ 
+ static unsigned long
+ arm_compute_save_reg_mask ()
+ {
+   unsigned int save_reg_mask = 0;
+   unsigned long func_type = arm_current_func_type ();
+ 
+   if (IS_NAKED (func_type))
+     /* This should never really happen.  */
+     return 0;
+ 
+   /* If we are creating a stack frame, then we must save the frame pointer,
+      IP (which will hold the old stack pointer), LR and the PC.  */
+   if (frame_pointer_needed)
+     save_reg_mask |=
+       (1 << ARM_HARD_FRAME_POINTER_REGNUM)
+       | (1 << IP_REGNUM)
+       | (1 << LR_REGNUM)
+       | (1 << PC_REGNUM);
+ 
+   /* Volatile functions do not return, so there
+      is no need to save any other registers.  */
+   if (IS_VOLATILE (func_type))
+     return save_reg_mask;
+ 
+   save_reg_mask |= arm_compute_save_reg0_reg12_mask ();
+ 
    /* Decide if we need to save the link register.
       Interrupt routines have their own banked link register,
       so they never need to save it.
*************** arm_output_epilogue (really_return)
*** 7535,7541 ****
        if (IS_INTERRUPT (func_type))
  	/* Interrupt handlers will have pushed the
  	   IP onto the stack, so restore it now.  */
! 	print_multi_reg (f, "ldmea\t%r", SP_REGNUM, 1 << IP_REGNUM);
      }
    else
      {
--- 7564,7570 ----
        if (IS_INTERRUPT (func_type))
  	/* Interrupt handlers will have pushed the
  	   IP onto the stack, so restore it now.  */
! 	print_multi_reg (f, "ldmfd\t%r", SP_REGNUM, 1 << IP_REGNUM);
      }
    else
      {
*************** arm_compute_initial_elimination_offset (
*** 7946,7972 ****
    call_saved_registers = 0;
    if (! IS_VOLATILE (func_type))
      {
        unsigned int reg;
  
!       /* In theory we should check all of the hard registers to
! 	 see if they will be saved onto the stack.  In practice
! 	 registers 11 upwards have special meanings and need to
! 	 be check individually.  */
!       for (reg = 0; reg <= 10; reg ++)
! 	if (regs_ever_live[reg] && ! call_used_regs[reg])
! 	  call_saved_registers += 4;
  
!       /* Determine if register 11 will be clobbered.  */
!       if (! TARGET_APCS_FRAME
! 	  && ! frame_pointer_needed
! 	  && regs_ever_live[HARD_FRAME_POINTER_REGNUM]
! 	  && ! call_used_regs[HARD_FRAME_POINTER_REGNUM])
! 	call_saved_registers += 4;
  
!       /* The PIC register is fixed, so if the function will
! 	 corrupt it, it has to be saved onto the stack.  */
!       if (flag_pic && regs_ever_live[PIC_OFFSET_TABLE_REGNUM])
! 	call_saved_registers += 4;
  
        if (regs_ever_live[LR_REGNUM]
  	  /* If a stack frame is going to be created, the LR will
--- 7975,7996 ----
    call_saved_registers = 0;
    if (! IS_VOLATILE (func_type))
      {
+       unsigned int reg_mask;
        unsigned int reg;
  
!       /* Makre sure that we compute which registers will be saved
! 	 on the stack using the same algorithm that is used by
! 	 arm_compute_save_reg_mask().  */
!       reg_mask = arm_compute_save_reg0_reg12_mask ();
  
!       /* Now count the number of bits set in save_reg_mask.
! 	 For each set bit we need 4 bytes of stack space.  */
  
!       while (reg_mask)
! 	{
! 	  call_saved_registers += 4;
! 	  reg_mask = reg_mask & ~ (reg_mask & - reg_mask);
! 	}
  
        if (regs_ever_live[LR_REGNUM]
  	  /* If a stack frame is going to be created, the LR will
*************** arm_expand_prologue ()
*** 8097,8103 ****
  	     Creating a frame pointer however, corrupts the IP
  	     register, so we must push it first.  */
  	  insn = emit_multi_reg_push (1 << IP_REGNUM);
! 	  RTX_FRAME_RELATED_P (insn) = 1;
  	}
        else if (IS_NESTED (func_type))
  	{
--- 8121,8138 ----
  	     Creating a frame pointer however, corrupts the IP
  	     register, so we must push it first.  */
  	  insn = emit_multi_reg_push (1 << IP_REGNUM);
! 
! 	  /* Do not set RTX_FRAME_RELATED_P on this insn.
! 	     The dwarf stack unwinding code only wants to see one
! 	     stack decrement per function, and this is not it.  If
! 	     this instruction is labeled as being part of the frame
! 	     creation sequence then dwarf2out_frame_debug_expr will
! 	     abort when it encounters the assignment of IP to FP
! 	     later on, since the use of SP here establishes SP as
! 	     the CFA register and not IP.
! 
! 	     Anyway this instruction is not really part of the stack
! 	     frame creation although it is part of the prologue.  */
  	}
        else if (IS_NESTED (func_type))
  	{

Index: testsuite/gcc.misc-tests/arm-isr.exp
===================================================================
RCS file: arm-isr.exp
diff -N arm-isr.exp
*** /dev/null	Tue May  5 13:32:27 1998
--- arm-isr.exp	Thu Dec 20 10:41:23 2001
***************
*** 0 ****
--- 1,27 ----
+ #   Copyright (C) 2001 Free Software Foundation, Inc.
+ 
+ # This program is free software; you can redistribute it and/or modify
+ # it under the terms of the GNU General Public License as published by
+ # the Free Software Foundation; either version 2 of the License, or
+ # (at your option) any later version.
+ # 
+ # This program is distributed in the hope that it will be useful,
+ # but WITHOUT ANY WARRANTY; without even the implied warranty of
+ # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ # GNU General Public License for more details.
+ # 
+ # You should have received a copy of the GNU General Public License
+ # along with this program; if not, write to the Free Software
+ # Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  
+ 
+ # This file is based on a bug report submitted to gcc-bugs:
+ 
+ # Load support procs.
+ load_lib gcc-dg.exp
+ 
+ dg-init
+ if {[istarget "*arm-*-*"] || [istarget "xscale-*-*"]} {
+   dg-runtest "$srcdir/$subdir/arm-isr.c" "" ""
+ }
+ dg-finish
+ 
Index: testsuite/gcc.misc-tests/arm-isr.c
===================================================================
RCS file: arm-isr.c
diff -N arm-isr.c
*** /dev/null	Tue May  5 13:32:27 1998
--- arm-isr.c	Thu Dec 20 10:41:23 2001
***************
*** 0 ****
--- 1,48 ----
+ #ifndef __thumb__
+ /* There used to be a couple of bugs in the ARM's prologue and epilogue
+    generation for ISR routines.  The wrong epilogue instruction would be
+    generated to restore the IP register if it had to be pushed onto the
+    stack, and the wrong offset was being computed for local variables if
+    r0 - r3 had to be saved.  This tests for both of these cases.  */
+ 
+ int z = 9;
+ 
+ int
+ bar (void)
+ {
+   return z;
+ }
+ 
+ int
+ foo (int a, int b, int c, int d, int e, int f, int g, int h)
+ {
+   volatile int i = (a + b) - (g + h) + bar ();
+   volatile int j = (e + f) - (c + d);
+ 
+   return a + b + c + d + e + f + g + h + i + j;
+ }
+ 
+ int foo1 (int a, int b, int c, int d, int e, int f, int g, int h) __attribute__ ((interrupt ("IRQ")));
+ 
+ int
+ foo1 (int a, int b, int c, int d, int e, int f, int g, int h)
+ {
+   volatile int i = (a + b) - (g + h) + bar ();
+   volatile int j = (e + f) - (c + d);
+ 
+   return a + b + c + d + e + f + g + h + i + j;
+ }
+ #endif
+ 
+ int
+ main (void)
+ {
+ #ifndef __thumb__
+   if (foo (1, 2, 3, 4, 5, 6, 7, 8) != 32)
+     abort ();
+     
+   if (foo1 (1, 2, 3, 4, 5, 6, 7, 8) != 32)
+     abort ();
+ #endif
+   exit (0);
+ }