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

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

[Kaushik Phatak]

Hi,
This patch adds gdbserver support for the National Semiconductor cr16 target.
This currently still has a limitation related to -fPIC usage and global variables.
However, I am hoping that major portions of this patch which have been tested 
and are working well get accepted here.

Regards,
Kaushik

2012-10-04 Kaushik Phatak  <kaushik.phatak@kpitcummins.com>
	gdb/Changelog
        * regformats/reg-cr16.dat: New.

      gdb/gdbserver/Changelog
        * Makefile.in (clean): Remove reg-cr16.c.
        (linux-cr16-low.o, reg-cr16.o): New rules.
        * configure.srv: Add support for cr16-*-uclinux.
        * linux-cr16-low.c: New.
        * linux-low.c (PT_TEXT_ADDR, PT_DATA_ADDR, PT_TEXT_END_ADDR): Define.


diff -uprN gdb-7.5/gdb/gdbserver/configure.srv ./gdb-7.5_working/gdb/gdbserver/configure.srv
--- gdb-7.5/gdb/gdbserver/configure.srv	2012-05-31 01:13:15.000000000 +0530
+++ ./gdb-7.5_working/gdb/gdbserver/configure.srv	2012-10-04 14:18:33.000000000 +0530
@@ -74,6 +74,11 @@ case "${target}" in
 			srv_linux_usrregs=yes
 			srv_linux_thread_db=yes
 			;;
+  cr16-*-uclinux*)	srv_regobj=reg-cr16.o
+			srv_tgtobj="linux-low.o linux-cr16-low.o"
+			srv_linux_usrregs=yes
+			srv_linux_thread_db=yes
+			;;
   crisv32-*-linux*)	srv_regobj=reg-crisv32.o
 			srv_tgtobj="linux-low.o linux-osdata.o linux-crisv32-low.o linux-procfs.o"
 			srv_tgtobj="${srv_tgtobj} linux-ptrace.o"
diff -uprN gdb-7.5/gdb/gdbserver/linux-cr16-low.c ./gdb-7.5_working/gdb/gdbserver/linux-cr16-low.c
--- gdb-7.5/gdb/gdbserver/linux-cr16-low.c	1970-01-01 05:30:00.000000000 +0530
+++ ./gdb-7.5_working/gdb/gdbserver/linux-cr16-low.c	2012-10-01 15:28:08.000000000 +0530
@@ -0,0 +1,159 @@
+/* GNU/Linux/CR16 specific low level interface, for the remote server for GDB.
+
+   Copyright (C) 2010-2012 Free Software Foundation, Inc.
+   Contributed by Kaushik Phatak (kaushik.pahatk@kpitcummins.com)
+   KPIT Cummins Infosystems Limited, Pune India.   
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "server.h"
+#include "linux-low.h"
+#include <sys/ptrace.h>
+
+/* Defined in auto-generated file reg-cr16.c.  */
+void init_registers_cr16 (void);
+
+/* CR16C  */
+/* Locations need to match <include/asm/arch/ptrace.h>.  */
+#define cr16_num_regs 16
+#define PC_REGNUM 11
+
+static int cr16_regmap[] = {
+ 0,	4,	8,	12,
+ 16,	20,	24,	28,
+ 32,	36,	40,	44,
+ 48,	52,	56,	60
+};
+
+extern int debug_threads;
+static int
+cr16_cannot_store_register (int regno)
+{
+  if (cr16_regmap[regno] == -1)
+    return 1;
+
+  return (regno >= cr16_num_regs);
+}
+
+static int
+cr16_cannot_fetch_register (int regno)
+{
+  if (cr16_regmap[regno] == -1)
+    return 1;
+
+  return (regno >= cr16_num_regs);
+}
+
+static void
+cr16_set_pc (struct regcache *regcache, CORE_ADDR pc)
+{
+  unsigned long newpc = pc;
+  supply_register_by_name (regcache, "pc", &newpc);
+}
+
+static CORE_ADDR
+cr16_get_pc (struct regcache *regcache)
+{
+  unsigned long pc;
+  collect_register_by_name (regcache, "pc", &pc);
+  return pc;
+}
+ 
+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);
+}
+
+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);
+  }
+
+}
+
+static const unsigned short cr16_breakpoint = 0xc700;
+#define cr16_breakpoint_len 2
+
+static int
+cr16_breakpoint_at (CORE_ADDR where)
+{
+  unsigned short insn;
+
+  (*the_target->read_memory) (where, (unsigned char *) &insn,
+			      cr16_breakpoint_len);
+  if (insn == cr16_breakpoint)
+  {
+    return 1;
+  }
+  /* If necessary, recognize more trap instructions here.  GDB only uses the
+     one.  */
+  return 0;
+}
+
+/* We only place breakpoints in empty marker functions, and thread locking
+   is outside of the function.  So rather than importing software single-step,
+   we can just run until exit.  */
+static CORE_ADDR
+cr16_reinsert_addr (void)
+{
+  struct regcache *regcache = get_thread_regcache (current_inferior, 1);
+  unsigned long pc;
+  /* R14/Ra is return address register  */
+  collect_register_by_name (regcache, "ra", &pc);
+  return pc;
+}
+struct linux_target_ops the_low_target = {
+  init_registers_cr16,
+  cr16_num_regs,
+  cr16_regmap,
+  NULL,
+  cr16_cannot_fetch_register,
+  cr16_cannot_store_register,
+  NULL,
+  cr16_get_pc,
+  cr16_set_pc,
+  (const unsigned char *) &cr16_breakpoint,
+  cr16_breakpoint_len,
+  cr16_reinsert_addr,
+  0,
+  cr16_breakpoint_at,
+  0,
+  0,
+  0,
+  0,
+  cr16_collect_ptrace_register,
+  cr16_supply_ptrace_register,
+};
diff -uprN gdb-7.5/gdb/gdbserver/linux-low.c ./gdb-7.5_working/gdb/gdbserver/linux-low.c
--- gdb-7.5/gdb/gdbserver/linux-low.c	2012-07-07 17:43:57.000000000 +0530
+++ ./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
diff -uprN gdb-7.5/gdb/gdbserver/Makefile.in ./gdb-7.5_working/gdb/gdbserver/Makefile.in
--- gdb-7.5/gdb/gdbserver/Makefile.in	2012-07-02 20:59:38.000000000 +0530
+++ ./gdb-7.5_working/gdb/gdbserver/Makefile.in	2012-09-13 14:42:51.000000000 +0530
@@ -126,6 +126,7 @@ SFILES=	$(srcdir)/gdbreplay.c $(srcdir)/
 	$(srcdir)/remote-utils.c $(srcdir)/server.c $(srcdir)/target.c \
 	$(srcdir)/thread-db.c $(srcdir)/utils.c \
 	$(srcdir)/linux-arm-low.c $(srcdir)/linux-bfin-low.c \
+	$(srcdir)/linux-cr16-low.c \
 	$(srcdir)/linux-cris-low.c $(srcdir)/linux-crisv32-low.c \
 	${srcdir}/i386-low.c $(srcdir)/i387-fp.c \
 	$(srcdir)/linux-ia64-low.c $(srcdir)/linux-low.c \
@@ -307,7 +308,7 @@ clean:
 	rm -f $(IPA_LIB)
 	rm -f reg-arm.c reg-bfin.c i386.c reg-ia64.c reg-m32r.c reg-m68k.c
 	rm -f reg-sh.c reg-sparc.c reg-spu.c amd64.c i386-linux.c
-	rm -f reg-cris.c reg-crisv32.c amd64-linux.c reg-xtensa.c
+	rm -f reg-cris.c reg-cr16.c reg-crisv32.c amd64-linux.c reg-xtensa.c
 	rm -f arm-with-iwmmxt.c
 	rm -f arm-with-vfpv2.c arm-with-vfpv3.c arm-with-neon.c
 	rm -f mips-linux.c mips64-linux.c
@@ -546,6 +547,7 @@ linux-low.o: linux-low.c $(linux_low_h) 
 linux-arm-low.o: linux-arm-low.c $(linux_low_h) $(server_h) \
 	$(gdb_proc_service_h)
 linux-bfin-low.o: linux-bfin-low.c $(linux_low_h) $(server_h)
+linux-cr16-low.o: linux-cr16-low.c $(linux_low_h) $(server_h)
 linux-cris-low.o: linux-cris-low.c $(linux_low_h) $(server_h)
 linux-crisv32-low.o: linux-crisv32-low.c $(linux_low_h) $(server_h)
 linux-ia64-low.o: linux-ia64-low.c $(linux_low_h) $(server_h)
@@ -593,6 +595,9 @@ arm-with-neon.c : $(srcdir)/../regformat
 reg-bfin.o : reg-bfin.c $(regdef_h)
 reg-bfin.c : $(srcdir)/../regformats/reg-bfin.dat $(regdat_sh)
 	$(SHELL) $(regdat_sh) $(srcdir)/../regformats/reg-bfin.dat reg-bfin.c
+reg-cr16.o : reg-cr16.c $(regdef_h)
+reg-cr16.c : $(srcdir)/../regformats/reg-cr16.dat $(regdat_sh)
+	$(SHELL) $(regdat_sh) $(srcdir)/../regformats/reg-cr16.dat reg-cr16.c
 reg-cris.o : reg-cris.c $(regdef_h)
 reg-cris.c : $(srcdir)/../regformats/reg-cris.dat $(regdat_sh)
 	$(SHELL) $(regdat_sh) $(srcdir)/../regformats/reg-cris.dat reg-cris.c
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
+32:r0and1
+32:r2and3
+32:r4and5
+32:r6and7
+32:r8and9
+32:r10and11
+32:r12
+32:r13
+32:ra
+16:psr
+16:pad
+32:pc
+32:orig_r0and1
+32:intbase
+32:usp
+32:cfg

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

[Pedro Alves]

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

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

[Kaushik Phatak]

Hi Pedro,
Thanks for the detailed review.

I will take care of all the formatting and re-base this with GDB mainline when
I resubmit the patch.

To answer some of your points,
> +cr16_supply_ptrace_register (struct regcache *regcache,
> 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?

This is an architecture quirk as the PC is 24 bits wide and only top 23 bits
can be read. The last bit is always zero, so user has to left shift to get
the correct value. The 'supply_ptrace_register' seemed like the correct place
to make these manipulations similar to s390 or the ppc ports.
Manipulations in the get_pc and set_pc calls did not seem to get the 
desired effect while reading PC from the target.

> +32:r10and11
> Eh.  What's the rationale for this? Peeking at the GDB patch, I saw no
> pseudo registers support.
This is the way the kernel port defines the PT_REGS structure. This allows for
faster access as register pair move reg_names (movd rp,rp) instruction is 
supported. However, we needed individual registers to be viewed under gdb.

> +expedite:psr
> Why only psr?  That's surprising.
In case I add any other registers, then the offset of that register is 
incorrect with respect to the "reg_names", and the size does not match 
which is returned by "cr16_register_type".
For example, if I add 
expedite:psr,ra
Then 'ra' is register number 8, and has size 32 bits as per reg-cr16.dat, 
however,for number 8, I have r8 in "reg_names" (cr16-tdep.c) which has 
size 16 bits.
When gdb requests for the expedite registers, it gets data and throws error,
"Badly formatted packet" -> I guess this is expected, as it returns 32 bits
for 'ra' as per reg-cr16.dat, but it expects only 16 bits for 'r8'.
I know this is bit of a hack, however to get the gdbserver running on the
kernel and keeping it in sync with the host, this seemed my best bet.

>> +  (*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.
I have probably borrowed this from cris port. Do I need to perform additional
here? Please let me know if there is any other port I can refer to.

Please let me know if the above justifications are ok.

Thanks & Best Regards,
Kaushik

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

[Pedro Alves]

On 12/28/2012 04:41 AM, Kaushik Phatak wrote:
> Hi Pedro,
> Thanks for the detailed review.
> 
> I will take care of all the formatting and re-base this with GDB mainline when
> I resubmit the patch.
> 
> To answer some of your points,
>> +cr16_supply_ptrace_register (struct regcache *regcache,
>> 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?
> 
> This is an architecture quirk as the PC is 24 bits wide and only top 23 bits
> can be read. The last bit is always zero, so user has to left shift to get
> the correct value. The 'supply_ptrace_register' seemed like the correct place
> to make these manipulations similar to s390 or the ppc ports.
> Manipulations in the get_pc and set_pc calls did not seem to get the 
> desired effect while reading PC from the target.

Okay.

> 
>> +32:r10and11
>> Eh.  What's the rationale for this? Peeking at the GDB patch, I saw no
>> pseudo registers support.
> This is the way the kernel port defines the PT_REGS structure. This allows for
> faster access as register pair move reg_names (movd rp,rp) instruction is 
> supported. However, we needed individual registers to be viewed under gdb.

But "r10and11" is not defined as a 32-bit (pseudo) register anywhere
I could see.   Wouldn't writing

...
 16:r10
 16:r11
...

work?

Then collect/supply_register_by_name could use "r11" if it wanted,
and it work out the same?

But most importantly, I see in gdbserver:

+#define cr16_num_regs 16
+#define PC_REGNUM 11
+
+static int cr16_regmap[] = {
+ 0,	4,	8,	12,
+ 16,	20,	24,	28,
+ 32,	36,	40,	44,
+ 48,	52,	56,	60
+};

while in GDB you have:

+/* Number of registers available for Linux targets  */
+#define CR16_LINUX_NUM_REGS  21
+
+/* The breakpoint instruction used by uClinux target  */
+static const gdb_byte breakpoint_uclinux[] = { 0xC7, 0x00 };
+
+static const char *const reg_names[] =
+{
+  "r0",
+  "r1",
+  "r2",
+  "r3",
+  "r4",
+  "r5",
+  "r6",
+  "r7",
+  "r8",
+  "r9",
+  "r10",
+  "r11",
+  "r12",
+  "r13",
+  "ra",
+  "psr",
+  "pc",
+  "r0r1_orig",
+  "intbase",
+  "usp",
+  "cfg"
+};
+

So if GDB would happen to request register number 7
with the 'p' packet (GDBserver doesn't support that packet
today, but it serves to show the design problem), GDBserver
would return the wrong register.

> 
>> +expedite:psr
>> Why only psr?  That's surprising.
> In case I add any other registers, then the offset of that register is 
> incorrect with respect to the "reg_names", and the size does not match 
> which is returned by "cr16_register_type".
> For example, if I add 
> expedite:psr,ra
> Then 'ra' is register number 8, and has size 32 bits as per reg-cr16.dat, 
> however,for number 8, I have r8 in "reg_names" (cr16-tdep.c) which has 
> size 16 bits.
> When gdb requests for the expedite registers, it gets data and throws error,
> "Badly formatted packet" -> I guess this is expected, as it returns 32 bits
> for 'ra' as per reg-cr16.dat, but it expects only 16 bits for 'r8'.
> I know this is bit of a hack, however to get the gdbserver running on the
> kernel and keeping it in sync with the host, this seemed my best bet.

Exactly, a manifestation of the above issue.

No, this needs to be fixed.  The .dat registers must agree with GDB's remote
register set.  Once that is fixed, you'll be able to put more registers
in the "expedite" set, which you do want -- you'll want to put there
the registers that gdb will need to fetch anyway at each single-step,
in order to reduce round-trip latency.  Usually, the PC, and stack pointer
registers and often the frame pointer too.

-- 
Pedro Alves

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

[Kaushik Phatak]

Hi Pedro,
Thanks for your feedback.

> No, this needs to be fixed.  The .dat registers must agree with GDB's remote
> register set.  Once that is fixed, you'll be able to put more registers
> in the "expedite" set

I am working on these points. From what I see, my cr16_regmap needs to be
4-byte aligned so that ptrace calls within linux-low.c can use these without
causing any I/O errors. The updated reg.dat file would not match in offset
to cr16_regmap, but would match gdb.
Some additional code would be needed to handle the padding and packing through 
ptrace in the kernel. This would allow the correct register number to be 
accessed which would match gdb as well as reg-cr16.dat.
Let me know if you have any thoughts on this.

Regards,
Kaushik

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

[Pedro Alves]

On 01/22/2013 01:42 PM, Kaushik Phatak wrote:
> Hi Pedro,
> Thanks for your feedback.
> 
>> No, this needs to be fixed.  The .dat registers must agree with GDB's remote
>> register set.  Once that is fixed, you'll be able to put more registers
>> in the "expedite" set
> 
> I am working on these points. From what I see, my cr16_regmap needs to be
> 4-byte aligned so that ptrace calls within linux-low.c can use these without
> causing any I/O errors. The updated reg.dat file would not match in offset
> to cr16_regmap, but would match gdb.
> Some additional code would be needed to handle the padding and packing through 
> ptrace in the kernel. 

You mean additional code in gdbserver, or in the kernel?  I'd think the
former.

> This would allow the correct register number to be
> accessed which would match gdb as well as reg-cr16.dat.
> Let me know if you have any thoughts on this.

The alternative would be to go all the other way around
-- always expose the registers as pairs in the remote protocol,
and then implement the user visible non-paired registers in
GDB as pseudo registers (and hide the pairs).  IOW, your .dat
file would stay the same, and gdbserver wouldn't change.  GDB's
core cr16 register numbers would be decoupled from the RSP
register set.  It sounds a little uglier to me though.

-- 
Pedro Alves

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

[Kaushik Phatak]

> You mean additional code in gdbserver, or in the kernel?  I'd think the
> former.
Yes, the code changes would be for gdbserver and gdb. 
I would prefer to keep my kernel code changes to a minimal.

> always expose the registers as pairs in the remote protocol,
> and then implement the user visible non-paired registers in
> GDB as pseudo registers (and hide the pairs).  IOW, your .dat
> file would stay the same, and gdbserver wouldn't change.  GDB's
> core cr16 register numbers would be decoupled from the RSP
> register set.
Yes, I agree. Disturbing the .dat file is causing issues in the 
remote protocol. So, using pseudo registers within gdb would be a 
better way to proceed as long as I can get the register numbers to 
match my expedite register numbers.

Thanks,
Kaushik

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

[Pedro Alves]

On 01/23/2013 12:50 PM, Kaushik Phatak wrote:
>> You mean additional code in gdbserver, or in the kernel?  I'd think the
>> former.
> Yes, the code changes would be for gdbserver and gdb. 
> I would prefer to keep my kernel code changes to a minimal.
> 
>> always expose the registers as pairs in the remote protocol,
>> and then implement the user visible non-paired registers in
>> GDB as pseudo registers (and hide the pairs).  IOW, your .dat
>> file would stay the same, and gdbserver wouldn't change.  GDB's
>> core cr16 register numbers would be decoupled from the RSP
>> register set.
> Yes, I agree. Disturbing the .dat file is causing issues in the 
> remote protocol. 

What issues?  Can you be more specific?

> So, using pseudo registers within gdb would be a 
> better way to proceed as long as I can get the register numbers to 
> match my expedite register numbers.

-- 
Pedro Alves

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

[Kaushik Phatak]

>> Yes, I agree. Disturbing the .dat file is causing issues in the 
>> remote protocol. 
>What issues?  Can you be more specific?

The kernel defines only 15 registers, r0 to r11 as pairs, (total 6) then
the other 9 32-bit registers r12, r13 and special registers ra,pc,usp,psr,
pad,intbase, and r0r1_orig.
If I want the .dat to match the register numbers in gdb, then my .dat would
look like this,
{{{
name:cr16
expedite:psr,pc,usp
16:r0
16:r1
16:r2
16:r3
16:r4
16:r5
16:r6
16:r7
16:r8
16:r9
16:r10
16:r11
32:r12
32:r13
32:ra
32:psr
32:pc
32:r0r1_orig
32:intbase
32:usp
32:cfg
}}}

This is consistent with gdb. However, this way my register count goes 
to 21 which is way above what my kernel can handle causing
it to error out. I could add special to handle this at kernel, but it
will slow it down quite a bit. Additionally, the ptrace in kernel gets
its data from a 'get_reg' function which accepts specific regnums and
it will tricky to disturb this.
 
Regards,
Kaushik

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

[Pedro Alves]

On 01/23/2013 02:04 PM, Kaushik Phatak wrote:
>>> Yes, I agree. Disturbing the .dat file is causing issues in the 
>>> remote protocol. 
>> What issues?  Can you be more specific?
> 
> The kernel defines only 15 registers, r0 to r11 as pairs, (total 6) then
> the other 9 32-bit registers r12, r13 and special registers ra,pc,usp,psr,
> pad,intbase, and r0r1_orig.
> If I want the .dat to match the register numbers in gdb, then my .dat would
> look like this,
> {{{
> name:cr16
> expedite:psr,pc,usp
> 16:r0
> 16:r1
> 16:r2
> 16:r3
> 16:r4
> 16:r5
> 16:r6
> 16:r7
> 16:r8
> 16:r9
> 16:r10
> 16:r11
> 32:r12
> 32:r13
> 32:ra
> 32:psr
> 32:pc
> 32:r0r1_orig
> 32:intbase
> 32:usp
> 32:cfg
> }}}
> 
> This is consistent with gdb. However, this way my register count goes 
> to 21 which is way above what my kernel can handle causing
> it to error out. 

This is not an issue with the remote protocol.  It's just an issue of
cr16 gdbserver implementation.
So you'd decouple the remote protocol register numbers from the
ptrace numbers in gdbserver.  The .dat file is about gdbserver's
register cache, which maps the RSP numbering.  IOW, nothing prevents
make gdbserver map the RSP numbering to ptrace numbering when
marshalling things to/from ptrace.  This is what cr16_regmap is about
afterall, except that you have the register pairs peculiarity, which
may need more code to support.  I was suspecting this confusion,
thus my prod.  ;-)

> I could add special to handle this at kernel, but it
> will slow it down quite a bit. 

I don't think any single ptrace call would be slowed down
by any significant amount.  It's the fact that you'd
require double the ptrace calls that could add up.  In any
case, the right fix for that is PTRACE_GETREGS.  I'd hope new
kernel ports wouldn't go upstream without that nowadays, though...

A keep-backward-compatibility-with-whats-in-the-field-already
card would be a better play here.
What style of RSP register numbering do bare metal stubs running
on cr16 use?  Do they also do the register pairing trick?

I don't really care whether the pairs end up in the RSP or
not.  If you prefer that path, it's okay with me, provided
RSP numbering ends up correctly decoupled on the GDB side.

-- 
Pedro Alves

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

[Kaushik Phatak]

[-- Attachment #1: Type: text/plain, Size: 9669 bytes --]

Hi,
Please find below an updated patch for the gdbserver port of the CR16 target.
This has been updated from my previous attempts of the same,
http://sourceware.org/ml/gdb-patches/2012-10/msg00064.html

The port has been updated based on feedback provided by Pedro in discussions
earlier this year in Jan,
http://sourceware.org/ml/gdb-patches/2013-01/msg00552.html

In addition to the comments, the port has been base-lined against latest sources
which needs the "initialize_low_arch" function.

The below gdbserver port has following changes,
1. Remove register pair access over remote protocol in the .dat file.
This allows for matching of the register numbers in the gdbserver with those
in linux-tdep.c. This was done by simplifying the PTRACE_GETREGS inside kernel
to read single registers.
2. Remove support for "r0r1_orig" register. This is a kernel internal register
and need not be user visible.
3. Set cr16_num_regs to 20 and PC_REGNUM to 16. This will now match the gdb port.
4. Add registers pc,usp,ra to the "expedite" set. 
5. Move the ptrace macro defines like PT_TEXT_ADDR from linux-low.c to the kernel's
ptrace.h

An updated gdb port for the same is also posted for this.

2013-06-19 Kaushik Phatak  <kaushik.phatak@kpitcummins.com>
	gdb/Changelog
	* regformats/reg-cr16.dat: New.

	gdb/gdbserver/Changelog
	* Makefile.in (clean): Remove reg-cr16.c.
	(linux-cr16-low.o, reg-cr16.o): New rules.
	* configure.srv: Add support for cr16-*-uclinux.
	* linux-cr16-low.c: New.

Index: gdb/gdbserver/Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/Makefile.in,v
retrieving revision 1.153
diff -u -r1.153 Makefile.in
--- gdb/gdbserver/Makefile.in	7 Jun 2013 10:46:58 -0000	1.153
+++ gdb/gdbserver/Makefile.in	19 Jun 2013 12:05:16 -0000
@@ -138,6 +138,7 @@
 	$(srcdir)/remote-utils.c $(srcdir)/server.c $(srcdir)/target.c \
 	$(srcdir)/thread-db.c $(srcdir)/utils.c \
 	$(srcdir)/linux-arm-low.c $(srcdir)/linux-bfin-low.c \
+	$(srcdir)/linux-cr16-low.c \
 	$(srcdir)/linux-cris-low.c $(srcdir)/linux-crisv32-low.c \
 	${srcdir}/i386-low.c $(srcdir)/i387-fp.c \
 	$(srcdir)/linux-ia64-low.c $(srcdir)/linux-low.c \
@@ -319,7 +320,7 @@
 	rm -f aarch64.c
 	rm -f reg-arm.c reg-bfin.c i386.c reg-ia64.c reg-m32r.c reg-m68k.c
 	rm -f reg-sh.c reg-sparc.c reg-spu.c amd64.c i386-linux.c
-	rm -f reg-cris.c reg-crisv32.c amd64-linux.c reg-xtensa.c
+	rm -f reg-cr16.c reg-cris.c reg-crisv32.c amd64-linux.c reg-xtensa.c
 	rm -f reg-tilegx.c reg-tilegx32.c
 	rm -f arm-with-iwmmxt.c
 	rm -f arm-with-vfpv2.c arm-with-vfpv3.c arm-with-neon.c
@@ -584,6 +585,8 @@
 	$(SHELL) $(regdat_sh) $(srcdir)/../regformats/arm-with-neon.dat arm-with-neon.c
 reg-bfin.c : $(srcdir)/../regformats/reg-bfin.dat $(regdat_sh)
 	$(SHELL) $(regdat_sh) $(srcdir)/../regformats/reg-bfin.dat reg-bfin.c
+reg-cr16.c : $(srcdir)/../regformats/reg-cr16.dat $(regdat_sh)
+	$(SHELL) $(regdat_sh) $(srcdir)/../regformats/reg-cr16.dat reg-cr16.c
 reg-cris.c : $(srcdir)/../regformats/reg-cris.dat $(regdat_sh)
 	$(SHELL) $(regdat_sh) $(srcdir)/../regformats/reg-cris.dat reg-cris.c
 reg-crisv32.c : $(srcdir)/../regformats/reg-crisv32.dat $(regdat_sh)
Index: gdb/gdbserver/configure.srv
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/configure.srv,v
retrieving revision 1.76
diff -u -r1.76 configure.srv
--- gdb/gdbserver/configure.srv	28 May 2013 10:41:17 -0000	1.76
+++ gdb/gdbserver/configure.srv	19 Jun 2013 12:05:16 -0000
@@ -88,6 +88,11 @@
 			srv_linux_usrregs=yes
 			srv_linux_thread_db=yes
 			;;
+  cr16-*-uclinux)	srv_regobj=reg-cr16.o
+			srv_tgtobj="linux-low.o linux-cr16-low.o"
+			srv_linux_usrregs=yes
+			srv_linux_thread_db=yes
+			;;
   crisv32-*-linux*)	srv_regobj=reg-crisv32.o
 			srv_tgtobj="linux-low.o linux-osdata.o linux-crisv32-low.o linux-procfs.o"
 			srv_tgtobj="${srv_tgtobj} linux-ptrace.o"
Index: gdb/gdbserver/cr16-linux-tdep.c
===================================================================
RCS file: gdb/gdbserver/linux-cr16-tdep.c
diff -N gdb/gdbserver/linux-cr16-tdep.c
--- /dev/null	2013-05-10 19:36:04.372328500 +0530
+++ ./gdb/gdbserver/linux-cr16-low.c	2013-06-19 17:44:04.000000000 +0530
@@ -0,0 +1,190 @@
+/* GNU/Linux/CR16 specific low level interface, for the remote server for GDB.
+
+   Copyright (C) 2012-2013 Free Software Foundation, Inc.
+   Contributed by Kaushik Phatak (kaushik.phatak@kpitcummins.com)
+   KPIT Cummins Infosystems Limited, Pune India. 
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "server.h"
+#include "linux-low.h"
+#include <sys/ptrace.h>
+
+/* Defined in auto-generated file reg-cr16.c.  */
+void init_registers_cr16 (void);
+extern const struct target_desc *tdesc_cr16;
+
+/* CR16C  */
+#define cr16_num_regs 20
+#define PC_REGNUM 16
+
+/* Locations need to match <include/asm/arch/ptrace.h>.  */
+static int cr16_regmap[] = {
+  0,   2,  4,  6,  8,
+  10, 12, 14, 16, 18,
+  20, 22, 24, 28, 32,
+  36, 40, 44, 48, 52
+};
+
+static int
+cr16_cannot_store_register (int regno)
+{
+  if (cr16_regmap[regno] == -1)
+    return 1;
+
+  return (regno >= cr16_num_regs);
+}
+
+static int
+cr16_cannot_fetch_register (int regno)
+{
+  if (cr16_regmap[regno] == -1)
+    return 1;
+
+  return (regno >= cr16_num_regs);
+}
+
+static void
+cr16_set_pc (struct regcache *regcache, CORE_ADDR pc)
+{
+  unsigned long newpc = pc;
+
+  supply_register_by_name (regcache, "pc", &newpc);
+}
+
+static CORE_ADDR
+cr16_get_pc (struct regcache *regcache)
+{
+  unsigned long pc;
+
+  collect_register_by_name (regcache, "pc", &pc);
+  return pc;
+}
+
+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);
+}
+
+static void
+cr16_supply_ptrace_register (struct regcache *regcache,
+			     int regno, const char *buf)
+{
+  unsigned long pc;
+  int size = register_size (regcache->tdesc, regno);
+
+  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);
+    }
+}
+
+static const unsigned short cr16_breakpoint = 0xc700;
+#define cr16_breakpoint_len 2
+
+static int
+cr16_breakpoint_at (CORE_ADDR where)
+{
+  unsigned short insn;
+
+  (*the_target->read_memory) (where, (unsigned char *) &insn,
+			      cr16_breakpoint_len);
+  if (insn == cr16_breakpoint)
+    return 1;
+  /* If necessary, recognize more trap instructions here.  GDB only
+     uses the one.  */
+  return 0;
+}
+
+/* We only place breakpoints in empty marker functions, and thread locking
+   is outside of the function.  So rather than importing software single-step,
+   we can just run until exit.  */
+static CORE_ADDR
+cr16_reinsert_addr (void)
+{
+  struct regcache *regcache = get_thread_regcache (current_inferior, 1);
+  unsigned long pc;
+
+  /* R14/Ra is return address register  */
+  collect_register_by_name (regcache, "ra", &pc);
+  return pc;
+}
+
+static void
+cr16_arch_setup (void)
+{
+  current_process ()->tdesc = tdesc_cr16;
+}
+
+static struct usrregs_info cr16_usrregs_info =
+  {
+    cr16_num_regs,
+    cr16_regmap,
+  };
+
+static struct regs_info regs_info =
+  {
+    NULL, /* regset_bitmap */
+    &cr16_usrregs_info,
+  };
+
+static const struct regs_info *
+cr16_regs_info (void)
+{
+  return &regs_info;
+}
+
+struct linux_target_ops the_low_target = {
+  cr16_arch_setup,
+  cr16_regs_info,
+  cr16_cannot_fetch_register,
+  cr16_cannot_store_register,
+  NULL,
+  cr16_get_pc,
+  cr16_set_pc,
+  (const unsigned char *) &cr16_breakpoint,
+  cr16_breakpoint_len,
+  cr16_reinsert_addr,
+  0,
+  cr16_breakpoint_at,
+  0,
+  0,
+  0,
+  0,
+  cr16_collect_ptrace_register,
+  cr16_supply_ptrace_register
+};
+
+void
+initialize_low_arch (void)
+{
+  init_registers_cr16 ();
+}
Index: gdb/regformats/reg-cr16.dat
===================================================================
RCS file: gdb/regformats/reg-cr16.dat
diff -N gdb/gdbserver/reg-cr16.dat
--- /dev/null	2013-05-10 19:36:04.372328500 +0530
+++ ./gdb/regformats/reg-cr16.dat	2013-06-18 15:10:23.000000000 +0530
@@ -0,0 +1,22 @@
+name:cr16
+expedite:psr,pc,usp,ra
+16:r0
+16:r1
+16:r2
+16:r3
+16:r4
+16:r5
+16:r6
+16:r7
+16:r8
+16:r9
+16:r10
+16:r11
+32:r12
+32:r13
+32:ra
+32:psr
+32:pc
+32:intbase
+32:usp
+32:cfg	

[-- Attachment #2: cr16_gdbserver.diff --]
[-- Type: application/octet-stream, Size: 7864 bytes --]

Index: gdb/gdbserver/Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/Makefile.in,v
retrieving revision 1.153
diff -u -r1.153 Makefile.in
--- gdb/gdbserver/Makefile.in	7 Jun 2013 10:46:58 -0000	1.153
+++ gdb/gdbserver/Makefile.in	19 Jun 2013 12:05:16 -0000
@@ -138,6 +138,7 @@
 	$(srcdir)/remote-utils.c $(srcdir)/server.c $(srcdir)/target.c \
 	$(srcdir)/thread-db.c $(srcdir)/utils.c \
 	$(srcdir)/linux-arm-low.c $(srcdir)/linux-bfin-low.c \
+	$(srcdir)/linux-cr16-low.c \
 	$(srcdir)/linux-cris-low.c $(srcdir)/linux-crisv32-low.c \
 	${srcdir}/i386-low.c $(srcdir)/i387-fp.c \
 	$(srcdir)/linux-ia64-low.c $(srcdir)/linux-low.c \
@@ -319,7 +320,7 @@
 	rm -f aarch64.c
 	rm -f reg-arm.c reg-bfin.c i386.c reg-ia64.c reg-m32r.c reg-m68k.c
 	rm -f reg-sh.c reg-sparc.c reg-spu.c amd64.c i386-linux.c
-	rm -f reg-cris.c reg-crisv32.c amd64-linux.c reg-xtensa.c
+	rm -f reg-cr16.c reg-cris.c reg-crisv32.c amd64-linux.c reg-xtensa.c
 	rm -f reg-tilegx.c reg-tilegx32.c
 	rm -f arm-with-iwmmxt.c
 	rm -f arm-with-vfpv2.c arm-with-vfpv3.c arm-with-neon.c
@@ -584,6 +585,8 @@
 	$(SHELL) $(regdat_sh) $(srcdir)/../regformats/arm-with-neon.dat arm-with-neon.c
 reg-bfin.c : $(srcdir)/../regformats/reg-bfin.dat $(regdat_sh)
 	$(SHELL) $(regdat_sh) $(srcdir)/../regformats/reg-bfin.dat reg-bfin.c
+reg-cr16.c : $(srcdir)/../regformats/reg-cr16.dat $(regdat_sh)
+	$(SHELL) $(regdat_sh) $(srcdir)/../regformats/reg-cr16.dat reg-cr16.c
 reg-cris.c : $(srcdir)/../regformats/reg-cris.dat $(regdat_sh)
 	$(SHELL) $(regdat_sh) $(srcdir)/../regformats/reg-cris.dat reg-cris.c
 reg-crisv32.c : $(srcdir)/../regformats/reg-crisv32.dat $(regdat_sh)
Index: gdb/gdbserver/configure.srv
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/configure.srv,v
retrieving revision 1.76
diff -u -r1.76 configure.srv
--- gdb/gdbserver/configure.srv	28 May 2013 10:41:17 -0000	1.76
+++ gdb/gdbserver/configure.srv	19 Jun 2013 12:05:16 -0000
@@ -88,6 +88,11 @@
 			srv_linux_usrregs=yes
 			srv_linux_thread_db=yes
 			;;
+  cr16-*-uclinux)	srv_regobj=reg-cr16.o
+			srv_tgtobj="linux-low.o linux-cr16-low.o"
+			srv_linux_usrregs=yes
+			srv_linux_thread_db=yes
+			;;
   crisv32-*-linux*)	srv_regobj=reg-crisv32.o
 			srv_tgtobj="linux-low.o linux-osdata.o linux-crisv32-low.o linux-procfs.o"
 			srv_tgtobj="${srv_tgtobj} linux-ptrace.o"
Index: gdb/gdbserver/cr16-linux-tdep.c
===================================================================
RCS file: gdb/gdbserver/linux-cr16-tdep.c
diff -N gdb/gdbserver/linux-cr16-tdep.c
--- /dev/null	2013-05-10 19:36:04.372328500 +0530
+++ ./gdb/gdbserver/linux-cr16-low.c	2013-06-19 17:44:04.000000000 +0530
@@ -0,0 +1,190 @@
+/* GNU/Linux/CR16 specific low level interface, for the remote server for GDB.
+
+   Copyright (C) 2012-2013 Free Software Foundation, Inc.
+   Contributed by Kaushik Phatak (kaushik.phatak@kpitcummins.com)
+   KPIT Cummins Infosystems Limited, Pune India. 
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "server.h"
+#include "linux-low.h"
+#include <sys/ptrace.h>
+
+/* Defined in auto-generated file reg-cr16.c.  */
+void init_registers_cr16 (void);
+extern const struct target_desc *tdesc_cr16;
+
+/* CR16C  */
+#define cr16_num_regs 20
+#define PC_REGNUM 16
+
+/* Locations need to match <include/asm/arch/ptrace.h>.  */
+static int cr16_regmap[] = {
+  0,   2,  4,  6,  8,
+  10, 12, 14, 16, 18,
+  20, 22, 24, 28, 32,
+  36, 40, 44, 48, 52
+};
+
+static int
+cr16_cannot_store_register (int regno)
+{
+  if (cr16_regmap[regno] == -1)
+    return 1;
+
+  return (regno >= cr16_num_regs);
+}
+
+static int
+cr16_cannot_fetch_register (int regno)
+{
+  if (cr16_regmap[regno] == -1)
+    return 1;
+
+  return (regno >= cr16_num_regs);
+}
+
+static void
+cr16_set_pc (struct regcache *regcache, CORE_ADDR pc)
+{
+  unsigned long newpc = pc;
+
+  supply_register_by_name (regcache, "pc", &newpc);
+}
+
+static CORE_ADDR
+cr16_get_pc (struct regcache *regcache)
+{
+  unsigned long pc;
+
+  collect_register_by_name (regcache, "pc", &pc);
+  return pc;
+}
+
+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);
+}
+
+static void
+cr16_supply_ptrace_register (struct regcache *regcache,
+			     int regno, const char *buf)
+{
+  unsigned long pc;
+  int size = register_size (regcache->tdesc, regno);
+
+  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);
+    }
+}
+
+static const unsigned short cr16_breakpoint = 0xc700;
+#define cr16_breakpoint_len 2
+
+static int
+cr16_breakpoint_at (CORE_ADDR where)
+{
+  unsigned short insn;
+
+  (*the_target->read_memory) (where, (unsigned char *) &insn,
+			      cr16_breakpoint_len);
+  if (insn == cr16_breakpoint)
+    return 1;
+  /* If necessary, recognize more trap instructions here.  GDB only
+     uses the one.  */
+  return 0;
+}
+
+/* We only place breakpoints in empty marker functions, and thread locking
+   is outside of the function.  So rather than importing software single-step,
+   we can just run until exit.  */
+static CORE_ADDR
+cr16_reinsert_addr (void)
+{
+  struct regcache *regcache = get_thread_regcache (current_inferior, 1);
+  unsigned long pc;
+
+  /* R14/Ra is return address register  */
+  collect_register_by_name (regcache, "ra", &pc);
+  return pc;
+}
+
+static void
+cr16_arch_setup (void)
+{
+  current_process ()->tdesc = tdesc_cr16;
+}
+
+static struct usrregs_info cr16_usrregs_info =
+  {
+    cr16_num_regs,
+    cr16_regmap,
+  };
+
+static struct regs_info regs_info =
+  {
+    NULL, /* regset_bitmap */
+    &cr16_usrregs_info,
+  };
+
+static const struct regs_info *
+cr16_regs_info (void)
+{
+  return &regs_info;
+}
+
+struct linux_target_ops the_low_target = {
+  cr16_arch_setup,
+  cr16_regs_info,
+  cr16_cannot_fetch_register,
+  cr16_cannot_store_register,
+  NULL,
+  cr16_get_pc,
+  cr16_set_pc,
+  (const unsigned char *) &cr16_breakpoint,
+  cr16_breakpoint_len,
+  cr16_reinsert_addr,
+  0,
+  cr16_breakpoint_at,
+  0,
+  0,
+  0,
+  0,
+  cr16_collect_ptrace_register,
+  cr16_supply_ptrace_register
+};
+
+void
+initialize_low_arch (void)
+{
+  init_registers_cr16 ();
+}
Index: gdb/regformats/reg-cr16.dat
===================================================================
RCS file: gdb/regformats/reg-cr16.dat
diff -N gdb/gdbserver/reg-cr16.dat
--- /dev/null	2013-05-10 19:36:04.372328500 +0530
+++ ./gdb/regformats/reg-cr16.dat	2013-06-18 15:10:23.000000000 +0530
@@ -0,0 +1,22 @@
+name:cr16
+expedite:psr,pc,usp,ra
+16:r0
+16:r1
+16:r2
+16:r3
+16:r4
+16:r5
+16:r6
+16:r7
+16:r8
+16:r9
+16:r10
+16:r11
+32:r12
+32:r13
+32:ra
+32:psr
+32:pc
+32:intbase
+32:usp
+32:cfg

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

[Pedro Alves]

On 06/19/2013 02:28 PM, Kaushik Phatak wrote:

> The below gdbserver port has following changes,
> 1. Remove register pair access over remote protocol in the .dat file.
> This allows for matching of the register numbers in the gdbserver with those
> in linux-tdep.c. This was done by simplifying the PTRACE_GETREGS inside kernel
> to read single registers.
> 2. Remove support for "r0r1_orig" register. This is a kernel internal register
> and need not be user visible.
> 3. Set cr16_num_regs to 20 and PC_REGNUM to 16. This will now match the gdb port.
> 4. Add registers pc,usp,ra to the "expedite" set. 
> 5. Move the ptrace macro defines like PT_TEXT_ADDR from linux-low.c to the kernel's
> ptrace.h

I'm not clear on point #2, but the rest sounds excellent.  Thanks!

> +
> +/* We only place breakpoints in empty marker functions, and thread locking
> +   is outside of the function.  So rather than importing software single-step,
> +   we can just run until exit.  */
> +static CORE_ADDR

Add empty line between describing comment and function definition.

> +cr16_reinsert_addr (void)
> +{
> +  struct regcache *regcache = get_thread_regcache (current_inferior, 1);
> +  unsigned long pc;
> +
> +  /* R14/Ra is return address register  */

"... is the return ..."

Other than that, this looks good.

-- 
Pedro Alves

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

[Kaushik Phatak]

>> I'm not clear on point #2, but the rest sounds excellent.  Thanks!

Thanks for the review and kind words.
I have tried to answer point #2 in my gdb port reply. I may add the "r0r1_orig"
register into the port to support system restarting similar to other ports.

>> Add empty line between describing comment and function definition.
I will run through the code again and fix up the formatting.

Thanks,
Kaushik