[PATCH][RX] v2 CPU register support.
Pedro Alves
palves@redhat.com
Tue Oct 4 16:56:00 GMT 2016
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
More information about the Gdb-patches
mailing list