Re: [PATCH v2 1/4] arc: Add XML target features for Linux targets

[Simon Marchi <simark@simark.ca>]

On 2020-04-28 12:04 p.m., Shahab Vahedi via Gdb-patches wrote:
> From: Anton Kolesov <Anton.Kolesov@synopsys.com>
> 
> Add XML target features for Linux targets.  Compared to default
> Linux features:
> 
> - Explicitly specify CPU machine.
> - Remove baremetal only ILINK{,1,2} registers.
> - Add LP_START and LP_END registers for hardware loops - required to
>   properly evaluate possible next instruction during software single
>   instruction stepping.
> - Add BTA register which contains branch target address - address of
>   next instruction when processor is in the delay slot.
> - ARC HS description also adds R30, R58 and R59 registers, specific to
>   this architecture.
> 
> gdb/ChangeLog:
> 2020-04-28  Anton Kolesov  <anton.kolesov@synopsys.com>
> 
> 	* arch/arc.h (arc_create_target_description): Support Linux targets.
> 	* arch/arc.c (arc_create_target_description): Likewise.
> 	* arc-tdep.c (arc_tdesc_init): Update invocation of
> 	  arc_read_description.
> 	* features/Makefile (FEATURE_XMLFILES): Add new files.
> 	* features/arc/aux-arcompact-linux.xml: New file.
> 	* features/arc/aux-v2-linux.xml: Likewise.
> 	* features/arc/core-arcompact-linux.xml: Likewise.
> 	* features/arc/core-v2-linux.xml: Likewise.
> 	* features/arc/aux-arcompact-linux.c: Generate.
> 	* features/arc/aux-v2-linux.c: Likewise.
> 	* features/arc/core-arcompact-linux.c: Likewise.
> 	* features/arc/core-v2-linux.c: Likewise.

Hi Shahab,

Instead of having multiple almost identical features, it's now usual
to build small features and compose them into the full target description.

For example, instead of having both:

- core-arcompact.xml
- core-arcompact-linux.xml

I think you should have:

- core-arcompact.xml, containing everything that's common in the two above
- core-arcompact-baremetal.xml, containing only the registers that are only present
  in the baremetal version.

When building the target description, the pseudo code would look like:

  if (arcompact)
    create_core_arcompact_feature ();
  else
    create_core_v2_feature ();

  if (baremetal) // or if (!linux)
    {
      if (arcompact)
        create_core_arccompact_baremetal_feature ();
      else
        create_core_v2_baremetal_feature ();
    }

Same goes for aux.  The i386 and arm target descriptions have examples of how to do
that.

Note that I don't know if the order of the registers in the target description
matters...  if so it could make this more difficult.

> @@ -37,21 +41,39 @@ arc_create_target_description (arc_sys_type sys_type)
>    long regnum = 0;
>  
>  #ifndef IN_PROCESS_AGENT
> -  if (sys_type == ARC_SYS_TYPE_ARCV2)
> +  if (sys_type == ARC_SYS_TYPE_ARCV2_BMT)
>      set_tdesc_architecture (tdesc, "arc:ARCv2");
> +  else if (sys_type == ARC_SYS_TYPE_ARCV2_LNX)
> +    /* If this is ARCv2 Linux, then it is ARC HS.  */
> +    set_tdesc_architecture (tdesc, "arc:HS");
>    else
>      set_tdesc_architecture (tdesc, "arc:ARC700");
> +
> +  if (sys_type == ARC_SYS_TYPE_ARCOMPACT_LNX
> +      || sys_type == ARC_SYS_TYPE_ARCV2_LNX)
> +    set_tdesc_osabi (tdesc, "GNU/Linux");
>  #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
> +  switch (sys_type)
>      {
> -      regnum = create_feature_arc_core_arcompact (tdesc, regnum);
> -      regnum = create_feature_arc_aux_arcompact (tdesc, regnum);
> +    case ARC_SYS_TYPE_ARCOMPACT_BMT:
> +	  regnum = create_feature_arc_core_arcompact (tdesc, regnum);
> +	  regnum = create_feature_arc_aux_arcompact (tdesc, regnum);
> +	  break;
> +    case ARC_SYS_TYPE_ARCOMPACT_LNX:
> +	  regnum = create_feature_arc_core_arcompact_linux (tdesc, regnum);
> +	  regnum = create_feature_arc_aux_arcompact_linux (tdesc, regnum);
> +	  break;
> +    case ARC_SYS_TYPE_ARCV2_BMT:
> +	  regnum = create_feature_arc_core_v2 (tdesc, regnum);
> +	  regnum = create_feature_arc_aux_v2 (tdesc, regnum);
> +	  break;
> +    case ARC_SYS_TYPE_ARCV2_LNX:
> +	  regnum = create_feature_arc_core_v2_linux (tdesc, regnum);
> +	  regnum = create_feature_arc_aux_v2_linux (tdesc, regnum);
> +	  break;
> +    default:
> +	  gdb_assert(!"Invalid arc_sys_type.");
>      }

The goal of having descriptions built at runtime with features is to avoid
this kind of combinatorial explosion.  Now, you have two axis of configuration
(arcompact vs arcv2, bare-metal vs linux).  If you had more, it would become
less and less practical.  That's why I would encourage you to consider what I
said above.

>  
>    return tdesc;
> diff --git a/gdb/arch/arc.h b/gdb/arch/arc.h
> index fd806ae7d34..3c19118b946 100644
> --- a/gdb/arch/arc.h
> +++ b/gdb/arch/arc.h
> @@ -23,8 +23,10 @@
>  /* Supported ARC system hardware types.  */
>  enum arc_sys_type
>  {
> -  ARC_SYS_TYPE_ARCOMPACT = 0,	  /* ARC600 or ARC700 */
> -  ARC_SYS_TYPE_ARCV2,		  /* ARC EM or ARC HS */
> +  ARC_SYS_TYPE_ARCOMPACT_BMT = 0, /* ARC600 or ARC700 (baremetal) */
> +  ARC_SYS_TYPE_ARCOMPACT_LNX,	  /* ARC600 or ARC700 (linux) */
> +  ARC_SYS_TYPE_ARCV2_BMT,	  /* ARC EM or ARC HS (baremetal) */
> +  ARC_SYS_TYPE_ARCV2_LNX,	  /* ARC HS (linux) */
>    ARC_SYS_TYPE_NUM

That's also not expected.  The fact that the target system is running Linux
should be handled by the fact that the selected osabi will be Linux.  So
you shouldn't need any changes here.

Simon