This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
[Patch ping] Re: Problems with branch-to-arm-from-thumb for typeless symbol
- From: Hans-Peter Nilsson <hans-peter dot nilsson at axis dot com>
- To: rearnsha at arm dot com, binutils at sourceware dot org
- Date: Fri, 15 Mar 2013 20:35:56 +0100
- Subject: [Patch ping] Re: Problems with branch-to-arm-from-thumb for typeless symbol
> From: Hans-Peter Nilsson <hp@axis.com>
> Date: Mon, 4 Mar 2013 04:40:27 +0100
Ping for the patch below.
I can add a linker warning for stubs from thumb to untyped
symbols if you like, but consistently for any thumb->arm, not
just inconsistently for the not-thumbmarked-but-in-range case in
question. I.e. also for ld/testsuite/ld-arm/fix-arm1176.s
(where a stub is triggered due to the branch being out of range)
and of course fixing that testcase; the branch target having no
type there seems just an oversight.
> > From: Richard Earnshaw <rearnsha@arm.com>
> > Date: Fri, 1 Mar 2013 15:16:39 +0100
>
> > On 01/03/13 05:23, Hans-Peter Nilsson wrote:
> > > See <http://sourceware.org/bugzilla/show_bug.cgi?id=15217>. Are
> > > target symbols required to have a type specified in order for
> > > arm/thumb stubs to work?
> > >
> > Yes.
>
> That's definitely the hard-and-fast rule for well-behaving
> AAELF-conformant code. But, the current linker behavior is both
> inconsistent and apparently an unintended change, so it should
> change back. At the very least, I hope you agree the linker
> behavior should be consistent and shouldn't be allowed to
> oscillate.
>
> I did a little digging. You know all this, but for the record,
> AAELF (ELF for the ARM Architecture; the BPAPI for ARM in ELF
> parlance) as per
> <http://infocenter.arm.com/help/topic/com.arm.doc.ihi0044e/IHI0044E_aaelf.pdf>
> says in "4.5.2 Symbol Types": "All code symbols exported from an
> object file (symbols with binding STB_GLOBAL) shall have type
> STT_FUNC", but I'm not sure whether that implies there's a bug
> in the assembler or if it just means to restrict where AAELF
> applies. Maybe best leave current behavior as-is there. AAELF
> seems to open up for the existence of untyped code symbols when
> it later states "The linker is only required to provide
> interworking support for symbols of type STT_FUNC (interworking
> for untyped symbols must be encoded directly in the object
> file)." Note "only required to", not "required only to",
> leaving whether it also does it for untyped symbols is optional.
> (Regarding splitting hairs, there's a "must" there, but as
> noted, it already accepted an exception to a "shall".)
>
> The issue would not have arisen if behavior had been consistent
> across versions. Indeed, a mode-changing stub *used* to be
> generated for the test-case; it was changed in binutils-1.21
> (apparently CVS 1.261 of elf32-arm.c), but the behavioral change
> seems to have been unintended; it wasn't mentioned as such in
> the patch, the post or covered by the test-suite. This would
> IMHO have been a newsworthy item. See
> <http://sourceware.org/ml/binutils/2011-03/msg00055.html>, where
> apparently this change was introduced (arm_type_of_stub as
> below) when doing a rewrite as part of ARM support for IFUNC,
> but the specific behavioral change is not noted in that thread,
> references or test-suite changes, while being a newsworthy
> change, silently causing such existing code to "fail".
>
> Also, the current linker behavior is inconsistent; if the branch
> is out-of-range, a mode-changing-stub *is* generated. Note the
> lack of symbol type specification for the global symbol
> func_to_branch_to in e.g. ld/testsuite/ld-arm/fix-arm1176.s that
> is used to trigger a stub or change to blx!
>
> Regarding the impact of this unintended change for borderline
> code, I'm guessing a low count of thousands of sites with code
> that assumes stubs are generated for calls from thumb to arm for
> untyped symbols, developed using binutils-2.21 or before back to
> at least 2.18. Ok, that was toungue-in-cheek as I'm including
> the linker test-suite as above but there are at least
> more-than-three; the chip vendor from whom we got the code where
> this behavior "changed" and their customers, which may or may
> not include the people at
> <http://sourceware.org/ml/binutils/2012-03/msg00121.html>.
>
> > The assumption is that if you call an untyped symbol the code knows what
> > it is doing and that special processing for interworking is not
> > required. Furthermore, the guarantee that IP (r12) is free as an
> > interworking register is not available for untyped symbol interfaces.
>
> I know I stated my question generally, but the issue is just
> with stubs *from thumb to arm* so IIUC clobbering r12/ip isn't
> an issue.
>
> I'm suggesting we change back behavior to always generate
> mode-changing stubs from thumb to arm as per below. Either way,
> consistency is expected and I can't see how this could cause
> harm for users, in contrast to the current situation.
>
> Checked arm-linux-eabi and arm-eabi.
>
> Ok to commit?
>
> bfd:
> PR ld/15217
> * elf32-arm.c (arm_type_of_stub): Match calls to untyped
> symbols as calls to ARM mode.
>
> diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
> index aba1814..9c3e5a2 100644
> --- a/bfd/elf32-arm.c
> +++ b/bfd/elf32-arm.c
> @@ -3637,7 +3637,9 @@ arm_type_of_stub (struct bfd_link_info *info,
> || (thumb2
> && (branch_offset > THM2_MAX_FWD_BRANCH_OFFSET
> || (branch_offset < THM2_MAX_BWD_BRANCH_OFFSET)))
> - || (branch_type == ST_BRANCH_TO_ARM
> + || ((branch_type == ST_BRANCH_TO_ARM
> + /* Match calls to untyped symbols as calls to ARM. */
> + || (branch_type == ST_BRANCH_UNKNOWN && hash != NULL))
> && (((r_type == R_ARM_THM_CALL
> || r_type == R_ARM_THM_TLS_CALL) && !globals->use_blx)
> || (r_type == R_ARM_THM_JUMP24))
>
> ld/testsuite:
> PR ld/15217
> * ld-arm/app-notype.s, ld-arm/lib-notype.s,
> ld-arm/interwork-notype.d: New test.
> * ld-arm/arm-elf.exp: Run it.
>
> diff --git a/ld/testsuite/ld-arm/app-notype.s b/ld/testsuite/ld-arm/app-notype.s
> new file mode 100644
> index 0000000..38cb3a5
> --- /dev/null
> +++ b/ld/testsuite/ld-arm/app-notype.s
> @@ -0,0 +1,35 @@
> +# Check that calls to untyped symbols get stubs.
> +# The lack of symbol types below is intended; the commented-out
> +# .type lines show proper symbol type designation.
> +
> + .text
> + .p2align 4
> + .globl _start
> +_start:
> + mov ip, sp
> + stmdb sp!, {r11, ip, lr, pc}
> + bl app_tfunc
> + ldmia sp, {r11, sp, lr}
> + bx lr
> +
> + .p2align 4
> + .globl app_tfunc
> +# .type app_tfunc,%function
> + .thumb_func
> + .code 16
> +app_tfunc:
> + push {lr}
> + bl lib_func2
> + pop {pc}
> + bx lr
> +
> + .p2align 4
> + .globl app_tfunc2
> +# .type app_tfunc2,%function
> + .thumb_func
> + .code 16
> +app_tfunc2:
> + push {lr}
> + bl app_tfunc
> + pop {pc}
> + bx lr
> diff --git a/ld/testsuite/ld-arm/arm-elf.exp b/ld/testsuite/ld-arm/arm-elf.exp
> index d9375ec..ea0beba 100644
> --- a/ld/testsuite/ld-arm/arm-elf.exp
> +++ b/ld/testsuite/ld-arm/arm-elf.exp
> @@ -789,6 +789,7 @@ run_dump_test "attr-merge-vfp-7"
> run_dump_test "attr-merge-vfp-7r"
> run_dump_test "attr-merge-incompatible"
> run_dump_test "unresolved-1"
> +run_dump_test "interwork-notype"
> if { ![istarget "arm*-*-nacl*"] } {
> run_dump_test "unresolved-1-dyn"
> }
> diff --git a/ld/testsuite/ld-arm/interwork-notype.d b/ld/testsuite/ld-arm/interwork-notype.d
> new file mode 100644
> index 0000000..f36cb18
> --- /dev/null
> +++ b/ld/testsuite/ld-arm/interwork-notype.d
> @@ -0,0 +1,63 @@
> +# source: app-notype.s
> +# source: lib-notype.s
> +# ld: --fix-arm1176
> +# objdump: -d
> +
> +.*: file format elf32-littlearm
> +
> +Disassembly of section .text:
> +
> +[0-9a-f]+ <_start>:
> + [0-9a-f]+: e1a0c00d mov ip, sp
> + [0-9a-f]+: e92dd800 push {fp, ip, lr, pc}
> + [0-9a-f]+: eb000016 bl [0-9a-f]+ <__app_tfunc_from_arm>
> + [0-9a-f]+: e89d6800 ldm sp, {fp, sp, lr}
> + [0-9a-f]+: e12fff1e bx lr
> + [0-9a-f]+: e1a00000 nop .*
> + [0-9a-f]+: e1a00000 nop .*
> + [0-9a-f]+: e1a00000 nop .*
> +
> +[0-9a-f]+ <app_tfunc>:
> + [0-9a-f]+: b500 push {lr}
> + [0-9a-f]+: f000 f81d bl [0-9a-f]+ <__lib_func2_(from_thumb|veneer)>
> + [0-9a-f]+: bd00 pop {pc}
> + [0-9a-f]+: 4770 bx lr
> + [0-9a-f]+: 46c0 nop .*
> + [0-9a-f]+: 46c0 nop .*
> + [0-9a-f]+: 46c0 nop .*
> +
> +[0-9a-f]+ <app_tfunc2>:
> + [0-9a-f]+: b500 push {lr}
> + [0-9a-f]+: f7ff fff5 bl [0-9a-f]+ <app_tfunc>
> + [0-9a-f]+: bd00 pop {pc}
> + [0-9a-f]+: 4770 bx lr
> + [0-9a-f]+: 46c0 nop .*
> + [0-9a-f]+: 46c0 nop .*
> + [0-9a-f]+: 46c0 nop .*
> +
> +[0-9a-f]+ <lib_func2>:
> + [0-9a-f]+: e1a0c00d mov ip, sp
> + [0-9a-f]+: e92dd800 push {fp, ip, lr, pc}
> + [0-9a-f]+: eb000009 bl [0-9a-f]+ <__app_tfunc2_from_arm>
> + [0-9a-f]+: e89d6800 ldm sp, {fp, sp, lr}
> + [0-9a-f]+: e12fff1e bx lr
> + [0-9a-f]+: e1a00000 nop .*
> + [0-9a-f]+: e1a00000 nop .*
> + [0-9a-f]+: e1a00000 nop .*
> +
> +[0-9a-f]+ <__lib_func2_(from_thumb|veneer)>:
> + [0-9a-f]+: 4778 bx pc
> + [0-9a-f]+: 46c0 nop .*
> +#...
> + [0-9a-f]+: eafffff5 b [0-9a-f]+ <lib_func2>
> +
> +[0-9a-f]+ <__app_tfunc_from_arm>:
> + [0-9a-f]+: e59fc000 ldr ip, \[pc[^]]*\] ; [0-9a-f]+ <__app_tfunc_from_arm\+0x8>
> + [0-9a-f]+: e12fff1c bx ip
> + [0-9a-f]+: ........ .word 0x........
> +
> +[0-9a-f]+ <__app_tfunc2_from_arm>:
> + [0-9a-f]+: e59fc000 ldr ip, \[pc[^]]*\] ; [0-9a-f]+ <__app_tfunc2_from_arm\+0x8>
> + [0-9a-f]+: e12fff1c bx ip
> + [0-9a-f]+: ........ .word 0x........
> +#...
> diff --git a/ld/testsuite/ld-arm/lib-notype.s b/ld/testsuite/ld-arm/lib-notype.s
> new file mode 100644
> index 0000000..ba84183
> --- /dev/null
> +++ b/ld/testsuite/ld-arm/lib-notype.s
> @@ -0,0 +1,14 @@
> + .text
> +
> + .p2align 4
> + .globl lib_func2
> +# The lack of symbol type is intended. Proper code would have a
> +# symbol type specified, like:
> +# .type lib_func2, %function
> +lib_func2:
> + mov ip, sp
> + stmdb sp!, {r11, ip, lr, pc}
> + bl app_tfunc2
> + ldmia sp, {r11, sp, lr}
> + bx lr
> + .size lib_func2, . - lib_func2
>
> brgds, H-P
>