This is the mail archive of the
mailing list for the binutils project.
Re: [PATCH 6/10, GAS/ARM] Rework Tag_CPU_arch build attribute value selection
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:
>>> === 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/requested not contributed by earlier
>>>  in case of autodetection mode
>>>  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
>>> 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
>>> 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
>>> 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 <firstname.lastname@example.org>
>>> * 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
>>> * testsuite/gas/arm/attr-march-armv1.d: Fix expected Tag_CPU_arch
>>> 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 <email@example.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...
>>> +/* Mapping from CPU features to EABI CPU arch values. Table must be
>>> + 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
> Indeed. I'll improve the comments in code and cover letter for that patch.
>>> 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.
> Best regards,