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

[Daniel Jacobowitz <drow@false.org>]

On Thu, May 12, 2005 at 02:59:16PM +0200, Orjan Friberg wrote:
> Daniel Jacobowitz wrote:
> >
> >Let's go ahead.  The patch is mostly OK, but I have a last question or
> >two...
> 
> Thanks for your comments, and apologies for the long delay.  (There are a 
> couple of questions at the end also.)

I didn't see any questions?

> Another thing: when removing cris_reinsert_addr, I got the same errors as 
> described in http://sourceware.org/ml/gdb/2005-01/msg00071.html (i.e. 
> "thread getmsg err: no event message for getmsg" and gdbserver getting a 
> SIGSEGV).  At the moment I have no idea what's going on, so as a temporary 
> solution I reinstated cris_reinsert_addr but with a FIXME.

Odd.  Does your hardware have real single step support, or implement
PTRACE_SINGLESTEP by managing breakpoints?  Other than that, I'm not
sure what could be wrong.

I've got more nits for you though.

> 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, cris_breakpoint_len);
>   if (insn == cris_breakpoint)
>     return 1;
> 
>   /* If necessary, recognize more trap instructions here.  GDB only uses the
>      one.  */
>   return 0;
> }

This is presumably going to work for you, but it's seriously untidy -
there's a hidden endianness dependency.  Can you see it?  Hint: the
types of cris_breakpoint and insn are wrong.

Same thing applies to linux-crisv32-low.c.

I just realized that these files will require an entry in
config/djgpp/fnchange.lst; their names are too similar.

> static void
> cris_write_data_breakpoint (int bp, unsigned long start, unsigned long end)
> {
>   switch (bp)
>     {
>     case 0:
>       supply_register_by_name("s3", &start);
>       supply_register_by_name("s4", &end);
>       break;

You were doing great on GNU formatting right up until here, and then
you lost it.  Everything from here down in this file needs to be
checked.  For instance:

> static int
> cris_insert_watchpoint(char type, CORE_ADDR addr, int len)

Space.

>   if (type < '2' || type > '4') {
>     /* Unsupported.  */
>     return 1;
>   }

Unnecessary braces; even if they were necessary they'd have to be on
their own lines and indented.

-- 
Daniel Jacobowitz
CodeSourcery, LLC