[PATCH] __builtin_apply / -fdefer-pop bug (ObjC fix!)

[PATCH] __builtin_apply / -fdefer-pop bug (ObjC fix!)

[Jason McMullan]

(please CC: to me, as I do not follow the *@gcc.gnu.org lists)

	As many well know, GCC since 2.8.x has had a lot
of bugs with the __builtin_apply() function, especially
with the ObjC front-end. Come to find out, it doesn't
appear to be a problem with __builtin_apply() at all,
but with an over-optimization of -fdefer-pop.

	When compiled with -fdefer-pop (without the following
patch) GCC neglects to set the stack pointer appropriately
after the memcpy() call (to move the stack arguments), and
the called function (that __builtin_apply() was trying to call)
is sent a bogus stack pointer.

	With the following patch, a NO_DEFER_POP/OK_DEFER_POP
pair - which temporarily inhibits -fdefer-pop - is placed 
around the critical section in:

	gcc/expr.c:expand_builtin_apply()

	Upon recompile of libobjc (or any other source that uses
__builtin_apply()) correct functionality of __builtin_apply
(and henve forward::/performv:: ) is restored to the ObjectiveC
language runtime.

	A closer inspection as to why the -fdefer-pop optimization
fails in this instance should be looked at, but this workaround
appears to mirror similar uses of NO_DEFER_POP/OK_DEFER_POP in
expr.c.

	* gcc/expr.c - Fix expand_builin_apply() due to -fdefer-pop breakage

------------------- Cut here ----------------
--- expr.c.old	Wed Jun 30 18:59:55 1999
+++ expr.c	Wed Jul 26 16:21:37 2000
@@ -9987,6 +9987,8 @@
   rtx old_stack_level = 0;
   rtx call_fusage = 0;
 
+  NO_DEFER_POP;
+
   /* Create a block where the return registers can be saved.  */
   result = assign_stack_local (BLKmode, apply_result_size (), -1);
 
@@ -10145,6 +10147,8 @@
   else
 #endif
     emit_stack_restore (SAVE_BLOCK, old_stack_level, NULL_RTX);
+
+  OK_DEFER_POP;
 
   /* Return the address of the result block.  */
   return copy_addr_to_reg (XEXP (result, 0));
------------------- Cut here ----------------
-- 
Jason McMullan, Senior Linux Consultant, Linuxcare, Inc.
412.422.8077 tel, 412.656.3519 cell, 415.701.0792 fax
jmcmullan@linuxcare.com, http://www.linuxcare.com/
Linuxcare. Support for the revolution.

Re: [PATCH] __builtin_apply / -fdefer-pop bug (ObjC fix!)

[Ovidiu Predescu]

This is really great! People have been complaining about __builtin_* functions
for a long time now. There were even thoughts on replacing these functions and
use external libraries to solve the method invocation. These solutions however
don't solve the forwarding problem so if your fix solves the issue, it would be
really nice!

Just as a curiosity, is the -fdefer-pop optimization performed when the ObjC
source is compiled with -O?

Best regards,
Ovidiu

-- 
Ovidiu Predescu <ovidiu@cup.hp.com>
http://orion.nsr.hp.com/ (inside HP's firewall only)
http://www.geocities.com/SiliconValley/Monitor/7464/

On Wed, 26 Jul 2000 16:36:07 -0400, Jason McMullan <jmcmullan@linuxcare.com> 
wrote:

> (please CC: to me, as I do not follow the *@gcc.gnu.org lists)
> 
> 	As many well know, GCC since 2.8.x has had a lot
> of bugs with the __builtin_apply() function, especially
> with the ObjC front-end. Come to find out, it doesn't
> appear to be a problem with __builtin_apply() at all,
> but with an over-optimization of -fdefer-pop.
> 
> 	When compiled with -fdefer-pop (without the following
> patch) GCC neglects to set the stack pointer appropriately
> after the memcpy() call (to move the stack arguments), and
> the called function (that __builtin_apply() was trying to call)
> is sent a bogus stack pointer.
> 
> 	With the following patch, a NO_DEFER_POP/OK_DEFER_POP
> pair - which temporarily inhibits -fdefer-pop - is placed 
> around the critical section in:
> 
> 	gcc/expr.c:expand_builtin_apply()
> 
> 	Upon recompile of libobjc (or any other source that uses
> __builtin_apply()) correct functionality of __builtin_apply
> (and henve forward::/performv:: ) is restored to the ObjectiveC
> language runtime.
> 
> 	A closer inspection as to why the -fdefer-pop optimization
> fails in this instance should be looked at, but this workaround
> appears to mirror similar uses of NO_DEFER_POP/OK_DEFER_POP in
> expr.c.
> 
> 	* gcc/expr.c - Fix expand_builin_apply() due to -fdefer-pop breakage
> 
> ------------------- Cut here ----------------
> --- expr.c.old	Wed Jun 30 18:59:55 1999
> +++ expr.c	Wed Jul 26 16:21:37 2000
> @@ -9987,6 +9987,8 @@
>    rtx old_stack_level = 0;
>    rtx call_fusage = 0;
>  
> +  NO_DEFER_POP;
> +
>    /* Create a block where the return registers can be saved.  */
>    result = assign_stack_local (BLKmode, apply_result_size (), -1);
>  
> @@ -10145,6 +10147,8 @@
>    else
>  #endif
>      emit_stack_restore (SAVE_BLOCK, old_stack_level, NULL_RTX);
> +
> +  OK_DEFER_POP;
>  
>    /* Return the address of the result block.  */
>    return copy_addr_to_reg (XEXP (result, 0));
> ------------------- Cut here ----------------
> -- 
> Jason McMullan, Senior Linux Consultant, Linuxcare, Inc.
> 412.422.8077 tel, 412.656.3519 cell, 415.701.0792 fax
> jmcmullan@linuxcare.com, http://www.linuxcare.com/
> Linuxcare. Support for the revolution.
> 

Re: [PATCH] __builtin_apply / -fdefer-pop bug (ObjC fix!)

[Jason McMullan]

Ovidiu Predescu said with excellent clarity:
> 
> This is really great! People have been complaining about __builtin_* functions
> for a long time now. There were even thoughts on replacing these functions and
> use external libraries to solve the method invocation. These solutions however
> don't solve the forwarding problem so if your fix solves the issue, it would be
> really nice!

	I'm just waiting on reports for other architectures.
 
> Just as a curiosity, is the -fdefer-pop optimization performed when the ObjC
> source is compiled with -O?

	Yes. -fdefer-pop is in -O1 and higher.

-- 
Jason McMullan, Senior Linux Consultant, Linuxcare, Inc.
412.422.8077 tel, 412.656.3519 cell, 415.701.0792 fax
jmcmullan@linuxcare.com, http://www.linuxcare.com/
Linuxcare. Support for the revolution.

Re: [PATCH] __builtin_apply / -fdefer-pop bug (ObjC fix!)

[Richard Henderson]

On Wed, Jul 26, 2000 at 04:36:07PM -0400, Jason McMullan wrote:
> 	When compiled with -fdefer-pop (without the following
> patch) GCC neglects to set the stack pointer appropriately
> after the memcpy() call (to move the stack arguments), and
> the called function (that __builtin_apply() was trying to call)
> is sent a bogus stack pointer.

You didn't point at a test case, which would have been helpful.
Nor did you work against current gcc sources, which have changed.

However, the solution seems reasonable.  I've applied the following.


r~


2000-07-31  Jason McMullan  <jmcmullan@linuxcare.com>

        * builtins.c (expand_builtin_apply): Don't defer pop during
        argument setup.

Index: builtins.c
===================================================================
RCS file: /cvs/gcc/egcs/gcc/builtins.c,v
retrieving revision 1.51
diff -c -p -d -r1.51 builtins.c
*** builtins.c	2000/07/03 18:28:33	1.51
--- builtins.c	2000/07/31 16:34:43
*************** expand_builtin_apply (function, argument
*** 885,892 ****
    /* Perform postincrements before actually calling the function.  */
    emit_queue ();
  
!   /* Push a new argument block and copy the arguments.  */
    do_pending_stack_adjust ();
  
    /* Save the stack with nonlocal if available */
  #ifdef HAVE_save_stack_nonlocal
--- 885,895 ----
    /* Perform postincrements before actually calling the function.  */
    emit_queue ();
  
!   /* Push a new argument block and copy the arguments.  Do not allow
!      the (potential) memcpy call below to interfere with our stack
!      manipulations.  */
    do_pending_stack_adjust ();
+   NO_DEFER_POP;
  
    /* Save the stack with nonlocal if available */
  #ifdef HAVE_save_stack_nonlocal
*************** expand_builtin_apply (function, argument
*** 1025,1030 ****
--- 1028,1035 ----
    else
  #endif
      emit_stack_restore (SAVE_BLOCK, old_stack_level, NULL_RTX);
+ 
+   OK_DEFER_POP;
  
    /* Return the address of the result block.  */
    return copy_addr_to_reg (XEXP (result, 0));