[ARM, Asm] Harden GNU LDRD/STRD aliases against invalid inputs

[olista01]

Summary

Previously, the code that implemented the GNU assembler aliases for the
LDRD and STRD instructions (where the second register is omitted)
assumed that the input was a valid instruction. This caused assertion
failures for every example in ldrd-strd-gnu-bad-inst.s.

This improves this code so that it bails out if the instruction is not
in the expected format, the check bails out, and the asm parser is run
on the unmodified instruction.

It also relaxes the alias on thumb targets, so that unaligned pairs of
registers can be used. The restriction that Rt must be even-numbered
only applies to the ARM versions of these instructions.

[rengolin]

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

5791

These were asserts, now they're returns... I wonder if that's going to allow some weird cases. The assembler is not straight forward enough to not have weird matches going through behind our backs, and a future commit can mix this one up.

test/MC/ARM/ldrd-strd-gnu-sp.s

11

No note that this is valid in v8?

test/MC/ARM/ldrd-strd-gnu-thumb-bad-regs.s

5

hum, I was expecting something a bit more explanatory here...

[olista01]

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

5791

The idea is that, if the instruction doesn't match what we expect, we just don't change it, and let the asm parser report whatever error it would for the instruction as written.

test/MC/ARM/ldrd-strd-gnu-sp.s

11

Since the instruction doesn't get transformed by fixupGNULDRDAlias, it has a missing operand compared to what the table-generated matcher expects, which isn't something we have a diagnostic for.

We could report a diagnostic from fixupGNULDRDAlias, but that would add a lot of complexity, because we'd want to report at least 2 notes: "requires V8", and "Rt must be in [r0,r11]". I'm not sure how commonly these aliases are used (I'm not sure why you'd want an instruction to implicitly read/write another register you didn't mention), so I didn't think that would be worth it.

What I would like to add (I've not tried this yet though) is different diagnostics for "unknown mnemonic" and "known mnemonic with invalid operands", which would at least give us something here.

test/MC/ARM/ldrd-strd-gnu-thumb-bad-regs.s

5

Same as above.

[rengolin]

Ok, LGTM. Thanks!

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

5791

Right, makes sense.

test/MC/ARM/ldrd-strd-gnu-sp.s

11

Right, I agree the mnemonic messages are a better way out of this mess.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

#include "ARMGenAsmMatcher.inc"
   void cvtThumbMultiply(MCInst &Inst, const OperandVector &);
   void cvtThumbBranches(MCInst &Inst, const OperandVector &);
 
   bool validateInstruction(MCInst &Inst, const OperandVector &Ops);
   bool processInstruction(MCInst &Inst, const OperandVector &Ops, MCStreamer &Out);
   bool shouldOmitCCOutOperand(StringRef Mnemonic, OperandVector &Operands);
   bool shouldOmitPredicateOperand(StringRef Mnemonic, OperandVector &Operands);
   bool isITBlockTerminator(MCInst &Inst) const;
+  void fixupGNULDRDAlias(StringRef Mnemonic, OperandVector &Operands);
 
 public:
   enum ARMMatchResultTy {
     Match_RequiresITBlock = FIRST_TARGET_MATCH_RESULT_TY,
     Match_RequiresNotITBlock,
     Match_RequiresV6,
     Match_RequiresThumb2,
     Match_RequiresV8,
if (AddressingMode == "ia" || AddressingMode == "db" ||
       AcceptDoublePrecisionOnly = Inst[6] == 'd' || Inst[6] == 'x';
       return true;
     }
   }
 
   return false;
 }
 
+// The GNU assembler has aliases of ldrd and strd with the second register
+// omitted. We don't have a way to do that in tablegen, so fix it up here.
+//
+// We have to be careful to not emit an invalid Rt2 here, because the rest of
+// the assmebly parser could then generate confusing diagnostics refering to
+// it. If we do find anything that prevents us from doing the transformation we
+// bail out, and let the assembly parser report an error on the instruction as
+// it is written.
+void ARMAsmParser::fixupGNULDRDAlias(StringRef Mnemonic,
+                                     OperandVector &Operands) {
+  if (Mnemonic != "ldrd" && Mnemonic != "strd")
+    return;
+  if (Operands.size() < 4)
+    return;
+
+  ARMOperand &Op2 = static_cast<ARMOperand &>(*Operands[2]);
+  ARMOperand &Op3 = static_cast<ARMOperand &>(*Operands[3]);
+
+  if (!Op2.isReg())
+    return;
+  if (!Op3.isMem())
+    return;
+
+  const MCRegisterClass &GPR = MRI->getRegClass(ARM::GPRRegClassID);
+  if (!GPR.contains(Op2.getReg()))
+    return;
+
+  unsigned RtEncoding = MRI->getEncodingValue(Op2.getReg());
+  if (!isThumb() && (RtEncoding & 1)) {
+    // In ARM mode, the registers must be from an aligned pair, this
+    // restriction does not apply in Thumb mode.
+    return;
+  }
+  if (Op2.getReg() == ARM::PC)
+    return;
+  unsigned PairedReg = GPR.getRegister(RtEncoding + 1);
+  if (!PairedReg || PairedReg == ARM::PC ||
+      (PairedReg == ARM::SP && !hasV8Ops()))
+    return;
+
+  Operands.insert(
+      Operands.begin() + 3,
+      ARMOperand::CreateReg(PairedReg, Op2.getStartLoc(), Op2.getEndLoc()));
+  return;
+}
+
 /// Parse an arm instruction mnemonic followed by its operands.
 bool ARMAsmParser::ParseInstruction(ParseInstructionInfo &Info, StringRef Name,
                                     SMLoc NameLoc, OperandVector &Operands) {
   MCAsmParser &Parser = getParser();
   // FIXME: Can this be done via tablegen in some fashion?
   bool RequireVFPRegisterListCheck;
   bool AcceptSinglePrecisionOnly;
   bool AcceptDoublePrecisionOnly;
if (Op1.isReg() && Op2.isReg() && MRC.contains(Op1.getReg()) &&
       unsigned NewReg = MRI->getMatchingSuperReg(Reg1, ARM::gsub_0,
           &(MRI->getRegClass(ARM::GPRPairRegClassID)));
       Operands[Idx] =
           ARMOperand::CreateReg(NewReg, Op1.getStartLoc(), Op2.getEndLoc());
       Operands.erase(Operands.begin() + Idx + 1);
     }
   }
 
-  // GNU Assembler extension (compatibility)
-  if ((Mnemonic == "ldrd" || Mnemonic == "strd")) {
-    ARMOperand &Op2 = static_cast<ARMOperand &>(*Operands[2]);
-    ARMOperand &Op3 = static_cast<ARMOperand &>(*Operands[3]);
-    if (Op3.isMem()) {
-      assert(Op2.isReg() && "expected register argument");
-
-      unsigned SuperReg = MRI->getMatchingSuperReg(
-          Op2.getReg(), ARM::gsub_0, &MRI->getRegClass(ARM::GPRPairRegClassID));
-
-      assert(SuperReg && "expected register pair");
-
-      unsigned PairedReg = MRI->getSubReg(SuperReg, ARM::gsub_1);
-
-      Operands.insert(
-          Operands.begin() + 3,
-          ARMOperand::CreateReg(PairedReg, Op2.getStartLoc(), Op2.getEndLoc()));
-    }
-  }
+  // GNU Assembler extension (compatibility).
+  fixupGNULDRDAlias(Mnemonic, Operands);
 
   // FIXME: As said above, this is all a pretty gross hack.  This instruction
   // does not fit with other "subs" and tblgen.
   // Adjust operands of B9.3.19 SUBS PC, LR, #imm (Thumb2) system instruction
   // so the Mnemonic is the original name "subs" and delete the predicate
   // operand so it will match the table entry.
   if (isThumbTwo() && Mnemonic == "sub" && Operands.size() == 6 &&
       static_cast<ARMOperand &>(*Operands[3]).isReg() &&

test/MC/ARM/ldrd-strd-gnu-arm-bad-regs.s

+@ RUN: not llvm-mc -triple=armv7-linux-gnueabi %s 2>&1 | FileCheck %s
+
+.text
+.arm
+@ CHECK: error: invalid instruction
+@ CHECK:         ldrd    r12, [r0, #512]
+        ldrd    r12, [r0, #512]
+
+@ CHECK: error: invalid instruction
+@ CHECK:         strd    r12, [r0, #512]
+        strd    r12, [r0, #512]
+
+@ CHECK: error: invalid instruction
+@ CHECK:         ldrd    r1, [r0, #512]
+        ldrd    r1, [r0, #512]
+
+@ CHECK: error: invalid instruction
+@ CHECK:         strd    r1, [r0, #512]
+        strd    r1, [r0, #512]

test/MC/ARM/ldrd-strd-gnu-bad-inst.s

+@ RUN: not llvm-mc -triple=armv7-linux-gnueabi %s 2>&1 | FileCheck %s
+
+  .text
+  .thumb
+@ CHECK: error: invalid instruction
+  strd
+@ CHECK: error: invalid instruction
+  ldrd
+@ CHECK: error: invalid instruction
+  strd r0
+@ CHECK: error: invalid instruction
+  ldrd r0
+@ CHECK: error: invalid instruction
+  strd s0, [r0]
+@ CHECK: error: invalid instruction
+  ldrd s0, [r0]
+  .arm
+@ CHECK: error: invalid instruction
+  strd
+@ CHECK: error: invalid instruction
+  ldrd
+@ CHECK: error: invalid instruction
+  strd r0
+@ CHECK: error: invalid instruction
+  ldrd r0
+@ CHECK: error: invalid instruction
+  strd s0, [r0]
+@ CHECK: error: invalid instruction
+  ldrd s0, [r0]

test/MC/ARM/ldrd-strd-gnu-sp.s

 // PR19320
-// RUN: llvm-mc -triple=armv7-linux-gnueabi -show-encoding < %s | FileCheck %s
+// RUN: not llvm-mc -triple=armv7a-linux-gnueabi -show-encoding < %s 2>&1 | FileCheck %s --check-prefix=V7
+// RUN:     llvm-mc -triple=armv8a-linux-gnueabi -show-encoding < %s 2>&1 | FileCheck %s --check-prefix=V8
   .text
 
-// CHECK: ldrd  r12, sp, [r0, #32]      @ encoding: [0xd0,0xc2,0xc0,0xe1]
+// This tries to use the GNU ldrd/strd alias to create an ldrd/strd instruction
+// using the sp register. This is valid for V8, but not earlier architectures.
+
+  .arm
+
+// V7: error: invalid instruction
+// V8: ldrd    r12, sp, [r0, #32]      @ encoding: [0xd0,0xc2,0xc0,0xe1]
+        ldrd    r12, [r0, #32]
+
+// V7: error: invalid instruction
+// V8: strd    r12, sp, [r0, #32]      @ encoding: [0xf0,0xc2,0xc0,0xe1]
+        strd    r12, [r0, #32]
+
+  .thumb
+
+// V7: error: invalid instruction
+// V8: ldrd    r12, sp, [r0, #32]      @ encoding: [0xd0,0xe9,0x08,0xcd]
         ldrd    r12, [r0, #32]
 
-// CHECK: strd  r12, sp, [r0, #32]      @ encoding: [0xf0,0xc2,0xc0,0xe1]
+// V7: error: invalid instruction
+// V8: strd    r12, sp, [r0, #32]      @ encoding: [0xc0,0xe9,0x08,0xcd]
         strd    r12, [r0, #32]

test/MC/ARM/ldrd-strd-gnu-thumb-bad-regs.s

 @ RUN: not llvm-mc -triple=armv7-linux-gnueabi %s 2>&1 | FileCheck %s
 
-@ FIXME: These errors are inaccurate because the error is being reported on the
-@ implicit r13 operand added after r12.
-
 .text
 .thumb
-@ CHECK: error: operand must be a register in range [r0, r12] or r14
+@ CHECK: error: invalid instruction
 @ CHECK:         ldrd    r12, [r0, #512]
         ldrd    r12, [r0, #512]
 
-@ CHECK: error: operand must be a register in range [r0, r12] or r14
+@ CHECK: error: invalid instruction
 @ CHECK:         strd    r12, [r0, #512]
         strd    r12, [r0, #512]

test/MC/ARM/ldrd-strd-gnu-thumb.s

@ CHECK: ldrd  r0, r1, [r10, #512]     @ encoding: [0xda,0xe9,0x80,0x01]
         ldrd    r0, [r10, #512]
 
 @ CHECK: strd  r0, r1, [r10, #512]!    @ encoding: [0xea,0xe9,0x80,0x01]
 @ CHECK: strd  r0, r1, [r10], #512     @ encoding: [0xea,0xe8,0x80,0x01]
 @ CHECK: strd  r0, r1, [r10, #512]     @ encoding: [0xca,0xe9,0x80,0x01]
         strd    r0, [r10, #512]!
         strd    r0, [r10], #512
         strd    r0, [r10, #512]
+
+@ Rt is allowed to be odd for Thumb (but not ARM)
+@ CHECK: ldrd  r1, r2, [r10, #512]!    @ encoding: [0xfa,0xe9,0x80,0x12]
+@ CHECK: ldrd  r1, r2, [r10], #512     @ encoding: [0xfa,0xe8,0x80,0x12]
+@ CHECK: ldrd  r1, r2, [r10, #512]     @ encoding: [0xda,0xe9,0x80,0x12]
+        ldrd    r1, [r10, #512]!
+        ldrd    r1, [r10], #512
+        ldrd    r1, [r10, #512]
+
+@ CHECK: strd  r1, r2, [r10, #512]!    @ encoding: [0xea,0xe9,0x80,0x12]
+@ CHECK: strd  r1, r2, [r10], #512     @ encoding: [0xea,0xe8,0x80,0x12]
+@ CHECK: strd  r1, r2, [r10, #512]     @ encoding: [0xca,0xe9,0x80,0x12]
+        strd    r1, [r10, #512]!
+        strd    r1, [r10], #512
+        strd    r1, [r10, #512]