* [PATCH] RX: Convert target-description @ 2019-08-21 3:33 Yoshinori Sato 2019-08-21 9:24 ` Andrew Burgess 0 siblings, 1 reply; 3+ messages in thread From: Yoshinori Sato @ 2019-08-21 3:33 UTC (permalink / raw) To: gdb-patches; +Cc: Yoshinori Sato 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 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 (); } -- 2.11.0 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] RX: Convert target-description 2019-08-21 3:33 [PATCH] RX: Convert target-description Yoshinori Sato @ 2019-08-21 9:24 ` Andrew Burgess 2019-08-21 13:31 ` Yoshinori Sato 0 siblings, 1 reply; 3+ messages in thread From: Andrew Burgess @ 2019-08-21 9:24 UTC (permalink / raw) To: Yoshinori Sato; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] RX: Convert target-description 2019-08-21 9:24 ` Andrew Burgess @ 2019-08-21 13:31 ` Yoshinori Sato 0 siblings, 0 replies; 3+ messages in thread From: Yoshinori Sato @ 2019-08-21 13:31 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-08-21 13:31 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-21 3:33 [PATCH] RX: Convert target-description Yoshinori Sato 2019-08-21 9:24 ` Andrew Burgess 2019-08-21 13:31 ` Yoshinori Sato
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).