From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22910 invoked by alias); 21 Aug 2019 13:31:06 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 22902 invoked by uid 89); 21 Aug 2019 13:31:06 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_LOW,SPF_SOFTFAIL autolearn=ham version=3.3.1 spammy= X-HELO: mail02.asahi-net.or.jp Received: from mail02.asahi-net.or.jp (HELO mail02.asahi-net.or.jp) (202.224.55.14) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 21 Aug 2019 13:31:03 +0000 Received: from h61-195-96-97.vps.ablenet.jp (h61-195-96-97.ablenetvps.ne.jp [61.195.96.97]) (Authenticated sender: PQ4Y-STU) by mail02.asahi-net.or.jp (Postfix) with ESMTPA id 650105B655; Wed, 21 Aug 2019 22:31:00 +0900 (JST) Received: from yo-satoh-debian.ysato.ml (ae227063.dynamic.ppp.asahi-net.or.jp [14.3.227.63]) by h61-195-96-97.vps.ablenet.jp (Postfix) with ESMTPSA id 82E19240085; Wed, 21 Aug 2019 22:30:59 +0900 (JST) Date: Wed, 21 Aug 2019 13:31:00 -0000 Message-ID: <87y2zmk5m4.wl-ysato@users.sourceforge.jp> From: Yoshinori Sato To: Andrew Burgess Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] RX: Convert target-description In-Reply-To: <20190821092432.GE6076@embecosm.com> References: <20190821033301.45309-1-ysato@users.sourceforge.jp> <20190821092432.GE6076@embecosm.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL/10.8 EasyPG/1.0.0 Emacs/25.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-IsSubscribed: yes X-SW-Source: 2019-08/txt/msg00478.txt.bz2 On Wed, 21 Aug 2019 18:24:33 +0900, Andrew Burgess wrote: > > Thanks for the patch. > > * Yoshinori Sato [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 > > > > * 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 @@ > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > 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 > > > > +#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