Bug 12532

Summary: arm: gas may assemble b to locally-defined, preemptible global symbol as "b.n"
Product: binutils Reporter: Dave Martin <dave.martin>
Component: gasAssignee: unassigned
Status: RESOLVED FIXED    
Severity: normal CC: jason, nickc
Priority: P2    
Version: 2.22   
Target Milestone: ---   
Host: Target:
Build: Last reconfirmed:
Attachments: Compiler output demonstrating the problem
sample disassembly and relocation info
Do not relax relocations to preemptable global symbols

Description Dave Martin 2011-03-02 11:26:32 UTC
Created attachment 5270 [details]
Compiler output demonstrating the problem

Gas sometimes emits a short branch and an R_ARM_THUMB_JUMP11 relocation for an unqualified branch ("b") to a global symbol defined in the same source file.

However, there's no guarantee that R_ARM_THM_JUMP11 relocation can be resolved correctly if the symbol gets preempted.

Observed in binutils-arm-linux-gnueabi 2.20.51.20100908-0ubuntu2cross1.52.
Observed on binutils trunk on 2011-02-25.

The problem seems to strike in practice when the compiler optimises a sibling call to "b <label>", for which the assembler generates a b.n.  Handed-coded assembler can also cause the same problem.

As a side-effect of the presence of this relocation, Thumb-2 kernel modules may fail to load, since the kernel doesn't support fixing up this relocation. I believe the kernel doesn't do any symbol preemption processing when loading modules -- if so, the relocation is actually redundant. But there's no way for the kernel to detect at module load time that the relocation can safely be ignored. In kernel builds, about 1 out every 6 modules suffers from this problem. It's not clear to me whether the method used by the kernel to link .ko objects is wrong or not.

It seems that either the tools should support symbol preemption in this scenario (implying that a short branch is not adequate to guarantee that preemption will work -- i.e., a gas bug) or not (in this case the fixup should be done locally and there should presumably be no relocation -- i.e., a different gas bug).

It appears that passing -fno-optimize-sibling-calls to GCC can work around the problem by avoiding the generation of local branches to global symbols, but this is clearly not the correct long-term fix.
Comment 1 Dave Martin 2011-03-02 11:27:55 UTC
Created attachment 5271 [details]
sample disassembly and relocation info
Comment 2 Nick Clifton 2011-04-11 16:18:01 UTC
Created attachment 5662 [details]
Do not relax relocations to preemptable global symbols
Comment 3 Nick Clifton 2011-04-11 16:19:38 UTC
Hi Dave,

  I think that we should be supporting the possibility of symbol preemption. So please could you try out the uploaded patch and let me know if it resolves the problem for you.

Cheers
  Nick

PS.  Hmm, I wonder if we should support a command line feature to enable/disable this behaviour, and what the default setting should be...
Comment 4 Dave Martin 2011-04-12 10:39:08 UTC
(In reply to comment #3)
> Hi Dave,
> 
>   I think that we should be supporting the possibility of symbol preemption. So
> please could you try out the uploaded patch and let me know if it resolves the
> problem for you.
> 
> Cheers
>   Nick
> 
> PS.  Hmm, I wonder if we should support a command line feature to
> enable/disable this behaviour, and what the default setting should be...

Your patch in http://sourceware.org/bugzilla/show_bug.cgi?id=12532#c2 seems to fix my instance of the problem.

Do you know when this is likely to get merged?  I don't see it in binutils trunk yet.

Cheers
---Dave
Comment 5 cvs-commit@gcc.gnu.org 2011-04-12 11:47:41 UTC
CVSROOT:	/cvs/src
Module name:	src
Changes by:	nickc@sourceware.org	2011-04-12 11:47:38

Modified files:
	gas            : ChangeLog 
	gas/config     : tc-arm.c 

Log message:
	PR gas/12532
	* config/tc-arm.c (relax_branch): Do not relax branches to
	preemptable global symbols.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/ChangeLog.diff?cvsroot=src&r1=1.4464&r2=1.4465
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/config/tc-arm.c.diff?cvsroot=src&r1=1.479&r2=1.480
Comment 6 Nick Clifton 2011-04-12 11:49:05 UTC
Hi Dave

> Do you know when this is likely to get merged?  I don't see it in binutils
> trunk yet.

That would be because I was waiting for your agreement that the patch worked...

Anyway it is all checked in now.

Cheers
  Nick
Comment 7 Dave Martin 2011-04-12 12:14:19 UTC
On Tue, Apr 12, 2011 at 12:49 PM, nickc at redhat dot com
<sourceware-bugzilla@sourceware.org> wrote:
> http://sourceware.org/bugzilla/show_bug.cgi?id=12532
>
> Nick Clifton <nickc at redhat dot com> changed:
>
>           What    |Removed                     |Added
> ----------------------------------------------------------------------------
>             Status|WAITING                     |RESOLVED
>         Resolution|                            |FIXED
>
> --- Comment #6 from Nick Clifton <nickc at redhat dot com> 2011-04-12 11:49:05 UTC ---
> Hi Dave
>
>> Do you know when this is likely to get merged?  I don't see it in binutils
>> trunk yet.
>
> That would be because I was waiting for your agreement that the patch worked...
>
> Anyway it is all checked in now.

OK!  Thanks

---Dave
Comment 8 cvs-commit@gcc.gnu.org 2011-04-12 15:44:40 UTC
CVSROOT:	/cvs/src
Module name:	src
Changes by:	nickc@sourceware.org	2011-04-12 15:44:36

Modified files:
	gas/testsuite  : ChangeLog 
	gas/testsuite/gas/arm: weakdef-1.d plt-1.d thumb2_bcond.d 

Log message:
	PR gas/12532
	* gas/arm/plt-1.d: Update expected disassembly.
	* gas/arm/thumb2_bcond.d: Likewise.
	* gas/arm/weakdef-1.d: Likewise.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/ChangeLog.diff?cvsroot=src&r1=1.1882&r2=1.1883
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/arm/weakdef-1.d.diff?cvsroot=src&r1=1.1&r2=1.2
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/arm/plt-1.d.diff?cvsroot=src&r1=1.1&r2=1.2
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/arm/thumb2_bcond.d.diff?cvsroot=src&r1=1.6&r2=1.7
Comment 9 Jason A. Donenfeld 2020-06-18 23:55:53 UTC
This problem still exists on binutils 2.33 when -fvisibility=hidden is passed to cflags. I imagine this is so due to some conflicting code where the forced B.W is only generated for static functions, since non-static ones will be relocated differently, but then because of -fvisibility=hidden, they get treated like statics, only B is used instead of the forced B.W, causing this issue to crop up again.

OpenWRT experienced this when including WireGuard on a new board. I fixed it like this: https://git.zx2c4.com/wireguard-linux-compat/commit/?id=178cdfffb99f2fd6fb4a5bfd2f9319461d93f53b
Comment 10 Jason A. Donenfeld 2020-06-19 19:23:51 UTC
Tracking the new bug here now: https://sourceware.org/bugzilla/show_bug.cgi?id=26141