[MC][ARM] Fix crash when assembling Thumb 'movs r0,#foo'.

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

@@ -8294,15 +8294,17 @@ bool ARMAsmParser::validateInstruction(MCInst &Inst,
     break;
   }
   case ARM::tADDi8: {
-    MCParsedAsmOperand &Op = *Operands[MnemonicOpsEndInd + 1];
+    int i = (Operands[MnemonicOpsEndInd + 1]->isImm()) ? MnemonicOpsEndInd + 1
+                                                       : MnemonicOpsEndInd + 2;
+    MCParsedAsmOperand &Op = *Operands[i];
     if (isARMMCExpr(Op) && !isThumbI8Relocation(Op))
       return Error(Op.getStartLoc(),
                    "Immediate expression for Thumb adds requires :lower0_7:,"
                    " :lower8_15:, :upper0_7: or :upper8_15:");
     break;
   }
   case ARM::tMOVi8: {
-    MCParsedAsmOperand &Op = *Operands[MnemonicOpsEndInd];
+    MCParsedAsmOperand &Op = *Operands[MnemonicOpsEndInd + 1];
     if (isARMMCExpr(Op) && !isThumbI8Relocation(Op))
       return Error(Op.getStartLoc(),
                    "Immediate expression for Thumb movs requires :lower0_7:,"
@@ -10709,14 +10711,26 @@ bool ARMAsmParser::processInstruction(MCInst &Inst,
     // the flags are compatible with the current IT status, use encoding T2
     // instead of T3. For compatibility with the system 'as'. Make sure the
     // wide encoding wasn't explicit.
-    if (Inst.getOperand(0).getReg() != Inst.getOperand(1).getReg() ||
-        !isARMLowRegister(Inst.getOperand(0).getReg()) ||
-        (Inst.getOperand(2).isImm() &&
-         (unsigned)Inst.getOperand(2).getImm() > 255) ||
-        Inst.getOperand(5).getReg() !=
-            (inITBlock() ? ARM::NoRegister : ARM::CPSR) ||
-        HasWideQualifier)
-      break;
+    if (HasWideQualifier)
+      break; // source code has asked for the 32-bit instruction
+    if (Inst.getOperand(0).getReg() != Inst.getOperand(1).getReg())
+      break; // tADDi8 can't take different input and output registers
+    if (!isARMLowRegister(Inst.getOperand(0).getReg()))
+      break; // high register that tADDi8 can't access
+    if (Inst.getOperand(5).getReg() !=
+        (inITBlock() ? ARM::NoRegister : ARM::CPSR))
+      break; // flag-modification would require overriding the IT state
+    if (Inst.getOperand(2).isImm()) {
+      if ((unsigned)Inst.getOperand(2).getImm() > 255)
+        break; // large immediate that tADDi8 can't contain
+    } else {
+      int i = (Operands[MnemonicOpsEndInd + 1]->isImm())
+                  ? MnemonicOpsEndInd + 1
+                  : MnemonicOpsEndInd + 2;
+      MCParsedAsmOperand &Op = *Operands[i];
+      if (isARMMCExpr(Op) && !isThumbI8Relocation(Op))
+        break; // a type of non-immediate that tADDi8 can't represent
+    }
     MCInst TmpInst;
     TmpInst.setOpcode(Inst.getOpcode() == ARM::t2ADDri ?
                       ARM::tADDi8 : ARM::tSUBi8);

llvm/test/MC/ARM/lower-upper-errors-2.s

@@ -0,0 +1,18 @@
+// RUN: not llvm-mc --triple thumbv7m -filetype=obj -o /dev/null %s 2>&1 | FileCheck %s
+
+// This test checks reporting of errors of the form "you should have
+// used :lower16: in this immediate field", when the errors are
+// discovered at the object-file output stage by checking the set of
+// available relocations.
+//
+// For errors that are reported earlier, when initially reading the
+// instructions, see lower-upper-errors.s.
+
+// CHECK: [[@LINE+1]]:1: error: unsupported relocation
+adds r0, r0, #foo
+
+// CHECK: [[@LINE+1]]:1: error: unsupported relocation
+add r9, r0, #foo
+
+// CHECK: [[@LINE+1]]:1: error: expected relocatable expression
+movs r11, :upper8_15:#foo

llvm/test/MC/ARM/lower-upper-errors.s

@@ -0,0 +1,51 @@
+// RUN: not llvm-mc --triple thumbv6m %s 2>&1 | FileCheck %s --check-prefixes=CHECK,THUMB1
+// RUN: not llvm-mc --triple thumbv7m %s 2>&1 | FileCheck %s --check-prefixes=CHECK,THUMB2
+
+// This test checks reporting of errors of the form "you should have
+// used :lower16: in this immediate field", during initial reading of
+// the source file.
+//
+// For errors that are reported at object-file output time, see
+// lower-upper-errors-2.s.
+
+// CHECK: :[[@LINE+1]]:10: error: Immediate expression for Thumb movs requires :lower0_7:, :lower8_15:, :upper0_7: or :upper8_15:
+movs r0, #foo
+
+// CHECK: :[[@LINE+1]]:10: error: Immediate expression for Thumb adds requires :lower0_7:, :lower8_15:, :upper0_7: or :upper8_15:
+adds r0, #foo
+
+// THUMB2: :[[@LINE+1]]:10: error: immediate expression for mov requires :lower16: or :upper16
+movw r0, #foo
+
+// THUMB2: :[[@LINE+1]]:10: error: immediate expression for mov requires :lower16: or :upper16
+movt r0, #foo
+
+// With a Thumb2 wide add instruction available, this case isn't an error
+// while reading the source file. It only causes a problem when an object
+// file is output, and it turns out there's no suitable relocation to ask
+// for the value of an external symbol to be turned into a Thumb shifted
+// immediate field. And in this case the other errors in this source file
+// cause assembly to terminate before reaching the object-file output stage
+// (even if a test run had had -filetype=obj).
+
+// THUMB1: :[[@LINE+2]]:14: error: Immediate expression for Thumb adds requires :lower0_7:, :lower8_15:, :upper0_7: or :upper8_15:
+// THUMB2-NOT: :[[@LINE+1]]:{{[0-9]+}}: error:
+adds r0, r0, #foo
+
+// Similarly for this version, which _must_ be the wide encoding due
+// to the use of a high register and the lack of flag-setting.
+
+// THUMB1: :[[@LINE+2]]:1: error: invalid instruction
+// THUMB2-NOT: :[[@LINE+1]]:{{[0-9]+}}: error:
+add r9, r0, #foo
+
+// CHECK: :[[@LINE+1]]:10: error: Immediate expression for Thumb movs requires :lower0_7:, :lower8_15:, :upper0_7: or :upper8_15:
+movs r0, :lower16:#foo
+
+// This is invalid in either architecture: in Thumb1 it can't use a
+// high register, and in Thumb2 it can't use :upper8_15:. But the
+// Thumb2 case won't cause an error until output.
+
+// THUMB1: :[[@LINE+2]]:1: error: invalid instruction
+// THUMB2-NOT: :[[@LINE+1]]:{{[0-9]+}}: error:
+movs r11, :upper8_15:#foo