Re: [PATCH 2/10, GAS/ARM] Keep separation between extensions and architecture bits throughout execution

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

On 21/06/17 11:04, Thomas Preudhomme wrote:
> Hi,
> 
> === Context ===
> 
> This patch is part of a patch series to add support for ARMv8-R
> architecture. Its purpose is to keep the distinction between
> architecture feature bits and extension ones after parsing has occured.
> 
> === Motivation ===
> 
> This distinction is necessary to allow the Tag_CPU_arch build attribute
> value to be exactly as per the architecture of the selected CPU. With
> mixed architecture and extension feature bit, it is impossible to find
> an architecture with an exact match of feature bit and the build
> attribute value logic must then select the closest match which might not
> be the right architecture. The previous patch in the patch series makes
> the distinction possible when parsing -mcpu and .cpu directives but the
> distinction gets lost after. Similarly feature bits contributed by
> extensions in -march or .arch_extensions directive are mixed together
> with architecture extensions.
> 
> === Patch description ===
> 
> The patch adds new feature bit pointer for extension feature bits.
> Information from the parsing regarding extensions can then be kept
> separate in those. This requires adapting arm_parse_extension to deal
> with two feature bits, allowing the architecture bits to be marked as
> const. It also requires extra care when setting cpu_variant and
> selected_cpu because the extension bits are optional since there might
> not be any extension in use.
> 
> Note that contrary to cpu feature bits, the extension feature bits are
> made read/write and are always dynamically allocated. This allows to
> unconditionally free them in arm_md_post_relax added for this occasion,
> thereby fixing a longstanding memory leak when arm_parse_extension was
> invoked (XNEW of ext_fset without corresponding XDELETE).
> Introduction of arm_md_post_relax is necessary to only free the
> extension feature bits after aeabi_set_attribute_string has been called
> for the last time.
> 
> ChangeLog entry is as follows:
> 
> *** gas/ChangeLog ***
> 
> 2017-03-24  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> 
>     * config/tc-arm.c (dyn_mcpu_ext_opt): New static variable.
>     (dyn_march_ext_opt): Likewise.
>     (md_begin): Copy extension feature bits alongside architecture ones.
>     Merge extensions feature bits in selected_cpu and cpu_variant if there
>     is some.
>     (arm_parse_extension): Pass architecture and extension feature bits in
>     separate parameters, with architecture bits being read only.  Update
>     **opt_p directly rather than *ext_set and initialize it if needed.
>     (arm_parse_cpu): Stop merging architecture and extension feature bits
>     and instead use mcpu_cpu_opt and dyn_mcpu_ext_opt to memorize them
>     respectively.  Adapt to change in parameters of arm_parse_extension.
>     (arm_parse_arch): Adapt to change in parameters of arm_parse_extension.
>     (aeabi_set_attribute_string): Make function static.
>     (arm_md_post_relax): New function.
>     (s_arm_cpu): Stop merging architecture and extension feature bits and
>     instead use mcpu_cpu_opt and dyn_mcpu_ext_opt to memorize them
>     respectively.  Merge extension feature bits in cpu_variant
>     if there is any.
>     (s_arm_arch): Reset extension feature bit.  Set selected_cpu from
>     *mcpu_cpu_opt and cpu_variant from selected_cpu and *mfpu_opt for
>     consistency with s_arm_cpu.
>     (s_arm_arch_extension): Update *dyn_mcpu_ext_opt rather than
>     selected_cpu, allocating it before hand if needed.  Set selected_cpu
>     from it and then cpu_variant.
>     (s_arm_fpu): Merge *mcpu_ext_opt feature bits if any in cpu_variant.
>     * config/tc-arm.h (md_post_relax_hook): Set to arm_md_post_relax.
>     (aeabi_set_public_attributes): Delete external declaration.
>     (arm_md_post_relax): Declare externally.
> 

OK.

R.

> === Testing ===
> 
> Testsuite shows no regression when run for arm-none-eabi targets.
> 
> Is this ok for master branch?
> 
> Best regards,
> 
> Thomas
> 
> 02_keep_extensions_architectures_separate.patch
> 
> 
> diff --git a/gas/config/tc-arm.h b/gas/config/tc-arm.h
> index ebcf27bef839bc8f51a8b00d30f53f88ae558e47..409f919223545cc64203f51c8de6fe3e5834629f 100644
> --- a/gas/config/tc-arm.h
> +++ b/gas/config/tc-arm.h
> @@ -118,8 +118,8 @@ extern bfd_boolean tc_start_label_without_colon (void);
>  extern void arm_md_end (void);
>  bfd_boolean arm_is_eabi (void);
>  
> -#define md_post_relax_hook		aeabi_set_public_attributes ()
> -extern void aeabi_set_public_attributes (void);
> +#define md_post_relax_hook		arm_md_post_relax ()
> +extern void arm_md_post_relax (void);
>  #endif
>  
>  /* NOTE: The fake label creation in stabs.c:s_stab_generic() has
> diff --git a/gas/config/tc-arm.c b/gas/config/tc-arm.c
> index be8e896700a290d725fe2f7b9cb4a72f94116961..3a7b7a6f345393ab66444a128a5288af2bcf439a 100644
> --- a/gas/config/tc-arm.c
> +++ b/gas/config/tc-arm.c
> @@ -147,8 +147,10 @@ static const arm_feature_set *legacy_cpu = NULL;
>  static const arm_feature_set *legacy_fpu = NULL;
>  
>  static const arm_feature_set *mcpu_cpu_opt = NULL;
> +static arm_feature_set *dyn_mcpu_ext_opt = NULL;
>  static const arm_feature_set *mcpu_fpu_opt = NULL;
>  static const arm_feature_set *march_cpu_opt = NULL;
> +static arm_feature_set *dyn_march_ext_opt = NULL;
>  static const arm_feature_set *march_fpu_opt = NULL;
>  static const arm_feature_set *mfpu_opt = NULL;
>  static const arm_feature_set *object_arch = NULL;
> @@ -25000,7 +25002,12 @@ md_begin (void)
>        mcpu_cpu_opt = legacy_cpu;
>      }
>    else if (!mcpu_cpu_opt)
> -    mcpu_cpu_opt = march_cpu_opt;
> +    {
> +      mcpu_cpu_opt = march_cpu_opt;
> +      dyn_mcpu_ext_opt = dyn_march_ext_opt;
> +      /* Avoid double free in arm_md_end.  */
> +      dyn_march_ext_opt = NULL;
> +    }
>  
>    if (legacy_fpu)
>      {
> @@ -25040,16 +25047,22 @@ md_begin (void)
>        mcpu_cpu_opt = &cpu_default;
>        selected_cpu = cpu_default;
>      }
> +  else if (dyn_mcpu_ext_opt)
> +    ARM_MERGE_FEATURE_SETS (selected_cpu, *mcpu_cpu_opt, *dyn_mcpu_ext_opt);
>    else
>      selected_cpu = *mcpu_cpu_opt;
>  #else
> -  if (mcpu_cpu_opt)
> +  if (mcpu_cpu_opt && dyn_mcpu_ext_opt)
> +    ARM_MERGE_FEATURE_SETS (selected_cpu, *mcpu_cpu_opt, *dyn_mcpu_ext_opt);
> +  else if (mcpu_cpu_opt)
>      selected_cpu = *mcpu_cpu_opt;
>    else
>      mcpu_cpu_opt = &arm_arch_any;
>  #endif
>  
>    ARM_MERGE_FEATURE_SETS (cpu_variant, *mcpu_cpu_opt, *mfpu_opt);
> +  if (dyn_mcpu_ext_opt)
> +    ARM_MERGE_FEATURE_SETS (cpu_variant, cpu_variant, *dyn_mcpu_ext_opt);
>  
>    autoselect_thumb_from_cpu_variant ();
>  
> @@ -26053,10 +26066,9 @@ struct arm_long_option_table
>  };
>  
>  static bfd_boolean
> -arm_parse_extension (const char *str, const arm_feature_set **opt_p)
> +arm_parse_extension (const char *str, const arm_feature_set *opt_set,
> +		     arm_feature_set **ext_set_p)
>  {
> -  arm_feature_set *ext_set = XNEW (arm_feature_set);
> -
>    /* We insist on extensions being specified in alphabetical order, and with
>       extensions being added before being removed.  We achieve this by having
>       the global ARM_EXTENSIONS table in alphabetical order, and using the
> @@ -26067,9 +26079,11 @@ arm_parse_extension (const char *str, const arm_feature_set **opt_p)
>    const arm_feature_set arm_any = ARM_ANY;
>    int adding_value = -1;
>  
> -  /* Copy the feature set, so that we can modify it.  */
> -  *ext_set = **opt_p;
> -  *opt_p = ext_set;
> +  if (!*ext_set_p)
> +    {
> +      *ext_set_p = XNEW (arm_feature_set);
> +      **ext_set_p = arm_arch_none;
> +    }
>  
>    while (str != NULL && *str != 0)
>      {
> @@ -26137,7 +26151,7 @@ arm_parse_extension (const char *str, const arm_feature_set **opt_p)
>  		/* Empty entry.  */
>  		if (ARM_FEATURE_EQUAL (opt->allowed_archs[i], arm_any))
>  		  continue;
> -		if (ARM_FSET_CPU_SUBSET (opt->allowed_archs[i], *ext_set))
> +		if (ARM_FSET_CPU_SUBSET (opt->allowed_archs[i], *opt_set))
>  		  break;
>  	      }
>  	    if (i == nb_allowed_archs)
> @@ -26148,9 +26162,10 @@ arm_parse_extension (const char *str, const arm_feature_set **opt_p)
>  
>  	    /* Add or remove the extension.  */
>  	    if (adding_value)
> -	      ARM_MERGE_FEATURE_SETS (*ext_set, *ext_set, opt->merge_value);
> +	      ARM_MERGE_FEATURE_SETS (**ext_set_p, **ext_set_p,
> +				      opt->merge_value);
>  	    else
> -	      ARM_CLEAR_FEATURE (*ext_set, *ext_set, opt->clear_value);
> +	      ARM_CLEAR_FEATURE (**ext_set_p, **ext_set_p, opt->clear_value);
>  
>  	    break;
>  	  }
> @@ -26206,9 +26221,10 @@ arm_parse_cpu (const char *str)
>    for (opt = arm_cpus; opt->name != NULL; opt++)
>      if (opt->name_len == len && strncmp (opt->name, str, len) == 0)
>        {
> -	arm_feature_set *cpu_set = XNEW (arm_feature_set);
> -	ARM_MERGE_FEATURE_SETS (*cpu_set, opt->value, opt->ext);
> -	mcpu_cpu_opt = cpu_set;
> +	mcpu_cpu_opt = &opt->value;
> +	if (!dyn_mcpu_ext_opt)
> +	  dyn_mcpu_ext_opt = XNEW (arm_feature_set);
> +	*dyn_mcpu_ext_opt = opt->ext;
>  	mcpu_fpu_opt = &opt->default_fpu;
>  	if (opt->canonical_name)
>  	  {
> @@ -26228,7 +26244,7 @@ arm_parse_cpu (const char *str)
>  	  }
>  
>  	if (ext != NULL)
> -	  return arm_parse_extension (ext, &mcpu_cpu_opt);
> +	  return arm_parse_extension (ext, mcpu_cpu_opt, &dyn_mcpu_ext_opt);
>  
>  	return TRUE;
>        }
> @@ -26263,7 +26279,7 @@ arm_parse_arch (const char *str)
>  	strcpy (selected_cpu_name, opt->name);
>  
>  	if (ext != NULL)
> -	  return arm_parse_extension (ext, &march_cpu_opt);
> +	  return arm_parse_extension (ext, march_cpu_opt, &dyn_march_ext_opt);
>  
>  	return TRUE;
>        }
> @@ -26549,7 +26565,7 @@ aeabi_set_attribute_string (int tag, const char *value)
>  }
>  
>  /* Set the public EABI object attributes.  */
> -void
> +static void
>  aeabi_set_public_attributes (void)
>  {
>    int arch;
> @@ -26796,6 +26812,18 @@ aeabi_set_public_attributes (void)
>      aeabi_set_attribute_int (Tag_Virtualization_use, virt_sec);
>  }
>  
> +/* Post relaxation hook.  Recompute ARM attributes now that relaxation is
> +   finished and free extension feature bits which will not be used anymore.  */
> +void
> +arm_md_post_relax (void)
> +{
> +  aeabi_set_public_attributes ();
> +  XDELETE (dyn_mcpu_ext_opt);
> +  dyn_mcpu_ext_opt = NULL;
> +  XDELETE (dyn_march_ext_opt);
> +  dyn_march_ext_opt = NULL;
> +}
> +
>  /* Add the default contents for the .ARM.attributes section.  */
>  void
>  arm_md_end (void)
> @@ -26827,10 +26855,11 @@ s_arm_cpu (int ignored ATTRIBUTE_UNUSED)
>    for (opt = arm_cpus + 1; opt->name != NULL; opt++)
>      if (streq (opt->name, name))
>        {
> -	arm_feature_set *cpu_set = XNEW (arm_feature_set);
> -	ARM_MERGE_FEATURE_SETS (*cpu_set, opt->value, opt->ext);
> -	mcpu_cpu_opt = cpu_set;
> -	selected_cpu = *mcpu_cpu_opt;
> +	mcpu_cpu_opt = &opt->value;
> +	if (!dyn_mcpu_ext_opt)
> +	  dyn_mcpu_ext_opt = XNEW (arm_feature_set);
> +	*dyn_mcpu_ext_opt = opt->ext;
> +	ARM_MERGE_FEATURE_SETS (selected_cpu, *mcpu_cpu_opt, *dyn_mcpu_ext_opt);
>  	if (opt->canonical_name)
>  	  strcpy (selected_cpu_name, opt->canonical_name);
>  	else
> @@ -26842,6 +26871,8 @@ s_arm_cpu (int ignored ATTRIBUTE_UNUSED)
>  	    selected_cpu_name[i] = 0;
>  	  }
>  	ARM_MERGE_FEATURE_SETS (cpu_variant, *mcpu_cpu_opt, *mfpu_opt);
> +	if (dyn_mcpu_ext_opt)
> +	  ARM_MERGE_FEATURE_SETS (cpu_variant, cpu_variant, *dyn_mcpu_ext_opt);
>  	*input_line_pointer = saved_char;
>  	demand_empty_rest_of_line ();
>  	return;
> @@ -26872,9 +26903,11 @@ s_arm_arch (int ignored ATTRIBUTE_UNUSED)
>      if (streq (opt->name, name))
>        {
>  	mcpu_cpu_opt = &opt->value;
> -	selected_cpu = opt->value;
> +	XDELETE (dyn_mcpu_ext_opt);
> +	dyn_mcpu_ext_opt = NULL;
> +	selected_cpu = *mcpu_cpu_opt;
>  	strcpy (selected_cpu_name, opt->name);
> -	ARM_MERGE_FEATURE_SETS (cpu_variant, *mcpu_cpu_opt, *mfpu_opt);
> +	ARM_MERGE_FEATURE_SETS (cpu_variant, selected_cpu, *mfpu_opt);
>  	*input_line_pointer = saved_char;
>  	demand_empty_rest_of_line ();
>  	return;
> @@ -26961,14 +26994,20 @@ s_arm_arch_extension (int ignored ATTRIBUTE_UNUSED)
>  	    break;
>  	  }
>  
> +	if (!dyn_mcpu_ext_opt)
> +	  {
> +	    dyn_mcpu_ext_opt = XNEW (arm_feature_set);
> +	    *dyn_mcpu_ext_opt = arm_arch_none;
> +	  }
>  	if (adding_value)
> -	  ARM_MERGE_FEATURE_SETS (selected_cpu, selected_cpu,
> +	  ARM_MERGE_FEATURE_SETS (*dyn_mcpu_ext_opt, *dyn_mcpu_ext_opt,
>  				  opt->merge_value);
>  	else
> -	  ARM_CLEAR_FEATURE (selected_cpu, selected_cpu, opt->clear_value);
> +	  ARM_CLEAR_FEATURE (*dyn_mcpu_ext_opt, *dyn_mcpu_ext_opt,
> +			     opt->clear_value);
>  
> -	mcpu_cpu_opt = &selected_cpu;
> -	ARM_MERGE_FEATURE_SETS (cpu_variant, *mcpu_cpu_opt, *mfpu_opt);
> +	ARM_MERGE_FEATURE_SETS (selected_cpu, *mcpu_cpu_opt, *dyn_mcpu_ext_opt);
> +	ARM_MERGE_FEATURE_SETS (cpu_variant, selected_cpu, *mfpu_opt);
>  	*input_line_pointer = saved_char;
>  	demand_empty_rest_of_line ();
>  	return;
> @@ -27001,6 +27040,8 @@ s_arm_fpu (int ignored ATTRIBUTE_UNUSED)
>        {
>  	mfpu_opt = &opt->value;
>  	ARM_MERGE_FEATURE_SETS (cpu_variant, *mcpu_cpu_opt, *mfpu_opt);
> +	if (dyn_mcpu_ext_opt)
> +	  ARM_MERGE_FEATURE_SETS (cpu_variant, cpu_variant, *dyn_mcpu_ext_opt);
>  	*input_line_pointer = saved_char;
>  	demand_empty_rest_of_line ();
>  	return;
>