[ARM] Issue an eror when non-general-purpose registers used in address operands

[chill]

Summary

Currently the assembler will happily assemble, e.g. ldr r0, [s0, #12] and similar.
This patch add checks that only general-purpose registers are used in address operands, shifted registers, and shift amounts.
This is done in the asm parser, as it results in better error messages, compared do doing the check in the various AsmOperandClass predicates.

[chill]

Less preferable (IMHO) alternative here: https://reviews.llvm.org/D39910

[olista01]

Hi Momchil,

I think your alternative patch is actually the right way to go here. The problem with doing this in ARMAsmParser::parseMemory and friends is that we can't reject SP and PC for instructions where they are not valid. Doing that here would require moving a lot more instruction-specific logic into these functions, which isn't going to scale well.

I have some WIP patches that do something similar to D39910, and also expand the diagnostics. These patches aren't complete (need to cover more operand types and more tests), but I'll send you a copy of them.

[chill]

Correct me if I'm wrong ...

For me it seems that the approach of returning just true/false from the asm operand class predicates, while
good for weeding out invalid instructions, isn't that great for diagnostics. The operand may be invalid for
a variety of reasons, but we have only one diagnostics string, which can at best list all the possible constraints
and let the user figure out by themselves what's wrong.

For example, assembling:

vld1.16 {d0}, [s0:64]

with llvm-mc -triple=armv7a-eabi gives error: alignment must be 64 or omitted, and if we need to improve
this message, we'd have to list the constrain that the register must be GPR. And do that for all/lot-of the other
AsmOperandClassses, which would create a huge number of repetetetetitions of the constraint "X must be a GPR".
If we instead diagnose usage of non-GPR registers in the parser, and leave checking more specific constraints
(like "X must not be PC" or "X must be SP") to asm operand class predicates, some diagnostics will be more
precise and the rest will require less effort by the user, because that'd list a comparatively small number of
constraints to manually check.

[chill]

So, this is now suspended in favor of https://reviews.llvm.org/D39910

[chill]

Superseded by https://reviews.llvm.org/D39910

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

class ARMAsmParser : public MCTargetAsmParser {
   }
 
   bool validatetLDMRegList(const MCInst &Inst, const OperandVector &Operands,
                            unsigned ListNo, bool IsARPop = false);
   bool validatetSTMRegList(const MCInst &Inst, const OperandVector &Operands,
                            unsigned ListNo);
 
   int tryParseRegister();
+  int tryParseGPRegister();
   bool tryParseRegisterWithWriteBack(OperandVector &);
   int tryParseShiftRegister(OperandVector &);
   bool parseRegisterList(OperandVector &);
   bool parseMemory(OperandVector &);
   bool parseOperand(OperandVector &, StringRef Mnemonic);
   bool parsePrefix(ARMMCExpr::VariantKind &RefKind);
   bool parseMemRegOffsetShift(ARM_AM::ShiftOpc &ShiftType,
                               unsigned &ShiftAmount);
int ARMAsmParser::tryParseRegister() {
   if (hasD16() && RegNum >= ARM::D16 && RegNum <= ARM::D31)
     return -1;
 
   Parser.Lex(); // Eat identifier token.
 
   return RegNum;
 }
 
+// Try to parse a register used as base or offset in a memory operand. We need
+// to distinguish between not parsing a register at all and parsing a
+// non-general purpose register. In the former case we may try other parsing
+// alternatives.
+int ARMAsmParser::tryParseGPRegister() {
+  auto Reg = tryParseRegister();
+  if (Reg < 0)
+    return -1;
+  if (!ARMMCRegisterClasses[ARM::GPRRegClassID].contains(Reg))
+    return -Reg;
+  return Reg;
+}
+
 // Try to parse a shifter  (e.g., "lsl <amt>"). On success, return 0.
 // If a recoverable error occurs, return 1. If an irrecoverable error
 // occurs, return -1. An irrecoverable error is one where tokens have been
 // consumed in the process of trying to parse the shifter (i.e., when it is
 // indeed a shifter operand, but malformed).
 int ARMAsmParser::tryParseShiftRegister(OperandVector &Operands) {
   MCAsmParser &Parser = getParser();
   SMLoc S = Parser.getTok().getLoc();
int ARMAsmParser::tryParseShiftRegister(OperandVector &Operands) {
 
   Parser.Lex(); // Eat the operator.
 
   // The source register for the shift has already been added to the
   // operand list, so we need to pop it off and combine it into the shifted
   // register operand instead.
   std::unique_ptr<ARMOperand> PrevOp(
       (ARMOperand *)Operands.pop_back_val().release());
-  if (!PrevOp->isReg())
-    return Error(PrevOp->getStartLoc(), "shift must be of a register");
-  int SrcReg = PrevOp->getReg();
+  int SrcReg = PrevOp->isReg() ? PrevOp->getReg() : -1;
+  if (SrcReg < 0 || !ARMMCRegisterClasses[ARM::GPRRegClassID].contains(SrcReg))
+    return Error(Parser.getTok().getLoc(), "shift must be of general-purpose register");
 
   SMLoc EndLoc;
   int64_t Imm = 0;
   int ShiftReg = 0;
   if (ShiftTy == ARM_AM::rrx) {
     // RRX Doesn't have an explicit shift amount. The encoder expects
     // the shift register to be the same as the source register. Seems odd,
     // but OK.
if (Parser.getTok().is(AsmToken::Hash) ||
       }
       // shift by zero is a nop. Always send it through as lsl.
       // ('as' compatibility)
       if (Imm == 0)
         ShiftTy = ARM_AM::lsl;
     } else if (Parser.getTok().is(AsmToken::Identifier)) {
       SMLoc L = Parser.getTok().getLoc();
       EndLoc = Parser.getTok().getEndLoc();
-      ShiftReg = tryParseRegister();
-      if (ShiftReg == -1) {
-        Error(L, "expected immediate or register in shift operand");
+      ShiftReg = tryParseGPRegister();
+      if (ShiftReg < 0) {
+        Error(L, "expected immediate or general-purpose register in shift operand");
         return -1;
       }
     } else {
       Error(Parser.getTok().getLoc(),
-            "expected immediate or register in shift operand");
+            "expected immediate or general-purpose register in shift operand");
       return -1;
     }
   }
 
   if (ShiftReg && ShiftTy != ARM_AM::rrx)
     Operands.push_back(ARMOperand::CreateShiftedRegister(ShiftTy, SrcReg,
                                                          ShiftReg, Imm,
                                                          S, EndLoc));
if (Tok.is(AsmToken::Plus)) {
     haveEaten = true;
   } else if (Tok.is(AsmToken::Minus)) {
     Parser.Lex(); // Eat the '-' token.
     isAdd = false;
     haveEaten = true;
   }
 
   SMLoc E = Parser.getTok().getEndLoc();
-  int Reg = tryParseRegister();
-  if (Reg == -1) {
-    if (!haveEaten)
+  int Reg = tryParseGPRegister();
+  if (Reg < 0) {
+    if (Reg == -1 && !haveEaten)
       return MatchOperand_NoMatch;
-    Error(Parser.getTok().getLoc(), "register expected");
+    Error(Parser.getTok().getLoc(), "general-purpose register expected");
     return MatchOperand_ParseFail;
   }
 
   ARM_AM::ShiftOpc ShiftTy = ARM_AM::no_shift;
   unsigned ShiftImm = 0;
   if (Parser.getTok().is(AsmToken::Comma)) {
     Parser.Lex(); // Eat the ','.
     if (parseMemRegOffsetShift(ShiftTy, ShiftImm))
if (Tok.is(AsmToken::Plus)) {
     haveEaten = true;
   } else if (Tok.is(AsmToken::Minus)) {
     Parser.Lex(); // Eat the '-' token.
     isAdd = false;
     haveEaten = true;
   }
 
   Tok = Parser.getTok();
-  int Reg = tryParseRegister();
-  if (Reg == -1) {
-    if (!haveEaten)
+  int Reg = tryParseGPRegister();
+  if (Reg < 0) {
+    if (Reg == -1 && !haveEaten)
       return MatchOperand_NoMatch;
-    Error(Tok.getLoc(), "register expected");
+    Error(Tok.getLoc(), "general-purpose register expected");
     return MatchOperand_ParseFail;
   }
 
   Operands.push_back(ARMOperand::CreatePostIdxReg(Reg, isAdd, ARM_AM::no_shift,
                                                   0, S, Tok.getEndLoc()));
 
   return MatchOperand_Success;
 }
bool ARMAsmParser::parseMemory(OperandVector &Operands) {
   MCAsmParser &Parser = getParser();
   SMLoc S, E;
   if (Parser.getTok().isNot(AsmToken::LBrac))
     return TokError("Token is not a Left Bracket");
   S = Parser.getTok().getLoc();
   Parser.Lex(); // Eat left bracket token.
 
   const AsmToken &BaseRegTok = Parser.getTok();
-  int BaseRegNum = tryParseRegister();
-  if (BaseRegNum == -1)
-    return Error(BaseRegTok.getLoc(), "register expected");
+  int BaseRegNum = tryParseGPRegister();
+  if (BaseRegNum < 0)
+    return Error(BaseRegTok.getLoc(), "general-purpose register expected");
 
   // The next token must either be a comma, a colon or a closing bracket.
   const AsmToken &Tok = Parser.getTok();
   if (!Tok.is(AsmToken::Colon) && !Tok.is(AsmToken::Comma) &&
       !Tok.is(AsmToken::RBrac))
     return Error(Tok.getLoc(), "malformed memory operand");
 
   if (Tok.is(AsmToken::RBrac)) {
if (Parser.getTok().is(AsmToken::Minus)) {
     isNegative = true;
     Parser.Lex(); // Eat the '-'.
   } else if (Parser.getTok().is(AsmToken::Plus)) {
     // Nothing to do.
     Parser.Lex(); // Eat the '+'.
   }
 
   E = Parser.getTok().getLoc();
-  int OffsetRegNum = tryParseRegister();
-  if (OffsetRegNum == -1)
-    return Error(E, "register expected");
+  int OffsetRegNum = tryParseGPRegister();
+  if (OffsetRegNum < 0)
+    return Error(E, "general-purpose register expected");
 
   // If there's a shift operator, handle it.
   ARM_AM::ShiftOpc ShiftType = ARM_AM::no_shift;
   unsigned ShiftImm = 0;
   if (Parser.getTok().is(AsmToken::Comma)) {
     Parser.Lex(); // Eat the ','.
     if (parseMemRegOffsetShift(ShiftType, ShiftImm))
       return true;

test/MC/ARM/arm-reg-addr-errors.s

+@ RUN: not llvm-mc -triple=armv7a-eabi < %s 2>&1 | FileCheck %s
+
+ldr r4, [s1, #12]
+@ CHECK: :[[@LINE-1]]:{{.*}} general-purpose register expected
+
+ldr r4, [d2, #12]
+@ CHECK: :[[@LINE-1]]:{{.*}} general-purpose register expected
+
+ldr r4, [q3, #12]
+@ CHECK: :[[@LINE-1]]:{{.*}} general-purpose register expected
+
+ldr r4, [cpsr, #12]
+@ CHECK: :[[@LINE-1]]:{{.*}} general-purpose register expected
+
+ldr r4, [r1, s12]
+@ CHECK: :[[@LINE-1]]:{{.*}} general-purpose register expected
+
+ldr r4, [r1, d12]
+@ CHECK: :[[@LINE-1]]:{{.*}} general-purpose register expected
+
+ldr r4, [r1, q12]
+@ CHECK: :[[@LINE-1]]:{{.*}} general-purpose register expected
+
+ldr r4, [r1, cpsr]
+@ CHECK: :[[@LINE-1]]:{{.*}} general-purpose register expected
+
+ldr r4, [r3], s12
+@ CHECK: :[[@LINE-1]]:{{.*}} general-purpose register expected
+
+ldr r4, [r3], d12
+@ CHECK: :[[@LINE-1]]:{{.*}} general-purpose register expected
+
+ldr r4, [r3], q12
+@ CHECK: :[[@LINE-1]]:{{.*}} general-purpose register expected
+
+ldr r4, [r3], cpsr
+@ CHECK: :[[@LINE-1]]:{{.*}} general-purpose register expected
+
+add r3, r0, s1, lsl #2
+@ CHECK: :[[@LINE-1]]:{{.*}}  shift must be of general-purpose register
+
+add r3, r0, d1, lsl #2
+@ CHECK: :[[@LINE-1]]:{{.*}}  shift must be of general-purpose register
+
+add r3, r0, q1, lsl #2
+@ CHECK: :[[@LINE-1]]:{{.*}}  shift must be of general-purpose register
+
+add r3, r0, cpsr, lsl #2
+@ CHECK: :[[@LINE-1]]:{{.*}} shift must be of general-purpose register
+
+add r3, r0, r1, lsl s6
+@ CHECK: :[[@LINE-1]]:{{.*}} expected immediate or general-purpose register in shift operand
+
+add r3, r0, r1, lsl d6
+@ CHECK: :[[@LINE-1]]:{{.*}} expected immediate or general-purpose register in shift operand
+
+add r3, r0, r1, lsl q6
+@ CHECK: :[[@LINE-1]]:{{.*}} expected immediate or general-purpose register in shift operand
+
+add r3, r0, r1, lsl cpsr
+@ CHECK: :[[@LINE-1]]:{{.*}} expected immediate or general-purpose register in shift operand
+
+ldrd r2, r3, [s4]
+@ CHECK: :[[@LINE-1]]:{{.*}} general-purpose register expected
+
+ldrd r2, r3, [r4, d5]
+@ CHECK: :[[@LINE-1]]:{{.*}} general-purpose register expected
+
+ldrd r2, r3, [r4], q5
+@ CHECK: :[[@LINE-1]]:{{.*}} general-purpose register expected