This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [gdbserver/rfa] CRIS/CRISv32 gdbserver support (part 2)
- From: Daniel Jacobowitz <drow at false dot org>
- To: Orjan Friberg <orjan dot friberg at axis dot com>
- Cc: gdb-patches at sources dot redhat dot com
- Date: Thu, 12 May 2005 09:11:03 -0400
- Subject: Re: [gdbserver/rfa] CRIS/CRISv32 gdbserver support (part 2)
- References: <41E7969F.3010009@axis.com> <20050130045359.GC25185@nevyn.them.org> <41FF9274.7030700@axis.com> <20050224204906.GC11751@nevyn.them.org> <42835324.70905@axis.com>
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