[PATCH v3] arc: Migrate to new target features

[Simon Marchi]

On 2020-03-13 6:37 a.m., Shahab Vahedi wrote:
> From: Anton Kolesov <Anton.Kolesov@synopsys.com>
> 
> 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 <http://www.gnu.org/licenses/>.  */
> +
> +
> +#include "gdbsupport/common-defs.h"
> +#include <stdlib.h>
> +
> +#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 <http://www.gnu.org/licenses/>.  */
> +
> +#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