[rfa:ppc] Eliminate write_sp, but how?

[rfa:ppc] Eliminate write_sp, but how?

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 937 bytes --]

This one liner isn't obvious.

The various ABIs have a requrement that the SP be decremented before 
writing anything to the stack (ignoring the red zone).  This is to stop 
signal handlers and the like trashing the callers stack.  The two PPC 
push_dummy_call (nee push_arguments) methods are already carefully 
complying to this requirement (setting SP before using the stack) and 
hence [deprecated] write_sp method which also sets the SP before a call 
is entirely redundant.

Thing is, I don't see any reason to exactly matching the ABI behavior 
(that thread is stopped so it won't get anything writing to its stack) 
and further, I think exactly matching the behavior makes the code harder 
to understand (I'm having trouble convincing my self that it does what I 
think it does :-).

So, in addition to eliminating deprecated write_sp, would it be ok to 
move the write SP code to the end of the push_dummy_call methods?

Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 733 bytes --]

2003-09-10  Andrew Cagney  <cagney@redhat.com>

	* rs6000-tdep.c (rs6000_gdbarch_init): Do not set
	"deprecated_dummy_write_sp".

Index: rs6000-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.155
diff -u -r1.155 rs6000-tdep.c
--- rs6000-tdep.c	9 Sep 2003 22:41:47 -0000	1.155
+++ rs6000-tdep.c	10 Sep 2003 21:03:39 -0000
@@ -2896,7 +2896,6 @@
     set_gdbarch_print_insn (gdbarch, gdb_print_insn_powerpc);
 
   set_gdbarch_write_pc (gdbarch, generic_target_write_pc);
-  set_gdbarch_deprecated_dummy_write_sp (gdbarch, deprecated_write_sp);
 
   set_gdbarch_num_regs (gdbarch, v->nregs);
   set_gdbarch_num_pseudo_regs (gdbarch, v->npregs);

Re: [rfa:ppc] Eliminate write_sp, but how?

[Kevin Buettner]

On Sep 10,  5:39pm, Andrew Cagney wrote:

> The various ABIs have a requrement that the SP be decremented before 
> writing anything to the stack (ignoring the red zone).  This is to stop 
> signal handlers and the like trashing the callers stack.  The two PPC 
> push_dummy_call (nee push_arguments) methods are already carefully 
> complying to this requirement (setting SP before using the stack) and 
> hence [deprecated] write_sp method which also sets the SP before a call 
> is entirely redundant.
> 
> Thing is, I don't see any reason to exactly matching the ABI behavior 
> (that thread is stopped so it won't get anything writing to its stack) 
> and further, I think exactly matching the behavior makes the code harder 
> to understand (I'm having trouble convincing my self that it does what I 
> think it does :-).

I agree with you.

> So, in addition to eliminating deprecated write_sp, would it be ok to 
> move the write SP code to the end of the push_dummy_call methods?

I can't think of any problems that would arise from moving the "write
SP" code to the end of the various push_dummy_call() methods.  But
just in case, when you change it, please note how it used to be done
and why doing it in a different location *shouldn't* be a problem. 
(It may someday make it easier to debug that obscure target which
randomly picks a stopped thread's stack to service an interrupt...)

> 2003-09-10  Andrew Cagney  <cagney@redhat.com>
> 
> 	* rs6000-tdep.c (rs6000_gdbarch_init): Do not set
> 	"deprecated_dummy_write_sp".

Okay.

Kevin

Re: [rfa:ppc] Eliminate write_sp, but how?

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 610 bytes --]


>> So, in addition to eliminating deprecated write_sp, would it be ok to 
>> move the write SP code to the end of the push_dummy_call methods?
> 
> 
> I can't think of any problems that would arise from moving the "write
> SP" code to the end of the various push_dummy_call() methods.  But
> just in case, when you change it, please note how it used to be done
> and why doing it in a different location *shouldn't* be a problem. 
> (It may someday make it easier to debug that obscure target which
> randomly picks a stopped thread's stack to service an interrupt...)

This is what I've checked in.

Andrew


[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 2983 bytes --]

2003-09-11  Andrew Cagney  <cagney@redhat.com>

	* rs6000-tdep.c (rs6000_push_dummy_call): Use
	regcache_raw_write_signed to set SP_REGNUM, move the operation to
	near the function's end.
	(rs6000_gdbarch_init): Do not set "deprecated_dummy_write_sp".
	* ppc-sysv-tdep.c (ppc_sysv_abi_push_dummy_call): Use
	regcache_raw_write_signed to set SP_REGNUM.

Index: ppc-sysv-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/ppc-sysv-tdep.c,v
retrieving revision 1.8
diff -u -r1.8 ppc-sysv-tdep.c
--- ppc-sysv-tdep.c	9 Sep 2003 22:41:47 -0000	1.8
+++ ppc-sysv-tdep.c	11 Sep 2003 19:23:29 -0000
@@ -189,8 +189,8 @@
   /* Make sure that we maintain 16 byte alignment */
   sp &= ~0x0f;
 
-  /* Update %sp before proceeding any further */
-  write_register (SP_REGNUM, sp);
+  /* Update %sp before proceeding any further.   */
+  regcache_raw_write_signed (regcache, SP_REGNUM, sp);
 
   /* write the backchain */
   store_unsigned_integer (old_sp_buf, 4, saved_sp);
Index: rs6000-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.155
diff -u -r1.155 rs6000-tdep.c
--- rs6000-tdep.c	9 Sep 2003 22:41:47 -0000	1.155
+++ rs6000-tdep.c	11 Sep 2003 19:23:34 -0000
@@ -1246,14 +1246,6 @@
       space = (space + 15) & -16;
       sp -= space;
 
-      /* This is another instance we need to be concerned about
-         securing our stack space. If we write anything underneath %sp
-         (r1), we might conflict with the kernel who thinks he is free
-         to use this area. So, update %sp first before doing anything
-         else.  */
-
-      write_register (SP_REGNUM, sp);
-
       /* If the last argument copied into the registers didn't fit there 
          completely, push the rest of it into stack.  */
 
@@ -1294,14 +1286,17 @@
 	  ii += ((len + 3) & -4) / 4;
 	}
     }
-  else
-    /* Secure stack areas first, before doing anything else.  */
-    write_register (SP_REGNUM, sp);
 
   /* set back chain properly */
   store_unsigned_integer (tmp_buffer, 4, saved_sp);
   write_memory (sp, tmp_buffer, 4);
 
+  /* Set the stack pointer.  According to the ABI, the SP is ment to
+     be set _before_ the corresponding stack space is used.  No need
+     for that here though - the target has been completly stopped - it
+     isn't possible for an exception handler to stomp on the stack.  */
+  regcache_raw_write_signed (regcache, SP_REGNUM, sp);
+
   /* Point the inferior function call's return address at the dummy's
      breakpoint.  */
   regcache_raw_write_signed (regcache, tdep->ppc_lr_regnum, bp_addr);
@@ -2896,7 +2891,6 @@
     set_gdbarch_print_insn (gdbarch, gdb_print_insn_powerpc);
 
   set_gdbarch_write_pc (gdbarch, generic_target_write_pc);
-  set_gdbarch_deprecated_dummy_write_sp (gdbarch, deprecated_write_sp);
 
   set_gdbarch_num_regs (gdbarch, v->nregs);
   set_gdbarch_num_pseudo_regs (gdbarch, v->npregs);