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 857F53858D35 for ; Thu, 30 Jul 2020 23:34:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 857F53858D35 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.11] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) (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 967CA1E792; Thu, 30 Jul 2020 19:34:30 -0400 (EDT) Subject: Re: [PATCH v4 1/3] arc: Add ARCv2 XML target along with refactoring To: Shahab Vahedi , gdb-patches@sourceware.org Cc: Shahab Vahedi , Tom Tromey , Anton Kolesov , Francois Bedard References: <20200713154527.13430-1-shahab.vahedi@gmail.com> <20200723193532.25812-1-shahab.vahedi@gmail.com> <20200723193532.25812-2-shahab.vahedi@gmail.com> From: Simon Marchi Message-ID: <7e15e727-9e95-7803-f81e-74108d1b669d@simark.ca> Date: Thu, 30 Jul 2020 19:34:23 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20200723193532.25812-2-shahab.vahedi@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.1 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP 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: Thu, 30 Jul 2020 23:34:33 -0000 I noted a few extra comments. The patch LGTM with those fixed. > @@ -1717,192 +1857,226 @@ static const struct frame_base arc_normal_base = { > arc_frame_base_address > }; > > -/* Initialize target description for the ARC. > - > - Returns TRUE if input tdesc was valid and in this case it will assign TDESC > - and TDESC_DATA output parameters. */ > - > -static bool > -arc_tdesc_init (struct gdbarch_info info, const struct target_desc **tdesc, > - struct tdesc_arch_data **tdesc_data) > +static enum arc_isa > +mach_type_to_arc_isa (const unsigned long mach) > { > - if (arc_debug) > - debug_printf ("arc: Target description initialization.\n"); > - > - const struct target_desc *tdesc_loc = info.target_desc; > + switch (mach) > + { > + case bfd_mach_arc_arc600: > + case bfd_mach_arc_arc601: > + case bfd_mach_arc_arc700: > + return ARC_ISA_ARCV1; > + break; > + case bfd_mach_arc_arcv2: > + return ARC_ISA_ARCV2; > + break; Remove the "break"s. > + default: > + internal_error (__FILE__, __LINE__, > + _("unknown machine id %lu"), mach); > + } > +} > > - /* Depending on whether this is ARCompact or ARCv2 we will assign > - different default registers sets (which will differ in exactly two core > - registers). GDB will also refuse to accept register feature from invalid > - ISA - v2 features can be used only with v2 ARChitecture. We read > - bfd_arch_info, which looks like to be a safe bet here, as it looks like it > - is always initialized even when we don't pass any elf file to GDB at all > - (it uses default arch in this case). Also GDB will call this function > - multiple times, and if XML target description file contains architecture > - specifications, then GDB will set this architecture to info.bfd_arch_info, > - overriding value from ELF file if they are different. That means that, > - where matters, this value is always our best guess on what CPU we are > - debugging. It has been noted that architecture specified in tdesc file > - has higher precedence over ELF and even "set architecture" - that is, > - using "set architecture" command will have no effect when tdesc has "arch" > - tag. */ > - /* Cannot use arc_mach_is_arcv2 (), because gdbarch is not created yet. */ > - const int is_arcv2 = (info.bfd_arch_info->mach == bfd_mach_arc_arcv2); > - bool is_reduced_rf; > - const char *const *core_regs; > - const char *core_feature_name; > +/* Common construction code for ARC_GDBARCH_FEATURES struct. If there > + is no ABFD, then a FEATURE with default values is returned. */ > > - /* If target doesn't provide a description, use the default ones. */ > - if (!tdesc_has_registers (tdesc_loc)) > +static arc_gdbarch_features > +arc_gdbarch_features_create (const bfd *abfd, const unsigned long mach) > +{ > + /* Use 4 as a fallback value. */ > + int reg_size = 4; > + > + /* Try to guess the features parameters by looking at the binary to be > + executed. If the user is providing a binary that does not match the > + target, then tough luck. This is the last effort to makes sense of > + what's going on. */ > + if (abfd != NULL && bfd_get_flavour (abfd) == bfd_target_elf_flavour) NULL -> nullptr, at least for new code. > { > - if (is_arcv2) > - tdesc_loc = arc_read_description (ARC_SYS_TYPE_ARCV2); > + unsigned char eclass = elf_elfheader (abfd)->e_ident[EI_CLASS]; > + > + if (eclass == ELFCLASS32) > + reg_size = 4; > + else if (eclass == ELFCLASS64) > + reg_size = 8; > else > - tdesc_loc = arc_read_description (ARC_SYS_TYPE_ARCOMPACT); > + internal_error (__FILE__, __LINE__, > + _("unknown ELF header class %d"), eclass); > } > - else > + > + /* MACH from a bfd_arch_info struct is used here. It should be a safe > + bet, as it looks like the struct is always initialized even when we > + don't pass any elf file to GDB at all (it uses default arch in that > + case). */ > + arc_isa isa = mach_type_to_arc_isa (mach); > + > + return arc_gdbarch_features (reg_size, isa); > +} > + > +/* Based on the MACH value, determines which core register features set > + must be used. Possible outcomes: > + {nullptr, &arc_v1_core_reg_feature, &arc_v2_core_reg_feature} */ > + > +static arc_register_feature * > +determine_core_reg_feature_set (const unsigned long mach) > +{ > + switch (mach_type_to_arc_isa (mach)) > { > - if (arc_debug) > - debug_printf ("arc: Using provided register set.\n"); > + case ARC_ISA_ARCV1: > + return &arc_v1_core_reg_feature; > + case ARC_ISA_ARCV2: > + return &arc_v2_core_reg_feature; > + default: > + return nullptr; I think this should be gdb_assert_not_reached, since it's not possible for mach_type_to_arc_isa to return another value. The contract of this function should be that it can't return nullptr, and then the callers can assume that the return value is always != nullptr. > } > - gdb_assert (tdesc_loc != NULL); > - > - /* Now we can search for base registers. Core registers can be either full > - or reduced. Summary: > - > - - core.v2 + aux-minimal > - - core-reduced.v2 + aux-minimal > - - core.arcompact + aux-minimal > - > - NB: It is entirely feasible to have ARCompact with reduced core regs, but > - we ignore that because GCC doesn't support that and at the same time > - ARCompact is considered obsolete, so there is not much reason to support > - that. */ > - const struct tdesc_feature *feature > - = tdesc_find_feature (tdesc_loc, core_v2_feature_name); > - if (feature != NULL) > - { > - /* Confirm that register and architecture match, to prevent accidents in > - some situations. This code will trigger an error if: > +} > > - 1. XML tdesc doesn't specify arch explicitly, registers are for arch > - X, but ELF specifies arch Y. > +/* At the moment, there is only 1 auxiliary register features set. > + This is a place holder for future extendability. */ > > - 2. XML tdesc specifies arch X, but contains registers for arch Y. > +static const arc_register_feature * > +determine_aux_reg_feature_set () > +{ > + return &arc_common_aux_reg_feature; > +} > > - It will not protect from case where XML or ELF specify arch X, > - registers are for the same arch X, but the real target is arch Y. To > - detect this case we need to check IDENTITY register. */ > - if (!is_arcv2) > - { > - arc_print (_("Error: ARC v2 target description supplied for " > - "non-ARCv2 target.\n")); > - return false; > - } > +/* Update accumulator register names (ACCH/ACCL) for r58 and r59 in the > + register sets. The endianness determines the assignment: > > - is_reduced_rf = false; > - core_feature_name = core_v2_feature_name; > - core_regs = core_v2_register_names; > - } > - else > + ,------.------. > + | LE | BE | > + ,-----|------+------| > + | r58 | accl | acch | > + | r59 | acch | accl | > + `-----^------^------' */ > + > +static void > +arc_update_acc_reg_names (const int byte_order) > +{ > + const char *r58_alias > + = byte_order == BFD_ENDIAN_LITTLE ? "accl" : "acch"; > + const char *r59_alias > + = byte_order == BFD_ENDIAN_LITTLE ? "acch" : "accl"; > + > + /* Subscript 1 must be OK because those registers have 2 names. */ > + arc_v1_core_reg_feature.registers[ARC_R58_REGNUM].names[1] = r58_alias; > + arc_v1_core_reg_feature.registers[ARC_R59_REGNUM].names[1] = r59_alias; > + arc_v2_core_reg_feature.registers[ARC_R58_REGNUM].names[1] = r58_alias; > + arc_v2_core_reg_feature.registers[ARC_R59_REGNUM].names[1] = r59_alias; > +} > + > +/* Go through all the registers in REG_SET and check if they exist > + in FEATURE. The TDESC_DATA is updated with the register number > + in REG_SET if it is found in the feature. If a required register > + is not found, this function returns false. */ > + > +static bool > +arc_check_tdesc_feature (struct tdesc_arch_data *tdesc_data, > + const struct tdesc_feature *feature, > + const struct arc_register_feature *reg_set) > +{ > + for (const auto ® : reg_set->registers) > { > - feature = tdesc_find_feature (tdesc_loc, core_reduced_v2_feature_name); > - if (feature != NULL) > + bool found = false; > + > + for (const char *name : reg.names) > { > - if (!is_arcv2) > - { > - arc_print (_("Error: ARC v2 target description supplied for " > - "non-ARCv2 target.\n")); > - return false; > - } > + found = > + The assignment operator goes on the next line: found = tdesc_numbered_register (feature, tdesc_data, reg.regnum, name); > @@ -2133,36 +2307,6 @@ dump_arc_instruction_command (const char *args, int from_tty) > > /* See arc-tdep.h. */ This comment is now dangling. > > -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_NUM)); > - 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 != NULL ? arch : ""; > - abi = abi != NULL ? abi : ""; > - debug_printf ("arc: Created target description for " > - "\"%s\": arch=\"%s\", ABI=\"%s\"\n", > - arc_sys_type_to_str (sys_type), arch, abi); > - } > - } > - > - return tdesc; > -} > - > void _initialize_arc_tdep (); > void > _initialize_arc_tdep () > diff --git a/gdb/arc-tdep.h b/gdb/arc-tdep.h > index d72332c7638..6ca759a661d 100644 > --- a/gdb/arc-tdep.h > +++ b/gdb/arc-tdep.h > @@ -35,7 +35,6 @@ enum arc_regnum > { > /* Core registers. */ > ARC_R0_REGNUM = 0, > - ARC_FIRST_CORE_REGNUM = ARC_R0_REGNUM, > ARC_R1_REGNUM = 1, > ARC_R4_REGNUM = 4, > ARC_R7_REGNUM = 7, > @@ -54,6 +53,9 @@ enum arc_regnum > ARC_R30_REGNUM, > /* Return address from function. */ > ARC_BLINK_REGNUM, > + /* Accumulator registers. */ > + ARC_R58_REGNUM = 58, > + ARC_R59_REGNUM, > /* Zero-delay loop counter. */ > ARC_LP_COUNT_REGNUM = 60, > /* Reserved register number. There should never be a register with such > @@ -69,14 +71,21 @@ enum arc_regnum > /* Program counter, aligned to 4-bytes, read-only. */ > ARC_PCL_REGNUM, > ARC_LAST_CORE_REGNUM = ARC_PCL_REGNUM, > + > /* AUX registers. */ > /* Actual program counter. */ > ARC_PC_REGNUM, > ARC_FIRST_AUX_REGNUM = ARC_PC_REGNUM, > /* Status register. */ > ARC_STATUS32_REGNUM, > - ARC_LAST_REGNUM = ARC_STATUS32_REGNUM, > - ARC_LAST_AUX_REGNUM = ARC_STATUS32_REGNUM, > + /* Zero-delay loop start instruction. */ > + ARC_LP_START_REGNUM, > + /* Zero-delay loop next-after-last instruction. */ > + ARC_LP_END_REGNUM, > + /* Branch target address. */ > + ARC_BTA_REGNUM, > + ARC_LAST_AUX_REGNUM = ARC_BTA_REGNUM, > + ARC_LAST_REGNUM = ARC_LAST_AUX_REGNUM, > > /* Additional ABI constants. */ > ARC_FIRST_ARG_REGNUM = ARC_R0_REGNUM, > @@ -164,7 +173,4 @@ 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 > index 9552b4aff97..585d1d881eb 100644 > --- a/gdb/arch/arc.c > +++ b/gdb/arch/arc.c > @@ -17,42 +17,106 @@ > > > #include "gdbsupport/common-defs.h" > -#include > - > #include "arc.h" > +#include > +#include > +#include > > /* 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" > +#include "features/arc/v1-core.c" > +#include "features/arc/v1-aux.c" > +#include "features/arc/v2-core.c" > +#include "features/arc/v2-aux.c" > > -/* See arc.h. */ > +#ifndef GDBSERVER > +#define STATIC_IN_GDB static > +#else > +#define STATIC_IN_GDB > +#endif > > -target_desc * > -arc_create_target_description (arc_sys_type sys_type) > +STATIC_IN_GDB target_desc * > +arc_create_target_description (const struct arc_gdbarch_features &features) > { > + /* Create a new target description. */ > 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 > + std::string arch_name; > > - if (sys_type == ARC_SYS_TYPE_ARCV2) > + /* Architecture names here must match the ones in > + ARCH_INFO_STRUCT in bfd/cpu-arc.c. */ > + if (features.isa == ARC_ISA_ARCV1 && features.reg_size == 4) > + arch_name = "arc:ARC700"; > + else if (features.isa == ARC_ISA_ARCV2 && features.reg_size == 4) > + arch_name = "arc:ARCv2"; > + else > { > - regnum = create_feature_arc_core_v2 (tdesc, regnum); > - regnum = create_feature_arc_aux_v2 (tdesc, regnum); > + std::string msg = string_printf > + ("Cannot determine architecture: ISA=%d; bitness=%d", > + features.isa, 8 * features.reg_size); > + gdb_assert_not_reached (msg.c_str ()); > } > - else > + > + set_tdesc_architecture (tdesc, arch_name.c_str ()); > +#endif > + > + long regnum = 0; > + > + switch (features.isa) > { > - regnum = create_feature_arc_core_arcompact (tdesc, regnum); > - regnum = create_feature_arc_aux_arcompact (tdesc, regnum); > + case ARC_ISA_ARCV1: > + regnum = create_feature_arc_v1_core (tdesc, regnum); > + regnum = create_feature_arc_v1_aux (tdesc, regnum); > + break; > + case ARC_ISA_ARCV2: > + regnum = create_feature_arc_v2_core (tdesc, regnum); > + regnum = create_feature_arc_v2_aux (tdesc, regnum); > + break; > + default: > + std::string msg = string_printf > + ("Cannot choose target description XML: %d", features.isa); > + gdb_assert_not_reached (msg.c_str ()); The code in the cases here has two too many indentation level. > } > > return tdesc; > } > + > +#ifndef GDBSERVER > + > +/* Wrapper used by std::unordered_map to generate hash for features set. */ > +struct arc_gdbarch_features_hasher > +{ > + std::size_t > + operator() (const arc_gdbarch_features &features) const noexcept > + { > + return features.hash (); > + } > +}; > + > +/* Cache of previously created target descriptions, indexed by the hash > + of the features set used to create them. */ > +static std::unordered_map + const target_desc_up, > + arc_gdbarch_features_hasher> arc_tdesc_cache; > + > +/* See arch/arc.h. */ > + > +const target_desc * > +arc_lookup_target_description (const struct arc_gdbarch_features &features) > +{ > + /* Lookup in the cache first. If found, return the pointer from the > + "target_desc_up" type which is a "unique_ptr". This should be fine > + as the "arc_tdesc_cache" will persist until GDB terminates. */ > + const auto it = arc_tdesc_cache.find (features); > + if (it != arc_tdesc_cache.end ()) > + return it->second.get (); > + > + target_desc *tdesc = arc_create_target_description (features); > + > + /* Add the newly created target description to the repertoire. */ Nice use of the word "repertoire" :). > + arc_tdesc_cache.emplace (features, tdesc); > + > + return tdesc; > +} > + > +#endif /* !GDBSERVER */ > diff --git a/gdb/arch/arc.h b/gdb/arch/arc.h > index fd806ae7d34..4f01d92bf09 100644 > --- a/gdb/arch/arc.h > +++ b/gdb/arch/arc.h > @@ -20,29 +20,68 @@ > > #include "gdbsupport/tdesc.h" > > -/* Supported ARC system hardware types. */ > -enum arc_sys_type > +/* Supported ARC ISAs. */ > +enum arc_isa > { > - ARC_SYS_TYPE_ARCOMPACT = 0, /* ARC600 or ARC700 */ > - ARC_SYS_TYPE_ARCV2, /* ARC EM or ARC HS */ > - ARC_SYS_TYPE_NUM > + ARC_ISA_ARCV1 = 1, /* a.k.a. ARCompact (ARC600, ARC700) */ > + ARC_ISA_ARCV2 /* such as ARC EM and ARC HS */ > }; > > -static inline const char * > -arc_sys_type_to_str (const arc_sys_type type) > +struct arc_gdbarch_features > { > - switch (type) > - { > - case ARC_SYS_TYPE_ARCOMPACT: > - return "ARC_SYS_TYPE_ARCOMPACT"; > - case ARC_SYS_TYPE_ARCV2: > - return "ARC_SYS_TYPE_ARCV2"; > - default: > - return "Invalid"; > - } > -} > - > -/* Create target description for the specified system type. */ > -target_desc *arc_create_target_description (arc_sys_type sys_type); > + arc_gdbarch_features (int reg_size, arc_isa isa) > + : reg_size (reg_size), isa (isa) > + {} > + > + /* Register size in bytes. Possible values are 4, and 8. A 0 indicates > + an uninitialised value. */ > + int reg_size; > + > + /* See ARC_ISA enum. */ > + arc_isa isa; I think these two fields could/should be const. Simon