Re: [RFA 4/5] New port: CR16: gdbserver

Pedro Alves
2012-12-14T18:47:28Z

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