Bug 12848 - ARM: Thumb-2: Range check on b.w is off by a factor of 2
Summary: ARM: Thumb-2: Range check on b.w is off by a factor of 2
Status: WAITING
Alias: None
Product: binutils
Classification: Unclassified
Component: gas (show other bugs)
Version: 2.22
: P2 normal
Target Milestone: ---
Assignee: unassigned
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-06 11:17 UTC by Dave Martin
Modified: 2011-07-20 09:56 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
Extended testcase (645 bytes, application/octet-stream)
2011-07-04 15:56 UTC, Dave Martin
Details
Patch which attempts to tidy up branch range checks, and fix the outstanding failures (1.10 KB, patch)
2011-07-04 15:58 UTC, Dave Martin
Details | Diff
Improved patch (3.16 KB, patch)
2011-07-20 09:49 UTC, Nick Clifton
Details | Diff
Example of uncaught bad branch (186 bytes, application/octet-stream)
2011-07-20 09:50 UTC, Nick Clifton
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Martin 2011-06-06 11:17:40 UTC
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.
Comment 1 Dave Martin 2011-06-08 14:14:07 UTC
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)
Comment 2 cvs-commit@gcc.gnu.org 2011-06-30 12:53:01 UTC
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
Comment 3 cvs-commit@gcc.gnu.org 2011-06-30 13:05:08 UTC
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
Comment 4 Nick Clifton 2011-06-30 13:09:58 UTC
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
Comment 5 Dave Martin 2011-07-04 15:54:35 UTC
(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
Comment 6 Dave Martin 2011-07-04 15:56:08 UTC
Created attachment 5831 [details]
Extended testcase
Comment 7 Dave Martin 2011-07-04 15:58:14 UTC
Created attachment 5832 [details]
Patch which attempts to tidy up branch range checks, and fix the outstanding failures
Comment 8 Nick Clifton 2011-07-20 09:49:38 UTC
Created attachment 5852 [details]
Improved patch
Comment 9 Nick Clifton 2011-07-20 09:50:22 UTC
Created attachment 5853 [details]
Example of uncaught bad branch
Comment 10 Nick Clifton 2011-07-20 09:56:40 UTC
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