Re: [PATCH, ARM 4/7] Add support for linking ARMv8-M object files

["Richard Earnshaw (lists)" <Richard dot Earnshaw at arm dot com>]

On 23/12/15 07:02, Thomas Preud'homme wrote:
> Hi Richard,
> 
>> From: binutils-owner@sourceware.org [mailto:binutils-
>> owner@sourceware.org] On Behalf Of Richard Earnshaw (lists)
>> Sent: Friday, December 18, 2015 10:50 PM
>>
>>>
>>>
>>> diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
>>> index
>> 49dfc5338e45fd1df518cc8781e918444dc1831e..b10a3430e2d0cd5ad22f13
>> 6aa5f706506a40759e 100644
>>> --- a/bfd/elf32-arm.c
>>> +++ b/bfd/elf32-arm.c
>>> @@ -3445,18 +3445,8 @@ create_ifunc_sections (struct bfd_link_info
>> *info)
>>>  static bfd_boolean
>>>  using_thumb_only (struct elf32_arm_link_hash_table *globals)
>>>  {
>>> -  int arch = bfd_elf_get_obj_attr_int (globals->obfd, OBJ_ATTR_PROC,
>>> -				       Tag_CPU_arch);
>>> -  int profile;
>>> -
>>> -  if (arch == TAG_CPU_ARCH_V6_M || arch ==
>> TAG_CPU_ARCH_V6S_M)
>>> -    return TRUE;
>>> -
>>> -  if (arch != TAG_CPU_ARCH_V7 && arch != TAG_CPU_ARCH_V7E_M)
>>> -    return FALSE;
>>> -
>>> -  profile = bfd_elf_get_obj_attr_int (globals->obfd, OBJ_ATTR_PROC,
>>> -				      Tag_CPU_arch_profile);
>>> +  int profile = bfd_elf_get_obj_attr_int (globals->obfd,
>> OBJ_ATTR_PROC,
>>> +					  Tag_CPU_arch_profile);
>>>
>>>    return profile == 'M';
>>>  }
>>
>>
>> I don't think it's completely mandatory that when an Arch tag specifies
>> an M-profile architecture that setting the profile tag is also required
>>  (don't forget that we may be processing object files produced by other
>> toolchains so we have to be as liberal as we can be in terms of what we
>> accept).  So the following two test files are probably legal, although
>> not standard with the GNU tools.
> 
> You're absolutely right. Be liberal in what you accept, and strict in what you produce.
> 
>>
>> t1.s:
>> 	.eabi_attribute	Tag_CPU_arch, 11	@ V6-M
>> 	.thumb
>> 	.type _start, function
>> 	.global _start
>> 	.text
>> _start:
>> 	bl	myfunc
>> 	b	.
>>
>> t2.s:
>> 	.eabi_attribute	Tag_CPU_arch, 11	@ V6-M
>> 	.thumb
>> 	.type myfunc, function
>> 	.global myfunc
>> 	.text
>> 	.space 102400000
>> myfunc:
>> 	bx	lr
>>
>> Now, if I link these together, I expect the linker to generate a veneer
>> which is compatible with execution on ARMv6-M devices.  With the
>> current
>> linker I get that behaviour:
>>
>> 00008000 <_start>:
>>     8000:       f000 f802       bl      8008 <__myfunc_veneer>
>>     8004:       e7fe            b.n     8004 <_start+0x4>
>>         ...
>>
>> 00008008 <__myfunc_veneer>:
>>     8008:       b401            push    {r0}
>>     800a:       4802            ldr     r0, [pc, #8]    ; (8014
>> <__myfunc_veneer+0xc>)
>>     800c:       4684            mov     ip, r0
>>     800e:       bc01            pop     {r0}
>>     8010:       4760            bx      ip
>>     8012:       bf00            nop
>>     8014:       061b0019        .word   0x061b0019
>>         ...
>>
>> 00080000 <_stack>:
>>         ...
>>
>> 061b0018 <myfunc>:
>>  61b0018:       4770            bx      lr
>>
>> However, when I apply your patch I then get the tools creating veneers
>> that use instructions that do not exist on M-profile devices:
>>
>> 00008000 <_start>:
>>     8000:       f000 e802       blx     8008 <__myfunc_veneer>
>>     8004:       e7fe            b.n     8004 <_start+0x4>
>>         ...
>>
>> 00008008 <__myfunc_veneer>:
>>     8008:       e51ff004        ldr     pc, [pc, #-4]   ; 800c
>> <__myfunc_veneer+0x4>
>>     800c:       061b0011        .word   0x061b0011
>>         ...
>>
>> 00080000 <_stack>:
>>         ...
>>
>> 061b0010 <myfunc>:
>>  61b0010:       4770            bx      lr
>>
>> which is clearly not what we want.
>>
>> If you're going to continue to examine the build attributes directly
>> rather than internalising them and canonicalizing the set of attributes
>> into a consistent internal description of the machine capabilities, then
>> I think you need to continue to explicitly check the architecture tags
>> as well here.
> 
> This is fixed in this updated patch. I reworked the logic a tad to look at profile first and use that if available, otherwise fallback on arch information. Here are the updated ChangeLog entries and patch:
> 
> *** bfd/ChangeLog ***
> 
> 2015-12-22  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> 
>         * elf32-arm.c (using_thumb_only): Check that profile is 'M' and update
>         logic around Tag_CPU_arch values to return TRUE for ARMv8-M
>         architectures.
>         (tag_cpu_arch_combine): Define v8m_baseline and v8m_mainline and update
>         v4t_plus_v6_m and comb to deal with ARMv8-M Tag_CPU_arch merging logic.
>         (elf32_arm_merge_eabi_attributes): Add Tag_CPU_name values for
>         ARMv8-M.
> 
> 
> *** bfd/testsuite/ChangeLog ***
> 
> 2015-12-14  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> 
>         * ld-arm/arm-elf.exp (armeabitests_common): Run new tests 
>         "Thumb-Thumb farcall v8-M", "EABI attribute merging 8",
>         "EABI attribute merging 9" and "EABI attribute merging 10".
>         (Thumb-Thumb farcall v8-M): Renamed to ...
>         (Thumb-Thumb farcall v8-M Mainline): This.
>         (Thumb-Thumb farcall v8-M Baseline): New test. 
>         * ld-arm/attr-merge-8a.s: New file.
>         * ld-arm/attr-merge-8b.s: Likewise.
>         * ld-arm/attr-merge-8.attr: Likewise.
>         * ld-arm/attr-merge-9a.s: Likewise.
>         * ld-arm/attr-merge-9b.s: Likewise.
>         * ld-arm/attr-merge-9.out: Likewise.
>         * ld-arm/attr-merge-10a.s: Likewise.
>         * ld-arm/attr-merge-10b.s: Likewise.
>         * ld-arm/attr-merge-10.attr: Likewise.
> 
> 

This is OK.

As a follow-up could you please convert the example I gave into a
testcase?  I'll pre-approve that :-)

R.