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

Daniel Jacobowitz
2005-05-12T13:11:39Z

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