Re: [RFA 1/2] mips: Switch inferior function calls to ON_STACK method.

[Mark Kettenis <mark dot kettenis at xs4all dot nl>]

> From: Joel Brobecker <brobecker@adacore.com>
> Date: Thu,  3 May 2012 15:03:21 -0400
> 
> This patch switches the mips code to use the ON_STACK method
> for function calls instead of AT_SYMBOL, which we want to remove.
> 
> The one difficulty came from the fact that we needed to make sure
> that the area on the stack just before where we insert the breakpoint
> instruction does not look like a branch instruction.  Otherwise,
> we get an automatic breakpoint adjustment which breaks everything.
> 
> Another little detail on the implementation of mips_push_dummy_code.
> It starts by aligning the stack.  AFAIK, the stack is supposed to
> always be aligned to at least 4 bytes (4 bytes for mips32, 8 bytes
> for mips64). So, the initial alignment shouldn't be necessary, since
> that's good enough aligment for our breakpoint instruction.  But
> in the end, I chose to keep it, JIC. We could possibly change the
> code to align to 4 instead of 16 like mips_frame_align does, if
> we want to.
> 
> gdb/ChangeLog:
> 
>         * mips-tdep.c (mips_push_dummy_code): New function.
>         (mips_gdbarch_init): Set the gdbarch call_dummy_location to
>         ON_STACK and install mips_push_dummy_code as our gdbarch
>         push_dummy_code routine.
> 
> Tested on mips-irix.  It might be nice to test on other mips targets
> as well, although it should not be necessary. OK to commit?
> 
> Thanks,
> -- 
> Joel
> 
> ---
>  gdb/mips-tdep.c |   36 ++++++++++++++++++++++++++++++++----
>  1 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> index 9a3c7fb..3e6b00b 100644
> --- a/gdb/mips-tdep.c
> +++ b/gdb/mips-tdep.c
> @@ -3009,6 +3009,36 @@ mips_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
>    return align_down (addr, 16);
>  }
>  
> +/* Implement the push_dummy_code gdbarch method for mips targets.  */

I notice people have been adding this style of comment in some other
newly contributed targets.  Do people really feel that having these is
useful?  If so, can we at least settle on a consitent style?

> +
> +static CORE_ADDR
> +mips_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,
> +		      CORE_ADDR funaddr, struct value **args,
> +		      int nargs, struct type *value_type,
> +		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
> +		      struct regcache *regcache)
> +{
> +  int bp_len;
> +  gdb_byte null_insn[4] = {0};

Missing spaces around the 0.

> +  *bp_addr = mips_frame_align (gdbarch, sp);

SP is guaranteed to be properly aligned here (see
infcall.c:call_function_by_hand()).

> +  gdbarch_breakpoint_from_pc (gdbarch, bp_addr, &bp_len);
> +
> +  /* The breakpoint layer automatically adjusts the address of
> +     breakpoints inserted in a branch delay slot.  With enough
> +     bad luck, the 4 bytes located just before our breakpoint
> +     instruction could look like a branch instruction, and thus
> +     trigger the adjustement, and break the function call entirely.
> +     So, we reserve those 4 bytes and write a null instruction
> +     to prevent that from happening.  */
> +  write_memory (*bp_addr - bp_len, null_insn, sizeof (null_insn));
> +  sp = mips_frame_align (gdbarch, *bp_addr - 2 * bp_len);
> +
> +  /* Inferior resumes at the function entry point.  */
> +  *real_pc = funaddr;
> +
> +  return sp;
> +}

Please add a blank line here.

>  static CORE_ADDR
>  mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>  			   struct regcache *regcache, CORE_ADDR bp_addr,
> @@ -6906,10 +6936,8 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  
>    /* MIPS version of CALL_DUMMY.  */
>  
> -  /* NOTE: cagney/2003-08-05: Eventually call dummy location will be
> -     replaced by a command, and all targets will default to on stack
> -     (regardless of the stack's execute status).  */
> -  set_gdbarch_call_dummy_location (gdbarch, AT_SYMBOL);
> +  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
> +  set_gdbarch_push_dummy_code (gdbarch, mips_push_dummy_code);
>    set_gdbarch_frame_align (gdbarch, mips_frame_align);
>  
>    set_gdbarch_convert_register_p (gdbarch, mips_convert_register_p);
> -- 
> 1.7.0.4
> 
>