From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 1CF4E3945C1E for ; Fri, 13 Mar 2020 13:49:07 +0000 (GMT) Received: from [10.0.0.11] (unknown [192.222.164.54]) (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 027381E598; Fri, 13 Mar 2020 09:49:05 -0400 (EDT) Subject: Re: [PATCH v3] 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> <20200313103712.5458-1-shahab.vahedi@gmail.com> From: Simon Marchi Message-ID: <45b47c96-ea73-78f1-ff61-277be5d1c2c9@simark.ca> Date: Fri, 13 Mar 2020 09:49:05 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: <20200313103712.5458-1-shahab.vahedi@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US-large Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS, SPF_PASS autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Mar 2020 13:49:08 -0000 On 2020-03-13 6:37 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. > > v2: > Removed arc.c from ALLDEPFILES in gdb/Makefile.in. > Removed vim modeline from arc-tdep.c to have it in a separate patch. > Removed braces from one line "if/else". > Undid the type change for "jb_pc" (kept it as "int"). > Joined the unnecessary line breaks into one line. > No more moving around arm targets in gdb/features/Makefile. > > v3: > Added include gaurds to arc.h. > Added arc_read_description to _create_ target descriptions less. Hi Shahab, I put some comments, they are all very minor, so the patch LGTM with those addressed. No need to send another version unless some other problem comes up. > @@ -2145,15 +2139,44 @@ dump_arc_instruction_command (const char *args, int from_tty) > arc_insn_dump (insn); > } > > +/* See arc-tdep.h. */ > + > +const target_desc * > +arc_read_description (arc_sys_type sys_type) > +{ > + if (arc_debug) > + debug_printf ("arc: Reading target description for \"%s\".\n", > + arc_sys_type_to_str (sys_type)); > + > + gdb_assert ((sys_type > 0) && (sys_type < ARC_SYS_TYPE_INVALID)); > + struct target_desc *tdesc = tdesc_arc_list[sys_type]; > + > + if (tdesc == nullptr) > + { > + tdesc = arc_create_target_description (sys_type); > + tdesc_arc_list[sys_type] = tdesc; > + > + if (arc_debug) > + { > + const char *arch = tdesc_architecture_name (tdesc); > + const char *abi = tdesc_osabi_name (tdesc); > + arch = arch ? arch : ""; > + abi = abi ? abi : ""; When comparing pointers, we always use explicit comparisons to NULL or nullptr. So here, it should be: arch = arch != nullptr ? arch : ""; > + debug_printf ("arc: Created target description for " > + "\"%s\": arch=\"%s\", ABI=\"%s\"\n", > + arc_sys_type_to_str (sys_type), arch, abi); > + } This block is missing an indentation level. > + } > + > + return tdesc; > +} > + > void _initialize_arc_tdep (); > void > _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. */ > diff --git a/gdb/arc-tdep.h b/gdb/arc-tdep.h > index bd0096741ff..d72332c7638 100644 > --- a/gdb/arc-tdep.h > +++ b/gdb/arc-tdep.h > @@ -23,6 +23,7 @@ > > /* Need disassemble_info. */ > #include "dis-asm.h" > +#include "arch/arc.h" > > /* To simplify GDB code this enum assumes that internal regnums should be same > as architectural register numbers, i.e. PCL regnum is 63. This allows to > @@ -163,4 +164,7 @@ CORE_ADDR arc_insn_get_branch_target (const struct arc_instruction &insn); > > CORE_ADDR arc_insn_get_linear_next_pc (const struct arc_instruction &insn); > > +/* Get the correct ARC target description for the given system type. */ > +const target_desc *arc_read_description (arc_sys_type sys_type); > + > #endif /* ARC_TDEP_H */ > diff --git a/gdb/arch/arc.c b/gdb/arch/arc.c > new file mode 100644 > index 00000000000..9552b4aff97 > --- /dev/null > +++ b/gdb/arch/arc.c > @@ -0,0 +1,58 @@ > +/* 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" > + > +/* See arc.h. */ > + > +target_desc * > +arc_create_target_description (arc_sys_type sys_type) > +{ > + target_desc *tdesc = allocate_target_description (); > + > + long regnum = 0; > + > +#ifndef IN_PROCESS_AGENT > + if (sys_type == ARC_SYS_TYPE_ARCV2) > + set_tdesc_architecture (tdesc, "arc:ARCv2"); > + else > + set_tdesc_architecture (tdesc, "arc:ARC700"); > +#endif > + > + if (sys_type == ARC_SYS_TYPE_ARCV2) > + { > + regnum = create_feature_arc_core_v2 (tdesc, regnum); > + regnum = create_feature_arc_aux_v2 (tdesc, regnum); > + } > + else > + { > + regnum = create_feature_arc_core_arcompact (tdesc, regnum); > + regnum = create_feature_arc_aux_arcompact (tdesc, regnum); > + } > + > + return tdesc; > +} > diff --git a/gdb/arch/arc.h b/gdb/arch/arc.h > new file mode 100644 > index 00000000000..0cb5a845ac1 > --- /dev/null > +++ b/gdb/arch/arc.h > @@ -0,0 +1,41 @@ > +/* 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 . */ > + > +#ifndef ARCH_ARC_H > +#define ARCH_ARC_H > + > +#include "gdbsupport/tdesc.h" > + > +/* Supported ARC system hardware types. */ > +enum arc_sys_type { Curly brace on next line. > + ARC_SYS_TYPE_NONE = 0, Is NONE really needed? > + ARC_SYS_TYPE_ARCOMPACT, /* ARC600 or ARC700 */ > + ARC_SYS_TYPE_ARCV2, /* ARC EM or ARC HS */ > + ARC_SYS_TYPE_INVALID Perhaps call the last one ARC_SYS_TYPE_NUM, since you use it to get the number of enumerators, to size the array of target descriptions. > +}; > + > +#define arc_sys_type_to_str(x) \ > + (x == ARC_SYS_TYPE_NONE ? "ARC_SYS_TYPE_NONE" : \ > + (x == ARC_SYS_TYPE_ARCOMPACT ? "ARC_SYS_TYPE_ARCOMPACT" : \ > + (x == ARC_SYS_TYPE_ARCV2 ? "ARC_SYS_TYPE_ARCV2" : \ > + (x == ARC_SYS_TYPE_INVALID ? "ARC_SYS_TYPE_INVALID" : \ > + "Unknown")))) Can you please make this a function instead? It's more readable of maintainable. static inline const char * arc_sys_type_to_str (arc_sys_type type) { switch (type) { case ARC_SYS_TYPE_NONE: return "ARC_SYS_TYPE_NONE"; ... } } Simon