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

[Daniel Jacobowitz <drow@false.org>]

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