RX: Convert target-description

Message ID 20190821033301.45309-1-ysato@users.sourceforge.jp
State New
Headers show
Series
  • RX: Convert target-description
Related show

Commit Message

Yoshinori Sato Aug. 21, 2019, 3:33 a.m.
gdb/ChangeLog

2019-08-21  Yoshinori Sato <ysato@users.sourceforge.jp>

	* gdb/rx-tdep.c (rx_register_names): New.
	(rx_register_name): Use rx_register_names.
	(rx_register_name): Add check range.
	(rx_register_g_packet_guesses): New.
	(rx_gdbarch_init): Convert target-descriptions.
	(_initialize_rx_tdep): Add initialize_tdesc_rx.
	* gdb/features/Makefile: Add rx.xml.
	* gdb/features/rx.xml: New.
---
 gdb/features/Makefile |  2 ++
 gdb/features/rx.xml   | 70 ++++++++++++++++++++++++++++++++++++
 gdb/rx-tdep.c         | 99 ++++++++++++++++++++++++++++++---------------------
 3 files changed, 130 insertions(+), 41 deletions(-)
 create mode 100644 gdb/features/rx.xml

-- 
2.11.0

Comments

Andrew Burgess Aug. 21, 2019, 9:24 a.m. | #1
Thanks for the patch.

* Yoshinori Sato <ysato@users.sourceforge.jp> [2019-08-21 12:33:01 +0900]:

GDB patches usually include a brief description to the patch before
the changelog entry in the commit message, even if its just one
sentence like:

  Convert the RX target to make use of target descriptions.

> gdb/ChangeLog

> 

> 2019-08-21  Yoshinori Sato <ysato@users.sourceforge.jp>

> 

> 	* gdb/rx-tdep.c (rx_register_names): New.

> 	(rx_register_name): Use rx_register_names.

> 	(rx_register_name): Add check range.

> 	(rx_register_g_packet_guesses): New.

> 	(rx_gdbarch_init): Convert target-descriptions.

> 	(_initialize_rx_tdep): Add initialize_tdesc_rx.

> 	* gdb/features/Makefile: Add rx.xml.

> 	* gdb/features/rx.xml: New.


You should generate and commit the gdb/features/rx.c file as well.
These files are not automatically generated by the build system and we
rely on people regenerating them when the xml file changes.

There's a few minor coding standard issues I've pointed out below.

> ---

>  gdb/features/Makefile |  2 ++

>  gdb/features/rx.xml   | 70 ++++++++++++++++++++++++++++++++++++

>  gdb/rx-tdep.c         | 99 ++++++++++++++++++++++++++++++---------------------

>  3 files changed, 130 insertions(+), 41 deletions(-)

>  create mode 100644 gdb/features/rx.xml

> 

> diff --git a/gdb/features/Makefile b/gdb/features/Makefile

> index 0c84faf405..2b65d46df0 100644

> --- a/gdb/features/Makefile

> +++ b/gdb/features/Makefile

> @@ -161,6 +161,7 @@ XMLTOC = \

>  	rs6000/powerpc-vsx64.xml \

>  	rs6000/powerpc-vsx64l.xml \

>  	rs6000/rs6000.xml \

> +	rx.xml \

>  	s390-linux32.xml \

>  	s390-linux32v1.xml \

>  	s390-linux32v2.xml \

> @@ -238,6 +239,7 @@ FEATURE_XMLFILES = aarch64-core.xml \

>  	riscv/64bit-cpu.xml \

>  	riscv/64bit-csr.xml \

>  	riscv/64bit-fpu.xml \

> +	rx.xml \

>  	tic6x-c6xp.xml \

>  	tic6x-core.xml \

>  	tic6x-gp.xml

> diff --git a/gdb/features/rx.xml b/gdb/features/rx.xml

> new file mode 100644

> index 0000000000..b5aa9ac4a8

> --- /dev/null

> +++ b/gdb/features/rx.xml

> @@ -0,0 +1,70 @@

> +<?xml version="1.0"?>

> +<!-- Copyright (C) 2019 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.rx.core">

> +  <reg name="r0" bitsize="32" type="data_ptr"/>

> +  <reg name="r1" bitsize="32" type="uint32"/>

> +  <reg name="r2" bitsize="32" type="uint32"/>

> +  <reg name="r3" bitsize="32" type="uint32"/>

> +  <reg name="r4" bitsize="32" type="uint32"/>

> +  <reg name="r5" bitsize="32" type="uint32"/>

> +  <reg name="r6" bitsize="32" type="uint32"/>

> +  <reg name="r7" bitsize="32" type="uint32"/>

> +  <reg name="r8" bitsize="32" type="uint32"/>

> +  <reg name="r9" bitsize="32" type="uint32"/>

> +  <reg name="r10" bitsize="32" type="uint32"/>

> +  <reg name="r11" bitsize="32" type="uint32"/>

> +  <reg name="r12" bitsize="32" type="uint32"/>

> +  <reg name="r13" bitsize="32" type="uint32"/>

> +  <reg name="r14" bitsize="32" type="uint32"/>

> +  <reg name="r15" bitsize="32" type="uint32"/>

> +

> +  <flags id="psw_flags" size="4">

> +    <field name="C" start="0" end="0"/>

> +    <field name="Z" start="1" end="1"/>

> +    <field name="S" start="2" end="2"/>

> +    <field name="O" start="3" end="3"/>

> +    <field name="I" start="16" end="16"/>

> +    <field name="U" start="17" end="17"/>

> +    <field name="PM" start="20" end="20"/>

> +    <field name="IPL" start="24" end="27"/>

> +  </flags>

> +

> +  <flags id="fpsw_flags" size="4">

> +    <field name="RM" start="0" end="1"/>

> +    <field name="CV" start="2" end="2"/>

> +    <field name="CO" start="3" end="3"/>

> +    <field name="CZ" start="4" end="4"/>

> +    <field name="CU" start="5" end="5"/>

> +    <field name="CX" start="6" end="6"/>

> +    <field name="CE" start="7" end="7"/>

> +    <field name="DN" start="8" end="8"/>

> +    <field name="EV" start="10" end="10"/>

> +    <field name="EO" start="11" end="11"/>

> +    <field name="EZ" start="12" end="12"/>

> +    <field name="EU" start="13" end="13"/>

> +    <field name="EX" start="14" end="14"/>

> +    <field name="FV" start="26" end="26"/>

> +    <field name="FO" start="27" end="27"/>

> +    <field name="FZ" start="28" end="28"/>

> +    <field name="FU" start="29" end="29"/>

> +    <field name="FX" start="30" end="30"/>

> +    <field name="FS" start="31" end="31"/>

> +  </flags>

> +

> +  <reg name="usp" bitsize="32" type="data_ptr"/>

> +  <reg name="isp" bitsize="32" type="data_ptr"/>

> +  <reg name="psw" bitsize="32" type="psw_flags"/>

> +  <reg name="pc" bitsize="32" type="code_ptr"/>

> +  <reg name="intb" bitsize="32" type="data_ptr"/>

> +  <reg name="bpsw" bitsize="32" type="psw_flags"/>

> +  <reg name="bpc" bitsize="32" type="code_ptr"/>

> +  <reg name="fintv" bitsize="32" type="code_ptr"/>

> +  <reg name="fpsw" bitsize="32" type="fpsw_flags"/>

> +  <reg name="acc" bitsize="64" type="uint64"/>

> +</feature>

> diff --git a/gdb/rx-tdep.c b/gdb/rx-tdep.c

> index 4cbf919db9..b3398f122a 100644

> --- a/gdb/rx-tdep.c

> +++ b/gdb/rx-tdep.c

> @@ -33,11 +33,15 @@

>  #include "value.h"

>  #include "gdbcore.h"

>  #include "dwarf2-frame.h"

> +#include "remote.h"

> +#include "target-descriptions.h"

>  

>  #include "elf/rx.h"

>  #include "elf-bfd.h"

>  #include <algorithm>

>  

> +#include "features/rx.c"

> +

>  /* Certain important register numbers.  */

>  enum

>  {

> @@ -114,40 +118,21 @@ struct rx_prologue

>    int reg_offset[RX_NUM_REGS];

>  };

>  

> +static const char *const rx_register_names[] = {

> +  "r0",  "r1",  "r2",  "r3",  "r4",  "r5",  "r6",  "r7",

> +  "r8",  "r9",  "r10", "r11", "r12", "r13", "r14", "r15",

> +  "usp", "isp", "psw", "pc",  "intb", "bpsw","bpc","fintv",

> +  "fpsw", "acc",

> +};


All file level variables and functions should have a header comment.

> +

>  /* Implement the "register_name" gdbarch method.  */

>  static const char *

>  rx_register_name (struct gdbarch *gdbarch, int regnr)

>  {

> -  static const char *const reg_names[] = {

> -    "r0",

> -    "r1",

> -    "r2",

> -    "r3",

> -    "r4",

> -    "r5",

> -    "r6",

> -    "r7",

> -    "r8",

> -    "r9",

> -    "r10",

> -    "r11",

> -    "r12",

> -    "r13",

> -    "r14",

> -    "r15",

> -    "usp",

> -    "isp",

> -    "psw",

> -    "pc",

> -    "intb",

> -    "bpsw",

> -    "bpc",

> -    "fintv",

> -    "fpsw",

> -    "acc"

> -  };

> -

> -  return reg_names[regnr];

> +  if (regnr >= 0 && regnr < RX_NUM_REGS)

> +    return rx_register_names[regnr];

> +  else

> +    return NULL;

>  }

>  

>  /* Construct the flags type for PSW and BPSW.  */

> @@ -1037,6 +1022,14 @@ rx_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int reg)

>      return -1;

>  }

>  

> +static void

> +rx_register_g_packet_guesses (struct gdbarch *gdbarch)

> +{

> +  register_remote_g_packet_guess (gdbarch,

> +                                  4 * RX_NUM_REGS,

> +                                  tdesc_rx);

> +}


Header comment again.

> +

>  /* Allocate and initialize a gdbarch object.  */

>  static struct gdbarch *

>  rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)

> @@ -1044,6 +1037,8 @@ rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)

>    struct gdbarch *gdbarch;

>    struct gdbarch_tdep *tdep;

>    int elf_flags;

> +  struct tdesc_arch_data *tdesc_data = NULL;

> +  const struct target_desc *tdesc = info.target_desc;

>  

>    /* Extract the elf_flags if available.  */

>    if (info.abfd != NULL

> @@ -1065,8 +1060,33 @@ rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)

>        return arches->gdbarch;

>      }

>  

> -  /* None found, create a new architecture from the information

> -     provided.  */

> +  if (tdesc == NULL)

> +      tdesc = tdesc_rx;

> +

> +  /* Check any target description for validity.  */

> +  if (tdesc_has_registers (tdesc))

> +    {

> +      const struct tdesc_feature *feature;

> +      int valid_p = 0;


This should be a bool.

> +      int i = 0;


This can move down into the if block scope.

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

> +

> +      if (feature != NULL)

> +	{

> +	  tdesc_data = tdesc_data_alloc ();

> +	  valid_p = 1;

> +	  for (i = 0; i < RX_NUM_REGS; i++)

> +	    valid_p &= tdesc_numbered_register (feature, tdesc_data, i,

> +                                            rx_register_names[i]);

> +	}

> +

> +      if (!valid_p)

> +	{

> +	  tdesc_data_cleanup (tdesc_data);

> +	  return NULL;

> +	}

> +    }

> +

>    tdep = XCNEW (struct gdbarch_tdep);

>    gdbarch = gdbarch_alloc (&info, tdep);

>    tdep->elf_flags = elf_flags;

> @@ -1083,15 +1103,6 @@ rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)

>    set_gdbarch_sw_breakpoint_from_kind (gdbarch, rx_breakpoint::bp_from_kind);

>    set_gdbarch_skip_prologue (gdbarch, rx_skip_prologue);

>  

> -  /* Target builtin data types.  */

> -  set_gdbarch_char_signed (gdbarch, 0);

> -  set_gdbarch_short_bit (gdbarch, 16);

> -  set_gdbarch_int_bit (gdbarch, 32);

> -  set_gdbarch_long_bit (gdbarch, 32);

> -  set_gdbarch_long_long_bit (gdbarch, 64);

> -  set_gdbarch_ptr_bit (gdbarch, 32);

> -  set_gdbarch_float_bit (gdbarch, 32);

> -  set_gdbarch_float_format (gdbarch, floatformats_ieee_single);


I don't understand why these are being deleted.  This doesn't feel
like it should be related to conversion to target descriptions.

>    if (elf_flags & E_FLAG_RX_64BIT_DOUBLES)

>      {

>        set_gdbarch_double_bit (gdbarch, 64);

> @@ -1115,6 +1126,8 @@ rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)

>    dwarf2_append_unwinders (gdbarch);

>    frame_unwind_append_unwinder (gdbarch, &rx_frame_unwind);

>  

> +  rx_register_g_packet_guesses (gdbarch);

> +

>    /* Methods setting up a dummy call, and extracting the return value from

>       a call.  */

>    set_gdbarch_push_dummy_call (gdbarch, rx_push_dummy_call);

> @@ -1123,6 +1136,9 @@ rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)

>    /* Virtual tables.  */

>    set_gdbarch_vbit_in_delta (gdbarch, 1);

>  

> +  if (tdesc_data != NULL)

> +    tdesc_use_registers (gdbarch, tdesc, tdesc_data);


I'm confused by the 'tdesc_data != NULL' check here, my reading of
things is that either:

  1. Target doesn't provide a target description, we fall back to the
  built in tdesc_rx, this will be valid, tdesc_data will not be NULL.

  2. The target provides a description, but its invalid, we bail out
  earlier after the validity check, we never reach this code.

  3. The target provides a description which is valid, tdesc_data will
  not be NULL.

I'd prefer to see a 'gdb_assert (tdesc_data != NULL)' immediately
after the earlier 'if (!valid_p)' block, and then after that just
assume tdesc_data is not NULL.

Actually, while we're moving code around, is there any reason that
this call to tdesc_use_registers couldn't move up to be just after the
valid_p check?  Given its closely related code?

> +

>    return gdbarch;

>  }

>  

> @@ -1132,4 +1148,5 @@ void

>  _initialize_rx_tdep (void)

>  {

>    register_gdbarch_init (bfd_arch_rx, rx_gdbarch_init);

> +  initialize_tdesc_rx ();

>  }

> -- 

> 2.11.0

> 


Thanks,
Andrew
Yoshinori Sato Aug. 21, 2019, 1:30 p.m. | #2
On Wed, 21 Aug 2019 18:24:33 +0900,
Andrew Burgess wrote:
> 

> Thanks for the patch.

> 

> * Yoshinori Sato <ysato@users.sourceforge.jp> [2019-08-21 12:33:01 +0900]:

> 

> GDB patches usually include a brief description to the patch before

> the changelog entry in the commit message, even if its just one

> sentence like:

> 

>   Convert the RX target to make use of target descriptions.


OK.

> > gdb/ChangeLog

> > 

> > 2019-08-21  Yoshinori Sato <ysato@users.sourceforge.jp>

> > 

> > 	* gdb/rx-tdep.c (rx_register_names): New.

> > 	(rx_register_name): Use rx_register_names.

> > 	(rx_register_name): Add check range.

> > 	(rx_register_g_packet_guesses): New.

> > 	(rx_gdbarch_init): Convert target-descriptions.

> > 	(_initialize_rx_tdep): Add initialize_tdesc_rx.

> > 	* gdb/features/Makefile: Add rx.xml.

> > 	* gdb/features/rx.xml: New.

> 

> You should generate and commit the gdb/features/rx.c file as well.

> These files are not automatically generated by the build system and we

> rely on people regenerating them when the xml file changes.


OK.
I forgat add it file.
Added it.

> There's a few minor coding standard issues I've pointed out below.

> 

> > ---

> >  gdb/features/Makefile |  2 ++

> >  gdb/features/rx.xml   | 70 ++++++++++++++++++++++++++++++++++++

> >  gdb/rx-tdep.c         | 99 ++++++++++++++++++++++++++++++---------------------

> >  3 files changed, 130 insertions(+), 41 deletions(-)

> >  create mode 100644 gdb/features/rx.xml

> > 

> > diff --git a/gdb/features/Makefile b/gdb/features/Makefile

> > index 0c84faf405..2b65d46df0 100644

> > --- a/gdb/features/Makefile

> > +++ b/gdb/features/Makefile

> > @@ -161,6 +161,7 @@ XMLTOC = \

> >  	rs6000/powerpc-vsx64.xml \

> >  	rs6000/powerpc-vsx64l.xml \

> >  	rs6000/rs6000.xml \

> > +	rx.xml \

> >  	s390-linux32.xml \

> >  	s390-linux32v1.xml \

> >  	s390-linux32v2.xml \

> > @@ -238,6 +239,7 @@ FEATURE_XMLFILES = aarch64-core.xml \

> >  	riscv/64bit-cpu.xml \

> >  	riscv/64bit-csr.xml \

> >  	riscv/64bit-fpu.xml \

> > +	rx.xml \

> >  	tic6x-c6xp.xml \

> >  	tic6x-core.xml \

> >  	tic6x-gp.xml

> > diff --git a/gdb/features/rx.xml b/gdb/features/rx.xml

> > new file mode 100644

> > index 0000000000..b5aa9ac4a8

> > --- /dev/null

> > +++ b/gdb/features/rx.xml

> > @@ -0,0 +1,70 @@

> > +<?xml version="1.0"?>

> > +<!-- Copyright (C) 2019 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.rx.core">

> > +  <reg name="r0" bitsize="32" type="data_ptr"/>

> > +  <reg name="r1" bitsize="32" type="uint32"/>

> > +  <reg name="r2" bitsize="32" type="uint32"/>

> > +  <reg name="r3" bitsize="32" type="uint32"/>

> > +  <reg name="r4" bitsize="32" type="uint32"/>

> > +  <reg name="r5" bitsize="32" type="uint32"/>

> > +  <reg name="r6" bitsize="32" type="uint32"/>

> > +  <reg name="r7" bitsize="32" type="uint32"/>

> > +  <reg name="r8" bitsize="32" type="uint32"/>

> > +  <reg name="r9" bitsize="32" type="uint32"/>

> > +  <reg name="r10" bitsize="32" type="uint32"/>

> > +  <reg name="r11" bitsize="32" type="uint32"/>

> > +  <reg name="r12" bitsize="32" type="uint32"/>

> > +  <reg name="r13" bitsize="32" type="uint32"/>

> > +  <reg name="r14" bitsize="32" type="uint32"/>

> > +  <reg name="r15" bitsize="32" type="uint32"/>

> > +

> > +  <flags id="psw_flags" size="4">

> > +    <field name="C" start="0" end="0"/>

> > +    <field name="Z" start="1" end="1"/>

> > +    <field name="S" start="2" end="2"/>

> > +    <field name="O" start="3" end="3"/>

> > +    <field name="I" start="16" end="16"/>

> > +    <field name="U" start="17" end="17"/>

> > +    <field name="PM" start="20" end="20"/>

> > +    <field name="IPL" start="24" end="27"/>

> > +  </flags>

> > +

> > +  <flags id="fpsw_flags" size="4">

> > +    <field name="RM" start="0" end="1"/>

> > +    <field name="CV" start="2" end="2"/>

> > +    <field name="CO" start="3" end="3"/>

> > +    <field name="CZ" start="4" end="4"/>

> > +    <field name="CU" start="5" end="5"/>

> > +    <field name="CX" start="6" end="6"/>

> > +    <field name="CE" start="7" end="7"/>

> > +    <field name="DN" start="8" end="8"/>

> > +    <field name="EV" start="10" end="10"/>

> > +    <field name="EO" start="11" end="11"/>

> > +    <field name="EZ" start="12" end="12"/>

> > +    <field name="EU" start="13" end="13"/>

> > +    <field name="EX" start="14" end="14"/>

> > +    <field name="FV" start="26" end="26"/>

> > +    <field name="FO" start="27" end="27"/>

> > +    <field name="FZ" start="28" end="28"/>

> > +    <field name="FU" start="29" end="29"/>

> > +    <field name="FX" start="30" end="30"/>

> > +    <field name="FS" start="31" end="31"/>

> > +  </flags>

> > +

> > +  <reg name="usp" bitsize="32" type="data_ptr"/>

> > +  <reg name="isp" bitsize="32" type="data_ptr"/>

> > +  <reg name="psw" bitsize="32" type="psw_flags"/>

> > +  <reg name="pc" bitsize="32" type="code_ptr"/>

> > +  <reg name="intb" bitsize="32" type="data_ptr"/>

> > +  <reg name="bpsw" bitsize="32" type="psw_flags"/>

> > +  <reg name="bpc" bitsize="32" type="code_ptr"/>

> > +  <reg name="fintv" bitsize="32" type="code_ptr"/>

> > +  <reg name="fpsw" bitsize="32" type="fpsw_flags"/>

> > +  <reg name="acc" bitsize="64" type="uint64"/>

> > +</feature>

> > diff --git a/gdb/rx-tdep.c b/gdb/rx-tdep.c

> > index 4cbf919db9..b3398f122a 100644

> > --- a/gdb/rx-tdep.c

> > +++ b/gdb/rx-tdep.c

> > @@ -33,11 +33,15 @@

> >  #include "value.h"

> >  #include "gdbcore.h"

> >  #include "dwarf2-frame.h"

> > +#include "remote.h"

> > +#include "target-descriptions.h"

> >  

> >  #include "elf/rx.h"

> >  #include "elf-bfd.h"

> >  #include <algorithm>

> >  

> > +#include "features/rx.c"

> > +

> >  /* Certain important register numbers.  */

> >  enum

> >  {

> > @@ -114,40 +118,21 @@ struct rx_prologue

> >    int reg_offset[RX_NUM_REGS];

> >  };

> >  

> > +static const char *const rx_register_names[] = {

> > +  "r0",  "r1",  "r2",  "r3",  "r4",  "r5",  "r6",  "r7",

> > +  "r8",  "r9",  "r10", "r11", "r12", "r13", "r14", "r15",

> > +  "usp", "isp", "psw", "pc",  "intb", "bpsw","bpc","fintv",

> > +  "fpsw", "acc",

> > +};

> 

> All file level variables and functions should have a header comment.


OK.
Add comment.

> > +

> >  /* Implement the "register_name" gdbarch method.  */

> >  static const char *

> >  rx_register_name (struct gdbarch *gdbarch, int regnr)

> >  {

> > -  static const char *const reg_names[] = {

> > -    "r0",

> > -    "r1",

> > -    "r2",

> > -    "r3",

> > -    "r4",

> > -    "r5",

> > -    "r6",

> > -    "r7",

> > -    "r8",

> > -    "r9",

> > -    "r10",

> > -    "r11",

> > -    "r12",

> > -    "r13",

> > -    "r14",

> > -    "r15",

> > -    "usp",

> > -    "isp",

> > -    "psw",

> > -    "pc",

> > -    "intb",

> > -    "bpsw",

> > -    "bpc",

> > -    "fintv",

> > -    "fpsw",

> > -    "acc"

> > -  };

> > -

> > -  return reg_names[regnr];

> > +  if (regnr >= 0 && regnr < RX_NUM_REGS)

> > +    return rx_register_names[regnr];

> > +  else

> > +    return NULL;

> >  }

> >  

> >  /* Construct the flags type for PSW and BPSW.  */

> > @@ -1037,6 +1022,14 @@ rx_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int reg)

> >      return -1;

> >  }

> >  

> > +static void

> > +rx_register_g_packet_guesses (struct gdbarch *gdbarch)

> > +{

> > +  register_remote_g_packet_guess (gdbarch,

> > +                                  4 * RX_NUM_REGS,

> > +                                  tdesc_rx);

> > +}

> 

> Header comment again.

> 

> > +

> >  /* Allocate and initialize a gdbarch object.  */

> >  static struct gdbarch *

> >  rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)

> > @@ -1044,6 +1037,8 @@ rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)

> >    struct gdbarch *gdbarch;

> >    struct gdbarch_tdep *tdep;

> >    int elf_flags;

> > +  struct tdesc_arch_data *tdesc_data = NULL;

> > +  const struct target_desc *tdesc = info.target_desc;

> >  

> >    /* Extract the elf_flags if available.  */

> >    if (info.abfd != NULL

> > @@ -1065,8 +1060,33 @@ rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)

> >        return arches->gdbarch;

> >      }

> >  

> > -  /* None found, create a new architecture from the information

> > -     provided.  */

> > +  if (tdesc == NULL)

> > +      tdesc = tdesc_rx;

> > +

> > +  /* Check any target description for validity.  */

> > +  if (tdesc_has_registers (tdesc))

> > +    {

> > +      const struct tdesc_feature *feature;

> > +      int valid_p = 0;

> 

> This should be a bool.


OK.

> > +      int i = 0;

> 

> This can move down into the if block scope.


OK.

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

> > +

> > +      if (feature != NULL)

> > +	{

> > +	  tdesc_data = tdesc_data_alloc ();

> > +	  valid_p = 1;

> > +	  for (i = 0; i < RX_NUM_REGS; i++)

> > +	    valid_p &= tdesc_numbered_register (feature, tdesc_data, i,

> > +                                            rx_register_names[i]);

> > +	}

> > +

> > +      if (!valid_p)

> > +	{

> > +	  tdesc_data_cleanup (tdesc_data);

> > +	  return NULL;

> > +	}

> > +    }

> > +

> >    tdep = XCNEW (struct gdbarch_tdep);

> >    gdbarch = gdbarch_alloc (&info, tdep);

> >    tdep->elf_flags = elf_flags;

> > @@ -1083,15 +1103,6 @@ rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)

> >    set_gdbarch_sw_breakpoint_from_kind (gdbarch, rx_breakpoint::bp_from_kind);

> >    set_gdbarch_skip_prologue (gdbarch, rx_skip_prologue);

> >  

> > -  /* Target builtin data types.  */

> > -  set_gdbarch_char_signed (gdbarch, 0);

> > -  set_gdbarch_short_bit (gdbarch, 16);

> > -  set_gdbarch_int_bit (gdbarch, 32);

> > -  set_gdbarch_long_bit (gdbarch, 32);

> > -  set_gdbarch_long_long_bit (gdbarch, 64);

> > -  set_gdbarch_ptr_bit (gdbarch, 32);

> > -  set_gdbarch_float_bit (gdbarch, 32);

> > -  set_gdbarch_float_format (gdbarch, floatformats_ieee_single);

> 

> I don't understand why these are being deleted.  This doesn't feel

> like it should be related to conversion to target descriptions.


OK. Revert it.

> >    if (elf_flags & E_FLAG_RX_64BIT_DOUBLES)

> >      {

> >        set_gdbarch_double_bit (gdbarch, 64);

> > @@ -1115,6 +1126,8 @@ rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)

> >    dwarf2_append_unwinders (gdbarch);

> >    frame_unwind_append_unwinder (gdbarch, &rx_frame_unwind);

> >  

> > +  rx_register_g_packet_guesses (gdbarch);

> > +

> >    /* Methods setting up a dummy call, and extracting the return value from

> >       a call.  */

> >    set_gdbarch_push_dummy_call (gdbarch, rx_push_dummy_call);

> > @@ -1123,6 +1136,9 @@ rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)

> >    /* Virtual tables.  */

> >    set_gdbarch_vbit_in_delta (gdbarch, 1);

> >  

> > +  if (tdesc_data != NULL)

> > +    tdesc_use_registers (gdbarch, tdesc, tdesc_data);

> 

> I'm confused by the 'tdesc_data != NULL' check here, my reading of

> things is that either:

> 

>   1. Target doesn't provide a target description, we fall back to the

>   built in tdesc_rx, this will be valid, tdesc_data will not be NULL.

> 

>   2. The target provides a description, but its invalid, we bail out

>   earlier after the validity check, we never reach this code.

> 

>   3. The target provides a description which is valid, tdesc_data will

>   not be NULL.

> 

> I'd prefer to see a 'gdb_assert (tdesc_data != NULL)' immediately

> after the earlier 'if (!valid_p)' block, and then after that just

> assume tdesc_data is not NULL.


OK. 

> Actually, while we're moving code around, is there any reason that

> this call to tdesc_use_registers couldn't move up to be just after the

> valid_p check?  Given its closely related code?


Since target-description can always be used, I moved to the front.

> > +

> >    return gdbarch;

> >  }

> >  

> > @@ -1132,4 +1148,5 @@ void

> >  _initialize_rx_tdep (void)

> >  {

> >    register_gdbarch_init (bfd_arch_rx, rx_gdbarch_init);

> > +  initialize_tdesc_rx ();

> >  }

> > -- 

> > 2.11.0

> > 

> 

> Thanks,

> Andrew


I'll sent v2 patch.
Thanks.

-- 
Yosinori Sato

Patch

diff --git a/gdb/features/Makefile b/gdb/features/Makefile
index 0c84faf405..2b65d46df0 100644
--- a/gdb/features/Makefile
+++ b/gdb/features/Makefile
@@ -161,6 +161,7 @@  XMLTOC = \
 	rs6000/powerpc-vsx64.xml \
 	rs6000/powerpc-vsx64l.xml \
 	rs6000/rs6000.xml \
+	rx.xml \
 	s390-linux32.xml \
 	s390-linux32v1.xml \
 	s390-linux32v2.xml \
@@ -238,6 +239,7 @@  FEATURE_XMLFILES = aarch64-core.xml \
 	riscv/64bit-cpu.xml \
 	riscv/64bit-csr.xml \
 	riscv/64bit-fpu.xml \
+	rx.xml \
 	tic6x-c6xp.xml \
 	tic6x-core.xml \
 	tic6x-gp.xml
diff --git a/gdb/features/rx.xml b/gdb/features/rx.xml
new file mode 100644
index 0000000000..b5aa9ac4a8
--- /dev/null
+++ b/gdb/features/rx.xml
@@ -0,0 +1,70 @@ 
+<?xml version="1.0"?>
+<!-- Copyright (C) 2019 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.rx.core">
+  <reg name="r0" bitsize="32" type="data_ptr"/>
+  <reg name="r1" bitsize="32" type="uint32"/>
+  <reg name="r2" bitsize="32" type="uint32"/>
+  <reg name="r3" bitsize="32" type="uint32"/>
+  <reg name="r4" bitsize="32" type="uint32"/>
+  <reg name="r5" bitsize="32" type="uint32"/>
+  <reg name="r6" bitsize="32" type="uint32"/>
+  <reg name="r7" bitsize="32" type="uint32"/>
+  <reg name="r8" bitsize="32" type="uint32"/>
+  <reg name="r9" bitsize="32" type="uint32"/>
+  <reg name="r10" bitsize="32" type="uint32"/>
+  <reg name="r11" bitsize="32" type="uint32"/>
+  <reg name="r12" bitsize="32" type="uint32"/>
+  <reg name="r13" bitsize="32" type="uint32"/>
+  <reg name="r14" bitsize="32" type="uint32"/>
+  <reg name="r15" bitsize="32" type="uint32"/>
+
+  <flags id="psw_flags" size="4">
+    <field name="C" start="0" end="0"/>
+    <field name="Z" start="1" end="1"/>
+    <field name="S" start="2" end="2"/>
+    <field name="O" start="3" end="3"/>
+    <field name="I" start="16" end="16"/>
+    <field name="U" start="17" end="17"/>
+    <field name="PM" start="20" end="20"/>
+    <field name="IPL" start="24" end="27"/>
+  </flags>
+
+  <flags id="fpsw_flags" size="4">
+    <field name="RM" start="0" end="1"/>
+    <field name="CV" start="2" end="2"/>
+    <field name="CO" start="3" end="3"/>
+    <field name="CZ" start="4" end="4"/>
+    <field name="CU" start="5" end="5"/>
+    <field name="CX" start="6" end="6"/>
+    <field name="CE" start="7" end="7"/>
+    <field name="DN" start="8" end="8"/>
+    <field name="EV" start="10" end="10"/>
+    <field name="EO" start="11" end="11"/>
+    <field name="EZ" start="12" end="12"/>
+    <field name="EU" start="13" end="13"/>
+    <field name="EX" start="14" end="14"/>
+    <field name="FV" start="26" end="26"/>
+    <field name="FO" start="27" end="27"/>
+    <field name="FZ" start="28" end="28"/>
+    <field name="FU" start="29" end="29"/>
+    <field name="FX" start="30" end="30"/>
+    <field name="FS" start="31" end="31"/>
+  </flags>
+
+  <reg name="usp" bitsize="32" type="data_ptr"/>
+  <reg name="isp" bitsize="32" type="data_ptr"/>
+  <reg name="psw" bitsize="32" type="psw_flags"/>
+  <reg name="pc" bitsize="32" type="code_ptr"/>
+  <reg name="intb" bitsize="32" type="data_ptr"/>
+  <reg name="bpsw" bitsize="32" type="psw_flags"/>
+  <reg name="bpc" bitsize="32" type="code_ptr"/>
+  <reg name="fintv" bitsize="32" type="code_ptr"/>
+  <reg name="fpsw" bitsize="32" type="fpsw_flags"/>
+  <reg name="acc" bitsize="64" type="uint64"/>
+</feature>
diff --git a/gdb/rx-tdep.c b/gdb/rx-tdep.c
index 4cbf919db9..b3398f122a 100644
--- a/gdb/rx-tdep.c
+++ b/gdb/rx-tdep.c
@@ -33,11 +33,15 @@ 
 #include "value.h"
 #include "gdbcore.h"
 #include "dwarf2-frame.h"
+#include "remote.h"
+#include "target-descriptions.h"
 
 #include "elf/rx.h"
 #include "elf-bfd.h"
 #include <algorithm>
 
+#include "features/rx.c"
+
 /* Certain important register numbers.  */
 enum
 {
@@ -114,40 +118,21 @@  struct rx_prologue
   int reg_offset[RX_NUM_REGS];
 };
 
+static const char *const rx_register_names[] = {
+  "r0",  "r1",  "r2",  "r3",  "r4",  "r5",  "r6",  "r7",
+  "r8",  "r9",  "r10", "r11", "r12", "r13", "r14", "r15",
+  "usp", "isp", "psw", "pc",  "intb", "bpsw","bpc","fintv",
+  "fpsw", "acc",
+};
+
 /* Implement the "register_name" gdbarch method.  */
 static const char *
 rx_register_name (struct gdbarch *gdbarch, int regnr)
 {
-  static const char *const reg_names[] = {
-    "r0",
-    "r1",
-    "r2",
-    "r3",
-    "r4",
-    "r5",
-    "r6",
-    "r7",
-    "r8",
-    "r9",
-    "r10",
-    "r11",
-    "r12",
-    "r13",
-    "r14",
-    "r15",
-    "usp",
-    "isp",
-    "psw",
-    "pc",
-    "intb",
-    "bpsw",
-    "bpc",
-    "fintv",
-    "fpsw",
-    "acc"
-  };
-
-  return reg_names[regnr];
+  if (regnr >= 0 && regnr < RX_NUM_REGS)
+    return rx_register_names[regnr];
+  else
+    return NULL;
 }
 
 /* Construct the flags type for PSW and BPSW.  */
@@ -1037,6 +1022,14 @@  rx_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int reg)
     return -1;
 }
 
+static void
+rx_register_g_packet_guesses (struct gdbarch *gdbarch)
+{
+  register_remote_g_packet_guess (gdbarch,
+                                  4 * RX_NUM_REGS,
+                                  tdesc_rx);
+}
+
 /* Allocate and initialize a gdbarch object.  */
 static struct gdbarch *
 rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
@@ -1044,6 +1037,8 @@  rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   struct gdbarch *gdbarch;
   struct gdbarch_tdep *tdep;
   int elf_flags;
+  struct tdesc_arch_data *tdesc_data = NULL;
+  const struct target_desc *tdesc = info.target_desc;
 
   /* Extract the elf_flags if available.  */
   if (info.abfd != NULL
@@ -1065,8 +1060,33 @@  rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
       return arches->gdbarch;
     }
 
-  /* None found, create a new architecture from the information
-     provided.  */
+  if (tdesc == NULL)
+      tdesc = tdesc_rx;
+
+  /* Check any target description for validity.  */
+  if (tdesc_has_registers (tdesc))
+    {
+      const struct tdesc_feature *feature;
+      int valid_p = 0;
+      int i = 0;
+      feature = tdesc_find_feature (tdesc, "org.gnu.gdb.rx.core");
+
+      if (feature != NULL)
+	{
+	  tdesc_data = tdesc_data_alloc ();
+	  valid_p = 1;
+	  for (i = 0; i < RX_NUM_REGS; i++)
+	    valid_p &= tdesc_numbered_register (feature, tdesc_data, i,
+                                            rx_register_names[i]);
+	}
+
+      if (!valid_p)
+	{
+	  tdesc_data_cleanup (tdesc_data);
+	  return NULL;
+	}
+    }
+
   tdep = XCNEW (struct gdbarch_tdep);
   gdbarch = gdbarch_alloc (&info, tdep);
   tdep->elf_flags = elf_flags;
@@ -1083,15 +1103,6 @@  rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_sw_breakpoint_from_kind (gdbarch, rx_breakpoint::bp_from_kind);
   set_gdbarch_skip_prologue (gdbarch, rx_skip_prologue);
 
-  /* Target builtin data types.  */
-  set_gdbarch_char_signed (gdbarch, 0);
-  set_gdbarch_short_bit (gdbarch, 16);
-  set_gdbarch_int_bit (gdbarch, 32);
-  set_gdbarch_long_bit (gdbarch, 32);
-  set_gdbarch_long_long_bit (gdbarch, 64);
-  set_gdbarch_ptr_bit (gdbarch, 32);
-  set_gdbarch_float_bit (gdbarch, 32);
-  set_gdbarch_float_format (gdbarch, floatformats_ieee_single);
   if (elf_flags & E_FLAG_RX_64BIT_DOUBLES)
     {
       set_gdbarch_double_bit (gdbarch, 64);
@@ -1115,6 +1126,8 @@  rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   dwarf2_append_unwinders (gdbarch);
   frame_unwind_append_unwinder (gdbarch, &rx_frame_unwind);
 
+  rx_register_g_packet_guesses (gdbarch);
+
   /* Methods setting up a dummy call, and extracting the return value from
      a call.  */
   set_gdbarch_push_dummy_call (gdbarch, rx_push_dummy_call);
@@ -1123,6 +1136,9 @@  rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   /* Virtual tables.  */
   set_gdbarch_vbit_in_delta (gdbarch, 1);
 
+  if (tdesc_data != NULL)
+    tdesc_use_registers (gdbarch, tdesc, tdesc_data);
+
   return gdbarch;
 }
 
@@ -1132,4 +1148,5 @@  void
 _initialize_rx_tdep (void)
 {
   register_gdbarch_init (bfd_arch_rx, rx_gdbarch_init);
+  initialize_tdesc_rx ();
 }