gas may assemble b to locally-defined, preemptible global symbol as "b.n"

Bug #725126 reported by Dave Martin on 2011-02-25
12
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Cortex String Routines
Invalid
Undecided
Unassigned
Linaro Binutils
Won't Fix
Undecided
Unassigned
binutils
Fix Released
Medium
armel-cross-toolchain-base (Ubuntu)
Undecided
Unassigned
binutils (Ubuntu)
Undecided
Unassigned

Bug Description

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 when the compiler optimises a sibling call to "b <label>", for which the assembler generates a b.n.

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

Dave Martin (dave-martin-arm) wrote :
description: updated
description: updated
Changed in cortex-strings:
status: New → Invalid

Created attachment 5270
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.

Created attachment 5271
sample disassembly and relocation info

Dave Martin (dave-martin-arm) wrote :

Any further interaction should go to sourceware bugzilla
http://sourceware.org/bugzilla/show_bug.cgi?id=12532

Created attachment 5662
Do not relax relocations to preemptable global symbols

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

(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

CVSROOT: /cvs/src
Module name: src
Changes by: <email address hidden> 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

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

On Tue, Apr 12, 2011 at 12:49 PM, nickc at redhat dot com
<email address hidden> 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

CVSROOT: /cvs/src
Module name: src
Changes by: <email address hidden> 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

Dave Martin (dave-martin-arm) wrote :

This bug is now resolved upstream -- see the bugzilla link.

I recommend that this gets pulled/backported into the linaro toolchain, since we currently have to disable some optimisations when building the Linux kernel in Thumb-2 to work around this bug.

I am out of the office until 17/04/2011.

Note: This is an automated response to your message "[Bug 725126] Re: gas
may assemble b to locally-defined, preemptible global symbol as "b.n""
sent on 13/4/2011 14:23:05.

This is the only notification you will receive while this person is away.

Changed in binutils:
importance: Unknown → Medium
status: Unknown → Fix Released
Marcin Juszkiewicz (hrw) wrote :

Can you check with 11.04 release packages did it got fixed?

Michael Hope (michaelh1) on 2011-06-06
Changed in binutils-linaro:
status: New → Won't Fix
Dave Martin (dave-martin-arm) wrote :

Apologies for the delay.

I can confirm that binutils-arm-linux-gnueabi/natty (2.21.0.20110327-2ubuntu2cross1.62) does indeed fix the problem. I'll try rebuilding the linaro kernel without the workaround in place and see if it works; if so, we may be able to remove that workaround from the affected kernel builds.

...however, the problem is over-fixed: now, gas resolves _all_ branches to using 32-bit branches, which is suboptimal.

That's suboptimality rather than breakage, though; I might raise that issue separately, but this particular bug is fixed.

Dave Martin (dave-martin-arm) wrote :

Please ignore my last post -- I was inadvertantly testing a local, patched version of gas instead of the packaged one.

The bug is *not* fixed in the natty released version of the tools.

Marcin Juszkiewicz (hrw) wrote :

Does problem exist in oneiric?

Marcin Juszkiewicz (hrw) wrote :

does not happen under oneiric in cross toolchain

Changed in armel-cross-toolchain-base (Ubuntu):
status: New → Fix Released
Changed in binutils (Ubuntu):
status: New → Fix Released
Marcin Juszkiewicz (hrw) wrote :

Does not happen on armel using native binutils.

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

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.