Re: [PATCH][GOLD] Attributes section part 3.

[=?big5?b?RG91ZyBLd2FuICjD9q62vHcp?= <dougkwan at google dot com>]

Hi,


2009/12/9 Ian Lance Taylor <iant@google.com>:

> 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.

If they are in elfcpp, they need to be protected by a nested namespace
like elfcpp::Arm because other targets my used conflict tags.

>
>> @@ -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.

I think we can assume all ARM objects we care have attributes
sections.  The AAELF document does not say anything about the position
of the section in the section headers.  gas seems to put them near the
end in the section headers so we may want to search from the end.
The document does specify the section type to be ARM_ATTRIBUTE and
name ".ARM.attributes".  Would it be more desirable if we just check
the section type instead of using memmem?

> 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?

We don't know how to combine unknown architectures meaningfully.  So
it makes sense to issue an error and abort.

>> +// 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;
>> +}
>

In BFD, Tag_CPU_name is left empty for unknown CPU.  The NULL return
value is for that case.  I change the code as you suggested.  That
causes Tag_CPU_name to have value "<unknown CPU n>".  I could not find
any document about use of the TAG, so I assume it is okay.

>> +// 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?

There numbers are positions in the output list of attributes.  I can
add a comment about how it works.

Other suggestions were made also.  I will send out a new patch after
rewriting the loop.

Thanks for reviewing.

-Doug