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

[Richard Earnshaw (lists)]

On 21/06/17 15:39, Thomas Preudhomme wrote:
> 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.
> 

Ah, ok, so it 'does' have an architecture.  Ignore that then.

R.

> Best regards,
> 
> Thomas