This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA 4/5] New port: CR16: gdbserver
- From: Pedro Alves <palves at redhat dot com>
- To: Kaushik Phatak <Kaushik dot Phatak at kpitcummins dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Fri, 14 Dec 2012 18:47:10 +0000
- Subject: Re: [RFA 4/5] New port: CR16: gdbserver
- References: <C6CA53A2A46BA7469348BDBD663AB65845B2FF84@KCHJEXMB03.kpit.com>
On 10/04/2012 11:23 AM, Kaushik Phatak wrote:
> +
> +static void
> +cr16_collect_ptrace_register (struct regcache *regcache, int regno, char *buf)
> +{
> + unsigned long pc;
> +
> + memset (buf, 0, sizeof (long));
> + collect_register_by_name (regcache, "pc", &pc);
> + if(regno == PC_REGNUM)
> + {
> + pc = pc >> 1;
> + cr16_set_pc(regcache, pc);
> + }
> + collect_register (regcache, regno, buf);
> +}
Formatting is wrong here.
> +
> +static void
> +cr16_supply_ptrace_register (struct regcache *regcache,
> + int regno, const char *buf)
> +{
> + unsigned long pc;
> + supply_register (regcache, regno, buf );
> + collect_register_by_name (regcache, "pc", &pc);
> +
> + /* For PC, leftshift the output as only top 21 bits are stored
> + This will make the value human readable for the host */
> + if(regno == PC_REGNUM)
> + {
> + pc = pc << 1;
> + cr16_set_pc(regcache, pc);
> + }
Here too. But what really caught my eye was the shifts
and the cr16_set_pc calls. They surprised me. I've been staring at this
for a good 10 minutes, and I can't get my head around it. It doesn't
look like these functions are idempotent, which is a sign of things not
being right. Is the shift visible in GDB or not? Is this a ptrace quirk,
or an architecture quirk? If the latter, why isn't GDB itself, and
the cr16_set_pc cr16_get_pc routines in gdbserver handling this?
> + (*the_target->read_memory) (where, (unsigned char *) &insn,
> + cr16_breakpoint_len);
Most ports fail to do this, but the above may fail. That should be
checked here.
> + if (insn == cr16_breakpoint)
> + {
> + return 1;
> + }
Single-line statements get no {}'.
> +struct linux_target_ops the_low_target = {
'{' goes on start of next line. Several other places in the patch with that issue.
> +++ ./gdb-7.5_working/gdb/gdbserver/linux-low.c 2012-10-01 16:21:48.000000000 +0530
> @@ -4809,7 +4810,7 @@ linux_stopped_data_address (void)
> #if ! (defined(PT_TEXT_ADDR) \
> || defined(PT_DATA_ADDR) \
> || defined(PT_TEXT_END_ADDR))
> -#if defined(__mcoldfire__)
> +#if defined(__mcoldfire__) || (__CR16__)
> /* These should really be defined in the kernel's ptrace.h header. */
> #define PT_TEXT_ADDR 49*4
> #define PT_DATA_ADDR 50*4
Please rebase against GDB mainline. These should be defined in <asm/ptrace.h>
instead, and linux-low.c should be picking those up.
> diff -uprN gdb-7.5/gdb/regformats/reg-cr16.dat ./gdb-7.5_working/gdb/regformats/reg-cr16.dat
> --- gdb-7.5/gdb/regformats/reg-cr16.dat 1970-01-01 05:30:00.000000000 +0530
> +++ ./gdb-7.5_working/gdb/regformats/reg-cr16.dat 2012-09-13 14:45:02.000000000 +0530
> @@ -0,0 +1,18 @@
> +name:cr16
> +expedite:psr
Why only psr? That's surprising.
> +32:r0and1
> +32:r2and3
> +32:r4and5
> +32:r6and7
> +32:r8and9
> +32:r10and11
Eh. What's the rationale for this? Peeking at the GDB patch, I saw no pseudo
registers support.
--
Pedro Alves