Re: [RFC] New port: CR16: gdb

[Pedro Alves <palves at redhat dot com>]

On 09/04/2013 06:44 PM, Kaushik M Phatak wrote:
> Hi,
> Please find attached 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-06/msg00489.html
> 
> This patch incorporates changes suggested by Pedro,
> http://sourceware.org/ml/gdb-patches/2013-06/msg00734.html
> 
>> > isn't exposing r0_orig to gDB necessary for syscall restarting,
> Added this register using separate target description feature,
> added in features/cr16-linux.xml. This will generate the cr16-linux.dat
> in gdb/regformats which will be used by the gdbserver.
> 
>>> >> 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.
> Added new target description which supports these additional debug registers
> under features/cr16-dbg.xml The tdep files have been updated to lookup these
> features and update their register sets accordingly.

Thanks.  This is looking better.  I fell we'll have this should
be ready for merging soon, though there are still a couple rough
edges that need sorting out.  See below.

> 
> 2013-09-04 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-*-*

This file is in src/, so this entry doesn't belong here.
(this is not about src/gdb/configure.ac).  Also, missing period.


>     * configure: Regenerate.

Likewise.

>     * features/cr16-linux-core.xml: New.
>     * features/cr16-linux.xml: New.
>     * features/cr16-core.xml: New.
>     * features/cr16-dbg.xml: New.
>     * features/cr16-linux.c: New (autogenerated).
>     * features/cr16-dbg.c: New (autogenerated).




> +/* 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);
> +
> +  /* The opcode of excp bpt is 0x00c8, however for uClinux we will
> +     use the excp flg (0x00c7) to insert a breakpoint.  The excp bpt

hmm, "the excp flg (0x00c7) what", is what comes to mind.
I guess "instruction" is missing, perhaps?  Or drop the "the".

> +     requires external hardware support for breakpoints to work on the
> +     CR16 target.  Software based breakpoints are implemented in the
> +     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 the CR16
> +     supports only little endian.  */
> +  tdep->breakpoint = breakpoint_uclinux;
> +
> +}


> +
> +/* Hardware register name declaration.  */
> +static const char *const reg_names[] =
> +{
> +  "r0",
> +  "r1",
> +  "r2",
> +  "r3",
> +  "r4",
> +  "r5",
> +  "r6",
> +  "r7",
> +  "r8",
> +  "r9",
> +  "r10",
> +  "r11",
> +  "r12",
> +  "r13",
> +  "ra",
> +  "sp",
> +  "pc",
> +  "isp",
> +  "usp",
> +  "intbase",
> +};
> +
> +static const char *const linux_reg_names[] =
> +{

An intro comment is missing.  A reader will go "why
is this necessary/different from reg_names ?" (well,
I have).

Also, all Linux things should be in cr16-linux-tdep.c, not here.

> +  "r0",
> +  "r1",
> +  "r2",
> +  "r3",
> +  "r4",
> +  "r5",
> +  "r6",
> +  "r7",
> +  "r8",
> +  "r9",
> +  "r10",
> +  "r11",
> +  "r12",
> +  "r13",
> +  "ra",
> +  "psr",
> +  "pc",
> +  "intbase",
> +  "usp",
> +  "cfg"
> +};
> +




> +/* 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);
> +  CORE_ADDR frame_base = cr16_frame_base (this_frame, this_prologue_cache);
> +
> +  if (regnum == CR16_SP_REGNUM)
> +    return frame_unwind_got_constant (this_frame, regnum, frame_base);
> +
> +  /* The call instruction has saved the return address on the RA
> +     register, CR16_R13_REGNUM.  So, we need not adjust anything
> +     directly.  We will analyze prologue as this RA register is
> +     pushed onto stack for further leaf function calls to work.  */
> +  else if (regnum == CR16_PC_REGNUM)
> +    {
> +      ULONGEST ra_prev;
> +
> +      ra_prev = frame_unwind_register_unsigned (this_frame, CR16_RA_REGNUM);
> +      ra_prev = ra_prev << 1;
> +      return frame_unwind_got_constant (this_frame, CR16_PC_REGNUM, ra_prev);
> +    }
> +
> +  /* If prologue analysis says we saved this register somewhere,
> +     return a description of the stack slot holding it.  */
> +  else if (p->reg_offset[regnum] != 1)
> +      return frame_unwind_got_memory (this_frame, regnum,
> +				      frame_base + p->reg_offset[regnum]);
> +
> +  /* Otherwise, presume we haven't changed the value of this
> +     register, and get it from the next frame.  */
> +  else
> +      return frame_unwind_got_register (this_frame, regnum, regnum);

Indentation in these 10 lines or so above doesn't look right.

> +static const gdb_byte *
> +cr16_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR * pcptr,
> +			 int *lenptr)
> +{
> +  /* We use different breakpoint instructions for ELF and uClinux.
> +     See cr16-linux-tdep.c for more details.  */
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> +  *lenptr = 2;
> +  return tdep->breakpoint;
> +}
> +
> +/* Allocate and initialize a gdbarch object.  */
> +
> +static struct gdbarch *
> +cr16_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> +{
> +  const struct target_desc *tdesc = info.target_desc;
> +  struct tdesc_arch_data *tdesc_data = NULL;
> +  struct gdbarch *gdbarch;
> +  struct gdbarch_tdep *tdep;
> +  int elf_flags;
> +
> +  const struct tdesc_feature *feature;
> +  int i, valid_p = 1;
> + 
> +  if(info.osabi == GDB_OSABI_LINUX)

Missing space after "if".  But it's wrong to do this here,
instead of on the linux-specific gdbarch_init routine.

> +    {
> +      tdesc = tdesc_cr16_linux;
> +    }
> +  else
> +    {
> +      tdesc = tdesc_cr16_dbg;
> +    }

Remove the {}'s in the then/else branches.  Not necessary
for single-statements.

> +
> +  feature = tdesc_find_feature (tdesc, "org.gnu.gdb.cr16.core");
> +
> +  if(feature)

Missing space.  Several more instances of this.  I won't point them
all out.

> +    {
> +      tdesc_data = tdesc_data_alloc ();
> +      valid_p = 1;
> +      for (i = 0; i < 20; i++)  /* r0 - r13, ra, sp, pc, isp, usp, intbase  */
> +        valid_p &= tdesc_numbered_register (feature, tdesc_data, i,
> +					    reg_names[i]);
> +      if (!valid_p)
> +	{
> +	  tdesc_data_cleanup (tdesc_data);
> +	  return NULL;
> +	}
> +    }
> +
> +  i = ARRAY_SIZE (reg_names);
> +  feature = tdesc_find_feature (tdesc, "org.gnu.gdb.cr16.dbg");
> +  if (feature)
> +    {
> +      valid_p = 1;
> +      valid_p &= tdesc_numbered_register (feature, tdesc_data, i++, "dbs");
> +      valid_p &= tdesc_numbered_register (feature, tdesc_data, i++, "dsr");
> +      valid_p &= tdesc_numbered_register (feature, tdesc_data, i++, "dcs");
> +      valid_p &= tdesc_numbered_register (feature, tdesc_data, i++, "car0");
> +      valid_p &= tdesc_numbered_register (feature, tdesc_data, i++, "car1");
> +      if (!valid_p)
> +	{
> +	  tdesc_data_cleanup (tdesc_data);
> +	  return NULL;
> +	}
> +    }
> +
> +  feature = tdesc_find_feature (tdesc, "org.gnu.gdb.cr16.core");

.core again?  Shouldn't this be .linux ?  (and then, move it to the
linux tdep file.)  Please double-check how other ports doo this.  E.g,
amd64-linux-tdep.c, i386-linux-tdep.c or ppc-linux-tdep.c.

> +  if(feature)
> +    {
> +      valid_p &= tdesc_numbered_register (feature, tdesc_data, i++, "cfg");
> +      valid_p &= tdesc_numbered_register (feature, tdesc_data, i++, "psr");

If these are Linux-specific, why aren't they part of the .linux feature?
Having two different .core descriptions looks wrong to me.

> +      if (!valid_p)
> +	{
> +	  tdesc_data_cleanup (tdesc_data);
> +	  return NULL;
> +	}
> +    }
> +
> +  if(info.osabi == GDB_OSABI_LINUX)
> +    {
> +      tdesc = tdesc_cr16_linux;
> +    }

Should be instead handled by:

  if (!tdesc_has_registers (tdesc))
    tdesc = tdesc_cr16_linux;

In a cr16_linux_init_abi function in the linux tdep file.

> +
> +  feature = tdesc_find_feature (tdesc, "org.gnu.gdb.cr16-linux.core");

.core again.  This one just looks like a copy-paste?

> +  if(feature)
> +    {
> +      tdesc_data = tdesc_data_alloc ();
> +      valid_p = 1;
> +      for (i = 0; i < 20; i++)  /* r0 - r13, ra, psr, pc, intbase, usp, cfg  */
> +	valid_p &= tdesc_numbered_register (feature, tdesc_data, i,
> +					    linux_reg_names[i]);
> +      if (!valid_p)
> +	{
> +	  tdesc_data_cleanup (tdesc_data);
> +	  return NULL;
> +	}
> +    }
> +
> +  feature = tdesc_find_feature (tdesc, "org.gnu.gdb.cr16.linux");

Ah, .linux now.  Looks like this whole section of code was removed out
of the oven too early.  ;-)

> +  if(feature)
> +    {
> +      valid_p = 1;
> +      valid_p &= tdesc_numbered_register (feature, tdesc_data,
> +					  CR16_ORIG_R0AND1_REGNUM,
> +					  "orig_r0and1");

[Curious that you still export r0/r1 in a single register here.
(or even that you export orig r1 at all).  The user shouldn't
really see this register usually, so it's fine with me.]

> +      if (!valid_p)
> +	{
> +	tdesc_data_cleanup (tdesc_data);
> +	return NULL;
> +	}
> +    }
> +




> --- /dev/null	2013-07-24 13:49:11.642170250 +0530
> +++ ./gdb/cr16-tdep.h	2013-08-12 13:42:39.000000000 +0530
> @@ -0,0 +1,43 @@
> +/* GNU/Linux on  CR16 target support.
> +   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/>.  */
> +
> +#ifndef CR16_TDEP_H
> +#define CR16_TDEP_H
> +
> +/* CR16 target descriptions.  */
> +extern struct target_desc *tdesc_cr16_linux;

All Linux things should move to linux tdep files.  I won't
point out all instances of this, but please go through the
patch doing that throughout.

> +extern struct target_desc *tdesc_cr16_dbg;
> +
> +/* Number of registers available for Linux targets.  */
> +#define CR16_LINUX_NUM_REGS  20
> +#define CR16_ORIG_R0AND1_REGNUM (CR16_LINUX_NUM_REGS + 1)
> +
> +/* Target-dependent structure in gdbarch.  */
> +struct gdbarch_tdep
> +{
> +  /* The ELF header flags specify the multilib used.  */
> +  int elf_flags;
> +
> +  /* Breakpoint instruction.  */
> +  const gdb_byte *breakpoint;
> +};
> +
> +#endif /* CR16_TDEP_H  */
> --- /dev/null	2013-07-24 13:49:11.642170250 +0530
> +++ ./gdb/features/cr16-linux-core.xml	2013-08-12 13:42:39.000000000 +0530
> @@ -0,0 +1,31 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2007-2013 Free Software Foundation, Inc.
> +
> +     Copying and distribution of this file, with or without modification,
> +     are permitted in any medium without royalty provided the copyright
> +     notice and this notice are preserved.  -->
> +
> +<!DOCTYPE feature SYSTEM "gdb-target.dtd">
> +<feature name="org.gnu.gdb.cr16-linux.core">
> +  <reg name="r0" bitsize="16" type="uint16"/>
> +  <reg name="r1" bitsize="16" type="uint16"/>
> +  <reg name="r2" bitsize="16" type="uint16"/>
> +  <reg name="r3" bitsize="16" type="uint16"/>
> +  <reg name="r4" bitsize="16" type="uint16"/>
> +  <reg name="r5" bitsize="16" type="uint16"/>
> +  <reg name="r6" bitsize="16" type="uint16"/>
> +  <reg name="r7" bitsize="16" type="uint16"/>
> +  <reg name="r8" bitsize="16" type="uint16"/>
> +  <reg name="r9" bitsize="16" type="uint16"/>
> +  <reg name="r10" bitsize="16" type="uint16"/>
> +  <reg name="r11" bitsize="16" type="uint16"/>
> +  <reg name="r12" bitsize="32" type="uint32"/>
> +  <reg name="r13" bitsize="32" type="uint32"/>
> +  <reg name="ra" bitsize="32" type="code_ptr"/>
> +  <reg name="psr" bitsize="32" type="uint32"/>
> +  <reg name="pc" bitsize="32" type="code_ptr"/>
> +  <reg name="intbase" bitsize="32" type="uint32"/>
> +  <reg name="usp" bitsize="32" type="uint32"/>
> +  <reg name="cfg" bitsize="32" type="uint32"/>
> +

Spurious line.

> +</feature>



> --- /dev/null	2013-07-24 13:49:11.642170250 +0530
> +++ ./gdb/features/cr16-core.xml	2013-08-12 13:42:39.000000000 +0530
> @@ -0,0 +1,36 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2007-2013 Free Software Foundation, Inc.
> +
> +     Copying and distribution of this file, with or without modification,
> +     are permitted in any medium without royalty provided the copyright
> +     notice and this notice are preserved.  -->
> +
> +<!DOCTYPE feature SYSTEM "gdb-target.dtd">
> +<feature name="org.gnu.gdb.cr16.core">
> +  <reg name="r0" bitsize="16" type="uint16"/>
> +  <reg name="r1" bitsize="16" type="uint16"/>
> +  <reg name="r2" bitsize="16" type="uint16"/>
> +  <reg name="r3" bitsize="16" type="uint16"/>
> +  <reg name="r4" bitsize="16" type="uint16"/>
> +  <reg name="r5" bitsize="16" type="uint16"/>
> +  <reg name="r6" bitsize="16" type="uint16"/>
> +  <reg name="r7" bitsize="16" type="uint16"/>
> +  <reg name="r8" bitsize="16" type="uint16"/>
> +  <reg name="r9" bitsize="16" type="uint16"/>
> +  <reg name="r10" bitsize="16" type="uint16"/>
> +  <reg name="r11" bitsize="16" type="uint16"/>
> +  <reg name="r12" bitsize="32" type="uint32"/>
> +  <reg name="r13" bitsize="32" type="uint32"/>
> +  <reg name="ra" bitsize="32" type="code_ptr"/>
> +  <reg name="sp" bitsize="32" type="data_ptr"/>
> +  <reg name="pc" bitsize="32" type="code_ptr"/>
> +  <reg name="isp" bitsize="32" type="data_ptr"/>
> +  <reg name="usp" bitsize="32" type="data_ptr"/>
> +  <reg name="intbase" bitsize="32" type="data_ptr"/>
> +
> +  <!-- The psr is register 27, and cfg is 26, because
> +       the Debug configuration registers are placed between the PC
> +       and the cfg.  -->
> +  <reg name="cfg" bitsize="32" regnum="26"/>
> +  <reg name="psr" bitsize="32" regnum="27"/>

So you want to have this gapping-scheme to maintain compatibility with
older stubs that didn't send out a description
(like arm-core.xml/arm-fpa.xml)?  Otherwise, you wouldn't really need this.

> +</feature>
> --- /dev/null	2013-07-24 13:49:11.642170250 +0530
> +++ ./gdb/features/cr16-dbg.xml	2013-08-12 13:42:39.000000000 +0530
> @@ -0,0 +1,20 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2010-2013 Free Software Foundation, Inc.
> +
> +     Copying and distribution of this file, with or without modification,
> +     are permitted in any medium without royalty provided the copyright
> +     notice and this notice are preserved.  -->
> +
> +<!DOCTYPE target SYSTEM "gdb-target.dtd">
> +<target>
> +  <architecture>cr16</architecture>
> +  <xi:include href="cr16-core.xml"/>
> +
> +  <feature name="org.gnu.gdb.cr16.dbg">
> +  <reg name="dbs" bitsize="32" type="uint32"/>

But then this doesn't set the register number explicitly,
so it doesn't look like that is working as intended anyhow?
But maybe I'm missing something.

> +  <reg name="dsr" bitsize="32" type="uint32"/>
> +  <reg name="dcs" bitsize="32" type="uint32"/>
> +  <reg name="car0" bitsize="32" type="uint32"/>
> +  <reg name="car1" bitsize="32" type="uint32"/>
> +  </feature>
> +</target>

-- 
Pedro Alves