Re: [gdbserver/rfa] CRIS/CRISv32 gdbserver support (part 2)

Daniel Jacobowitz
2005-02-24T20:49:17Z

On Tue, Feb 01, 2005 at 03:30:12PM +0100, Orjan Friberg wrote:
> For CRISv32, there might be an additional problem lurking in the shadows:
> we treat the watchpoint registers as global (i.e., only one process at a
> time can use them, and thus they are not saved/restored on context switches
> etc).

As I wrote in my other message, that's a pretty serious bug.

> Anyway, is this something you want resolved before I commit the
> CRIS/CRISv32 support? (Here's an updated ChangeLog entry and updated
> patches if not.)

Let's go ahead. The patch is mostly OK, but I have a last question or
two...

> 2005-02-01 Orjan Friberg <orjanf@axis.com>
>
> * linux-cris-low.c: New file with support for CRIS.
> * linux-crisv32-low.c: Ditto for CRISv32.
> * Makefile.in (SFILES): Add linux-cris-low.c, linux-crisv32-low.c.
> (clean): Add reg-cris.c and reg-crisv32.c.
> Add linux-cris-low.o, linux-crisv32-low.o, reg-cris.o, reg-cris.c,
> reg-crisv32.o, and reg-crisv32.c to make rules.
> * configure.srv: Add cris-*-linux* and crisv32-*-linux* to list of
> recognized targets.
>
> --
> Orjan Friberg
> Axis Communications
>

> static const unsigned long cris_breakpoint = 0xe938;
> #define cris_breakpoint_len 2
>
> static int
> cris_breakpoint_at (CORE_ADDR where)
> {
> unsigned long insn;
>
> (*the_target->read_memory) (where, (char *) &insn, 4);
> if (insn == cris_breakpoint)
> return 1;
>
> /* If necessary, recognize more trap instructions here. GDB only uses the
> one. */
> return 0;
> }

If the breakpoint length is two, why are you reading and testing four
bytes?

> void
> cris_write_data_breakpoint (int bp, unsigned long start, unsigned long end)

Please make new functions static where possible, which is pretty much
everywhere.

> int
> cris_stopped_by_watchpoint ()

And use prototypes - that's not a prototype, because of the missing
(void).

> struct regset_info target_regsets[] = {
> { PTRACE_GETREGS, PTRACE_SETREGS, sizeof (elf_gregset_t),
> GENERAL_REGS, cris_fill_gregset, cris_store_gregset },
> { 0, 0, -1, -1, NULL, NULL }
> };

Does the regset cover everything that that regmap covers, including
debug registers? The current support for both PEEKUSER and regsets is
purely for compatibility; it doesn't support getting some registers
from one and some from another. So if all crisv32 kernels support
PTRACE_GETREGS, your regmap is never getting used.

I have code around here somewhere to support targets that need both,
but it's gross.

--
Daniel Jacobowitz
CodeSourcery, LLC