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

["Maciej W. Rozycki" <macro@codesourcery.com>]

On Tue, 8 May 2012, Joel Brobecker wrote:

> > After some thinking I realised that the reliance on signal delivery to 
> > work properly to trap non-executable stack may actually be a problem for 
> > bare-iron targets.
> 
> IMO, I think we can start worrying about those when we actually
> encounter the problem; and I am assuming that this is not going to
> be specific to mips.

 I don't consider this a showstopper for your change either; in particular 
it does not apply to Linux kernel debugging because except from one or two 
obscure corner cases of some very old hardware that did not have memory at 
0 (except from a page worth shadow area for exception handlers) Linux runs 
its own code using unmapped segments so no memory access permissions 
apply.

 But I decided the issue had to be noted.

> Are we still good to go with this patch? Attached is the latest
> version. For any additional comments that you'd like to be added
> (in particular, with respect to what you just pointed out), I suggest
> add them as a followup patch.  This way, this one is out of the way,
> and we can just focus on the comments themselves.

 I agree about the comments.  However it's just struck me there's 
something weird about your code, see below.

> I am also adding a second patch, which shows the changes I made
> in this iteration.

 Thanks, that's useful.

> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> index 9a3c7fb..68ac858 100644
> --- a/gdb/mips-tdep.c
> +++ b/gdb/mips-tdep.c
> @@ -3009,6 +3009,37 @@ mips_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
>    return align_down (addr, 16);
>  }
>  
> +/* Implement the "push_dummy_call" gdbarch method.  */
> +
> +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;
> +  static gdb_byte nop_insn[] = { 0, 0, 0, 0 };
> +
> +  *bp_addr = sp;
> +  gdbarch_breakpoint_from_pc (gdbarch, bp_addr, &bp_len);

 You set bp_addr to SP here, so you rely on the stack pointer to have been 
implicitly adjusted down below the current frame by call_function_by_hand 
(or otherwise the breakpoint would occupy a valid stack frame location at 
this point -- which is dangerous if software breakpoints are used).  I 
find it a bit fragile, how about putting the breakpoint entirely in space 
allocated here?

 Then I think that you can avoid the call to gdbarch_breakpoint_from_pc, 
just as I noted in the microMIPS consideration.  We know that for an 
aligned stack location it's always going to return 4 in bp_len anyway and 
if we want to go below the top of the stack, then we sort of get into a 
chicken and egg problem -- we'll only know how far to go below SP once 
we've called this function, but to call it we need to know the location to 
ask about first.

 Fortunately getting this value exactly right does not really matter -- we 
just need enough space to cover the worst case.  So I suggest this 
instead:

  CORE_ADDR addr;

  addr = sp - sizeof (nop_insn);

so that the breakpoint itself is below the requested SP...

> +
> +  /* 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 nop instruction
> +     to prevent that from happening.  */
> +  write_memory (*bp_addr - bp_len, nop_insn, sizeof (nop_insn));
> +  sp = mips_frame_align (gdbarch, *bp_addr - 2 * bp_len);

... and then, consequently:

  write_memory (addr - sizeof (nop_insn), nop_insn, sizeof (nop_insn));
  sp = mips_frame_align (gdbarch, addr - sizeof (nop_insn));

  *bp_addr = addr;

(note that in your variation you write the NOP into space of bp_len bytes 
-- as noted above we know that this is going to be 4 anyway, but strictly 
speaking this is risky as it does not tie the number of bytes written to 
the space available).

 OK with these adjustments and sorry that I missed this previously.

  Maciej