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

[Simon Marchi <simark@simark.ca>]

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