[Thumb] Validate branch target for CBZ/CBNZ instructions.

[prakhar]

Summary

The assembler currently does not check the branch target for CBZ/CBNZ
instructions, which only permit branching forwards with a positive offset. This
adds validation for the branch target to ensure negative PC-relative offsets are
not encoded into the instruction, whether specified as a literal or as an
assembler symbol.

[olista01]

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

863

Why don't you use isUnsignedOffset, which would also check the upper limit and alignment?

lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp

582

We should also check the upper bound and alignment here.

[prakhar]

Use isUnsignedOffset() directly and add upper bound checking with tests.

[olista01]

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

6699

These instructions accept a 6-bit immediate (shifted by one bit), not 5-bit. This should allow all even numbers in the range 0 to 126 (inclusive at both ends).

lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp

582

We should also be checking that the low bit is clear (and see my comment above about the immediate being 6 bits).

test/MC/ARM/thumb-diagnostics.s

241

It would be better to test as close to the bounds as possible, and add some tests for valid immediates close to the boundary. I think these should cover it:
-2 (invalid)
0 (lowest valid)
17 (invalid due to alignment)
126 (highest valid)
128 (invalid)

[prakhar]

Immediate operand is 6 bits and should have cleared LSB.

[olista01]

LGTM

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

public:
   bool isThumbBranchTarget() const {
     if (!isImm()) return false;
 
     if (const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(getImm()))
       return CE->getValue() % 2 == 0;
     return true;
   }
 
+  bool isPositiveOffset() const {
+    if (!isImm()) return false;
+    const MCExpr *ref = getImm();
+
+    if (ref->getKind() == MCExpr::Constant)
+      return dyn_cast<MCConstantExpr>(ref)->getValue() > 0;
+
+    return true;
+  }
+
   // checks whether this operand is an unsigned offset which fits is a field
   // of specified width and scaled by a specific number of bits
   template<unsigned width, unsigned scale>
   bool isUnsignedOffset() const {
     if (!isImm()) return false;
     if (isa<MCSymbolRefExpr>(Imm.Val)) return true;
     if (const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(Imm.Val)) {
       int64_t Val = CE->getValue();
if (!static_cast<ARMOperand &>(*Operands[2]).isSignedOffset<8, 1>())
       return Error(Operands[2]->getStartLoc(), "branch target out of range");
     break;
   case ARM::t2Bcc: {
     int Op = (Operands[2]->isImm()) ? 2 : 3;
     if (!static_cast<ARMOperand &>(*Operands[Op]).isSignedOffset<20, 1>())
       return Error(Operands[Op]->getStartLoc(), "branch target out of range");
     break;
   }
+  case ARM::tCBZ:
+  case ARM::tCBNZ: {
+    if (!static_cast<ARMOperand &>(*Operands[2]).isPositiveOffset())
+      return Error(Operands[2]->getStartLoc(), "branch target out of range");
+    break;
+  }
   case ARM::MOVi16:
   case ARM::t2MOVi16:
   case ARM::t2MOVTi16:
     {
     // We want to avoid misleadingly allowing something like "mov r0, <symbol>"
     // especially when we turn it into a movw and the expression <symbol> does
     // not have a :lower16: or :upper16 as part of the expression.  We don't
     // want the behavior of silently truncating, which can be unexpected and

lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp

if (Ctx && !STI->getFeatureBits()[ARM::FeatureThumb2] && IsResolved) {
       if (FixupDiagnostic) {
         Ctx->reportError(Fixup.getLoc(), FixupDiagnostic);
         return 0;
       }
     }
     // Offset by 4, and don't encode the low two bits.
     return ((Value - 4) >> 2) & 0xff;
   case ARM::fixup_arm_thumb_cb: {
+    // CB instructions cannot branch backwards with a negative offset
+    if (Ctx && (int64_t)Value < 0) {
+      Ctx->reportError(Fixup.getLoc(), "out of range pc-relative fixup value");
+      return 0;
+    }
     // Offset by 4 and don't encode the lower bit, which is always 0.
     // FIXME: diagnose if no Thumb2
     uint32_t Binary = (Value - 4) >> 1;
     return ((Binary & 0x20) << 4) | ((Binary & 0x1f) << 3);
   }
   case ARM::fixup_arm_thumb_br:
     // Offset by 4 and don't encode the lower bit, which is always 0.
     if (Ctx && !STI->getFeatureBits()[ARM::FeatureThumb2] &&

test/MC/ARM/thumb-cb-negative-offsets.s

+@ RUN: not llvm-mc -triple thumbv7m-none-eabi      -filetype=obj -o /dev/null %s 2>&1 | FileCheck %s
+@ RUN: not llvm-mc -triple thumbv8m.base-none-eabi -filetype=obj -o /dev/null %s 2>&1 | FileCheck %s
+
+label0:
+  .word 4
+
+@ CHECK: out of range pc-relative fixup value
+  cbz r0, label0
+@ CHECK: out of range pc-relative fixup value
+  cbnz r0, label0

test/MC/ARM/thumb-diagnostics.s

 @ CHECK-ERRORS: error: branch target out of range
 @ CHECK-ERRORS: error: branch target out of range
 @ CHECK-ERRORS: error: branch target out of range
 @ CHECK-ERRORS: error: branch target out of range
 @ CHECK-ERRORS: error: branch target out of range
 @ CHECK-ERRORS: error: branch target out of range
 
 @------------------------------------------------------------------------------
+@ CBZ/CBNZ - out of range immediates for branches
+@------------------------------------------------------------------------------
+
+        cbz    r0, #-4
+        cbnz   r0, #-8
+
+@ CHECK-ERRORS-V7M: error: branch target out of range
+@ CHECK-ERRORS-V7M: error: branch target out of range
+@ CHECK-ERRORS-V8: error: branch target out of range
+@ CHECK-ERRORS-V8: error: branch target out of range
+
+@------------------------------------------------------------------------------
 @ SEV/WFE/WFI/YIELD - are not supported pre v6M or v6T2
 @------------------------------------------------------------------------------
         sev
         wfe
         wfi
         yield
 
 @ CHECK-ERRORS: error: instruction requires: armv6m or armv6t2