Re: builtin_return_addr vs frame_pointer_needed vs -O3

[Ian Dall <Ian dot Dall at dsto dot defence dot gov dot au>]

I have come across the same problem for the ns32k port and in looking
at this solution for the i386 from the archives I don't think it is
correct, or at least not general.

Richard Henderson <rth@redhat.com> writes:

  > On Wed, Jan 31, 2001 at 05:26:51PM -0500, DJ Delorie wrote:
  >> OK, I'm looking at a 20010122-1 failure (ix86-linux), which seems to
  >> be caused by test4a() not having a valid frame through which to find
  >> the right return address.

  > Yep.  Technically we don't need one in this example, because we could
  > just use the incomming ebp value.  However, with a non-trivial function
  > it becomes non-trivial to arrange for this to happen.  So let's not.

  >> So, I use SETUP_FRAME_ADDRESSES to set frame_pointer_needed, but it
  >> doesn't work.  A few printfs later, I see that gcc is parsing a bunch
  >> of functions all at once, but deferring the asm output until later,
  >> when frame_pointer_needed is no longer valid.

  > Actually, it's worse than that -- we overwrite frame_pointer_needed
  > at the beginning of reload.  Every time.  So we need to store this
  > information away somewhere until needed.

The problem is, test4a() uses __builtin_return_address(1). Turning of
FRAME_POINTER_REQUIRED for the current function if it accesses a
previous frame does not work for __builtin_return_address(n) when n
greater than 1. To do that, you need to ensure that there are frame
pointers in all the intervening frames, not just the current frame.

It is not practical (or maybe even determinable) to work out all the
functions which could be in the stack trace, so I think a better
solution would be for RETURN_ADDR_RTX to do something like:

 ((COUNT > 0 && flag_omit_frame_pointer) ? NULL_RTX: GEN_RTX (...))

This could possibly be used in addition to the current hack by testing
for COUNT > 1 instead. One problem with this is it only addresses the
question of __builtin_return_address. If there are other places where
we need to access contents of previous frames, then it is really it would
be better to be able to tell gcc that the generation of the frame_address
has failed, possibly by appropriate definition of DYNAMIC_CHAIN_ADDRESS
and have the middle end automatically do the right thing for anything
which tries to reference the frame address.

Ian



  > 	* config/i386/i386.c (ix86_frame_pointer_required): New.
  > 	(ix86_setup_frame_addresses): New.
  > 	(struct machine_funciton): Add accesses_prev_frame.
  > 	* config/i386/i386.h (FRAME_POINTER_REQUIRED): Call
  > 	ix86_frame_pointer_required.
  > 	(SUBTARGET_FRAME_POINTER_REQUIRED): New.
  > 	(SETUP_FRAME_ADDRESSES): New.
  > 	* config/i386/i386-protos.h: Update.
  > 	* config/i386/sco5.h (SUBTARGET_FRAME_POINTER_REQUIRED): Rename
  > 	from FRAME_POINTER_REQUIRED.
  > 	* config/i386/svr3gas.h: Likewise.
  > 	* config/i386/sysv3.h: Likewise.
  > 	* config/i386/v3gas.h: Likewise.

  > Index: i386-protos.h
  > ===================================================================
  > RCS file: /cvs/gcc/egcs/gcc/config/i386/i386-protos.h,v
  > retrieving revision 1.35
  > diff -c -p -d -r1.35 i386-protos.h
  > *** i386-protos.h	2001/01/15 23:43:10	1.35
  > --- i386-protos.h	2001/02/01 00:54:02
  > *************** extern void order_regs_for_local_alloc P
  > *** 25,30 ****
  > --- 25,32 ----
  >   extern void optimization_options PARAMS ((int, int));
  
  >   extern int ix86_can_use_return_insn_p PARAMS ((void));
  > + extern int ix86_frame_pointer_required PARAMS ((void));
  > + extern void ix86_setup_frame_addresses PARAMS ((void));
  
  >   extern void ix86_asm_file_end PARAMS ((FILE *));
  >   extern void load_pic_register PARAMS ((void));
  > Index: i386.c
  > ===================================================================
  > RCS file: /cvs/gcc/egcs/gcc/config/i386/i386.c,v
  > retrieving revision 1.208
  > diff -c -p -d -r1.208 i386.c
  > *** i386.c	2001/01/28 01:50:22	1.208
  > --- i386.c	2001/02/01 00:54:02
  > *************** struct rtx_def *ix86_compare_op1 = NULL_
  > *** 331,336 ****
  > --- 331,337 ----
  >   struct machine_function
  >   {
  >     rtx stack_locals[(int) MAX_MACHINE_MODE][MAX_386_STACK_LOCALS];
  > +   int accesses_prev_frame;
  >   };
  
  >   #define ix86_stack_locals (cfun->machine->stack_locals)
  > *************** ix86_can_use_return_insn_p ()
  > *** 1687,1692 ****
  > --- 1688,1727 ----
  
  >     tsize = ix86_compute_frame_size (get_frame_size (), &nregs, NULL, NULL);
  >     return tsize == 0 && nregs == 0;
  > + }
  > + 
  > + /* Value should be nonzero if functions must have frame pointers.
  > +    Zero means the frame pointer need not be set up (and parms may
  > +    be accessed via the stack pointer) in functions that seem suitable.  */
  > + 
  > + int
  > + ix86_frame_pointer_required ()
  > + {
  > +   /* If we accessed previous frames, then the generated code expects
  > +      to be able to access the saved ebp value in our frame.  */
  > +   if (cfun->machine->accesses_prev_frame)
  > +     return 1;
  > +   
  > +   /* Several x86 os'es need a frame pointer for other reasons,
  > +      usually pertaining to setjmp.  */
  > +   if (SUBTARGET_FRAME_POINTER_REQUIRED)
  > +     return 1;
  > + 
  > +   /* In override_options, TARGET_OMIT_LEAF_FRAME_POINTER turns off
  > +      the frame pointer by default.  Turn it back on now if we've not
  > +      got a leaf function.  */
  > +   if (TARGET_OMIT_LEAF_FRAME_POINTER && ! leaf_function_p ())
  > +     return 1;
  > + 
  > +   return 0;
  > + }
  > + 
  > + /* Record that the current function accesses previous call frames.  */
  > + 
  > + void
  > + ix86_setup_frame_addresses ()
  > + {
  > +   cfun->machine->accesses_prev_frame = 1;
  >   }
  >   
  >   static char pic_label_name[32];
  > Index: i386.h
  > ===================================================================
  > RCS file: /cvs/gcc/egcs/gcc/config/i386/i386.h,v
  > retrieving revision 1.146
  > diff -c -p -d -r1.146 i386.h
  > *** i386.h	2001/01/16 17:32:26	1.146
  > --- i386.h	2001/02/01 00:54:02
  > *************** extern int ix86_arch;
  > *** 852,858 ****
  >      Zero means the frame pointer need not be set up (and parms
  >      may be accessed via the stack pointer) in functions that seem suitable.
  >      This is computed in `reload', in reload1.c.  */
  > ! #define FRAME_POINTER_REQUIRED (TARGET_OMIT_LEAF_FRAME_POINTER && !leaf_function_p ()) 	
  
  >   /* Base register for access to arguments of the function.  */
  >   #define ARG_POINTER_REGNUM 16
  > --- 852,867 ----
  >      Zero means the frame pointer need not be set up (and parms
  >      may be accessed via the stack pointer) in functions that seem suitable.
  >      This is computed in `reload', in reload1.c.  */
  > ! #define FRAME_POINTER_REQUIRED  ix86_frame_pointer_required ()
  > ! 
  > ! /* Override this in other tm.h files to cope with various OS losage
  > !    requiring a frame pointer.  */
  > ! #ifndef SUBTARGET_FRAME_POINTER_REQUIRED
  > ! #define SUBTARGET_FRAME_POINTER_REQUIRED 0
  > ! #endif
  > ! 
  > ! /* Make sure we can access arbitrary call frames.  */
  > ! #define SETUP_FRAME_ADDRESSES()  ix86_setup_frame_addresses ()
  
  >   /* Base register for access to arguments of the function.  */
  >   #define ARG_POINTER_REGNUM 16
  > Index: sco5.h
  > ===================================================================
  > RCS file: /cvs/gcc/egcs/gcc/config/i386/sco5.h,v
  > retrieving revision 1.42
  > diff -c -p -d -r1.42 sco5.h
  > *** sco5.h	2001/01/15 23:43:10	1.42
  > --- sco5.h	2001/02/01 00:54:02
  > *************** dtors_section ()							\
  > *** 632,639 ****
  >       }									\
  >   }
  
  > ! #undef FRAME_POINTER_REQUIRED
  > ! #define FRAME_POINTER_REQUIRED						\
  >     ((TARGET_ELF) ? 0 : 							\
  >      (current_function_calls_setjmp || current_function_calls_longjmp))
  
  > --- 632,639 ----
  >       }									\
  >   }
  
  > ! #undef SUBTARGET_FRAME_POINTER_REQUIRED
  > ! #define SUBTARGET_FRAME_POINTER_REQUIRED				\
  >     ((TARGET_ELF) ? 0 : 							\
  >      (current_function_calls_setjmp || current_function_calls_longjmp))
  
  > Index: svr3gas.h
  > ===================================================================
  > RCS file: /cvs/gcc/egcs/gcc/config/i386/svr3gas.h,v
  > retrieving revision 1.5
  > diff -c -p -d -r1.5 svr3gas.h
  > *** svr3gas.h	2000/09/25 13:03:19	1.5
  > --- svr3gas.h	2001/02/01 00:54:02
  > *************** Boston, MA 02111-1307, USA.  */
  > *** 37,44 ****
  >      Since a frame pointer will be required in such a function, it is OK
  >      that the stack pointer is not restored.  */
  
  > ! #undef FRAME_POINTER_REQUIRED
  > ! #define FRAME_POINTER_REQUIRED \
  >     (current_function_calls_setjmp || current_function_calls_longjmp)
  
  >   /* Modify ASM_OUTPUT_LOCAL slightly to test -msvr3-shlib, adapted to gas  */
  > --- 37,44 ----
  >      Since a frame pointer will be required in such a function, it is OK
  >      that the stack pointer is not restored.  */
  
  > ! #undef SUBTARGET_FRAME_POINTER_REQUIRED
  > ! #define SUBTARGET_FRAME_POINTER_REQUIRED \
  >     (current_function_calls_setjmp || current_function_calls_longjmp)
  
  >   /* Modify ASM_OUTPUT_LOCAL slightly to test -msvr3-shlib, adapted to gas  */
  > Index: sysv3.h
  > ===================================================================
  > RCS file: /cvs/gcc/egcs/gcc/config/i386/sysv3.h,v
  > retrieving revision 1.5
  > diff -c -p -d -r1.5 sysv3.h
  > *** sysv3.h	2000/11/02 23:29:10	1.5
  > --- sysv3.h	2001/02/01 00:54:02
  > *************** Boston, MA 02111-1307, USA.  */
  > *** 78,85 ****
  >      Since a frame pointer will be required in such a function, it is OK
  >      that the stack pointer is not restored.  */
  
  > ! #undef FRAME_POINTER_REQUIRED
  > ! #define FRAME_POINTER_REQUIRED \
  >     (current_function_calls_setjmp || current_function_calls_longjmp)
  
  >   /* Modify ASM_OUTPUT_LOCAL slightly to test -msvr3-shlib.  */
  > --- 78,85 ----
  >      Since a frame pointer will be required in such a function, it is OK
  >      that the stack pointer is not restored.  */
  
  > ! #undef SUBTARGET_FRAME_POINTER_REQUIRED
  > ! #define SUBTARGET_FRAME_POINTER_REQUIRED \
  >     (current_function_calls_setjmp || current_function_calls_longjmp)
  
  >   /* Modify ASM_OUTPUT_LOCAL slightly to test -msvr3-shlib.  */
  > Index: v3gas.h
  > ===================================================================
  > RCS file: /cvs/gcc/egcs/gcc/config/i386/v3gas.h,v
  > retrieving revision 1.2
  > diff -c -p -d -r1.2 v3gas.h
  > *** v3gas.h	1998/12/16 21:04:44	1.2
  > --- v3gas.h	2001/02/01 00:54:02
  > *************** Boston, MA 02111-1307, USA.  */
  > *** 37,44 ****
  >      Since a frame pointer will be required in such a function, it is OK
  >      that the stack pointer is not restored.  */
  
  > ! #undef FRAME_POINTER_REQUIRED
  > ! #define FRAME_POINTER_REQUIRED \
  >     (current_function_calls_setjmp || current_function_calls_longjmp)
  
  >   /* Modify ASM_OUTPUT_LOCAL slightly to test -msvr3-shlib, adapted to gas  */
  > --- 37,44 ----
  >      Since a frame pointer will be required in such a function, it is OK
  >      that the stack pointer is not restored.  */
  
  > ! #undef SUBTARGET_FRAME_POINTER_REQUIRED
  > ! #define SUBTARGET_FRAME_POINTER_REQUIRED \
  >     (current_function_calls_setjmp || current_function_calls_longjmp)
  
  >   /* Modify ASM_OUTPUT_LOCAL slightly to test -msvr3-shlib, adapted to gas  */