Re: [PATCH 7/10, GAS/ARM] Remove ARMv6S-M special casing

["Richard Earnshaw (lists)" <Richard dot Earnshaw at arm dot com>]

On 21/06/17 11:13, Thomas Preudhomme wrote:
> Hi,
> 
> === Context ===
> 
> This patch is part of a patch series to add support for ARMv8-R
> architecture. Its purpose is to remove special casing for ARMv6S-M
> autodetection.
> 
> === Motivation ===
> 
> Currently, SWI and SVC mnemonics are enabled for ARMv4T and successor
> architectures with extra checks in the handler function (do_t_swi) to
> give an error message when ARMv6-M is targeted and some more special
> casing in aeabi_set_public_attributes. This was made to exclude these
> mnemonics for ARMv6-M unless the OS extension is in use.
> 
> However this logic is superfluous: there is already code to check
> whether an instruction is available based on the feature bit it is part
> of and whether the targeted architecture has that feature bit. This
> patch aims at removing that unneeded complexity.
> 
> === Patch description ===
> 
> The OS extension is already limited to the ARMv6-M architecture so all
> this patch does is redefined availability of the ARM_EXT_OS feature bit
> to not be present for ARM_ARCH_V6M. ARM_ARCH_V6SM does not need any
> change either because it already includes ARM_EXT_OS.
> 
> The patch also make sure that the error message that was given by
> do_t_swi when SWI/SVC is unavailable is still the same by detecting the
> situation in md_assemble. It also adjusts pr12198-2.d which only
> contains instructions available in all ARMv7 profiles (A, R and M).
> 
> ChangeLog entries are as follow:
> 
> *** gas/ChangeLog ***
> 
> 2017-01-26  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> 
>     * config/tc-arm.c (arm_ext_v6m): Delete.
>     (arm_ext_v7m): Delete.
>     (arm_ext_m): Remove ARM_EXT_OS from the set of feature defined M
>     profile.
>     (arm_arch_v6m_only): Delete.
>     (do_t_swi): Remove special case for ARMv6S-M.
>     (md_assemble): Display error message previously in do_t_swi when
>     SVC is not available.
>     (insns): Guard swi and svc by arm_ext_os for Thumb mode.
>     (aeabi_set_public_attributes): Remove special case for ARMv6S-M.
>     * testsuite/gas/arm/pr12198-2.d: Fix expectation.
> 
> *** include/ChangeLog ***
> 
> 2017-02-02  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> 
>     * opcode/arm.h (ARM_AEXT_V4TxM): Add ARM_EXT_OS bit to the set.
>     (ARM_AEXT_V4T): Likewise.
>     (ARM_AEXT_V5TxM): Likewise.
>     (ARM_AEXT_V5T): Likewise.
>     (ARM_AEXT_V6M): Mask off ARM_EXT_OS bit.
> 
> === Testing ===
> 
> Testsuite shows no regression when run for arm-none-eabi targets.
> 
> Is this ok for master branch?
> 

ok.

r.

> Best regards,
> 
> Thomas
> 
> 07_remove_armv8s-m_special_casing.patch
> 
> 
> diff --git a/gas/config/tc-arm.c b/gas/config/tc-arm.c
> index 769debeba25e8da255495b6e9124f8a09b054203..9932f383fff8ac53006aa688323c37955d47e868 100644
> --- a/gas/config/tc-arm.c
> +++ b/gas/config/tc-arm.c
> @@ -189,7 +189,6 @@ static const arm_feature_set arm_ext_v5j = ARM_FEATURE_CORE_LOW (ARM_EXT_V5J);
>  static const arm_feature_set arm_ext_v6 = ARM_FEATURE_CORE_LOW (ARM_EXT_V6);
>  static const arm_feature_set arm_ext_v6k = ARM_FEATURE_CORE_LOW (ARM_EXT_V6K);
>  static const arm_feature_set arm_ext_v6t2 = ARM_FEATURE_CORE_LOW (ARM_EXT_V6T2);
> -static const arm_feature_set arm_ext_v6m = ARM_FEATURE_CORE_LOW (ARM_EXT_V6M);
>  static const arm_feature_set arm_ext_v6_notm =
>    ARM_FEATURE_CORE_LOW (ARM_EXT_V6_NOTM);
>  static const arm_feature_set arm_ext_v6_dsp =
> @@ -202,12 +201,9 @@ static const arm_feature_set arm_ext_div = ARM_FEATURE_CORE_LOW (ARM_EXT_DIV);
>  static const arm_feature_set arm_ext_v7 = ARM_FEATURE_CORE_LOW (ARM_EXT_V7);
>  static const arm_feature_set arm_ext_v7a = ARM_FEATURE_CORE_LOW (ARM_EXT_V7A);
>  static const arm_feature_set arm_ext_v7r = ARM_FEATURE_CORE_LOW (ARM_EXT_V7R);
> -#ifdef OBJ_ELF
> -static const arm_feature_set arm_ext_v7m = ARM_FEATURE_CORE_LOW (ARM_EXT_V7M);
> -#endif
>  static const arm_feature_set arm_ext_v8 = ARM_FEATURE_CORE_LOW (ARM_EXT_V8);
>  static const arm_feature_set arm_ext_m =
> -  ARM_FEATURE_CORE (ARM_EXT_V6M | ARM_EXT_OS | ARM_EXT_V7M,
> +  ARM_FEATURE_CORE (ARM_EXT_V6M | ARM_EXT_V7M,
>  		    ARM_EXT2_V8M | ARM_EXT2_V8M_MAIN);
>  static const arm_feature_set arm_ext_mp = ARM_FEATURE_CORE_LOW (ARM_EXT_MP);
>  static const arm_feature_set arm_ext_sec = ARM_FEATURE_CORE_LOW (ARM_EXT_SEC);
> @@ -244,9 +240,6 @@ static const arm_feature_set fpu_any = FPU_ANY;
>  static const arm_feature_set arm_arch_full ATTRIBUTE_UNUSED = ARM_FEATURE (-1, -1, -1);
>  static const arm_feature_set arm_arch_t2 = ARM_ARCH_THUMB2;
>  static const arm_feature_set arm_arch_none = ARM_ARCH_NONE;
> -#ifdef OBJ_ELF
> -static const arm_feature_set arm_arch_v6m_only = ARM_ARCH_V6M_ONLY;
> -#endif
>  
>  static const arm_feature_set arm_cext_iwmmxt2 =
>    ARM_FEATURE_COPROC (ARM_CEXT_IWMMXT2);
> @@ -13089,17 +13082,6 @@ do_t_sxth (void)
>  static void
>  do_t_swi (void)
>  {
> -  /* We have to do the following check manually as ARM_EXT_OS only applies
> -     to ARM_EXT_V6M.  */
> -  if (ARM_CPU_HAS_FEATURE (cpu_variant, arm_ext_v6m))
> -    {
> -      if (!ARM_CPU_HAS_FEATURE (cpu_variant, arm_ext_os)
> -	  /* This only applies to the v6m however, not later architectures.  */
> -	  && ! ARM_CPU_HAS_FEATURE (cpu_variant, arm_ext_v7))
> -	as_bad (_("SVC is not permitted on this architecture"));
> -      ARM_MERGE_FEATURE_SETS (thumb_arch_used, thumb_arch_used, arm_ext_os);
> -    }
> -
>    inst.reloc.type = BFD_RELOC_ARM_SWI;
>  }
>  
> @@ -18439,7 +18421,10 @@ md_assemble (char *str)
>  	  || (thumb_mode == 1
>  	      && !ARM_CPU_HAS_FEATURE (variant, *opcode->tvariant)))
>  	{
> -	  as_bad (_("selected processor does not support `%s' in Thumb mode"), str);
> +	  if (opcode->tencode == do_t_swi)
> +	    as_bad (_("SVC is not permitted on this architecture"));
> +	  else
> +	    as_bad (_("selected processor does not support `%s' in Thumb mode"), str);
>  	  return;
>  	}
>        if (inst.cond != COND_ALWAYS && !unified_syntax
> @@ -19263,8 +19248,6 @@ static const struct asm_opcode insns[] =
>   tC3("ldmia",	8900000, _ldmia,    2, (RRw, REGLST), ldmstm, t_ldmstm),
>   tC3("ldmfd",	8900000, _ldmia,    2, (RRw, REGLST), ldmstm, t_ldmstm),
>  
> - TCE("swi",	f000000, df00,     1, (EXPi),        swi, t_swi),
> - TCE("svc",	f000000, df00,     1, (EXPi),        swi, t_swi),
>   tCE("b",	a000000, _b,	   1, (EXPr),	     branch, t_branch),
>   TCE("bl",	b000000, f000f800, 1, (EXPr),	     bl, t_branch23),
>  
> @@ -19292,6 +19275,12 @@ static const struct asm_opcode insns[] =
>   TCE("rsb",	0600000, ebc00000, 3, (RR, oRR, SH), arit, t_rsb),
>   TC3("rsbs",	0700000, ebd00000, 3, (RR, oRR, SH), arit, t_rsb),
>  
> +#undef THUMB_VARIANT
> +#define THUMB_VARIANT  & arm_ext_os
> +
> + TCE("swi",	f000000, df00,     1, (EXPi),        swi, t_swi),
> + TCE("svc",	f000000, df00,     1, (EXPi),        swi, t_swi),
> +
>  #undef  THUMB_VARIANT
>  #define THUMB_VARIANT  & arm_ext_v6
>  
> @@ -26694,13 +26683,6 @@ aeabi_set_public_attributes (void)
>        if (ARM_CPU_HAS_FEATURE (thumb_arch_used, arm_arch_any))
>  	ARM_MERGE_FEATURE_SETS (flags, flags, arm_ext_v4t);
>  
> -      /* We need to make sure that the attributes do not identify us as v6S-M
> -	 when the only v6S-M feature in use is the Operating System
> -	 Extensions.  */
> -      if (ARM_CPU_HAS_FEATURE (flags, arm_ext_os))
> -	if (!ARM_CPU_HAS_FEATURE (flags, arm_arch_v6m_only))
> -	  ARM_CLEAR_FEATURE (flags, flags, arm_ext_os);
> -
>        /* Code run during relaxation relies on selected_cpu being set.  */
>        selected_cpu = flags;
>      }
> diff --git a/gas/testsuite/gas/arm/pr12198-2.d b/gas/testsuite/gas/arm/pr12198-2.d
> index c3ac2b0499c0bb6aec03d6631a472bb29151f280..0eb76c439a0a7e0fdf95a070c2f86b50edc87288 100644
> --- a/gas/testsuite/gas/arm/pr12198-2.d
> +++ b/gas/testsuite/gas/arm/pr12198-2.d
> @@ -8,5 +8,4 @@
>  Attribute Section: aeabi
>  File Attributes
>    Tag_CPU_arch: v7
> -  Tag_CPU_arch_profile: Microcontroller
>    Tag_THUMB_ISA_use: Thumb-1
> diff --git a/include/opcode/arm.h b/include/opcode/arm.h
> index 5691a85545cf924383f9c73d9ce9a97a3a835cdf..62683a381f167a6714b1a16bb09ff858b49277ae 100644
> --- a/include/opcode/arm.h
> +++ b/include/opcode/arm.h
> @@ -106,12 +106,14 @@
>  #define ARM_AEXT_V3M	(ARM_AEXT_V3	| ARM_EXT_V3M)
>  #define ARM_AEXT_V4xM	(ARM_AEXT_V3	| ARM_EXT_V4)
>  #define ARM_AEXT_V4	(ARM_AEXT_V3M	| ARM_EXT_V4)
> -#define ARM_AEXT_V4TxM	(ARM_AEXT_V4xM	| ARM_EXT_V4T)
> -#define ARM_AEXT_V4T	(ARM_AEXT_V4	| ARM_EXT_V4T)
> +#define ARM_AEXT_V4TxM	(ARM_AEXT_V4xM	| ARM_EXT_V4T | ARM_EXT_OS)
> +#define ARM_AEXT_V4T	(ARM_AEXT_V4	| ARM_EXT_V4T | ARM_EXT_OS)
>  #define ARM_AEXT_V5xM	(ARM_AEXT_V4xM	| ARM_EXT_V5)
>  #define ARM_AEXT_V5	(ARM_AEXT_V4	| ARM_EXT_V5)
> -#define ARM_AEXT_V5TxM	(ARM_AEXT_V5xM	| ARM_EXT_V4T | ARM_EXT_V5T)
> -#define ARM_AEXT_V5T	(ARM_AEXT_V5	| ARM_EXT_V4T | ARM_EXT_V5T)
> +#define ARM_AEXT_V5TxM	(ARM_AEXT_V5xM	| ARM_EXT_V4T | ARM_EXT_V5T \
> +			 | ARM_EXT_OS)
> +#define ARM_AEXT_V5T	(ARM_AEXT_V5	| ARM_EXT_V4T | ARM_EXT_V5T \
> +			 | ARM_EXT_OS)
>  #define ARM_AEXT_V5TExP	(ARM_AEXT_V5T	| ARM_EXT_V5ExP)
>  #define ARM_AEXT_V5TE	(ARM_AEXT_V5TExP | ARM_EXT_V5E)
>  #define ARM_AEXT_V5TEJ	(ARM_AEXT_V5TE	| ARM_EXT_V5J)
> @@ -136,7 +138,7 @@
>  #define ARM_AEXT_V6M_ONLY \
>    ((ARM_EXT_BARRIER | ARM_EXT_V6M | ARM_EXT_THUMB_MSR) & ~(ARM_AEXT_NOTM))
>  #define ARM_AEXT_V6M \
> -  ((ARM_AEXT_V6K | ARM_AEXT_V6M_ONLY) & ~(ARM_AEXT_NOTM))
> +  ((ARM_AEXT_V6K | ARM_AEXT_V6M_ONLY) & ~(ARM_AEXT_NOTM | ARM_EXT_OS))
>  #define ARM_AEXT_V6SM (ARM_AEXT_V6M | ARM_EXT_OS)
>  #define ARM_AEXT_V7M \
>    ((ARM_AEXT_V7_ARM | ARM_EXT_V6M | ARM_EXT_V7M | ARM_EXT_DIV) \
>