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

[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.
The check are done in the various AsmOperandClass predicates, which seems simple and as the right place to do it, but
the error messages aren't that good as in the alternative patch here: https://reviews.llvm.org/D39909

[chill]

In D39909#924657, @chill wrote:

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.

Having said that, I still tried this approach to a few asm operand classes and the results look promising.

Anyway, I suggest separating the correctness check for general-purpose registers from a set of follow-up of patches, which
just improve the error message "invalid operand for instruction".

[chill]

Updated the asm operand diagnostic messages.

[olista01]

It looks like there are a lot of inaccuracies here still with regards to accepting r13/sp and r15/pc. Rejecting non-GPRs is obviously an improvement, but I don't think we should be adding diagnostics which give wrong information to the user.

Maybe the best course of action here would be to commit just the part restricting registers to GPR now, leaving the diagnostics as "invalid operand". We can then fix the operand classes precisely a few at a time, adding predicate functions and diagnostics that match.

lib/Target/ARM/ARMInstrInfo.td

608 ↗

(On Diff #123317)

I've been trying to standardise the grammar of these messages on "operand must be a <type> ..." rather than "<type> operand must be ...", see D36689.

609 ↗

(On Diff #123317)

Neither register can be r15, at least for ADD on v7A and v8A.

939 ↗

(On Diff #123317)

The index register can't be r15 (at least for ARM LDR on v7A).

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

1157 ↗

(On Diff #123317)

There is no code here checking for anything more specific than GPR (other than the TBB/TBH operands), but there are lots of instructions that cannot use sp or pc.

[chill]

lib/Target/ARM/ARMInstrInfo.td

608 ↗

(On Diff #123317)

The reason for this phrasing is that it makes the diagnostic more clear, for example, when assembling

add r3, r0, s1, lsl r6

the error message is like

addr.s:17:17: note: shifted register operand must use two registers in the range [r0, r15]
add r3, r0, s1, lsl r6
                ^

with the caret pointing to the shift amount, whereas it's the s1 register that needs to be fixed.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

1157 ↗

(On Diff #123317)

Right. So, I have to draw the line somewhere and currently it's at not allowing non-GPRs in address and shifted register operands, and updating the diagnostics to match the code (in that sense, I probably shouldn't have made the changes for TBB/TBH).

Perhaps we would like to reject syntaxes, which generate an encoding with unpredictable behavior, or perhaps it's not worth the trouble. To me it seems like a subject for further discussion and a follow-up patch(es).

[chill]

I'll be happy going forward without the diagnostics changes, though.

[rengolin]

I think this change goes against the recent trend of printing multiple diagnostics, instead of a huge dump that will certainly be inaccurate at least some times.

I agree with @olista01 that terse messages are better than wrong messages. If there is inaccuracies, let's fix them, but this patch is way beyond that.

[chill]

Reverted to the first variant of the patch.

[olista01]

This patch doesn't have full context (git diff -U999999), please use that in future as it makes reviewing easier.

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

1 ↗

(On Diff #124342)

The previous version of this patch also tested Thumb, and FP/SIMD loads and stores. The code change looks like it should still affect those (except for the diagnostic strings), so could those tests also be kept?

[chill]

The preceding version contained more tests as they were covering each affected AsmOperandClass. Without the error messages, the tests just cover the changed functions, i.e. adding a test for vld4.32 {d0[], d1[], d2[], d3[]}, [s7:64]
won't test more than the already tested isMem().
I'll re-upload the diff with more context.

[chill]

Updated the diff to include full context.

[chill]

Ping?

[chill]

Ping?

[chill]

Ping?

[olista01]

I approved this patch back in November.

[chandlerc]

In D39910#968198, @olista01 wrote:

I approved this patch back in November.

Just as an FYI, Phab won't send an email to let folks know about this unless you actually write something in the comment box.

When marking patches as approved, try to always write "LGTM" or something in the message.

[chill]

In D39910#968243, @chandlerc wrote:

In D39910#968198, @olista01 wrote:

I approved this patch back in November.

Just as an FYI, Phab won't send an email to let folks know about this unless you actually write something in the comment box.

When marking patches as approved, try to always write "LGTM" or something in the message.

chandlerc, thanks. I got all the emails, and was well aware about the status of the review at all time, just wasn't sure what is the proper way to proceed when having conflicting reviews.

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

public:
 
   bool isReg() const override { return Kind == k_Register; }
   bool isRegList() const { return Kind == k_RegisterList; }
   bool isDPRRegList() const { return Kind == k_DPRRegisterList; }
   bool isSPRRegList() const { return Kind == k_SPRRegisterList; }
   bool isToken() const override { return Kind == k_Token; }
   bool isMemBarrierOpt() const { return Kind == k_MemBarrierOpt; }
   bool isInstSyncBarrierOpt() const { return Kind == k_InstSyncBarrierOpt; }
-  bool isMem() const override { return Kind == k_Memory; }
+  bool isMem() const override {
+    if (Kind != k_Memory)
+      return false;
+    if (Memory.BaseRegNum &&
+        !ARMMCRegisterClasses[ARM::GPRRegClassID].contains(Memory.BaseRegNum))
+      return false;
+    if (Memory.OffsetRegNum &&
+        !ARMMCRegisterClasses[ARM::GPRRegClassID].contains(Memory.OffsetRegNum))
+      return false;
+    return true;
+  }
   bool isShifterImm() const { return Kind == k_ShifterImmediate; }
-  bool isRegShiftedReg() const { return Kind == k_ShiftedRegister; }
-  bool isRegShiftedImm() const { return Kind == k_ShiftedImmediate; }
+  bool isRegShiftedReg() const {
+    return Kind == k_ShiftedRegister &&
+           ARMMCRegisterClasses[ARM::GPRRegClassID].contains(
+               RegShiftedReg.SrcReg) &&
+           ARMMCRegisterClasses[ARM::GPRRegClassID].contains(
+               RegShiftedReg.ShiftReg);
+  }
+  bool isRegShiftedImm() const {
+    return Kind == k_ShiftedImmediate &&
+           ARMMCRegisterClasses[ARM::GPRRegClassID].contains(
+               RegShiftedImm.SrcReg);
+  }
   bool isRotImm() const { return Kind == k_RotateImmediate; }
   bool isModImm() const { return Kind == k_ModifiedImmediate; }
 
   bool isModImmNot() const {
     if (!isImm()) return false;
     const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(getImm());
     if (!CE) return false;
     int64_t Value = CE->getValue();
bool isThumbModImmNeg8_255() const {
     const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(getImm());
     if (!CE) return false;
     int32_t Value = -(int32_t)CE->getValue();
     return 7 < Value && Value < 256;
   }
 
   bool isConstantPoolImm() const { return Kind == k_ConstantPoolImmediate; }
   bool isBitfield() const { return Kind == k_BitfieldDescriptor; }
-  bool isPostIdxRegShifted() const { return Kind == k_PostIndexRegister; }
+  bool isPostIdxRegShifted() const {
+    return Kind == k_PostIndexRegister &&
+           ARMMCRegisterClasses[ARM::GPRRegClassID].contains(PostIdxReg.RegNum);
+  }
   bool isPostIdxReg() const {
-    return Kind == k_PostIndexRegister && PostIdxReg.ShiftTy ==ARM_AM::no_shift;
+    return isPostIdxRegShifted() && PostIdxReg.ShiftTy == ARM_AM::no_shift;
   }
   bool isMemNoOffset(bool alignOK = false, unsigned Alignment = 0) const {
     if (!isMem())
       return false;
     // No offset of any kind.
     return Memory.OffsetRegNum == 0 && Memory.OffsetImm == nullptr &&
      (alignOK || Memory.Alignment == Alignment);
   }
bool isAddrMode3() const {
     int64_t Val = Memory.OffsetImm->getValue();
     // The #-0 offset is encoded as std::numeric_limits<int32_t>::min(), and we
     // have to check for this too.
     return (Val > -256 && Val < 256) ||
            Val == std::numeric_limits<int32_t>::min();
   }
 
   bool isAM3Offset() const {
-    if (Kind != k_Immediate && Kind != k_PostIndexRegister)
+    if (isPostIdxReg())
+      return true;
+    if (!isImm())
       return false;
-    if (Kind == k_PostIndexRegister)
-      return PostIdxReg.ShiftTy == ARM_AM::no_shift;
     // Immediate offset in range [-255, 255].
     const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(getImm());
     if (!CE) return false;
     int64_t Val = CE->getValue();
     // Special case, #-0 is std::numeric_limits<int32_t>::min().
     return (Val > -256 && Val < 256) ||
            Val == std::numeric_limits<int32_t>::min();
   }

llvm/trunk/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]]{{.*}}error: invalid operand for instruction
+
+ldr r4, [d2, #12]
+@ CHECK: [[@LINE-1]]{{.*}}error: invalid operand for instruction
+
+ldr r4, [q3, #12]
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+ldr r4, [cpsr, #12]
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+ldr r4, [r1, s12]
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+ldr r4, [r1, d12]
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+ldr r4, [r1, q12]
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+ldr r4, [r1, cpsr]
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+ldr r4, [r3], s12
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+ldr r4, [r3], d12
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+ldr r4, [r3], q12
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+ldr r4, [r3], cpsr
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+add r3, r0, s1, lsl #2
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+add r3, r0, d1, lsl #2
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+add r3, r0, q1, lsl #2
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+add r3, r0, cpsr, lsl #2
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+add r3, r0, r1, lsl s6
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+add r3, r0, r1, lsl d6
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+add r3, r0, r1, lsl q6
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+add r3, r0, r1, lsl cpsr
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+ldrd r2, r3, [s4]
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+ldrd r2, r3, [r4, s5]
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction
+ldrd r2, r3, [r4], s5
+@ CHECK: [[@LINE-1]]{{.*}} invalid operand for instruction