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

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

Joel,

> This patch switches the mips code to use the ON_STACK method
> for function calls instead of AT_SYMBOL, which we want to remove.

 Thanks for this work -- can you give me a reference to some background 
information as to why exactly we want to remove the AT_SYMBOL method?

> 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.

 For the record: the respective ABIs mandate that the stack is aligned to 
8 bytes for 32-bit targets and to 16 bytes for 64-bit targets.  However 
the user may have fiddled with SP, so I think it's better to stay safe 
and therefore I agree it's better if we prealign the stack and avoid 
crashing the debuggee in this context.

> 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?

 There's one fundamental problem with this change -- software breakpoints 
must not be used unconditionally, because some configurations do not 
expect or support them.  This affects remote JTAG targets and simulators, 
appropriate `Z0' packets should be used -- this is because JTAG uses SDBBP 
instructions rather than BREAK instructions -- the latters are ordinary 
instructions as far as JTAG debugging is considered and do not trap into 
the debug mode (this is so that you can debug an OS that uses BREAK 
instructions for user debugging); simulators may use yet other means, 
typically they don't use memory breakpoints at all (again, to allow users 
to debug their software within the simulator, e.g. the simulator runs 
Linux and the user debugs some userland app within that instance of 
Linux).

> 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.  */
> +
> +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};
> +
> +  *bp_addr = mips_frame_align (gdbarch, sp);
> +  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;
> +}
>  static CORE_ADDR
>  mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>  			   struct regcache *regcache, CORE_ADDR bp_addr,

 So essentially gdbarch_breakpoint_from_pc may only be used on targets 
known to use software breakpoints.

 Additionally once microMIPS support is in the above must execute as 
microMIPS code where appropriate to support processors that do not have 
the standard MIPS mode implemented -- no such processor currently exists, 
but the architecture spec explicitly permits pure-microMIPS 
implementations and some are expected to happen sooner or later.

 IOW the callee must return to an address that has the ISA bit set 
(that'll be the address of the breakpoint above ORed with 1) or otherwise 
SIGBUS may be thrown (hardware signals an Address Error exception if the 
ISA bit is clear).  Is that going to work with an update to the piece 
above like:

[...]
  if (APPROPRIATE)
    sp = make_compact_addr (sp);
  return sp;
}

(plus a corresponding update to the breakpoint address) or is more surgery 
required, especially to any generic code?  I guess not as I gather the 
return value will not only be used as the return address, but as the stack 
pointer as well, right?

 For simplicity I think:

#define APPROPRIATE is_micromips_addr (gdbarch, funaddr)

will do.

 Finally, I'd be happier if the unpredictability of the microMIPS 
breakpoint kind selection was avoided (there are two software and JTAG 
breakpoints each, BREAK16/BREAK32 and SDBBP16/SDBBP32, used by GDB 
like-for-like with 16-bit and 32-bit microMIPS instructions being 
replaced), so I suggest that the stack frame is preallocated and 
preinitialised along the lines of:

{
  gdb_byte *null_insn;
  CORE_ADDR new_sp;

  sp = mips_frame_align (gdbarch, sp);
  new_sp = mips_frame_align (gdbarch, sp - 2 * MIPS_INSN32_SIZE);
  null_insn = alloca (sp - new_sp);
  memset (null_insn, 0, sp - new_sp);
  write_memory (new_sp, null_insn, sp - new_sp);
[...]

-- that applies to remote targets that use `Z0' too as the expected 
breakpoint length will be encoded by GDB as appropriate in the request.

 I can address all these microMIPS issues if you like as I'm likely more 
familiar with that stuff, but please find a solution to the software 
breakpoint problem.

> @@ -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);

 So what if the stack pages are indeed not executable (their page entries 
have the XI aka Execute Inhibit bit set)?

  Maciej