Re: Fwd: [PATCH, ARM]An accurate way to calculate the target arch attribute

[Richard Earnshaw <rearnsha at arm dot com>]

On 19/11/14 13:45, Richard Earnshaw wrote:
> On 19/11/14 12:37, Terry Guo wrote:
>>
>>
>>> -----Original Message-----
>>> From: Richard Earnshaw
>>> Sent: Wednesday, November 19, 2014 7:47 PM
>>> To: Terry Guo
>>> Cc: binutils@sourceware.org
>>> Subject: Re: Fwd: [PATCH, ARM]An accurate way to calculate the target arch
>>> attribute
>>>
>>> On 19/11/14 10:33, Terry Guo wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: binutils-owner@sourceware.org [mailto:binutils-
>>>>> owner@sourceware.org] On Behalf Of Richard Earnshaw
>>>>> Sent: Wednesday, November 19, 2014 6:16 PM
>>>>> To: Terry Guo; binutils@sourceware.org
>>>>> Subject: Re: Fwd: [PATCH, ARM]An accurate way to calculate the target
>>>>> arch attribute
>>>>>
>>>>> On 29/09/14 04:10, Terry Guo wrote:
>>>>>> Hi there,
>>>>>>
>>>>>> When target isn't specified from either command line or .arch/.cpu
>>>>>> directive, current gas will calculate the target arch before
>>>>>> relaxation step and assume that a possible relaxation will happen
>>>>>> anyway even this relaxation doesn't happen in late relaxation step.
>>>>>> This causes a overly assumption to target arch e.g. set target arch
>>>>>> to
>>>>>> armv6t2 where armv4t is good enough.
>>>>>>
>>>>>> The attached patch intends to do an accurate calculation of target
>>>>>> arch by considering the target arch in relaxation step.
>>>>>>
>>>>>> Tested with Binutils regression test. Is it OK?
>>>>>>
>>>>>> BR,
>>>>>> Terry
>>>>>>
>>>>>> gas/
>>>>>> 2014-09-29  Terry Guo  <terry.guo@arm.com>
>>>>>>
>>>>>>           * config/tc-arm.c (md_assemble): Do not consider relaxation.
>>>>>>           (md_convert_frag): Test and set target arch attribute
>>>> accordingly.
>>>>>>           (aeabi_set_attribute_string): Turn it into a global
>> function.
>>>>>>           * config/tc-arm.h (md_post_relax_hook): Enable it for ARM
>>>> target.
>>>>>>           (aeabi_set_public_attributes): Declare it.
>>>>>>
>>>>>> gas/testsuite/
>>>>>> 2014-09-29  Terry Guo  <terry.guo@arm.com>
>>>>>>
>>>>>>           * gas/arm/attr-arch-assumption.d: New file.
>>>>>>           * gas/arm/attr-arch-assumption.s: Likewise.
>>>>>>
>>>>>
>>>>> These bits seem essentially OK, but ...
>>>>>
>>>>>> ld/testsuite/
>>>>>> 2014-09-29  Terry Guo  <terry.guo@arm.com>
>>>>>>
>>>>>>           * ld-arm/tls-longplt-lib.d: Updated.
>>>>>>           * ld-arm/tls-longplt-lib.s: Likewise.
>>>>>>           * ld-arm/tls-longplt.d: Likewise.
>>>>>>           * ld-arm/tls-longplt.s: Likewise.
>>>>>
>>>>> Why is this needed?  This code doesn't look like it needs to assume
>>>>> v6t2
>>>> as a
>>>>> baseline.
>>>>>
>>>>> R.
>>>>
>>>> Thanks for review. Before my patch, the target arch for those .s files
>>>> are calculated in an inaccurate way, the armv6t2 is assumed here.
>>>> Hence the content of .d files are for armv6t2. Now with my patch, the
>>>> arch for those files are calculated in an accurate way, the armv4t is
>>>> assumed here, so the existing .d files can't be matched. There are two
>>>> ways to avoid such mismatch, one is to explicitly set arch to armv6t2
>>>> in .s files, then no need to change the .d files. Another is to update
>>>> the .d files without touching the .s files. Which way is better in your
>> opinion?
>>>>
>>>
>>> Except that you do end up modifying the .d files since the NOP opcode used
>>> is different.  So what else changes if you don't set the architecture?
>>>
>>>
>> Before my patch, the arch is assumed to armv6t2, so 'blx 81ac' is legal
>> instruction. With my patch, the arch is assumed to armv5t, the 'blx 81ac' is
>> illegal, thus an extra veneer code are needed for such jump. This causes a
>> lot of changes to .d file.
>>
>> BR,
>> Terry
>>
>>
> 
> So why don't you use armv5t as the baseline, rather than v6t2?  Then not
> even the NOP would change in the .d files.
> 
> Either way, your changelog doesn't say anything useful.  Updated should
> only really be used for auto-generated things.
> 
> R.
> 
> 

I've been looking at this again.  It appears that the code is relying
not just on the presence of BLX in the ISA, but also on the range of
that instruction.  This was increased in ARMv6T2 to add another couple
of bits.  As such, I now feel that your existing patch to this test is
OK.  Please just fix the ChangeLog - something like:

           * ld-arm/tls-longplt-lib.s: Require ARMv6T2.
           * ld-arm/tls-longplt.s: Likewise.
           * ld-arm/tls-longplt-lib.d: Updated.
           * ld-arm/tls-longplt.d: Likewise.

R.