From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <gdb-patches-return-133638-listarch-gdb-patches=sources.redhat.com@sourceware.org>
Received: (qmail 80093 invoked by alias); 4 Oct 2016 16:56:59 -0000
Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm
Precedence: bulk
List-Id: <gdb-patches.sourceware.org>
List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org>
List-Archive: <http://sourceware.org/ml/gdb-patches/>
List-Post: <mailto:gdb-patches@sourceware.org>
List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs>
Sender: gdb-patches-owner@sourceware.org
Received: (qmail 80037 invoked by uid 89); 4 Oct 2016 16:56:55 -0000
Authentication-Results: sourceware.org; auth=none
X-Virus-Found: No
X-Spam-SWARE-Status: No, score=-4.5 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=promptly, usp, HTo:D*jp, psw
X-HELO: mx1.redhat.com
Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 04 Oct 2016 16:56:45 +0000
Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23])	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))	(No client certificate requested)	by mx1.redhat.com (Postfix) with ESMTPS id 4D714C04D301;	Tue,  4 Oct 2016 16:56:44 +0000 (UTC)
Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11])	by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u94GugTW023239;	Tue, 4 Oct 2016 12:56:43 -0400
Subject: Re: [PATCH][RX] v2 CPU register support.
To: Yoshinori Sato <ysato@users.sourceforge.jp>, gdb-patches@sourceware.org
References: <1450719388-16232-1-git-send-email-ysato@users.sourceforge.jp>
From: Pedro Alves <palves@redhat.com>
Message-ID: <fe6bf430-7ed3-eb1c-a98a-68d84c9b6d33@redhat.com>
Date: Tue, 04 Oct 2016 16:56:00 -0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0
MIME-Version: 1.0
In-Reply-To: <1450719388-16232-1-git-send-email-ysato@users.sourceforge.jp>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
X-SW-Source: 2016-10/txt/msg00049.txt.bz2

Hi Yoshinori,

Looks like this fell through the cracks.  Very sorry about that.
Better late and never, I hope!

On 12/21/2015 05:36 PM, Yoshinori Sato wrote:
> gdb/ChangeLog
> 	* features/rx.xml: New. v1 regsiter define.
> 	* features/rx.c: New.
> 	* features/rxv2.xml: New. v2 regsiter define.

Typo "regsiter".

> 	* features/rxv2.c: New.
> 	* rx-tdep.c: include register define.

Uppercase "Include".

> 	(RX_ACC_NUM): Delete.
> 	(RX_V1_NUM_REGS): New. num of v1 regster.
> 	(RX_V2_NUM_REGS): New. num of v2 regster.

Uppercase "Num".  Typo "regster".

> 	(RX_NUM_REGS): v2 register support.
> 	(rx_reg_names): New.
> 	(rx_v2_reg_names): New.
> 	(rx_register_name): Use target description.
> 	(rxv2_register_name): New.
> 	(rx_register_type): Use target description.
> 	(rxv2_register_type): New.
> 	(rx_analyze_prologue): Use RX_V2_NUM_REGS.
> 	(rx_gdbarch_init): Use target description.
> 	(initalize_rx_tdep): Add target description initialize.
> 
> ---
>  gdb/features/rx.c     |  78 +++++++++++++++++++++
>  gdb/features/rx.xml   |  70 +++++++++++++++++++
>  gdb/features/rxv2.c   |  80 ++++++++++++++++++++++
>  gdb/features/rxv2.xml |  72 +++++++++++++++++++
>  gdb/rx-tdep.c         | 186 ++++++++++++++++++++++++++------------------------
>  5 files changed, 397 insertions(+), 89 deletions(-)
>  create mode 100644 gdb/features/rx.c
>  create mode 100644 gdb/features/rx.xml
>  create mode 100644 gdb/features/rxv2.c
>  create mode 100644 gdb/features/rxv2.xml
> 
> diff --git a/gdb/features/rx.c b/gdb/features/rx.c
> new file mode 100644
> index 0000000..937e840
> --- /dev/null
> +++ b/gdb/features/rx.c
> @@ -0,0 +1,78 @@
> +/* THIS FILE IS GENERATED.  -*- buffer-read-only: t -*- vi:set ro:
> +  Original: rx.xml */

There's been changes in the generator code meanwhile.  I think
you'll need to regenerate these.  Hopefully that won't be much
trouble.

> +++ b/gdb/features/rx.xml
> @@ -0,0 +1,70 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2015 Free Software Foundation, Inc.

Needs to be 2015-2016 now.

> +

>  
> +#define RX_NUM_REGS(arch) ((gdbarch_tdep(arch)->elf_flags & E_FLAG_RX_V2)? \
> +			   RX_V2_NUM_REGS:RX_V1_NUM_REGS)

Spaces around ? and :.

> +
> +static const char *const rx_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"
> +  };
> +
> +static const char *const rxv2_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","acc0","extb","acc1"
> +  };
> +
>  /* Implement the "register_name" gdbarch method.  */
>  static const char *
> -rx_register_name (struct gdbarch *gdbarch, int regnr)
> +rx_register_name (struct gdbarch *gdbarch, int reg_nr)
>  {
> -  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"
> -  };
> +  if (reg_nr < RX_V1_NUM_REGS)
> +    return rx_reg_names[reg_nr];
> +  else
> +    return tdesc_register_name (gdbarch, reg_nr);

I think you should prefer the tdesc's register names if
the tdesc as registers.  See tic6x_register_name for
example.  Not sure you can get here without a tdesc, even,
given the fallback tdescs in place.

> +}
>  
> -  return reg_names[regnr];
> +static const char *
> +rxv2_register_name (struct gdbarch *gdbarch, int reg_nr)
> +{
> +  if (reg_nr < RX_V2_NUM_REGS)
> +    return rxv2_reg_names[reg_nr];
> +  else
> +    return tdesc_register_name (gdbarch, reg_nr);
>  }
>  

Likewise.

>  /* Implement the "register_type" gdbarch method.  */
>  static struct type *
>  rx_register_type (struct gdbarch *gdbarch, int reg_nr)
>  {
> -  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> -
> -  if (reg_nr == RX_PC_REGNUM)
> -    return builtin_type (gdbarch)->builtin_func_ptr;
> -  else if (reg_nr == RX_PSW_REGNUM || reg_nr == RX_BPSW_REGNUM)
> -    return tdep->rx_psw_type;
> -  else if (reg_nr == RX_FPSW_REGNUM)
> -    return tdep->rx_fpsw_type;
> -  else if (reg_nr == RX_ACC_REGNUM)
> -    return builtin_type (gdbarch)->builtin_unsigned_long_long;
> -  else
> -    return builtin_type (gdbarch)->builtin_unsigned_long;
> +  return tdesc_register_type (gdbarch, reg_nr);
>  }
>  
> +static struct type *
> +rxv2_register_type (struct gdbarch *gdbarch, int reg_nr)
> +{
> +  return tdesc_register_type (gdbarch, reg_nr);
> +}

Here it seems like you're assuming a tdesc is available, which
gives more weight to my comment above.

>  
>  /* Function for finding saved registers in a 'struct pv_area'; this
>     function is passed to pv_area_scan.
> @@ -225,7 +222,7 @@ rx_analyze_prologue (CORE_ADDR start_pc, CORE_ADDR limit_pc,
>  {
>    CORE_ADDR pc, next_pc;
>    int rn;
> -  pv_t reg[RX_NUM_REGS];
> +  pv_t reg[RX_V2_NUM_REGS];
>    struct pv_area *stack;
>    struct cleanup *back_to;
>    CORE_ADDR after_last_frame_setup_insn = start_pc;
> @@ -234,7 +231,7 @@ rx_analyze_prologue (CORE_ADDR start_pc, CORE_ADDR limit_pc,
>  
>    result->frame_type = frame_type;
>  
> -  for (rn = 0; rn < RX_NUM_REGS; rn++)
> +  for (rn = 0; rn < RX_V2_NUM_REGS; rn++)
>      {
>        reg[rn] = pv_register (rn, 0);
>        result->reg_offset[rn] = 1;

Will these loops still do the right thing for v1?

> @@ -1021,6 +1018,10 @@ rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>    struct gdbarch *gdbarch;
>    struct gdbarch_tdep *tdep;
>    int elf_flags;
> +  int register_bytes, i;
> +  int numregs;
> +  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
> @@ -1028,7 +1029,40 @@ rx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>      elf_flags = elf_elfheader (info.abfd)->e_flags;
>    else
>      elf_flags = 0;
> +  if (!tdesc_has_registers (tdesc))
> +    /* Pick a default target description.  */
> +    tdesc = (info.bfd_arch_info->mach == bfd_mach_rx)?tdesc_rx:tdesc_rxv2;

Spaces around ? and :.  Won't info.bfd_arch_info be NULL if you have
no program loaded in gdb at all?

>  
> +  numregs = (info.bfd_arch_info->mach == bfd_mach_rx)?RX_V1_NUM_REGS:RX_V2_NUM_REGS;

Spaces.

> +
> +  /* Check any target description for validity.  */
> +  if (tdesc_has_registers (tdesc))
> +    {
> +      const struct tdesc_feature *feature;
> +      int valid_p;
> +      const char * const *reg;
> +
> +      if (info.bfd_arch_info->mach == bfd_mach_rx)
> +	feature = tdesc_find_feature (tdesc, "org.gnu.gdb.rx");
> +      else
> +	feature = tdesc_find_feature (tdesc, "org.gnu.gdb.rx.v2");
> +      if (feature == NULL)
> +	return NULL;
> +
> +      tdesc_data = tdesc_data_alloc ();
> +
> +      valid_p = 1;
> +
> +      reg = (info.bfd_arch_info->mach == bfd_mach_rx)?rx_reg_names:rxv2_reg_names;

Spaces.

> +      for (i = 0; i < numregs; i++)
> +	valid_p &= tdesc_numbered_register (feature, tdesc_data, i,
> +					    reg[i]);
> +      if (!valid_p)
> +	{
> +	  tdesc_data_cleanup (tdesc_data);
> +	  return NULL;
> +	}
> +    }
>  
>    /* Try to find the architecture in the list of already defined
>       architectures.  */

We're missing a change to the manual to document the new standard
target description features:

  https://sourceware.org/gdb/onlinedocs/gdb/Standard-Target-Features.html#Standard-Target-Features

along with a NEWS entry.

Other that that, this looks pretty close to being ready.  Thanks for the
v2, and again, sorry for the long delay.  Going through a target description
makes it possible for the remote stub to describe random non-standard registers
(like e.g., i/o control registers) and gdb display them, all without further
changes to gdb, so I think it's well worth it to extra initial effort.

Please repost a fixed patch with a (concise, self-contained) rationale for
the change, as per [1], and I'll try to review it promptly.  The idea is that
the commit log is an integral part of the patch that ends up pushed to git.

[1] - https://sourceware.org/gdb/wiki/ContributionChecklist

Thanks,
Pedro Alves