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

[Pedro Alves <palves@redhat.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