Re: [PATCH] Incorrect code for __builtin_frame_address(0)

[James E Wilson <wilson at specifix dot com>]

Maciej W. Rozycki wrote:
> 	* config/alpha/alpha.h (INITIAL_FRAME_ADDRESS_RTX): New macro.
> 	* config/i386/i386.h (INITIAL_FRAME_ADDRESS_RTX): Likewise.
> 	* config/mips/mips.h (INITIAL_FRAME_ADDRESS_RTX): Likewise.

Tracking this down, I found 5 different threads that discussed various
parts of this problem.  I have attached a list of them.

This leads to the following possible solutions
1) Add machine dependent definitions of INITIAL_FRAME_ADDRESS_RTX to all
ports other than s390.
2) Define INITIAL_FRAME_ADDRESS_RTX to frame_pointer_rtx in a few ports.
 Or perhaps all other than s390.
3) Default INITIAL_FRAME_ADDRESS_RTX to frame_pointer_rtx.
4) Default INITIAL_FRAME_POINTER_RTX to frame_pointer_rtx only if the
count is 0.
5) Disable frame pointer elimination if INITIAL_FRAME_POINTER_RTX is not
defined.
6) Use 4 if count == 0, and 5 otherwise.

Solution 1 is impractical.  Andreas Krebbel claims that solutions 2 and
3 will break support for count != 0.  I haven't tried looking at this
myself, but the argument looks credible.  Solution 4 looks good, except
that it leaves the count != 0 case broken.  Solution 5 is safe, and easy
to implement, but disables optimization unnecessarily for the count == 0
case.  Solution 6 is less safe, but avoids the optimization loss except
when count != 0, which will be rare, and hence seems like the best
choice.  The main problem with this choice is that it is harder to write
the docs for the INITIAL_FRAME_POINTER_RTX macro.

I've attached patches for solutions 5 and 6.  Unless someone recommends
otherwise, I expect that I will go with solution 6.  I haven't tested
this patches yet, except on trivial testcases.

:REVIEWURL http://gcc.gnu.org/ml/gcc-patches/2005-06/msg00251.html:
-- 
Jim Wilson, GNU Tools Support, http://www.specifix.com

References:
[1] http://gcc.gnu.org/ml/gcc-patches/2001-03/msg00984.html
[2] http://gcc.gnu.org/ml/gcc/2002-01/msg01420.html
[3] http://gcc.gnu.org/ml/gcc-patches/2005-01/msg00820.html
[4] http://gcc.gnu.org/ml/gcc-patches/2005-01/msg00998.html
[5] http://gcc.gnu.org/ml/gcc-patches/2005-06/msg00251.html

[1] Mail from Jim Wilson, pointing out that reload eliminates the FP even
when we have direct uses of the hard_frame_pointer, and that reload should be
fixed.  I didn't notice that this meant that -fomit-frame-pointer broke
__builtin_frame_address.  I was looking at a different more obscure problem.
Did not attempt the reload fix because we were too close to a release.
[2] Mail from Geoff Keating, referring to my mail, and pointing out the
__builtin_frame_address and -fomit-frame-pointer problem implied by it.  Did
not attempt fix.
[3] Mail from Andreas Krebbel pointing out that mudflap's usage of
builtin_frame_address was broken because of the -fomit-frame-pointer problem.
[4] Mail from Andreas Krebbel proposing the INITIAL_FRAME_ADDRESS_RTX macro.
[5] Mail from Maciej W. Rozycki proposing to define INITIAL_FRAME_ADDRESS_RTX
is multiple back ends.

In threads 3 and 4, Andreas Krebbel points out that simply defining 
INITIAL_FRAME_ADDRESS_RTX to frame_pointer_rtx breaks builtin_frame_address
for counts larger than 0.

solution 5
safe patch, some optimization loss always, clear documentation

2005-08-17  James E Wilson  <wilson@specifix.com>

	* builtins.c (expand_builtin_return_addr): Set
	current_function_calls_builtin_return_addr.
	* function.h (struct function): Add calls_builtin_return_addr field.
	(current_function_calls_builtin_return_addr): Define.
	* reload1.c (init_elim_table): Check
	current_function_calls_builtin_return_addr.
	* doc/tm.texi (INITIAL_FRAME_ADDRESS_RTX): Update docs.

Index: builtins.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/builtins.c,v
retrieving revision 1.470
diff -p -p -r1.470 builtins.c
*** builtins.c	16 Aug 2005 12:14:08 -0000	1.470
--- builtins.c	18 Aug 2005 02:04:35 -0000
*************** expand_builtin_return_addr (enum built_i
*** 486,491 ****
--- 486,494 ----
    rtx tem = INITIAL_FRAME_ADDRESS_RTX;
  #else
    rtx tem = hard_frame_pointer_rtx;
+ 
+   /* Tell reload not to eliminate the frame pointer.  */
+   current_function_calls_builtin_return_addr = 1;
  #endif
  
    /* Some machines need special handling before we can access
Index: function.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/function.h,v
retrieving revision 1.156
diff -p -p -r1.156 function.h
*** function.h	30 Jun 2005 00:47:48 -0000	1.156
--- function.h	18 Aug 2005 02:04:35 -0000
*************** struct function GTY(())
*** 394,399 ****
--- 394,402 ----
       either as a subroutine or builtin.  */
    unsigned int calls_alloca : 1;
  
+   /* Nonzero if function being compiled can call builtin_return_addr.  */
+   unsigned int calls_builtin_return_addr : 1;
+ 
    /* Nonzero if the function calls __builtin_eh_return.  */
    unsigned int calls_eh_return : 1;
  
*************** extern int trampolines_created;
*** 483,488 ****
--- 486,493 ----
  #define current_function_returns_pointer (cfun->returns_pointer)
  #define current_function_calls_setjmp (cfun->calls_setjmp)
  #define current_function_calls_alloca (cfun->calls_alloca)
+ #define current_function_calls_builtin_return_addr \
+   (cfun->calls_builtin_return_addr)
  #define current_function_calls_eh_return (cfun->calls_eh_return)
  #define current_function_is_thunk (cfun->is_thunk)
  #define current_function_args_info (cfun->args_info)
Index: reload1.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/reload1.c,v
retrieving revision 1.478
diff -p -p -r1.478 reload1.c
*** reload1.c	1 Aug 2005 03:54:47 -0000	1.478
--- reload1.c	18 Aug 2005 02:04:36 -0000
*************** init_elim_table (void)
*** 3492,3497 ****
--- 3492,3498 ----
  			     sp-adjusting insns for this case.  */
  			  || (current_function_calls_alloca
  			      && EXIT_IGNORE_STACK)
+ 			  || current_function_calls_builtin_return_addr
  			  || FRAME_POINTER_REQUIRED);
  
    num_eliminable = 0;
Index: doc/tm.texi
===================================================================
RCS file: /cvs/gcc/gcc/gcc/doc/tm.texi,v
retrieving revision 1.445
diff -p -p -r1.445 tm.texi
*** doc/tm.texi	16 Aug 2005 22:29:09 -0000	1.445
--- doc/tm.texi	18 Aug 2005 03:03:55 -0000
*************** machines.  See @file{function.c} for det
*** 2812,2825 ****
  
  @defmac INITIAL_FRAME_ADDRESS_RTX
  A C expression whose value is RTL representing the address of the initial
!  stack frame. This address is passed to @code{RETURN_ADDR_RTX} and 
! @code{DYNAMIC_CHAIN_ADDRESS}.
! If you don't define this macro, the default is to return 
! @code{hard_frame_pointer_rtx}.
! This default is usually correct unless @code{-fomit-frame-pointer} is in 
! effect.
! Define this macro in order to make @code{__builtin_frame_address (0)} and 
! @code{__builtin_return_address (0)} work even in absence of a hard frame pointer.
  @end defmac
  
  @defmac DYNAMIC_CHAIN_ADDRESS (@var{frameaddr})
--- 2812,2823 ----
  
  @defmac INITIAL_FRAME_ADDRESS_RTX
  A C expression whose value is RTL representing the address of the initial
! stack frame. This address is passed to @code{RETURN_ADDR_RTX} and 
! @code{DYNAMIC_CHAIN_ADDRESS}.  If you don't define this macro, the default
! is to return  @code{hard_frame_pointer_rtx}, and disable frame pointer
! elimination.  Define this macro in order to make frame pointer elimination
! work in the presence of @code{__builtin_frame_address (0)} and 
! @code{__builtin_return_address (0)}.
  @end defmac
  
  @defmac DYNAMIC_CHAIN_ADDRESS (@var{frameaddr})

solution 6
less safe patch, optimization loss only with count != 0, less clear docs

2005-08-17  James E Wilson  <wilson@specifix.com>

	* builtins.c (expand_builtin_return_addr): Set
	current_function_calls_builtin_return_addr when count != 0.  Use
	frame_pointer_rtx when count == 0.
	* function.h (struct function): Add calls_builtin_return_addr field.
	(current_function_calls_builtin_return_addr): Define.
	* reload1.c (init_elim_table): Check
	current_function_calls_builtin_return_addr.
	* doc/tm.texi (INITIAL_FRAME_ADDRESS_RTX): Update docs.

Index: builtins.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/builtins.c,v
retrieving revision 1.470
diff -p -p -r1.470 builtins.c
*** builtins.c	16 Aug 2005 12:14:08 -0000	1.470
--- builtins.c	18 Aug 2005 03:27:52 -0000
*************** expand_builtin_return_addr (enum built_i
*** 485,491 ****
  #ifdef INITIAL_FRAME_ADDRESS_RTX
    rtx tem = INITIAL_FRAME_ADDRESS_RTX;
  #else
!   rtx tem = hard_frame_pointer_rtx;
  #endif
  
    /* Some machines need special handling before we can access
--- 485,501 ----
  #ifdef INITIAL_FRAME_ADDRESS_RTX
    rtx tem = INITIAL_FRAME_ADDRESS_RTX;
  #else
!   rtx tem;
! 
!   if (count == 0)
!     tem = frame_pointer_rtx;
!   else 
!     {
!       tem = hard_frame_pointer_rtx;
! 
!       /* Tell reload not to eliminate the frame pointer.  */
!       current_function_calls_builtin_return_addr = 1;
!     }
  #endif
  
    /* Some machines need special handling before we can access
Index: function.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/function.h,v
retrieving revision 1.156
diff -p -p -r1.156 function.h
*** function.h	30 Jun 2005 00:47:48 -0000	1.156
--- function.h	18 Aug 2005 03:27:52 -0000
*************** struct function GTY(())
*** 394,399 ****
--- 394,402 ----
       either as a subroutine or builtin.  */
    unsigned int calls_alloca : 1;
  
+   /* Nonzero if function being compiled can call builtin_return_addr.  */
+   unsigned int calls_builtin_return_addr : 1;
+ 
    /* Nonzero if the function calls __builtin_eh_return.  */
    unsigned int calls_eh_return : 1;
  
*************** extern int trampolines_created;
*** 483,488 ****
--- 486,493 ----
  #define current_function_returns_pointer (cfun->returns_pointer)
  #define current_function_calls_setjmp (cfun->calls_setjmp)
  #define current_function_calls_alloca (cfun->calls_alloca)
+ #define current_function_calls_builtin_return_addr \
+   (cfun->calls_builtin_return_addr)
  #define current_function_calls_eh_return (cfun->calls_eh_return)
  #define current_function_is_thunk (cfun->is_thunk)
  #define current_function_args_info (cfun->args_info)
Index: reload1.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/reload1.c,v
retrieving revision 1.478
diff -p -p -r1.478 reload1.c
*** reload1.c	1 Aug 2005 03:54:47 -0000	1.478
--- reload1.c	18 Aug 2005 03:27:53 -0000
*************** init_elim_table (void)
*** 3492,3497 ****
--- 3492,3498 ----
  			     sp-adjusting insns for this case.  */
  			  || (current_function_calls_alloca
  			      && EXIT_IGNORE_STACK)
+ 			  || current_function_calls_builtin_return_addr
  			  || FRAME_POINTER_REQUIRED);
  
    num_eliminable = 0;
Index: doc/tm.texi
===================================================================
RCS file: /cvs/gcc/gcc/gcc/doc/tm.texi,v
retrieving revision 1.445
diff -p -p -r1.445 tm.texi
*** doc/tm.texi	16 Aug 2005 22:29:09 -0000	1.445
--- doc/tm.texi	18 Aug 2005 03:27:54 -0000
*************** machines.  See @file{function.c} for det
*** 2812,2825 ****
  
  @defmac INITIAL_FRAME_ADDRESS_RTX
  A C expression whose value is RTL representing the address of the initial
!  stack frame. This address is passed to @code{RETURN_ADDR_RTX} and 
! @code{DYNAMIC_CHAIN_ADDRESS}.
! If you don't define this macro, the default is to return 
! @code{hard_frame_pointer_rtx}.
! This default is usually correct unless @code{-fomit-frame-pointer} is in 
! effect.
! Define this macro in order to make @code{__builtin_frame_address (0)} and 
! @code{__builtin_return_address (0)} work even in absence of a hard frame pointer.
  @end defmac
  
  @defmac DYNAMIC_CHAIN_ADDRESS (@var{frameaddr})
--- 2812,2822 ----
  
  @defmac INITIAL_FRAME_ADDRESS_RTX
  A C expression whose value is RTL representing the address of the initial
! stack frame. This address is passed to @code{RETURN_ADDR_RTX} and 
! @code{DYNAMIC_CHAIN_ADDRESS}.  If you don't define this macro, a reasonable
! default value will be used.  Define this macro in order to make frame pointer
! elimination work in the presence of @code{__builtin_frame_address (count)} and 
! @code{__builtin_return_address (count)} for @code{count} not equal to zero.
  @end defmac
  
  @defmac DYNAMIC_CHAIN_ADDRESS (@var{frameaddr})