[PATCH 6/10, GAS/ARM] Rework Tag_CPU_arch build attribute value selection

[Thomas Preudhomme]

Hi Richard,

On 21/06/17 15:33, Richard Earnshaw (lists) wrote:
> On 21/06/17 11:12, Thomas Preudhomme wrote:
>> Hi,
>>
>> === Context ===
>>
>> This patch is part of a patch series to add support for ARMv8-R
>> architecture. Its purpose is to rework the Tag_CPU_arch build attribute
>> value selection to (i) match architecture or CPU if specified by user
>> without any need for hack and (ii) match an architecture with all the
>> features used if in autodetection mode or return an error.
>>
>> === Motivation ===
>>
>> Currently, Tag_CPU_arch build attribute value selection assumes that an
>> architecture is always a superset of architectures released earlier. As
>> such, the logic is to browse architectures in chronological order of
>> release and selecting the Tag_CPU_arch value of the last one to
>> contribute a feature used[1]/requested[2] not contributed by earlier
>> architectures.
>>
>> [1] in case of autodetection mode
>> [2] otherwise, ie. in case of -mcpu/-march or associated directives
>>
>> This logic fails the two objectives mentionned in the Context section.
>> First, due to the assumption it does an architecture can be selected
>> while not having all the features used/requested which fails the second
>> objective. Second, not doing an exact match when an architecture or CPU
>> is selected by the user means the wrong value is chosen when a later
>> architecture provides a subset of the feature bits of an earlier
>> architecture. This is the case for the implementation of ARMv8-R (see
>> later patch).
>>
>> An added benefit of this patch is that it is possible to easily generate
>> more consistent build attribute by setting the feature bits from the
>> architecture matched in aeabi_set_public_attributes in autodetection
>> mode. This is better done as a separate patch because lots of testcase'
>> expected results must then be updated accordingly.
>>
>> === Patch description ===
>>
>> The patch changes the main logic for Tag_CPU_arch and Tag_CPU_arch_profile
>> values selection to:
>> - look for an exact match in case an architecture or CPU was specified
>>   on the command line or in a directive
>> - select the first released architecture that provides a superset of the
>>   feature used in the autodetection case
>> - select the most featureful architecture in case of -march=all
>> The array cpu_arch_ver is updated to include all architectures in order
>> to make the first point work.
>>
>> Note that when looking for an exact match, the architecture with
>> selected extension is tried first and then only the architecture. This
>> is because some architectures are exactly equivalent to an earlier
>> architecture with its extensions selected. ARMv6S-M (= ARMv6-M + OS
>> extension) and ARMv6KZ (ARMv6K + security extension) are two such
>> examples.
>>
>> Other adjustments are also necessary in aeabi_set_public_attributes to
>> make this change work.
>>
>> 1) The logic to set Tag_ARM_ISA_use and Tag_THUMB_ISA_use must check the
>> absence of feature bit used/requested to decide whether to give the
>> default value for empty files (see EABI attribute defaults test). It was
>> previously checking that arch == 0 which would only happen if no feature
>> bit could be matched by any architecture, ie there is no feature bit to
>> match.
>>
>> 2) A fallback to a superset match must exist when no_cpu_selected ()
>> returns true. This is because aeabi_set_public_attributes is called
>> again after relaxation and at this point selected_cpu is set from the
>> previous execution of that function. There is therefore no way to check
>> whether the user specified an architecture or CPU.
>>
>> ChangeLog entries are as follow:
>>
>> *** gas/ChangeLog ***
>>
>> 2017-06-13  Thomas Preud'homme  <thomas.preudhomme@arm.com>
>>
>>     * config/tc-arm.c (fpu_any): Defined from FPU_ANY.
>>     (cpu_arch_ver): Add all architectures and sort by release date.
>>     (have_ext_for_needed_feat_p): New.
>>     (get_aeabi_cpu_arch_from_fset): New.
>>     (aeabi_set_public_attributes): Call above function to determine
>>     Tag_CPU_arch and Tag_CPU_arch_profile values.  Adapt Tag_ARM_ISA_use
>>     and Tag_THUMB_ISA_use selection logic to check absence of feature bit
>>     accordingly.
>>     * testsuite/gas/arm/attr-march-armv1.d: Fix expected Tag_CPU_arch build
>>     attribute value.
>>     * testsuite/gas/arm/attr-march-armv2.d: Likewise.
>>     * testsuite/gas/arm/attr-march-armv2a.d: Likewise.
>>     * testsuite/gas/arm/attr-march-armv2s.d: Likewise.
>>     * testsuite/gas/arm/attr-march-armv3.d: Likewise.
>>     * testsuite/gas/arm/attr-march-armv3m.d: Likewise.
>>     * testsuite/gas/arm/pr12198-2.d: Likewise.
>>
>> *** include/ChangeLog ***
>>
>> 2017-01-26  Thomas Preud'homme  <thomas.preudhomme@arm.com>
>>
>>     * opcode/arm.h (FPU_ANY): New macro.
>>
>> === Testing ===
>>
>> Testsuite shows no regression when run for arm-none-eabi targets.
>>
>> Is this ok for master branch?
>>
>
> I have two issues with this patch...
>
> 1)
>> +/* Mapping from CPU features to EABI CPU arch values.  Table must be
> sorted
>> +   chronologically for architectures, with an exception for ARMv6-M and
>> +   ARMv6S-M due to legacy reasons.  No new architecture should have a
>> +   special case.  */
>
> This comment warrants some justification.  We should explain that the
> reason for maintaining chronological order is to avoid the build
> attributes of object files changing as new variant architectures get added.

Indeed. I'll improve the comments in code and cover letter for that patch.

>
> 2)
>>  File Attributes
>>    Tag_CPU_name: "1"
>> -  Tag_CPU_arch: v4
>>    Tag_ARM_ISA_use: Yes
>
> I think we should always emit a Tag_CPU_arch directive.  It's wrong to
> assume that the consumer of an object file will be able to deduce the
> base architecture from a simple CPU name - especially an odd-ball one
> like '1' which isn't an official product name.
>
> Tag_CPU_arch = v4 is probably the right thing to emit here - we don't
> have tags for architectures older than that.

The document "Addenda to, and Errata in, the ABI® for the ARM 
Architecture"(document ID: ARM IHI 0045D) issue E r2.10 says that a value of 0 
is for pre-v4 architectures. Since 0 is the default value this at least follows 
the documentation.

Best regards,

Thomas