[MC][ARM] Correct Thumb BL instruction range

[peter.smith]

Summary

The Thumb BL range is +- either 16 Megabytes or 4 Megabytes depending on whether the CPU supports Thumb2 or the v8-m baseline ops. The existing check for BL range is incorrectly set at +- 32 Megabytes. This change corrects the higher range and uses the lower range if the featurebits don't have the necessary support for it.

The ranges for the branch instructions can be found in https://developer.arm.com/docs/ddi0406/c/arm-architecture-reference-manual-armv7-a-and-armv7-r-edition
For version C (latest) the higher range can be found in A4.3 Branch instructions
For the shorter range search for "BL and BLX (immediate) instructions, before ARMv6T2", this also applies to ARMv6M.

I note that there isn't a range check for BLX, but as far as I can tell, the shouldForceRelocation() always generates an external relocation for this case.
There are also no range checks for B.W and Bcc.W, I have a follow up patch that adds checks for these that I'll post shortly.

[efriedma]

I'd like to see this rebased on top of https://reviews.llvm.org/D44928, I think.

Not that it's really related to this patch, but do we have some way to tell the linker whether a Thumb call supports a 4MB or 16MB offset?

[peter.smith]

Thanks for the comments. I've rebased on top of D44928, only change to the source is STI. goes to STI->. I've added .arch directives to the test to check that we can switch and keep the correct check. By the way I'd be really grateful if you could accept D44928, I've been threatening to commit it unless there are any objections, but I'd feel better if the review was accepted.

On the linker side. From the BuildAttributes, specifically Tag_CPU_arch, a static linker can work out at the object level what a file was compiled for. In general linkers follow the combination rules in the ABI so that if at least one object contains Tag_CPU_arch with an architecture that supports the longer branch range, then the linker will use the longer branch range. This model doesn't work so well when there are different attributes in different functions or sections, finer grained attributes are defined in the ABI but are deprecated/optional as most linkers don't support them. However, in the most common case, where the object architecture is lower than any local change to a higher architecture for runtime selection, the linker should use the shorter range for any relocation which is at least safe.

[fhahn]

Thanks Peter, LGTM. It's probably best to wait with committing this a bit in case Eli has any more comments.

[peter.smith]

Thanks for the review, the code here is rebased on top of D44928 (not landed yet) which passes STI in as a pointer. The existing code has STI as a member (const reference) so I'd need to change STI-> to STI. but nothing else would change.

[efriedma]

lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp

529

I think we also need to check for HasV6MOps here.

[peter.smith]

Thanks for the comments I've updated the diff to show:

• No longer rebased on D44928
• We now allow architecture v6m to have the long range BL (I checked in the Arm ARM and this is true for v6m)
• Test updated for v6m
• Removed extra tests for .arch switching as these are dependent on D44928 landing.

[efriedma]

LGTM

lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp

case ARM::fixup_t2_condbranch: {
     out |= (Value & 0x40000) >> 7; // J2 bit
     out |= (Value & 0x20000) >> 4; // J1 bit
     out |= (Value & 0x1F800) << 5; // imm6 field
     out |= (Value & 0x007FF);      // imm11 field
 
     return swapHalfWords(out, IsLittleEndian);
   }
   case ARM::fixup_arm_thumb_bl: {
-    // FIXME: We get both thumb1 and thumb2 in here, so we can only check for
-    // the less strict thumb2 value.
-    if (!isInt<26>(Value - 4)) {
+    assert(STI != nullptr);
+    if (!isInt<25>(Value - 4) ||
+        (!STI->getFeatureBits()[ARM::FeatureThumb2] &&
+         !STI->getFeatureBits()[ARM::HasV8MBaselineOps] &&
+         !isInt<23>(Value - 4))) {
       Ctx.reportError(Fixup.getLoc(), "Relocation out of range");
       return 0;
     }
 
     // The value doesn't encode the low bit (always zero) and is offset by
     // four. The 32-bit immediate value is encoded as
     //   imm32 = SignExtend(S:I1:I2:imm10:imm11:0)
     // where I1 = NOT(J1 ^ S) and I2 = NOT(J2 ^ S).

test/MC/ARM/thumb-branches.s

-@ RUN: not llvm-mc %s -triple thumbv5-linux-gnueabi -filetype=obj -o /dev/null 2>&1 | FileCheck %s
+@ RUN: not llvm-mc %s -triple thumbv7-linux-gnueabi -filetype=obj -o /dev/null 2>&1 | FileCheck %s
+@ RUN: not llvm-mc %s -triple thumbv8-m.baseline-none-eabi -filetype=obj -o /dev/null 2>&1 | FileCheck %s
+@ RUN: not llvm-mc %s -triple thumbv8-m.mainline-none-eabi -filetype=obj -o /dev/null 2>&1 | FileCheck %s
+@ RUN: not llvm-mc %s -triple thumbv5-linux-gnueabi -filetype=obj -o /dev/null 2>&1 | FileCheck -check-prefix=CHECKSHORT %s
+@ RUN: not llvm-mc %s -triple thumbv6-m-none-eabi -filetype=obj -o /dev/null 2>&1 | FileCheck -check-prefix=CHECKSHORT %s
+
+// Thumb BL has range +- 4 Megabytes if CPU does not support Thumb2 or does not
+// have v8-M baseline ops, it is +- 16 Megabytes otherwise.
 
         .code 16
+        bl      shortend
+        .space 0x3fffff
+shortend:
+// CHECKSHORT-NOT: error
+// CHECKSHORT: [[@LINE+1]]:{{[0-9]}}: error: Relocation out of range
+        bl      shortend2
+        .space 0x400000
+shortend2:
 
+// CHECKSHORT: [[@LINE+1]]:{{[0-9]}}: error: Relocation out of range
         bl      end
-        .space 0x1ffffff
+        .space 0xffffff
 end:
-
         bl      end2
-        .space 0x1ffffff
+        .space 0xffffff
         .global end2
 end2:
 
         bl      end3
-        .space 0x2000000
+        .space 0x1000000
         .global end3
 end3:
 
 // CHECK-NOT: error
+// CHECKSHORT-NOT: error
+// CHECKSHORT: [[@LINE+2]]:{{[0-9]}}: error: Relocation out of range
 // CHECK: [[@LINE+1]]:{{[0-9]}}: error: Relocation out of range
         bl      end4
-// CHECK-NOT: error
-        .space 0x2000000
+        .space 0x1000000
 end4:
 
+shortstart1:
+        .space 0x3ffffc
+        bl shortstart1
+
+shortstart2:
+        .space 0x400000
+// CHECKSHORT: [[@LINE+1]]:{{[0-9]}}: error: Relocation out of range
+        bl shortstart2
+
 start1:
-        .space 0x1fffffc
+        .space 0xfffffc
+// CHECKSHORT: [[@LINE+1]]:{{[0-9]}}: error: Relocation out of range
         bl start1
 
         .global start2
 start2:
-        .space 0x1fffffc
+        .space 0xfffffc
         bl start2
 
         .global start3
 start3:
-        .space 0x1fffffd
+        .space 0xfffffd
         bl start3
 
+// CHECK-NOT: error
 start4:
-        .space 0x1fffffd
-// CHECK: [[@LINE+1]]:{{[0-9]}}: error: Relocation out of range
+        .space 0xfffffd
+// CHECK: [[@LINE+2]]:{{[0-9]}}: error: Relocation out of range
+// CHECKSHORT: [[@LINE+1]]:{{[0-9]}}: error: Relocation out of range
         bl start4
+
+// Check that we can switch architecture and get the appropriate range check
+start5:
+        .space 0x400000
+        .arch armv5t
+// CHECK: [[@LINE+2]]:{{[0-9]}}: error: Relocation out of range
+// CHECKSHORT: [[@LINE+1]]:{{[0-9]}}: error: Relocation out of range
+        bl start5
+
+start6:
+        .space 0x400000
+        .arch armv7a
+// CHECK-NOT: error
+// CHECKSHORT-NOT: error
+        bl start6