This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[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
> 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]