PR9743 / ARM: long branch stubs to PLT

[Christophe LYON]

Thanks for the fast review!

On 06.02.2009 17:25, Daniel Jacobowitz wrote:
> On Thu, Feb 05, 2009 at 05:28:22PM +0100, Christophe LYON wrote:
>> - I removed some interworking warnings because I don't feel very  
>> confortable with them. More skilled people are welcome to explain me if I 
>> got it wrong.
> 
> Could you expand on this?
> 
While I was working on this improvement, I got these warnings in 
unexpected cases. I read several ARM docs to figure out what was wrong, 
but I couldn't understand. I no longer have the case at hand, sorry.

> The warning applies to pre-EABI code.  The stubs won't help if you're
> going to return from the called function using "mov pc".
So I should put them back?

> In general - if it's not too much of a hassle - it helps to do
> reformating patches separately from real changes.  In this case, that
> means breaking out the renaming from the rest.  I won't insist, but
> it sure does make it easier to see what's going on.

I have wondered about the best approach; I should have submitted a 
renaming-only patch first. I could probably propose one if you want.

>> -#define CHANGE_TO_ARM "__%s_change_to_arm"
>> -#define BACK_FROM_ARM "__%s_back_from_arm"
> 
> Nothing else generates the CHANGE_TO_ARM naming, and you didn't delete
> a test case... that means there wasn't a test case for this feature,
> boo.  So I can't see what effect you've had on whatever this used to
> handle.  Is this one of the new stubs?
> 
> (BACK_FROM_ARM was already unused...)
> 
R_ARM_THM_CALL and R_ARM_THM_JUMP24 are now handled by the new stubs. 
Those strings used to be symbols inside the older stubs (for readability 
I guess).

Indeed I have found several omissions in the test cases, but did not 
take time to fix them all.

>> diff -uN -r --exclude=CVS --exclude='*~' --exclude='.#*' binutils-cvs-ref/src/ld/testsuite/ld-arm/arm-call.d binutils-cvs/src/ld/testsuite/ld-arm/arm-call.d
>> --- binutils-cvs-ref/src/ld/testsuite/ld-arm/arm-call.d	2007-05-22 19:17:53.000000000 +0200
>> +++ binutils-cvs/src/ld/testsuite/ld-arm/arm-call.d	2009-02-02 15:21:10.000000000 +0100
> 
> Why'd this file change from __t1_from_arm to __t1_veneer?
I did not want to change the code you inserted to give names to the 
stubs, as I was unsure about the historical reasons you mention in the 
comment.

> Also, this whole "putting stubs at the front of .text" is going to be
> a real problem.  We've only just updated our internal binutils, and I
> haven't had time to construct a test case yet, but I'm expecting this
> to mess up interrupt vectors in our startup code.  I don't suppose
> it's easy to put them at the end instead?
> 
You have already expressed your opinion about that :-)
The rationale was to use the same scheme as other architectures (HPPA, 
PPC), from where I grabbed generic parts. Are there known problems on 
those architectures?

>> diff -uN -r --exclude=CVS --exclude='*~' --exclude='.#*' binutils-cvs-ref/src/ld/testsuite/ld-arm/arm-elf.exp binutils-cvs/src/ld/testsuite/ld-arm/arm-elf.exp
>> --- binutils-cvs-ref/src/ld/testsuite/ld-arm/arm-elf.exp	2009-01-23 14:13:45.000000000 +0100
>> +++ binutils-cvs/src/ld/testsuite/ld-arm/arm-elf.exp	2009-02-05 16:04:43.000000000 +0100
>> @@ -190,10 +190,6 @@
>>  # Exclude non-ARM-EABI targets.
>>  
>>  if { ![istarget "arm*-*-*eabi"] } {
>> -    # Special variants of these tests, as no farcall stub is generated
>> -    # for a non-ARM-EABI target
>> -    run_dump_test "thumb2-bl-as-thumb1-bad-noeabi"
>> -    run_dump_test "thumb2-bl-bad-noeabi"
>>      return
>>  }
> 
> Why?  Did you remove the non-EABI support?

This test used to give a linker error, because it was not supported 
(XFAIL). Now the link succeeds, but the generated stub is different from 
the EABI one (no use_blx property). I did not write a specific expected 
result, and preferred to remove the non-EABI test. Do you want me to 
write the non-EABI variant?

>> -    {"ARM-Thumb farcall with BLX (PIC veneer)" "-Ttext 0x1000 --section-start .foo=0x2001014 --pic-veneer" "-march=armv5t" {farcall-arm-thumb.s}
>> +    {"ARM-Thumb farcall with BLX (PIC veneer)" "-Ttext 0x1000 --section-start .foo=0x2002014 --pic-veneer" "-march=armv5t" {farcall-arm-thumb.s}
> 
> So that's why the addresses in the test all changed; could you explain
> why you made this change?
With the older address, after inserting the stub, the branch gets close 
enough to remain a direct branch, so I put it a little further. 
Previously the linker did not notice it was close enough (the code in 
elf32_arm_final_link_relocate is now more clever when deciding to use a 
stub -- see line 6122)

> In general, I'm going to ask about any change to an existing
> testcase's output.  Yes, I'm that picky.
No problem, it's a good thing.
Actually, I would have liked to document such changes but I didn't know 
what was the appropriate place (the ChangeLog entries are usually not so 
informative).

>> diff -uN -r --exclude=CVS --exclude='*~' --exclude='.#*' binutils-cvs-ref/src/ld/testsuite/ld-arm/farcall-arm-thumb-pic-veneer.d binutils-cvs/src/ld/testsuite/ld-arm/farcall-arm-thumb-pic-veneer.d
>> --- binutils-cvs-ref/src/ld/testsuite/ld-arm/farcall-arm-thumb-pic-veneer.d	2008-06-25 16:28:48.000000000 +0200
>> +++ binutils-cvs/src/ld/testsuite/ld-arm/farcall-arm-thumb-pic-veneer.d	2009-02-02 15:40:05.000000000 +0100
>> @@ -3,10 +3,10 @@
>>  Disassembly of section .text:
>>  
>>  00001000 <__bar_from_arm>:
>> -    1000:	e59fc000 	ldr	ip, \[pc, #0\]	; 1008 <__bar_from_arm\+0x8>
>> -    1004:	e08ff00c 	add	pc, pc, ip
>> -    1008:	0200000d 	.word	0x0200000d
>> -    100c:	00000000 	.word	0x00000000
>> +    1000:	e59fc004 	ldr	ip, \[pc, #4\]	; 100c <__bar_from_arm\+0xc>
>> +    1004:	e08cc00f 	add	ip, ip, pc
>> +    1008:	e12fff1c 	bx	ip
>> +    100c:	02000009 	.word	0x02000009
> 
> Why the switch from a v5 stub to a v4t stub?
> 
This test is intended for v4t. farcall-arm-thumb-blx-pic-veneer.d is 
intented for v5. I was also thinking that some renaming in the tests 
would help, but choose to wait for another patch to propose that :-)

Christophe.