Re: [PATCH v3] Convert the RX target to make use of target descriptions.

[Simon Marchi <simark@simark.ca>]

Hi Yoshinori,

I looked at the patch because I was curious about RX.  I found a few formatting nits that you might want to fix before pushing.

On 2019-08-22 10:30 a.m., Yoshinori Sato wrote:
> gdb/ChangeLog
> 
> 2019-08-22  Yoshinori Sato <ysato@users.sourceforge.jp>
> 
> 	* gdb/rx-tdep.c (rx_register_names): New.
> 	(rx_register_name): delete.
> 	(rx_psw_type): delete.
> 	(rx_fpsw_type): delete.
> 	(rx_register_type): delete.

Use capital letters, "Delete.".

> 	(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/rx.c: Likewise.
> 	* gdb/NEWS: Mention target description support.
> 
> gdb/doc/ChangeLog:
> 
> 2019-08-22  Yoshinori Sato <ysato@users.sourceforge.jp>
> 
> 	* gdb.texinfo (Standard Target Features): Add RX Features sub-section.
> ---
>  gdb/NEWS              |   2 +
>  gdb/doc/gdb.texinfo   |  10 ++++
>  gdb/features/Makefile |   2 +
>  gdb/features/rx.c     |  76 +++++++++++++++++++++++
>  gdb/features/rx.xml   |  70 ++++++++++++++++++++++
>  gdb/rx-tdep.c         | 162 +++++++++++++++-----------------------------------
>  6 files changed, 207 insertions(+), 115 deletions(-)
>  create mode 100644 gdb/features/rx.c
>  create mode 100644 gdb/features/rx.xml
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 462247f486..dc32e10a2c 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -27,6 +27,8 @@
>    provide the exitcode or exit status of the shell commands launched by
>    GDB commands such as "shell", "pipe" and "make".
>  
> +* The RX port now supports XML target descriptions.
> +
>  * Python API
>  
>    ** The gdb.Value type has a new method 'format_string' which returns a
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 523e3d0bfe..ad70807953 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -44068,6 +44068,7 @@ registers using the capitalization used in the description.
>  * OpenRISC 1000 Features::
>  * PowerPC Features::
>  * RISC-V Features::
> +* RX Features::
>  * S/390 and System z Features::
>  * Sparc Features::
>  * TIC6x Features::
> @@ -44498,6 +44499,15 @@ target has floating point hardware, but can be moved into the csr
>  feature if the target has the floating point control registers, but no
>  other floating point hardware.
>  
> +@node RX Features
> +@subsection RX Features
> +@cindex target descriptions, RX Features
> +
> +The @samp{org.gnu.gdb.rx.core} feature is required for RX
> +targets.  It should contain the registers @samp{r0} through
> +@samp{r15}, @samp{usp}, @samp{isp}, @samp{psw}, @samp{pc}, @samp{intb},
> +@samp{bpsw}, @samp{bpc}, @samp{fintv}, @samp{fpsw}, and @samp{acc}.
> +
>  @node S/390 and System z Features
>  @subsection S/390 and System z Features
>  @cindex target descriptions, S/390 features
> 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.c b/gdb/features/rx.c
> new file mode 100644
> index 0000000000..8144103e48
> --- /dev/null
> +++ b/gdb/features/rx.c
> @@ -0,0 +1,76 @@
> +/* THIS FILE IS GENERATED.  -*- buffer-read-only: t -*- vi:set ro:
> +  Original: rx.xml.tmp */
> +
> +#include "defs.h"
> +#include "osabi.h"
> +#include "target-descriptions.h"
> +
> +struct target_desc *tdesc_rx;
> +static void
> +initialize_tdesc_rx (void)
> +{
> +  struct target_desc *result = allocate_target_description ();
> +  struct tdesc_feature *feature;
> +
> +  feature = tdesc_create_feature (result, "org.gnu.gdb.rx.core");
> +  tdesc_type_with_fields *type_with_fields;
> +  type_with_fields = tdesc_create_flags (feature, "psw_flags", 4);
> +  tdesc_add_flag (type_with_fields, 0, "C");
> +  tdesc_add_flag (type_with_fields, 1, "Z");
> +  tdesc_add_flag (type_with_fields, 2, "S");
> +  tdesc_add_flag (type_with_fields, 3, "O");
> +  tdesc_add_flag (type_with_fields, 16, "I");
> +  tdesc_add_flag (type_with_fields, 17, "U");
> +  tdesc_add_flag (type_with_fields, 20, "PM");
> +  tdesc_add_bitfield (type_with_fields, "IPL", 24, 27);

The old code (done by hand) did:

      append_flags_type_flag (tdep->rx_psw_type, 24, "IPL0");
      append_flags_type_flag (tdep->rx_psw_type, 25, "IPL1");
      append_flags_type_flag (tdep->rx_psw_type, 26, "IPL2");
      append_flags_type_flag (tdep->rx_psw_type, 27, "IPL3");

Is the result identical for the end user?

> @@ -1065,16 +967,44 @@ 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;
> +      bool valid_p = true;
> +
> +      feature = tdesc_find_feature (tdesc, "org.gnu.gdb.rx.core");
> +
> +      if (feature != NULL)
> +	{
> +	  int i;

Declare `i` in the for loop.

> +
> +	  tdesc_data = tdesc_data_alloc ();
> +	  for (i = 0; i < RX_NUM_REGS; i++)
> +	    valid_p &= tdesc_numbered_register (feature, tdesc_data, i,
> +                                            rx_register_names[i]);
> +	}
> +
> +      if (valid_p == false)

Use !valid_p.

> +	{
> +	  tdesc_data_cleanup (tdesc_data);
> +	  return NULL;
> +	}
> +    }
> +
> +  gdb_assert(tdesc_data != NULL);

Missing space before parenthesis.

Thanks,

Simon