The 32-bit Thumb long branch encoding has a range of +/-16MiB, but gas checks +/-32MiB instead (which is, perhaps coincidentally, the range of the B instruction in ARM state). This can result in bogus branch offsets in the assembly output. Having checked the ARM ARM, this looks like a big in gas, not objdump. Since this behaviour only affects long distance branches within a single section (i.e., branches fixed up directly by the assembler) it is unlikely to be seen often in the wild. (*) indicates the incorrect branch instructions. $ binutils/gas/as-new -mthumb -o tst.o tst.s && arm-linux-gnueabi-objdump -d tst.o tst.o: file format elf32-littlearm Disassembly of section .text: 00000000 <e>: 0: f3ff 97ff b.w 1000002 <f+0xffffee> (*) 4: f400 9000 b.w ff000008 <g+0xfd000000> 8: f3ff 97ff b.w 100000a <f+0xfffff6> c: f400 9000 b.w ff000010 <g+0xfd000008> (*) 10: f7ff bfff b.w 12 <e+0x12> (*) 00000014 <f>: 14: f7ff bff8 b.w 8 <e+0x8> (*) ... 02000008 <g>: 2000008: f000 b804 b.w 2000014 <g+0xc> (*) $ cat tst.s .syntax unified .type f, %function e: b . - 0xfffffe @ gas mis-assembles as a forward branch b . - 0xfffffc b . + 0x1000002 b . + 0x1000004 @ gas mis-assembles as a backward branch b . + 0x2000002 @ gas mis-assembles as a backward branch @ b . + 0x2000004 @ gas correctly detects this as out of range f: b g @ gas mis-assembles as a backward branch .space 0x1fffff0 g: b f @ gas mis-assembles as a forward branch @ If "b.w" is used instead of "b", the results are exactly the same.
On Mon, Jun 6, 2011 at 12:18 PM, dave.martin at linaro dot org <sourceware-bugzilla@sourceware.org> wrote: > http://sourceware.org/bugzilla/show_bug.cgi?id=12848 > > Summary: ARM: Thumb-2: Range check on b.w is off by a factor of > 2 > Product: binutils > Version: 2.22 (HEAD) > Status: NEW > Severity: normal > Priority: P2 > Component: gas > AssignedTo: unassigned@sources.redhat.com > ReportedBy: dave.martin@linaro.org > > > The 32-bit Thumb long branch encoding has a range of +/-16MiB, but gas checks > +/-32MiB instead (which is, perhaps coincidentally, the range of the B > instruction in ARM state). Having played with this a bit more, it appears that all the 32-bit Thumb branch encodings are affected. In each case, the range checked for is double what it should be: b.w, bl and blx seem to be range-checked as +/- 32 MiB (should be +/- 16 MiB) b<cc>.w seems to be range-checked as +/- 2 MiB (should be +/- 1 MiB)
CVSROOT: /cvs/src Module name: src Changes by: nickc@sourceware.org 2011-06-30 12:52:58 Modified files: gas : ChangeLog gas/config : tc-arm.c Log message: PR gas/12848 * config/tc-arm.c (BAD_RANGE): New error message define. (md_apply_fix): Use it. Fix range check for thumb branch instructions. Patches: http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/ChangeLog.diff?cvsroot=src&r1=1.4534&r2=1.4535 http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/config/tc-arm.c.diff?cvsroot=src&r1=1.496&r2=1.497
CVSROOT: /cvs/src Module name: src Changes by: nickc@sourceware.org 2011-06-30 13:05:04 Modified files: gas/testsuite : ChangeLog Added files: gas/testsuite/gas/arm: thumb-b-bad.d thumb-b-bad.l thumb-b-bad.s Log message: PR 12848 * gas/arm/thumb-b-bad.s: New test. * gas/arm/thumb-b-bad.d: Test control file. * gas/arm/thumb-b-bad.l: Expected error output. Patches: http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/ChangeLog.diff?cvsroot=src&r1=1.1924&r2=1.1925 http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/arm/thumb-b-bad.d.diff?cvsroot=src&r1=NONE&r2=1.1 http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/arm/thumb-b-bad.l.diff?cvsroot=src&r1=NONE&r2=1.1 http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/arm/thumb-b-bad.s.diff?cvsroot=src&r1=NONE&r2=1.1
Hi Dave, I have checked in a patch which should address this problem. Please let me know if I have missed any of the checks though. Cheers Nick
(In reply to comment #4) > Hi Dave, > > I have checked in a patch which should address this problem. Please let me > know if I have missed any of the checks though. > > Cheers > Nick Some things are now fixed, but forward 32-bit conditional branches with ranges from . + 4 + 0x100000 to . + 4 + 0x1ffffe are still not handled correctly. Reverse branches with ranges of the same magnitude are correctly rejected though. It also occurred to me to check bl and blx instructions, since these have an encoding closely related to the 32-bit unconditional branch, and the same range. It turns out that these are wongly range-checked in the same way that the 32-bit unconditional branches were previously wrongly checked. I'll attach my extended testcase. I also have a patch which attempts to tidy up the branch range fixups, and reduce the amount of magic numbers floating around the code. I'm not sure whether it's appropriate/necessary to check alignment at the same time as checking the range, and it's possible that the macros I've added duplicate functionality already implemented somewhere else... Finally, should we get rid of all the variant branch-out-of-range messages? I think that BAD_RANGE is actually adequately descriptive for all these cases. Cheers ---Dave
Created attachment 5831 [details] Extended testcase
Created attachment 5832 [details] Patch which attempts to tidy up branch range checks, and fix the outstanding failures
Created attachment 5852 [details] Improved patch
Created attachment 5853 [details] Example of uncaught bad branch
Hi Dave, Thanks for the extended test case and revised patch. Unfortunately there are a couple of problems with this new patch which I hope you won't mind taking a look at: 1. The assembler can still miss a badly aligned BLX instruction. For example try assembling the uploaded fred.s file and then disassembling the result. You should see this: [...] 2000006: f400 f800 bl 1c0000a <f-0x3ffff6> 200000a: f3ff ffff bl 240000c <g+0x3ffff0> 200000e: f400 e801 ; <UNDEFINED> instruction: 0xf400e801 2000012: f000 e000 blx 2400014 <g+0x3ffff8> [...] 2. The patch will reported mis-aligned branches as being out of range. 3. For some reason, with some of the out of range branches we also get an error message about trying to stuff a large value into a small bitfield. Ideally there should only be one error message per problem in the assembler source file. 4. There ought to be a proper set of test cases in the assembler testsuite to check this sort of thing. I have uploaded a revised version of your patch which is my attempt to address points 2 and 4. But points 1 and 3 still need investigating. Would you mind doing the honours ? Cheers Nick