[PATCH][GCC][ARM] Implement a stricter self check.

[Richard Earnshaw (lists)]

On 20/07/17 15:57, Tamar Christina wrote:
> Hi All,
> 
> This patch makes the self-test for the ARM options validations a bit
> stricter.
> 
> When you specify a new architecture extension, neither the options flag in
> arm-cpus.in nor the ISA definition in arm.h should contain any other
> architecture bits, as this will confuse the parser. The options will be added
> but it won't behave as expected.
> 
> This patch adds a stronger claim, which says you're not allowed to repeat any
> feature bits already enabled by the parent architecture. This forces you to have
> the smallest definition as possible for your option and also prevents the problem
> stated above.
> 
> Regtested on arm-none-linux-eabi and no regressions.
> 
> Ok for trunk?
> 
> gcc/
> 2017-07-20  Tamar Christina  <tamar.christina@arm.com>
> 
> 	* config/arm/arm.c (arm_test_cpu_arch_dat):
> 	Check for overlap.
> 
> 

OK.

R.

> arm-selfcheck.patch
> 
> 
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 139ab701a956c27f40d23529f065c535ed8e19fd..c3feb4983c73c7141b7c0799644c45df429d41d8 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -31222,12 +31222,15 @@ namespace selftest {
>     inconsistencies in the option extensions at present (extensions
>     that duplicate others but aren't marked as aliases).  Furthermore,
>     for correct canonicalization later options must never be a subset
> -   of an earlier option.  */
> +   of an earlier option.  Any extension should also only specify other
> +   feature bits and never an architecture bit.  The architecture is inferred
> +   from the declaration of the extension.  */
>  static void
>  arm_test_cpu_arch_data (void)
>  {
>    const arch_option *arch;
>    const cpu_option *cpu;
> +  auto_sbitmap target_isa (isa_num_bits);
>    auto_sbitmap isa1 (isa_num_bits);
>    auto_sbitmap isa2 (isa_num_bits);
>  
> @@ -31238,6 +31241,8 @@ arm_test_cpu_arch_data (void)
>        if (arch->common.extensions == NULL)
>  	continue;
>  
> +      arm_initialize_isa (target_isa, arch->common.isa_bits);
> +
>        for (ext1 = arch->common.extensions; ext1->name != NULL; ++ext1)
>  	{
>  	  if (ext1->alias)
> @@ -31250,7 +31255,13 @@ arm_test_cpu_arch_data (void)
>  		continue;
>  
>  	      arm_initialize_isa (isa2, ext2->isa_bits);
> +	      /* If the option is a subset of the parent option, it doesn't
> +		 add anything and so isn't useful.  */
>  	      ASSERT_TRUE (!bitmap_subset_p (isa2, isa1));
> +
> +	      /* If the extension specifies any architectural bits then
> +		 disallow it.  Extensions should only specify feature bits.  */
> +	      ASSERT_TRUE (!bitmap_intersect_p (isa2, target_isa));
>  	    }
>  	}
>      }
> @@ -31262,6 +31273,8 @@ arm_test_cpu_arch_data (void)
>        if (cpu->common.extensions == NULL)
>  	continue;
>  
> +      arm_initialize_isa (target_isa, arch->common.isa_bits);
> +
>        for (ext1 = cpu->common.extensions; ext1->name != NULL; ++ext1)
>  	{
>  	  if (ext1->alias)
> @@ -31274,7 +31287,13 @@ arm_test_cpu_arch_data (void)
>  		continue;
>  
>  	      arm_initialize_isa (isa2, ext2->isa_bits);
> +	      /* If the option is a subset of the parent option, it doesn't
> +		 add anything and so isn't useful.  */
>  	      ASSERT_TRUE (!bitmap_subset_p (isa2, isa1));
> +
> +	      /* If the extension specifies any architectural bits then
> +		 disallow it.  Extensions should only specify feature bits.  */
> +	      ASSERT_TRUE (!bitmap_intersect_p (isa2, target_isa));
>  	    }
>  	}
>      }
>