This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] Arm EABI object attributes


On Wed, 2005-09-28 at 23:02, Paul Brook wrote:
> The attached patch adds partial support for Arm EABI object attributes.
> Partial because it only supports file scope attributes. Finer grained 
> (section/symbol level) attributes are currently ignored.

> There are 4 new assembly directives. The first 3 (.cpu, .arch and .fpu) are 
> technically independent of this patch. They implement the equivalent of the 
> the -mcpu=, -march= and -mfpu= commandline options.  Some changes were 
> necessary to make ".cpu all" (the default on many targets) mark the object 
> file based on the instructions actually user, rather than marking it as being 
> for the most advanced CPU we know about.
> 

I don't think we want to allow a user to write ".cpu all".  Having the
concept of an unrestricted assembly space is already problematic
(consider the architectural nops; and Cortex-M3 which only supports the
T2 instruction set).  So I think we should start restricting the ways in
which the user can put the assembler into 'all' mode.

> The .eabi_attribute allows the user to add specific tags to an object.
> 
> Ok?
> Comments/suggestions?
> 

Overall this is cool.  Some niggles, but see below.


> +
> +/* Override final_link to handle EABI EABI object attribute sections.  */
                                         ^^^^^^^^^^
EABI EABI

> +
> +static bfd_boolean
> +elf32_arm_bfd_final_link (bfd *abfd, struct bfd_link_info *info)
> +{

> +
> +  /* elf32_arm_merge_private_bfd_data wil already have merged the
                                              ^^^^

'will'
>  
> +/* Create a new attribute, or return the existing one if it already exists.  */

This comment appears to differ from the implementation.  It always
creates a new attribute.

> +static aeabi_attribute *
> +elf32_arm_new_eabi_attr (bfd *abfd, int tag, int type)
> +{
> +  aeabi_attribute *attr;
> +
> +  /* Create a new tag.  */
> +  attr = (aeabi_attribute *) bfd_alloc (abfd, sizeof (aeabi_attribute));
> +  memset (attr, 0, sizeof (aeabi_attribute));
> +  attr->tag = tag;
> +  attr->type = type;
> +  attr->next = elf32_arm_tdata (abfd)->eabi_attributes;
> +  elf32_arm_tdata (abfd)->eabi_attributes = attr;
> +  return attr;
> +}

> +
> +/* Find the eabi object attrbute with the given TAG.  Return NULL if no

Typos (attribute).

> +/* Merge EABI object attributes from IBFD into OBFD.  Raise an error if there
> +   are conflicting attributes.  */
...
> +	    case 4: /* Tag_CPU_raw_name.  */
> +	    case 5: /* Tag_CPU_name.  */
> +	    case 30: /* Tag_ABI_optimization_goals.  */
> +	    case 31: /* Tag_ABI_FP_optimization_goals.  */

Can we have an enumeration for these attribute tags (or defines) in
include/elf/arm.h

I'm not sure this is correct for the CPU name attributes.  We probably
want one with the 'highest' architecture.  For CPU_name, the names are
canonical and defined by the spec, so this is tractable.  For
CPU_raw_name it's harder (perhaps the first one from a file with the
right architecture/cpu_name).


> +	      /* Use the first value seen.  */
> +	      break;
> +	    case 6: /* Tag_CPU_arch.  */
> +	    case 7: /* Tag_ARM_ISA_use.  */
> +	    case 9: /* Tag_THUMB_ISA_use.  */
> +	    case 10: /* Tag_VFP_arch.  */
> +	    case 11: /* Tag_WMMX_arch.  */
> +	    case 12: /* Tag_NEON_arch.  */
> +	      /* ??? Do NEON and WMMX conflict?  */
> +	    case 19: /* Tag_ABI_FP_rounding.  */
> +	    case 20: /* Tag_ABI_FP_denormal.  */
> +	    case 21: /* Tag_ABI_FP_exceptions.  */
> +	    case 22: /* Tag_ABI_FP_user_exceptions.  */
> +	    case 23: /* Tag_ABI_FP_number_model.  */
> +	    case 25: /* Tag_ABI_align8_preserved.  */
> +	    case 27: /* Tag_ABI_HardFP_use.  */
> +	      /* Use the largest value specified.  */
> +	      if (attr->i > prev->i)
> +		prev->i = attr->i;
> +	      break;

This is probably a reasonable first start.

> +	    case 8: /* Tag_CPU_arch_profile N/A.  */
> +	      /* Warn if conflicting architecture profiules used.  */

Typo (profiles)

> +	      if (prev->i && attr->i && attr->i != prev->i)
> +		{
> +		  _bfd_error_handler
> +		    (_("ERROR: %B: Conflicting architecture profiles %c/%c"),
> +		     ibfd, attr->i, prev->i);
> +		  return FALSE;
> +		}
> +	      if (attr->i)
> +		prev->i = attr->i;
> +	      break;
> +	    case 13: /* Tag_PCS_config.  */
> +	      if (prev->i && attr->i)
> +		{
> +		  /* ??? It's probably ok to mix some of these configs,
> +		     so this is only a warning.  */
Agreed.  For example linux + linux_DSO are probably safe to mix in a
non-dso.

> +		  _bfd_error_handler
> +		    (_("ERROR: %B: Conflicting platform configuration"), ibfd);
> +		  return FALSE;
> +		}
> +	      else if (attr->i)
> +		attr->i = prev->i;
> +	      break;
> +	    case 14: /* Tag_ABI_PCS_R9_use.  */
> +	      if (prev->i != 3 && attr->i != 3)

This would be clearer with an enumeration.

> +		{
> +		  _bfd_error_handler
> +		    (_("ERROR: %B: Conflicting use of R9"), ibfd);
> +		  return FALSE;
> +		}
> +	      if (prev->i == 3)
> +		prev->i = attr->i;
> +	      break;
> +	    case 15: /* Tag_ABI_PCS_RW_data.  */
> +	    case 16: /* Tag_ABI_PCS_RO_data.  */
> +	      /* Use the smallest value specified.  */
> +	      if (attr->i < prev->i)
> +		prev->i = attr->i;
> +	      break;

This isn't this simple.  It depends on other attributes.  For example,
if RW_data is SB-relative, it isn't safe to have R9 used as the TLS
register or as a general register.

> +	    case 17: /* Tag_ABI_PCS_GOT_use.  */
> +	      if (attr->i > 2 || prev->i > 2
> +		  || order_312[attr->i] < order_312[prev->i])
> +		prev->i = attr->i;
> +	      break;
> +	    case 18: /* Tag_ABI_ABI_PCS_wchar_t.  */
> +	      if (prev->i && attr->i && prev->i != attr->i)
> +		{
> +		  _bfd_error_handler
> +		    (_("ERROR: %B: Conflicting definitions of wchar_t"), ibfd);
> +		  return FALSE;
> +		}
> +	      if (attr->i)
> +		prev->i = attr->i;
> +	      break;
> +	    case 24: /* Tag_ABI_align8_needed.  */
> +	      /* ??? Check agains Tag_ABI_align8_preserved.  */
> +	      if (attr->i > 2 || prev->i > 2
> +		  || order_312[attr->i] < order_312[prev->i])
> +		prev->i = attr->i;
> +	      break;
> +	    case 26: /* Tag_ABI_enum_size.  */
> +	      if (prev->i == 0)
> +		prev->i = attr->i;
> +	      else if (attr->i != 0)
> +		{
> +		  if ((attr->i == 1 && prev->i != 1)
> +		      || (attr->i != 1 && prev->i == 1))
> +		    {
> +		      _bfd_error_handler
> +			(_("ERROR: %B: Conflicting enum sizes"), ibfd);
> +		    }
> +		}

Not quite.  A setting of 3 (all enums are defined with a 32-bit guard
value) is compatible with both short and non-short enums.

> +	      break;
> +	    case 28: /* Tag_ABI_VFP_args.  */
> +	      if (attr->i != prev->i)
> +		{
> +		  _bfd_error_handler
> +		    (_("ERROR: %B uses VFP register arguments, %B does not"),
> +		     ibfd, obfd);
> +		  return FALSE;
> +		}

Ok for a first cut, but really we should also take into account
FP_number_model.  If this is 0 then there is no passing of VFP arguments
so the code is always compatible.  (I think the same is implied by
omitting the tag).


> +      /* blx is marked as both v4T and v5 to work around the fact that
> +	 v5 doesn't really exist (only v5t).  */

Not quite.  Even if a v5-only core did exist it would implement BX/BLX. 
An attempt to transition to Thumb mode would then take the undef trap at
the *target* of the branch.

> @@ -12896,6 +13019,10 @@ arm_parse_cpu (char * str)
>        {
>  	mcpu_cpu_opt = opt->value;
>  	mcpu_fpu_opt = opt->default_fpu;
> +	if (streq(str, "any"))
> +	  selected_cpu = 0;
> +	else
> +	  selected_cpu_name = opt->name;
>  
>  	if (ext != NULL)
>  	  return arm_parse_extension (ext, &mcpu_cpu_opt);

See my comments above about 'any' and 'all'.  Also, the attribute must
use the canonical name for the CPU, not any random GNUish name. 
Typically these are the same except that the canonical name is in upper
case.  However, see the table in the EABI docs.

> +/* Parse a .cpu directive.  */

> +    if (streq (opt->name, name))
> +      {
> +	mcpu_cpu_opt = opt->value;
> +	if (!streq(name, "all"))
> +	  {

Again, see above.

As a final note, some attributes have a default value when missing
(normally the value 0) that is 'maximally INcompatible'.  I think the
best way to handle this is to add the relevant missing attributes once
the object's table has been read.  When writing out the attributes it
would be possible to suppress any attribute that has the default value
in order to save object file space.  For example, omitting
TAG_ABI_Align8_preserved means that the stack is not kept double-word
aligned: this is generally incompatible with code that needs the stack
to be kept aligned (and yes, this encoding was deliberate to prevent
accidental "mistakes by omission").

R.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]