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

[Dave Martin]

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

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.

[Dave Martin (dave-martin-arm)]

Created attachment 5271
sample disassembly and relocation info

[Dave Martin (dave-martin-arm)]

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

[Nickc (nickc)]

Created attachment 5662
Do not relax relocations to preemptable global symbols

[Nickc (nickc)]

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

[Dave Martin (dave-martin-arm)]

(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

[Cvs-commit (cvs-commit)]

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

[Nickc (nickc)]

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

[Dave Martin (dave-martin-arm)]

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

[Cvs-commit (cvs-commit)]

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

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.

[Ira Rosen (irar)]

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.

[Marcin Juszkiewicz (hrw)]

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

[Dave Martin (dave-martin-arm)]

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

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

Does problem exist in oneiric?

[Marcin Juszkiewicz (hrw)]

does not happen under oneiric in cross toolchain

[Marcin Juszkiewicz (hrw)]

Does not happen on armel using native binutils.

[Jason A. Donenfeld (zx2c4)]

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

[Jason A. Donenfeld (zx2c4)]

Tracking the new bug here now: https://sourceware.org/bugzilla/show_bug.cgi?id=26141