This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH][GOLD] Attributes section part 3.
- From: Ian Lance Taylor <iant at google dot com>
- To: Doug Kwan (éæå) <dougkwan at google dot com>
- Cc: binutils <binutils at sourceware dot org>
- Date: Wed, 09 Dec 2009 17:28:11 -0800
- Subject: Re: [PATCH][GOLD] Attributes section part 3.
- References: <498552560912091534l4865dcb5w2030dd0584b00e3b@mail.gmail.com>
"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