From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 610 invoked by alias); 2 Feb 2009 20:01:17 -0000 Received: (qmail 32410 invoked by uid 22791); 2 Feb 2009 20:01:14 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from NaN.false.org (HELO nan.false.org) (208.75.86.248) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 02 Feb 2009 20:01:10 +0000 Received: from nan.false.org (localhost [127.0.0.1]) by nan.false.org (Postfix) with ESMTP id 68C39107F4; Mon, 2 Feb 2009 20:01:08 +0000 (GMT) Received: from caradoc.them.org (209.195.188.212.nauticom.net [209.195.188.212]) by nan.false.org (Postfix) with ESMTP id EEAB610781; Mon, 2 Feb 2009 20:01:07 +0000 (GMT) Received: from drow by caradoc.them.org with local (Exim 4.69) (envelope-from ) id 1LU4yh-0007xQ-Cd; Mon, 02 Feb 2009 15:01:07 -0500 Date: Mon, 02 Feb 2009 20:01:00 -0000 From: Daniel Jacobowitz To: Julian Brown Cc: gdb-patches@sourceware.org, pedro@codesourcery.com Subject: Re: [PATCH] Displaced stepping (non-stop debugging) support for ARM Linux Message-ID: <20090202200107.GA26459@caradoc.them.org> Mail-Followup-To: Julian Brown , gdb-patches@sourceware.org, pedro@codesourcery.com References: <20090120221355.46ac23e6@rex.config> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20090120221355.46ac23e6@rex.config> User-Agent: Mutt/1.5.17 (2008-05-11) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-02/txt/msg00037.txt.bz2 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 on the target for BL? 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 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 == +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