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

Daniel Jacobowitz
2009-02-02T20:01:17Z

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