This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: PR9743 / ARM: long branch stubs to PLT
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.