Re: [PATCH] Displaced stepping (non-stop debugging) support for ARM Linux

[Daniel Jacobowitz <drow at false dot org>]

On Tue, Jan 20, 2009 at 10:13:55PM +0000, Julian Brown wrote:
> OK to apply, or any comments?

General comments:

* Please make more of the functions static.

* More comments would be nice.  Some of the helper functions need
individual comments, and there needs to be an overview comment
explaining the structure.  For instance, "cleanup_* does X, copy_X
does Y".

* Why do you convert all register reads to fixed temporaries - is this
much simpler than detecting and replacing only PC references?  Or are
their other tricky cases?  This causes a lot of register reads and
writes that are not strictly required.

* If you reordered the cleanup and copy functions, you wouldn't need
all the static prototypes.

* What's the point of executing mov<cond> on the target for BL<cond>?
At that point it seems like we ought to skip the target step entirely;
just simulate the instruction.  We've already got a function to check
conditions (condition_true).

* Using arm_write_pc is a bit dodgy here; I don't think it's what we
want.  That function updates the CPSR based on a number of things
including symbol tables.  We know exactly what is supposed to happen
to CPSR for a given instruction and should honor it.  An example of
why this matters: people regularly get a blx in Cortex-M3 code by use
of bad libraries, untyped ELF symbols, or other such circumstances.
That blx had better update the CPSR even when we step over it.

* You've got FIXMEs.  Let's fix them rather than introduce bug
minefields, please.  If they're questions, I can probably answer them.

> +  /* FIXME: BLX immediate is probably broken!  */

How so?

> +static int
> +copy_dp_imm (unsigned long insn, struct regcache *regs,
> +	     struct displaced_step_closure *dsc)

What's "dp" mean?  Data-processing?

> +/* FIXME: This should depend on the arch version.  */
> +
> +static ULONGEST
> +modify_store_pc (ULONGEST pc)
> +{
> +  return pc + 4;
> +}

This one we might not be able to fix in current GDB but we can at
least expand the comment... if I remember right the +4 is correct for
everything since ARMv5 and most ARMv4?

> +/* Handle ldm/stm.  Doesn't handle any difficult cases (exception return,
> +   user-register transfer).  */

If we don't handle them we should detect them and fail noisily.

> +  /* ldm/stm is always emulated, because there are too many corner cases to
> +     deal with otherwise.  Implement as mov<cond> r0, #1, then do actual
> +     transfer in cleanup routine if condition passes.  FIXME: Non-priveleged
> +     transfers.  */
> +
> +  /* Hmm, this might not work, because of memory permissions differing for
> +     the debugger & the debugged program.  I wonder what to do about that?  */

Yes, we just can't emulate loads or stores.  Anything that could cause
an exception that won't be delayed till the next instruction, I think.

> +  if (!do_transfer)
> +    return;
> +
> +  /* FIXME: Implement non-priveleged transfers!  */
> +  gdb_assert (!dsc->u.block.user);
> +
> +  /* FIXME: Exception return.  */

This is not an internal error; it should not be a gdb_assert.  Instead
we should error().

> +static int
> +copy_svc (unsigned long insn, CORE_ADDR to, struct regcache *regs,
> +	  struct displaced_step_closure *dsc)
> +{
> +  CORE_ADDR from = dsc->insn_addr;
> +
> +  if (debug_displaced)
> +    fprintf_unfiltered (gdb_stdlog, "displaced: copying svc insn %.8lx\n",
> +			insn);
> +
> +  /* Preparation: tmp[0] <- to.
> +     Insn: unmodified svc.
> +     Cleanup: if (pc == <scratch>+4) pc <- insn_addr + 4;
> +	      else leave PC alone.  */

What about the saved PC?  Don't really want the OS service routine to
return to the scratchpad.

> +static void
> +cleanup_svc (struct regcache *regs, struct displaced_step_closure *dsc)
> +{
> +  CORE_ADDR from = dsc->insn_addr;
> +  CORE_ADDR to = dsc->tmp[0];
> +  ULONGEST pc;
> +
> +  /* Note: we want the real PC, so don't use displaced_read_reg here.  */
> +  regcache_cooked_read_unsigned (regs, ARM_PC_REGNUM, &pc);
> +
> +  if (pc == to + 4)
> +    displaced_write_reg (regs, dsc, ARM_PC_REGNUM, from + 4);
> +
> +  /* FIXME: What can we do about signal trampolines?  */
> +}

Maybe this is referring to the same question I asked above?

If so, I think you get to unwind and if you find the scratchpad,
update the saved PC.

> +static struct displaced_step_closure *
> +arm_catch_kernel_helper_return (CORE_ADDR from, CORE_ADDR to,
> +				struct regcache *regs)

Definitely would like a comment about what's going on here.

> +struct displaced_step_closure *
> +arm_displaced_step_copy_insn (struct gdbarch *gdbarch,
> +			      CORE_ADDR from, CORE_ADDR to,
> +			      struct regcache *regs)
> +{
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +  const size_t len = 4;
> +  gdb_byte *buf = xmalloc (len);
> +  struct displaced_step_closure *dsc;
> +  unsigned long insn;
> +  int i;
> +
> +  /* A linux-specific hack.  Detect when we've entered (inaccessible by GDB)
> +     kernel helpers, and stop at the return location.  */
> +  if (gdbarch_osabi (gdbarch) == GDB_OSABI_LINUX && from > 0xffff0000)
> +    {
> +      if (debug_displaced)
> +        fprintf_unfiltered (gdb_stdlog, "displaced: detected kernel helper "
> +			    "at %.8lx\n", (unsigned long) from);
> +
> +      dsc = arm_catch_kernel_helper_return (from, to, regs);
> +    }
> +  else
> +    {
> +      insn = read_memory_unsigned_integer (from, len);
> +
> +      if (debug_displaced)
> +	fprintf_unfiltered (gdb_stdlog, "displaced: stepping insn %.8lx "
> +			    "at %.8lx\n", insn, (unsigned long) from);
> +
> +      dsc = arm_process_displaced_insn (insn, from, to, regs);
> +    }

Can the Linux-specific hack go in arm-linux-tdep.c?  Shouldn't have to
make many functions global to do that.

> +  /* Poke modified instruction(s).  FIXME: Thumb, endianness.  */

I didn't see any endianness problems, but testing on BE is a good idea
anyway.  There ought to be an error for Thumb somewhere.

> @@ -3252,6 +4702,10 @@ arm_gdbarch_init (struct gdbarch_info in
>    /* On ARM targets char defaults to unsigned.  */
>    set_gdbarch_char_signed (gdbarch, 0);
>  
> +  /* Note: for displaced stepping, this includes the breakpoint, and one word
> +     of additional scratch space.  */
> +  set_gdbarch_max_insn_length (gdbarch, 12);
> +
>    /* This should be low enough for everything.  */
>    tdep->lowest_pc = 0x20;
>    tdep->jb_pc = -1;	/* Longjump support not enabled by default.  */

Does this relate to the size of modinsns, which has its own constant?

-- 
Daniel Jacobowitz
CodeSourcery