From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 106664 invoked by alias); 5 Mar 2020 23:06:58 -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 106655 invoked by uid 89); 5 Mar 2020 23:06:56 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-17.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 spammy=presume, motivated X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 05 Mar 2020 23:06:54 +0000 Received: from [172.16.0.95] (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id CA87B1E4B2; Thu, 5 Mar 2020 18:06:52 -0500 (EST) Subject: Re: [PATCH] arc: Migrate to new target features To: Shahab Vahedi , gdb-patches@sourceware.org Cc: Shahab Vahedi , Anton Kolesov , Francois Bedard References: <20200305160552.29193-1-shahab.vahedi@gmail.com> From: Simon Marchi Message-ID: Date: Thu, 05 Mar 2020 23:06:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <20200305160552.29193-1-shahab.vahedi@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2020-03/txt/msg00134.txt On 2020-03-05 11:05 a.m., Shahab Vahedi wrote: > From: Anton Kolesov > > This patch replaces usage of target descriptions in ARC, where the whole > description is fixed in XML, with new target descriptions where XML describes > individual features, and GDB assembles those features into actual target > description. > > gdb/ChangeLog: > 2017-10-25 Anton Kolesov > Shahab Vahedi > > * Makefile.in: Add arch/arc.o > * configure.tgt: Likewise. > * arc-tdep.h: Change type of "jb_pc" to CORE_ADDR. > * arc-tdep.c (arc_tdesc_init): Use arc_create_target_description. > (_initialize_arc_tdep): Don't initialize old target descriptions. > (arc_dump_tdep): Use "%li" to print "jb_pc". > * arch/arc.c: New file. > * arch/arc.h: Likewise. > * features/Makefile: Replace old target descriptions with new. > * features/arc-arcompact.c: Remove. > * features/arc-arcompact.xml: Likewise. > * features/arc-v2.c: Likewise > * features/arc-v2.xml: Likewise > * features/arc/aux-arcompact.xml: New file. > * features/arc/aux-v2.xml: Likewise. > * features/arc/core-arcompact.xml: Likewise. > * features/arc/core-v2.xml: Likewise. > * features/arc/aux-arcompact.c: Generate. > * features/arc/aux-v2.c: Likewise. > * features/arc/core-arcompact.c: Likewise. > * features/arc/core-v2.c: Likewise. > * target-descriptions (maint_print_c_tdesc_cmd): Support ARC features. Hi Shahab, > @@ -2119,6 +2121,7 @@ ALLDEPFILES = \ > amd64-obsd-tdep.c \ > amd64-sol2-tdep.c \ > amd64-tdep.c \ > + arc.c \ That looks wrong, there is no gdb/arc.c file. The arm.c entry below looks wrong as well... but at least put the right path to arc.c. > @@ -1753,18 +1752,7 @@ arc_tdesc_init (struct gdbarch_info info, const struct target_desc **tdesc, > /* If target doesn't provide a description - use default one. */ > if (!tdesc_has_registers (tdesc_loc)) > { > - if (is_arcv2) > - { > - tdesc_loc = tdesc_arc_v2; > - if (arc_debug) > - debug_printf ("arc: Using default register set for ARC v2.\n"); > - } > - else > - { > - tdesc_loc = tdesc_arc_arcompact; > - if (arc_debug) > - debug_printf ("arc: Using default register set for ARCompact.\n"); > - } > + tdesc_loc = arc_create_target_description (arc_debug, is_arcv2); > } Remove braces here. > else > { > @@ -2114,7 +2102,7 @@ arc_dump_tdep (struct gdbarch *gdbarch, struct ui_file *file) > { > struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > > - fprintf_unfiltered (file, "arc_dump_tdep: jb_pc = %i\n", tdep->jb_pc); > + fprintf_unfiltered (file, "arc_dump_tdep: jb_pc = %li\n", tdep->jb_pc); Printing a CORE_ADDR should be done using the paddress function, so that it works fine regardless of the host machine type. > } > > /* Wrapper for "maintenance print arc" list of commands. */ > @@ -2151,9 +2139,6 @@ _initialize_arc_tdep () > { > gdbarch_register (bfd_arch_arc, arc_gdbarch_init, arc_dump_tdep); > > - initialize_tdesc_arc_v2 (); > - initialize_tdesc_arc_arcompact (); > - > /* Register ARC-specific commands with gdb. */ > > /* Add root prefix command for "maintenance print arc" commands. */ > @@ -2176,3 +2161,5 @@ _initialize_arc_tdep () > _("Non-zero enables ARC specific debugging."), > NULL, NULL, &setdebuglist, &showdebuglist); > } > + > +/* vim: set sts=2 shiftwidth=2 ts=8: */ That change shouldn't be included in the patch. I would welcome some auto configuration for vim, however, like we have for emacs (.dir-locals.el). Please propose this as a separate patch. > diff --git a/gdb/arc-tdep.h b/gdb/arc-tdep.h > index bd0096741ff..3f2f5449722 100644 > --- a/gdb/arc-tdep.h > +++ b/gdb/arc-tdep.h > @@ -99,7 +99,7 @@ struct gdbarch_tdep > { > /* Offset to PC value in jump buffer. If this is negative, longjmp > support will be disabled. */ > - int jb_pc; > + CORE_ADDR jb_pc; Just wondering what motivates this change. Since this changes a signed type to an unsigned one, and a negative value has a special meaning, did you check all uses to make sure this is fine? For example, this use in arc_gdbarch_init: if (tdep->jb_pc >= 0) Isn't it always true now? If the change is not tied to the target description changes, please make a separate patch for that. If it is, then it's fine to include it here. In both cases, please explain what motivated you to change it. Finally, since the comment above says "negative" but a CORE_ADDR can't really be negative, the comment should be updated. The CORE_ADDR_MAX value (which is equivalent to the -1 used currently) could maybe be used instead? > }; > > /* Utility functions used by other ARC-specific modules. */ > diff --git a/gdb/arch/arc.c b/gdb/arch/arc.c > new file mode 100644 > index 00000000000..effa6ec3d6f > --- /dev/null > +++ b/gdb/arch/arc.c > @@ -0,0 +1,70 @@ > +/* Copyright (C) 2017-2020 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > + > +#include "gdbsupport/common-defs.h" > +#include > + > +#include "arc.h" > + > +/* Target description features. */ > +#include "features/arc/core-v2.c" > +#include "features/arc/aux-v2.c" > +#include "features/arc/core-arcompact.c" > +#include "features/arc/aux-arcompact.c" > + > +/* Create target description for a target with specified parameters. > + PRINT_DEBUG defines whether to print debug messages to the stderr stream. > + IS_ARCV2 defines if this is an ARCv2 (ARC EM or ARC HS) target or ARCompact > + (ARC600 or ARC700); there is no use for more specific information about > + target processor. */ > + > +target_desc * > +arc_create_target_description (bool print_debug, bool is_arcv2) > +{ > + target_desc *tdesc = allocate_target_description (); > + > + if (print_debug) > + debug_printf ("arc: Creating target description, " > + "is_arcv2=%i\n", is_arcv2); I think the last statement fits within 80 column without wrapping. > + > + long regnum = 0; > + > +#ifndef IN_PROCESS_AGENT > + if (is_arcv2) > + { > + set_tdesc_architecture (tdesc, "arc:ARCv2"); > + } > + else > + { > + set_tdesc_architecture (tdesc, "arc:ARC700"); > + } Remove braces above. > diff --git a/gdb/features/Makefile b/gdb/features/Makefile > index 9a98b0542c4..60be548780d 100644 > --- a/gdb/features/Makefile > +++ b/gdb/features/Makefile > @@ -108,8 +108,14 @@ OUTPUTS = $(patsubst %,$(outdir)/%.dat,$(WHICH)) > # --enable-targets=all GDB. You can override this by passing XMLTOC > # to make on the command line. > XMLTOC = \ > - arc-v2.xml \ > - arc-arcompact.xml \ > + aarch64.xml \ > + arm/arm-with-iwmmxt.xml \ > + arm/arm-with-m-fpa-layout.xml \ > + arm/arm-with-m-vfp-d16.xml \ > + arm/arm-with-m.xml \ > + arm/arm-with-neon.xml \ > + arm/arm-with-vfpv2.xml \ > + arm/arm-with-vfpv3.xml \ > microblaze-with-stack-protect.xml \ > microblaze.xml \ > mips-dsp-linux.xml \ > @@ -203,16 +209,11 @@ $(outdir)/%.dat: %.xml number-regs.xsl sort-regs.xsl gdbserver-regs.xsl > > # For targets with feature based target descriptions, > # the set of xml files we'll generate .c files for GDB from. > -FEATURE_XMLFILES = aarch64-core.xml \ > - aarch64-fpu.xml \ > - aarch64-pauth.xml \ > - arm/arm-core.xml \ > - arm/arm-fpa.xml \ > - arm/arm-m-profile.xml \ > - arm/arm-m-profile-with-fpa.xml \ > - arm/arm-vfpv2.xml \ > - arm/arm-vfpv3.xml \ > - arm/xscale-iwmmxt.xml \ > +FEATURE_XMLFILES = \ > + arc/core-v2.xml \ > + arc/aux-v2.xml \ > + arc/core-arcompact.xml \ > + arc/aux-arcompact.xml \ Why are all these arm files moving? I presume there was a bad merge. > diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c > index 04711ba2fa5..7909fee7d5c 100644 > --- a/gdb/target-descriptions.c > +++ b/gdb/target-descriptions.c > @@ -1715,7 +1715,9 @@ maint_print_c_tdesc_cmd (const char *args, int from_tty) > || startswith (filename_after_features.c_str (), "riscv/") > || startswith (filename_after_features.c_str (), "tic6x-") > || startswith (filename_after_features.c_str (), "aarch64") > - || startswith (filename_after_features.c_str (), "arm/")) > + || startswith (filename_after_features.c_str (), "arm/") > + || startswith (filename_after_features.c_str (), "arc/core-") > + || startswith (filename_after_features.c_str (), "arc/aux-")) Might as well just compare to "arc/" ? Simon