Re: [Patch ping] Re: Problems with branch-to-arm-from-thumb for typeless symbol

[Hans-Peter Nilsson <hans-peter dot nilsson at axis dot com>]

Another ping.  Executive summary: fix for PR ld/15217; fixing an
inconsistency in the borderland of undefined behavior (stubs
from thumb to untyped symbols only switches mode sometimes),
introduced accidentally causing different behavior to binutils
<= 2..21 (which consistently emits stubs that changes mode).
Plus, offer to make the linker emit a warning for any
thumb->untyped symbol stub.

> From: Hans-Peter Nilsson <hp@axis.com>
> Date: Fri, 15 Mar 2013 20:35:56 +0100

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