Re: [RFA 3/5] New port: CR16: gdb port

[Pedro Alves <palves at redhat dot com>]

(dropping gcc-patches@)

On 06/19/2013 02:14 PM, Kaushik Phatak wrote:
> Hi,
> Please find below an updated patch for the gdb port of the CR16 target.
> This has been updated from my previous attempts of the same,
> http://sourceware.org/ml/gdb-patches/2013-01/msg00299.html
> 
> The gdb port required cleanup and some tweaking and is largely unchanged, 
> however the gdbserver port required some structural changes in the RSP for 
> register access.
> The below gdb port has following changes,
> 1. Removed pseudo register "r0r1_orig" which is not part of user visible 
> registers and used internally by the kernel.

Hmm.  Just to be clear, isn't exposing r0_orig to gDB necessary for
syscall restarting, like orig_eax/orig_rax on x86/x86_64, orig_r3 on ppc,
orig_r2 on s390, etc.?  See e.g., i386_linux_write_pc, amd64_linux_write_pc,
ppc_linux_write_pc, s390_write_pc.

> 2. Added support for debug registers in the ELF port. This will make it consistent 
> with the current simulator port which already supports them.

Are these always present in all versions of CR16 silicon?  IOW, are
we safe with adding them to the core register set (*)?  Otherwise,
you should really split them to a separate target description feature.

(*) which registers are those btw?  I'm not that familiar with CR16.  :-)

> 3. Use gdb_insn_length to read the length of the current instruction. This removes
> a lot of the bfd code that used exported functions and globals. Pedro and Joel had
> concerns over using this piece of code, which is now removed.

Excellent.

> 4. Use local variable "cr16_insn_words" instead of global "cr16_words" for opcode
> analysis in cr16_analyze_prologue.
> 
> I will also be posting an updated gdbserver port which will compliment this port.
> I am copying gcc-patches here, as there are changes to configure.ac.
> 

This otherwise looks very good.  Thanks for the update.  A few minor
comments below.

> 2013-06-19 Kaushik Phatak  <kaushik.phatak@kpitcummins.com>
> 	gdb/Changelog
> 	* configure.tgt: Handle cr16*-*-*linux and cr16*-*-*.
> 	* cr16-linux-tdep.c: New file.
> 	* cr16-tdep.c: New file.
> 	* cr16-tdep.h: New file.
> 	* configure.ac : Add support for cr16-*-*

No space before ':'.

> 	* configure: Regenerate.

> Index: gdb/cr16-linux-tdep.c
> ===================================================================
> RCS file: gdb/cr16-linux-tdep.c
> diff -N gdb/cr16-linux-tdep.c

> +
> +/* Number of registers available for Linux targets  */

Full stop and double space.

/* Number of registers available for Linux targets.  */

> +#define CR16_LINUX_NUM_REGS  20
> +
> +/* The breakpoint instruction used by uClinux target  */

Ditto.  "by the uClinux".

> +static const gdb_byte breakpoint_uclinux[] = { 0xC7, 0x00 };


> +/* OS specific initialization of gdbarch.  */
> +
> +static void
> +cr16_uclinux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
> +{
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> +  linux_init_abi (info, gdbarch);
> +
> +  set_gdbarch_num_regs (gdbarch, CR16_LINUX_NUM_REGS);
> +  set_gdbarch_register_name (gdbarch, cr16_linux_register_name);
> +
> +  /* The opcode of excp bpt is 0x00C8, however for uclinux we will
> +     use the excp flg (0x00C7) to insert a breakpoint.  The excp bpt
> +     requires external hardware support for breakpoints to work on
> +     CR16 target.  Software based breakpoints are implemented in the

"on _the_ CR16 target".

> +     kernel using excp flg and tested on the SC14452 target.  Use
> +     0x00C7 with gdbserver/kernel and 0x00C8 for sim/ELF.  We
> +     represent the breakpoint in little endian format since CR16
> +     supports only little endian.  */
> +  tdep->breakpoint = breakpoint_uclinux;



> +/* Implement the "frame_this_id" method for unwinding frames.  */
> +
> +static void
> +cr16_frame_this_id (struct frame_info *this_frame,
> +		    void **this_prologue_cache, struct frame_id *this_id)
> +{
> +  *this_id =
> +    frame_id_build (cr16_frame_base (this_frame, this_prologue_cache),

= goes on the next line.

> +		    get_frame_func (this_frame));
> +}
> +
> +/* Implement the "frame_prev_register" method for unwinding frames.  */
> +
> +static struct value *
> +cr16_frame_prev_register (struct frame_info *this_frame,
> +			  void **this_prologue_cache, int regnum)
> +{
> +  struct cr16_prologue *p =
> +    cr16_analyze_frame_prologue (this_frame, this_prologue_cache);

Ditto.

> +
> +/* Implement the "return_value" gdbarch method.  */
> +
> +static enum return_value_convention
> +cr16_return_value (struct gdbarch *gdbarch,
> +		   struct type *func_type,
> +		   struct type *valtype,
> +		   struct regcache *regcache,
> +		   gdb_byte * readbuf, const gdb_byte * writebuf)
> +{
...
> +
> +  if (readbuf)

  if (readbuf != NULL)

> +    {
> +      ULONGEST u;
> +      int argreg = CR16_R0_REGNUM;
> +      int offset = 0;
> +
> +      while (valtype_len > 0)
> +	{
> +	  int len = min (valtype_len, 4);
> +
> +	  regcache_cooked_read_unsigned (regcache, argreg, &u);
> +	  store_unsigned_integer (readbuf + offset, len, byte_order, u);
> +	  valtype_len -= len;
> +	  offset += len;
> +	  argreg++;
> +	}
> +    }
> +
> +  if (writebuf)

 if (writebuf != NULL)

> Index: gdb/cr16-tdep.h
> ===================================================================
> RCS file: gdb/cr16-tdep.h
> diff -N gdb/cr16-tdep.h
> --- /dev/null	2013-05-10 19:36:04.372328500 +0530
> +++ ./gdb/cr16-tdep.h	2013-06-17 12:49:05.000000000 +0530
> @@ -0,0 +1,32 @@
> +/* GNU/Linux on  CR16 target support.

Nope, this is not the GNU/Linux file.  Also spurious double space
there.

-- 
Pedro Alves