Re: [PATCH v4 1/3] arc: Add ARCv2 XML target along with refactoring

Simon Marchi
2020-07-30T23:34:31Z

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 : 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 <stdlib.h>
> -
> #include "arc.h"
> +#include <stdlib.h>
> +#include <unordered_map>
> +#include <string>
>
> /* 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<arc_gdbarch_features,
> + 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