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][GOLD] Attributes section part 3.


"Doug Kwan (éæå)" <dougkwan@google.com> writes:

> 2009-12-09  Doug Kwan  <dougkwan@google.com>
>
>         * arm.cc (attributes.h): New include.
>         (Arm_relobj::Arm_relobj): Initialize attributes_section_data_.
>         (Arm_relobj::~Arm_relobj): Delete object pointed by
>         attributes_section_data_.
>         (Arm_relobj::attributes_section_data): New method definition.
>         (Arm_relobj::attributes_section_data_): New data member declaration.
>         (Arm_dynobj::Arm_dynobj): Initialize attributes_section_data_.
>         (Arm_dynobj::~Arm_dynobj): Delete object pointed by
>         attributes_section_data_.
>         (Arm_dynobj::attributes_section_data): New method definition.
>         (Arm_dynobj::attributes_section_data_): New data member declaration.
>         (Target_arm::Target_arm): Initialize attributes_section_data_.  Change
>         initialization value of may_use_blx_ to false.
>         (Target_arm::using_thumb2, Target_arm::using_thumb_only,
>         Target_arm::may_use_arm_nop, Target_arm::may_use_thumb2_nop): Use
>         object attributes to compute results instead of hard-coding.
>         (Target_arm::do_attribute_arg_type, Target_arm::do_attributes_order,
>         Target_arm::get_secondary_compatible_arch,
>         Target_arm::set_secondary_compatible_arch
>         Target_arm::tag_cpu_arch_combine, Target_arm::aeabi_enum_name,
>         Target_arm::tag_cpu_name_value, Target_arm::merge_object_attributes):
>         New method declarations.
>         (Target_arm::get_aeabi_object_attribute): New method definition.
>         (Target_arm): New enums for EABI object attribute values.
>         (Target_arm::attributes_section_data_): New data member declaration.
>         (Arm_relobj::do_read_symbols): Read attributes section if it exists.
>         (Arm_dynobj::do_read_symbols): Ditto.
>         (Target_arm::do_finalize_sections): Merge attributes sections from
>         input.  Check for BLX use after attributes section merging.
>         Fix __exidx_start and __exidx_end visibility.  Create an
>         .ARM.attributes section if necessary.
>         (Target_arm::get_secondary_compatible_arch,
>         Target_arm::set_secondary_compatible_arch,
>         Target_arm::tag_cpu_arch_combine, Target_arm::aeabi_enum_name,
>         Target_arm::tag_cpu_name_value, Target_arm::merge_object_attributes,
>         Target_arm::do_attribute_arg_type, Target_arm::do_attributes_order):
>         New method definitions.


>    // Whether we have an NOP instruction.  If not, use mov r0, r0 instead.
>    bool
>    may_use_arm_nop() const
>    {
> -    // FIXME:  This should not hard-coded.
> -    return false;
> +    Object_attribute* attr = this->get_aeabi_object_attribute(Tag_CPU_arch);
> +    int arch = attr->int_value();
> +    return arch == elfcpp::TAG_CPU_ARCH_V6T2
> +	   || arch == elfcpp::TAG_CPU_ARCH_V6K
> +	   || arch == elfcpp::TAG_CPU_ARCH_V7
> +	   || arch == elfcpp::TAG_CPU_ARCH_V7E_M;
>    }

Please parenthesize the expression so that emacs indents it correctly.

> +  // Helepr to print AEABI enum tag value.
> +  static const char*
> +  aeabi_enum_name(unsigned int);

s/Helepr/Helper/ .


> +  // EABI object attributes.
> +  enum
> +  {
> +    // 0-3 are generic.
> +    Tag_CPU_raw_name = 4,
> +    Tag_CPU_name,
> +    Tag_CPU_arch,
> +    Tag_CPU_arch_profile,
> +    Tag_ARM_ISA_use,
> +    Tag_THUMB_ISA_use,
> +    Tag_VFP_arch,
> +    Tag_WMMX_arch,
> +    Tag_Advanced_SIMD_arch,
> +    Tag_PCS_config,
> +    Tag_ABI_PCS_R9_use,
> +    Tag_ABI_PCS_RW_data,
> +    Tag_ABI_PCS_RO_data,
> +    Tag_ABI_PCS_GOT_use,
> +    Tag_ABI_PCS_wchar_t,
> +    Tag_ABI_FP_rounding,
> +    Tag_ABI_FP_denormal,
> +    Tag_ABI_FP_exceptions,
> +    Tag_ABI_FP_user_exceptions,
> +    Tag_ABI_FP_number_model,
> +    Tag_ABI_align8_needed,
> +    Tag_ABI_align8_preserved,
> +    Tag_ABI_enum_size,
> +    Tag_ABI_HardFP_use,
> +    Tag_ABI_VFP_args,
> +    Tag_ABI_WMMX_args,
> +    Tag_ABI_optimization_goals,
> +    Tag_ABI_FP_optimization_goals,
> +    // 32 is generic (Tag_compatibility).
> +    Tag_undefined33 = 33,
> +    Tag_CPU_unaligned_access,
> +    Tag_undefined35,
> +    Tag_VFP_HP_extension,
> +    Tag_undefined37,
> +    Tag_ABI_FP_16bit_format,
> +    Tag_undefined39,
> +    Tag_nodefaults = 64,
> +    Tag_also_compatible_with,
> +    Tag_T2EE_use,
> +    Tag_conformance,
> +    Tag_Virtualization_use,
> +    Tag_undefined69,
> +    Tag_MPextension_use
> +  };
> +
> +  // Values for Tag_ABI_PCS_R9_use.
> +  enum
> +  {
> +    AEABI_R9_V6,
> +    AEABI_R9_SB,
> +    AEABI_R9_TLS,
> +    AEABI_R9_unused
> +  };
> +
> +  // Values for Tag_ABI_PCS_RW_data.
> +  enum
> +  {
> +    AEABI_PCS_RW_data_absolute,
> +    AEABI_PCS_RW_data_PCrel,
> +    AEABI_PCS_RW_data_SBrel,
> +    AEABI_PCS_RW_data_unused
> +  };
> +
> +  // Values for Tag_ABI_enum_size.
> +  enum
> +  {
> +    AEABI_enum_unused,
> +    AEABI_enum_short,
> +    AEABI_enum_wide,
> +    AEABI_enum_forced_wide
> +  };

First, I wonder if these values should all be in elfcpp/arm.h.  I'm OK
with it either way, but please consider moving them.

Second, if I understand correctly these represent values that appear
in object files--that is, they are part of the ABI.  Therefore, I
think that the enum constants should all be initialized--you should
not rely on starting at 0 and counting up.  I know that it is a bit
tedious but it protects us from deep confusion if some table gets
scrambled at some point.  See, e.g., the enums in elfcpp.h.


> @@ -3797,6 +3971,29 @@ Arm_dynobj<big_endian>::do_read_symbols(
>  					      true, false);
>    elfcpp::Ehdr<32, big_endian> ehdr(pehdr);
>    this->processor_specific_flags_ = ehdr.get_e_flags();
> +  
> +  // Read the attributes section if there is one.
> +
> +  // Skip the first, dummy section.
> +  const size_t shdr_size = elfcpp::Elf_sizes<32>::shdr_size;
> +  const unsigned char *ps = sd->section_headers->data() + shdr_size;
> +  for (unsigned int i = 1; i < this->shnum(); ++i, ps += shdr_size)
> +    {
> +      elfcpp::Shdr<32, big_endian> shdr(ps);
> +      const char* section_name =
> +	reinterpret_cast<const char*>(sd->section_names->data()
> +				      + shdr.get_sh_name());
> +      if (parameters->target().is_attributes_section(section_name))
> +	{
> +	  section_offset_type section_offset = shdr.get_sh_offset();
> +	  section_size_type section_size =
> +	    convert_to_section_size_type(shdr.get_sh_size());
> +	  File_view* view = this->get_lasting_view(section_offset,
> +						   section_size, true, false);
> +	  this->attributes_section_data_ =
> +	    new Attributes_section_data(view->data(), section_size);
> +	}
> +    }
>  }

Please pull this duplicated loop into a helper function.

This kind of loop can be expensive in a big C++ object.a  Do most
objects have an attributes section?  If not, it may be worth doing a
memmem, like the use just before find_eh_frame in
Sized_relobj::do_read_symbols.

In objects that have an attributes section, is it usually among the
last sections in the file?  If so, it may be worth running the loop in
reverse.


> +  // Check we've not got a higher architecture than we know about.
> +
> +  if (oldtag >= elfcpp::MAX_TAG_CPU_ARCH || newtag >= elfcpp::MAX_TAG_CPU_ARCH)
> +    {
> +      gold_error(_("%s: unknown CPU architecture"), name);
> +      return -1;
> +    }

Should this be a warning?  Will we do the wrong thing if we see
something newer than we know about?


> +// Helepr to print AEABI enum tag value.
> +
> +template<bool big_endian>
> +const char*
> +Target_arm<big_endian>::aeabi_enum_name(unsigned int value)

s/Helepr/Helper/ .


> +{
> +  static const char *aeabi_enum_names[] =
> +    { "", "variable-size", "32-bit", "" };
> +  const size_t aeabi_enum_names_size =
> +    sizeof(aeabi_enum_names) / sizeof(aeabi_enum_names[0]);
> +
> +  return (value < aeabi_enum_names_size
> +	  ? aeabi_enum_names[value]
> +	  : "<unknown>");
> +}

Change this to return std::string and sprintf the value if it is out
of range.  It's frustrating to learn that something is unknown but to
not know the actual value.


> +// Return the string value to store in TAG_CPU_name.
> +
> +template<bool big_endian>
> +const char*
> +Target_arm<big_endian>::tag_cpu_name_value(unsigned int value)
> +{
> +  static const char *name_table[] = {
> +    // These aren't real CPU names, but we can't guess
> +    // that from the architecture version alone.
> +   "Pre v4",
> +   "ARM v4",
> +   "ARM v4T",
> +   "ARM v5T",
> +   "ARM v5TE",
> +   "ARM v5TEJ",
> +   "ARM v6",
> +   "ARM v6KZ",
> +   "ARM v6T2",
> +   "ARM v6K",
> +   "ARM v7",
> +   "ARM v6-M",
> +   "ARM v6S-M",
> +   "ARM v7E-M"
> + };
> + const size_t name_table_size = sizeof(name_table) / sizeof(name_table[0]);
> + return value < name_table_size? name_table[value] : NULL;
> +}

Same here.


> +// The ABI defines that Tag_conformance should be emitted first, and that
> +// Tag_nodefaults should be second (if either is defined).  This sets those
> +// two positions, and bumps up the position of all the remaining tags to
> +// compensate.
> +
> +template<bool big_endian>
> +int
> +Target_arm<big_endian>::do_attributes_order(int num) const
> +{
> +  if (num == 4)
> +    return Tag_conformance;
> +  if (num == 5)
> +    return Tag_nodefaults;
> +  if ((num - 2) < Tag_nodefaults)
> +    return num - 2;
> +  if ((num - 1) < Tag_conformance)
> +    return num - 1;
> +  return num;
> +}

These numbers seem kind of magical, can you comment them or give them
names?

This is OK with the above changes.

Thanks.

Ian


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